From 09938ff7b83216bfe494bb159c5edc8ccd04b05e Mon Sep 17 00:00:00 2001 From: VIVPM Date: Fri, 30 Oct 2026 17:00:41 +0530 Subject: [PATCH] fix(skills): mark installed catalog skills --- src/main/skills.ts | 39 +-------- src/renderer/src/components/SkillsTab.tsx | 19 +++-- src/shared/skillCatalogMatch.ts | 74 +++++++++++++++++ test/skill-catalog-match.test.cjs | 71 ++++++++++++++++ test/skills-catalog-state.test.cjs | 99 +++++++++++++++++++++++ 5 files changed, 258 insertions(+), 44 deletions(-) create mode 100644 src/shared/skillCatalogMatch.ts create mode 100644 test/skill-catalog-match.test.cjs create mode 100644 test/skills-catalog-state.test.cjs diff --git a/src/main/skills.ts b/src/main/skills.ts index 1e2121d255..83123a95b6 100644 --- a/src/main/skills.ts +++ b/src/main/skills.ts @@ -24,6 +24,8 @@ import { existsSync, readFileSync, readdirSync, statSync, writeFileSync, mkdirSy import { join, basename, dirname, resolve, sep } from 'node:path'; import { homedir } from 'node:os'; import { getText } from './fetchText'; +import { parseGitHubSourceUrl, safeSkillDirName } from '../shared/skillCatalogMatch'; +export { parseGitHubSourceUrl, safeSkillDirName } from '../shared/skillCatalogMatch'; export interface LocalSkill { id: string; @@ -279,43 +281,6 @@ const MAX_FILES = 60; const MAX_TOTAL_BYTES = 2 * 1024 * 1024; const MAX_DEPTH = 5; -/** - * A GitHub source URL → the pieces the contents API needs. - * - * Two shapes, because the catalog uses both: a folder inside a repo - * (`owner/repo/tree//`, 145 entries) and a whole repo whose root IS - * the skill (`owner/repo`, 81 entries). For the latter `ref` is empty and the - * caller omits it, which makes the API use the repo's default branch — there is - * no reliable way to guess between `main` and `master` and no need to. - */ -export function parseGitHubSourceUrl(url: string): { owner: string; repo: string; ref: string; path: string } | null { - const clean = url.trim().replace(/[#?].*$/, '').replace(/\/+$/, ''); - const tree = /^https:\/\/github\.com\/([^/]+)\/([^/]+)\/tree\/([^/]+)\/?(.*)$/.exec(clean); - if (tree) { - const [, owner, repo, ref, path] = tree; - if (!owner || !repo || !ref) return null; - return { owner, repo, ref, path: path.replace(/\/+$/, '') }; - } - const root = /^https:\/\/github\.com\/([^/]+)\/([^/]+)$/.exec(clean); - if (root) { - const [, owner, repo] = root; - // Reserved paths that are not repositories. - if (['orgs', 'topics', 'collections', 'sponsors', 'features'].includes(owner.toLowerCase())) return null; - return { owner, repo: repo.replace(/\.git$/i, ''), ref: '', path: '' }; - } - return null; -} - -/** A folder name we are willing to create. Anything with a separator, a dot-dot, - * or a leading dot is rejected outright rather than massaged into safety. */ -export function safeSkillDirName(raw: string): string | null { - const base = raw.trim().split('/').pop() ?? ''; - if (!base || base === '.' || base === '..') return null; - if (!/^[A-Za-z0-9][A-Za-z0-9._-]{0,63}$/.test(base)) return null; - if (base.includes('..')) return null; - return base; -} - function getJson(url: string): Promise { return getText(url).then((t) => JSON.parse(t) as T); } diff --git a/src/renderer/src/components/SkillsTab.tsx b/src/renderer/src/components/SkillsTab.tsx index 606ea8f8bd..31a7cf73a7 100644 --- a/src/renderer/src/components/SkillsTab.tsx +++ b/src/renderer/src/components/SkillsTab.tsx @@ -13,6 +13,7 @@ import { useCallback, useEffect, useMemo, useState } from 'react'; import { useTranslation } from 'react-i18next'; import { PixelButton } from './PixelButton'; import type { LocalSkill, CatalogSkill } from '../../../preload'; +import { installedSkillKeys, isCatalogSkillInstalled } from '../../../shared/skillCatalogMatch'; type Mode = 'installed' | 'browse'; @@ -115,6 +116,8 @@ export function SkillsTab({ agentCwd }: { agentCwd?: string }) { s.name.toLowerCase().includes(q) || s.description.toLowerCase().includes(q)).length; }, [catalog, owner, category, q]); + const installedKeys = useMemo(() => installedSkillKeys(local ?? []), [local]); + const install = async (c: CatalogSkill) => { setAction((a) => ({ ...a, [c.url]: { busy: true } })); try { @@ -307,8 +310,9 @@ export function SkillsTab({ agentCwd }: { agentCwd?: string }) { {' · '}{t('skillsTab.curatedBy')}
- {shownCatalog.map((s) => ( -
+ {shownCatalog.map((s) => { + const installed = isCatalogSkillInstalled(s, installedKeys); + return
{s.name.toUpperCase()} @@ -322,10 +326,11 @@ export function SkillsTab({ agentCwd }: { agentCwd?: string }) {
-
- ))} +
; + })}
)} diff --git a/src/shared/skillCatalogMatch.ts b/src/shared/skillCatalogMatch.ts new file mode 100644 index 0000000000..712dc54841 --- /dev/null +++ b/src/shared/skillCatalogMatch.ts @@ -0,0 +1,74 @@ +/** Source identity shared by the installer and the catalog's Installed state. + * No Node imports: the same URL parsing and folder validation run in both + * Electron's main process and the renderer. */ + +/** + * A GitHub source URL → the pieces the contents API needs. + * + * Two shapes, because the catalog uses both: a folder inside a repo + * (`owner/repo/tree//`, 145 entries) and a whole repo whose root IS + * the skill (`owner/repo`, 81 entries). For the latter `ref` is empty and the + * caller omits it, which makes the API use the repo's default branch — there is + * no reliable way to guess between `main` and `master` and no need to. + */ +export function parseGitHubSourceUrl(url: string): { owner: string; repo: string; ref: string; path: string } | null { + const clean = url.trim().replace(/[#?].*$/, '').replace(/\/+$/, ''); + const tree = /^https:\/\/github\.com\/([^/]+)\/([^/]+)\/tree\/([^/]+)\/?(.*)$/.exec(clean); + if (tree) { + const [, owner, repo, ref, path] = tree; + if (!owner || !repo || !ref) return null; + return { owner, repo, ref, path: path.replace(/\/+$/, '') }; + } + const root = /^https:\/\/github\.com\/([^/]+)\/([^/]+)$/.exec(clean); + if (root) { + const [, owner, repo] = root; + // Reserved paths that are not repositories. + if (['orgs', 'topics', 'collections', 'sponsors', 'features'].includes(owner.toLowerCase())) return null; + return { owner, repo: repo.replace(/\.git$/i, ''), ref: '', path: '' }; + } + return null; +} + +/** A folder name we are willing to create. Anything with a separator, a dot-dot, + * or a leading dot is rejected outright rather than massaged into safety. */ +export function safeSkillDirName(raw: string): string | null { + const base = raw.trim().split('/').pop() ?? ''; + if (!base || base === '.' || base === '..') return null; + if (!/^[A-Za-z0-9][A-Za-z0-9._-]{0,63}$/.test(base)) return null; + if (base.includes('..')) return null; + return base; +} + +interface InstalledSkillRef { + path: string; + provider: string; +} + +/** Claude skills can be installed outside this app (including as symlinks). + * Match the install folder, not arbitrary frontmatter/display names: an alias + * can otherwise hide a different skill whose destination is still available. + * OpenCode/Codex plugins are not Claude skills, even with an identical name. */ +export function installedSkillKeys(local: readonly InstalledSkillRef[]): Set { + const keys = new Set(); + for (const skill of local) { + if (skill.provider !== 'claude') continue; + const folder = skill.path.replace(/[\\/]+$/, '').split(/[\\/]/).pop() ?? ''; + const key = folder.toLowerCase(); + if (key) keys.add(key); + } + return keys; +} + +/** A tree URL's folder wins over its display name. Repo-root entries use the + * catalog name, as the installer does for a root SKILL.md or named subfolder. + * Unresolved third-party directory pages stay installable: their URL alone + * does not identify a destination, and guessing can hide a different skill. */ +export function isCatalogSkillInstalled( + skill: { name: string; url: string }, + installed: ReadonlySet +): boolean { + const source = parseGitHubSourceUrl(skill.url); + if (!source) return false; + const folder = safeSkillDirName(source.path || skill.name); + return folder !== null && installed.has(folder.toLowerCase()); +} diff --git a/test/skill-catalog-match.test.cjs b/test/skill-catalog-match.test.cjs new file mode 100644 index 0000000000..15fd4e8346 --- /dev/null +++ b/test/skill-catalog-match.test.cjs @@ -0,0 +1,71 @@ +'use strict'; + +const test = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const loadTs = require('./load-ts.cjs'); + +const skill = (name, folder = name, provider = 'claude') => ({ + id: name, name, description: '', provider, scope: 'user', path: folder +}); +const catalogSkill = (name, source = `skills/${name}`) => ({ + name, url: `https://github.com/example/skills/tree/main/${source}` +}); +const docx = catalogSkill('Word Documents', 'skills/docx'); + +const { installedSkillKeys, isCatalogSkillInstalled } = loadTs('src/shared/skillCatalogMatch.ts'); + +test('matches POSIX and Windows folders with spaces, trailing separators, and mixed case', () => { + const keys = installedSkillKeys([ + skill('Document tools', '/home/New Contributor/.claude/skills/DOCX/'), + skill('Other name', 'C:\\Users\\New Contributor\\.claude\\skills\\PDF\\') + ]); + assert.equal(isCatalogSkillInstalled(catalogSkill('Word Documents', 'skills/docx/'), keys), true); + assert.equal(isCatalogSkillInstalled(catalogSkill('PDF tools', 'skills/pdf'), keys), true); +}); + +test('a repo-root catalog name identifies the destination, not a frontmatter alias', () => { + const keys = installedSkillKeys([skill('color-expert', '/home/me/.claude/skills/custom-folder')]); + assert.equal(isCatalogSkillInstalled({ name: 'COLOR-EXPERT', url: 'https://github.com/example/colors.git/?tab=readme#usage' }, keys), false); + assert.equal(isCatalogSkillInstalled(catalogSkill('Colors', 'skills/color-expert'), keys), false); + assert.equal(isCatalogSkillInstalled({ name: 'CUSTOM-FOLDER', url: 'https://github.com/example/colors' }, keys), true); +}); + +test('unsafe or unresolved sources are not guessed from a matching display name', () => { + const keys = installedSkillKeys([skill('docx', '/home/me/.claude/skills/docx')]); + for (const url of [ + 'https://officialskills.sh/example/docx', + 'https://example.com/docx', + 'https://github.com/orgs/docx', + 'https://github.com/example/skills/tree/main/.docx', + 'https://github.com/example/skills/tree/main/a..docx' + ]) assert.equal(isCatalogSkillInstalled({ name: 'docx', url }, keys), false, url); + assert.equal(isCatalogSkillInstalled({ name: '', url: 'https://github.com/example/docx' }, keys), false); +}); + +test('URL query strings and fragments do not change an explicit source folder', () => { + const keys = installedSkillKeys([skill('docx', '/home/me/.claude/skills/docx')]); + assert.equal(isCatalogSkillInstalled({ ...docx, url: docx.url + '/?plain=1#readme' }, keys), true); +}); + +test('Codex plugins cannot satisfy a Claude skill match', () => { + assert.equal(installedSkillKeys([skill('docx', '/home/me/.codex/plugins/docx', 'codex')]).size, 0); +}); + +test('real discovery reports a symlinked project skill and the catalog recognizes it', (t) => { + const os = require('node:os'); + const { listLocalSkills } = loadTs('src/main/skills.ts'); + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'skill catalog with spaces-')); + t.after(() => fs.rmSync(root, { recursive: true, force: true })); + const target = path.join(root, 'shared', 'document-tools'); + const skills = path.join(root, 'project', '.claude', 'skills'); + fs.mkdirSync(target, { recursive: true }); + fs.mkdirSync(skills, { recursive: true }); + fs.writeFileSync(path.join(target, 'SKILL.md'), '---\nname: document-tools\ndescription: A fixture\n---\n'); + fs.symlinkSync(target, path.join(skills, 'docx'), process.platform === 'win32' ? 'junction' : 'dir'); + const local = listLocalSkills({ cwds: [path.join(root, 'project')], bundledDir: null }); + const discovered = local.find((s) => s.path === path.join(skills, 'docx')); + assert.ok(discovered, 'discovery follows the symlink and retains its install-folder name'); + assert.equal(isCatalogSkillInstalled(docx, installedSkillKeys([discovered])), true); +}); diff --git a/test/skills-catalog-state.test.cjs b/test/skills-catalog-state.test.cjs new file mode 100644 index 0000000000..2583c0525b --- /dev/null +++ b/test/skills-catalog-state.test.cjs @@ -0,0 +1,99 @@ +'use strict'; + +const test = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const ts = require('typescript'); +const React = require('react'); +const { renderToStaticMarkup } = require('react-dom/server'); +const loadTs = require('./load-ts.cjs'); + +const skill = (name, folder = name, provider = 'claude') => ({ + id: name, name, description: '', provider, scope: 'user', path: folder +}); +const catalogSkill = (name, source = `skills/${name}`) => ({ + name, description: '', owner: 'example', category: 'Tools', + url: `https://github.com/example/skills/tree/main/${source}` +}); + +// Render the production component with a loaded IPC snapshot. No Electron, +// network, disk mutation, or additional test dependency is needed. The state +// order deliberately mirrors SkillsTab; an extra state fails this fixture. +function renderCatalog(local, catalog, action = {}) { + const states = ['browse', '', local, catalog, null, 'all', 'all', false, action, null]; + let state = 0; + const hooks = { + ...React, + useState: () => { + assert.ok(state < states.length, 'unexpected SkillsTab state'); + return [states[state++], () => {}]; + }, + useEffect: () => {}, useMemo: (fn) => fn(), useCallback: (fn) => fn + }; + const t = (key) => key; + const filename = path.join(__dirname, '../src/renderer/src/components/SkillsTab.tsx'); + const js = ts.transpileModule(fs.readFileSync(filename, 'utf8'), { + fileName: filename, + compilerOptions: { module: ts.ModuleKind.CommonJS, target: ts.ScriptTarget.ES2022, jsx: ts.JsxEmit.ReactJSX } + }).outputText; + const mod = { exports: {} }; + const localRequire = (id) => { + if (id === 'react') return hooks; + if (id === 'react-i18next') return { useTranslation: () => ({ t }) }; + if (id === './PixelButton') return { PixelButton: ({ children }) => React.createElement('button', null, children) }; + if (id.startsWith('../../../shared/')) return loadTs(`src/shared/${id.split('/').pop()}.ts`); + return require(id); + }; + new Function('require', 'module', 'exports', js)(localRequire, mod, mod.exports); + const html = renderToStaticMarkup(React.createElement(mod.exports.SkillsTab)); + assert.equal(state, states.length, 'fixture matches SkillsTab state'); + return html; +} + +function actionButton(html) { + const buttons = [...html.matchAll(/]*)>([^<]*)<\/button>/g)]; + const button = buttons.find(([, , text]) => /^skillsTab\.(install|installed|installing)$/.test(text)); + assert.ok(button, 'catalog action button exists'); + return { label: button[2], disabled: /\bdisabled=/.test(button[1]), style: button[1] }; +} + +const docx = catalogSkill('Word Documents', 'skills/docx'); + +test('Browse marks an already-installed tree skill as Installed, not Install', () => { + const button = actionButton(renderCatalog([skill('Document tools', '/home/New Contributor/.claude/skills/docx')], [docx])); + console.log(`docx catalog action: ${button.label}, disabled=${button.disabled}`); + assert.equal(button.label, 'skillsTab.installed'); + assert.equal(button.disabled, true); + assert.ok(button.style.includes('--cth-cream-200'), 'existing quiet style'); +}); + +test('an empty local snapshot offers Install', () => { + const button = actionButton(renderCatalog([], [docx])); + assert.equal(button.label, 'skillsTab.install'); + assert.equal(button.disabled, false); +}); + +test('a same-named OpenCode plugin does not disable a Claude catalog install', () => { + const button = actionButton(renderCatalog([skill('docx', '/home/me/.config/opencode/plugin/docx', 'opencode')], [docx])); + assert.equal(button.label, 'skillsTab.install'); + assert.equal(button.disabled, false); +}); + +test('an unrelated source folder is installable even when its display name matches', () => { + const button = actionButton(renderCatalog([skill('docx', '/home/me/.claude/skills/docx')], [catalogSkill('docx', 'word-tools')])); + assert.equal(button.label, 'skillsTab.install'); + assert.equal(button.disabled, false); +}); + +test('an in-flight install keeps its existing busy state', () => { + const button = actionButton(renderCatalog([], [docx], { [docx.url]: { busy: true } })); + assert.equal(button.label, 'skillsTab.installing'); + assert.equal(button.disabled, true); +}); + +test('a Claude frontmatter alias does not hide an unrelated destination', () => { + const button = actionButton(renderCatalog([skill('docx', '/home/me/.claude/skills/company-documents')], [docx])); + assert.equal(button.label, 'skillsTab.install'); + assert.equal(button.disabled, false); +});