From 3b147b9fc2459b36d23359daa431675c9e05c8ff Mon Sep 17 00:00:00 2001 From: laststylebender14 Date: Thu, 14 May 2026 13:24:37 +0530 Subject: [PATCH 1/5] refactor(mcp): decouple permission orchestration from service layer --- crates/forge_api/src/api.rs | 14 +- crates/forge_api/src/forge_api.rs | 19 +- crates/forge_app/src/lib.rs | 2 + crates/forge_app/src/mcp_app.rs | 110 ++++++++++ crates/forge_app/src/mcp_executor.rs | 6 +- crates/forge_app/src/services.rs | 10 +- crates/forge_app/src/tool_registry.rs | 4 +- crates/forge_main/src/ui.rs | 40 ++-- crates/forge_services/src/forge_services.rs | 9 +- crates/forge_services/src/mcp/service.rs | 228 +++++--------------- 10 files changed, 219 insertions(+), 223 deletions(-) create mode 100644 crates/forge_app/src/mcp_app.rs diff --git a/crates/forge_api/src/api.rs b/crates/forge_api/src/api.rs index e4e968db6f..a55f3496f8 100644 --- a/crates/forge_api/src/api.rs +++ b/crates/forge_api/src/api.rs @@ -3,7 +3,7 @@ use std::path::PathBuf; use anyhow::Result; use forge_app::dto::ToolsOverview; use forge_app::{User, UserUsage}; -use forge_domain::{AgentId, Effort, ModelId, PermissionOperation, ProviderModels}; +use forge_domain::{AgentId, Effort, ModelId, ProviderModels}; use forge_stream::MpscStream; use futures::stream::BoxStream; use url::Url; @@ -124,10 +124,14 @@ pub trait API: Sync + Send { /// project directory async fn write_mcp_config(&self, scope: &Scope, config: &McpConfig) -> Result<()>; - /// Unconditionally persists an allow policy for the given operation. - /// Use this when the user has explicitly opted in (e.g. via `mcp import`) - /// so no interactive confirmation is required on first use. - async fn allow_operation(&self, operation: &PermissionOperation) -> Result<()>; + /// Prompts for missing permissions for each enabled server in `cfg`. + /// Idempotent — servers with existing decisions are skipped. + /// Call this synchronously at startup before the REPL takes over stdin. + async fn request_mcp_permissions(&self, cfg: McpConfig) -> Result<()>; + + /// Persist `Allow` decisions for the named servers without prompting. + /// Used by `mcp import` to record consent on the user's behalf. + async fn allow_mcp_servers(&self, names: &[ServerName]) -> Result<()>; /// Retrieves the provider configuration for the specified agent async fn get_agent_provider(&self, agent_id: AgentId) -> anyhow::Result>; diff --git a/crates/forge_api/src/forge_api.rs b/crates/forge_api/src/forge_api.rs index 002d9e0d81..23efad7222 100644 --- a/crates/forge_api/src/forge_api.rs +++ b/crates/forge_api/src/forge_api.rs @@ -7,7 +7,7 @@ use forge_app::dto::ToolsOverview; use forge_app::{ AgentProviderResolver, AgentRegistry, AppConfigService, AuthService, CommandInfra, CommandLoaderService, ConversationService, DataGenerationApp, EnvironmentInfra, - FileDiscoveryService, ForgeApp, GitApp, GrpcInfra, McpConfigManager, McpService, PolicyService, + FileDiscoveryService, ForgeApp, GitApp, GrpcInfra, McpApp, McpConfigManager, McpService, ProviderAuthService, ProviderService, Services, User, UserUsage, Walker, WorkspaceService, }; use forge_config::ForgeConfig; @@ -39,6 +39,15 @@ impl ForgeAPI { { ForgeApp::new(self.services.clone()) } + + /// Creates an McpApp instance for MCP permission and connection orchestration. + fn mcp_app(&self) -> McpApp + where + A: Services + EnvironmentInfra, + F: EnvironmentInfra, + { + McpApp::new(self.services.clone()) + } } impl ForgeAPI>, ForgeRepo> { @@ -227,8 +236,12 @@ impl< .map_err(|e| anyhow::anyhow!(e)) } - async fn allow_operation(&self, operation: &PermissionOperation) -> Result<()> { - self.services.allow_operation(operation).await + async fn allow_mcp_servers(&self, names: &[ServerName]) -> Result<()> { + self.mcp_app().allow_mcp_servers(names).await + } + + async fn request_mcp_permissions(&self, cfg: McpConfig) -> Result<()> { + self.mcp_app().request_mcp_permissions(cfg).await } async fn execute_shell_command_raw( diff --git a/crates/forge_app/src/lib.rs b/crates/forge_app/src/lib.rs index 66de3e618d..644ace81aa 100644 --- a/crates/forge_app/src/lib.rs +++ b/crates/forge_app/src/lib.rs @@ -15,6 +15,7 @@ mod git_app; mod hooks; mod infra; mod init_conversation_metrics; +mod mcp_app; mod mcp_executor; mod operation; mod orch; @@ -47,6 +48,7 @@ pub use data_gen::*; pub use error::*; pub use git_app::*; pub use infra::*; +pub use mcp_app::*; pub use services::*; pub use template_engine::*; pub use terminal_context::*; diff --git a/crates/forge_app/src/mcp_app.rs b/crates/forge_app/src/mcp_app.rs new file mode 100644 index 0000000000..b8c457c3a0 --- /dev/null +++ b/crates/forge_app/src/mcp_app.rs @@ -0,0 +1,110 @@ +use std::sync::Arc; + +use anyhow::Result; +use forge_domain::*; +use merge::Merge; + +use crate::services::{McpConfigManager, McpService, PolicyService}; +use crate::{EnvironmentInfra, Services}; + +/// McpApp handles MCP permission reconciliation and policy-filtered +/// connections, keeping `McpService` free of any policy awareness. +pub struct McpApp { + services: Arc, +} + +impl McpApp { + /// Creates a new McpApp instance with the provided services. + pub fn new(services: Arc) -> Self { + Self { services } + } +} + +impl> McpApp { + /// Prompts for missing permissions for each enabled server in `cfg`. + /// Idempotent — servers that already have a recorded decision are skipped + /// silently. + /// + /// This is the only place a permission prompt can fire for MCP. Call this + /// synchronously at startup (before the REPL takes over stdin) so prompts + /// don't race with user input. + pub async fn request_mcp_permissions(&self, cfg: McpConfig) -> Result<()> { + let cwd = self.services.get_environment().cwd; + for (name, server) in cfg + .mcp_servers + .into_iter() + .filter(|(_, s)| !s.is_disabled()) + { + let op = PermissionOperation::Mcp { + config: server, + cwd: cwd.clone(), + message: format!("Allow MCP server \"{name}\" to connect?"), + }; + // check_operation_permission handles the prompt + persist. + // The return value is intentionally discarded here; the caller + // just needs all decisions to be recorded before connections start. + let _ = self.services.check_operation_permission(&op).await?; + } + Ok(()) + } + + /// Returns a merged MCP config where user-scope servers are trusted + /// unconditionally and local-scope servers are filtered to those with an + /// explicit `Allow` policy. Never prompts — call + /// [`Self::request_mcp_permissions`] first to ensure decisions exist. + pub async fn permitted_mcp_config(&self) -> Result { + let mut user = self + .services + .read_mcp_config(Some(&Scope::User)) + .await?; + let local = self + .services + .read_mcp_config(Some(&Scope::Local)) + .await?; + let cwd = self.services.get_environment().cwd; + + let mut filtered_local = McpConfig::default(); + for (name, server) in local.mcp_servers { + if server.is_disabled() { + continue; + } + let op = PermissionOperation::Mcp { + config: server.clone(), + cwd: cwd.clone(), + message: String::new(), + }; + if self.services.is_operation_permitted(&op).await? { + filtered_local.mcp_servers.insert(name, server); + } + } + user.merge(filtered_local); + Ok(user) + } + + /// Lists MCP tools, connecting only servers that have an explicit `Allow` + /// policy for local-scope entries (user-scope are trusted unconditionally). + /// Never prompts. + pub async fn get_mcp_servers(&self) -> Result { + let cfg = self.permitted_mcp_config().await?; + self.services.get_mcp_servers(cfg).await + } + + /// Persist `Allow` decisions for the named servers without prompting. + /// Used by `mcp import` to record consent on the user's behalf — importing + /// is itself an explicit opt-in. + pub async fn allow_mcp_servers(&self, names: &[ServerName]) -> Result<()> { + let cfg = self.services.read_mcp_config(None).await?; + let cwd = self.services.get_environment().cwd; + for name in names { + if let Some(server) = cfg.mcp_servers.get(name) { + let op = PermissionOperation::Mcp { + config: server.clone(), + cwd: cwd.clone(), + message: format!("Connect to MCP server: {name}"), + }; + self.services.allow_operation(&op).await?; + } + } + Ok(()) + } +} diff --git a/crates/forge_app/src/mcp_executor.rs b/crates/forge_app/src/mcp_executor.rs index 21e3d024ba..c7ffde2c54 100644 --- a/crates/forge_app/src/mcp_executor.rs +++ b/crates/forge_app/src/mcp_executor.rs @@ -2,7 +2,7 @@ use std::sync::Arc; use forge_domain::{TitleFormat, ToolCallContext, ToolCallFull, ToolName, ToolOutput}; -use crate::McpService; +use crate::{EnvironmentInfra, McpApp, McpService, Services}; pub struct McpExecutor { services: Arc, @@ -24,9 +24,11 @@ impl McpExecutor { self.services.execute_mcp(input).await } +} +impl> McpExecutor { pub async fn contains_tool(&self, tool_name: &ToolName) -> anyhow::Result { - let mcp_servers = self.services.get_mcp_servers().await?; + let mcp_servers = McpApp::new(self.services.clone()).get_mcp_servers().await?; // Convert Claude Code format (mcp__{server}__{tool}) to the internal legacy // format (mcp_{server}_tool_{tool}) before checking, so both name styles match. let legacy = tool_name.to_legacy_mcp_name(); diff --git a/crates/forge_app/src/services.rs b/crates/forge_app/src/services.rs index 5a990d947f..522d65820e 100644 --- a/crates/forge_app/src/services.rs +++ b/crates/forge_app/src/services.rs @@ -218,7 +218,11 @@ pub trait McpConfigManager: Send + Sync { #[async_trait::async_trait] pub trait McpService: Send + Sync { - async fn get_mcp_servers(&self) -> anyhow::Result; + /// Connect to and list tools from the given MCP servers. + /// The caller is responsible for filtering `cfg` through any policy + /// gating before calling; this method connects every enabled server + /// in `cfg` without re-checking permissions. + async fn get_mcp_servers(&self, cfg: McpConfig) -> anyhow::Result; async fn execute_mcp(&self, call: ToolCallFull) -> anyhow::Result; /// Refresh the MCP cache by fetching fresh data async fn reload_mcp(&self) -> anyhow::Result<()>; @@ -702,8 +706,8 @@ impl McpConfigManager for I { #[async_trait::async_trait] impl McpService for I { - async fn get_mcp_servers(&self) -> anyhow::Result { - self.mcp_service().get_mcp_servers().await + async fn get_mcp_servers(&self, cfg: McpConfig) -> anyhow::Result { + self.mcp_service().get_mcp_servers(cfg).await } async fn execute_mcp(&self, call: ToolCallFull) -> anyhow::Result { diff --git a/crates/forge_app/src/tool_registry.rs b/crates/forge_app/src/tool_registry.rs index dbfff3da06..4d46bc4e48 100644 --- a/crates/forge_app/src/tool_registry.rs +++ b/crates/forge_app/src/tool_registry.rs @@ -21,7 +21,7 @@ use crate::fmt::content::FormatContent; use crate::mcp_executor::McpExecutor; use crate::tool_executor::ToolExecutor; use crate::{ - AgentRegistry, EnvironmentInfra, McpService, PolicyService, ProviderService, Services, + AgentRegistry, EnvironmentInfra, McpApp, PolicyService, ProviderService, Services, ToolResolver, WorkspaceService, }; @@ -241,7 +241,7 @@ impl> ToolReg } pub async fn tools_overview(&self) -> anyhow::Result { - let mcp_tools = self.services.get_mcp_servers().await?; + let mcp_tools = McpApp::new(self.services.clone()).get_mcp_servers().await?; let agent_tools = self.agent_executor.agent_definitions().await?; // Get agents for template rendering in Task tool description diff --git a/crates/forge_main/src/ui.rs b/crates/forge_main/src/ui.rs index 9650a70b57..efea6a0896 100644 --- a/crates/forge_main/src/ui.rs +++ b/crates/forge_main/src/ui.rs @@ -11,7 +11,7 @@ use convert_case::{Case, Casing}; use forge_api::{ API, AgentId, AnyProvider, ApiKeyRequest, AuthContextRequest, AuthContextResponse, ChatRequest, ChatResponse, CodeRequest, ConfigOperation, Conversation, ConversationId, DeviceCodeRequest, - Event, InterruptionReason, ModelId, Provider, ProviderId, TextMessage, UserPrompt, + Event, InterruptionReason, ModelId, Provider, ProviderId, Scope, TextMessage, UserPrompt, }; use forge_app::utils::{format_display_path, truncate_key}; use forge_app::{CommitResult, ToolResolver}; @@ -223,9 +223,7 @@ impl A + Send + Sync> UI self.display_banner()?; self.trace_user(); self.hydrate_caches(); - // Resolve MCP connections up front and surface any permission - // warnings before control returns to the caller. - let _ = self.api.get_tools().await; + self.request_local_mcp_permissions().await?; Ok(()) } @@ -370,10 +368,7 @@ impl A + Send + Sync> UI self.trace_user(); self.hydrate_caches(); self.init_conversation().await?; - - // Initialise MCP connections and display any permission warnings - // before the REPL takes over stdin. - let _ = self.api.get_tools().await; + self.request_local_mcp_permissions().await?; // Check for dispatch flag first if let Some(dispatch_json) = self.cli.event.clone() { @@ -450,11 +445,22 @@ impl A + Send + Sync> UI } } + /// Reads the local-scope MCP config and asks the user for permission for + /// each server that does not yet have a recorded decision. Call this + /// synchronously before the REPL takes over stdin so prompts don't race + /// with user input. + async fn request_local_mcp_permissions(&self) -> Result<()> { + let local_cfg = self.api.read_mcp_config(Some(&Scope::Local)).await?; + self.api.request_mcp_permissions(local_cfg).await + } + // Improve startup time by hydrating caches fn hydrate_caches(&self) { let api = self.api.clone(); tokio::spawn(async move { api.get_models().await }); let api = self.api.clone(); + tokio::spawn(async move { let _ = api.get_tools().await; }); + let api = self.api.clone(); tokio::spawn(async move { api.get_agent_infos().await }); let api = self.api.clone(); tokio::spawn(async move { @@ -574,21 +580,9 @@ impl A + Send + Sync> UI // Write back to the specific scope only self.api.write_mcp_config(&scope, &scope_config).await?; - let cwd = self.api.environment().cwd; - - // Grant allow permission for each imported server so the user - // is not prompted again on first use — importing is itself an - // explicit opt-in. - for server_name in &added_servers { - if let Some(server_config) = scope_config.mcp_servers.get(server_name) { - let operation = forge_domain::PermissionOperation::Mcp { - config: server_config.clone(), - cwd: cwd.clone(), - message: format!("Connect to MCP server: {server_name}"), - }; - self.api.allow_operation(&operation).await?; - } - } + // Importing is an explicit opt-in — persist Allow decisions so + // the user is not prompted on first use. + self.api.allow_mcp_servers(&added_servers).await?; // Log each added server after successful write for server_name in added_servers { diff --git a/crates/forge_services/src/forge_services.rs b/crates/forge_services/src/forge_services.rs index 32a5b9a379..52f61beaa8 100644 --- a/crates/forge_services/src/forge_services.rs +++ b/crates/forge_services/src/forge_services.rs @@ -30,8 +30,7 @@ use crate::tool_services::{ ForgeFsUndo, ForgeFsWrite, ForgeImageRead, ForgePlanCreate, ForgeShell, ForgeSkillFetch, }; -type McpService = - ForgeMcpService, F, ::Client, ForgePolicyService>; +type McpService = ForgeMcpService, F, ::Client>; type AuthService = ForgeAuthService; /// ForgeApp is the main application container that implements the App trait. @@ -112,11 +111,7 @@ impl< pub fn new(infra: Arc) -> Self { let mcp_manager = Arc::new(ForgeMcpManager::new(infra.clone())); let policy_service = ForgePolicyService::new(infra.clone()); - let mcp_service = Arc::new(ForgeMcpService::new( - mcp_manager.clone(), - infra.clone(), - Arc::new(policy_service.clone()), - )); + let mcp_service = Arc::new(ForgeMcpService::new(mcp_manager.clone(), infra.clone())); let template_service = Arc::new(ForgeTemplateService::new(infra.clone())); let attachment_service = Arc::new(ForgeChatRequest::new(infra.clone())); let suggestion_service = Arc::new(ForgeDiscoveryService::new(infra.clone())); diff --git a/crates/forge_services/src/mcp/service.rs b/crates/forge_services/src/mcp/service.rs index 35a78e28c4..706e0f510c 100644 --- a/crates/forge_services/src/mcp/service.rs +++ b/crates/forge_services/src/mcp/service.rs @@ -1,16 +1,12 @@ -use std::collections::{HashMap, HashSet}; +use std::collections::HashMap; use std::sync::Arc; use anyhow::Context; use forge_app::domain::{ - McpConfig, McpServerConfig, McpServers, PermissionOperation, Scope, ServerName, ToolCallFull, - ToolDefinition, ToolName, ToolOutput, + McpConfig, McpServerConfig, McpServers, ServerName, ToolCallFull, ToolDefinition, ToolName, + ToolOutput, }; -use forge_app::{ - EnvironmentInfra, KVStore, McpClientInfra, McpConfigManager, McpServerInfra, McpService, - PolicyService, -}; -use merge::Merge; +use forge_app::{EnvironmentInfra, KVStore, McpClientInfra, McpConfigManager, McpServerInfra, McpService}; use tokio::sync::{Mutex, RwLock}; use crate::mcp::tool::McpExecutor; @@ -24,28 +20,14 @@ fn generate_mcp_tool_name(server_name: &ServerName, tool_name: &ToolName) -> Too )) } -pub struct ForgeMcpService { +#[derive(Clone)] +pub struct ForgeMcpService { tools: Arc>>>>, failed_servers: Arc>>, previous_config_hash: Arc>, init_lock: Arc>, manager: Arc, infra: Arc, - policy: Arc

, -} - -impl Clone for ForgeMcpService { - fn clone(&self) -> Self { - ForgeMcpService { - tools: Arc::clone(&self.tools), - failed_servers: Arc::clone(&self.failed_servers), - previous_config_hash: Arc::clone(&self.previous_config_hash), - init_lock: Arc::clone(&self.init_lock), - manager: Arc::clone(&self.manager), - infra: Arc::clone(&self.infra), - policy: Arc::clone(&self.policy), - } - } } #[derive(Clone)] @@ -55,15 +37,14 @@ struct ToolHolder { server_name: String, } -impl ForgeMcpService +impl ForgeMcpService where M: McpConfigManager + 'static, I: McpServerInfra + KVStore + EnvironmentInfra + 'static, C: McpClientInfra + Clone, C: From<::Client>, - P: PolicyService + 'static, { - pub fn new(manager: Arc, infra: Arc, policy: Arc

) -> Self { + pub fn new(manager: Arc, infra: Arc) -> Self { Self { tools: Default::default(), failed_servers: Default::default(), @@ -71,7 +52,6 @@ where init_lock: Arc::new(Mutex::new(())), manager, infra, - policy, } } @@ -118,15 +98,10 @@ where Ok(()) } - async fn init_mcp(&self) -> anyhow::Result<()> { - let user_cfg = self.manager.read_mcp_config(Some(&Scope::User)).await?; - let local_cfg = self.manager.read_mcp_config(Some(&Scope::Local)).await?; - let mut merged = user_cfg.clone(); - merged.merge(local_cfg.clone()); - + async fn init_mcp(&self, cfg: McpConfig) -> anyhow::Result<()> { // Fast path: if config is unchanged, skip reinitialization without acquiring // the lock - if !self.is_config_modified(&merged).await { + if !self.is_config_modified(&cfg).await { return Ok(()); } @@ -135,42 +110,25 @@ where let _guard = self.init_lock.lock().await; // Double-check under the lock: a concurrent caller may have already updated - if !self.is_config_modified(&merged).await { + if !self.is_config_modified(&cfg).await { return Ok(()); } - self.update_mcp(user_cfg, local_cfg, merged).await + self.update_mcp(cfg).await } - async fn update_mcp( - &self, - user_cfg: McpConfig, - local_cfg: McpConfig, - merged: McpConfig, - ) -> anyhow::Result<()> { - // Compute the hash early before `merged` is consumed, but write it only + async fn update_mcp(&self, mcp: McpConfig) -> anyhow::Result<()> { + // Compute the hash early before `mcp` is consumed, but write it only // after all connections are established so waiters on init_lock see a // consistent state. - let new_hash = merged.cache_key(); + let new_hash = mcp.cache_key(); self.clear_tools().await; self.failed_servers.write().await.clear(); - // User-scoped servers are trusted by default — authorize without policy check. - let mut authorized: HashSet = user_cfg - .mcp_servers - .iter() - .filter(|(_, s)| !s.is_disabled()) - .map(|(name, _)| name.clone()) - .collect(); - - // Only local-scoped servers go through the policy engine. - let local_authorized = self.authorize_servers(&local_cfg).await?; - authorized.extend(local_authorized); - - let connections: Vec<_> = merged + let connections: Vec<_> = mcp .mcp_servers .into_iter() - .filter(|(name, server)| !server.is_disabled() && authorized.contains(name)) + .filter(|(_, server)| !server.is_disabled()) .map(|(name, server)| async move { let conn = self .connect(&name, server) @@ -199,46 +157,8 @@ where Ok(()) } - /// Runs the permission policy against every enabled server in `cfg`. - /// Returns the set of authorised server names. - /// Denials are recorded in `failed_servers`. - async fn authorize_servers(&self, cfg: &McpConfig) -> anyhow::Result> { - let env = self.infra.get_environment(); - - let mut authorized = HashSet::new(); - let mut failures = Vec::new(); - - for (name, server) in cfg.mcp_servers.iter().filter(|(_, s)| !s.is_disabled()) { - let operation = PermissionOperation::Mcp { - config: server.clone(), - cwd: env.cwd.clone(), - message: format!("Allow MCP server \"{name}\" to connect?"), - }; - match self.policy.check_operation_permission(&operation).await { - Ok(decision) if decision.allowed => { - authorized.insert(name.clone()); - } - Ok(_) => { - failures.push((name.clone(), "Connection denied by policy".to_string())); - } - Err(err) => { - failures.push((name.clone(), format!("Policy check failed: {err:?}"))); - } - } - } - - if !failures.is_empty() { - let mut failed = self.failed_servers.write().await; - for (name, reason) in failures { - failed.insert(name, reason); - } - } - - Ok(authorized) - } - - async fn list(&self) -> anyhow::Result { - self.init_mcp().await?; + async fn list(&self, cfg: McpConfig) -> anyhow::Result { + self.init_mcp(cfg).await?; let tools = self.tools.read().await; let mut grouped_tools = std::collections::HashMap::new(); @@ -254,13 +174,17 @@ where Ok(McpServers::new(grouped_tools, failures)) } + async fn clear_tools(&self) { self.tools.write().await.clear() } async fn call(&self, call: ToolCallFull) -> anyhow::Result { - // Ensure MCP connections are initialized before calling tools - self.init_mcp().await?; + // Ensure MCP connections are initialized before calling tools. + // For execute_mcp we read the merged (unfiltered) config so that all + // previously-connected servers are reachable regardless of scope. + let cfg = self.manager.read_mcp_config(None).await?; + self.init_mcp(cfg).await?; let tools = self.tools.read().await; @@ -274,23 +198,6 @@ where tool.executable.call_tool(call.arguments.parse()?).await } - /// Returns `true` if any enabled server in `cfg` does not have a - /// persisted `Allow` policy, meaning a permission prompt would be required. - async fn has_servers_requiring_permission(&self, cfg: &McpConfig) -> anyhow::Result { - let env = self.infra.get_environment(); - for (name, server) in cfg.mcp_servers.iter().filter(|(_, s)| !s.is_disabled()) { - let operation = PermissionOperation::Mcp { - config: server.clone(), - cwd: env.cwd.clone(), - message: format!("Allow MCP server \"{name}\" to connect?"), - }; - if !self.policy.is_operation_permitted(&operation).await? { - return Ok(true); - } - } - Ok(false) - } - /// Refresh the MCP cache by clearing cached data. /// Does NOT eagerly connect to servers - connections happen lazily /// when list() or call() is invoked, avoiding interactive OAuth during @@ -310,34 +217,22 @@ where } #[async_trait::async_trait] -impl McpService for ForgeMcpService +impl McpService for ForgeMcpService where M: McpConfigManager + 'static, I: McpServerInfra + KVStore + EnvironmentInfra + 'static, C: McpClientInfra + Clone, C: From<::Client>, - P: PolicyService + 'static, { - async fn get_mcp_servers(&self) -> anyhow::Result { - let user_cfg = self.manager.read_mcp_config(Some(&Scope::User)).await?; - let local_cfg = self.manager.read_mcp_config(Some(&Scope::Local)).await?; - let mut merged_cfg = user_cfg.clone(); - merged_cfg.merge(local_cfg.clone()); - let config_hash = merged_cfg.cache_key(); - - let needs_permission_check = self.has_servers_requiring_permission(&local_cfg).await?; + async fn get_mcp_servers(&self, cfg: McpConfig) -> anyhow::Result { + let config_hash = cfg.cache_key(); - if !needs_permission_check - && let Some(cache) = self.infra.cache_get::<_, McpServers>(&config_hash).await? - { - return Ok(cache.clone()); + if let Some(cache) = self.infra.cache_get::<_, McpServers>(&config_hash).await? { + return Ok(cache); } - let servers = self.list().await?; - - if !needs_permission_check { - self.infra.cache_set(&config_hash, &servers).await?; - } + let servers = self.list(cfg).await?; + self.infra.cache_set(&config_hash, &servers).await?; Ok(servers) } @@ -358,13 +253,10 @@ mod tests { use fake::{Fake, Faker}; use forge_app::domain::{ - ConfigOperation, Environment, McpConfig, McpServerConfig, PermissionOperation, Scope, - ServerName, ToolCallFull, ToolDefinition, ToolName, ToolOutput, - }; - use forge_app::{ - EnvironmentInfra, KVStore, McpClientInfra, McpConfigManager, McpServerInfra, McpService, - PolicyDecision, PolicyService, + ConfigOperation, Environment, McpConfig, McpServerConfig, Scope, ServerName, ToolCallFull, + ToolDefinition, ToolName, ToolOutput, }; + use forge_app::{EnvironmentInfra, KVStore, McpClientInfra, McpConfigManager, McpServerInfra, McpService}; use forge_config::ForgeConfig; use pretty_assertions::assert_eq; use serde::de::DeserializeOwned; @@ -481,42 +373,19 @@ mod tests { } } - // ── Mock policy service ────────────────────────────────────────────────── - - /// Permits every operation. Tests need MCP connections to go through - /// without prompting; production behaviour (Confirm by default) is covered - /// by `forge_services::policy` and `forge_domain::policies::engine` tests. - struct AlwaysAllowPolicy; - - #[async_trait::async_trait] - impl PolicyService for AlwaysAllowPolicy { - async fn check_operation_permission( - &self, - _operation: &PermissionOperation, - ) -> anyhow::Result { - Ok(PolicyDecision { allowed: true, path: None }) - } - - async fn is_operation_permitted( - &self, - _operation: &PermissionOperation, - ) -> anyhow::Result { - Ok(true) - } + // ── Fixture ────────────────────────────────────────────────────────────── - async fn allow_operation(&self, _operation: &PermissionOperation) -> anyhow::Result<()> { - Ok(()) - } + fn fixture() -> ForgeMcpService { + ForgeMcpService::new(Arc::new(MockMcpManager), Arc::new(MockInfra)) } - // ── Fixture ────────────────────────────────────────────────────────────── - - fn fixture() -> ForgeMcpService { - ForgeMcpService::new( - Arc::new(MockMcpManager), - Arc::new(MockInfra), - Arc::new(AlwaysAllowPolicy), - ) + fn fixture_cfg() -> McpConfig { + let mut servers = BTreeMap::new(); + servers.insert( + ServerName::from("test-server".to_string()), + McpServerConfig::new_stdio("echo", vec![], None), + ); + McpConfig { mcp_servers: servers } } #[test] @@ -570,14 +439,17 @@ mod tests { #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_concurrent_init_does_not_race() { let service = Arc::new(fixture()); + let cfg = fixture_cfg(); let s1 = service.clone(); let s2 = service.clone(); - let (r1, r2) = tokio::join!(s1.get_mcp_servers(), s2.get_mcp_servers()); + let c1 = cfg.clone(); + let c2 = cfg.clone(); + let (r1, r2) = tokio::join!(s1.get_mcp_servers(c1), s2.get_mcp_servers(c2)); r1.unwrap(); r2.unwrap(); - let servers = service.get_mcp_servers().await.unwrap(); + let servers = service.get_mcp_servers(cfg).await.unwrap(); let tool_name = servers .get_servers() .values() From d4f2489d060f76534106907847ea0e4324f673dd Mon Sep 17 00:00:00 2001 From: laststylebender14 Date: Thu, 14 May 2026 13:39:25 +0530 Subject: [PATCH 2/5] refactor(mcp): accept config in execute_mcp --- crates/forge_app/src/mcp_executor.rs | 8 +++--- crates/forge_app/src/services.rs | 9 ++++--- crates/forge_services/src/forge_services.rs | 4 +-- crates/forge_services/src/mcp/service.rs | 30 +++++++++------------ 4 files changed, 24 insertions(+), 27 deletions(-) diff --git a/crates/forge_app/src/mcp_executor.rs b/crates/forge_app/src/mcp_executor.rs index c7ffde2c54..94bc63a483 100644 --- a/crates/forge_app/src/mcp_executor.rs +++ b/crates/forge_app/src/mcp_executor.rs @@ -8,7 +8,7 @@ pub struct McpExecutor { services: Arc, } -impl McpExecutor { +impl> McpExecutor { pub fn new(services: Arc) -> Self { Self { services } } @@ -22,11 +22,11 @@ impl McpExecutor { .send_tool_input(TitleFormat::info("MCP").sub_title(input.name.as_str())) .await?; - self.services.execute_mcp(input).await + let mcp_app = McpApp::new(self.services.clone()); + let cfg = mcp_app.permitted_mcp_config().await?; + self.services.execute_mcp(input, cfg).await } -} -impl> McpExecutor { pub async fn contains_tool(&self, tool_name: &ToolName) -> anyhow::Result { let mcp_servers = McpApp::new(self.services.clone()).get_mcp_servers().await?; // Convert Claude Code format (mcp__{server}__{tool}) to the internal legacy diff --git a/crates/forge_app/src/services.rs b/crates/forge_app/src/services.rs index 522d65820e..3a3dae5965 100644 --- a/crates/forge_app/src/services.rs +++ b/crates/forge_app/src/services.rs @@ -223,7 +223,10 @@ pub trait McpService: Send + Sync { /// gating before calling; this method connects every enabled server /// in `cfg` without re-checking permissions. async fn get_mcp_servers(&self, cfg: McpConfig) -> anyhow::Result; - async fn execute_mcp(&self, call: ToolCallFull) -> anyhow::Result; + /// Execute a tool call against an already-connected server. The caller is + /// responsible for supplying a pre-filtered `cfg` (same one used for + /// `get_mcp_servers`) so denied servers are never reconnected here. + async fn execute_mcp(&self, call: ToolCallFull, cfg: McpConfig) -> anyhow::Result; /// Refresh the MCP cache by fetching fresh data async fn reload_mcp(&self) -> anyhow::Result<()>; } @@ -710,8 +713,8 @@ impl McpService for I { self.mcp_service().get_mcp_servers(cfg).await } - async fn execute_mcp(&self, call: ToolCallFull) -> anyhow::Result { - self.mcp_service().execute_mcp(call).await + async fn execute_mcp(&self, call: ToolCallFull, cfg: McpConfig) -> anyhow::Result { + self.mcp_service().execute_mcp(call, cfg).await } async fn reload_mcp(&self) -> anyhow::Result<()> { diff --git a/crates/forge_services/src/forge_services.rs b/crates/forge_services/src/forge_services.rs index 52f61beaa8..d2ba2b0522 100644 --- a/crates/forge_services/src/forge_services.rs +++ b/crates/forge_services/src/forge_services.rs @@ -30,7 +30,7 @@ use crate::tool_services::{ ForgeFsUndo, ForgeFsWrite, ForgeImageRead, ForgePlanCreate, ForgeShell, ForgeSkillFetch, }; -type McpService = ForgeMcpService, F, ::Client>; +type McpService = ForgeMcpService::Client>; type AuthService = ForgeAuthService; /// ForgeApp is the main application container that implements the App trait. @@ -109,9 +109,9 @@ impl< > ForgeServices { pub fn new(infra: Arc) -> Self { + let mcp_service = Arc::new(ForgeMcpService::new(infra.clone())); let mcp_manager = Arc::new(ForgeMcpManager::new(infra.clone())); let policy_service = ForgePolicyService::new(infra.clone()); - let mcp_service = Arc::new(ForgeMcpService::new(mcp_manager.clone(), infra.clone())); let template_service = Arc::new(ForgeTemplateService::new(infra.clone())); let attachment_service = Arc::new(ForgeChatRequest::new(infra.clone())); let suggestion_service = Arc::new(ForgeDiscoveryService::new(infra.clone())); diff --git a/crates/forge_services/src/mcp/service.rs b/crates/forge_services/src/mcp/service.rs index 706e0f510c..1530d6cf06 100644 --- a/crates/forge_services/src/mcp/service.rs +++ b/crates/forge_services/src/mcp/service.rs @@ -6,7 +6,7 @@ use forge_app::domain::{ McpConfig, McpServerConfig, McpServers, ServerName, ToolCallFull, ToolDefinition, ToolName, ToolOutput, }; -use forge_app::{EnvironmentInfra, KVStore, McpClientInfra, McpConfigManager, McpServerInfra, McpService}; +use forge_app::{EnvironmentInfra, KVStore, McpClientInfra, McpServerInfra, McpService}; use tokio::sync::{Mutex, RwLock}; use crate::mcp::tool::McpExecutor; @@ -21,12 +21,11 @@ fn generate_mcp_tool_name(server_name: &ServerName, tool_name: &ToolName) -> Too } #[derive(Clone)] -pub struct ForgeMcpService { +pub struct ForgeMcpService { tools: Arc>>>>, failed_servers: Arc>>, previous_config_hash: Arc>, init_lock: Arc>, - manager: Arc, infra: Arc, } @@ -37,20 +36,18 @@ struct ToolHolder { server_name: String, } -impl ForgeMcpService +impl ForgeMcpService where - M: McpConfigManager + 'static, I: McpServerInfra + KVStore + EnvironmentInfra + 'static, C: McpClientInfra + Clone, C: From<::Client>, { - pub fn new(manager: Arc, infra: Arc) -> Self { + pub fn new(infra: Arc) -> Self { Self { tools: Default::default(), failed_servers: Default::default(), previous_config_hash: Arc::new(Mutex::new(Default::default())), init_lock: Arc::new(Mutex::new(())), - manager, infra, } } @@ -179,11 +176,9 @@ where self.tools.write().await.clear() } - async fn call(&self, call: ToolCallFull) -> anyhow::Result { - // Ensure MCP connections are initialized before calling tools. - // For execute_mcp we read the merged (unfiltered) config so that all - // previously-connected servers are reachable regardless of scope. - let cfg = self.manager.read_mcp_config(None).await?; + async fn call(&self, call: ToolCallFull, cfg: McpConfig) -> anyhow::Result { + // Use the caller-supplied pre-filtered config so only permitted servers + // are (re)connected here. self.init_mcp(cfg).await?; let tools = self.tools.read().await; @@ -217,9 +212,8 @@ where } #[async_trait::async_trait] -impl McpService for ForgeMcpService +impl McpService for ForgeMcpService where - M: McpConfigManager + 'static, I: McpServerInfra + KVStore + EnvironmentInfra + 'static, C: McpClientInfra + Clone, C: From<::Client>, @@ -237,8 +231,8 @@ where Ok(servers) } - async fn execute_mcp(&self, call: ToolCallFull) -> anyhow::Result { - self.call(call).await + async fn execute_mcp(&self, call: ToolCallFull, cfg: McpConfig) -> anyhow::Result { + self.call(call, cfg).await } async fn reload_mcp(&self) -> anyhow::Result<()> { @@ -256,7 +250,7 @@ mod tests { ConfigOperation, Environment, McpConfig, McpServerConfig, Scope, ServerName, ToolCallFull, ToolDefinition, ToolName, ToolOutput, }; - use forge_app::{EnvironmentInfra, KVStore, McpClientInfra, McpConfigManager, McpServerInfra, McpService}; + use forge_app::{EnvironmentInfra, KVStore, McpClientInfra, McpServerInfra, McpService}; use forge_config::ForgeConfig; use pretty_assertions::assert_eq; use serde::de::DeserializeOwned; @@ -460,7 +454,7 @@ mod tests { .clone(); let call = ToolCallFull::new(tool_name); - let actual = service.execute_mcp(call).await.unwrap(); + let actual = service.execute_mcp(call, fixture_cfg()).await.unwrap(); let expected = ToolOutput::text("mock result"); assert_eq!(actual, expected); } From 8b1515b717ed5967fd9a8870f90ee9dd0a6cbd7c Mon Sep 17 00:00:00 2001 From: laststylebender14 Date: Thu, 14 May 2026 13:46:22 +0530 Subject: [PATCH 3/5] refactor(policy): derive Clone on ForgePolicyService --- crates/forge_services/src/policy.rs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/crates/forge_services/src/policy.rs b/crates/forge_services/src/policy.rs index 735714a48e..0f88ed2745 100644 --- a/crates/forge_services/src/policy.rs +++ b/crates/forge_services/src/policy.rs @@ -40,17 +40,11 @@ enum McpPermission { Reject, } +#[derive(Clone)] pub struct ForgePolicyService { infra: Arc, } -impl Clone for ForgePolicyService { - // Manual impl so callers don't need `I: Clone`; we only ever clone the - // `Arc` which is always cheap. - fn clone(&self) -> Self { - Self { infra: self.infra.clone() } - } -} /// Default policies loaded once at startup from the embedded YAML file static DEFAULT_POLICIES: LazyLock = LazyLock::new(|| { let yaml_content = include_str!("./permissions.default.yaml"); From 3eb4942354ae837cb8dc977eee0a5fb18031e9c6 Mon Sep 17 00:00:00 2001 From: laststylebender14 Date: Thu, 14 May 2026 13:50:23 +0530 Subject: [PATCH 4/5] refactor(policy): rename McpPermission to ConfirmPermission --- crates/forge_services/src/policy.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/crates/forge_services/src/policy.rs b/crates/forge_services/src/policy.rs index 0f88ed2745..fb45254e46 100644 --- a/crates/forge_services/src/policy.rs +++ b/crates/forge_services/src/policy.rs @@ -27,15 +27,15 @@ pub enum PolicyPermission { AcceptAndRemember, } -/// Two-choice prompt used exclusively for MCP server connections. -/// Both choices are persisted so the user is not re-prompted on subsequent -/// starts. +/// Two-choice prompt for operations where both Accept and Reject are +/// persisted so the user is never asked again. Use this instead of +/// [`PolicyPermission`] when there is no meaningful "one-off allow" path. #[derive(Debug, Clone, PartialEq, Eq, Display, EnumIter, strum_macros::EnumString)] -enum McpPermission { - /// Allow this MCP server to connect and persist the decision +enum ConfirmPermission { + /// Allow the operation and remember this choice #[strum(to_string = "Accept")] Accept, - /// Deny this MCP server and persist the decision + /// Deny the operation and remember this choice #[strum(to_string = "Reject")] Reject, } @@ -219,12 +219,12 @@ where PermissionOperation::Mcp { message, config, cwd } => { let header = mcp_config_header(config); let prompt = SelectPrompt::new(message.clone()).with_header(header); - return match self.infra.select_one_enum::(prompt).await? { - Some(McpPermission::Accept) => { + return match self.infra.select_one_enum::(prompt).await? { + Some(ConfirmPermission::Accept) => { let update_path = self.add_policy_for_operation(operation).await?; Ok(PolicyDecision { allowed: true, path: update_path.or(path) }) } - Some(McpPermission::Reject) | None => { + Some(ConfirmPermission::Reject) | None => { let deny_policy = Policy::Simple { permission: Permission::Deny, rule: forge_app::domain::Rule::Mcp( From 4c56edb678c31e641e37ac2915742db0e818cad8 Mon Sep 17 00:00:00 2001 From: laststylebender14 Date: Thu, 14 May 2026 13:52:59 +0530 Subject: [PATCH 5/5] refactor(mcp): remove MockMcpManager from test fixture --- crates/forge_services/src/mcp/service.rs | 30 +++--------------------- 1 file changed, 3 insertions(+), 27 deletions(-) diff --git a/crates/forge_services/src/mcp/service.rs b/crates/forge_services/src/mcp/service.rs index 1530d6cf06..5c03cfa7c6 100644 --- a/crates/forge_services/src/mcp/service.rs +++ b/crates/forge_services/src/mcp/service.rs @@ -247,7 +247,7 @@ mod tests { use fake::{Fake, Faker}; use forge_app::domain::{ - ConfigOperation, Environment, McpConfig, McpServerConfig, Scope, ServerName, ToolCallFull, + ConfigOperation, Environment, McpConfig, McpServerConfig, ServerName, ToolCallFull, ToolDefinition, ToolName, ToolOutput, }; use forge_app::{EnvironmentInfra, KVStore, McpClientInfra, McpServerInfra, McpService}; @@ -277,30 +277,6 @@ mod tests { } } - // ── Mock config manager ────────────────────────────────────────────────── - - struct MockMcpManager; - - #[async_trait::async_trait] - impl McpConfigManager for MockMcpManager { - async fn read_mcp_config(&self, _scope: Option<&Scope>) -> anyhow::Result { - let mut servers = BTreeMap::new(); - servers.insert( - ServerName::from("test-server".to_string()), - McpServerConfig::new_stdio("echo", vec![], None), - ); - Ok(McpConfig { mcp_servers: servers }) - } - - async fn write_mcp_config( - &self, - _config: &McpConfig, - _scope: &Scope, - ) -> anyhow::Result<()> { - Ok(()) - } - } - // ── Mock infrastructure ────────────────────────────────────────────────── #[derive(Clone)] @@ -369,8 +345,8 @@ mod tests { // ── Fixture ────────────────────────────────────────────────────────────── - fn fixture() -> ForgeMcpService { - ForgeMcpService::new(Arc::new(MockMcpManager), Arc::new(MockInfra)) + fn fixture() -> ForgeMcpService { + ForgeMcpService::new(Arc::new(MockInfra)) } fn fixture_cfg() -> McpConfig {