chore: promote staging to staging-promote/c8f87537-24658689024 (2026-04-20 14:07 UTC)

crates/ironclaw_common/src/identity.rs

@@ -388,11 +388,11 @@ impl PartialEq<&str> for ExternalThreadId {
 
 /// Maximum length for an [`McpServerName`], measured in bytes.
 ///
-/// MCP server names are used as tool-name prefixes in LLM providers (which
+/// Alias of [`MAX_NAME_LEN`] to prevent drift between the two limits. MCP
+/// server names are used as tool-name prefixes in LLM providers (which
 /// typically require `^[a-zA-Z0-9_-]+$`), as components of secret-store keys
 /// (e.g. `mcp_<name>_access_token`), and as filesystem-adjacent identifiers.
-/// 64 bytes matches the shared `MAX_NAME_LEN` used for other identity names.
-pub const MAX_MCP_SERVER_NAME_LEN: usize = 64;
+pub const MAX_MCP_SERVER_NAME_LEN: usize = MAX_NAME_LEN;
 
 /// Why a candidate string is not a valid MCP server name.
 #[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)]

crates/ironclaw_common/src/lib.rs

@@ -11,7 +11,8 @@ pub use event::{
 };
 pub use identity::{
     CredentialName, ExtensionName, ExternalThreadId, ExternalThreadIdError, IdentityError,
-    MAX_EXTERNAL_THREAD_ID_LEN, MAX_NAME_LEN,
+    MAX_EXTERNAL_THREAD_ID_LEN, MAX_MCP_SERVER_NAME_LEN, MAX_NAME_LEN, McpServerName,
+    McpServerNameError,
 };
 pub use timezone::{ValidTimezone, deserialize_option_lenient};
 pub use util::truncate_preview;

src/bridge/router.rs

@@ -4616,7 +4616,12 @@ pub struct ProjectsOverviewResponse {
 /// Mission summary for list views.
 #[derive(Debug, Clone, serde::Serialize)]
 pub struct EngineMissionInfo {
-    pub id: String,
+    /// Typed mission identifier, carried through from the engine rather
+    /// than round-tripped to `String` at the adapter boundary. Serializes
+    /// transparently as a UUID string (via `MissionId`'s derived
+    /// `Serialize`), so the wire shape stays identical to the pre-newtype
+    /// DTO.
+    pub id: ironclaw_engine::MissionId,
     pub name: String,
     pub goal: String,
     pub status: String,
@@ -5220,7 +5225,7 @@ pub async fn list_engine_missions(
     Ok(missions
         .iter()
         .map(|m| EngineMissionInfo {
-            id: m.id.to_string(),
+            id: m.id,
             name: m.name.clone(),
             goal: m.goal.clone(),
             status: format!("{:?}", m.status),
@@ -5275,7 +5280,7 @@ pub async fn get_engine_mission(
 
     Ok(Some(EngineMissionDetail {
         info: EngineMissionInfo {
-            id: m.id.to_string(),
+            id: m.id,
             name: m.name.clone(),
             goal: m.goal.clone(),
             status: format!("{:?}", m.status),

src/tools/mcp/config.rs

@@ -6,6 +6,7 @@
 use std::collections::HashMap;
 use std::path::{Path, PathBuf};
 
+use ironclaw_common::{MAX_MCP_SERVER_NAME_LEN, McpServerName};
 use serde::{Deserialize, Serialize};
 use tokio::fs;
 
@@ -159,32 +160,17 @@ impl McpServerConfig {
 
     /// Validate the server configuration.
     pub fn validate(&self) -> Result<(), ConfigError> {
-        if self.name.is_empty() {
-            return Err(ConfigError::InvalidConfig {
-                reason: "Server name cannot be empty".to_string(),
-            });
-        }
-
-        // Allowlist: alphanumeric, dash, underscore.
-        // Rejects shell metacharacters (;|&`$), path separators (/\),
-        // dots (LLM providers require tool names match ^[a-zA-Z0-9_-]+$
-        // and server names are used as tool name prefixes), null bytes,
-        // spaces, and other dangerous characters that could cause injection
-        // when names are interpolated into secret keys, tool name prefixes,
-        // or provider tags.
-        if !self
-            .name
-            .chars()
-            .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_')
-        {
-            return Err(ConfigError::InvalidConfig {
-                reason: format!(
-                    "Server name '{}' contains invalid characters \
-                     (only alphanumeric, dash, underscore are allowed)",
-                    self.name
-                ),
-            });
-        }
+        // The server-name allowlist (non-empty, length cap, alphanumeric /
+        // dash / underscore only) now lives in `McpServerName::new` — see
+        // `ironclaw_common::identity`. Delegating here keeps the on-disk
+        // wire format a plain string (via `McpServerConfig.name: String`)
+        // while gating every construction path through the newtype's
+        // validation. The allowlist itself originated in #2400 as
+        // defence against shell-metacharacter injection when the name is
+        // interpolated into secret keys or tool-name prefixes.
+        McpServerName::new(&self.name).map_err(|e| ConfigError::InvalidConfig {
+            reason: e.to_string(),
+        })?;
 
         match self.effective_transport() {
             EffectiveTransport::Http => {
@@ -555,6 +541,42 @@ pub async fn load_mcp_servers() -> Result<McpServersFile, ConfigError> {
     load_mcp_servers_from(default_config_path()).await
 }
 
+/// In-place migrate a legacy server name whose length exceeds the
+/// [`MAX_MCP_SERVER_NAME_LEN`] cap introduced with the `McpServerName`
+/// newtype.
+///
+/// Before the newtype landed, `validate()` only enforced non-empty +
+/// `[A-Za-z0-9_-]` — there was no length bound. Delegating to
+/// `McpServerName::new` added a 64-byte cap that would otherwise silently
+/// drop legacy persisted configs via the `retain(...)` guard in the load
+/// paths. Truncating here keeps the entry usable while still bringing it
+/// within the new invariant on the next save.
+///
+/// Truncation is char-boundary safe: the loaded string may contain
+/// arbitrary UTF-8 even though the allowlist ultimately rejects non-ASCII,
+/// because this runs *before* `validate()`.
+fn migrate_legacy_server_name(name: &mut String) {
+    if name.len() <= MAX_MCP_SERVER_NAME_LEN {
+        return;
+    }
+    let original_len = name.len();
+    let mut end = MAX_MCP_SERVER_NAME_LEN;
+    while end > 0 && !name.is_char_boundary(end) {
+        end -= 1;
+    }
+    let truncated = name[..end].to_string();
+    tracing::warn!(
+        original_name = %name,
+        truncated_name = %truncated,
+        original_len,
+        new_len = end,
+        max = MAX_MCP_SERVER_NAME_LEN,
+        "Truncating legacy MCP server name that exceeded the {MAX_MCP_SERVER_NAME_LEN}-byte cap \
+         introduced with McpServerName; re-save to persist the shorter form"
+    );
+    *name = truncated;
+}
+
 /// Load MCP server configurations from a specific path.
 pub async fn load_mcp_servers_from(path: impl AsRef<Path>) -> Result<McpServersFile, ConfigError> {
     let path = path.as_ref();
@@ -570,7 +592,8 @@ pub async fn load_mcp_servers_from(path: impl AsRef<Path>) -> Result<McpServersF
     // warning instead of failing the entire config — this prevents legacy
     // names (e.g. "My Server") from disabling all MCP integrations after
     // an upgrade that tightened validation.
-    config.servers.retain(|server| {
+    config.servers.retain_mut(|server| {
+        migrate_legacy_server_name(&mut server.name);
         if let Err(e) = server.validate() {
             tracing::warn!(
                 server_name = %server.name,
@@ -666,7 +689,8 @@ pub async fn load_mcp_servers_from_db(
             // Validate every server on load. Invalid entries are skipped
             // with a warning to avoid breaking all MCP integrations when
             // legacy names don't pass tightened validation.
-            config.servers.retain(|server| {
+            config.servers.retain_mut(|server| {
+                migrate_legacy_server_name(&mut server.name);
                 if let Err(e) = server.validate() {
                     tracing::warn!(
                         server_name = %server.name,
@@ -1559,6 +1583,100 @@ mod tests {
         );
     }
 
+    /// Regression for PR nearai/ironclaw#2681 review comment 3110617080.
+    ///
+    /// Before the `McpServerName` newtype landed, `McpServerConfig::validate()`
+    /// had no length cap. Delegating validation to `McpServerName::new` added
+    /// a 64-byte cap, which would have silently dropped legacy persisted
+    /// configs via the `retain(...)` guard in `load_mcp_servers_from*`. The
+    /// load path now truncates overlong legacy names at a char boundary and
+    /// keeps the entry usable instead of dropping it.
+    #[tokio::test]
+    async fn test_load_truncates_legacy_overlong_server_names() {
+        let dir = tempdir().unwrap();
+        let path = dir.path().join("mcp-servers.json");
+
+        // 100-byte valid-char name — passes the old empty+allowlist check
+        // but exceeds MAX_MCP_SERVER_NAME_LEN (64).
+        let long_name = "a".repeat(100);
+        let mixed = serde_json::json!({
+            "servers": [
+                {
+                    "name": long_name,
+                    "url": "https://mcp.good.com",
+                    "enabled": true,
+                    "headers": {}
+                },
+                {
+                    "name": "short-server",
+                    "url": "https://mcp.short.com",
+                    "enabled": true,
+                    "headers": {}
+                }
+            ]
+        });
+        tokio::fs::write(&path, mixed.to_string()).await.unwrap();
+
+        let result = load_mcp_servers_from(&path).await.unwrap();
+        assert_eq!(
+            result.servers.len(),
+            2,
+            "overlong legacy name must be migrated in place, not silently dropped"
+        );
+        let migrated = result
+            .servers
+            .iter()
+            .find(|s| s.name.starts_with('a'))
+            .expect("long-name server retained after migration");
+        assert!(
+            migrated.name.len() <= MAX_MCP_SERVER_NAME_LEN,
+            "migrated name must be within the new cap, got {} bytes",
+            migrated.name.len()
+        );
+        assert_eq!(
+            migrated.name.len(),
+            MAX_MCP_SERVER_NAME_LEN,
+            "ASCII-only overlong name should truncate to exactly the cap"
+        );
+    }
+
+    /// Char-boundary safety: an overlong name whose byte 64 falls in the
+    /// middle of a multi-byte UTF-8 sequence must not panic and must land
+    /// on a valid char boundary. The truncated name will then typically
+    /// fail the allowlist (non-ASCII) and be dropped by `retain`, which
+    /// is the same end state as before this PR — the guarantee here is
+    /// *no panic*, not acceptance.
+    #[tokio::test]
+    async fn test_load_truncation_is_char_boundary_safe() {
+        let dir = tempdir().unwrap();
+        let path = dir.path().join("mcp-servers.json");
+
+        // 63 ASCII bytes + multi-byte character that straddles byte 64.
+        let mut name = "a".repeat(63);
+        name.push('é'); // 2 bytes; now 65 bytes total, byte 64 is mid-char
+        name.push('é'); // extend further to ensure we're over the cap
+        let payload = serde_json::json!({
+            "servers": [
+                {
+                    "name": name,
+                    "url": "https://mcp.good.com",
+                    "enabled": true,
+                    "headers": {}
+                }
+            ]
+        });
+        tokio::fs::write(&path, payload.to_string()).await.unwrap();
+
+        // Must not panic during load (would have with naive `&name[..64]`).
+        let result = load_mcp_servers_from(&path).await.unwrap();
+        for s in &result.servers {
+            assert!(
+                s.name.is_char_boundary(s.name.len()),
+                "truncated name must sit on a valid UTF-8 boundary"
+            );
+        }
+    }
+
     #[tokio::test]
     async fn test_load_skips_invalid_server_names() {
         let dir = tempdir().unwrap();

src/tools/mcp/factory.rs

@@ -5,6 +5,8 @@
 
 use std::sync::Arc;
 
+use ironclaw_common::McpServerName;
+
 use crate::secrets::SecretsStore;
 use crate::tools::mcp::config::{EffectiveTransport, McpServerConfig};
 use crate::tools::mcp::http_transport::HttpMcpTransport;
@@ -40,18 +42,37 @@ pub async fn create_client_from_config(
     server.name = server.name.replace('-', "_");
     let server_name = server.name.clone();
 
+    // Re-validate through `McpServerName::new` so a malformed name (e.g.
+    // a config row persisted before the #2400 allowlist tightened) fails
+    // fast at factory time instead of silently producing an MCP session
+    // keyed by an un-checked string. Capture the validated value and
+    // thread it through the hottest internal uses (transport constructors,
+    // process spawn, client factories below). The remaining `String`
+    // uses (e.g. `McpServerConfig.name` inside the moved `server`) will
+    // be migrated in the follow-up that switches `McpServerConfig.name`
+    // to the typed newtype.
+    // TODO(type-safety PR 4 of 4): thread `validated_name` through
+    // `McpServerConfig.name`, `McpClient::new_with_transport`, and
+    // `McpClient::new_authenticated` so the String clones below can go
+    // away entirely.
+    let validated_name =
+        McpServerName::new(&server_name).map_err(|e| McpFactoryError::InvalidConfig {
+            name: server_name.clone(),
+            reason: e.to_string(),
+        })?;
+
     match server.effective_transport() {
         EffectiveTransport::Stdio { command, args, env } => {
             let transport = process_manager
-                .spawn_stdio(&server_name, command, args.to_vec(), env.clone())
+                .spawn_stdio(validated_name.as_str(), command, args.to_vec(), env.clone())
                 .await
                 .map_err(|e| McpFactoryError::StdioSpawn {
                     name: server_name.clone(),
                     reason: e.to_string(),
                 })?;
 
             Ok(McpClient::new_with_transport(
-                &server_name,
+                validated_name.as_str(),
                 transport as Arc<dyn McpTransport>,
                 None,
                 secrets,
@@ -62,7 +83,7 @@ pub async fn create_client_from_config(
         #[cfg(unix)]
         EffectiveTransport::Unix { socket_path } => {
             let transport = crate::tools::mcp::unix_transport::UnixMcpTransport::connect(
-                &server_name,
+                validated_name.as_str(),
                 socket_path,
             )
             .await
@@ -72,7 +93,7 @@ pub async fn create_client_from_config(
             })?;
 
             Ok(McpClient::new_with_transport(
-                &server_name,
+                validated_name.as_str(),
                 Arc::new(transport) as Arc<dyn McpTransport>,
                 None,
                 secrets,
@@ -105,11 +126,11 @@ pub async fn create_client_from_config(
             // the client (via `with_session_manager`) is not enough — the
             // transport must know about it to read/write the header.
             let transport = Arc::new(
-                HttpMcpTransport::new(server.url.clone(), server_name.clone())
+                HttpMcpTransport::new(server.url.clone(), validated_name.as_str())
                     .with_session_manager(Arc::clone(session_manager)),
             );
             Ok(McpClient::new_with_transport(
-                server_name,
+                validated_name.as_str(),
                 transport,
                 Some(Arc::clone(session_manager)),
                 secrets,
@@ -378,8 +399,8 @@ mod tests {
         // Pre-create a session entry so that update_session_id has something to update.
         // In production, the MCP initialize handshake calls get_or_create before responses arrive.
         // Use the normalised server name (hyphens → underscores) that the factory applies.
-        let normalised_name = "session_test";
-        session_manager.get_or_create(normalised_name, &url).await;
+        let normalised_name = McpServerName::new("session_test").expect("valid");
+        session_manager.get_or_create(&normalised_name, &url).await;
 
         // Send a request through the client's transport to trigger session capture.
         use crate::tools::mcp::protocol::McpRequest;
@@ -397,7 +418,7 @@ mod tests {
             .expect("request should succeed");
 
         // Verify the session manager captured the session ID from the response.
-        let captured = session_manager.get_session_id(normalised_name).await;
+        let captured = session_manager.get_session_id(&normalised_name).await;
         assert_eq!(
             captured.as_deref(),
             Some(SESSION_ID),