opensource/mateclaw
2
0
Fork 0
mirror of https://gitee.com/mateos/mateclaw.git synced 2026-09-13 03:13:41 +08:00
Code Issues Actions Packages Projects Releases Wiki Activity

fix(skill): cascade-delete agent-skill bindings on skill removal (#127)

Browse Source
This commit is contained in:
matevip 2026-05-15 10:17:41 +08:00
parent eb6badeb61
commit 65dd4c1f49
6 changed files with 234 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

53
mateclaw-server/src/main/java/vip/mate/agent/binding/service/AgentBindingSkillRemovalListener.java Normal file
View File

@ -0,0 +1,53 @@
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.AgentSkillBinding;
import vip.mate.agent.binding.repository.AgentSkillBindingMapper;
import vip.mate.skill.event.SkillRemovedEvent;
/**
* Drops {@code mate_agent_skill} rows that pointed at a now-removed skill.
*
* <p>Without this listener, deleting a skill from the skill management page
* leaves orphan binding rows behind:
* <ul>
* <li>the agent edit modal still shows a non-zero badge from
* {@code GET /agents/{id}/skills},</li>
* <li>the picker list (sourced from {@code /skills} enabled set) no longer
* contains a checkbox for that id so the user can't uncheck it, and</li>
* <li>a subsequent {@code PUT /agents/{id}/skills} payload that still
* carries the orphan id is rejected by
* {@code AgentBindingService.setSkillBindings} with
* {@code err.skill.not_found}, leaving the user with no way to clear
* the stale binding.</li>
* </ul>
*
* <p>The event is dispatched synchronously from {@code SkillService} after
* the {@code mate_skill} row deletion, so the cleanup is part of the same
* request and observable in the very next list call.
*/
@Slf4j
@Component
@RequiredArgsConstructor
public class AgentBindingSkillRemovalListener {
private final AgentSkillBindingMapper skillBindingMapper;
@EventListener
public void onSkillRemoved(SkillRemovedEvent event) {
if (event == null || event.skillId() == null) {
return;
}
int dropped = skillBindingMapper.delete(
new LambdaQueryWrapper<AgentSkillBinding>()
.eq(AgentSkillBinding::getSkillId, event.skillId()));
if (dropped > 0) {
log.info("Cleaned {} agent-skill binding row(s) for removed skill {} (id={})",
dropped, event.skillName(), event.skillId());
}
}
}

17
mateclaw-server/src/main/java/vip/mate/skill/event/SkillRemovedEvent.java Normal file
View File

@ -0,0 +1,17 @@
package vip.mate.skill.event;
/**
* Fires after a skill row has been removed from {@code mate_skill}, whether
* through the user-facing uninstall path or the admin hard-delete path.
*
* <p>Downstream listeners use this to scrub records that reference the
* deleted skill — most importantly the agent-skill binding rows in
* {@code mate_agent_skill}, which would otherwise leave orphan bindings the
* UI can't unset (the binding count stays > 0 and the picker can no longer
* render the row to uncheck it).
*
* @param skillId DB id of the removed skill row
* @param skillName slug identifier the row carried, useful for log lines
*/
public record SkillRemovedEvent(Long skillId, String skillName) {
}

17
mateclaw-server/src/main/java/vip/mate/skill/service/SkillService.java
View File

@ -5,8 +5,10 @@ import com.baomidou.mybatisplus.core.metadata.IPage;
import com.baomidou.mybatisplus.extension.plugins.pagination.Page;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.springframework.context.ApplicationEventPublisher;
import org.springframework.stereotype.Service;
import vip.mate.exception.MateClawException;
import vip.mate.skill.event.SkillRemovedEvent;
import vip.mate.skill.model.SkillEntity;
import vip.mate.skill.repository.SkillFileMapper;
import vip.mate.skill.repository.SkillMapper;
@ -47,6 +49,13 @@ public class SkillService {
private final SkillWorkspaceManager workspaceManager;
private final SkillWorkspaceProperties workspaceProperties;
private final SkillSecretService skillSecretService;
/**
* Fires {@link SkillRemovedEvent} on both uninstall and hard-delete so
* the agent-binding listener (and any future subscriber) can scrub
* dependent rows — without this, {@code mate_agent_skill} keeps orphan
* rows that the UI can no longer clear from the picker.
*/
private final ApplicationEventPublisher eventPublisher;
private vip.mate.skill.runtime.SkillRuntimeService runtimeService;
/**
@ -414,6 +423,10 @@ public class SkillService {
skillMapper.deleteById(id); // logical delete (deleted=1)
log.info("Uninstalled skill (logical delete + archive): {}", skill.getName());
// Notify listeners (e.g. agent-binding cleanup) so dependent rows
// referencing this skill_id don't outlive the row itself.
eventPublisher.publishEvent(new SkillRemovedEvent(id, skill.getName()));
if ("archive".equals(workspaceProperties.getDeletePolicy())) {
workspaceManager.archiveWorkspace(skill.getName());
}
@ -448,6 +461,10 @@ public class SkillService {
}
log.info("Hard-deleted skill (physical delete + purge): {}", skill.getName());
// Same notification as the uninstall path — agent-binding cleanup
// applies regardless of which delete flavor the admin chose.
eventPublisher.publishEvent(new SkillRemovedEvent(id, skill.getName()));
// RFC-091 settings bridge — purge any per-skill secrets so a
// future skill reusing this id doesn't inherit stale credentials.
try {

49
mateclaw-server/src/test/java/vip/mate/agent/binding/AgentBindingSkillRemovalListenerTest.java Normal file
View File

@ -0,0 +1,49 @@
package vip.mate.agent.binding;
import com.baomidou.mybatisplus.core.conditions.query.LambdaQueryWrapper;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import vip.mate.agent.binding.model.AgentSkillBinding;
import vip.mate.agent.binding.repository.AgentSkillBindingMapper;
import vip.mate.agent.binding.service.AgentBindingSkillRemovalListener;
import vip.mate.skill.event.SkillRemovedEvent;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
/**
* Issue #127 regression — deleting a skill from the skill management page
* left orphan rows in {@code mate_agent_skill}, so the agent edit modal kept
* the old binding count and the user couldn't clear it. This listener drops
* those rows in response to {@link SkillRemovedEvent}.
*/
class AgentBindingSkillRemovalListenerTest {
@Test
@DisplayName("event triggers a delete on mate_agent_skill scoped to the removed skill id")
void removalDropsBindingRows() {
AgentSkillBindingMapper mapper = mock(AgentSkillBindingMapper.class);
when(mapper.delete(any(LambdaQueryWrapper.class))).thenReturn(2);
AgentBindingSkillRemovalListener listener = new AgentBindingSkillRemovalListener(mapper);
listener.onSkillRemoved(new SkillRemovedEvent(77L, "pdf"));
verify(mapper, times(1)).delete(any(LambdaQueryWrapper.class));
}
@Test
@DisplayName("null event or null skillId is a no-op — defensive guard")
void nullEventDoesNothing() {
AgentSkillBindingMapper mapper = mock(AgentSkillBindingMapper.class);
AgentBindingSkillRemovalListener listener = new AgentBindingSkillRemovalListener(mapper);
listener.onSkillRemoved(null);
listener.onSkillRemoved(new SkillRemovedEvent(null, "dangling"));
verify(mapper, never()).delete(any());
}
}

94
mateclaw-server/src/test/java/vip/mate/skill/service/SkillServiceRemovalEventTest.java Normal file
View File

@ -0,0 +1,94 @@
package vip.mate.skill.service;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import org.mockito.ArgumentCaptor;
import org.springframework.context.ApplicationEventPublisher;
import vip.mate.skill.event.SkillRemovedEvent;
import vip.mate.skill.model.SkillEntity;
import vip.mate.skill.repository.SkillFileMapper;
import vip.mate.skill.repository.SkillMapper;
import vip.mate.skill.runtime.SkillRuntimeService;
import vip.mate.skill.secret.SkillSecretService;
import vip.mate.skill.workspace.SkillWorkspaceManager;
import vip.mate.skill.workspace.SkillWorkspaceProperties;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
/**
* Issue #127 — verifies the delete paths publish {@link SkillRemovedEvent}
* so the agent-binding listener can scrub {@code mate_agent_skill} orphans.
*
* <p>Earlier behavior dropped only {@code mate_skill}/{@code mate_skill_file}
* /secrets/workspace and left binding rows pointing at a vanished skill id,
* which is what users saw as "agent still shows N skills bound".
*/
class SkillServiceRemovalEventTest {
@Test
@DisplayName("uninstallSkill publishes SkillRemovedEvent with the row's id and name")
void uninstallPublishesEvent() {
SkillMapper mapper = mock(SkillMapper.class);
SkillFileMapper fileMapper = mock(SkillFileMapper.class);
SkillWorkspaceManager workspaceManager = mock(SkillWorkspaceManager.class);
SkillWorkspaceProperties workspaceProps = mock(SkillWorkspaceProperties.class);
SkillSecretService secretService = mock(SkillSecretService.class);
SkillRuntimeService runtimeService = mock(SkillRuntimeService.class);
ApplicationEventPublisher publisher = mock(ApplicationEventPublisher.class);
SkillEntity row = new SkillEntity();
row.setId(42L);
row.setName("pdf");
row.setBuiltin(false);
when(mapper.selectById(42L)).thenReturn(row);
// Workspace policy other than "archive" keeps the test focused on the
// event publish behavior — the archive branch has its own coverage.
when(workspaceProps.getDeletePolicy()).thenReturn("purge");
SkillService service = new SkillService(
mapper, fileMapper, workspaceManager, workspaceProps, secretService, publisher);
service.setRuntimeService(runtimeService);
service.uninstallSkill(42L);
ArgumentCaptor<SkillRemovedEvent> captor = ArgumentCaptor.forClass(SkillRemovedEvent.class);
verify(publisher).publishEvent(captor.capture());
SkillRemovedEvent event = captor.getValue();
assertEquals(42L, event.skillId());
assertEquals("pdf", event.skillName());
}
@Test
@DisplayName("hardDeleteSkill publishes SkillRemovedEvent for the admin-only delete path")
void hardDeletePublishesEvent() {
SkillMapper mapper = mock(SkillMapper.class);
SkillFileMapper fileMapper = mock(SkillFileMapper.class);
SkillWorkspaceManager workspaceManager = mock(SkillWorkspaceManager.class);
SkillWorkspaceProperties workspaceProps = mock(SkillWorkspaceProperties.class);
SkillSecretService secretService = mock(SkillSecretService.class);
SkillRuntimeService runtimeService = mock(SkillRuntimeService.class);
ApplicationEventPublisher publisher = mock(ApplicationEventPublisher.class);
SkillEntity row = new SkillEntity();
row.setId(99L);
row.setName("legacy-cleanup");
row.setBuiltin(false);
when(mapper.selectById(99L)).thenReturn(row);
when(fileMapper.deleteBySkillId(99L)).thenReturn(0);
SkillService service = new SkillService(
mapper, fileMapper, workspaceManager, workspaceProps, secretService, publisher);
service.setRuntimeService(runtimeService);
service.hardDeleteSkill(99L);
ArgumentCaptor<SkillRemovedEvent> captor = ArgumentCaptor.forClass(SkillRemovedEvent.class);
verify(publisher).publishEvent(captor.capture());
SkillRemovedEvent event = captor.getValue();
assertEquals(99L, event.skillId());
assertEquals("legacy-cleanup", event.skillName());
}
}

6
mateclaw-server/src/test/java/vip/mate/skill/service/SkillServiceUpdatePartialTest.java
View File

@ -59,7 +59,8 @@ class SkillServiceUpdatePartialTest {
SkillService service = new SkillService(
mapper, mock(vip.mate.skill.repository.SkillFileMapper.class),
workspaceManager, workspaceProps, secretService);
workspaceManager, workspaceProps, secretService,
mock(org.springframework.context.ApplicationEventPublisher.class));
service.setRuntimeService(runtimeService);
SkillEntity existing = new SkillEntity();
@ -137,7 +138,8 @@ class SkillServiceUpdatePartialTest {
SkillService service = new SkillService(
mapper, mock(vip.mate.skill.repository.SkillFileMapper.class),
workspaceManager, workspaceProps, secretService);
workspaceManager, workspaceProps, secretService,
mock(org.springframework.context.ApplicationEventPublisher.class));
service.setRuntimeService(runtimeService);
SkillEntity existing = new SkillEntity();