mirror of
https://gitee.com/mateos/mateclaw.git
synced 2026-09-13 19:23:42 +08:00
fix(mcp): cascade-delete agent-tool bindings when an MCP server is removed
This commit is contained in:
parent
421fd3cd61
commit
181e81a236
@ -0,0 +1,71 @@
|
||||
package vip.mate.agent.binding.service;
|
||||
|
||||
import com.baomidou.mybatisplus.core.conditions.query.LambdaQueryWrapper;
|
||||
import lombok.RequiredArgsConstructor;
|
||||
import lombok.extern.slf4j.Slf4j;
|
||||
import org.springframework.context.event.EventListener;
|
||||
import org.springframework.stereotype.Component;
|
||||
import vip.mate.agent.binding.model.AgentToolBinding;
|
||||
import vip.mate.agent.binding.repository.AgentToolBindingMapper;
|
||||
import vip.mate.tool.mcp.event.McpServerRemovedEvent;
|
||||
import vip.mate.tool.mcp.runtime.McpToolNameResolver;
|
||||
|
||||
import java.util.List;
|
||||
|
||||
/**
|
||||
* Drops {@code mate_agent_tool} rows that pointed at a now-removed MCP server's
|
||||
* tools (issue #127, MCP half).
|
||||
*
|
||||
* <p>MCP tool bindings are stored under the resolved name
|
||||
* {@code mcp_<serverId>_<slug>_<hash6>}. Deleting the server used to leave these
|
||||
* rows behind: the agent edit page kept showing the bindings and the user could
|
||||
* not clear them (the tools no longer exist in the live set, so the picker can't
|
||||
* render a row to uncheck). This mirrors the agent-skill cleanup for removed
|
||||
* skills.
|
||||
*
|
||||
* <p>Matching is done with an exact Java prefix rather than a SQL {@code LIKE}:
|
||||
* the literal underscores in {@code mcp_<serverId>_} are wildcards in {@code LIKE},
|
||||
* so {@code mcp_123_%} would also match server {@code 1234}'s tools. The coarse
|
||||
* query narrows to MCP bindings; the precise {@code startsWith} avoids deleting a
|
||||
* sibling server's rows.
|
||||
*/
|
||||
@Slf4j
|
||||
@Component
|
||||
@RequiredArgsConstructor
|
||||
public class AgentBindingMcpRemovalListener {
|
||||
|
||||
private final AgentToolBindingMapper toolBindingMapper;
|
||||
|
||||
@EventListener
|
||||
public void onMcpServerRemoved(McpServerRemovedEvent event) {
|
||||
if (event == null || event.serverId() == null) {
|
||||
return;
|
||||
}
|
||||
String serverPrefix = McpToolNameResolver.PREFIX + event.serverId() + "_";
|
||||
|
||||
// Coarse-filter to MCP bindings in SQL, then match the exact server
|
||||
// prefix in Java to avoid the LIKE-underscore-wildcard false match.
|
||||
List<AgentToolBinding> candidates = toolBindingMapper.selectList(
|
||||
new LambdaQueryWrapper<AgentToolBinding>()
|
||||
.likeRight(AgentToolBinding::getToolName, McpToolNameResolver.PREFIX));
|
||||
List<Long> orphanIds = candidates.stream()
|
||||
.filter(b -> belongsToServer(b.getToolName(), serverPrefix))
|
||||
.map(AgentToolBinding::getId)
|
||||
.toList();
|
||||
if (orphanIds.isEmpty()) {
|
||||
return;
|
||||
}
|
||||
int dropped = toolBindingMapper.delete(
|
||||
new LambdaQueryWrapper<AgentToolBinding>()
|
||||
.in(AgentToolBinding::getId, orphanIds));
|
||||
if (dropped > 0) {
|
||||
log.info("Cleaned {} agent-tool binding row(s) for removed MCP server {} (id={})",
|
||||
dropped, event.serverName(), event.serverId());
|
||||
}
|
||||
}
|
||||
|
||||
/** Exact prefix test: {@code mcp_123_x} belongs to server 123, {@code mcp_1234_x} does not. */
|
||||
static boolean belongsToServer(String toolName, String serverPrefix) {
|
||||
return toolName != null && toolName.startsWith(serverPrefix);
|
||||
}
|
||||
}
|
||||
@ -0,0 +1,20 @@
|
||||
package vip.mate.tool.mcp.event;
|
||||
|
||||
/**
|
||||
* Fires after an MCP server row has been deleted from {@code mate_mcp_server}.
|
||||
*
|
||||
* <p>Downstream listeners scrub records that reference the server's tools — most
|
||||
* importantly the agent-tool binding rows in {@code mate_agent_tool}, whose tool
|
||||
* names are {@code mcp_<serverId>_<slug>_<hash6>}. Without this cleanup, deleting
|
||||
* an MCP server leaves orphan bindings the agent edit page still shows and the
|
||||
* user can no longer clear (the tool no longer exists in the live set, so the
|
||||
* picker can't render a row to uncheck).
|
||||
*
|
||||
* <p>Distinct from {@link McpServerChangedEvent}, which signals a connection-state
|
||||
* change (the agent cache is rebuilt) but does not imply the server row is gone.
|
||||
*
|
||||
* @param serverId DB id of the removed {@code mate_mcp_server} row
|
||||
* @param serverName name the row carried, for log lines
|
||||
*/
|
||||
public record McpServerRemovedEvent(Long serverId, String serverName) {
|
||||
}
|
||||
@ -13,6 +13,7 @@ import org.springframework.stereotype.Service;
|
||||
import vip.mate.exception.MateClawException;
|
||||
import vip.mate.tool.mcp.event.McpConnectionLostEvent;
|
||||
import vip.mate.tool.mcp.event.McpServerChangedEvent;
|
||||
import vip.mate.tool.mcp.event.McpServerRemovedEvent;
|
||||
import vip.mate.tool.mcp.model.McpServerEntity;
|
||||
import vip.mate.tool.mcp.repository.McpServerMapper;
|
||||
import vip.mate.tool.mcp.runtime.McpClientManager;
|
||||
@ -221,6 +222,9 @@ public class McpServerService {
|
||||
mcpClientManager.remove(id);
|
||||
mcpServerMapper.deleteById(id);
|
||||
publishChanged("server-deleted");
|
||||
// Cascade-clean agent-tool bindings for this server's tools so the agent
|
||||
// edit page doesn't keep showing orphan bindings the user can't clear.
|
||||
eventPublisher.publishEvent(new McpServerRemovedEvent(id, entity.getName()));
|
||||
log.info("MCP server deleted: name={}, id={}", entity.getName(), id);
|
||||
}
|
||||
|
||||
|
||||
@ -0,0 +1,90 @@
|
||||
package vip.mate.agent.binding.service;
|
||||
|
||||
import org.junit.jupiter.api.Test;
|
||||
import org.junit.jupiter.api.DisplayName;
|
||||
import org.springframework.beans.factory.annotation.Autowired;
|
||||
import org.springframework.boot.test.context.SpringBootTest;
|
||||
import org.springframework.context.ApplicationEventPublisher;
|
||||
import com.baomidou.mybatisplus.core.conditions.query.LambdaQueryWrapper;
|
||||
import vip.mate.agent.binding.model.AgentToolBinding;
|
||||
import vip.mate.agent.binding.repository.AgentToolBindingMapper;
|
||||
import vip.mate.tool.mcp.event.McpServerRemovedEvent;
|
||||
|
||||
import java.time.LocalDateTime;
|
||||
import java.util.List;
|
||||
import java.util.Set;
|
||||
import java.util.concurrent.atomic.AtomicLong;
|
||||
import java.util.stream.Collectors;
|
||||
|
||||
import static org.junit.jupiter.api.Assertions.assertEquals;
|
||||
import static org.junit.jupiter.api.Assertions.assertTrue;
|
||||
|
||||
/**
|
||||
* Full-path regression (issue #127, MCP half): deleting an MCP server fires
|
||||
* {@link McpServerRemovedEvent}, and the listener must drop exactly that
|
||||
* server's agent-tool bindings — not a sibling server's whose id shares a
|
||||
* prefix, and not unrelated tools.
|
||||
*/
|
||||
@SpringBootTest(
|
||||
webEnvironment = SpringBootTest.WebEnvironment.NONE,
|
||||
properties = {
|
||||
"spring.flyway.enabled=true",
|
||||
"spring.flyway.locations=classpath:db/migration/h2",
|
||||
"mateclaw.feature-flag.refresh-ms=999999"
|
||||
}
|
||||
)
|
||||
class AgentBindingMcpRemovalE2ETest {
|
||||
|
||||
private static final AtomicLong SEQ = new AtomicLong(System.nanoTime());
|
||||
|
||||
@Autowired
|
||||
private AgentToolBindingMapper toolBindingMapper;
|
||||
@Autowired
|
||||
private ApplicationEventPublisher publisher;
|
||||
|
||||
private void bind(long agentId, String toolName) {
|
||||
AgentToolBinding b = new AgentToolBinding();
|
||||
b.setId(SEQ.incrementAndGet());
|
||||
b.setAgentId(agentId);
|
||||
b.setToolName(toolName);
|
||||
b.setEnabled(true);
|
||||
b.setCreateTime(LocalDateTime.now());
|
||||
b.setUpdateTime(LocalDateTime.now());
|
||||
b.setDeleted(0);
|
||||
toolBindingMapper.insert(b);
|
||||
}
|
||||
|
||||
private Set<String> toolsOf(long agentId) {
|
||||
return toolBindingMapper.selectList(
|
||||
new LambdaQueryWrapper<AgentToolBinding>().eq(AgentToolBinding::getAgentId, agentId))
|
||||
.stream().map(AgentToolBinding::getToolName).collect(Collectors.toSet());
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("Removing an MCP server drops only its tool bindings")
|
||||
void cascadeDropsOnlyTargetServerBindings() {
|
||||
long agent = SEQ.incrementAndGet();
|
||||
bind(agent, "mcp_123_ping_ab12cd");
|
||||
bind(agent, "mcp_123_search_99ffaa");
|
||||
bind(agent, "mcp_1234_ping_ff0011"); // sibling server — must survive
|
||||
bind(agent, "web_search"); // builtin — must survive
|
||||
|
||||
assertEquals(4, toolsOf(agent).size());
|
||||
|
||||
publisher.publishEvent(new McpServerRemovedEvent(123L, "test-mcp"));
|
||||
|
||||
Set<String> remaining = toolsOf(agent);
|
||||
assertEquals(Set.of("mcp_1234_ping_ff0011", "web_search"), remaining,
|
||||
"only server 123's bindings should be cleaned; sibling 1234 and builtin stay");
|
||||
assertTrue(remaining.stream().noneMatch(t -> t.startsWith("mcp_123_")));
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("No matching bindings is a no-op")
|
||||
void noMatchIsNoop() {
|
||||
long agent = SEQ.incrementAndGet();
|
||||
bind(agent, "web_search");
|
||||
publisher.publishEvent(new McpServerRemovedEvent(SEQ.incrementAndGet(), "empty-mcp"));
|
||||
assertEquals(Set.of("web_search"), toolsOf(agent));
|
||||
}
|
||||
}
|
||||
@ -0,0 +1,47 @@
|
||||
package vip.mate.agent.binding.service;
|
||||
|
||||
import org.junit.jupiter.api.DisplayName;
|
||||
import org.junit.jupiter.api.Test;
|
||||
import vip.mate.tool.mcp.runtime.McpToolNameResolver;
|
||||
|
||||
import static org.junit.jupiter.api.Assertions.assertFalse;
|
||||
import static org.junit.jupiter.api.Assertions.assertTrue;
|
||||
|
||||
/**
|
||||
* The MCP binding-cleanup prefix match must be exact: deleting server 123's
|
||||
* bindings must not also delete server 1234's, which a naive SQL
|
||||
* {@code LIKE 'mcp_123_%'} would (the underscores are wildcards).
|
||||
*/
|
||||
class AgentBindingMcpRemovalListenerTest {
|
||||
|
||||
private static String prefix(long serverId) {
|
||||
return McpToolNameResolver.PREFIX + serverId + "_";
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("Matches only the exact server's tools")
|
||||
void matchesExactServer() {
|
||||
String p = prefix(123L);
|
||||
assertTrue(AgentBindingMcpRemovalListener.belongsToServer("mcp_123_ping_ab12cd", p));
|
||||
assertTrue(AgentBindingMcpRemovalListener.belongsToServer("mcp_123_search_99ffaa", p));
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("Does NOT match a sibling server whose id shares the prefix digits")
|
||||
void doesNotMatchSiblingServer() {
|
||||
String p = prefix(123L);
|
||||
// The LIKE-wildcard trap: 'mcp_123_%' would match these, startsWith must not.
|
||||
assertFalse(AgentBindingMcpRemovalListener.belongsToServer("mcp_1234_ping_ab12cd", p));
|
||||
assertFalse(AgentBindingMcpRemovalListener.belongsToServer("mcp_12_ping_ab12cd", p));
|
||||
assertFalse(AgentBindingMcpRemovalListener.belongsToServer("mcp_1230_x_y", p));
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("Non-MCP and malformed names never match")
|
||||
void ignoresNonMcp() {
|
||||
String p = prefix(123L);
|
||||
assertFalse(AgentBindingMcpRemovalListener.belongsToServer("web_search", p));
|
||||
assertFalse(AgentBindingMcpRemovalListener.belongsToServer("mcp_123", p)); // no trailing separator
|
||||
assertFalse(AgentBindingMcpRemovalListener.belongsToServer(null, p));
|
||||
}
|
||||
}
|
||||
Loading…
Reference in New Issue
Block a user