From 34cfbbed54d1e84811ec19101bd5ad34dd2333cd Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Tue, 30 Jun 2026 14:46:27 -0400 Subject: [PATCH] fix(server): satisfy MCP catalog clippy lints --- lib/crates/fabro-config/src/layers/run.rs | 76 +++++++++---------- .../fabro-config/src/tests/resolve_run.rs | 6 +- lib/crates/fabro-mcp-store/src/store.rs | 45 +++++------ .../src/server/handler/mcp_servers.rs | 4 +- 4 files changed, 63 insertions(+), 68 deletions(-) diff --git a/lib/crates/fabro-config/src/layers/run.rs b/lib/crates/fabro-config/src/layers/run.rs index d02ad1289..da4fb0d07 100644 --- a/lib/crates/fabro-config/src/layers/run.rs +++ b/lib/crates/fabro-config/src/layers/run.rs @@ -576,44 +576,12 @@ impl<'de> Deserialize<'de> for McpEntryLayer { where D: Deserializer<'de>, { - let value = serde_json::Value::deserialize(deserializer)?; - let serde_json::Value::Object(map) = &value else { - return Err(de::Error::custom("MCP entry must be a table")); - }; - - let has_reference_fields = map.contains_key("id"); - let has_inline_fields = map.contains_key("type") - || map.contains_key("script") - || map.contains_key("command") - || map.contains_key("url") - || map.contains_key("headers") - || map.contains_key("port") - || map.contains_key("env") - || map.contains_key("startup_timeout") - || map.contains_key("tool_timeout"); - - if has_reference_fields && has_inline_fields { - return Err(de::Error::custom( - "MCP entry cannot mix catalog reference fields (`id`, `enabled`) with inline \ - server fields", - )); - } - - if has_reference_fields { - #[derive(Deserialize)] - #[serde(deny_unknown_fields)] - struct McpReferenceLayer { - id: String, - #[serde(default)] - enabled: Option, - } - - let reference: McpReferenceLayer = - serde_json::from_value(value).map_err(de::Error::custom)?; - return Ok(Self::Reference { - id: reference.id, - enabled: reference.enabled, - }); + #[derive(Deserialize)] + #[serde(deny_unknown_fields)] + struct McpReferenceLayer { + id: String, + #[serde(default)] + enabled: Option, } #[derive(Deserialize)] @@ -665,6 +633,38 @@ impl<'de> Deserialize<'de> for McpEntryLayer { }, } + let value = serde_json::Value::deserialize(deserializer)?; + let serde_json::Value::Object(map) = &value else { + return Err(de::Error::custom("MCP entry must be a table")); + }; + + let has_reference_fields = map.contains_key("id"); + let has_inline_fields = map.contains_key("type") + || map.contains_key("script") + || map.contains_key("command") + || map.contains_key("url") + || map.contains_key("headers") + || map.contains_key("port") + || map.contains_key("env") + || map.contains_key("startup_timeout") + || map.contains_key("tool_timeout"); + + if has_reference_fields && has_inline_fields { + return Err(de::Error::custom( + "MCP entry cannot mix catalog reference fields (`id`, `enabled`) with inline \ + server fields", + )); + } + + if has_reference_fields { + let reference: McpReferenceLayer = + serde_json::from_value(value).map_err(de::Error::custom)?; + return Ok(Self::Reference { + id: reference.id, + enabled: reference.enabled, + }); + } + match serde_json::from_value(value).map_err(de::Error::custom)? { TaggedMcpEntryLayer::Http { enabled, diff --git a/lib/crates/fabro-config/src/tests/resolve_run.rs b/lib/crates/fabro-config/src/tests/resolve_run.rs index cdeea2751..cb754659a 100644 --- a/lib/crates/fabro-config/src/tests/resolve_run.rs +++ b/lib/crates/fabro-config/src/tests/resolve_run.rs @@ -1019,7 +1019,9 @@ mod run_agent_mcps { use std::collections::HashMap; - use fabro_types::settings::run::{McpServerSettings, McpTransport, ResolvedMcpEntry}; + use fabro_types::settings::run::{ + McpHttpProtocol, McpServerSettings, McpTransport, ResolvedMcpEntry, + }; use crate::layers::Combine; use crate::tests::seeded_environment_catalog; @@ -1043,7 +1045,7 @@ mod run_agent_mcps { HashMap::from([("sentry".to_string(), McpServerSettings { name: "sentry".to_string(), transport: McpTransport::Http { - protocol: fabro_types::settings::run::McpHttpProtocol::default(), + protocol: McpHttpProtocol::default(), url: "https://sentry.example.com/mcp".to_string(), headers: HashMap::from([( "Authorization".to_string(), diff --git a/lib/crates/fabro-mcp-store/src/store.rs b/lib/crates/fabro-mcp-store/src/store.rs index d9122ca86..7ba18c2e0 100644 --- a/lib/crates/fabro-mcp-store/src/store.rs +++ b/lib/crates/fabro-mcp-store/src/store.rs @@ -4,6 +4,7 @@ use std::path::{Path, PathBuf}; use std::sync::RwLock; use std::time::{SystemTime, UNIX_EPOCH}; +use fabro_types::settings::run::McpServerSettings; use fabro_types::{ McpServerDefinition, McpServerDraft, McpServerId, McpServerReplace, McpServerRevision, McpServerView, @@ -18,9 +19,8 @@ use crate::model; /// Durable per-file TOML store for server-managed MCP server definitions. /// /// Concrete by design (no trait): a future FS→SQL move is a one-time migration, -/// not a runtime backend choice. The migration seam is the async, storage- -/// agnostic method surface; only [`McpServerStore::load`] knows about the -/// filesystem. +/// not a runtime backend choice. The method surface keeps callers storage- +/// agnostic; only [`McpServerStore::load`] knows about the filesystem. #[derive(Debug)] pub struct McpServerStore { dir: PathBuf, @@ -55,23 +55,21 @@ impl McpServerStore { self.defs.write().expect("mcp server store lock poisoned") } - pub async fn list(&self) -> Vec { + pub fn list(&self) -> Vec { let defs = self.read_defs(); let mut values = defs.values().cloned().collect::>(); values.sort_by(|left, right| left.id.cmp(&right.id)); values } - pub async fn list_views(&self) -> Vec { + pub fn list_views(&self) -> Vec { let defs = self.read_defs(); let mut values = defs.values().map(McpServerView::from).collect::>(); values.sort_by(|left, right| left.id.cmp(&right.id)); values } - pub fn catalog_settings( - &self, - ) -> HashMap { + pub fn catalog_settings(&self) -> HashMap { let defs = self.read_defs(); defs.iter() .map(|(id, definition)| (id.to_string(), server_settings_from_definition(definition))) @@ -81,14 +79,14 @@ impl McpServerStore { /// Sorted ids only, without cloning the (potentially sensitive) env/header /// maps carried by full definitions. Used by missing-reference errors to /// list available ids cheaply. - pub async fn ids(&self) -> Vec { + pub fn ids(&self) -> Vec { let defs = self.read_defs(); let mut ids = defs.keys().cloned().collect::>(); ids.sort(); ids } - pub async fn get(&self, id: &McpServerId) -> Option { + pub fn get(&self, id: &McpServerId) -> Option { self.read_defs().get(id).cloned() } @@ -171,10 +169,8 @@ fn check_revision( Ok(()) } -fn server_settings_from_definition( - definition: &McpServerDefinition, -) -> fabro_types::settings::run::McpServerSettings { - fabro_types::settings::run::McpServerSettings { +fn server_settings_from_definition(definition: &McpServerDefinition) -> McpServerSettings { + McpServerSettings { name: definition.id.to_string(), transport: definition.transport.clone(), current_dir: None, @@ -367,8 +363,8 @@ mod tests { let dir = tempfile::tempdir().unwrap(); let store = McpServerStore::load(dir.path().join("mcps")).unwrap(); - assert!(store.list().await.is_empty()); - assert!(store.ids().await.is_empty()); + assert!(store.list().is_empty()); + assert!(store.ids().is_empty()); } #[tokio::test] @@ -397,7 +393,7 @@ url = "https://sentry.example.com/mcp" .unwrap(); let store = McpServerStore::load(&mcp_dir).unwrap(); - let defs = store.list().await; + let defs = store.list(); assert_eq!(defs.len(), 1); assert_eq!(defs[0].id.as_str(), "sentry"); @@ -447,10 +443,10 @@ url = "https://sentry.example.com/mcp" McpServerRevision::from_bytes(persisted.as_bytes()) ); - assert_eq!(store.get(&created.id).await.unwrap(), created); - let listed = store.list().await; + assert_eq!(store.get(&created.id).unwrap(), created); + let listed = store.list(); assert_eq!(listed.len(), 1); - assert_eq!(store.ids().await, vec![created.id.clone()]); + assert_eq!(store.ids(), vec![created.id.clone()]); let replaced = store .replace(&created.id, &created.revision, replacement("Sentry v2")) @@ -458,13 +454,10 @@ url = "https://sentry.example.com/mcp" .unwrap(); assert_ne!(replaced.revision, created.revision); assert_eq!(replaced.display_name, "Sentry v2"); - assert_eq!( - store.get(&created.id).await.unwrap().revision, - replaced.revision - ); + assert_eq!(store.get(&created.id).unwrap().revision, replaced.revision); store.delete(&created.id, &replaced.revision).await.unwrap(); - assert!(store.get(&created.id).await.is_none()); + assert!(store.get(&created.id).is_none()); assert!(!path.exists()); } @@ -482,7 +475,7 @@ url = "https://sentry.example.com/mcp" assert!(matches!(err, McpServerStoreError::StaleRevision { .. })); // The on-disk and in-memory definition is unchanged after a rejected replace. - assert_eq!(store.get(&created.id).await.unwrap(), created); + assert_eq!(store.get(&created.id).unwrap(), created); } #[tokio::test] diff --git a/lib/crates/fabro-server/src/server/handler/mcp_servers.rs b/lib/crates/fabro-server/src/server/handler/mcp_servers.rs index dc0d1ce07..1de58810b 100644 --- a/lib/crates/fabro-server/src/server/handler/mcp_servers.rs +++ b/lib/crates/fabro-server/src/server/handler/mcp_servers.rs @@ -39,7 +39,7 @@ pub(super) fn routes() -> Router> { } async fn list_mcp_servers(_auth: RequiredUser, State(state): State>) -> Response { - let data = state.mcp_server_store().list_views().await; + let data = state.mcp_server_store().list_views(); let total = data.len(); ( StatusCode::OK, @@ -75,7 +75,7 @@ async fn get_mcp_server( Path(id): Path, ) -> Result { let id = parse_path_id(id)?; - match state.mcp_server_store().get(&id).await { + match state.mcp_server_store().get(&id) { Some(definition) => Ok(mcp_server_with_etag_response(StatusCode::OK, definition)), None => Err(ApiError::not_found(format!("mcp server not found: {id}"))), }