Skip to content

Commit 64c451e

Browse files
committed
refactor(tools): integrate mcp_call with confirmation flow and improve lazy loading
- Add `match_against_confirm_deny` to `ToolMcpCall` to proxy confirmation checks to underlying MCP tools before execution, enabling normal pause/deny flow - Disable parallel execution for `mcp_call` and add input validation for `args` - Filter disabled MCP tools from `mcp_search` results - Enhance MCP lazy loading with `is_integration_mcp_tool` helper to exclude proxy builtins (`mcp_call`, `mcp_search`) from filtering logic, making it idempotent and cache-safe - Apply lazy filter consistently in chat tool handling
1 parent 0f34925 commit 64c451e

4 files changed

Lines changed: 83 additions & 29 deletions

File tree

refact-agent/engine/src/chat/tools.rs

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,8 @@ async fn resolve_tool_call_aliases(
6464
mode_id: &str,
6565
model_id: Option<&str>,
6666
) -> Vec<ChatToolCall> {
67-
let available_tools = crate::tools::tools_list::get_tools_for_mode(gcx, mode_id, model_id).await;
67+
let raw_tools = crate::tools::tools_list::get_tools_for_mode(gcx, mode_id, model_id).await;
68+
let available_tools = crate::tools::tools_list::apply_mcp_lazy_filter(raw_tools).tools;
6869
let tool_names: Vec<String> = available_tools.iter().map(|t| t.tool_description().name.clone()).collect();
6970
let registry = build_registry_from_names(&tool_names);
7071
if !registry.needs_aliasing() {
@@ -863,10 +864,11 @@ pub async fn check_tools_confirmation(
863864
.collect();
864865

865866
let all_tools =
866-
crate::tools::tools_list::get_tools_for_mode(gcx.clone(), mode_id, model_id)
867-
.await
868-
.into_iter()
869-
.filter_map(|tool| {
867+
crate::tools::tools_list::apply_mcp_lazy_filter(
868+
crate::tools::tools_list::get_tools_for_mode(gcx.clone(), mode_id, model_id).await
869+
).tools
870+
.into_iter()
871+
.filter_map(|tool| {
870872
let spec = tool.tool_description();
871873
if needed_names.contains(spec.name.as_str()) {
872874
Some((spec.name, tool))

refact-agent/engine/src/tools/tool_mcp_call.rs

Lines changed: 58 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ use tokio::sync::Mutex as AMutex;
77

88
use crate::at_commands::at_commands::AtCommandsContext;
99
use crate::call_validation::ContextEnum;
10-
use crate::tools::tools_description::{MatchConfirmDenyResult, Tool, ToolConfig, ToolDesc, ToolGroupCategory, ToolSource, ToolSourceType};
10+
use crate::tools::tools_description::{MatchConfirmDeny, MatchConfirmDenyResult, Tool, ToolConfig, ToolDesc, ToolGroupCategory, ToolSource, ToolSourceType};
1111
use crate::tools::tools_list::get_integration_tools;
1212

1313
pub struct ToolMcpCall {}
@@ -18,7 +18,7 @@ impl Tool for ToolMcpCall {
1818
ToolDesc {
1919
name: "mcp_call".to_string(),
2020
experimental: false,
21-
allow_parallel: true,
21+
allow_parallel: false,
2222
description: "Execute any MCP tool by name with the given arguments. \
2323
Use `mcp_tool_search` first to discover the tool name and its input schema, \
2424
then call this with the exact arguments the schema requires."
@@ -51,6 +51,53 @@ impl Tool for ToolMcpCall {
5151
Ok(ToolConfig { enabled: true, allow_parallel: None })
5252
}
5353

54+
/// Proxy confirmation/deny checks to the underlying MCP tool so that
55+
/// `check_tools_confirmation()` can trigger the normal pause/deny flow
56+
/// before `tool_execute` is ever called.
57+
async fn match_against_confirm_deny(
58+
&self,
59+
ccx: Arc<AMutex<AtCommandsContext>>,
60+
args: &HashMap<String, Value>,
61+
) -> Result<crate::tools::tools_description::MatchConfirmDeny, String> {
62+
let tool_name = match args.get("tool_name").and_then(|v| v.as_str()) {
63+
Some(n) => n.to_string(),
64+
None => return Ok(MatchConfirmDeny {
65+
result: MatchConfirmDenyResult::PASS,
66+
command: String::new(),
67+
rule: String::new(),
68+
}),
69+
};
70+
71+
let tool_args: HashMap<String, Value> = args.get("args")
72+
.and_then(|v| v.as_object())
73+
.map(|obj| obj.iter().map(|(k, v)| (k.clone(), v.clone())).collect())
74+
.unwrap_or_default();
75+
76+
let gcx = ccx.lock().await.global_context.clone();
77+
let mut integration_groups = get_integration_tools(gcx).await;
78+
79+
// Move the tool out of the groups so it can be awaited safely.
80+
let mut found_tool: Option<Box<dyn Tool + Send>> = None;
81+
'outer: for group in &mut integration_groups {
82+
if !matches!(group.category, ToolGroupCategory::MCP) {
83+
continue;
84+
}
85+
if let Some(pos) = group.tools.iter().position(|t| t.tool_description().name == tool_name) {
86+
found_tool = Some(group.tools.remove(pos));
87+
break 'outer;
88+
}
89+
}
90+
91+
match found_tool {
92+
Some(tool) => tool.match_against_confirm_deny(ccx, &tool_args).await,
93+
None => Ok(MatchConfirmDeny {
94+
result: MatchConfirmDenyResult::PASS,
95+
command: String::new(),
96+
rule: String::new(),
97+
}),
98+
}
99+
}
100+
54101
async fn tool_execute(
55102
&mut self,
56103
ccx: Arc<AMutex<AtCommandsContext>>,
@@ -62,10 +109,13 @@ impl Tool for ToolMcpCall {
62109
.ok_or_else(|| "mcp_call: missing required argument 'tool_name'".to_string())?
63110
.to_string();
64111

65-
let tool_args: HashMap<String, Value> = args.get("args")
66-
.and_then(|v| v.as_object())
67-
.map(|obj| obj.iter().map(|(k, v)| (k.clone(), v.clone())).collect())
68-
.unwrap_or_default();
112+
let tool_args: HashMap<String, Value> = match args.get("args") {
113+
None => return Err("mcp_call: missing required argument 'args'".to_string()),
114+
Some(v) => match v.as_object() {
115+
None => return Err("mcp_call: argument 'args' must be an object".to_string()),
116+
Some(obj) => obj.iter().map(|(k, v)| (k.clone(), v.clone())).collect(),
117+
},
118+
};
69119

70120
let gcx = ccx.lock().await.global_context.clone();
71121
let mut integration_groups = get_integration_tools(gcx).await;
@@ -89,19 +139,8 @@ impl Tool for ToolMcpCall {
89139
)
90140
})?;
91141

92-
let confirm_result = tool.match_against_confirm_deny(ccx.clone(), &tool_args).await
93-
.map_err(|e| format!("mcp_call confirmation check failed: {}", e))?;
94-
if confirm_result.result == MatchConfirmDenyResult::DENY {
95-
return Err(format!(
96-
"MCP tool '{}' was denied by rule '{}'",
97-
tool_name, confirm_result.rule
98-
));
99-
}
100-
if confirm_result.result == MatchConfirmDenyResult::CONFIRMATION {
101-
return Err(format!(
102-
"MCP tool '{}' requires user confirmation (rule '{}'). Use the tool directly instead of via mcp_call to enable the confirmation flow.",
103-
tool_name, confirm_result.rule
104-
));
142+
if !tool.config().unwrap_or_default().enabled {
143+
return Err(format!("MCP tool '{}' is disabled.", tool_name));
105144
}
106145

107146
tool.tool_execute(ccx, tool_call_id, &tool_args).await

refact-agent/engine/src/tools/tool_mcp_search.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,7 @@ impl Tool for ToolMcpSearch {
7575
let matched: Vec<(String, String, Value)> = integration_groups.iter()
7676
.filter(|g| matches!(g.category, ToolGroupCategory::MCP))
7777
.flat_map(|g| g.tools.iter())
78+
.filter(|tool| tool.config().unwrap_or_default().enabled)
7879
.filter(|tool| {
7980
let d = tool.tool_description();
8081
re.is_match(&d.name) || re.is_match(&d.description)

refact-agent/engine/src/tools/tools_list.rs

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ use crate::integrations::running_integrations::load_integrations;
88
use crate::yaml_configs::customization_registry::get_project_registry;
99
use crate::caps::resolve_chat_model;
1010

11-
use super::tools_description::{Tool, ToolGroup, ToolGroupCategory};
11+
use super::tools_description::{Tool, ToolGroup, ToolGroupCategory, ToolSourceType};
1212
use super::tool_config_subagent::ToolConfigSubagent;
1313

1414
/// When MCP tool count exceeds this threshold, lazy loading activates.
@@ -32,15 +32,27 @@ pub struct ToolsForMode {
3232
pub mcp_tool_index: Vec<(String, String)>,
3333
}
3434

35+
/// Returns true for real MCP integration tools, false for the proxy builtins
36+
/// (`mcp_call`, `mcp_tool_search`) which share the "mcp" name prefix but have
37+
/// `ToolSourceType::Builtin`. This makes `apply_mcp_lazy_filter` idempotent.
38+
fn is_integration_mcp_tool(t: &Box<dyn Tool + Send>) -> bool {
39+
let d = t.tool_description();
40+
d.name.starts_with("mcp") && matches!(d.source.source_type, ToolSourceType::Integration)
41+
}
42+
3543
/// Apply MCP lazy-loading to a flat tool list returned by `get_tools_for_mode`.
3644
///
3745
/// When there are more than `MCP_LAZY_THRESHOLD` MCP tools, ALL individual MCP
3846
/// schemas are replaced by two fixed proxy tools (`mcp_tool_search` + `mcp_call`).
3947
/// The tool list produced here NEVER changes during the session — cache-safe.
48+
///
49+
/// Safe to call multiple times: proxy tools have `ToolSourceType::Builtin` so they
50+
/// are never counted or removed by subsequent calls.
4051
pub fn apply_mcp_lazy_filter(mut tools: Vec<Box<dyn Tool + Send>>) -> ToolsForMode {
41-
// Collect the index of ALL MCP tools (by name prefix convention) before filtering.
52+
// Collect the index of ALL real MCP integration tools before filtering.
53+
// Proxy builtins (mcp_call / mcp_tool_search) are excluded via source_type check.
4254
let mcp_tool_index: Vec<(String, String)> = tools.iter()
43-
.filter(|t| t.tool_description().name.starts_with("mcp"))
55+
.filter(|t| is_integration_mcp_tool(t))
4456
.map(|t| {
4557
let d = t.tool_description();
4658
(d.name, d.description)
@@ -51,8 +63,8 @@ pub fn apply_mcp_lazy_filter(mut tools: Vec<Box<dyn Tool + Send>>) -> ToolsForMo
5163
let mcp_lazy_mode = mcp_total_count > MCP_LAZY_THRESHOLD;
5264

5365
if mcp_lazy_mode {
54-
// Drop ALL individual MCP tool schemas.
55-
tools.retain(|t| !t.tool_description().name.starts_with("mcp"));
66+
// Drop ALL individual MCP tool schemas (integration tools only).
67+
tools.retain(|t| !is_integration_mcp_tool(t));
5668
// Inject two fixed proxies — tool list is now stable for the session.
5769
tools.push(Box::new(crate::tools::tool_mcp_search::ToolMcpSearch {}));
5870
tools.push(Box::new(crate::tools::tool_mcp_call::ToolMcpCall {}));

0 commit comments

Comments
 (0)