From b321d5792efcc300ecd096239fb9ce4250259808 Mon Sep 17 00:00:00 2001 From: matevip Date: Fri, 1 May 2026 09:50:53 +0800 Subject: [PATCH] fix: address review comments (4 items) --- .../vip/mate/cron/service/CronJobService.java | 44 ++++++++++++- .../skill/runtime/SkillPackageResolver.java | 63 ++++++++++++++++--- .../skill/runtime/SkillRuntimeService.java | 13 ++++ .../vip/mate/skill/service/SkillService.java | 14 +++++ .../h2/V70__cron_job_dedup_safety.sql | 26 ++++++++ .../mysql/V70__cron_job_dedup_safety.sql | 16 +++++ 6 files changed, 165 insertions(+), 11 deletions(-) create mode 100644 mateclaw-server/src/main/resources/db/migration/h2/V70__cron_job_dedup_safety.sql create mode 100644 mateclaw-server/src/main/resources/db/migration/mysql/V70__cron_job_dedup_safety.sql diff --git a/mateclaw-server/src/main/java/vip/mate/cron/service/CronJobService.java b/mateclaw-server/src/main/java/vip/mate/cron/service/CronJobService.java index a1acf390..97c681f7 100644 --- a/mateclaw-server/src/main/java/vip/mate/cron/service/CronJobService.java +++ b/mateclaw-server/src/main/java/vip/mate/cron/service/CronJobService.java @@ -243,6 +243,23 @@ public class CronJobService implements ApplicationRunner { * No {@code @TableLogic} on this entity — {@code deleted=0} must be * filtered explicitly. */ + /** + * Issue #50 review #6 — excluding-self variant for the update path. + * Same lookup as {@link #findActiveDuplicate} but skips the row + * currently being edited so a no-op save (same name, same agent) + * doesn't false-positive as a duplicate. + */ + private CronJobEntity findActiveDuplicateExcluding(Long workspaceId, Long agentId, String name, Long excludeId) { + if (workspaceId == null || agentId == null || name == null) return null; + LambdaQueryWrapper q = new LambdaQueryWrapper() + .eq(CronJobEntity::getWorkspaceId, workspaceId) + .eq(CronJobEntity::getAgentId, agentId) + .eq(CronJobEntity::getName, name) + .eq(CronJobEntity::getDeleted, 0); + if (excludeId != null) q.ne(CronJobEntity::getId, excludeId); + return cronJobMapper.selectOne(q); + } + private CronJobEntity findActiveDuplicate(Long workspaceId, Long agentId, String name) { if (workspaceId == null || agentId == null || name == null) return null; return cronJobMapper.selectOne( @@ -264,6 +281,22 @@ public class CronJobService implements ApplicationRunner { validateDto(dto); String springCron = toSpringCron(dto.getCronExpression()); + // Issue #50 review #6: excluding-self duplicate check. Without + // this, renaming a job onto an existing (workspace, agent, name) + // tuple surfaces as a raw DataIntegrityViolationException from + // the V69 unique index instead of a controlled validation + // error. Try app-level check first; the catch below is the + // race-protection net. + Long newAgentId = dto.getAgentId(); + String newName = dto.getName(); + if (newName != null && newAgentId != null) { + CronJobEntity collision = findActiveDuplicateExcluding(workspaceId, newAgentId, newName, id); + if (collision != null) { + throw new MateClawException("err.cron.duplicate_name", + "已存在同名定时任务: name=" + newName + ", agentId=" + newAgentId); + } + } + existing.setName(dto.getName()); existing.setCronExpression(dto.getCronExpression()); existing.setTimezone(dto.getTimezone() != null ? dto.getTimezone() : "Asia/Shanghai"); @@ -276,7 +309,16 @@ public class CronJobService implements ApplicationRunner { } existing.setNextRunTime(calcNextRunTime(springCron, existing.getTimezone())); - cronJobMapper.updateById(existing); + try { + cronJobMapper.updateById(existing); + } catch (DuplicateKeyException e) { + // Race: another concurrent rename grabbed the natural key + // between our app-level check and the UPDATE. Translate to + // a clean validation error so the controller can 4xx instead + // of a 500 leaking the unique-key constraint name. + throw new MateClawException("err.cron.duplicate_name", + "已存在同名定时任务: name=" + dto.getName() + ", agentId=" + dto.getAgentId()); + } // 加锁保证 cancel + register 的原子性（ReentrantLock 支持同线程重入） schedulerLock.lock(); diff --git a/mateclaw-server/src/main/java/vip/mate/skill/runtime/SkillPackageResolver.java b/mateclaw-server/src/main/java/vip/mate/skill/runtime/SkillPackageResolver.java index 97fc9b56..58ad648d 100644 --- a/mateclaw-server/src/main/java/vip/mate/skill/runtime/SkillPackageResolver.java +++ b/mateclaw-server/src/main/java/vip/mate/skill/runtime/SkillPackageResolver.java @@ -555,14 +555,13 @@ public class SkillPackageResolver { if (kbId == null) { kbId = wikiWrapperFactory.resolveKbId(manifest.getKnowledge().getBindKb()); if (kbId == null) { + String slug = manifest.getKnowledge().getBindKb(); log.warn("Skill '{}' has type=knowledge but bind_kb '{}' did not resolve to a KB", - resolved.getName(), manifest.getKnowledge().getBindKb()); + resolved.getName(), slug); deregisterSkillWrappers(resolved.getId()); - // Still surface the resolution failure as a missing - // requirement so the UI shows the skill as - // SETUP_NEEDED rather than READY-but-broken. - resolved.setMissingDependencies(java.util.List.of( - "kb:" + manifest.getKnowledge().getBindKb())); + markBindingFailure(resolved, + "kb:" + slug, + "Knowledge skill bind_kb '" + slug + "' did not resolve to any Wiki KB"); return; } manifest.getKnowledge().setBoundKbId(kbId); @@ -604,7 +603,48 @@ public class SkillPackageResolver { manifest.setAllowedTools(mergedAllowed); } - private void deregisterSkillWrappers(Long skillId) { + /** + * RFC-090 review #2 — when a knowledge / acp skill's external + * binding (bind_kb / endpoint) cannot be resolved, downgrade the + * resolved view so {@code passesActiveGate} fails. Without this, + * the synthesized default feature still evaluates READY (because + * the unresolved binding isn't expressed as a manifest requirement + * the dependency checker can fail), the skill enters the active + * set, and the LLM advertises tools it can never actually use. + * + *

Triple-belt-and-braces: + *

    + *
  1. {@code missingDependencies} populated for the UI.
    2. + *
  2. {@code dependencyReady=false} so legacy gate path also fails.
    3. + *
  3. {@code runtimeAvailable=false} so the manifest-aware path + * in {@code passesActiveGate} fails too — even though the + * feature would otherwise be READY.
    4. + *
+ * The later {@code resolveRuntimeAvailability} step won't undo + * these because it only flips {@code runtimeAvailable=false}, it + * never flips it back to true. + */ + private void markBindingFailure(ResolvedSkill resolved, String missingKey, String summary) { + resolved.setMissingDependencies(java.util.List.of(missingKey)); + resolved.setDependencyReady(false); + resolved.setDependencySummary(summary); + resolved.setRuntimeAvailable(false); + if (resolved.getResolutionError() == null) { + resolved.setResolutionError(summary); + } + } + + /** + * RFC-090 review #3 — explicit deregistration entry point. Called + * from {@code SkillService.toggleSkill (disable)} / uninstall / + * hardDelete so wrapper tools don't outlive the skill's lifecycle. + * + *

Without this, the {@code availabilityCheck} supplier closes + * over the {@link ResolvedSkill} captured at registration time + * and keeps returning {@code enabled=true} forever. Stale wrapper + * advertisements survive a disable/uninstall. + */ + public void deregisterSkillWrappers(Long skillId) { if (skillId == null) return; java.util.Set previous = registeredWrappers.remove(skillId); if (previous == null || previous.isEmpty()) return; @@ -615,6 +655,7 @@ public class SkillPackageResolver { log.debug("unregister wrapper {} failed: {}", name, e.getMessage()); } } + log.info("Deregistered {} wrapper tool(s) for skill id={}", previous.size(), skillId); } /** @@ -648,11 +689,13 @@ public class SkillPackageResolver { if (endpointId == null) { endpointId = acpWrapperFactory.resolveEndpointId(manifest.getAcp().getEndpoint()); if (endpointId == null) { + String slug = manifest.getAcp().getEndpoint(); log.warn("Skill '{}' type=acp but endpoint '{}' did not resolve", - resolved.getName(), manifest.getAcp().getEndpoint()); + resolved.getName(), slug); deregisterSkillWrappers(resolved.getId()); - resolved.setMissingDependencies(java.util.List.of( - "acp:" + manifest.getAcp().getEndpoint())); + markBindingFailure(resolved, + "acp:" + slug, + "ACP skill endpoint '" + slug + "' did not resolve to any registered ACP endpoint"); return; } manifest.getAcp().setResolvedEndpointId(endpointId); diff --git a/mateclaw-server/src/main/java/vip/mate/skill/runtime/SkillRuntimeService.java b/mateclaw-server/src/main/java/vip/mate/skill/runtime/SkillRuntimeService.java index a7c758c8..24775ba6 100644 --- a/mateclaw-server/src/main/java/vip/mate/skill/runtime/SkillRuntimeService.java +++ b/mateclaw-server/src/main/java/vip/mate/skill/runtime/SkillRuntimeService.java @@ -187,6 +187,19 @@ public class SkillRuntimeService { return resolved; } + /** + * RFC-090 review #3 — explicit lifecycle hook so SkillService + * can deregister wrapper tools without poking at the resolver + * directly. Safe to call for skill ids that never had wrappers. + */ + public void deregisterSkillWrappers(Long skillId) { + try { + packageResolver.deregisterSkillWrappers(skillId); + } catch (Exception e) { + log.warn("Failed to deregister wrappers for skill {}: {}", skillId, e.getMessage()); + } + } + /** * 根据名称查找 active skill */ diff --git a/mateclaw-server/src/main/java/vip/mate/skill/service/SkillService.java b/mateclaw-server/src/main/java/vip/mate/skill/service/SkillService.java index 4be03b24..035cf65d 100644 --- a/mateclaw-server/src/main/java/vip/mate/skill/service/SkillService.java +++ b/mateclaw-server/src/main/java/vip/mate/skill/service/SkillService.java @@ -303,7 +303,11 @@ public class SkillService { if ("archive".equals(workspaceProperties.getDeletePolicy())) { workspaceManager.archiveWorkspace(skill.getName()); } + // RFC-090 review #3 — refresh won't deregister wrappers for a + // soft-deleted row (it only resolves rows still in + // listEnabledSkills), so do it explicitly here. if (runtimeService != null) { + runtimeService.deregisterSkillWrappers(id); runtimeService.refreshActiveSkills(); } } @@ -328,7 +332,9 @@ public class SkillService { workspaceManager.purgeWorkspace(skill.getName()); + // RFC-090 review #3 — same explicit deregister as uninstall. if (runtimeService != null) { + runtimeService.deregisterSkillWrappers(id); runtimeService.refreshActiveSkills(); } } @@ -357,6 +363,14 @@ public class SkillService { skillMapper.updateById(skill); log.info("Skill {} {}", skill.getName(), enabled ? "enabled" : "disabled"); + // RFC-090 review #3 — when disabling, explicitly tear down any + // registered wrapper tools (knowledge / acp). Without this the + // wrappers stay advertised because the availability supplier + // closes over the snapshot ResolvedSkill captured at registration. + if (!enabled && runtimeService != null) { + runtimeService.deregisterSkillWrappers(id); + } + // 刷新 runtime cache if (runtimeService != null) { runtimeService.refreshActiveSkills(); diff --git a/mateclaw-server/src/main/resources/db/migration/h2/V70__cron_job_dedup_safety.sql b/mateclaw-server/src/main/resources/db/migration/h2/V70__cron_job_dedup_safety.sql new file mode 100644 index 00000000..ecb039c1 --- /dev/null +++ b/mateclaw-server/src/main/resources/db/migration/h2/V70__cron_job_dedup_safety.sql @@ -0,0 +1,26 @@ +-- Issue #50 follow-up: V69 dedup grouped MIN(id) without filtering deleted=0. +-- If a (workspace_id, agent_id, name) tuple had [deleted=1 row id=100, +-- deleted=0 row id=200], V69 kept id=100 and physically removed id=200, +-- making the active job invisible to the runtime (which queries deleted=0). +-- CronJobEntity.deleted is declared but never set by current code, so +-- defensively treat any deleted=1 row as stale and remove it physically. + +-- Step 1: physically purge any deleted=1 rows. The cron service treats +-- the entity as hard-delete-only (no @TableLogic, no global logic-delete +-- config), so any deleted=1 rows are legacy artifacts and unsafe to keep. +DELETE FROM mate_cron_job WHERE deleted = 1; + +-- Step 2: idempotent re-dedup against active rows only. If V69 already +-- left the table clean, this is a no-op. If V69 picked a deleted=1 row +-- as the survivor and Step 1 just removed it, an active duplicate may +-- still need re-converging. +DELETE FROM mate_cron_job +WHERE deleted = 0 + AND id NOT IN ( + SELECT keep_id FROM ( + SELECT MIN(id) AS keep_id + FROM mate_cron_job + WHERE deleted = 0 + GROUP BY workspace_id, agent_id, name + ) + ); diff --git a/mateclaw-server/src/main/resources/db/migration/mysql/V70__cron_job_dedup_safety.sql b/mateclaw-server/src/main/resources/db/migration/mysql/V70__cron_job_dedup_safety.sql new file mode 100644 index 00000000..378378cb --- /dev/null +++ b/mateclaw-server/src/main/resources/db/migration/mysql/V70__cron_job_dedup_safety.sql @@ -0,0 +1,16 @@ +-- Issue #50 follow-up: see h2/V70 for rationale. MySQL doesn't allow +-- DELETE with a subquery scanning the same table directly, so use +-- the LEFT JOIN + IS NULL pattern. + +-- Step 1: physically purge any deleted=1 rows. +DELETE FROM mate_cron_job WHERE deleted = 1; + +-- Step 2: idempotent re-dedup against active rows only. +DELETE t FROM mate_cron_job t +LEFT JOIN ( + SELECT MIN(id) AS keep_id + FROM mate_cron_job + WHERE deleted = 0 + GROUP BY workspace_id, agent_id, name +) k ON t.id = k.keep_id +WHERE t.deleted = 0 AND k.keep_id IS NULL;