fix: address review comments (4 items)

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<CronJobEntity> q = new LambdaQueryWrapper<CronJobEntity>()
+                .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();

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
+                markBindingFailure(resolved,
-                // requirement so the UI shows the skill as
+                        "kb:" + slug,
-                // SETUP_NEEDED rather than READY-but-broken.
+                        "Knowledge skill bind_kb '" + slug + "' did not resolve to any Wiki KB");
-                resolved.setMissingDependencies(java.util.List.of(
-                        "kb:" + manifest.getKnowledge().getBindKb()));
                 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.
+     *
+     * <p>Triple-belt-and-braces:
+     * <ol>
+     *   <li>{@code missingDependencies} populated for the UI.</li>
+     *   <li>{@code dependencyReady=false} so legacy gate path also fails.</li>
+     *   <li>{@code runtimeAvailable=false} so the manifest-aware path
+     *       in {@code passesActiveGate} fails too — even though the
+     *       feature would otherwise be READY.</li>
+     * </ol>
+     * 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.
+     *
+     * <p>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<String> 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(
+                markBindingFailure(resolved,
-                        "acp:" + manifest.getAcp().getEndpoint()));
+                        "acp:" + slug,
+                        "ACP skill endpoint '" + slug + "' did not resolve to any registered ACP endpoint");
                 return;
             }
             manifest.getAcp().setResolvedEndpointId(endpointId);

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
      */

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();

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
+    )
+  );

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;