From 5cb82ed8f8cbe7ec001a7692200bf277f3869696 Mon Sep 17 00:00:00 2001 From: matevip Date: Thu, 7 May 2026 08:17:53 +0800 Subject: [PATCH] feat(agent,mcp): validate tool bindings on save and keep returnDirect raw-name config working --- .../binding/service/AgentBindingService.java | 84 ++++++++++++++++++- .../runtime/McpReturnDirectProperties.java | 49 +++++++++-- .../mcp/runtime/McpToolCallbackProvider.java | 24 +++++- 3 files changed, 143 insertions(+), 14 deletions(-) diff --git a/mateclaw-server/src/main/java/vip/mate/agent/binding/service/AgentBindingService.java b/mateclaw-server/src/main/java/vip/mate/agent/binding/service/AgentBindingService.java index 9ecf1166..438834c9 100644 --- a/mateclaw-server/src/main/java/vip/mate/agent/binding/service/AgentBindingService.java +++ b/mateclaw-server/src/main/java/vip/mate/agent/binding/service/AgentBindingService.java @@ -12,8 +12,11 @@ import vip.mate.agent.binding.model.AgentToolBinding; import vip.mate.agent.binding.repository.AgentProviderPreferenceMapper; import vip.mate.agent.binding.repository.AgentSkillBindingMapper; import vip.mate.agent.binding.repository.AgentToolBindingMapper; +import vip.mate.exception.MateClawException; import vip.mate.skill.runtime.SkillRuntimeService; import vip.mate.skill.runtime.model.ResolvedSkill; +import vip.mate.tool.model.AvailableToolDTO; +import vip.mate.tool.service.AvailableToolService; import java.util.Collections; import java.util.LinkedHashSet; @@ -43,16 +46,25 @@ public class AgentBindingService { * graph when SkillRuntimeService initializes after binding. */ private final SkillRuntimeService skillRuntimeService; + /** + * Source of truth for what the picker can offer (built-in + MCP). Used + * by {@link #setToolBindings} to refuse new tool names that the runtime + * couldn't resolve anyway — closes the gap where a UI-disabled row + * could still be saved by hitting the API directly. + */ + private final AvailableToolService availableToolService; @Autowired public AgentBindingService(AgentSkillBindingMapper skillBindingMapper, AgentToolBindingMapper toolBindingMapper, AgentProviderPreferenceMapper providerPreferenceMapper, - @Lazy SkillRuntimeService skillRuntimeService) { + @Lazy SkillRuntimeService skillRuntimeService, + AvailableToolService availableToolService) { this.skillBindingMapper = skillBindingMapper; this.toolBindingMapper = toolBindingMapper; this.providerPreferenceMapper = providerPreferenceMapper; this.skillRuntimeService = skillRuntimeService; + this.availableToolService = availableToolService; } // ==================== Skill Bindings ==================== @@ -335,9 +347,27 @@ public class AgentBindingService { } /** - * 批量设置 Agent 的 tool 绑定（替换模式） + * Replace the agent's tool binding set. + * + *

Validation rule for each incoming name: + *

*/ public void setToolBindings(Long agentId, List toolNames) { + validateNewToolBindings(agentId, toolNames); + toolBindingMapper.delete( new LambdaQueryWrapper() .eq(AgentToolBinding::getAgentId, agentId)); @@ -352,6 +382,56 @@ public class AgentBindingService { } } + /** + * Refuse the save when any *newly-added* tool name doesn't resolve to + * an {@code available=true} row in the picker. Names already in the + * existing binding are exempt so that subsequent edits (especially + * "remove this stale tool") still succeed even if upstream state has + * drifted. + */ + private void validateNewToolBindings(Long agentId, List incoming) { + if (incoming == null || incoming.isEmpty()) { + return; + } + Set existing = listToolBindings(agentId).stream() + .map(AgentToolBinding::getToolName) + .collect(Collectors.toSet()); + Set bindable; + try { + bindable = availableToolService.listAvailable().stream() + .filter(AvailableToolDTO::isAvailable) + .map(AvailableToolDTO::getName) + .collect(Collectors.toSet()); + } catch (Exception e) { + // The picker source briefly failing must not block the user + // from saving a binding that's still in their existing set. + // Re-validate everything against just the existing set — + // strictly conservative: only allow keeps, refuse adds. + log.warn("AvailableToolService unavailable during binding validation, falling back to existing-only: {}", + e.getMessage()); + bindable = Set.of(); + } + + List rejected = new java.util.ArrayList<>(); + for (String name : incoming) { + if (name == null || name.isBlank()) { + rejected.add(""); + continue; + } + if (existing.contains(name)) continue; // keeps are always allowed + if (!bindable.contains(name)) rejected.add(name); + } + if (!rejected.isEmpty()) { + String preview = rejected.size() <= 5 + ? String.join(", ", rejected) + : String.join(", ", rejected.subList(0, 5)) + " (+" + (rejected.size() - 5) + " more)"; + throw new MateClawException("err.agent.tool_binding_unbindable", + "Tool name(s) cannot be bound: " + preview + + ". Either the name is unknown or the picker marked it unavailable " + + "(e.g. hash collision, upstream server removed)."); + } + } + // ==================== Provider Preferences (RFC-009 PR-3) ==================== /** Raw rows for the agent edit form. Sorted by sort_order ascending. */ diff --git a/mateclaw-server/src/main/java/vip/mate/tool/mcp/runtime/McpReturnDirectProperties.java b/mateclaw-server/src/main/java/vip/mate/tool/mcp/runtime/McpReturnDirectProperties.java index 6f83f4b9..ab456613 100644 --- a/mateclaw-server/src/main/java/vip/mate/tool/mcp/runtime/McpReturnDirectProperties.java +++ b/mateclaw-server/src/main/java/vip/mate/tool/mcp/runtime/McpReturnDirectProperties.java @@ -8,7 +8,7 @@ import java.util.LinkedHashSet; import java.util.Set; /** - * RFC-052 §3.4 / PR-4: MCP tool return-direct opt-in list. + * MCP tool return-direct opt-in list. * *

Tools listed here are wrapped in {@link ReturnDirectMcpToolCallback} so * their results bypass the LLM context (see {@code ToolExecutionExecutor} and @@ -20,14 +20,27 @@ import java.util.Set; * mcp: * return-direct: * tools: - * - query_employee_salary - * - read_medical_record + * - query_employee_salary # raw upstream name (legacy form, still supported) + * - mcp_42_query_employee_salary_aB3xYz # full prefixed callback name (server-scoped, precise) * * - *

Match is by tool name only (matching the upstream {@code ToolDefinition.name()}). - * Per-server scoping is intentionally out of scope for the first iteration; if - * the same tool name comes from two servers and only one should be direct, give - * one of them a name prefix at the MCP server config layer. + *

Two accepted name forms: + *

    + *
  • Raw upstream name ({@code query_employee_salary}) — + * matches the wrapped callback's underlying delegate name. This is + * the form that existed before the runtime started prefixing + * callback names; existing deployments keep working unchanged. + * A raw name matches every server that exposes that tool, so use + * this form when a sensitive name should be direct on every + * server it appears.
    • + *
  • Prefixed callback name + * ({@code mcp___}) — server-scoped, precise. + * Use this form when only one of several MCP servers exposing the + * same raw name should be treated as direct.
    • + *
+ * Matching happens via {@link #matches(String, String)} from the consumer + * side; see {@link McpToolCallbackProvider#getToolCallbacks} for the call + * site. * * @author MateClaw Team */ @@ -35,7 +48,7 @@ import java.util.Set; @ConfigurationProperties(prefix = "mateclaw.mcp.return-direct") public class McpReturnDirectProperties { - /** Tool names that should be treated as returnDirect. */ + /** Tool names (raw or prefixed) that should be treated as returnDirect. */ private Set tools = Collections.emptySet(); public Set getTools() { @@ -46,7 +59,27 @@ public class McpReturnDirectProperties { this.tools = tools != null ? new LinkedHashSet<>(tools) : Collections.emptySet(); } + /** + * Single-string check kept for back-compat with any caller that has + * only one form of the name. Prefer {@link #matches(String, String)} + * from the wrapping path so both prefixed and raw forms get a chance + * to match. + */ public boolean isReturnDirect(String toolName) { return toolName != null && tools.contains(toolName); } + + /** + * @return {@code true} iff the configured set contains either the + * prefixed callback name OR the raw upstream tool name. Either + * argument may be {@code null} (e.g. when a callback isn't a + * {@link PrefixedNameToolCallback} so no raw form is + * available); the other is checked on its own. + */ + public boolean matches(String prefixedName, String rawName) { + if (tools.isEmpty()) return false; + if (prefixedName != null && tools.contains(prefixedName)) return true; + if (rawName != null && tools.contains(rawName)) return true; + return false; + } } diff --git a/mateclaw-server/src/main/java/vip/mate/tool/mcp/runtime/McpToolCallbackProvider.java b/mateclaw-server/src/main/java/vip/mate/tool/mcp/runtime/McpToolCallbackProvider.java index 4a8496ea..0710259a 100644 --- a/mateclaw-server/src/main/java/vip/mate/tool/mcp/runtime/McpToolCallbackProvider.java +++ b/mateclaw-server/src/main/java/vip/mate/tool/mcp/runtime/McpToolCallbackProvider.java @@ -40,14 +40,30 @@ public class McpToolCallbackProvider implements ToolCallbackProvider { callbacks.size(), mcpClientManager.getActiveCount()); } - // RFC-052: opt-in returnDirect wrapping. The decorator only changes + // Opt-in returnDirect wrapping. The decorator only changes // ToolMetadata.returnDirect(); guard/approval/observability still // see the original callback through the wrapper. + // + // Names registered by the manager are now prefixed + // (mcp___) — but operators have been + // configuring the return-direct list with raw upstream names + // (e.g. `query_employee_salary`) since long before the prefix + // existed. Match on EITHER form so an existing deployment's + // sensitive-tool isolation doesn't silently regress when this + // change rolls out: a tool counts as return-direct if its + // configured token equals (a) the prefixed callback name OR + // (b) the underlying raw tool name visible through the + // PrefixedNameToolCallback wrapper. List wrapped = new ArrayList<>(callbacks.size()); for (ToolCallback cb : callbacks) { - String name = cb.getToolDefinition() != null ? cb.getToolDefinition().name() : null; - if (returnDirectProperties.isReturnDirect(name)) { - log.info("[McpToolCallbackProvider] wrapping MCP tool '{}' as returnDirect (RFC-052)", name); + String prefixed = cb.getToolDefinition() != null ? cb.getToolDefinition().name() : null; + String raw = (cb instanceof PrefixedNameToolCallback w && w.getDelegate() != null + && w.getDelegate().getToolDefinition() != null) + ? w.getDelegate().getToolDefinition().name() + : null; + if (returnDirectProperties.matches(prefixed, raw)) { + log.info("[McpToolCallbackProvider] wrapping MCP tool as returnDirect (prefixed='{}', raw='{}')", + prefixed, raw); wrapped.add(new ReturnDirectMcpToolCallback(cb)); } else { wrapped.add(cb);