From dc6208bc7558c5e70ca9d3c8bb522ccbfa770db3 Mon Sep 17 00:00:00 2001 From: Haileyesus <118998054+blackmammoth@users.noreply.github.com> Date: Mon, 22 Jun 2026 14:21:03 +0300 Subject: [PATCH] fix(skills): validate installs before writing Preserve bundled files and normalize fallback names across skill installation paths. Validate complete batches before writing and reject existing targets to avoid partial installs. Keep project metadata and make folder selection tolerant of casing and cancelled dialogs. --- server/modules/providers/provider.routes.ts | 1 + .../shared/skills/skills.provider.ts | 85 ++++++++++++++----- server/modules/providers/tests/skills.test.ts | 48 +++++++++++ .../skills/hooks/useProviderSkills.ts | 8 +- src/components/skills/view/ProviderSkills.tsx | 18 ++-- 5 files changed, 130 insertions(+), 30 deletions(-) diff --git a/server/modules/providers/provider.routes.ts b/server/modules/providers/provider.routes.ts index 74592beb..9b9fb576 100644 --- a/server/modules/providers/provider.routes.ts +++ b/server/modules/providers/provider.routes.ts @@ -197,6 +197,7 @@ const parseProviderSkillCreatePayload = (payload: unknown): ProviderSkillCreateI content: body.content, directoryName: body.directoryName, fileName: body.fileName, + files: body.files, }] : null; diff --git a/server/modules/providers/shared/skills/skills.provider.ts b/server/modules/providers/shared/skills/skills.provider.ts index 55f88184..e42237b7 100644 --- a/server/modules/providers/shared/skills/skills.provider.ts +++ b/server/modules/providers/shared/skills/skills.provider.ts @@ -1,5 +1,5 @@ import path from 'node:path'; -import { mkdir, writeFile } from 'node:fs/promises'; +import { mkdir, stat, writeFile } from 'node:fs/promises'; import type { IProviderSkills } from '@/shared/interfaces.js'; import type { @@ -33,6 +33,30 @@ const normalizeSkillDirectoryName = (value: string): string => ( .replace(/^-+|-+$/g, '') ); +type PendingSkillInstall = { + skillDirectoryPath: string; + skillPath: string; + content: string; + supportingFiles: Array<{ + targetPath: string; + content: string | Buffer; + }>; + skill: ProviderSkill; +}; + +const pathExists = async (targetPath: string): Promise => { + try { + await stat(targetPath); + return true; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') { + return false; + } + + throw error; + } +}; + const resolveSkillSupportingFilePath = ( skillDirectoryPath: string, relativePath: string, @@ -131,10 +155,8 @@ export abstract class SkillsProvider implements IProviderSkills { }); } - await mkdir(globalSkillSource.rootDir, { recursive: true }); - - const createdSkills: ProviderSkill[] = []; const seenSkillPaths = new Set(); + const pendingInstalls: PendingSkillInstall[] = []; for (const [index, entry] of input.entries.entries()) { const content = typeof entry.content === 'string' ? entry.content.trim() : ''; @@ -147,8 +169,10 @@ export abstract class SkillsProvider implements IProviderSkills { const fileNameFallback = readOptionalString(entry.fileName); const requestedDirectoryName = readOptionalString(entry.directoryName); - const fallbackSkillName = requestedDirectoryName - ?? (fileNameFallback ? stripMarkdownExtension(fileNameFallback) : `skill-${index + 1}`); + const fallbackSkillName = normalizeSkillDirectoryName( + requestedDirectoryName + ?? (fileNameFallback ? stripMarkdownExtension(fileNameFallback) : `skill-${index + 1}`), + ); const definition = readProviderSkillMarkdownDefinitionFromContent(content, fallbackSkillName); const resolvedDirectoryName = normalizeSkillDirectoryName( requestedDirectoryName ?? definition.name, @@ -172,6 +196,13 @@ export abstract class SkillsProvider implements IProviderSkills { } seenSkillPaths.add(normalizedSkillPath); + if (await pathExists(skillDirectoryPath)) { + throw new AppError(`Skill target "${resolvedDirectoryName}" already exists.`, { + code: 'PROVIDER_SKILL_ALREADY_EXISTS', + statusCode: 409, + }); + } + const supportingFiles = (entry.files ?? []).map((file) => ({ targetPath: resolveSkillSupportingFilePath(skillDirectoryPath, file.relativePath, index), content: file.encoding === 'base64' @@ -189,30 +220,38 @@ export abstract class SkillsProvider implements IProviderSkills { seenSupportingPaths.add(file.targetPath); } - await mkdir(skillDirectoryPath, { recursive: true }); - await writeFile(skillPath, `${content}\n`, 'utf8'); - for (const file of supportingFiles) { - await mkdir(path.dirname(file.targetPath), { recursive: true }); - await writeFile(file.targetPath, file.content); - } - const command = globalSkillSource.commandForSkill ? globalSkillSource.commandForSkill(definition.name) : `${globalSkillSource.commandPrefix ?? '/'}${definition.name}`; - createdSkills.push({ - provider: this.provider, - name: definition.name, - description: definition.description, - command, - scope: globalSkillSource.scope, - sourcePath: skillPath, - pluginName: globalSkillSource.pluginName, - pluginId: globalSkillSource.pluginId, + pendingInstalls.push({ + skillDirectoryPath, + skillPath, + content, + supportingFiles, + skill: { + provider: this.provider, + name: definition.name, + description: definition.description, + command, + scope: globalSkillSource.scope, + sourcePath: skillPath, + pluginName: globalSkillSource.pluginName, + pluginId: globalSkillSource.pluginId, + }, }); } - return createdSkills; + for (const install of pendingInstalls) { + await mkdir(install.skillDirectoryPath, { recursive: true }); + await writeFile(install.skillPath, `${install.content}\n`, 'utf8'); + for (const file of install.supportingFiles) { + await mkdir(path.dirname(file.targetPath), { recursive: true }); + await writeFile(file.targetPath, file.content); + } + } + + return pendingInstalls.map((install) => install.skill); } protected abstract getSkillSources(workspacePath: string): Promise; diff --git a/server/modules/providers/tests/skills.test.ts b/server/modules/providers/tests/skills.test.ts index 16c660ae..eea13945 100644 --- a/server/modules/providers/tests/skills.test.ts +++ b/server/modules/providers/tests/skills.test.ts @@ -568,6 +568,54 @@ test('providerSkillsService adds global skills for claude, codex, gemini, and cu 'console.log("codex skill");\n', ); + const fallbackNamedSkills = await providerSkillsService.addProviderSkills('codex', { + entries: [ + { + fileName: 'fallback / skill.md', + content: '---\ndescription: Normalized fallback skill\n---\n\nFallback body.\n', + }, + ], + }); + const fallbackNamedSkill = fallbackNamedSkills[0]; + assert.ok(fallbackNamedSkill); + assert.equal(fallbackNamedSkill.name, 'fallback-skill'); + assert.equal(fallbackNamedSkill.command, '$fallback-skill'); + assert.equal( + fallbackNamedSkill.sourcePath.endsWith(path.join('.agents', 'skills', 'fallback-skill', 'SKILL.md')), + true, + ); + + await assert.rejects( + providerSkillsService.addProviderSkills('codex', { + entries: [ + { + directoryName: 'uploaded-codex-folder', + content: '---\nname: replacement\n---\n\nReplacement body.\n', + }, + ], + }), + /already exists/i, + ); + assert.match(await fs.readFile(createdCodexSkill.sourcePath, 'utf8'), /Codex body\./); + + const pendingBatchSkillPath = path.join(tempRoot, '.agents', 'skills', 'pending-batch', 'SKILL.md'); + await assert.rejects( + providerSkillsService.addProviderSkills('codex', { + entries: [ + { + directoryName: 'pending-batch', + content: '---\nname: pending-batch\n---\n\nPending body.\n', + }, + { + directoryName: 'pending-batch', + content: '---\nname: duplicate-batch\n---\n\nDuplicate body.\n', + }, + ], + }), + /duplicate skill target/i, + ); + await assert.rejects(fs.stat(pendingBatchSkillPath), { code: 'ENOENT' }); + const createdGeminiSkills = await providerSkillsService.addProviderSkills('gemini', { entries: [ { diff --git a/src/components/skills/hooks/useProviderSkills.ts b/src/components/skills/hooks/useProviderSkills.ts index 225ca3c2..4655acd5 100644 --- a/src/components/skills/hooks/useProviderSkills.ts +++ b/src/components/skills/hooks/useProviderSkills.ts @@ -111,8 +111,12 @@ const normalizeSkill = ( sourcePath: String(skill.sourcePath ?? ''), pluginName: typeof skill.pluginName === 'string' ? skill.pluginName : undefined, pluginId: typeof skill.pluginId === 'string' ? skill.pluginId : undefined, - projectDisplayName: shouldAttachProject ? project?.displayName : undefined, - projectPath: shouldAttachProject ? project?.path : undefined, + projectDisplayName: shouldAttachProject + ? project?.displayName ?? skill.projectDisplayName + : skill.projectDisplayName, + projectPath: shouldAttachProject + ? project?.path ?? skill.projectPath + : skill.projectPath, }; }; diff --git a/src/components/skills/view/ProviderSkills.tsx b/src/components/skills/view/ProviderSkills.tsx index 8c7f9b22..186b6d35 100644 --- a/src/components/skills/view/ProviderSkills.tsx +++ b/src/components/skills/view/ProviderSkills.tsx @@ -170,14 +170,18 @@ const buildQueuedSkillFolders = (selectedFiles: File[]): QueuedSkillFile[] => { return skillRoots.map((skillRoot) => { const skillFiles = files.filter(({ relativePath }) => { - const owningRoot = skillRoots.find((candidateRoot) => ( - relativePath === `${candidateRoot}/SKILL.md` - || relativePath.startsWith(`${candidateRoot}/`) - )); + const owningRoot = skillRoots.find((candidateRoot) => { + const normalizedRelativePath = relativePath.toLowerCase(); + const normalizedSkillPath = `${candidateRoot}/skill.md`.toLowerCase(); + return normalizedRelativePath === normalizedSkillPath + || relativePath.startsWith(`${candidateRoot}/`); + }); return owningRoot === skillRoot; }); const skillSourceFile = skillFiles.find( - ({ relativePath }) => relativePath === `${skillRoot}/SKILL.md`, + ({ relativePath }) => ( + relativePath.toLowerCase() === `${skillRoot}/skill.md`.toLowerCase() + ), ); if (!skillSourceFile) { throw new Error(`Could not read SKILL.md from ${getBaseName(skillRoot)}.`); @@ -303,6 +307,10 @@ export default function ProviderSkills({ selectedProvider, currentProjects }: Pr }, [queueSkillFolders]); const handleFolderSelection = useCallback((selectedFiles: File[]) => { + if (selectedFiles.length === 0) { + return; + } + try { queueSkillFolders(selectedFiles); setSubmitError(null);