diff --git a/mateclaw-server/src/main/java/vip/mate/agent/binding/service/AgentBindingMcpRemovalListener.java b/mateclaw-server/src/main/java/vip/mate/agent/binding/service/AgentBindingMcpRemovalListener.java new file mode 100644 index 00000000..048a8c1e --- /dev/null +++ b/mateclaw-server/src/main/java/vip/mate/agent/binding/service/AgentBindingMcpRemovalListener.java @@ -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). + * + *

MCP tool bindings are stored under the resolved name + * {@code mcp___}. 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. + * + *

Matching is done with an exact Java prefix rather than a SQL {@code LIKE}: + * the literal underscores in {@code mcp__} 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 candidates = toolBindingMapper.selectList( + new LambdaQueryWrapper() + .likeRight(AgentToolBinding::getToolName, McpToolNameResolver.PREFIX)); + List orphanIds = candidates.stream() + .filter(b -> belongsToServer(b.getToolName(), serverPrefix)) + .map(AgentToolBinding::getId) + .toList(); + if (orphanIds.isEmpty()) { + return; + } + int dropped = toolBindingMapper.delete( + new LambdaQueryWrapper() + .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); + } +} diff --git a/mateclaw-server/src/main/java/vip/mate/tool/mcp/event/McpServerRemovedEvent.java b/mateclaw-server/src/main/java/vip/mate/tool/mcp/event/McpServerRemovedEvent.java new file mode 100644 index 00000000..105d900e --- /dev/null +++ b/mateclaw-server/src/main/java/vip/mate/tool/mcp/event/McpServerRemovedEvent.java @@ -0,0 +1,20 @@ +package vip.mate.tool.mcp.event; + +/** + * Fires after an MCP server row has been deleted from {@code mate_mcp_server}. + * + *

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___}. 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). + * + *

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) { +} diff --git a/mateclaw-server/src/main/java/vip/mate/tool/mcp/service/McpServerService.java b/mateclaw-server/src/main/java/vip/mate/tool/mcp/service/McpServerService.java index e79bf753..615207bd 100644 --- a/mateclaw-server/src/main/java/vip/mate/tool/mcp/service/McpServerService.java +++ b/mateclaw-server/src/main/java/vip/mate/tool/mcp/service/McpServerService.java @@ -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); } diff --git a/mateclaw-server/src/test/java/vip/mate/agent/binding/service/AgentBindingMcpRemovalE2ETest.java b/mateclaw-server/src/test/java/vip/mate/agent/binding/service/AgentBindingMcpRemovalE2ETest.java new file mode 100644 index 00000000..ffd91062 --- /dev/null +++ b/mateclaw-server/src/test/java/vip/mate/agent/binding/service/AgentBindingMcpRemovalE2ETest.java @@ -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 toolsOf(long agentId) { + return toolBindingMapper.selectList( + new LambdaQueryWrapper().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 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)); + } +} diff --git a/mateclaw-server/src/test/java/vip/mate/agent/binding/service/AgentBindingMcpRemovalListenerTest.java b/mateclaw-server/src/test/java/vip/mate/agent/binding/service/AgentBindingMcpRemovalListenerTest.java new file mode 100644 index 00000000..3127fb20 --- /dev/null +++ b/mateclaw-server/src/test/java/vip/mate/agent/binding/service/AgentBindingMcpRemovalListenerTest.java @@ -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)); + } +}