From c18ff31ae88b4df6836324d1e38de296085360d5 Mon Sep 17 00:00:00 2001 From: matevip Date: Fri, 24 Jul 2026 10:26:57 +0800 Subject: [PATCH] feat(skill): scope skill workspace filesystem paths by workspace to isolate same-named skills --- .../skill/controller/SkillController.java | 8 +- .../mate/skill/installer/SkillInstaller.java | 19 +-- .../skill/lessons/SkillLessonsService.java | 2 +- .../mate/skill/lifecycle/SkillCuratorJob.java | 2 +- .../lifecycle/SkillLifecycleService.java | 6 +- .../skill/runtime/SkillPackageResolver.java | 4 +- .../vip/mate/skill/service/SkillService.java | 10 +- .../synthesis/SkillSynthesisService.java | 2 +- .../skill/template/SkillTemplateService.java | 6 +- .../skill/workspace/BundledSkillSyncer.java | 8 +- .../mate/skill/workspace/SkillFileSyncer.java | 2 +- .../SkillWorkspaceBootstrapRunner.java | 31 ++++ .../workspace/SkillWorkspaceManager.java | 146 ++++++++++++------ .../mate/tool/builtin/SkillManageTool.java | 8 +- .../SkillControllerBundleFilesTest.java | 5 +- .../lessons/SkillLessonsServiceTest.java | 4 +- .../skill/lifecycle/SkillCuratorJobTest.java | 3 +- .../lifecycle/SkillLifecycleServiceTest.java | 17 +- .../SkillServiceUpdatePartialTest.java | 10 +- .../workspace/BundledSkillSyncerTest.java | 4 +- .../skill/workspace/SkillFileSyncerTest.java | 8 +- .../SkillWorkspaceManagerApplyBundleTest.java | 24 +-- .../SkillWorkspaceManagerPathTest.java | 99 ++++++++++-- .../builtin/SkillManageToolWriteFileTest.java | 13 +- 24 files changed, 314 insertions(+), 127 deletions(-) diff --git a/mateclaw-server/src/main/java/vip/mate/skill/controller/SkillController.java b/mateclaw-server/src/main/java/vip/mate/skill/controller/SkillController.java index cd6deb21..a6981e99 100644 --- a/mateclaw-server/src/main/java/vip/mate/skill/controller/SkillController.java +++ b/mateclaw-server/src/main/java/vip/mate/skill/controller/SkillController.java @@ -432,7 +432,7 @@ public class SkillController { SkillFileEntity row = skillFileService.upsertFile(id, normalized, content); try { - workspaceManager.writeWorkspaceFile(skill.getName(), normalized, content); + workspaceManager.writeWorkspaceFile(skill.getName(), normalized, content, skill.getWorkspaceId()); } catch (Exception e) { // Canonical store is updated; the syncer heals the cache later. } @@ -464,7 +464,7 @@ public class SkillController { } boolean removed = skillFileService.deleteFile(id, normalized); try { - workspaceManager.deleteWorkspaceFile(skill.getName(), normalized); + workspaceManager.deleteWorkspaceFile(skill.getName(), normalized, skill.getWorkspaceId()); } catch (Exception e) { // Cache cleanup is best-effort; the canonical row is gone. } @@ -956,7 +956,7 @@ public class SkillController { @RequestHeader(value = "X-Workspace-Id", required = false) Long workspaceId) { SkillEntity skill = skillService.getSkill(id); verifyResourceWorkspace(skill, workspaceId); - var path = workspaceManager.exportToWorkspace(skill.getName(), skill.getSkillContent()); + var path = workspaceManager.exportToWorkspace(skill.getName(), skill.getSkillContent(), skill.getWorkspaceId()); if (path == null) { return R.ok(Map.of("success", false, "message", "Failed to export workspace")); } @@ -970,7 +970,7 @@ public class SkillController { @RequestHeader(value = "X-Workspace-Id", required = false) Long workspaceId) { SkillEntity skill = skillService.getSkill(id); verifyResourceWorkspace(skill, workspaceId); - return R.ok(workspaceManager.getWorkspaceInfo(skill.getName())); + return R.ok(workspaceManager.getWorkspaceInfo(skill.getName(), skill.getWorkspaceId())); } // ==================== Skill lifecycle & curator ==================== diff --git a/mateclaw-server/src/main/java/vip/mate/skill/installer/SkillInstaller.java b/mateclaw-server/src/main/java/vip/mate/skill/installer/SkillInstaller.java index a3dee481..944b6dbd 100644 --- a/mateclaw-server/src/main/java/vip/mate/skill/installer/SkillInstaller.java +++ b/mateclaw-server/src/main/java/vip/mate/skill/installer/SkillInstaller.java @@ -161,10 +161,10 @@ public class SkillInstaller { } // 4. Materialize SKILL.md (overwrite on reinstall, keep on first create). - workspaceManager.initWorkspace(skillName, bundle.content(), exists); + workspaceManager.initWorkspace(skillName, bundle.content(), exists, request.getWorkspaceId()); if (task.isCancelRequested()) { - workspaceManager.archiveWorkspace(skillName); + workspaceManager.archiveWorkspace(skillName, request.getWorkspaceId()); task.markCancelled(); return CompletableFuture.completedFuture(null); } @@ -182,12 +182,12 @@ public class SkillInstaller { // Empty-bundle guard protects both sides from a malformed bundle // silently wiping pre-existing scripts/references. boolean force = Boolean.TRUE.equals(request.getForcePrune()); - persistBundleFiles(skillEntity, bundle, force, "url"); + persistBundleFiles(skillEntity, bundle, force, "url", request.getWorkspaceId()); // 7. Publish event for runtime refresh / sibling-node materialization. eventPublisher.publishEvent(new SkillWorkspaceEvent( skillName, SkillWorkspaceEvent.Type.INSTALLED, - workspaceManager.resolveConventionPath(skillName))); + workspaceManager.resolveConventionPath(skillName, request.getWorkspaceId()))); task.markCompleted(InstallResult.builder() .name(skillName) @@ -226,17 +226,17 @@ public class SkillInstaller { } // Materialize SKILL.md (always overwrite on reinstall path). - workspaceManager.initWorkspace(skillName, bundle.content(), exists); + workspaceManager.initWorkspace(skillName, bundle.content(), exists, workspaceId); // Register/update skill row first so we have an id to anchor the file rows. SkillEntity skillEntity = upsertSkillRow(bundle, skillName, exists, enable, workspaceId); // DB-canonical, FS-cache. Empty-bundle guard on both sides. - persistBundleFiles(skillEntity, bundle, false, "zip"); + persistBundleFiles(skillEntity, bundle, false, "zip", workspaceId); eventPublisher.publishEvent(new SkillWorkspaceEvent( skillName, SkillWorkspaceEvent.Type.INSTALLED, - workspaceManager.resolveConventionPath(skillName))); + workspaceManager.resolveConventionPath(skillName, workspaceId))); int filesCount = (bundle.references() != null ? bundle.references().size() : 0) + (bundle.scripts() != null ? bundle.scripts().size() : 0) + 1; @@ -296,7 +296,8 @@ public class SkillInstaller { * same prefixed-key map. Logs a single combined summary so multi-instance * deployments can see what each node persisted vs preserved. */ - private void persistBundleFiles(SkillEntity skillEntity, SkillBundle bundle, boolean force, String origin) { + private void persistBundleFiles(SkillEntity skillEntity, SkillBundle bundle, boolean force, String origin, + Long workspaceId) { Map combined = new LinkedHashMap<>(); if (bundle.references() != null) { for (var e : bundle.references().entrySet()) { @@ -313,7 +314,7 @@ public class SkillInstaller { var dbApply = skillFileService.applyBundleFiles(skillEntity.getId(), combined, force); var fsApply = workspaceManager.applyBundleFiles(skillEntity.getName(), - bundle.references(), bundle.scripts(), force); + bundle.references(), bundle.scripts(), force, workspaceId); log.info("Persisted bundle for '{}' ({}): db(write={}, prune={}, preservedScripts={}, preservedRefs={}) " + "fs(refs write={}, prune={}, preserved={} | scripts write={}, prune={}, preserved={})", diff --git a/mateclaw-server/src/main/java/vip/mate/skill/lessons/SkillLessonsService.java b/mateclaw-server/src/main/java/vip/mate/skill/lessons/SkillLessonsService.java index 1a1e88e8..14c382e9 100644 --- a/mateclaw-server/src/main/java/vip/mate/skill/lessons/SkillLessonsService.java +++ b/mateclaw-server/src/main/java/vip/mate/skill/lessons/SkillLessonsService.java @@ -205,7 +205,7 @@ public class SkillLessonsService { private Path resolveWorkspace(ResolvedSkill resolved) { if (resolved == null || resolved.getName() == null) return null; if (resolved.getSkillDir() != null) return resolved.getSkillDir(); - Path convention = workspaceManager.resolveConventionPath(resolved.getName()); + Path convention = workspaceManager.resolveConventionPath(resolved.getName(), resolved.getWorkspaceId()); return Files.exists(convention) && Files.isDirectory(convention) ? convention : null; } diff --git a/mateclaw-server/src/main/java/vip/mate/skill/lifecycle/SkillCuratorJob.java b/mateclaw-server/src/main/java/vip/mate/skill/lifecycle/SkillCuratorJob.java index 882e2895..a5bb7c8f 100644 --- a/mateclaw-server/src/main/java/vip/mate/skill/lifecycle/SkillCuratorJob.java +++ b/mateclaw-server/src/main/java/vip/mate/skill/lifecycle/SkillCuratorJob.java @@ -269,7 +269,7 @@ public class SkillCuratorJob { List archived = skillMapper.selectList(new LambdaQueryWrapper() .eq(SkillEntity::getLifecycleState, "archived")); for (SkillEntity skill : archived) { - if (skill.getName() == null || !workspaceManager.conventionWorkspaceExists(skill.getName())) { + if (skill.getName() == null || !workspaceManager.conventionWorkspaceExists(skill.getName(), skill.getWorkspaceId())) { continue; } report.reconciliation("skill '" + skill.getName() + "' (id=" + skill.getId() diff --git a/mateclaw-server/src/main/java/vip/mate/skill/lifecycle/SkillLifecycleService.java b/mateclaw-server/src/main/java/vip/mate/skill/lifecycle/SkillLifecycleService.java index be149025..b8515474 100644 --- a/mateclaw-server/src/main/java/vip/mate/skill/lifecycle/SkillLifecycleService.java +++ b/mateclaw-server/src/main/java/vip/mate/skill/lifecycle/SkillLifecycleService.java @@ -166,7 +166,7 @@ public class SkillLifecycleService { "Skill is not archived: " + skill.getName()); } - SkillWorkspaceManager.RestoreResult fs = workspaceManager.restoreWorkspace(skill.getName()); + SkillWorkspaceManager.RestoreResult fs = workspaceManager.restoreWorkspace(skill.getName(), skill.getWorkspaceId()); switch (fs) { case MOVED -> { /* normal path */ } case MISSING -> { @@ -266,7 +266,7 @@ public class SkillLifecycleService { // FAILED defers the whole transition. SkillWorkspaceManager.ArchiveResult fsResult = SkillWorkspaceManager.ArchiveResult.MISSING; if ("archive".equals(workspaceProperties.getDeletePolicy())) { - fsResult = workspaceManager.archiveWorkspace(skill.getName()); + fsResult = workspaceManager.archiveWorkspace(skill.getName(), skill.getWorkspaceId()); } if (fsResult == SkillWorkspaceManager.ArchiveResult.FAILED) { log.warn("Skill '{}' workspace archive failed; deferring DB transition", skill.getName()); @@ -288,7 +288,7 @@ public class SkillLifecycleService { if (rows == 0) { log.warn("Skill '{}' DB archive update touched 0 rows; compensating workspace", skill.getName()); if (fsResult == SkillWorkspaceManager.ArchiveResult.MOVED) { - workspaceManager.restoreWorkspace(skill.getName()); + workspaceManager.restoreWorkspace(skill.getName(), skill.getWorkspaceId()); } return false; } 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 4f78a39e..af823a94 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 @@ -149,7 +149,7 @@ public class SkillPackageResolver { resolved = resolveFromDirectory(entity, skillDir, configuredDir, "directory"); } else { // 2. 约定路径 {workspace-root}/{skillName}/ - Path conventionPath = workspaceManager.resolveConventionPath(entity.getName()); + Path conventionPath = workspaceManager.resolveConventionPath(entity.getName(), entity.getWorkspaceId()); if (Files.exists(conventionPath) && Files.isDirectory(conventionPath)) { resolved = resolveFromDirectory(entity, conventionPath, conventionPath.toString(), "convention"); } else { @@ -213,7 +213,7 @@ public class SkillPackageResolver { } } - Path convention = workspaceManager.resolveConventionPath(entity.getName()); + Path convention = workspaceManager.resolveConventionPath(entity.getName(), entity.getWorkspaceId()); if (!Files.exists(convention) || !Files.isDirectory(convention)) return false; SkillContentReconciler.Outcome outcome = contentReconciler.reconcile(entity, convention); return outcome.action() == SkillContentReconciler.Action.INGESTED_TO_DB 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 7294cfd9..6b48796d 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 @@ -361,7 +361,7 @@ public class SkillService { // 自动初始化工作区目录 if (workspaceProperties.isAutoInit() && !hasExplicitSkillDir(skill)) { - workspaceManager.initWorkspace(skill.getName(), skill.getSkillContent()); + workspaceManager.initWorkspace(skill.getName(), skill.getSkillContent(), skill.getWorkspaceId()); } // 刷新 runtime cache @@ -514,7 +514,7 @@ public class SkillService { eventPublisher.publishEvent(new SkillRemovedEvent(id, skill.getName())); if ("archive".equals(workspaceProperties.getDeletePolicy())) { - workspaceManager.archiveWorkspace(skill.getName()); + workspaceManager.archiveWorkspace(skill.getName(), skill.getWorkspaceId()); } // RFC-090 review #3 — refresh won't deregister wrappers for a // soft-deleted row (it only resolves rows still in @@ -562,7 +562,7 @@ public class SkillService { log.warn("Failed to purge secrets for skill {}: {}", skill.getName(), e.getMessage()); } - workspaceManager.purgeWorkspace(skill.getName()); + workspaceManager.purgeWorkspace(skill.getName(), skill.getWorkspaceId()); // RFC-090 review #3 — same explicit deregister as uninstall. if (runtimeService != null) { @@ -786,8 +786,8 @@ public class SkillService { if (skill.getSkillContent() == null || skill.getSkillContent().isBlank()) { return; } - if (workspaceManager.conventionWorkspaceExists(skill.getName())) { - Path workspaceDir = workspaceManager.resolveConventionPath(skill.getName()); + if (workspaceManager.conventionWorkspaceExists(skill.getName(), skill.getWorkspaceId())) { + Path workspaceDir = workspaceManager.resolveConventionPath(skill.getName(), skill.getWorkspaceId()); Path skillMd = workspaceDir.resolve("SKILL.md"); try { Files.writeString(skillMd, skill.getSkillContent()); diff --git a/mateclaw-server/src/main/java/vip/mate/skill/synthesis/SkillSynthesisService.java b/mateclaw-server/src/main/java/vip/mate/skill/synthesis/SkillSynthesisService.java index 61fc5f6b..230f448c 100644 --- a/mateclaw-server/src/main/java/vip/mate/skill/synthesis/SkillSynthesisService.java +++ b/mateclaw-server/src/main/java/vip/mate/skill/synthesis/SkillSynthesisService.java @@ -127,7 +127,7 @@ public class SkillSynthesisService { skillService.createSkill(skill); try { - workspaceManager.exportToWorkspace(name, skillMd); + workspaceManager.exportToWorkspace(name, skillMd, workspaceId); } catch (Exception e) { log.warn("[SkillSynthesis] Workspace export failed for '{}': {}", name, e.getMessage()); } diff --git a/mateclaw-server/src/main/java/vip/mate/skill/template/SkillTemplateService.java b/mateclaw-server/src/main/java/vip/mate/skill/template/SkillTemplateService.java index 6d261a64..9c70ff2f 100644 --- a/mateclaw-server/src/main/java/vip/mate/skill/template/SkillTemplateService.java +++ b/mateclaw-server/src/main/java/vip/mate/skill/template/SkillTemplateService.java @@ -106,7 +106,7 @@ public class SkillTemplateService { // classpath:{bundlePath}/** — scripts, references, fonts, etc. // Top-level SKILL.md in the bundle is skipped automatically so // the rendered manifest from step 2 stays authoritative. - overlayBundle(template, created.getName()); + overlayBundle(template, created.getName(), created.getWorkspaceId()); // 5. Persist any `secret` field values into mate_skill_secret so // the runtime can decrypt + inject them as env vars at exec @@ -138,13 +138,13 @@ public class SkillTemplateService { } } - private void overlayBundle(SkillTemplate template, String skillName) { + private void overlayBundle(SkillTemplate template, String skillName, Long workspaceId) { String bundlePath = template.getBundlePath(); if (bundlePath == null || bundlePath.isBlank()) { return; } SkillBundleSource source = new ClasspathBundleSource(resourceResolver, bundlePath); - Path workspaceDir = workspaceManager.resolveConventionPath(skillName); + Path workspaceDir = workspaceManager.resolveConventionPath(skillName, workspaceId); try { SkillBundleMaterializer.Result result = bundleMaterializer.materialize( source, workspaceDir, MaterializeOptions.templateOverlay()); diff --git a/mateclaw-server/src/main/java/vip/mate/skill/workspace/BundledSkillSyncer.java b/mateclaw-server/src/main/java/vip/mate/skill/workspace/BundledSkillSyncer.java index 1efa596b..f427a4b5 100644 --- a/mateclaw-server/src/main/java/vip/mate/skill/workspace/BundledSkillSyncer.java +++ b/mateclaw-server/src/main/java/vip/mate/skill/workspace/BundledSkillSyncer.java @@ -63,6 +63,9 @@ public class BundledSkillSyncer { private static final Pattern VERSION_PATTERN = Pattern.compile("^version:\\s*[\"']?([^\"'\\s]+)[\"']?", Pattern.MULTILINE); + /** Builtin/bundled skills are global and materialized under the default workspace. */ + private static final Long BUILTIN_WORKSPACE_ID = 1L; + private final SkillWorkspaceProperties properties; private final SkillWorkspaceManager workspaceManager; private final SkillBundleMaterializer bundleMaterializer; @@ -109,7 +112,8 @@ public class BundledSkillSyncer { */ private boolean syncOne(ResourcePatternResolver resolver, String bundledPath, String skillName, Resource manifest) { - Path targetDir = workspaceManager.resolveConventionPath(skillName); + // Builtin/bundled skills are global and seeded into workspace 1. + Path targetDir = workspaceManager.resolveConventionPath(skillName, BUILTIN_WORKSPACE_ID); boolean firstInstall = !Files.exists(targetDir); SkillBundleSource source = new ClasspathBundleSource(resolver, @@ -133,7 +137,7 @@ public class BundledSkillSyncer { skillName, missing); } // Archive (never overwrite in place) so local edits stay recoverable. - workspaceManager.archiveWorkspace(skillName); + workspaceManager.archiveWorkspace(skillName, BUILTIN_WORKSPACE_ID); } copyBundle(source, targetDir); diff --git a/mateclaw-server/src/main/java/vip/mate/skill/workspace/SkillFileSyncer.java b/mateclaw-server/src/main/java/vip/mate/skill/workspace/SkillFileSyncer.java index b5367247..488ca086 100644 --- a/mateclaw-server/src/main/java/vip/mate/skill/workspace/SkillFileSyncer.java +++ b/mateclaw-server/src/main/java/vip/mate/skill/workspace/SkillFileSyncer.java @@ -102,7 +102,7 @@ public class SkillFileSyncer { * are restored. */ public PerSkillReport syncOne(SkillEntity skill) { - Path workspaceDir = workspaceManager.resolveConventionPath(skill.getName()); + Path workspaceDir = workspaceManager.resolveConventionPath(skill.getName(), skill.getWorkspaceId()); List dbFiles = skillFileService.listBySkillId(skill.getId()); boolean didBackfill = false; diff --git a/mateclaw-server/src/main/java/vip/mate/skill/workspace/SkillWorkspaceBootstrapRunner.java b/mateclaw-server/src/main/java/vip/mate/skill/workspace/SkillWorkspaceBootstrapRunner.java index 029cda4a..a506863c 100644 --- a/mateclaw-server/src/main/java/vip/mate/skill/workspace/SkillWorkspaceBootstrapRunner.java +++ b/mateclaw-server/src/main/java/vip/mate/skill/workspace/SkillWorkspaceBootstrapRunner.java @@ -6,6 +6,8 @@ import org.springframework.boot.ApplicationArguments; import org.springframework.boot.ApplicationRunner; import org.springframework.core.annotation.Order; import org.springframework.stereotype.Component; +import vip.mate.skill.model.SkillEntity; +import vip.mate.skill.service.SkillService; import java.util.List; @@ -37,12 +39,20 @@ public class SkillWorkspaceBootstrapRunner implements ApplicationRunner { private final SkillWorkspaceManager workspaceManager; private final BundledSkillSyncer bundledSkillSyncer; private final SkillFileSyncer skillFileSyncer; + private final SkillService skillService; @Override public void run(ApplicationArguments args) { var root = workspaceManager.getWorkspaceRoot(); log.info("Skill workspace root ready: {}", root); + // Step 0 — one-time layout migration BEFORE any sync: move legacy flat + // {root}/{name} dirs into their workspace-scoped {root}/{workspaceId}/{name} + // location. Must run before skillFileSyncer.syncAll(), else the syncer + // would materialize DB content at the new scoped path first and leave the + // old flat dir (and any on-disk-only files it holds) orphaned. + migrateLegacyLayout(); + List synced = bundledSkillSyncer.sync(); if (!synced.isEmpty()) { log.info("Synced {} bundled skill(s) to workspace: {}", synced.size(), synced); @@ -57,4 +67,25 @@ public class SkillWorkspaceBootstrapRunner implements ApplicationRunner { report.filesAlreadyCurrent(), report.skillsBackfilled(), report.filesBackfilledFromDisk()); } + + /** + * Walk every persisted skill and migrate its legacy flat workspace directory + * into the workspace-scoped layout. Idempotent — once migrated, subsequent + * starts find nothing to move. + */ + private void migrateLegacyLayout() { + List skills = skillService.listSkills(); + int moved = 0; + for (SkillEntity skill : skills) { + if (skill.getName() == null) { + continue; + } + if (workspaceManager.migrateLegacyFlatDir(skill.getName(), skill.getWorkspaceId())) { + moved++; + } + } + if (moved > 0) { + log.info("Migrated {} legacy skill workspace dir(s) to the workspace-scoped layout", moved); + } + } } diff --git a/mateclaw-server/src/main/java/vip/mate/skill/workspace/SkillWorkspaceManager.java b/mateclaw-server/src/main/java/vip/mate/skill/workspace/SkillWorkspaceManager.java index 4d9de5d8..17dfdeed 100644 --- a/mateclaw-server/src/main/java/vip/mate/skill/workspace/SkillWorkspaceManager.java +++ b/mateclaw-server/src/main/java/vip/mate/skill/workspace/SkillWorkspaceManager.java @@ -47,18 +47,34 @@ public class SkillWorkspaceManager { } /** - * Resolve the conventional skill workspace path: {@code {root}/{sanitizedName}}. + * Resolve the conventional skill workspace path: + * {@code {root}/{workspaceId}/{sanitizedName}}. *

- * Deterministic in {@code skillName} alone (no filesystem-state dependency). The - * non-ASCII collision fixed in #254 comes from {@link #sanitizeNameForFs} preserving - * Unicode letters/digits, so distinct names already map to distinct directories. No - * {@code -hash} suffix is appended: skill names are charset-constrained, so two names - * sanitizing to the same string is not a real case, and keeping the bare name avoids - * changing the path scheme for every existing skill (which would orphan already-created - * workspaces with no migration). + * Scoping the path by {@code workspaceId} keeps same-named skills in different + * workspaces on disjoint disk directories, so they no longer share a workspace + * directory, overwrite each other, or cross script execution between tenants. + * A {@code null} workspaceId falls back to workspace {@code 1} (the default + * workspace) — this is a defensive fallback only; real call sites must pass the + * skill's true owning workspace. + *

+ * Deterministic in {@code (skillName, workspaceId)} alone (no filesystem-state + * dependency). The non-ASCII collision fixed in #254 comes from + * {@link #sanitizeNameForFs} preserving Unicode letters/digits, so distinct names + * already map to distinct directories. No {@code -hash} suffix is appended: skill + * names are charset-constrained, so two names sanitizing to the same string is not + * a real case. */ - public Path resolveConventionPath(String skillName) { - return getWorkspaceRoot().resolve(sanitizeNameForFs(skillName)); + public Path resolveConventionPath(String skillName, Long workspaceId) { + return workspaceScopedRoot(workspaceId).resolve(sanitizeNameForFs(skillName)); + } + + /** + * Per-workspace root: {@code {root}/{workspaceId}}. A {@code null} workspaceId + * maps to workspace {@code 1} so a missing scope never resolves to the bare root + * (which would reintroduce the cross-workspace collision this scheme prevents). + */ + private Path workspaceScopedRoot(Long workspaceId) { + return getWorkspaceRoot().resolve(String.valueOf(workspaceId == null ? 1L : workspaceId)); } /** @@ -87,7 +103,7 @@ public class SkillWorkspaceManager { *

  • null（无目录，回退数据库）
    • * */ - public Path resolveEffectivePath(String skillName, String configuredDir) { + public Path resolveEffectivePath(String skillName, String configuredDir, Long workspaceId) { // 1. 显式配置 if (configuredDir != null && !configuredDir.isBlank()) { Path explicit = Paths.get(configuredDir); @@ -96,7 +112,7 @@ public class SkillWorkspaceManager { } } // 2. 约定路径 - Path convention = resolveConventionPath(skillName); + Path convention = resolveConventionPath(skillName, workspaceId); if (Files.exists(convention) && Files.isDirectory(convention)) { return convention; } @@ -107,11 +123,53 @@ public class SkillWorkspaceManager { /** * 检查约定路径的 workspace 是否存在 */ - public boolean conventionWorkspaceExists(String skillName) { - Path convention = resolveConventionPath(skillName); + public boolean conventionWorkspaceExists(String skillName, Long workspaceId) { + Path convention = resolveConventionPath(skillName, workspaceId); return Files.exists(convention) && Files.isDirectory(convention); } + /** + * One-time layout migration: the legacy scheme stored every skill flat at + * {@code {root}/{name}}; the workspace-scoped scheme stores it at + * {@code {root}/{workspaceId}/{name}}. Move a pre-existing flat directory + * into its scoped location so the skill's scripts/references survive the + * scheme change instead of being silently orphaned (the runtime would + * otherwise fall back to DB content and lose on-disk-only files). + * + *

    Idempotent and safe to run on every startup: + *

      + *
    • no-op if the flat directory is absent (already migrated, or new install);
      • + *
    • no-op if the scoped target already exists (never overwrites);
      • + *
    • skips anything without a top-level {@code SKILL.md} so a workspace-scoped + * root like {@code {root}/1} (whose children are skills, not a skill itself) + * is never mistaken for a legacy flat skill directory.
      • + *
    + * + * @return {@code true} only when a move actually happened. + */ + public boolean migrateLegacyFlatDir(String skillName, Long workspaceId) { + Path legacy = getWorkspaceRoot().resolve(sanitizeNameForFs(skillName)); + Path scoped = resolveConventionPath(skillName, workspaceId); + if (legacy.equals(scoped)) { + return false; // scoped always adds a {workspaceId} segment; defensive only + } + if (!Files.isDirectory(legacy) || !Files.exists(legacy.resolve("SKILL.md"))) { + return false; // absent, or not a real flat skill workspace + } + if (Files.exists(scoped)) { + return false; // already migrated / target present — never overwrite + } + try { + Files.createDirectories(scoped.getParent()); + Files.move(legacy, scoped, StandardCopyOption.ATOMIC_MOVE); + log.info("Migrated legacy skill workspace {} -> {}", legacy, scoped); + return true; + } catch (IOException e) { + log.warn("Failed to migrate legacy skill workspace {} -> {}: {}", legacy, scoped, e.getMessage()); + return false; + } + } + // ==================== 生命周期操作 ==================== /** @@ -121,8 +179,8 @@ public class SkillWorkspaceManager { * @param initialContent SKILL.md 初始内容（可为 null） * @return 创建的工作区路径 */ - public Path initWorkspace(String skillName, String initialContent) { - return initWorkspace(skillName, initialContent, false); + public Path initWorkspace(String skillName, String initialContent, Long workspaceId) { + return initWorkspace(skillName, initialContent, false, workspaceId); } /** @@ -134,8 +192,8 @@ public class SkillWorkspaceManager { * false 时仅在 SKILL.md 不存在时写入（用于首次创建） * @return 创建的工作区路径 */ - public Path initWorkspace(String skillName, String initialContent, boolean overwrite) { - Path workspaceDir = resolveConventionPath(skillName); + public Path initWorkspace(String skillName, String initialContent, boolean overwrite, Long workspaceId) { + Path workspaceDir = resolveConventionPath(skillName, workspaceId); try { Files.createDirectories(workspaceDir); Files.createDirectories(workspaceDir.resolve("references")); @@ -170,8 +228,8 @@ public class SkillWorkspaceManager { * by hard-delete only; uninstall still calls * {@link #archiveWorkspace} so users can recover by re-installing. */ - public void purgeWorkspace(String skillName) { - Path workspaceDir = resolveConventionPath(skillName); + public void purgeWorkspace(String skillName, Long workspaceId) { + Path workspaceDir = resolveConventionPath(skillName, workspaceId); if (!Files.exists(workspaceDir)) return; try { // Walk and delete bottom-up so non-empty dirs go away too. @@ -218,7 +276,8 @@ public class SkillWorkspaceManager { } /** - * Move {@code {root}/{name}/} to {@code {root}/.archived/{name}-{ts}/}. + * Move {@code {root}/{workspaceId}/{name}/} to + * {@code {root}/{workspaceId}/.archived/{name}-{ts}/}. * *

    Returns {@link ArchiveResult#MISSING} when the workspace doesn't * exist — callers may treat this as a successful no-op since the runtime @@ -228,14 +287,14 @@ public class SkillWorkspaceManager { * {@link ArchiveResult#MOVED} on success, having already published * {@link SkillWorkspaceEvent.Type#ARCHIVED}. */ - public ArchiveResult archiveWorkspace(String skillName) { - Path workspaceDir = resolveConventionPath(skillName); + public ArchiveResult archiveWorkspace(String skillName, Long workspaceId) { + Path workspaceDir = resolveConventionPath(skillName, workspaceId); if (!Files.exists(workspaceDir)) { return ArchiveResult.MISSING; } try { - Path archiveRoot = getWorkspaceRoot().resolve(".archived"); + Path archiveRoot = workspaceScopedRoot(workspaceId).resolve(".archived"); Files.createDirectories(archiveRoot); String archiveName = sanitizeName(skillName) + "-" + LocalDateTime.now().format(ARCHIVE_TS); @@ -261,13 +320,13 @@ public class SkillWorkspaceManager { * {@link RestoreResult#FAILED} when an archive directory exists but the * move-back fails. */ - public RestoreResult restoreWorkspace(String skillName) { - Path target = resolveConventionPath(skillName); + public RestoreResult restoreWorkspace(String skillName, Long workspaceId) { + Path target = resolveConventionPath(skillName, workspaceId); if (Files.exists(target)) { log.warn("restoreWorkspace skipped: target {} already exists", target); return RestoreResult.MISSING; } - Path archiveRoot = getWorkspaceRoot().resolve(".archived"); + Path archiveRoot = workspaceScopedRoot(workspaceId).resolve(".archived"); if (!Files.exists(archiveRoot)) { return RestoreResult.MISSING; } @@ -313,9 +372,9 @@ public class SkillWorkspaceManager { /** * 将数据库 skill 内容导出到工作区目录 */ - public Path exportToWorkspace(String skillName, String skillContent) { + public Path exportToWorkspace(String skillName, String skillContent, Long workspaceId) { // 始终覆写 SKILL.md（initWorkspace 内部已写入），无需再做一次冗余 IO - Path workspaceDir = initWorkspace(skillName, skillContent, true); + Path workspaceDir = initWorkspace(skillName, skillContent, true, workspaceId); if (workspaceDir != null) { log.info("Exported skill '{}' to workspace: {}", skillName, workspaceDir); eventPublisher.publishEvent(new SkillWorkspaceEvent(skillName, SkillWorkspaceEvent.Type.EXPORTED, workspaceDir)); @@ -338,8 +397,8 @@ public class SkillWorkspaceManager { * @param content 文件内容 * @throws IllegalArgumentException 如果路径不安全 */ - public void writeWorkspaceFile(String skillName, String relativePath, String content) { - Path workspaceDir = resolveConventionPath(skillName); + public void writeWorkspaceFile(String skillName, String relativePath, String content, Long workspaceId) { + Path workspaceDir = resolveConventionPath(skillName, workspaceId); // 路径安全校验 Path safePath = validateWritePath(workspaceDir, relativePath); @@ -363,8 +422,8 @@ public class SkillWorkspaceManager { * * @throws IllegalArgumentException 如果路径不安全 */ - public void deleteWorkspaceFile(String skillName, String relativePath) { - Path workspaceDir = resolveConventionPath(skillName); + public void deleteWorkspaceFile(String skillName, String relativePath, Long workspaceId) { + Path workspaceDir = resolveConventionPath(skillName, workspaceId); Path safePath = validateWritePath(workspaceDir, relativePath); if (safePath == null) { throw new IllegalArgumentException("Unsafe file path rejected: " + relativePath); @@ -394,8 +453,8 @@ public class SkillWorkspaceManager { * 清空 skill 工作区中的 references/ 和 scripts/ 目录内容（保留目录本身） * 用于 overwrite 安装前清除旧版本残留文件 */ - public void cleanWorkspaceDataDirs(String skillName) { - Path workspaceDir = resolveConventionPath(skillName); + public void cleanWorkspaceDataDirs(String skillName, Long workspaceId) { + Path workspaceDir = resolveConventionPath(skillName, workspaceId); cleanDirectoryContents(workspaceDir.resolve("references")); cleanDirectoryContents(workspaceDir.resolve("scripts")); } @@ -439,8 +498,9 @@ public class SkillWorkspaceManager { public ApplyBundleResult applyBundleFiles(String skillName, Map references, Map scripts, - boolean force) { - Path workspaceDir = resolveConventionPath(skillName); + boolean force, + Long workspaceId) { + Path workspaceDir = resolveConventionPath(skillName, workspaceId); try { Files.createDirectories(workspaceDir.resolve("references")); Files.createDirectories(workspaceDir.resolve("scripts")); @@ -448,8 +508,8 @@ public class SkillWorkspaceManager { log.warn("Failed to ensure data dirs for skill '{}': {}", skillName, e.getMessage()); } - int refsWritten = applyBucket(skillName, "references/", references); - int scriptsWritten = applyBucket(skillName, "scripts/", scripts); + int refsWritten = applyBucket(skillName, "references/", references, workspaceId); + int scriptsWritten = applyBucket(skillName, "scripts/", scripts, workspaceId); var refsPrune = pruneBucket(workspaceDir.resolve("references"), normalizeKeys(references), force, skillName, "references"); @@ -462,14 +522,14 @@ public class SkillWorkspaceManager { ); } - private int applyBucket(String skillName, String bucketPrefix, Map entries) { + private int applyBucket(String skillName, String bucketPrefix, Map entries, Long workspaceId) { if (entries == null || entries.isEmpty()) return 0; int written = 0; for (var e : entries.entrySet()) { String key = e.getKey(); String relative = key.startsWith(bucketPrefix) ? key : (bucketPrefix + key); try { - writeWorkspaceFile(skillName, relative, e.getValue()); + writeWorkspaceFile(skillName, relative, e.getValue(), workspaceId); written++; } catch (RuntimeException ex) { log.warn("Failed to write {} for skill '{}': {}", relative, skillName, ex.getMessage()); @@ -618,8 +678,8 @@ public class SkillWorkspaceManager { /** * 获取 skill 工作区信息 */ - public Map getWorkspaceInfo(String skillName) { - Path workspaceDir = resolveConventionPath(skillName); + public Map getWorkspaceInfo(String skillName, Long workspaceId) { + Path workspaceDir = resolveConventionPath(skillName, workspaceId); Map info = new LinkedHashMap<>(); info.put("skillName", skillName); info.put("conventionPath", workspaceDir.toString()); diff --git a/mateclaw-server/src/main/java/vip/mate/tool/builtin/SkillManageTool.java b/mateclaw-server/src/main/java/vip/mate/tool/builtin/SkillManageTool.java index 3d8518bc..38e1e3a1 100644 --- a/mateclaw-server/src/main/java/vip/mate/tool/builtin/SkillManageTool.java +++ b/mateclaw-server/src/main/java/vip/mate/tool/builtin/SkillManageTool.java @@ -210,7 +210,7 @@ public class SkillManageTool { // 同步到 workspace 文件系统 try { - workspaceManager.exportToWorkspace(name, content); + workspaceManager.exportToWorkspace(name, content, skill.getWorkspaceId()); } catch (Exception e) { log.warn("[SkillManage] Workspace export failed for '{}': {}", name, e.getMessage()); } @@ -254,7 +254,7 @@ public class SkillManageTool { skillService.updateSkill(existing); try { - workspaceManager.exportToWorkspace(name, content); + workspaceManager.exportToWorkspace(name, content, existing.getWorkspaceId()); } catch (Exception e) { log.warn("[SkillManage] Workspace export failed for '{}': {}", name, e.getMessage()); } @@ -335,7 +335,7 @@ public class SkillManageTool { skillService.updateSkill(existing); try { - workspaceManager.exportToWorkspace(name, patchedContent); + workspaceManager.exportToWorkspace(name, patchedContent, existing.getWorkspaceId()); } catch (Exception e) { log.warn("[SkillManage] Workspace export failed for '{}': {}", name, e.getMessage()); } @@ -385,7 +385,7 @@ public class SkillManageTool { } try { - workspaceManager.writeWorkspaceFile(name, filePath, content); + workspaceManager.writeWorkspaceFile(name, filePath, content, existing.getWorkspaceId()); } catch (IllegalArgumentException e) { return "Error: " + e.getMessage() + " (paths must start with references/, scripts/ or templates/, and may not contain '..')."; diff --git a/mateclaw-server/src/test/java/vip/mate/skill/controller/SkillControllerBundleFilesTest.java b/mateclaw-server/src/test/java/vip/mate/skill/controller/SkillControllerBundleFilesTest.java index 39f8b291..8193139e 100644 --- a/mateclaw-server/src/test/java/vip/mate/skill/controller/SkillControllerBundleFilesTest.java +++ b/mateclaw-server/src/test/java/vip/mate/skill/controller/SkillControllerBundleFilesTest.java @@ -54,6 +54,7 @@ class SkillControllerBundleFilesTest { private SkillEntity skill(boolean builtin) { SkillEntity s = new SkillEntity(); s.setId(SID); + s.setWorkspaceId(1L); s.setName("demo-skill"); s.setBuiltin(builtin); return s; @@ -131,7 +132,7 @@ class SkillControllerBundleFilesTest { assertThat(resp.getData()).containsEntry("path", "templates/report.html"); verify(fileService).upsertFile(SID, "templates/report.html", ""); - verify(workspaceManager).writeWorkspaceFile("demo-skill", "templates/report.html", ""); + verify(workspaceManager).writeWorkspaceFile("demo-skill", "templates/report.html", "", 1L); verify(runtimeService).rescanSingle(any(SkillEntity.class)); } @@ -170,7 +171,7 @@ class SkillControllerBundleFilesTest { R> resp = controller.deleteBundleFile(SID, "scripts/run.py", null); assertThat(resp.getData()).containsEntry("removed", true); - verify(workspaceManager).deleteWorkspaceFile("demo-skill", "scripts/run.py"); + verify(workspaceManager).deleteWorkspaceFile("demo-skill", "scripts/run.py", 1L); verify(runtimeService).rescanSingle(any(SkillEntity.class)); } diff --git a/mateclaw-server/src/test/java/vip/mate/skill/lessons/SkillLessonsServiceTest.java b/mateclaw-server/src/test/java/vip/mate/skill/lessons/SkillLessonsServiceTest.java index b579790b..e82ce403 100644 --- a/mateclaw-server/src/test/java/vip/mate/skill/lessons/SkillLessonsServiceTest.java +++ b/mateclaw-server/src/test/java/vip/mate/skill/lessons/SkillLessonsServiceTest.java @@ -48,7 +48,7 @@ class SkillLessonsServiceTest { workspaceManager = mock(SkillWorkspaceManager.class); publishedEvents = new ArrayList<>(); publisher = event -> publishedEvents.add(event); - when(workspaceManager.resolveConventionPath(anyString())) + when(workspaceManager.resolveConventionPath(anyString(), any())) .thenAnswer(inv -> tempDir.resolve(inv.getArgument(0, String.class))); service = new SkillLessonsService(workspaceManager, publisher); } @@ -141,7 +141,7 @@ class SkillLessonsServiceTest { void noWorkspaceNoOp() { ResolvedSkill skill = ResolvedSkill.builder().id(1L).name("nope").build(); // Force a non-existent convention path so resolveWorkspace returns null. - when(workspaceManager.resolveConventionPath("nope")) + when(workspaceManager.resolveConventionPath("nope", null)) .thenReturn(tempDir.resolve("does-not-exist")); String id = service.recordLesson(skill, null, null, "won't write", 50); assertNull(id); diff --git a/mateclaw-server/src/test/java/vip/mate/skill/lifecycle/SkillCuratorJobTest.java b/mateclaw-server/src/test/java/vip/mate/skill/lifecycle/SkillCuratorJobTest.java index 93dacba6..2e3c449f 100644 --- a/mateclaw-server/src/test/java/vip/mate/skill/lifecycle/SkillCuratorJobTest.java +++ b/mateclaw-server/src/test/java/vip/mate/skill/lifecycle/SkillCuratorJobTest.java @@ -78,6 +78,7 @@ class SkillCuratorJobTest { private SkillEntity candidate(long id, String state, LocalDateTime lastActivity) { SkillEntity s = new SkillEntity(); s.setId(id); + s.setWorkspaceId(1L); s.setName("skill-" + id); s.setSkillType("dynamic"); s.setBuiltin(false); @@ -205,7 +206,7 @@ class SkillCuratorJobTest { SkillEntity orphan = candidate(9L, "archived", now.minusDays(100)); // 1st selectList = reconcile (archived rows); 2nd = loadCandidates. when(skillMapper.selectList(any())).thenReturn(List.of(orphan), List.of()); - when(workspaceManager.conventionWorkspaceExists("skill-9")).thenReturn(true); + when(workspaceManager.conventionWorkspaceExists("skill-9", 1L)).thenReturn(true); job.run(); diff --git a/mateclaw-server/src/test/java/vip/mate/skill/lifecycle/SkillLifecycleServiceTest.java b/mateclaw-server/src/test/java/vip/mate/skill/lifecycle/SkillLifecycleServiceTest.java index f817dc80..0e77e2e2 100644 --- a/mateclaw-server/src/test/java/vip/mate/skill/lifecycle/SkillLifecycleServiceTest.java +++ b/mateclaw-server/src/test/java/vip/mate/skill/lifecycle/SkillLifecycleServiceTest.java @@ -71,6 +71,7 @@ class SkillLifecycleServiceTest { private SkillEntity skill(String type, String state, LocalDateTime lastActivity) { SkillEntity s = new SkillEntity(); s.setId(1L); + s.setWorkspaceId(1L); s.setName("demo-skill"); s.setSkillType(type); s.setBuiltin(false); @@ -175,7 +176,7 @@ class SkillLifecycleServiceTest { @Test void restoreMovesWorkspaceBackAndFlipsTheRow() { when(skillMapper.selectById(1L)).thenReturn(archivedSkill()); - when(workspaceManager.restoreWorkspace("demo-skill")) + when(workspaceManager.restoreWorkspace("demo-skill", 1L)) .thenReturn(SkillWorkspaceManager.RestoreResult.MOVED); service.restore(1L); @@ -187,7 +188,7 @@ class SkillLifecycleServiceTest { @Test void restoreDbOnlySkillFlipsRowWithoutWorkspace() { when(skillMapper.selectById(1L)).thenReturn(archivedSkill()); - when(workspaceManager.restoreWorkspace("demo-skill")) + when(workspaceManager.restoreWorkspace("demo-skill", 1L)) .thenReturn(SkillWorkspaceManager.RestoreResult.MISSING); service.restore(1L); @@ -200,7 +201,7 @@ class SkillLifecycleServiceTest { SkillEntity s = archivedSkill(); s.setSkillContent(" "); when(skillMapper.selectById(1L)).thenReturn(s); - when(workspaceManager.restoreWorkspace("demo-skill")) + when(workspaceManager.restoreWorkspace("demo-skill", 1L)) .thenReturn(SkillWorkspaceManager.RestoreResult.MISSING); assertThrows(MateClawException.class, () -> service.restore(1L)); @@ -210,7 +211,7 @@ class SkillLifecycleServiceTest { @Test void restoreRejectsWhenWorkspaceMoveBackFails() { when(skillMapper.selectById(1L)).thenReturn(archivedSkill()); - when(workspaceManager.restoreWorkspace("demo-skill")) + when(workspaceManager.restoreWorkspace("demo-skill", 1L)) .thenReturn(SkillWorkspaceManager.RestoreResult.FAILED); assertThrows(MateClawException.class, () -> service.restore(1L)); @@ -234,7 +235,7 @@ class SkillLifecycleServiceTest { @Test void archiveDefersWhenWorkspaceMoveFails() { SkillEntity s = skill("dynamic", "stale", now.minusDays(100)); - when(workspaceManager.archiveWorkspace(anyString())) + when(workspaceManager.archiveWorkspace(anyString(), any())) .thenReturn(SkillWorkspaceManager.ArchiveResult.FAILED); boolean applied = service.apply(s, LifecycleTransition.TO_ARCHIVED, now); @@ -246,7 +247,7 @@ class SkillLifecycleServiceTest { @Test void archiveCommitsForDbOnlySkillWithNoWorkspace() { SkillEntity s = skill("dynamic", "stale", now.minusDays(100)); - when(workspaceManager.archiveWorkspace(anyString())) + when(workspaceManager.archiveWorkspace(anyString(), any())) .thenReturn(SkillWorkspaceManager.ArchiveResult.MISSING); when(skillMapper.update(any(), any())).thenReturn(1); @@ -260,13 +261,13 @@ class SkillLifecycleServiceTest { @Test void archiveCompensatesWorkspaceWhenDbWriteTouchesNoRows() { SkillEntity s = skill("dynamic", "stale", now.minusDays(100)); - when(workspaceManager.archiveWorkspace(anyString())) + when(workspaceManager.archiveWorkspace(anyString(), any())) .thenReturn(SkillWorkspaceManager.ArchiveResult.MOVED); when(skillMapper.update(any(), any())).thenReturn(0); boolean applied = service.apply(s, LifecycleTransition.TO_ARCHIVED, now); assertFalse(applied); - verify(workspaceManager).restoreWorkspace("demo-skill"); + verify(workspaceManager).restoreWorkspace("demo-skill", 1L); } } diff --git a/mateclaw-server/src/test/java/vip/mate/skill/service/SkillServiceUpdatePartialTest.java b/mateclaw-server/src/test/java/vip/mate/skill/service/SkillServiceUpdatePartialTest.java index 094c0b3d..fe04a0d1 100644 --- a/mateclaw-server/src/test/java/vip/mate/skill/service/SkillServiceUpdatePartialTest.java +++ b/mateclaw-server/src/test/java/vip/mate/skill/service/SkillServiceUpdatePartialTest.java @@ -17,6 +17,7 @@ import java.nio.file.Path; import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; @@ -67,6 +68,7 @@ class SkillServiceUpdatePartialTest { SkillEntity existing = new SkillEntity(); existing.setId(101L); + existing.setWorkspaceId(1L); existing.setName("docx"); existing.setDescription("placeholder"); existing.setSkillType("dynamic"); @@ -86,8 +88,8 @@ class SkillServiceUpdatePartialTest { Path tempRoot = Files.createTempDirectory("skill-svc-test"); Path skillDir = tempRoot.resolve("docx"); Files.createDirectories(skillDir); - when(workspaceManager.conventionWorkspaceExists("docx")).thenReturn(true); - when(workspaceManager.resolveConventionPath("docx")).thenReturn(skillDir); + when(workspaceManager.conventionWorkspaceExists("docx", 1L)).thenReturn(true); + when(workspaceManager.resolveConventionPath("docx", 1L)).thenReturn(skillDir); // What the controller deserializes from the partial PUT body: // only id + skillContent + sourceCode. @@ -121,7 +123,7 @@ class SkillServiceUpdatePartialTest { "skill_content from the partial PUT must be applied"); // Workspace sync runs — using the merged name, not the partial null. - verify(workspaceManager).conventionWorkspaceExists("docx"); + verify(workspaceManager).conventionWorkspaceExists("docx", 1L); // Best-effort cleanup of the temp workspace. Files.deleteIfExists(skillDir.resolve("SKILL.md")); @@ -152,7 +154,7 @@ class SkillServiceUpdatePartialTest { existing.setBuiltin(false); existing.setSkillContent("---\nname: notes\n---\n# previously authored body\n"); when(mapper.selectById(202L)).thenReturn(existing); - when(workspaceManager.conventionWorkspaceExists(anyString())).thenReturn(false); + when(workspaceManager.conventionWorkspaceExists(anyString(), any())).thenReturn(false); // Identity edit: nameZh / description only — skill_content is // never touched and must survive. diff --git a/mateclaw-server/src/test/java/vip/mate/skill/workspace/BundledSkillSyncerTest.java b/mateclaw-server/src/test/java/vip/mate/skill/workspace/BundledSkillSyncerTest.java index 81738485..207432b6 100644 --- a/mateclaw-server/src/test/java/vip/mate/skill/workspace/BundledSkillSyncerTest.java +++ b/mateclaw-server/src/test/java/vip/mate/skill/workspace/BundledSkillSyncerTest.java @@ -83,7 +83,7 @@ class BundledSkillSyncerTest { @Test @DisplayName("Sync self-heals a workspace missing scripts/ despite unchanged version") void syncForcesDiskCopyWhenScriptsDirMissing() throws IOException { - Path pptxDir = tmp.resolve("pptx"); + Path pptxDir = tmp.resolve("1").resolve("pptx"); Files.createDirectories(pptxDir); // Same SKILL.md as the bundle (identical version) but no scripts/ // directory — the state left behind by a build that shipped without @@ -104,7 +104,7 @@ class BundledSkillSyncerTest { assertTrue(synced.contains("pptx"), "Should re-sync when scripts directory is missing"); assertTrue(Files.isDirectory(pptxDir.resolve("scripts")), "scripts directory should now be on disk"); verify(skillFileService, atLeastOnce()).applyBundleFiles(eq(100L), anyMap(), eq(false)); - try (var archived = Files.list(tmp.resolve(".archived"))) { + try (var archived = Files.list(tmp.resolve("1").resolve(".archived"))) { assertTrue(archived.anyMatch(p -> p.getFileName().toString().startsWith("pptx-")), "old workspace should be archived, not overwritten in place"); } diff --git a/mateclaw-server/src/test/java/vip/mate/skill/workspace/SkillFileSyncerTest.java b/mateclaw-server/src/test/java/vip/mate/skill/workspace/SkillFileSyncerTest.java index 5ed3a43e..a4dc1bf2 100644 --- a/mateclaw-server/src/test/java/vip/mate/skill/workspace/SkillFileSyncerTest.java +++ b/mateclaw-server/src/test/java/vip/mate/skill/workspace/SkillFileSyncerTest.java @@ -70,7 +70,7 @@ class SkillFileSyncerTest { var report = syncer.syncAll(); - Path workspace = tmp.resolve("demo"); + Path workspace = tmp.resolve("1").resolve("demo"); assertEquals("print('a')\n", Files.readString(workspace.resolve("scripts/run.py"))); assertEquals("hello", Files.readString(workspace.resolve("references/notes.md"))); assertEquals(2, report.filesMaterialized()); @@ -84,7 +84,7 @@ class SkillFileSyncerTest { SkillEntity skill = newSkill(10L, "demo"); when(skillService.listSkills()).thenReturn(List.of(skill)); - Path workspace = tmp.resolve("demo"); + Path workspace = tmp.resolve("1").resolve("demo"); Files.createDirectories(workspace.resolve("scripts")); Files.writeString(workspace.resolve("scripts/run.py"), "stable"); @@ -104,7 +104,7 @@ class SkillFileSyncerTest { SkillEntity skill = newSkill(10L, "demo"); when(skillService.listSkills()).thenReturn(List.of(skill)); - Path workspace = tmp.resolve("demo"); + Path workspace = tmp.resolve("1").resolve("demo"); Files.createDirectories(workspace.resolve("scripts")); Files.createDirectories(workspace.resolve("references")); Files.writeString(workspace.resolve("scripts/run.py"), "legacy"); @@ -152,6 +152,8 @@ class SkillFileSyncerTest { SkillEntity s = new SkillEntity(); s.setId(id); s.setName(name); + // Workspace-scoped FS layout: the syncer resolves {root}/{workspaceId}/{name}. + s.setWorkspaceId(1L); return s; } diff --git a/mateclaw-server/src/test/java/vip/mate/skill/workspace/SkillWorkspaceManagerApplyBundleTest.java b/mateclaw-server/src/test/java/vip/mate/skill/workspace/SkillWorkspaceManagerApplyBundleTest.java index ccf7fea6..54b02a98 100644 --- a/mateclaw-server/src/test/java/vip/mate/skill/workspace/SkillWorkspaceManagerApplyBundleTest.java +++ b/mateclaw-server/src/test/java/vip/mate/skill/workspace/SkillWorkspaceManagerApplyBundleTest.java @@ -36,20 +36,21 @@ class SkillWorkspaceManagerApplyBundleTest { props.setRoot(tmp.toString()); ApplicationEventPublisher publisher = mock(ApplicationEventPublisher.class); manager = new SkillWorkspaceManager(props, publisher); - manager.initWorkspace(skill, "---\nname: demo\n---\nbody\n"); + manager.initWorkspace(skill, "---\nname: demo\n---\nbody\n", 1L); } @Test @DisplayName("write-then-prune: new files added, removed files pruned") void writeThenPruneNormalCase() throws IOException { - Path scripts = tmp.resolve(skill).resolve("scripts"); + Path scripts = tmp.resolve("1").resolve(skill).resolve("scripts"); Files.writeString(scripts.resolve("old.py"), "old"); Files.writeString(scripts.resolve("keep.py"), "v1"); var result = manager.applyBundleFiles(skill, Map.of(), Map.of("keep.py", "v2", "new.py", "fresh"), - false); + false, + 1L); assertEquals(2, result.scriptsWritten()); assertEquals(1, result.scriptsPruned(), "old.py should be pruned"); @@ -62,14 +63,15 @@ class SkillWorkspaceManagerApplyBundleTest { @Test @DisplayName("empty-bundle guard: existing scripts preserved when new bundle has none") void emptyBundleGuardPreservesExistingScripts() throws IOException { - Path scripts = tmp.resolve(skill).resolve("scripts"); + Path scripts = tmp.resolve("1").resolve(skill).resolve("scripts"); Files.writeString(scripts.resolve("run.py"), "important"); Files.writeString(scripts.resolve("helper.py"), "more important"); var result = manager.applyBundleFiles(skill, Map.of("notes.md", "ref"), Map.of(), // empty scripts — simulates the issue #104 extractor bug - false); + false, + 1L); assertEquals(0, result.scriptsWritten()); assertEquals(0, result.scriptsPruned()); @@ -83,13 +85,14 @@ class SkillWorkspaceManagerApplyBundleTest { @Test @DisplayName("force=true bypasses empty-bundle guard and prunes everything") void forceFlagPrunesEvenWhenBundleEmpty() throws IOException { - Path scripts = tmp.resolve(skill).resolve("scripts"); + Path scripts = tmp.resolve("1").resolve(skill).resolve("scripts"); Files.writeString(scripts.resolve("doomed.py"), "x"); var result = manager.applyBundleFiles(skill, Map.of(), Map.of(), - true); + true, + 1L); assertFalse(result.scriptsPreservedDueToEmptyBundle()); assertEquals(1, result.scriptsPruned()); @@ -99,15 +102,16 @@ class SkillWorkspaceManagerApplyBundleTest { @Test @DisplayName("references and scripts buckets prune independently") void bucketsAreIndependent() throws IOException { - Path scripts = tmp.resolve(skill).resolve("scripts"); - Path references = tmp.resolve(skill).resolve("references"); + Path scripts = tmp.resolve("1").resolve(skill).resolve("scripts"); + Path references = tmp.resolve("1").resolve(skill).resolve("references"); Files.writeString(scripts.resolve("run.py"), "stay-on-disk"); Files.writeString(references.resolve("notes.md"), "stale-ref"); var result = manager.applyBundleFiles(skill, Map.of("notes.md", "fresh-ref"), Map.of(), // empty scripts → preserved - false); + false, + 1L); assertTrue(result.scriptsPreservedDueToEmptyBundle()); assertFalse(result.referencesPreservedDueToEmptyBundle()); diff --git a/mateclaw-server/src/test/java/vip/mate/skill/workspace/SkillWorkspaceManagerPathTest.java b/mateclaw-server/src/test/java/vip/mate/skill/workspace/SkillWorkspaceManagerPathTest.java index df762dd4..9cd4cfe0 100644 --- a/mateclaw-server/src/test/java/vip/mate/skill/workspace/SkillWorkspaceManagerPathTest.java +++ b/mateclaw-server/src/test/java/vip/mate/skill/workspace/SkillWorkspaceManagerPathTest.java @@ -5,19 +5,26 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; import org.springframework.context.ApplicationEventPublisher; +import java.io.IOException; +import java.nio.file.Files; import java.nio.file.Path; import static org.junit.jupiter.api.Assertions.*; import static org.mockito.Mockito.mock; /** - * Regression tests for {@link SkillWorkspaceManager#resolveConventionPath}. + * Tests for {@link SkillWorkspaceManager#resolveConventionPath} and the + * workspace-scoped layout. * *

    Issue #254: non-ASCII (e.g. Chinese) skill names collapsed to underscores in the * workspace path, so distinct names resolved to the same directory and overwrote each * other. The fix preserves Unicode letters/digits so distinct names map to distinct - * directories. The path is the bare sanitized name (no {@code -hash} suffix), so ASCII - * workspace paths stay stable across upgrades. + * directories. + * + *

    Workspace isolation: the path is scoped by {@code workspaceId} + * ({@code {root}/{workspaceId}/{name}}) so two skills with the SAME name in DIFFERENT + * workspaces no longer share a directory. {@link #migrateLegacyFlatDir} moves a + * pre-existing flat {@code {root}/{name}} directory into its scoped location. */ class SkillWorkspaceManagerPathTest { @@ -34,24 +41,96 @@ class SkillWorkspaceManagerPathTest { @DisplayName("distinct non-ASCII names resolve to distinct directories (no collision)") void nonAsciiNamesDoNotCollide() { SkillWorkspaceManager m = newManager(); - Path a = m.resolveConventionPath("我的技能"); - Path b = m.resolveConventionPath("你的技能"); + Path a = m.resolveConventionPath("我的技能", 1L); + Path b = m.resolveConventionPath("你的技能", 1L); assertNotEquals(a, b, "Chinese names must not collapse to the same directory"); assertTrue(a.getFileName().toString().contains("我的技能"), "Unicode letters must be preserved"); assertTrue(b.getFileName().toString().contains("你的技能"), "Unicode letters must be preserved"); } @Test - @DisplayName("ASCII name maps to the bare sanitized name with no -hash suffix") - void asciiNameKeepsBarePath() { + @DisplayName("ASCII name maps to {root}/{workspaceId}/{name}") + void asciiNameScopedByWorkspace() { SkillWorkspaceManager m = newManager(); - assertEquals(tmp.resolve("my-skill"), m.resolveConventionPath("my-skill")); + assertEquals(tmp.resolve("1").resolve("my-skill"), m.resolveConventionPath("my-skill", 1L)); } @Test - @DisplayName("path is deterministic for the same name") + @DisplayName("path is deterministic for the same (name, workspaceId)") void deterministicForSameName() { SkillWorkspaceManager m = newManager(); - assertEquals(m.resolveConventionPath("demo"), m.resolveConventionPath("demo")); + assertEquals(m.resolveConventionPath("demo", 1L), m.resolveConventionPath("demo", 1L)); + } + + // ─── Workspace isolation ──────────────────────────────────────────────── + + @Test + @DisplayName("same name in different workspaces resolves to disjoint directories") + void sameNameDifferentWorkspacesAreDisjoint() { + SkillWorkspaceManager m = newManager(); + Path ws1 = m.resolveConventionPath("预约会议", 1L); + Path ws2 = m.resolveConventionPath("预约会议", 2055554700078690305L); + assertNotEquals(ws1, ws2, "same-named skills in different workspaces must not share a directory"); + assertTrue(ws1.startsWith(tmp.resolve("1")), "workspace-1 skill lives under {root}/1"); + assertTrue(ws2.startsWith(tmp.resolve("2055554700078690305")), "workspace-2 skill lives under {root}/"); + } + + @Test + @DisplayName("null workspaceId falls back to workspace 1 (defensive only)") + void nullWorkspaceIdFallsBackToOne() { + SkillWorkspaceManager m = newManager(); + assertEquals(m.resolveConventionPath("demo", 1L), m.resolveConventionPath("demo", null)); + } + + // ─── Legacy layout migration ──────────────────────────────────────────── + + @Test + @DisplayName("migrateLegacyFlatDir moves {root}/{name} to {root}/{wsId}/{name}") + void migratesLegacyFlatDir() throws IOException { + SkillWorkspaceManager m = newManager(); + Path legacy = tmp.resolve("demo"); + Files.createDirectories(legacy.resolve("scripts")); + Files.writeString(legacy.resolve("SKILL.md"), "# demo"); + Files.writeString(legacy.resolve("scripts").resolve("run.py"), "print(1)"); + + boolean moved = m.migrateLegacyFlatDir("demo", 7L); + + assertTrue(moved, "a flat dir with SKILL.md should migrate"); + Path scoped = tmp.resolve("7").resolve("demo"); + assertTrue(Files.exists(scoped.resolve("SKILL.md")), "SKILL.md follows to the scoped location"); + assertTrue(Files.exists(scoped.resolve("scripts").resolve("run.py")), "on-disk scripts survive the move"); + assertFalse(Files.exists(legacy), "the legacy flat dir is gone after migration"); + } + + @Test + @DisplayName("migrateLegacyFlatDir is idempotent and never overwrites an existing scoped dir") + void migrationIdempotentAndNonDestructive() throws IOException { + SkillWorkspaceManager m = newManager(); + Path legacy = tmp.resolve("demo"); + Files.createDirectories(legacy); + Files.writeString(legacy.resolve("SKILL.md"), "# legacy"); + Path scoped = tmp.resolve("7").resolve("demo"); + Files.createDirectories(scoped); + Files.writeString(scoped.resolve("SKILL.md"), "# already-migrated"); + + boolean moved = m.migrateLegacyFlatDir("demo", 7L); + + assertFalse(moved, "must not move when the scoped target already exists"); + assertEquals("# already-migrated", Files.readString(scoped.resolve("SKILL.md")), + "existing scoped content must never be overwritten"); + } + + @Test + @DisplayName("migrateLegacyFlatDir skips a directory without SKILL.md (e.g. a scoped root)") + void migrationSkipsNonSkillDirs() throws IOException { + SkillWorkspaceManager m = newManager(); + // A workspace-scoped root {root}/1 whose child is a skill — no top-level SKILL.md. + Path scopedRoot = tmp.resolve("1"); + Files.createDirectories(scopedRoot.resolve("inner").resolve("scripts")); + + boolean moved = m.migrateLegacyFlatDir("1", 1L); + + assertFalse(moved, "a dir without a top-level SKILL.md is not a legacy flat skill workspace"); + assertTrue(Files.exists(scopedRoot.resolve("inner")), "the scoped root is left untouched"); } } diff --git a/mateclaw-server/src/test/java/vip/mate/tool/builtin/SkillManageToolWriteFileTest.java b/mateclaw-server/src/test/java/vip/mate/tool/builtin/SkillManageToolWriteFileTest.java index d36491a9..5bb236cd 100644 --- a/mateclaw-server/src/test/java/vip/mate/tool/builtin/SkillManageToolWriteFileTest.java +++ b/mateclaw-server/src/test/java/vip/mate/tool/builtin/SkillManageToolWriteFileTest.java @@ -49,6 +49,7 @@ class SkillManageToolWriteFileTest { private SkillEntity skill(String name, boolean builtin) { SkillEntity s = new SkillEntity(); s.setId(42L); + s.setWorkspaceId(1L); s.setName(name); s.setBuiltin(builtin); s.setSkillContent("---\nname: " + name + "\n---\n# x"); @@ -72,7 +73,7 @@ class SkillManageToolWriteFileTest { null, null, "scripts/run.sh", null); assertTrue(result.startsWith("File 'scripts/run.sh' written"), result); - verify(workspaceManager, times(1)).writeWorkspaceFile("my-skill", "scripts/run.sh", "echo hi"); + verify(workspaceManager, times(1)).writeWorkspaceFile("my-skill", "scripts/run.sh", "echo hi", 1L); // The canonical store row must be written too, not just the FS cache. verify(skillFileService, times(1)).upsertFile(42L, "scripts/run.sh", "echo hi"); } @@ -87,7 +88,7 @@ class SkillManageToolWriteFileTest { null, null, "templates/report.html", null); assertTrue(result.startsWith("File 'templates/report.html' written"), result); - verify(workspaceManager, times(1)).writeWorkspaceFile("my-skill", "templates/report.html", ""); + verify(workspaceManager, times(1)).writeWorkspaceFile("my-skill", "templates/report.html", "", 1L); verify(skillFileService, times(1)).upsertFile(42L, "templates/report.html", ""); } @@ -97,7 +98,7 @@ class SkillManageToolWriteFileTest { String result = tool.skill_manage("write_file", "my-skill", "body", null, null, null, null); assertTrue(result.startsWith("Error"), result); - verify(workspaceManager, never()).writeWorkspaceFile(any(), any(), any()); + verify(workspaceManager, never()).writeWorkspaceFile(any(), any(), any(), any()); } @Test @@ -107,7 +108,7 @@ class SkillManageToolWriteFileTest { String result = tool.skill_manage("write_file", "core", "body", null, null, "references/x.md", null); assertTrue(result.contains("builtin"), result); - verify(workspaceManager, never()).writeWorkspaceFile(any(), any(), any()); + verify(workspaceManager, never()).writeWorkspaceFile(any(), any(), any(), any()); } @Test @@ -117,7 +118,7 @@ class SkillManageToolWriteFileTest { String result = tool.skill_manage("write_file", "ghost", "body", null, null, "references/x.md", null); assertTrue(result.contains("not found"), result); - verify(workspaceManager, never()).writeWorkspaceFile(any(), any(), any()); + verify(workspaceManager, never()).writeWorkspaceFile(any(), any(), any(), any()); } @Test @@ -126,7 +127,7 @@ class SkillManageToolWriteFileTest { when(skillService.findByName("my-skill")).thenReturn(skill("my-skill", false)); scanPasses(); doThrow(new IllegalArgumentException("Unsafe file path rejected: ../etc/passwd")) - .when(workspaceManager).writeWorkspaceFile(eq("my-skill"), any(), any()); + .when(workspaceManager).writeWorkspaceFile(eq("my-skill"), any(), any(), any()); String result = tool.skill_manage("write_file", "my-skill", "body", null, null, "../etc/passwd", null);