From 1025bae2663246965ac96217451f0e5951fa68c0 Mon Sep 17 00:00:00 2001 From: Povilas Kanapickas Date: Fri, 27 Feb 2026 20:46:42 +0200 Subject: [PATCH] Implement symlinks for skills --- src/i18n/locales/ca/skills.json | 4 +- src/i18n/locales/de/skills.json | 4 +- src/i18n/locales/en/skills.json | 4 +- src/i18n/locales/es/skills.json | 4 +- src/i18n/locales/fr/skills.json | 4 +- src/i18n/locales/hi/skills.json | 4 +- src/i18n/locales/id/skills.json | 4 +- src/i18n/locales/it/skills.json | 4 +- src/i18n/locales/ja/skills.json | 4 +- src/i18n/locales/ko/skills.json | 4 +- src/i18n/locales/nl/skills.json | 4 +- src/i18n/locales/pl/skills.json | 4 +- src/i18n/locales/pt-BR/skills.json | 4 +- src/i18n/locales/ru/skills.json | 4 +- src/i18n/locales/tr/skills.json | 4 +- src/i18n/locales/vi/skills.json | 4 +- src/i18n/locales/zh-CN/skills.json | 4 +- src/i18n/locales/zh-TW/skills.json | 4 +- src/services/skills/SkillsManager.ts | 489 +++++- .../skills/__tests__/SkillsManager.spec.ts | 1360 +++++++++++++++++ 20 files changed, 1857 insertions(+), 64 deletions(-) diff --git a/src/i18n/locales/ca/skills.json b/src/i18n/locales/ca/skills.json index 1fb358a350..9a48689160 100644 --- a/src/i18n/locales/ca/skills.json +++ b/src/i18n/locales/ca/skills.json @@ -11,6 +11,8 @@ "missing_update_modes_fields": "Falten camps obligatoris: skillName o source", "manager_unavailable": "El gestor d'habilitats no està disponible", "missing_delete_fields": "Falten camps obligatoris: skillName o source", - "skill_not_found": "No s'ha trobat l'habilitat \"{{name}}\"" + "skill_not_found": "No s'ha trobat l'habilitat \"{{name}}\"", + "container_skill_read_only": "L'habilitat \"{{name}}\" prové d'una carpeta d'habilitats enllaçada simbòlicament ({{path}}) i no es pot moure ni eliminar aquí. Canvia-la a la seva carpeta d'origen.", + "symlink_escapes_skill": "No es pot moure l'habilitat: l'enllaç simbòlic {{link}} apunta fora de la carpeta de l'habilitat i es trencaria a la nova ubicació." } } diff --git a/src/i18n/locales/de/skills.json b/src/i18n/locales/de/skills.json index 9c1107e9bf..c9670ce845 100644 --- a/src/i18n/locales/de/skills.json +++ b/src/i18n/locales/de/skills.json @@ -11,6 +11,8 @@ "missing_update_modes_fields": "Erforderliche Felder fehlen: skillName oder source", "manager_unavailable": "Skill-Manager nicht verfügbar", "missing_delete_fields": "Erforderliche Felder fehlen: skillName oder source", - "skill_not_found": "Skill \"{{name}}\" nicht gefunden" + "skill_not_found": "Skill \"{{name}}\" nicht gefunden", + "container_skill_read_only": "Skill \"{{name}}\" stammt aus einem per Symlink eingebundenen Skills-Ordner ({{path}}) und kann hier nicht verschoben oder gelöscht werden. Ändere ihn in seinem Quellordner.", + "symlink_escapes_skill": "Skill kann nicht verschoben werden: Der Symlink {{link}} zeigt aus dem Skill-Ordner heraus und würde am neuen Ort nicht mehr funktionieren." } } diff --git a/src/i18n/locales/en/skills.json b/src/i18n/locales/en/skills.json index 307b59d365..b66c3fc18a 100644 --- a/src/i18n/locales/en/skills.json +++ b/src/i18n/locales/en/skills.json @@ -11,6 +11,8 @@ "missing_update_modes_fields": "Missing required fields: skillName or source", "manager_unavailable": "Skills manager not available", "missing_delete_fields": "Missing required fields: skillName or source", - "skill_not_found": "Skill \"{{name}}\" not found" + "skill_not_found": "Skill \"{{name}}\" not found", + "container_skill_read_only": "Skill \"{{name}}\" comes from a symlinked skills folder ({{path}}) and can't be moved or deleted here. Change it in its source folder.", + "symlink_escapes_skill": "Can't move the skill: the symlink {{link}} points outside the skill folder and would break at the new location." } } diff --git a/src/i18n/locales/es/skills.json b/src/i18n/locales/es/skills.json index 6e10006eff..c6fe11ae33 100644 --- a/src/i18n/locales/es/skills.json +++ b/src/i18n/locales/es/skills.json @@ -11,6 +11,8 @@ "missing_update_modes_fields": "Faltan campos obligatorios: skillName o source", "manager_unavailable": "El gestor de habilidades no está disponible", "missing_delete_fields": "Faltan campos obligatorios: skillName o source", - "skill_not_found": "No se encontró la habilidad \"{{name}}\"" + "skill_not_found": "No se encontró la habilidad \"{{name}}\"", + "container_skill_read_only": "La habilidad \"{{name}}\" proviene de una carpeta de habilidades enlazada simbólicamente ({{path}}) y no se puede mover ni eliminar aquí. Cámbiala en su carpeta de origen.", + "symlink_escapes_skill": "No se puede mover la habilidad: el enlace simbólico {{link}} apunta fuera de la carpeta de la habilidad y se rompería en la nueva ubicación." } } diff --git a/src/i18n/locales/fr/skills.json b/src/i18n/locales/fr/skills.json index 3f2b6ac529..049c1ef01d 100644 --- a/src/i18n/locales/fr/skills.json +++ b/src/i18n/locales/fr/skills.json @@ -11,6 +11,8 @@ "missing_update_modes_fields": "Champs obligatoires manquants : skillName ou source", "manager_unavailable": "Le gestionnaire de compétences n'est pas disponible", "missing_delete_fields": "Champs obligatoires manquants : skillName ou source", - "skill_not_found": "Compétence \"{{name}}\" introuvable" + "skill_not_found": "Compétence \"{{name}}\" introuvable", + "container_skill_read_only": "La compétence \"{{name}}\" provient d'un dossier de compétences lié par un lien symbolique ({{path}}) et ne peut pas être déplacée ni supprimée ici. Modifie-la dans son dossier source.", + "symlink_escapes_skill": "Impossible de déplacer la compétence : le lien symbolique {{link}} pointe hors du dossier de la compétence et serait cassé au nouvel emplacement." } } diff --git a/src/i18n/locales/hi/skills.json b/src/i18n/locales/hi/skills.json index ed04e50b5e..cab5b50dde 100644 --- a/src/i18n/locales/hi/skills.json +++ b/src/i18n/locales/hi/skills.json @@ -11,6 +11,8 @@ "missing_update_modes_fields": "आवश्यक फ़ील्ड गायब हैं: skillName या source", "manager_unavailable": "स्किल मैनेजर उपलब्ध नहीं है", "missing_delete_fields": "आवश्यक फ़ील्ड गायब हैं: skillName या source", - "skill_not_found": "स्किल \"{{name}}\" नहीं मिला" + "skill_not_found": "स्किल \"{{name}}\" नहीं मिला", + "container_skill_read_only": "स्किल \"{{name}}\" एक सिमलिंक किए गए स्किल फ़ोल्डर ({{path}}) से आता है और इसे यहाँ से न तो हटाया जा सकता है और न ही स्थानांतरित किया जा सकता है। इसे इसके मूल फ़ोल्डर में बदलें।", + "symlink_escapes_skill": "स्किल स्थानांतरित नहीं किया जा सकता: सिमलिंक {{link}} स्किल फ़ोल्डर के बाहर इंगित करता है और नए स्थान पर टूट जाएगा।" } } diff --git a/src/i18n/locales/id/skills.json b/src/i18n/locales/id/skills.json index 433fe0b0c4..a60f376a9b 100644 --- a/src/i18n/locales/id/skills.json +++ b/src/i18n/locales/id/skills.json @@ -11,6 +11,8 @@ "missing_update_modes_fields": "Bidang wajib tidak ada: skillName atau source", "manager_unavailable": "Manajer skill tidak tersedia", "missing_delete_fields": "Bidang wajib tidak ada: skillName atau source", - "skill_not_found": "Skill \"{{name}}\" tidak ditemukan" + "skill_not_found": "Skill \"{{name}}\" tidak ditemukan", + "container_skill_read_only": "Skill \"{{name}}\" berasal dari folder skill yang di-symlink ({{path}}) dan tidak bisa dipindahkan atau dihapus di sini. Ubah di folder sumbernya.", + "symlink_escapes_skill": "Tidak bisa memindahkan skill: symlink {{link}} mengarah ke luar folder skill dan akan rusak di lokasi baru." } } diff --git a/src/i18n/locales/it/skills.json b/src/i18n/locales/it/skills.json index 2f363a6cd0..b1c8299a5c 100644 --- a/src/i18n/locales/it/skills.json +++ b/src/i18n/locales/it/skills.json @@ -11,6 +11,8 @@ "missing_update_modes_fields": "Campi obbligatori mancanti: skillName o source", "manager_unavailable": "Il gestore delle skill non è disponibile", "missing_delete_fields": "Campi obbligatori mancanti: skillName o source", - "skill_not_found": "Skill \"{{name}}\" non trovata" + "skill_not_found": "Skill \"{{name}}\" non trovata", + "container_skill_read_only": "La skill \"{{name}}\" proviene da una cartella di skill collegata tramite symlink ({{path}}) e non può essere spostata o eliminata qui. Modificala nella sua cartella di origine.", + "symlink_escapes_skill": "Impossibile spostare la skill: il symlink {{link}} punta fuori dalla cartella della skill e non funzionerebbe nella nuova posizione." } } diff --git a/src/i18n/locales/ja/skills.json b/src/i18n/locales/ja/skills.json index 90b44d9c95..121a250afe 100644 --- a/src/i18n/locales/ja/skills.json +++ b/src/i18n/locales/ja/skills.json @@ -11,6 +11,8 @@ "missing_update_modes_fields": "必須フィールドが不足しています：skillNameまたはsource", "manager_unavailable": "スキルマネージャーが利用できません", "missing_delete_fields": "必須フィールドが不足しています：skillNameまたはsource", - "skill_not_found": "スキル「{{name}}」が見つかりません" + "skill_not_found": "スキル「{{name}}」が見つかりません", + "container_skill_read_only": "スキル「{{name}}」はシンボリックリンクされたスキルフォルダー（{{path}}）にあるため、ここでは移動も削除もできません。元のフォルダーで変更してください。", + "symlink_escapes_skill": "スキルを移動できません：シンボリックリンク {{link}} がスキルフォルダーの外を指しているため、新しい場所では壊れてしまいます。" } } diff --git a/src/i18n/locales/ko/skills.json b/src/i18n/locales/ko/skills.json index 5e4d59f92c..e19b6d8b60 100644 --- a/src/i18n/locales/ko/skills.json +++ b/src/i18n/locales/ko/skills.json @@ -11,6 +11,8 @@ "missing_update_modes_fields": "필수 필드 누락: skillName 또는 source", "manager_unavailable": "스킬 관리자를 사용할 수 없습니다", "missing_delete_fields": "필수 필드 누락: skillName 또는 source", - "skill_not_found": "스킬 \"{{name}}\"을(를) 찾을 수 없습니다" + "skill_not_found": "스킬 \"{{name}}\"을(를) 찾을 수 없습니다", + "container_skill_read_only": "스킬 \"{{name}}\"은(는) 심볼릭 링크된 스킬 폴더({{path}})에 있어 여기서 이동하거나 삭제할 수 없습니다. 원본 폴더에서 변경하세요.", + "symlink_escapes_skill": "스킬을 이동할 수 없습니다: 심볼릭 링크 {{link}}이(가) 스킬 폴더 바깥을 가리키므로 새 위치에서 깨집니다." } } diff --git a/src/i18n/locales/nl/skills.json b/src/i18n/locales/nl/skills.json index 4ca83f1a35..e60ee4280b 100644 --- a/src/i18n/locales/nl/skills.json +++ b/src/i18n/locales/nl/skills.json @@ -11,6 +11,8 @@ "missing_update_modes_fields": "Vereiste velden ontbreken: skillName of source", "manager_unavailable": "Vaardigheidenbeheerder niet beschikbaar", "missing_delete_fields": "Vereiste velden ontbreken: skillName of source", - "skill_not_found": "Vaardigheid \"{{name}}\" niet gevonden" + "skill_not_found": "Vaardigheid \"{{name}}\" niet gevonden", + "container_skill_read_only": "Vaardigheid \"{{name}}\" komt uit een via symlink gekoppelde vaardighedenmap ({{path}}) en kan hier niet worden verplaatst of verwijderd. Wijzig hem in de bronmap.", + "symlink_escapes_skill": "Kan de vaardigheid niet verplaatsen: de symlink {{link}} wijst naar buiten de vaardighedenmap en zou op de nieuwe locatie niet meer werken." } } diff --git a/src/i18n/locales/pl/skills.json b/src/i18n/locales/pl/skills.json index 93927d1d14..b88fad1692 100644 --- a/src/i18n/locales/pl/skills.json +++ b/src/i18n/locales/pl/skills.json @@ -11,6 +11,8 @@ "missing_update_modes_fields": "Brakuje wymaganych pól: skillName lub source", "manager_unavailable": "Menedżer umiejętności niedostępny", "missing_delete_fields": "Brakuje wymaganych pól: skillName lub source", - "skill_not_found": "Nie znaleziono umiejętności \"{{name}}\"" + "skill_not_found": "Nie znaleziono umiejętności \"{{name}}\"", + "container_skill_read_only": "Umiejętność \"{{name}}\" pochodzi z folderu umiejętności podłączonego dowiązaniem symbolicznym ({{path}}) i nie można jej tutaj przenieść ani usunąć. Zmień ją w folderze źródłowym.", + "symlink_escapes_skill": "Nie można przenieść umiejętności: dowiązanie symboliczne {{link}} wskazuje poza folder umiejętności i przestałoby działać w nowej lokalizacji." } } diff --git a/src/i18n/locales/pt-BR/skills.json b/src/i18n/locales/pt-BR/skills.json index 2a0881bd8f..8e92ed36d3 100644 --- a/src/i18n/locales/pt-BR/skills.json +++ b/src/i18n/locales/pt-BR/skills.json @@ -11,6 +11,8 @@ "missing_update_modes_fields": "Campos obrigatórios ausentes: skillName ou source", "manager_unavailable": "Gerenciador de habilidades não disponível", "missing_delete_fields": "Campos obrigatórios ausentes: skillName ou source", - "skill_not_found": "Habilidade \"{{name}}\" não encontrada" + "skill_not_found": "Habilidade \"{{name}}\" não encontrada", + "container_skill_read_only": "A habilidade \"{{name}}\" vem de uma pasta de habilidades vinculada por link simbólico ({{path}}) e não pode ser movida nem excluída aqui. Altere-a na pasta de origem.", + "symlink_escapes_skill": "Não é possível mover a habilidade: o link simbólico {{link}} aponta para fora da pasta da habilidade e quebraria no novo local." } } diff --git a/src/i18n/locales/ru/skills.json b/src/i18n/locales/ru/skills.json index c505d51de7..76969c4cb2 100644 --- a/src/i18n/locales/ru/skills.json +++ b/src/i18n/locales/ru/skills.json @@ -11,6 +11,8 @@ "missing_update_modes_fields": "Отсутствуют обязательные поля: skillName или source", "manager_unavailable": "Менеджер навыков недоступен", "missing_delete_fields": "Отсутствуют обязательные поля: skillName или source", - "skill_not_found": "Навык \"{{name}}\" не найден" + "skill_not_found": "Навык \"{{name}}\" не найден", + "container_skill_read_only": "Навык \"{{name}}\" находится в папке навыков, подключённой через символическую ссылку ({{path}}), и его нельзя переместить или удалить здесь. Измени его в исходной папке.", + "symlink_escapes_skill": "Не удалось переместить навык: символическая ссылка {{link}} указывает за пределы папки навыка и сломается в новом месте." } } diff --git a/src/i18n/locales/tr/skills.json b/src/i18n/locales/tr/skills.json index 459d9c8f6d..c0ab1a549d 100644 --- a/src/i18n/locales/tr/skills.json +++ b/src/i18n/locales/tr/skills.json @@ -11,6 +11,8 @@ "missing_update_modes_fields": "Gerekli alanlar eksik: skillName veya source", "manager_unavailable": "Beceri yöneticisi kullanılamıyor", "missing_delete_fields": "Gerekli alanlar eksik: skillName veya source", - "skill_not_found": "\"{{name}}\" becerisi bulunamadı" + "skill_not_found": "\"{{name}}\" becerisi bulunamadı", + "container_skill_read_only": "\"{{name}}\" becerisi sembolik bağlantılı bir beceri klasöründen ({{path}}) geliyor ve burada taşınamaz veya silinemez. Kaynak klasöründe değiştir.", + "symlink_escapes_skill": "Beceri taşınamıyor: {{link}} sembolik bağlantısı beceri klasörünün dışını gösteriyor ve yeni konumda bozulur." } } diff --git a/src/i18n/locales/vi/skills.json b/src/i18n/locales/vi/skills.json index 3bd28a8c0b..67d2b6509b 100644 --- a/src/i18n/locales/vi/skills.json +++ b/src/i18n/locales/vi/skills.json @@ -11,6 +11,8 @@ "missing_update_modes_fields": "Thiếu các trường bắt buộc: skillName hoặc source", "manager_unavailable": "Trình quản lý kỹ năng không khả dụng", "missing_delete_fields": "Thiếu các trường bắt buộc: skillName hoặc source", - "skill_not_found": "Không tìm thấy kỹ năng \"{{name}}\"" + "skill_not_found": "Không tìm thấy kỹ năng \"{{name}}\"", + "container_skill_read_only": "Kỹ năng \"{{name}}\" đến từ một thư mục kỹ năng được liên kết tượng trưng ({{path}}) nên không thể di chuyển hoặc xóa ở đây. Hãy thay đổi nó trong thư mục nguồn.", + "symlink_escapes_skill": "Không thể di chuyển kỹ năng: liên kết tượng trưng {{link}} trỏ ra ngoài thư mục kỹ năng và sẽ bị hỏng ở vị trí mới." } } diff --git a/src/i18n/locales/zh-CN/skills.json b/src/i18n/locales/zh-CN/skills.json index ade7833363..8d657d8b91 100644 --- a/src/i18n/locales/zh-CN/skills.json +++ b/src/i18n/locales/zh-CN/skills.json @@ -11,6 +11,8 @@ "missing_update_modes_fields": "缺少必填字段：skillName 或 source", "manager_unavailable": "技能管理器不可用", "missing_delete_fields": "缺少必填字段：skillName 或 source", - "skill_not_found": "未找到技能 \"{{name}}\"" + "skill_not_found": "未找到技能 \"{{name}}\"", + "container_skill_read_only": "技能 \"{{name}}\" 来自通过符号链接引入的技能文件夹（{{path}}），无法在此移动或删除。请在其源文件夹中修改。", + "symlink_escapes_skill": "无法移动技能：符号链接 {{link}} 指向技能文件夹之外，移动后会失效。" } } diff --git a/src/i18n/locales/zh-TW/skills.json b/src/i18n/locales/zh-TW/skills.json index e2c1fcf305..412827f8a3 100644 --- a/src/i18n/locales/zh-TW/skills.json +++ b/src/i18n/locales/zh-TW/skills.json @@ -11,6 +11,8 @@ "missing_update_modes_fields": "缺少必填欄位：skillName 或 source", "manager_unavailable": "技能管理器無法使用", "missing_delete_fields": "缺少必填欄位：skillName 或 source", - "skill_not_found": "找不到技能「{{name}}」" + "skill_not_found": "找不到技能「{{name}}」", + "container_skill_read_only": "技能「{{name}}」來自透過符號連結引入的技能資料夾（{{path}}），無法在此移動或刪除。請在其來源資料夾中修改。", + "symlink_escapes_skill": "無法移動技能：符號連結 {{link}} 指向技能資料夾之外，移動後會失效。" } } diff --git a/src/services/skills/SkillsManager.ts b/src/services/skills/SkillsManager.ts index 0959b977c9..82bcb38723 100644 --- a/src/services/skills/SkillsManager.ts +++ b/src/services/skills/SkillsManager.ts @@ -18,10 +18,60 @@ import { t } from "../../i18n" // Re-export for convenience export type { SkillMetadata, SkillContent } +// Direct children (new skill folders or container symlinks) and their SKILL.md files. +const SKILLS_WATCH_PATTERN = "{*,*/SKILL.md}" + +/** Mutable state shared by every scan in a single discovery pass. */ +interface DiscoveryPass { + /** Skills found in this pass. Published to the manager only when the pass ends. */ + skills: Map + /** SKILL.md paths of the skills found in this pass that come from a symlinked container. */ + containerSkillPaths: Set + /** Symlinked containers found in this pass: real path -> the root entry that links to it. */ + containers: Map + /** + * Paths that an unexpected error (anything except ENOENT/ENOTDIR) hid. Previously + * discovered skills and containers under them are kept, because they might only be + * unreadable for a moment. Everything else is replaced by the results of the pass. + */ + failedPaths: string[] +} + +function isNotFoundError(error: unknown): boolean { + const code = (error as NodeJS.ErrnoException | undefined)?.code + return code === "ENOENT" || code === "ENOTDIR" +} + +function isUnderAny(filePath: string, dirs: Iterable): boolean { + for (const dir of dirs) { + if (filePath === dir || filePath.startsWith(dir + path.sep)) return true + } + return false +} + +/** + * Dot-prefixed entries are never valid skill names. They are also used for the + * hidden staging and trash dirs of a cross-filesystem move. + */ +function isHiddenEntry(name: string): boolean { + return name.startsWith(".") +} + export class SkillsManager { private skills: Map = new Map() + // SKILL.md paths of skills that come from a symlinked container. These are read-only for move/delete. + private containerSkillPaths: Set = new Set() + // Published symlinked containers: real path -> the root entry that links to it + private containers: Map = new Map() + // Skills root -> the real path it last resolved to, so a root that later fails to + // resolve still covers the skills found under its target + private rootRealPaths: Map = new Map() private providerRef: WeakRef private disposables: vscode.Disposable[] = [] + // Keyed by container real path. VS Code watchers don't follow symlinks. + private containerWatchers: Map = new Map() + // Runs discovery passes one at a time, so an older pass can't finish after a newer one + private discoveryQueue: Promise = Promise.resolve() private isDisposed = false constructor(provider: ClineProvider) { @@ -39,68 +89,232 @@ export class SkillsManager { * Also supports symlinks: * - .roo/skills can be a symlink to a directory containing skill subdirectories * - .roo/skills/[dirname] can be a symlink to a skill directory + * - .roo/skills/[dirname] can be a symlink to a container of skill directories */ async discoverSkills(): Promise { - this.skills.clear() + const run = this.discoveryQueue.then(() => this.runDiscovery()) + // A failed pass must not block the passes queued after it + this.discoveryQueue = run.catch(() => {}) + return run + } + + private async runDiscovery(): Promise { + if (this.isDisposed) return + + // Build into new collections, so readers keep seeing the last published skills + // during the scan, and a pass that throws leaves them untouched. + const pass: DiscoveryPass = { + skills: new Map(), + containerSkillPaths: new Set(), + containers: new Map(), + failedPaths: [], + } const skillsDirs = await this.getSkillsDirectories() for (const { dir, source, mode } of skillsDirs) { - await this.scanSkillsDirectory(dir, source, mode) + await this.scanSkillsDirectory(dir, source, mode, pass) + } + + if (this.isDisposed) return + + this.publishSkills(pass) + this.syncContainerWatchers() + } + + /** + * Replace the published skills and containers with the results of a pass. + * Previously discovered ones that the pass did not find are kept only if they + * are under a path that failed to read, until a pass reads that path again. + */ + private publishSkills(pass: DiscoveryPass): void { + const keptContainers: string[] = [] + for (const [realPath, linkPath] of this.containers) { + if (pass.containers.has(realPath)) continue + if (isUnderAny(realPath, pass.failedPaths) || isUnderAny(linkPath, pass.failedPaths)) { + pass.containers.set(realPath, linkPath) + keptContainers.push(realPath) + } + } + + for (const [key, skill] of this.skills) { + if (pass.skills.has(key)) continue + if (!isUnderAny(skill.path, pass.failedPaths) && !isUnderAny(skill.path, keptContainers)) continue + pass.skills.set(key, skill) + if (this.containerSkillPaths.has(skill.path)) { + pass.containerSkillPaths.add(skill.path) + } } + + this.skills = pass.skills + this.containerSkillPaths = pass.containerSkillPaths + this.containers = pass.containers } /** * Scan a skills directory for skill subdirectories. - * Handles two symlink cases: + * Handles symlink cases: * 1. The skills directory itself is a symlink (resolved by directoryExists using realpath) * 2. Individual skill subdirectories are symlinks + * 3. Symlinked container of skills (e.g., .roo/skills/shared -> /repo/skills). + * Only symlinks are treated as containers, and only one level deep. + * + * On name collisions the last loaded skill wins. Containers are loaded first + * in reverse order, so direct skills win over container skills, and the + * alphabetically first container wins over the others. + * + * Errors are handled per entry, so one unreadable entry never hides the rest of the root. */ - private async scanSkillsDirectory(dirPath: string, source: "global" | "project", mode?: string): Promise { - if (!(await directoryExists(dirPath))) { + private async scanSkillsDirectory( + dirPath: string, + source: "global" | "project", + mode: string | undefined, + pass: DiscoveryPass, + ): Promise { + let realDirPath = dirPath + let entries: string[] + try { + if (!(await directoryExists(dirPath))) { + return + } + // Get the real path (resolves if dirPath is a symlink) + realDirPath = await fs.realpath(dirPath) + // Sorted so collision handling doesn't depend on filesystem order + entries = [...(await fs.readdir(realDirPath))].sort() + } catch (error) { + if (!isNotFoundError(error)) { + pass.failedPaths.push(dirPath, realDirPath, this.rootRealPaths.get(dirPath) ?? realDirPath) + console.error(`Failed to scan skills directory ${dirPath}:`, error) + } return } + this.rootRealPaths.set(dirPath, realDirPath) - try { - // Get the real path (resolves if dirPath is a symlink) - const realDirPath = await fs.realpath(dirPath) + const directSkills: string[] = [] + const containerPaths: string[] = [] + + for (const entryName of entries) { + if (isHiddenEntry(entryName)) continue + const entryPath = path.join(realDirPath, entryName) + + try { + // Check if this entry is a directory (follows symlinks automatically) + const stats = await fs.stat(entryPath) + if (!stats.isDirectory()) continue + + if (await fileExists(path.join(entryPath, "SKILL.md"))) { + directSkills.push(entryName) + } else if (await this.isSymlink(entryPath, pass)) { + containerPaths.push(entryPath) + } + } catch (error) { + // A broken symlink (ENOENT) is simply not a skill + if (!isNotFoundError(error)) { + pass.failedPaths.push(entryPath) + console.error(`Failed to check skill entry ${entryPath}:`, error) + } + } + } + + for (const containerPath of containerPaths.reverse()) { + await this.scanSkillContainer(containerPath, source, mode, pass) + } - // Read directory entries - const entries = await fs.readdir(realDirPath) + for (const entryName of directSkills) { + // The skill name comes from the entry name (symlink name if symlinked) + await this.loadSkillMetadata(pass, path.join(realDirPath, entryName), source, mode, entryName) + } + } + + private async scanSkillContainer( + containerPath: string, + source: "global" | "project", + mode: string | undefined, + pass: DiscoveryPass, + ): Promise { + let realContainerPath = containerPath + try { + realContainerPath = await fs.realpath(containerPath) + pass.containers.set(realContainerPath, containerPath) + const entries = [...(await fs.readdir(realContainerPath))].sort() for (const entryName of entries) { - const entryPath = path.join(realDirPath, entryName) + if (isHiddenEntry(entryName)) continue + const entryPath = path.join(realContainerPath, entryName) + + let isDirectory = false + try { + isDirectory = (await fs.stat(entryPath)).isDirectory() + } catch (error) { + // A broken symlink (ENOENT) is simply not a skill + if (!isNotFoundError(error)) { + pass.failedPaths.push(entryPath) + console.error(`Failed to stat skill entry ${entryPath} in container ${containerPath}:`, error) + } + } + if (!isDirectory) continue - // Check if this entry is a directory (follows symlinks automatically) - const stats = await fs.stat(entryPath).catch(() => null) - if (!stats?.isDirectory()) continue + const skillMdPath = await this.loadSkillMetadata(pass, entryPath, source, mode, entryName) + if (skillMdPath) { + pass.containerSkillPaths.add(skillMdPath) + } + } + } catch (error) { + if (!isNotFoundError(error)) { + pass.failedPaths.push(containerPath, realContainerPath) + console.error(`Failed to scan skills container ${containerPath}:`, error) + } + } + } - // Load skill metadata - the skill name comes from the entry name (symlink name if symlinked) - await this.loadSkillMetadata(entryPath, source, mode, entryName) + /** + * @param pass - When called during discovery, an unexpected error marks the entry + * as failed, since it might be a container that is only unreadable for a moment. + */ + private async isSymlink(entryPath: string, pass?: DiscoveryPass): Promise { + try { + return (await fs.lstat(entryPath)).isSymbolicLink() + } catch (error) { + if (!isNotFoundError(error)) { + pass?.failedPaths.push(entryPath) + console.error(`Failed to check whether ${entryPath} is a symlink:`, error) } - } catch { - // Directory doesn't exist or can't be read - this is fine + return false } } /** * Load skill metadata from a skill directory. + * @param pass - The discovery pass that collects the loaded skill * @param skillDir - The resolved path to the skill directory (target of symlink if symlinked) * @param source - Whether this is a global or project skill * @param mode - The mode this skill is specific to (undefined for generic skills) * @param skillName - The skill name (from symlink name if symlinked, otherwise from directory name) + * @returns The SKILL.md path if the skill was loaded, otherwise undefined */ private async loadSkillMetadata( + pass: DiscoveryPass, skillDir: string, source: "global" | "project", mode?: string, skillName?: string, - ): Promise { + ): Promise { const skillMdPath = path.join(skillDir, "SKILL.md") - if (!(await fileExists(skillMdPath))) return + let fileContent: string try { - const fileContent = await fs.readFile(skillMdPath, "utf-8") + if (!(await fileExists(skillMdPath))) return undefined + fileContent = await fs.readFile(skillMdPath, "utf-8") + } catch (error) { + // An unreadable SKILL.md may only be unreadable for a moment, so keep the + // previously discovered skill. A file removed mid-scan is simply gone. + if (!isNotFoundError(error)) { + pass.failedPaths.push(skillDir) + console.error(`Failed to read skill at ${skillDir}:`, error) + } + return undefined + } + try { // Use gray-matter to parse frontmatter const { data: frontmatter, content: body } = matter(fileContent) @@ -162,7 +376,7 @@ export class SkillsManager { const primaryMode = modeSlugs?.[0] const skillKey = this.getSkillKey(effectiveSkillName, source, primaryMode) - this.skills.set(skillKey, { + pass.skills.set(skillKey, { name: effectiveSkillName, description, path: skillMdPath, @@ -170,8 +384,10 @@ export class SkillsManager { mode: primaryMode, // Deprecated: kept for backward compatibility modeSlugs, // New: array of mode slugs, undefined = any mode }) + return skillMdPath } catch (error) { console.error(`Failed to load skill at ${skillDir}:`, error) + return undefined } } @@ -437,6 +653,8 @@ Add your skill instructions here. throw new Error(t("skills:errors.not_found", { name, source, modeInfo })) } + this.assertNotContainerSkill(skill) + // Get the skill directory (parent of SKILL.md) const skillDir = path.dirname(skill.path) @@ -472,6 +690,8 @@ Add your skill instructions here. throw new Error(t("skills:errors.not_found", { name, source, modeInfo })) } + this.assertNotContainerSkill(skill) + // Determine base directory let baseDir: string if (source === "global") { @@ -484,10 +704,24 @@ Add your skill instructions here. baseDir = path.join(provider.cwd, ".roo") } - // Determine source and destination directories - const sourceDirName = currentMode ? `skills-${currentMode}` : "skills" + // Determine source and destination directories. The source comes from the + // discovered path, since the skills directory itself may be a symlink. const destDirName = newMode ? `skills-${newMode}` : "skills" - const sourceDir = path.join(baseDir, sourceDirName, name) + const sourceDir = path.dirname(skill.path) + const sourceRoot = path.join(baseDir, currentMode ? `skills-${currentMode}` : "skills") + + // Only move skills that live directly in the .roo source root (possibly through a + // symlinked root). Skills sharing the same key may come from .agents, which is shared + // with other agents and must never be moved out from under them. + const realSourceRoot = await fs.realpath(sourceRoot).catch((error: unknown) => { + if (isNotFoundError(error)) return undefined + throw error + }) + if (path.dirname(sourceDir) !== realSourceRoot) { + const modeInfo = currentMode ? ` (mode: ${currentMode})` : "" + throw new Error(t("skills:errors.not_found", { name, source, modeInfo })) + } + const destSkillsDir = path.join(baseDir, destDirName) const destDir = path.join(destSkillsDir, name) const destSkillMdPath = path.join(destDir, "SKILL.md") @@ -500,15 +734,17 @@ Add your skill instructions here. // Ensure destination skills directory exists await fs.mkdir(destSkillsDir, { recursive: true }) - // Move the skill directory - await fs.rename(sourceDir, destDir) + // Move the skill directory (falls back to copy+remove across filesystems) + await this.moveDirectory(sourceDir, destDir) - // Clean up empty source skills directory - const sourceSkillsDir = path.join(baseDir, sourceDirName) + // Clean up empty source skills directory. Skip it if it's a symlink, so we + // never leave a dangling symlink. try { - const entries = await fs.readdir(sourceSkillsDir) - if (entries.length === 0) { - await fs.rmdir(sourceSkillsDir) + if (!(await this.isSymlink(sourceRoot))) { + const entries = await fs.readdir(sourceRoot) + if (entries.length === 0) { + await fs.rmdir(sourceRoot) + } } } catch { // Ignore errors - directory might not exist or have permission issues @@ -518,6 +754,136 @@ Add your skill instructions here. await this.discoverSkills() } + /** + * Skills inside a symlinked container live outside the workspace (for example, in a + * shared repo). Moving or deleting them would change that shared content. Unlinking + * the container would remove all of its skills. So they are read-only here. + */ + private assertNotContainerSkill(skill: SkillMetadata): void { + if (this.containerSkillPaths.has(skill.path)) { + throw new Error(t("skills:errors.container_skill_read_only", { name: skill.name, path: skill.path })) + } + } + + /** + * Rename a directory, falling back to copy + delete across filesystems + * (a skills directory may be a symlink to another device). + * + * The fallback never leaves the skill at two discoverable locations: + * 1. Copy the source into a hidden staging dir next to destDir (dest filesystem), + * and check that no relative symlink in the copy points outside it. + * 2. Atomically rename the source aside to a hidden trash dir (source filesystem). + * 3. Atomically promote staging to destDir. On failure, restore the source from trash. + * 4. Best-effort delete of the trash dir. The move is already committed, so a + * failure here is logged instead of thrown and never leaves a duplicate skill. + * + * Between steps 2 and 3 the skill is briefly not discoverable at all. Discovery and + * the watchers skip the dot-prefixed staging and trash dirs. + */ + private async moveDirectory(sourceDir: string, destDir: string): Promise { + try { + await fs.rename(sourceDir, destDir) + return + } catch (error) { + if ((error as NodeJS.ErrnoException)?.code !== "EXDEV") { + throw error + } + } + + const suffix = `${process.pid}-${Date.now()}-${Math.random().toString(36).slice(2, 10)}` + const stagingDir = path.join(path.dirname(destDir), `.${path.basename(destDir)}.moving-${suffix}`) + const trashDir = path.join(path.dirname(sourceDir), `.${path.basename(sourceDir)}.removing-${suffix}`) + let sourceMovedAside = false + + try { + // verbatimSymlinks: otherwise relative links get rewritten to point into the deleted source + await fs.cp(sourceDir, stagingDir, { + recursive: true, + errorOnExist: true, + force: false, + verbatimSymlinks: true, + }) + + // A relative link that points outside the skill would dangle at the new + // location. Fail now, while the source still exists. + const escapingLink = await this.findEscapingRelativeSymlink(stagingDir) + if (escapingLink) { + throw new Error( + t("skills:errors.symlink_escapes_skill", { + link: path.join(sourceDir, path.relative(stagingDir, escapingLink)), + }), + ) + } + + const destExists = await fs.lstat(destDir).then( + () => true, + (error: NodeJS.ErrnoException) => { + if (error?.code === "ENOENT") { + return false + } + throw error + }, + ) + if (destExists) { + throw Object.assign(new Error(`Destination already exists: ${destDir}`), { code: "EEXIST" }) + } + + // Same parent directory, so this is atomic and cannot fail with EXDEV + await fs.rename(sourceDir, trashDir) + sourceMovedAside = true + + await fs.rename(stagingDir, destDir) + } catch (moveError) { + if (sourceMovedAside) { + try { + await fs.rename(trashDir, sourceDir) + } catch (restoreError) { + // Keep the trash dir so the original content is never lost + console.error( + `Failed to restore skill directory ${sourceDir} from ${trashDir} after a failed move:`, + restoreError, + ) + } + } + await fs.rm(stagingDir, { recursive: true, force: true }).catch(() => {}) + throw moveError + } + + // The move is committed and the source is no longer discoverable under its + // original name, so leftover trash must not turn a successful move into an error. + try { + await fs.rm(trashDir, { recursive: true, force: true }) + } catch (cleanupError) { + console.error(`Failed to remove moved skill's old directory ${trashDir}:`, cleanupError) + } + } + + /** + * Find a relative symlink under rootDir whose target resolves outside rootDir. + * Absolute links keep working after a move, so they are allowed. + * @returns The path of the first such link, or undefined if there is none + */ + private async findEscapingRelativeSymlink(rootDir: string): Promise { + const pending = [rootDir] + while (pending.length > 0) { + const dir = pending.pop()! + for (const entry of await fs.readdir(dir, { withFileTypes: true })) { + const entryPath = path.join(dir, entry.name) + if (entry.isDirectory()) { + pending.push(entryPath) + } else if (entry.isSymbolicLink()) { + const target = await fs.readlink(entryPath) + if (path.isAbsolute(target)) continue + const relativeToRoot = path.relative(rootDir, path.resolve(dir, target)) + if (relativeToRoot === ".." || relativeToRoot.startsWith(`..${path.sep}`)) { + return entryPath + } + } + } + } + return undefined + } + /** * Update the mode associations for a skill by modifying its SKILL.md frontmatter. * @param name - Skill name @@ -538,6 +904,8 @@ Add your skill instructions here. throw new Error(t("skills:errors.not_found", { name, source, modeInfo: "" })) } + this.assertNotContainerSkill(skill) + // Read the current SKILL.md file const fileContent = await fs.readFile(skill.path, "utf-8") const { data: frontmatter, content: body } = matter(fileContent) @@ -685,35 +1053,64 @@ Add your skill instructions here. } private watchDirectory(dirPath: string): void { + const watcher = this.createSkillsWatcher(dirPath) + if (watcher) { + this.disposables.push(watcher) + } + } + + private syncContainerWatchers(): void { + const containers = this.containers + for (const [containerPath, watcher] of this.containerWatchers) { + if (this.isDisposed || !containers.has(containerPath)) { + watcher.dispose() + this.containerWatchers.delete(containerPath) + } + } + + if (this.isDisposed) return + + for (const containerPath of containers.keys()) { + if (this.containerWatchers.has(containerPath)) continue + const watcher = this.createSkillsWatcher(containerPath) + if (watcher) { + this.containerWatchers.set(containerPath, watcher) + } + } + } + + private createSkillsWatcher(dirPath: string): vscode.Disposable | undefined { if (process.env.NODE_ENV === "test" || !vscode.workspace.createFileSystemWatcher) { - return + return undefined } - const pattern = new vscode.RelativePattern(dirPath, "**/SKILL.md") + const pattern = new vscode.RelativePattern(dirPath, SKILLS_WATCH_PATTERN) const watcher = vscode.workspace.createFileSystemWatcher(pattern) - watcher.onDidChange(async (uri) => { + const onEvent = async (uri: vscode.Uri) => { if (this.isDisposed) return + // Skip hidden entries, such as the staging and trash dirs of a cross-filesystem move + const relativePath = path.relative(dirPath, uri.fsPath) + if (relativePath.split(path.sep).some(isHiddenEntry)) return await this.discoverSkills() - }) - - watcher.onDidCreate(async (uri) => { - if (this.isDisposed) return - await this.discoverSkills() - }) + } - watcher.onDidDelete(async (uri) => { - if (this.isDisposed) return - await this.discoverSkills() - }) + watcher.onDidChange(onEvent) + watcher.onDidCreate(onEvent) + watcher.onDidDelete(onEvent) - this.disposables.push(watcher) + return watcher } async dispose(): Promise { this.isDisposed = true this.disposables.forEach((d) => d.dispose()) this.disposables = [] + this.containerWatchers.forEach((w) => w.dispose()) + this.containerWatchers.clear() this.skills.clear() + this.containerSkillPaths.clear() + this.containers.clear() + this.rootRealPaths.clear() } } diff --git a/src/services/skills/__tests__/SkillsManager.spec.ts b/src/services/skills/__tests__/SkillsManager.spec.ts index d36582d893..5249f25681 100644 --- a/src/services/skills/__tests__/SkillsManager.spec.ts +++ b/src/services/skills/__tests__/SkillsManager.spec.ts @@ -2,6 +2,7 @@ import * as path from "path" // Use vi.hoisted to ensure mocks are available during hoisting const { + mockReadlink, mockStat, mockReadFile, mockReaddir, @@ -14,7 +15,12 @@ const { mockRm, mockRename, mockRmdir, + mockCp, + mockLstat, } = vi.hoisted(() => ({ + mockReadlink: vi.fn(), + mockCp: vi.fn(), + mockLstat: vi.fn(), mockStat: vi.fn(), mockReadFile: vi.fn(), mockReaddir: vi.fn(), @@ -38,6 +44,13 @@ const SHARED_DIR = process.platform === "win32" ? "C:\\shared\\skills" : "/share // Helper to create platform-appropriate paths const p = (...segments: string[]) => path.join(...segments) +// Make fs.lstat report the given paths as symlinks +const mockSymlinks = (...paths: string[]) => + mockLstat.mockImplementation(async (pathArg: string) => ({ isSymbolicLink: () => paths.includes(pathArg) })) + +// A real not-found error, as fs throws for a missing path or broken symlink +const enoent = () => Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + // Mock fs/promises module vi.mock("fs/promises", () => ({ default: { @@ -50,6 +63,9 @@ vi.mock("fs/promises", () => ({ rm: mockRm, rename: mockRename, rmdir: mockRmdir, + cp: mockCp, + lstat: mockLstat, + readlink: mockReadlink, }, stat: mockStat, readFile: mockReadFile, @@ -60,6 +76,9 @@ vi.mock("fs/promises", () => ({ rm: mockRm, rename: mockRename, rmdir: mockRmdir, + cp: mockCp, + lstat: mockLstat, + readlink: mockReadlink, })) // Mock os module @@ -109,6 +128,7 @@ vi.mock("../../../i18n", () => ({ }, })) +import * as vscode from "vscode" import { SkillsManager } from "../SkillsManager" import { ClineProvider } from "../../../core/webview/ClineProvider" @@ -130,6 +150,7 @@ describe("SkillsManager", () => { beforeEach(() => { vi.clearAllMocks() + mockLstat.mockReset() mockHomedir.mockReturnValue(HOME_DIR) // Create mock provider @@ -616,6 +637,840 @@ Instructions here...` expect(skills[0].source).toBe("global") }) + it("should discover skills from symlinked container directory with multiple skills", async () => { + // .roo/skills/skills -> /repo/skills, containing skill-a/ and skill-b/ + const containerDir = p(globalSkillsDir, "skills") // the symlinked container + const repoSkillsDir = p("/repo", "skills") // the actual target + const skillADir = p(repoSkillsDir, "skill-a") + const skillAMd = p(skillADir, "SKILL.md") + const skillBDir = p(repoSkillsDir, "skill-b") + const skillBMd = p(skillBDir, "SKILL.md") + + mockDirectoryExists.mockImplementation(async (dir: string) => { + return dir === globalSkillsDir || dir === containerDir + }) + + mockRealpath.mockImplementation(async (pathArg: string) => { + if (pathArg === globalSkillsDir) return globalSkillsDir + if (pathArg === containerDir) return repoSkillsDir + return pathArg + }) + + mockReaddir.mockImplementation(async (dir: string) => { + if (dir === globalSkillsDir) return ["skills"] // the symlinked container entry + if (dir === repoSkillsDir) return ["skill-a", "skill-b"] + return [] + }) + + mockStat.mockImplementation(async (pathArg: string) => { + if (pathArg === containerDir) return { isDirectory: () => true } + if (pathArg === skillADir) return { isDirectory: () => true } + if (pathArg === skillBDir) return { isDirectory: () => true } + throw enoent() + }) + mockSymlinks(containerDir) + + mockFileExists.mockImplementation(async (file: string) => { + return file === skillAMd || file === skillBMd + }) + + mockReadFile.mockImplementation(async (file: string) => { + if (file === skillAMd) { + return `--- +name: skill-a +description: First skill from symlinked repo +--- + +# Skill A` + } + if (file === skillBMd) { + return `--- +name: skill-b +description: Second skill from symlinked repo +--- + +# Skill B` + } + throw new Error("File not found") + }) + + await skillsManager.discoverSkills() + + const skills = skillsManager.getAllSkills() + expect(skills).toHaveLength(2) + const names = skills.map((s) => s.name).sort() + expect(names).toEqual(["skill-a", "skill-b"]) + expect(skills.every((s) => s.source === "global")).toBe(true) + }) + + it("should not treat ordinary (non-symlinked) nested directories as containers", async () => { + // .roo/skills/group/nested-skill/SKILL.md where "group" is a real directory + const groupDir = p(globalSkillsDir, "group") + const skillDir = p(groupDir, "nested-skill") + const skillMd = p(skillDir, "SKILL.md") + + mockDirectoryExists.mockImplementation(async (dir: string) => dir === globalSkillsDir) + mockRealpath.mockImplementation(async (pathArg: string) => pathArg) + mockReaddir.mockImplementation(async (dir: string) => { + if (dir === globalSkillsDir) return ["group"] + if (dir === groupDir) return ["nested-skill"] + return [] + }) + mockStat.mockImplementation(async (pathArg: string) => { + if (pathArg === groupDir || pathArg === skillDir) return { isDirectory: () => true } + throw enoent() + }) + mockFileExists.mockImplementation(async (file: string) => file === skillMd) + mockReadFile.mockResolvedValue(`--- +name: nested-skill +description: A nested skill +--- +Instructions`) + + await skillsManager.discoverSkills() + + expect(skillsManager.getAllSkills()).toHaveLength(0) + expect(mockReaddir).not.toHaveBeenCalledWith(groupDir) + }) + + it("should only scan one level into a symlinked container", async () => { + // .roo/skills/shared -> /shared/skills, which contains another container "inner" + const containerEntry = p(globalSkillsDir, "shared") + const innerDir = p(SHARED_DIR, "inner") + const innerSkillDir = p(innerDir, "inner-skill") + const innerSkillMd = p(innerSkillDir, "SKILL.md") + + mockDirectoryExists.mockImplementation(async (dir: string) => dir === globalSkillsDir) + mockRealpath.mockImplementation(async (pathArg: string) => + pathArg === containerEntry ? SHARED_DIR : pathArg, + ) + mockReaddir.mockImplementation(async (dir: string) => { + if (dir === globalSkillsDir) return ["shared"] + if (dir === SHARED_DIR) return ["inner"] + if (dir === innerDir) return ["inner-skill"] + return [] + }) + mockStat.mockImplementation(async (pathArg: string) => { + if ([containerEntry, innerDir, innerSkillDir].includes(pathArg)) return { isDirectory: () => true } + throw enoent() + }) + mockSymlinks(containerEntry, innerDir) + mockFileExists.mockImplementation(async (file: string) => file === innerSkillMd) + mockReadFile.mockResolvedValue(`--- +name: inner-skill +description: Too deep +--- +Instructions`) + + await skillsManager.discoverSkills() + + expect(skillsManager.getAllSkills()).toHaveLength(0) + expect(mockReaddir).not.toHaveBeenCalledWith(innerDir) + }) + + it.each([ + ["container listed first", ["a-repo", "my-skill"]], + ["direct skill listed first", ["my-skill", "z-repo"]], + ])( + "should prefer a direct skill over a container skill with the same identity (%s)", + async (_label, rootEntries) => { + const containerName = rootEntries.find((e) => e !== "my-skill")! + const containerDir = p(globalSkillsDir, containerName) + const directSkillDir = p(globalSkillsDir, "my-skill") + const directSkillMd = p(directSkillDir, "SKILL.md") + const nestedSkillDir = p(containerDir, "my-skill") + const nestedSkillMd = p(nestedSkillDir, "SKILL.md") + + mockDirectoryExists.mockImplementation(async (dir: string) => dir === globalSkillsDir) + mockRealpath.mockImplementation(async (pathArg: string) => pathArg) + mockReaddir.mockImplementation(async (dir: string) => { + if (dir === globalSkillsDir) return rootEntries + if (dir === containerDir) return ["my-skill"] + return [] + }) + mockStat.mockImplementation(async (pathArg: string) => { + if ([containerDir, directSkillDir, nestedSkillDir].includes(pathArg)) { + return { isDirectory: () => true } + } + throw enoent() + }) + mockSymlinks(containerDir) + mockFileExists.mockImplementation( + async (file: string) => file === directSkillMd || file === nestedSkillMd, + ) + mockReadFile.mockImplementation(async (file: string) => { + if (file === directSkillMd || file === nestedSkillMd) { + return `--- +name: my-skill +description: ${file === directSkillMd ? "Direct" : "Nested"} skill +--- + +# My Skill` + } + throw new Error("File not found") + }) + + await skillsManager.discoverSkills() + + const skills = skillsManager.getAllSkills() + expect(skills).toHaveLength(1) + expect(skills[0].path).toBe(directSkillMd) + expect(skills[0].description).toBe("Direct skill") + }, + ) + + it("should prefer the alphabetically first container on collisions regardless of readdir order", async () => { + const aRepoDir = p(globalSkillsDir, "a-repo") + const bRepoDir = p(globalSkillsDir, "b-repo") + const aSkillDir = p(aRepoDir, "my-skill") + const bSkillDir = p(bRepoDir, "my-skill") + const aSkillMd = p(aSkillDir, "SKILL.md") + const bSkillMd = p(bSkillDir, "SKILL.md") + + mockDirectoryExists.mockImplementation(async (dir: string) => dir === globalSkillsDir) + mockRealpath.mockImplementation(async (pathArg: string) => pathArg) + mockReaddir.mockImplementation(async (dir: string) => { + if (dir === globalSkillsDir) return ["b-repo", "a-repo"] + if (dir === aRepoDir || dir === bRepoDir) return ["my-skill"] + return [] + }) + mockStat.mockImplementation(async (pathArg: string) => { + if ([aRepoDir, bRepoDir, aSkillDir, bSkillDir].includes(pathArg)) { + return { isDirectory: () => true } + } + throw enoent() + }) + mockSymlinks(aRepoDir, bRepoDir) + mockFileExists.mockImplementation(async (file: string) => file === aSkillMd || file === bSkillMd) + mockReadFile.mockImplementation(async (file: string) => { + if (file === aSkillMd || file === bSkillMd) { + return `--- +name: my-skill +description: ${file === aSkillMd ? "From a-repo" : "From b-repo"} +--- + +# My Skill` + } + throw new Error("File not found") + }) + + await skillsManager.discoverSkills() + + const skills = skillsManager.getAllSkills() + expect(skills).toHaveLength(1) + expect(skills[0].path).toBe(aSkillMd) + }) + + it("should handle broken symlinks in container directories gracefully", async () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + const containerDir = p(globalSkillsDir, "repo-skills") + const brokenDir = p(containerDir, "broken-link") + const validSkillDir = p(containerDir, "valid-skill") + const validSkillMd = p(validSkillDir, "SKILL.md") + + mockDirectoryExists.mockImplementation(async (dir: string) => dir === globalSkillsDir) + mockRealpath.mockImplementation(async (pathArg: string) => pathArg) + mockReaddir.mockImplementation(async (dir: string) => { + if (dir === globalSkillsDir) return ["repo-skills"] + if (dir === containerDir) return ["broken-link", "valid-skill"] + return [] + }) + mockStat.mockImplementation(async (pathArg: string) => { + if (pathArg === containerDir) return { isDirectory: () => true } + if (pathArg === brokenDir) throw enoent() + if (pathArg === validSkillDir) return { isDirectory: () => true } + throw enoent() + }) + mockSymlinks(containerDir) + mockFileExists.mockImplementation(async (file: string) => file === validSkillMd) + mockReadFile.mockResolvedValue(`--- +name: valid-skill +description: A valid skill next to a broken symlink +--- + +# Valid Skill`) + + await skillsManager.discoverSkills() + + const skills = skillsManager.getAllSkills() + expect(skills).toHaveLength(1) + expect(skills[0].name).toBe("valid-skill") + expect(consoleErrorSpy).not.toHaveBeenCalledWith( + expect.stringContaining(`Failed to stat skill entry ${brokenDir}`), + expect.anything(), + ) + consoleErrorSpy.mockRestore() + }) + + it("should log an error when a symlinked container cannot be read", async () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + const containerDir = p(globalSkillsDir, "repo-skills") + const readError = new Error("EACCES: permission denied") + + mockDirectoryExists.mockImplementation(async (dir: string) => dir === globalSkillsDir) + mockRealpath.mockImplementation(async (pathArg: string) => pathArg) + mockReaddir.mockImplementation(async (dir: string) => { + if (dir === globalSkillsDir) return ["repo-skills"] + if (dir === containerDir) throw readError + return [] + }) + mockStat.mockImplementation(async (pathArg: string) => { + if (pathArg === containerDir) return { isDirectory: () => true } + throw enoent() + }) + mockSymlinks(containerDir) + mockFileExists.mockResolvedValue(false) + + await skillsManager.discoverSkills() + + expect(skillsManager.getAllSkills()).toHaveLength(0) + expect(consoleErrorSpy).toHaveBeenCalledWith(`Failed to scan skills container ${containerDir}:`, readError) + consoleErrorSpy.mockRestore() + }) + + it("should log an error when checking for a symlink fails", async () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + const entryDir = p(globalSkillsDir, "not-a-skill") + const lstatError = new Error("EIO: i/o error") + + mockDirectoryExists.mockImplementation(async (dir: string) => dir === globalSkillsDir) + mockRealpath.mockImplementation(async (pathArg: string) => pathArg) + mockReaddir.mockImplementation(async (dir: string) => (dir === globalSkillsDir ? ["not-a-skill"] : [])) + mockStat.mockImplementation(async (pathArg: string) => { + if (pathArg === entryDir) return { isDirectory: () => true } + throw enoent() + }) + mockLstat.mockRejectedValue(lstatError) + mockFileExists.mockResolvedValue(false) + + await skillsManager.discoverSkills() + + expect(skillsManager.getAllSkills()).toHaveLength(0) + expect(consoleErrorSpy).toHaveBeenCalledWith( + `Failed to check whether ${entryDir} is a symlink:`, + lstatError, + ) + consoleErrorSpy.mockRestore() + }) + + it("should keep scanning the other entries when checking one SKILL.md fails unexpectedly", async () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + // "a-broken" sorts first, so before the fix its error dropped every later entry + const brokenDir = p(globalSkillsDir, "a-broken") + const brokenMd = p(brokenDir, "SKILL.md") + const validDir = p(globalSkillsDir, "b-valid") + const validMd = p(validDir, "SKILL.md") + const accessError = Object.assign(new Error("permission denied"), { code: "EACCES" }) + + mockDirectoryExists.mockImplementation(async (dir: string) => dir === globalSkillsDir) + mockRealpath.mockImplementation(async (pathArg: string) => pathArg) + mockReaddir.mockImplementation(async (dir: string) => + dir === globalSkillsDir ? ["a-broken", "b-valid"] : [], + ) + mockStat.mockImplementation(async (pathArg: string) => { + if (pathArg === brokenDir || pathArg === validDir) return { isDirectory: () => true } + throw enoent() + }) + mockFileExists.mockImplementation(async (file: string) => { + if (file === brokenMd) throw accessError + return file === validMd + }) + mockReadFile.mockResolvedValue(`--- +name: b-valid +description: Still discovered +--- +Instructions`) + + await skillsManager.discoverSkills() + + expect(skillsManager.getAllSkills().map((s) => s.name)).toEqual(["b-valid"]) + expect(consoleErrorSpy).toHaveBeenCalledWith(`Failed to check skill entry ${brokenDir}:`, accessError) + consoleErrorSpy.mockRestore() + }) + + it("should skip hidden entries such as the staging and trash dirs of a move", async () => { + const hiddenNames = [".my-skill.moving-1-2-abc", ".my-skill.removing-1-2-abc"] + const hiddenDirs = hiddenNames.map((name) => p(globalSkillsDir, name)) + + mockDirectoryExists.mockImplementation(async (dir: string) => dir === globalSkillsDir) + mockRealpath.mockImplementation(async (pathArg: string) => pathArg) + mockReaddir.mockImplementation(async (dir: string) => (dir === globalSkillsDir ? hiddenNames : [])) + mockStat.mockResolvedValue({ isDirectory: () => true }) + mockFileExists.mockResolvedValue(true) + mockReadFile.mockResolvedValue(`--- +name: my-skill +description: A hidden copy +--- +Instructions`) + + await skillsManager.discoverSkills() + + expect(skillsManager.getAllSkills()).toHaveLength(0) + for (const dir of hiddenDirs) { + expect(mockStat).not.toHaveBeenCalledWith(dir) + } + }) + + it("should run discovery passes one at a time", async () => { + let active = 0 + let maxActive = 0 + let releaseFirst: () => void = () => {} + const firstBlocked = new Promise((resolve) => { + releaseFirst = resolve + }) + let calls = 0 + + mockDirectoryExists.mockImplementation(async (dir: string) => dir === globalSkillsDir) + mockRealpath.mockImplementation(async (pathArg: string) => pathArg) + mockReaddir.mockImplementation(async (dir: string) => { + if (dir !== globalSkillsDir) return [] + active++ + maxActive = Math.max(maxActive, active) + calls++ + if (calls === 1) await firstBlocked + active-- + return [] + }) + + const first = skillsManager.discoverSkills() + const second = skillsManager.discoverSkills() + // Let the first pass reach the blocked readdir + await vi.waitFor(() => expect(calls).toBe(1)) + releaseFirst() + await Promise.all([first, second]) + + expect(calls).toBe(2) + expect(maxActive).toBe(1) + }) + + it("should keep running later passes after a pass fails", async () => { + mockDirectoryExists.mockResolvedValue(false) + // Reading the workspace path fails once, so the first pass rejects + let failNext = true + Object.defineProperty(mockProvider, "cwd", { + configurable: true, + get: () => { + if (failNext) { + failNext = false + throw new Error("boom") + } + return PROJECT_DIR + }, + }) + + await expect(skillsManager.discoverSkills()).rejects.toThrow("boom") + await expect(skillsManager.discoverSkills()).resolves.toBeUndefined() + expect(mockDirectoryExists).toHaveBeenCalledWith(projectSkillsDir) + }) + + describe("incomplete passes", () => { + const containerEntry = p(globalSkillsDir, "shared") + const sharedSkillDir = p(SHARED_DIR, "shared-skill") + const sharedSkillMd = p(sharedSkillDir, "SKILL.md") + const directSkillDir = p(globalSkillsDir, "direct-skill") + const directSkillMd = p(directSkillDir, "SKILL.md") + const newSkillDir = p(globalSkillsDir, "new-skill") + const newSkillMd = p(newSkillDir, "SKILL.md") + + let containerReadError: Error | undefined + let rootEntries: string[] + + beforeEach(() => { + containerReadError = undefined + rootEntries = ["direct-skill", "shared"] + + mockDirectoryExists.mockImplementation(async (dir: string) => dir === globalSkillsDir) + mockRealpath.mockImplementation(async (pathArg: string) => + pathArg === containerEntry ? SHARED_DIR : pathArg, + ) + mockReaddir.mockImplementation(async (dir: string) => { + if (dir === globalSkillsDir) return rootEntries + if (dir === SHARED_DIR) { + if (containerReadError) throw containerReadError + return ["shared-skill"] + } + return [] + }) + mockStat.mockImplementation(async (pathArg: string) => { + if ([containerEntry, sharedSkillDir, directSkillDir, newSkillDir].includes(pathArg)) { + return { isDirectory: () => true } + } + throw enoent() + }) + mockSymlinks(containerEntry) + mockFileExists.mockImplementation(async (file: string) => + [sharedSkillMd, directSkillMd, newSkillMd].includes(file), + ) + mockReadFile.mockImplementation(async (file: string) => { + const name = path.basename(path.dirname(file)) + return `---\nname: ${name}\ndescription: ${name} description\n---\nInstructions` + }) + }) + + const skillNames = () => + skillsManager + .getAllSkills() + .map((s) => s.name) + .sort() + + it("should keep the skills that a transient read error hid, until a complete pass", async () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + await skillsManager.discoverSkills() + expect(skillNames()).toEqual(["direct-skill", "shared-skill"]) + + // The container can't be read for a moment. Its skill must stay available, + // and must stay read-only. + containerReadError = Object.assign(new Error("i/o error"), { code: "EIO" }) + await skillsManager.discoverSkills() + expect(skillNames()).toEqual(["direct-skill", "shared-skill"]) + await expect(skillsManager.getSkillContent("shared-skill")).resolves.toMatchObject({ + name: "shared-skill", + }) + await expect(skillsManager.deleteSkill("shared-skill", "global")).rejects.toThrow( + "container_skill_read_only", + ) + + // The container is really gone now, so a complete pass drops its skill + containerReadError = Object.assign(new Error("no such file or directory"), { code: "ENOENT" }) + await skillsManager.discoverSkills() + expect(skillNames()).toEqual(["direct-skill"]) + consoleErrorSpy.mockRestore() + }) + + it("should keep a container skill whose entry can't be inspected, until a complete pass", async () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + await skillsManager.discoverSkills() + expect(skillNames()).toEqual(["direct-skill", "shared-skill"]) + + let statError: Error = Object.assign(new Error("i/o error"), { code: "EIO" }) + mockStat.mockImplementation(async (pathArg: string) => { + if (pathArg === sharedSkillDir) throw statError + if ([containerEntry, directSkillDir].includes(pathArg)) return { isDirectory: () => true } + throw Object.assign(new Error("no such file or directory"), { code: "ENOENT" }) + }) + await skillsManager.discoverSkills() + expect(skillNames()).toEqual(["direct-skill", "shared-skill"]) + await expect(skillsManager.deleteSkill("shared-skill", "global")).rejects.toThrow( + "container_skill_read_only", + ) + + // The entry is really gone now (e.g. a broken symlink), so a complete pass drops it + statError = Object.assign(new Error("no such file or directory"), { code: "ENOENT" }) + await skillsManager.discoverSkills() + expect(skillNames()).toEqual(["direct-skill"]) + consoleErrorSpy.mockRestore() + }) + + it("should keep a skill whose SKILL.md can't be read, until a complete pass", async () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + await skillsManager.discoverSkills() + expect(skillNames()).toEqual(["direct-skill", "shared-skill"]) + + let skillMdError: Error = Object.assign(new Error("permission denied"), { code: "EACCES" }) + mockFileExists.mockImplementation(async (file: string) => { + if (file === sharedSkillMd) throw skillMdError + return file === directSkillMd + }) + await skillsManager.discoverSkills() + expect(skillNames()).toEqual(["direct-skill", "shared-skill"]) + + // A transient readFile failure is handled the same way + mockFileExists.mockImplementation(async (file: string) => [sharedSkillMd, directSkillMd].includes(file)) + mockReadFile.mockImplementation(async (file: string) => { + if (file === sharedSkillMd) throw Object.assign(new Error("i/o error"), { code: "EIO" }) + const name = path.basename(path.dirname(file)) + return `---\nname: ${name}\ndescription: ${name} description\n---\nInstructions` + }) + await skillsManager.discoverSkills() + expect(skillNames()).toEqual(["direct-skill", "shared-skill"]) + + // SKILL.md is really gone now, so a complete pass drops the skill + skillMdError = Object.assign(new Error("no such file or directory"), { code: "ENOENT" }) + mockFileExists.mockImplementation(async (file: string) => { + if (file === sharedSkillMd) throw skillMdError + return file === directSkillMd + }) + await skillsManager.discoverSkills() + expect(skillNames()).toEqual(["direct-skill"]) + consoleErrorSpy.mockRestore() + }) + + it("should still publish changes found by an incomplete pass", async () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + await skillsManager.discoverSkills() + + containerReadError = Object.assign(new Error("i/o error"), { code: "EIO" }) + rootEntries = ["new-skill", "shared"] + mockReadFile.mockImplementation(async (file: string) => { + const name = path.basename(path.dirname(file)) + return `---\nname: ${name}\ndescription: updated ${name}\n---\nInstructions` + }) + await skillsManager.discoverSkills() + + // New skills appear, and skills removed outside the failed container disappear. + // Kept skills keep their old metadata, since they weren't re-read. + expect(skillNames()).toEqual(["new-skill", "shared-skill"]) + expect(skillsManager.getSkill("new-skill", "global")?.description).toBe("updated new-skill") + expect(skillsManager.getSkill("shared-skill", "global")?.description).toBe("shared-skill description") + consoleErrorSpy.mockRestore() + }) + + it("should keep the skills of a symlinked root that fails to resolve, until it resolves again", async () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + const rootTarget = p(SHARED_DIR, "root-target") + const skillDir = p(rootTarget, "root-skill") + mockRealpath.mockImplementation(async (pathArg: string) => + pathArg === globalSkillsDir ? rootTarget : pathArg, + ) + mockReaddir.mockImplementation(async (dir: string) => (dir === rootTarget ? ["root-skill"] : [])) + mockStat.mockImplementation(async (pathArg: string) => { + if (pathArg === skillDir) return { isDirectory: () => true } + throw enoent() + }) + mockFileExists.mockImplementation(async (file: string) => file === p(skillDir, "SKILL.md")) + + await skillsManager.discoverSkills() + expect(skillNames()).toEqual(["root-skill"]) + + const ioError = Object.assign(new Error("i/o error"), { code: "EIO" }) + mockDirectoryExists.mockImplementation(async (dir: string) => { + if (dir === globalSkillsDir) throw ioError + return false + }) + await skillsManager.discoverSkills() + expect(skillNames()).toEqual(["root-skill"]) + + mockDirectoryExists.mockImplementation(async (dir: string) => dir === globalSkillsDir) + mockRealpath.mockRejectedValue(ioError) + await skillsManager.discoverSkills() + expect(skillNames()).toEqual(["root-skill"]) + + // The root is really gone now, so a pass that reads it drops the skill + mockDirectoryExists.mockResolvedValue(false) + await skillsManager.discoverSkills() + expect(skillNames()).toEqual([]) + consoleErrorSpy.mockRestore() + }) + + it("should drop a deleted skill while an unrelated entry fails persistently", async () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + await skillsManager.discoverSkills() + + containerReadError = Object.assign(new Error("permission denied"), { code: "EACCES" }) + mockRm.mockImplementation(async () => { + rootEntries = ["shared"] + }) + await skillsManager.deleteSkill("direct-skill", "global") + + expect(skillNames()).toEqual(["shared-skill"]) + consoleErrorSpy.mockRestore() + }) + + it("should keep the published skills visible while a pass is running", async () => { + await skillsManager.discoverSkills() + + let releaseScan: () => void = () => {} + const scanBlocked = new Promise((resolve) => { + releaseScan = resolve + }) + let blocked = false + mockReaddir.mockImplementation(async (dir: string) => { + if (dir === globalSkillsDir) { + blocked = true + await scanBlocked + return [] + } + return [] + }) + + const run = skillsManager.discoverSkills() + await vi.waitFor(() => expect(blocked).toBe(true)) + expect(skillNames()).toEqual(["direct-skill", "shared-skill"]) + + releaseScan() + await run + expect(skillNames()).toEqual([]) + }) + + it("should not publish a pass that finishes after dispose", async () => { + let releaseScan: () => void = () => {} + const scanBlocked = new Promise((resolve) => { + releaseScan = resolve + }) + let blocked = false + mockReaddir.mockImplementation(async (dir: string) => { + if (dir === globalSkillsDir) { + blocked = true + await scanBlocked + return rootEntries + } + return dir === SHARED_DIR ? ["shared-skill"] : [] + }) + + const run = skillsManager.discoverSkills() + await vi.waitFor(() => expect(blocked).toBe(true)) + await skillsManager.dispose() + releaseScan() + await run + + expect(skillsManager.getAllSkills()).toEqual([]) + }) + }) + + describe("container watchers", () => { + const containerEntry = p(globalSkillsDir, "shared") + + beforeEach(() => { + // Watchers are disabled under NODE_ENV=test + vi.stubEnv("NODE_ENV", "development") + + mockDirectoryExists.mockImplementation(async (dir: string) => dir === globalSkillsDir) + mockRealpath.mockImplementation(async (pathArg: string) => + pathArg === containerEntry ? SHARED_DIR : pathArg, + ) + mockStat.mockImplementation(async (pathArg: string) => { + if (pathArg === containerEntry) return { isDirectory: () => true } + throw enoent() + }) + mockSymlinks(containerEntry) + mockFileExists.mockResolvedValue(false) + }) + + afterEach(() => { + vi.unstubAllEnvs() + }) + + const containerWatcherCalls = () => + vi.mocked(vscode.RelativePattern).mock.calls.filter((call) => call[0] === SHARED_DIR) + + it("should watch a discovered container's real path once and dispose it when the container is gone", async () => { + let linked = true + mockReaddir.mockImplementation(async (dir: string) => { + if (dir === globalSkillsDir) return linked ? ["shared"] : [] + return [] + }) + + await skillsManager.discoverSkills() + await skillsManager.discoverSkills() + + expect(containerWatcherCalls()).toEqual([[SHARED_DIR, "{*,*/SKILL.md}"]]) + const createWatcher = vi.mocked(vscode.workspace.createFileSystemWatcher) + const watcherIndex = vi + .mocked(vscode.RelativePattern) + .mock.calls.findIndex((call) => call[0] === SHARED_DIR) + const watcher = createWatcher.mock.results[watcherIndex].value as { dispose: ReturnType } + expect(watcher.dispose).not.toHaveBeenCalled() + + linked = false + await skillsManager.discoverSkills() + + expect(watcher.dispose).toHaveBeenCalledTimes(1) + }) + + it("should keep a container watcher when a later pass can't read the tree", async () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + let readError: Error | undefined + mockReaddir.mockImplementation(async (dir: string) => { + if (dir !== globalSkillsDir) return [] + if (readError) throw readError + return ["shared"] + }) + + await skillsManager.discoverSkills() + const watcherIndex = vi + .mocked(vscode.RelativePattern) + .mock.calls.findIndex((call) => call[0] === SHARED_DIR) + const watcher = vi.mocked(vscode.workspace.createFileSystemWatcher).mock.results[watcherIndex] + .value as { dispose: ReturnType } + + // A transient error hides the container, so its watcher must survive + readError = Object.assign(new Error("i/o error"), { code: "EIO" }) + await skillsManager.discoverSkills() + expect(watcher.dispose).not.toHaveBeenCalled() + + // The root is really gone now, so the watcher is disposed + readError = Object.assign(new Error("no such file or directory"), { code: "ENOENT" }) + await skillsManager.discoverSkills() + expect(watcher.dispose).toHaveBeenCalledTimes(1) + consoleErrorSpy.mockRestore() + }) + + it("should keep a container's skills and watcher when checking for the symlink fails unexpectedly", async () => { + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + const sharedSkillDir = p(SHARED_DIR, "shared-skill") + const sharedSkillMd = p(sharedSkillDir, "SKILL.md") + mockReaddir.mockImplementation(async (dir: string) => { + if (dir === globalSkillsDir) return ["shared"] + if (dir === SHARED_DIR) return ["shared-skill"] + return [] + }) + mockStat.mockImplementation(async (pathArg: string) => { + if ([containerEntry, sharedSkillDir].includes(pathArg)) return { isDirectory: () => true } + throw Object.assign(new Error("no such file or directory"), { code: "ENOENT" }) + }) + mockFileExists.mockImplementation(async (file: string) => file === sharedSkillMd) + mockReadFile.mockResolvedValue(`---\nname: shared-skill\ndescription: Shared skill\n---\nInstructions`) + + await skillsManager.discoverSkills() + expect(skillsManager.getAllSkills().map((s) => s.name)).toEqual(["shared-skill"]) + const watcherIndex = vi + .mocked(vscode.RelativePattern) + .mock.calls.findIndex((call) => call[0] === SHARED_DIR) + const watcher = vi.mocked(vscode.workspace.createFileSystemWatcher).mock.results[watcherIndex] + .value as { dispose: ReturnType } + + // A transient lstat error hides that the entry is a container + mockLstat.mockRejectedValue(Object.assign(new Error("i/o error"), { code: "EIO" })) + await skillsManager.discoverSkills() + expect(skillsManager.getAllSkills().map((s) => s.name)).toEqual(["shared-skill"]) + expect(watcher.dispose).not.toHaveBeenCalled() + + // The entry is really gone now, so a complete pass drops the skill and the watcher + mockLstat.mockRejectedValue(Object.assign(new Error("no such file or directory"), { code: "ENOENT" })) + await skillsManager.discoverSkills() + expect(skillsManager.getAllSkills()).toEqual([]) + expect(watcher.dispose).toHaveBeenCalledTimes(1) + consoleErrorSpy.mockRestore() + }) + + it("should not rediscover skills for events on hidden staging or trash paths", async () => { + mockReaddir.mockImplementation(async (dir: string) => (dir === globalSkillsDir ? ["shared"] : [])) + await skillsManager.discoverSkills() + + const watcherIndex = vi + .mocked(vscode.RelativePattern) + .mock.calls.findIndex((call) => call[0] === SHARED_DIR) + const watcher = vi.mocked(vscode.workspace.createFileSystemWatcher).mock.results[watcherIndex] + .value as { onDidCreate: ReturnType } + const onCreate = watcher.onDidCreate.mock.calls[0][0] as (uri: { fsPath: string }) => Promise + const discoverSpy = vi.spyOn(skillsManager, "discoverSkills") + + await onCreate({ fsPath: p(SHARED_DIR, ".my-skill.moving-1-2-abc") }) + await onCreate({ fsPath: p(SHARED_DIR, ".my-skill.removing-1-2-abc", "SKILL.md") }) + expect(discoverSpy).not.toHaveBeenCalled() + + await onCreate({ fsPath: p(SHARED_DIR, "my-skill", "SKILL.md") }) + expect(discoverSpy).toHaveBeenCalledTimes(1) + }) + + it("should dispose container watchers on dispose", async () => { + mockReaddir.mockImplementation(async (dir: string) => (dir === globalSkillsDir ? ["shared"] : [])) + + await skillsManager.discoverSkills() + const watcher = vi.mocked(vscode.workspace.createFileSystemWatcher).mock.results[0].value as { + dispose: ReturnType + } + + await skillsManager.dispose() + + expect(watcher.dispose).toHaveBeenCalledTimes(1) + }) + }) + it("should discover skills from global .agents directory", async () => { const agentSkillDir = p(globalAgentsSkillsDir, "agent-skill") const agentSkillMd = p(agentSkillDir, "SKILL.md") @@ -1297,6 +2152,36 @@ Instructions`) "already exists", ) }) + + it("should allow creating a .roo skill when the duplicate lives in the lower-priority .agents root", async () => { + const agentsSkillDir = p(globalAgentsSkillsDir, "my-skill") + const agentsSkillMd = p(agentsSkillDir, "SKILL.md") + const rooSkillMd = p(globalSkillsDir, "my-skill", "SKILL.md") + + mockDirectoryExists.mockImplementation(async (dir: string) => dir === globalAgentsSkillsDir) + mockRealpath.mockImplementation(async (pathArg: string) => pathArg) + mockReaddir.mockImplementation(async (dir: string) => (dir === globalAgentsSkillsDir ? ["my-skill"] : [])) + mockStat.mockImplementation(async (pathArg: string) => { + if (pathArg === agentsSkillDir) return { isDirectory: () => true } + throw enoent() + }) + mockFileExists.mockImplementation(async (file: string) => file === agentsSkillMd) + mockReadFile.mockResolvedValue(`--- +name: my-skill +description: An agents skill +--- +Instructions`) + mockMkdir.mockResolvedValue(undefined) + mockWriteFile.mockResolvedValue(undefined) + + await skillsManager.discoverSkills() + expect(skillsManager.getSkill("my-skill", "global")?.path).toBe(agentsSkillMd) + + const created = await skillsManager.createSkill("my-skill", "global", "Description") + + expect(created).toBe(rooSkillMd) + expect(mockWriteFile).toHaveBeenCalledWith(rooSkillMd, expect.any(String), "utf-8") + }) }) describe("deleteSkill", () => { @@ -1357,9 +2242,70 @@ Instructions`) await expect(skillsManager.deleteSkill("non-existent", "global")).rejects.toThrow("not found") }) + + it("should refuse to delete a skill from a symlinked container", async () => { + const containerEntry = p(globalSkillsDir, "shared") + const sharedSkillDir = p(SHARED_DIR, "my-skill") + + mockDirectoryExists.mockImplementation(async (dir: string) => dir === globalSkillsDir) + mockRealpath.mockImplementation(async (pathArg: string) => + pathArg === containerEntry ? SHARED_DIR : pathArg, + ) + mockReaddir.mockImplementation(async (dir: string) => { + if (dir === globalSkillsDir) return ["shared"] + if (dir === SHARED_DIR) return ["my-skill"] + return [] + }) + mockStat.mockImplementation(async (pathArg: string) => { + if (pathArg === containerEntry || pathArg === sharedSkillDir) return { isDirectory: () => true } + throw enoent() + }) + mockSymlinks(containerEntry) + mockFileExists.mockImplementation(async (file: string) => file === p(sharedSkillDir, "SKILL.md")) + mockReadFile.mockResolvedValue(`--- +name: my-skill +description: A shared skill +--- +Instructions`) + + await skillsManager.discoverSkills() + expect(skillsManager.getSkill("my-skill", "global")).toBeDefined() + + await expect(skillsManager.deleteSkill("my-skill", "global")).rejects.toThrow( + "skills:errors.container_skill_read_only", + ) + expect(mockRm).not.toHaveBeenCalled() + + // Changing its modes would rewrite the shared SKILL.md, so that is refused too + await expect(skillsManager.updateSkillModes("my-skill", "global", ["code"])).rejects.toThrow( + "skills:errors.container_skill_read_only", + ) + expect(mockWriteFile).not.toHaveBeenCalled() + }) }) describe("moveSkill", () => { + it("should report an unreadable source root instead of 'not found'", async () => { + const sourceDir = p(globalSkillsDir, "test-skill") + const accessError = Object.assign(new Error("permission denied"), { code: "EACCES" }) + + mockDirectoryExists.mockImplementation(async (dir: string) => dir === globalSkillsDir) + mockRealpath.mockImplementation(async (pathArg: string) => pathArg) + mockReaddir.mockImplementation(async (dir: string) => (dir === globalSkillsDir ? ["test-skill"] : [])) + mockStat.mockImplementation(async (pathArg: string) => { + if (pathArg === sourceDir) return { isDirectory: () => true } + throw enoent() + }) + mockFileExists.mockImplementation(async (file: string) => file === p(sourceDir, "SKILL.md")) + mockReadFile.mockResolvedValue(`---\nname: test-skill\ndescription: A test skill\n---\nInstructions`) + + await skillsManager.discoverSkills() + mockRealpath.mockRejectedValue(accessError) + + await expect(skillsManager.moveSkill("test-skill", "global", undefined, "code")).rejects.toBe(accessError) + expect(mockRename).not.toHaveBeenCalled() + }) + it("should move a skill from generic to mode-specific directory", async () => { const sourceDir = p(globalSkillsDir, "test-skill") const testSkillMd = p(sourceDir, "SKILL.md") @@ -1755,5 +2701,419 @@ Instructions`) // Verify directory was NOT cleaned up (still has other skills) expect(mockRmdir).not.toHaveBeenCalled() }) + + it("should refuse to move a skill from a symlinked container", async () => { + const containerDir = p(globalSkillsDir, "repo") + const nestedSkillDir = p(containerDir, "my-skill") + const nestedSkillMd = p(nestedSkillDir, "SKILL.md") + + mockDirectoryExists.mockImplementation(async (dir: string) => dir === globalSkillsDir) + mockRealpath.mockImplementation(async (pathArg: string) => pathArg) + mockReaddir.mockImplementation(async (dir: string) => { + if (dir === globalSkillsDir) return ["repo"] + if (dir === containerDir) return ["my-skill"] + return [] + }) + mockStat.mockImplementation(async (pathArg: string) => { + if (pathArg === containerDir || pathArg === nestedSkillDir) { + return { isDirectory: () => true } + } + throw enoent() + }) + mockSymlinks(containerDir) + mockFileExists.mockImplementation(async (file: string) => file === nestedSkillMd) + mockReadFile.mockResolvedValue(`--- +name: my-skill +description: A container skill +--- +Instructions`) + + await skillsManager.discoverSkills() + expect(skillsManager.getSkill("my-skill", "global")?.path).toBe(nestedSkillMd) + + await expect(skillsManager.moveSkill("my-skill", "global", undefined, "code")).rejects.toThrow( + "skills:errors.container_skill_read_only", + ) + expect(mockMkdir).not.toHaveBeenCalled() + expect(mockRename).not.toHaveBeenCalled() + expect(mockCp).not.toHaveBeenCalled() + }) + + it("should refuse to move a skill that exists only under .agents", async () => { + const agentsSkillDir = p(globalAgentsSkillsCodeDir, "test-skill") + const agentsSkillMd = p(agentsSkillDir, "SKILL.md") + + mockDirectoryExists.mockImplementation(async (dir: string) => dir === globalAgentsSkillsCodeDir) + mockRealpath.mockImplementation(async (pathArg: string) => { + if (pathArg === p(GLOBAL_ROO_DIR, "skills-code")) { + throw Object.assign(new Error("no such file or directory"), { code: "ENOENT" }) + } + return pathArg + }) + mockReaddir.mockImplementation(async (dir: string) => + dir === globalAgentsSkillsCodeDir ? ["test-skill"] : [], + ) + mockStat.mockImplementation(async (pathArg: string) => { + if (pathArg === agentsSkillDir) return { isDirectory: () => true } + throw enoent() + }) + mockFileExists.mockImplementation(async (file: string) => file === agentsSkillMd) + mockReadFile.mockResolvedValue(`--- +name: test-skill +description: A shared agents skill +--- +Instructions`) + + await skillsManager.discoverSkills() + expect(skillsManager.getSkill("test-skill", "global", "code")?.path).toBe(agentsSkillMd) + + await expect(skillsManager.moveSkill("test-skill", "global", "code", "architect")).rejects.toThrow( + "not found", + ) + expect(mockMkdir).not.toHaveBeenCalled() + expect(mockRename).not.toHaveBeenCalled() + expect(mockCp).not.toHaveBeenCalled() + }) + + describe("cross-filesystem and cleanup safety", () => { + const sourceSkillsDir = p(GLOBAL_ROO_DIR, "skills-code") + const sourceDir = p(sourceSkillsDir, "test-skill") + const destDir = p(GLOBAL_ROO_DIR, "skills-architect", "test-skill") + + const setupCodeSkill = () => { + mockDirectoryExists.mockImplementation(async (dir: string) => dir === sourceSkillsDir) + mockRealpath.mockImplementation(async (pathArg: string) => pathArg) + mockReaddir.mockImplementation(async (dir: string) => (dir === sourceSkillsDir ? ["test-skill"] : [])) + mockStat.mockImplementation(async (pathArg: string) => { + if (pathArg === sourceDir) return { isDirectory: () => true } + throw enoent() + }) + mockFileExists.mockImplementation(async (file: string) => file === p(sourceDir, "SKILL.md")) + mockReadFile.mockResolvedValue(`--- +name: test-skill +description: A test skill +--- +Instructions`) + mockMkdir.mockResolvedValue(undefined) + mockRm.mockResolvedValue(undefined) + mockRmdir.mockResolvedValue(undefined) + } + + const exdevError = () => Object.assign(new Error("cross-device link not permitted"), { code: "EXDEV" }) + const enoentError = () => Object.assign(new Error("no such file or directory"), { code: "ENOENT" }) + + // Direct rename from source to dest fails across devices; same-device renames succeed. + const setupExdevRename = () => { + mockRename.mockImplementation(async (from: string, to: string) => { + if (from === sourceDir && to === destDir) { + throw exdevError() + } + }) + } + + const getStagingDir = (): string => { + const call = mockCp.mock.calls[0] + expect(call).toBeDefined() + return call[1] as string + } + + // The hidden dir the source is renamed aside to before promoting the staging copy + const getTrashDir = (): string => { + const call = mockRename.mock.calls.find(([from, to]) => from === sourceDir && to !== destDir) + expect(call).toBeDefined() + return call![1] as string + } + + it("should fall back to copy into a staging dir and promote it when rename fails with EXDEV", async () => { + setupCodeSkill() + setupExdevRename() + mockCp.mockResolvedValue(undefined) + mockLstat.mockRejectedValue(enoentError()) + + await skillsManager.discoverSkills() + await skillsManager.moveSkill("test-skill", "global", "code", "architect") + + expect(mockRename).toHaveBeenCalledWith(sourceDir, destDir) + const stagingDir = getStagingDir() + expect(stagingDir).not.toBe(destDir) + expect(path.dirname(stagingDir)).toBe(path.dirname(destDir)) + expect(mockCp).toHaveBeenCalledWith(sourceDir, stagingDir, { + recursive: true, + errorOnExist: true, + force: false, + verbatimSymlinks: true, + }) + const trashDir = getTrashDir() + expect(path.dirname(trashDir)).toBe(path.dirname(sourceDir)) + expect(path.basename(trashDir).startsWith(".")).toBe(true) + + // The source is moved aside before the staging copy is promoted + const renameTargets = mockRename.mock.calls.map(([, to]) => to) + expect(renameTargets.indexOf(trashDir)).toBeLessThan(renameTargets.lastIndexOf(destDir)) + expect(mockRename).toHaveBeenLastCalledWith(stagingDir, destDir) + + expect(mockRm).toHaveBeenCalledWith(trashDir, { recursive: true, force: true }) + expect(mockRm).not.toHaveBeenCalledWith(sourceDir, expect.anything()) + expect(mockRm).not.toHaveBeenCalledWith(destDir, expect.anything()) + }) + + it("should resolve and keep a single discoverable copy when deleting the old source fails after promotion", async () => { + setupCodeSkill() + setupExdevRename() + mockCp.mockResolvedValue(undefined) + mockLstat.mockRejectedValue(enoentError()) + mockRm.mockImplementation(async (target: string) => { + if (target !== getStagingDir()) { + throw Object.assign(new Error("permission denied"), { code: "EACCES" }) + } + }) + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + await skillsManager.discoverSkills() + await expect(skillsManager.moveSkill("test-skill", "global", "code", "architect")).resolves.toBe( + undefined, + ) + + const trashDir = getTrashDir() + expect(mockRename).toHaveBeenLastCalledWith(getStagingDir(), destDir) + expect(mockRm).toHaveBeenCalledWith(trashDir, { recursive: true, force: true }) + // The original source path no longer holds the skill, so no duplicate remains + expect(mockRename).not.toHaveBeenCalledWith(trashDir, sourceDir) + expect(consoleErrorSpy).toHaveBeenCalledWith( + expect.stringContaining(trashDir), + expect.objectContaining({ code: "EACCES" }), + ) + consoleErrorSpy.mockRestore() + }) + + it("should restore the source when promoting the staging copy fails after moving the source aside", async () => { + setupCodeSkill() + mockRename.mockImplementation(async (from: string, to: string) => { + if (from === sourceDir && to === destDir) { + throw exdevError() + } + if (to === destDir) { + throw Object.assign(new Error("directory not empty"), { code: "ENOTEMPTY" }) + } + }) + mockCp.mockResolvedValue(undefined) + mockLstat.mockRejectedValue(enoentError()) + + await skillsManager.discoverSkills() + await expect(skillsManager.moveSkill("test-skill", "global", "code", "architect")).rejects.toThrow( + "directory not empty", + ) + + const trashDir = getTrashDir() + expect(mockRename).toHaveBeenCalledWith(trashDir, sourceDir) + expect(mockRm).toHaveBeenCalledWith(getStagingDir(), { recursive: true, force: true }) + expect(mockRm).not.toHaveBeenCalledWith(trashDir, expect.anything()) + expect(mockRm).not.toHaveBeenCalledWith(sourceDir, expect.anything()) + expect(mockRm).not.toHaveBeenCalledWith(destDir, expect.anything()) + }) + + it("should not promote the staging copy when moving the source aside fails", async () => { + setupCodeSkill() + mockRename.mockImplementation(async (from: string, to: string) => { + if (from === sourceDir && to === destDir) { + throw exdevError() + } + if (from === sourceDir) { + throw Object.assign(new Error("resource busy"), { code: "EBUSY" }) + } + }) + mockCp.mockResolvedValue(undefined) + mockLstat.mockRejectedValue(enoentError()) + + await skillsManager.discoverSkills() + await expect(skillsManager.moveSkill("test-skill", "global", "code", "architect")).rejects.toThrow( + "resource busy", + ) + + const stagingDir = getStagingDir() + expect(mockRename).not.toHaveBeenCalledWith(stagingDir, destDir) + expect(mockRm).toHaveBeenCalledWith(stagingDir, { recursive: true, force: true }) + expect(mockRm).not.toHaveBeenCalledWith(sourceDir, expect.anything()) + }) + + it("should not touch an existing destination directory when the EXDEV fallback finds it", async () => { + setupCodeSkill() + setupExdevRename() + mockCp.mockResolvedValue(undefined) + // destDir exists (e.g., contains unrelated files but no SKILL.md) + mockLstat.mockResolvedValue({ isDirectory: () => true }) + + await skillsManager.discoverSkills() + await expect(skillsManager.moveSkill("test-skill", "global", "code", "architect")).rejects.toThrow( + "Destination already exists", + ) + + const stagingDir = getStagingDir() + expect(mockRename).not.toHaveBeenCalledWith(stagingDir, destDir) + expect(mockRm).toHaveBeenCalledWith(stagingDir, { recursive: true, force: true }) + expect(mockRm).not.toHaveBeenCalledWith(destDir, expect.anything()) + expect(mockRm).not.toHaveBeenCalledWith(sourceDir, expect.anything()) + }) + + it("should clean up only the staging dir when promoting the copy fails", async () => { + setupCodeSkill() + mockRename.mockImplementation(async (from: string, to: string) => { + if (from === sourceDir && to === destDir) { + throw exdevError() + } + if (to === destDir) { + throw Object.assign(new Error("directory not empty"), { code: "ENOTEMPTY" }) + } + }) + mockCp.mockResolvedValue(undefined) + mockLstat.mockRejectedValue(enoentError()) + + await skillsManager.discoverSkills() + await expect(skillsManager.moveSkill("test-skill", "global", "code", "architect")).rejects.toThrow( + "directory not empty", + ) + + const stagingDir = getStagingDir() + expect(mockRm).toHaveBeenCalledWith(stagingDir, { recursive: true, force: true }) + expect(mockRm).not.toHaveBeenCalledWith(destDir, expect.anything()) + expect(mockRm).not.toHaveBeenCalledWith(sourceDir, expect.anything()) + }) + + it("should not copy when rename succeeds on the same filesystem", async () => { + setupCodeSkill() + mockRename.mockResolvedValue(undefined) + + await skillsManager.discoverSkills() + await skillsManager.moveSkill("test-skill", "global", "code", "architect") + + expect(mockCp).not.toHaveBeenCalled() + expect(mockRm).not.toHaveBeenCalled() + }) + + it("should remove only the partial staging copy and keep the source when the EXDEV copy fails", async () => { + setupCodeSkill() + setupExdevRename() + mockCp.mockRejectedValue(new Error("copy failed")) + + await skillsManager.discoverSkills() + await expect(skillsManager.moveSkill("test-skill", "global", "code", "architect")).rejects.toThrow( + "copy failed", + ) + + const stagingDir = getStagingDir() + expect(mockRm).toHaveBeenCalledWith(stagingDir, { recursive: true, force: true }) + expect(mockRm).not.toHaveBeenCalledWith(destDir, expect.anything()) + expect(mockRm).not.toHaveBeenCalledWith(sourceDir, expect.anything()) + }) + + // Directory entry for readdir({ withFileTypes: true }) + const dirent = (name: string, kind: "dir" | "link" | "file") => ({ + name, + isDirectory: () => kind === "dir", + isSymbolicLink: () => kind === "link", + }) + + // The link lives at /refs/shared, so these resolve outside the skill dir + it.each([ + ["a parent-relative link", "../../_shared/f"], + ["a link to the skill dir's parent", "../.."], + ])( + "should fail while the source still exists when the copy has %s escaping the skill", + async (_label, target) => { + setupCodeSkill() + setupExdevRename() + mockCp.mockResolvedValue(undefined) + mockLstat.mockRejectedValue(enoentError()) + mockReaddir.mockImplementation(async (dir: string, options?: { withFileTypes?: boolean }) => { + if (!options?.withFileTypes) return dir === sourceSkillsDir ? ["test-skill"] : [] + if (dir === getStagingDir()) return [dirent("refs", "dir"), dirent("SKILL.md", "file")] + if (dir === p(getStagingDir(), "refs")) return [dirent("shared", "link")] + return [] + }) + mockReadlink.mockResolvedValue(target) + + await skillsManager.discoverSkills() + await expect(skillsManager.moveSkill("test-skill", "global", "code", "architect")).rejects.toThrow( + "skills:errors.symlink_escapes_skill", + ) + + const stagingDir = getStagingDir() + expect(mockReadlink).toHaveBeenCalledWith(p(stagingDir, "refs", "shared")) + // The source is never moved aside and the staging copy is removed + expect(mockRename).toHaveBeenCalledTimes(1) + expect(mockRename).not.toHaveBeenCalledWith(stagingDir, destDir) + expect(mockRm).toHaveBeenCalledWith(stagingDir, { recursive: true, force: true }) + expect(mockRm).not.toHaveBeenCalledWith(sourceDir, expect.anything()) + }, + ) + + it("should allow relative links that stay inside the skill and absolute links", async () => { + setupCodeSkill() + setupExdevRename() + mockCp.mockResolvedValue(undefined) + mockLstat.mockRejectedValue(enoentError()) + mockReaddir.mockImplementation(async (dir: string, options?: { withFileTypes?: boolean }) => { + if (!options?.withFileTypes) return dir === sourceSkillsDir ? ["test-skill"] : [] + if (dir === getStagingDir()) return [dirent("docs", "dir"), dirent("abs", "link")] + if (dir === p(getStagingDir(), "docs")) return [dirent("inner", "link")] + return [] + }) + mockReadlink.mockImplementation(async (link: string) => + link.endsWith("abs") ? p(SHARED_DIR, "f") : p("..", "SKILL.md"), + ) + + await skillsManager.discoverSkills() + await skillsManager.moveSkill("test-skill", "global", "code", "architect") + + expect(mockReadlink).toHaveBeenCalledTimes(2) + expect(mockRename).toHaveBeenLastCalledWith(getStagingDir(), destDir) + }) + + it("should rethrow non-EXDEV rename errors without copying", async () => { + setupCodeSkill() + mockRename.mockRejectedValue(Object.assign(new Error("permission denied"), { code: "EACCES" })) + + await skillsManager.discoverSkills() + await expect(skillsManager.moveSkill("test-skill", "global", "code", "architect")).rejects.toThrow( + "permission denied", + ) + + expect(mockCp).not.toHaveBeenCalled() + }) + + it("should not remove a symlinked skills directory after moving its last skill out", async () => { + // skills-code -> /shared/skills + const sharedSkillDir = p(SHARED_DIR, "test-skill") + mockDirectoryExists.mockImplementation(async (dir: string) => dir === sourceSkillsDir) + mockRealpath.mockImplementation(async (pathArg: string) => + pathArg === sourceSkillsDir ? SHARED_DIR : pathArg, + ) + let discovering = true + mockReaddir.mockImplementation(async (dir: string) => + dir === SHARED_DIR && discovering ? ["test-skill"] : [], + ) + mockStat.mockImplementation(async (pathArg: string) => { + if (pathArg === sharedSkillDir) return { isDirectory: () => true } + throw enoent() + }) + mockSymlinks(sourceSkillsDir) + mockFileExists.mockImplementation(async (file: string) => file === p(sharedSkillDir, "SKILL.md")) + mockReadFile.mockResolvedValue(`--- +name: test-skill +description: A test skill +--- +Instructions`) + mockMkdir.mockResolvedValue(undefined) + mockRename.mockResolvedValue(undefined) + mockRmdir.mockResolvedValue(undefined) + + await skillsManager.discoverSkills() + discovering = false + await skillsManager.moveSkill("test-skill", "global", "code", "architect") + + expect(mockRename).toHaveBeenCalledWith(sharedSkillDir, destDir) + expect(mockRmdir).not.toHaveBeenCalled() + }) + }) }) })