refactor(skills): extract skill loading into dedicated module (#34)

- Add skill-loader.ts module centralizing skill loading logic
- Prefix skill command names with 'systematic:' for namespacing
- Prefix skill descriptions with '(systematic - Skill)' for clarity
- Wrap skill templates in <skill-instruction> tags with base directory
- Extract body from wrapped template when displaying via tool
- Fix double-prefix bug in formatSkillsXml integration
- Refactor config-handler to use new loadSkill function
- Refactor skill-tool to use centralized loading utilities
- Add comprehensive unit tests for skill-loader module
- Add integration tests for config handler skill registration
- Update existing tests to verify new prefixing behavior

Author: marcusrbrown

src/lib/config-handler.ts

@@ -4,7 +4,8 @@ import { extractCommandFrontmatter, findCommandsInDir } from './commands.js'
 import { loadConfig } from './config.js'
 import { convertFileWithCache } from './converter.js'
 import { stripFrontmatter } from './frontmatter.js'
-import { findSkillsInDir, type SkillInfo } from './skills.js'
+import { type LoadedSkill, loadSkill } from './skill-loader.js'
+import { findSkillsInDir } from './skills.js'
 
 export interface ConfigHandlerDeps {
   directory: string
@@ -58,18 +59,10 @@ function loadCommandAsConfig(commandInfo: {
   }
 }
 
-function loadSkillAsCommand(skillInfo: SkillInfo): CommandConfig | null {
-  try {
-    const converted = convertFileWithCache(skillInfo.skillFile, 'skill', {
-      source: 'bundled',
-    })
-
-    return {
-      template: stripFrontmatter(converted),
-      description: skillInfo.description || `${skillInfo.name} skill`,
-    }
-  } catch {
-    return null
+function loadSkillAsCommand(loaded: LoadedSkill): CommandConfig {
+  return {
+    template: loaded.wrappedTemplate,
+    description: loaded.description,
   }
 }
 
@@ -122,9 +115,9 @@ function collectSkillsAsCommands(
   for (const skillInfo of skillList) {
     if (disabledSkills.includes(skillInfo.name)) continue
 
-    const config = loadSkillAsCommand(skillInfo)
-    if (config) {
-      commands[skillInfo.name] = config
+    const loaded = loadSkill(skillInfo)
+    if (loaded) {
+      commands[loaded.prefixedName] = loadSkillAsCommand(loaded)
     }
   }
 

src/lib/skill-loader.ts

@@ -0,0 +1,75 @@
+import path from 'node:path'
+import { convertFileWithCache } from './converter.js'
+import { stripFrontmatter } from './frontmatter.js'
+import type { SkillInfo } from './skills.js'
+
+const SKILL_PREFIX = 'systematic:'
+const SKILL_DESCRIPTION_PREFIX = '(systematic - Skill) '
+
+export interface LoadedSkill {
+  name: string
+  prefixedName: string
+  description: string
+  path: string
+  skillFile: string
+  wrappedTemplate: string
+}
+
+export function formatSkillCommandName(name: string): string {
+  if (name.startsWith(SKILL_PREFIX)) {
+    return name
+  }
+  return `${SKILL_PREFIX}${name}`
+}
+
+export function formatSkillDescription(
+  description: string,
+  fallbackName: string,
+): string {
+  const desc = description || `${fallbackName} skill`
+  if (desc.startsWith(SKILL_DESCRIPTION_PREFIX)) {
+    return desc
+  }
+  return `${SKILL_DESCRIPTION_PREFIX}${desc}`
+}
+
+export function wrapSkillTemplate(skillPath: string, body: string): string {
+  const skillDir = path.dirname(skillPath)
+  return `<skill-instruction>
+Base directory for this skill: ${skillDir}/
+File references (@path) in this skill are relative to this directory.
+
+${body.trim()}
+</skill-instruction>`
+}
+
+export function extractSkillBody(wrappedTemplate: string): string {
+  const match = wrappedTemplate.match(
+    /<skill-instruction>([\s\S]*?)<\/skill-instruction>/,
+  )
+  return match ? match[1].trim() : wrappedTemplate
+}
+
+export function loadSkill(skillInfo: SkillInfo): LoadedSkill | null {
+  try {
+    const converted = convertFileWithCache(skillInfo.skillFile, 'skill', {
+      source: 'bundled',
+    })
+    const body = stripFrontmatter(converted)
+    const wrappedTemplate = wrapSkillTemplate(skillInfo.skillFile, body)
+
+    return {
+      name: skillInfo.name,
+      prefixedName: formatSkillCommandName(skillInfo.name),
+      description: formatSkillDescription(
+        skillInfo.description,
+        skillInfo.name,
+      ),
+      path: skillInfo.path,
+      skillFile: skillInfo.skillFile,
+      wrappedTemplate,
+    }
+  } catch {
+    return null
+  }
+}

src/lib/skill-tool.ts

@@ -1,25 +1,13 @@
-import fs from 'node:fs'
 import path from 'node:path'
 import type { ToolDefinition } from '@opencode-ai/plugin'
 import { tool } from '@opencode-ai/plugin/tool'
-import { convertContent } from './converter.js'
-import { stripFrontmatter } from './frontmatter.js'
-import type { SkillInfo } from './skills.js'
+import {
+  extractSkillBody,
+  type LoadedSkill,
+  loadSkill,
+} from './skill-loader.js'
 import { findSkillsInDir, formatSkillsXml } from './skills.js'
 
-function wrapSkillContent(skillPath: string, content: string): string {
-  const skillDir = path.dirname(skillPath)
-  const converted = convertContent(content, 'skill', { source: 'bundled' })
-  const body = stripFrontmatter(converted)
-
-  return `<skill-instruction>
-Base directory for this skill: ${skillDir}/
-File references (@path) in this skill are relative to this directory.
-
-${body.trim()}
-</skill-instruction>`
-}
-
 export interface SkillToolOptions {
   bundledSkillsDir: string
   disabledSkills: string[]
@@ -28,15 +16,23 @@ export interface SkillToolOptions {
 export function createSkillTool(options: SkillToolOptions): ToolDefinition {
   const { bundledSkillsDir, disabledSkills } = options
 
-  const getSystematicSkills = (): SkillInfo[] => {
+  const getSystematicSkills = (): LoadedSkill[] => {
     return findSkillsInDir(bundledSkillsDir)
       .filter((s) => !disabledSkills.includes(s.name))
+      .map((skillInfo) => loadSkill(skillInfo))
+      .filter((s): s is LoadedSkill => s !== null)
       .sort((a, b) => a.name.localeCompare(b.name))
   }
 
   const buildDescription = (): string => {
     const skills = getSystematicSkills()
-    const systematicXml = formatSkillsXml(skills)
+    const skillInfos = skills.map((s) => ({
+      name: s.name,
+      description: s.description,
+      path: s.path,
+      skillFile: s.skillFile,
+    }))
+    const systematicXml = formatSkillsXml(skillInfos)
 
     const baseDescription = `Load a skill to get detailed instructions for a specific task.
 
@@ -73,25 +69,17 @@ Use this when a task matches an available skill's description.`
       const matchedSkill = skills.find((s) => s.name === normalizedName)
 
       if (matchedSkill) {
-        try {
-          const content = fs.readFileSync(matchedSkill.skillFile, 'utf8')
-          const wrapped = wrapSkillContent(matchedSkill.skillFile, content)
-
-          return `## Skill: systematic:${matchedSkill.name}
-
-**Base directory**: ${matchedSkill.path}
-
-${wrapped}`
-        } catch (error) {
-          const errorMessage =
-            error instanceof Error ? error.message : String(error)
-          throw new Error(
-            `Failed to load skill "${requestedName}": ${errorMessage}`,
-          )
-        }
+        const body = extractSkillBody(matchedSkill.wrappedTemplate)
+        const dir = path.dirname(matchedSkill.skillFile)
+
+        return `## Skill: ${matchedSkill.prefixedName}
+
+**Base directory**: ${dir}
+
+${body}`
       }
 
-      const availableSystematic = skills.map((s) => `systematic:${s.name}`)
+      const availableSystematic = skills.map((s) => s.prefixedName)
       throw new Error(
         `Skill "${requestedName}" not found. Available systematic skills: ${availableSystematic.join(', ')}`,
       )

tests/integration/opencode.test.ts

@@ -2,6 +2,8 @@ import { afterEach, beforeEach, describe, expect, test } from 'bun:test'
 import fs from 'node:fs'
 import os from 'node:os'
 import path from 'node:path'
+import type { Config } from '@opencode-ai/sdk'
+import { createConfigHandler } from '../../src/lib/config-handler.ts'
 
 const OPENCODE_AVAILABLE = (() => {
   const result = Bun.spawnSync(['which', 'opencode'])
@@ -118,14 +120,126 @@ describe.skipIf(!OPENCODE_AVAILABLE)('opencode integration', () => {
         'What skills are available? List the systematic skills you can load.',
       )
 
-      expect(result.stdout).toMatch(
-        /systematic:brainstorming|systematic:.*|available.*skills/i,
-      )
+      expect(result.stdout).toMatch(/brainstorming|systematic.*skills/i)
     },
     TIMEOUT_MS * MAX_RETRIES,
   )
 })
 
+describe('config handler integration', () => {
+  let testEnv: {
+    tempDir: string
+    bundledDir: string
+    projectDir: string
+  }
+
+  beforeEach(() => {
+    const tempBase = fs.mkdtempSync(
+      path.join(os.tmpdir(), 'systematic-config-integration-'),
+    )
+
+    testEnv = {
+      tempDir: tempBase,
+      bundledDir: path.join(tempBase, 'bundled'),
+      projectDir: path.join(tempBase, 'project'),
+    }
+
+    fs.mkdirSync(path.join(testEnv.bundledDir, 'skills', 'test-skill'), {
+      recursive: true,
+    })
+    fs.mkdirSync(path.join(testEnv.bundledDir, 'agents'), { recursive: true })
+    fs.mkdirSync(path.join(testEnv.bundledDir, 'commands'), { recursive: true })
+    fs.mkdirSync(testEnv.projectDir, { recursive: true })
+
+    fs.writeFileSync(
+      path.join(testEnv.bundledDir, 'skills', 'test-skill', 'SKILL.md'),
+      `---
+name: test-skill
+description: A skill for integration testing
+---
+# Test Skill
+
+Integration test content.`,
+    )
+  })
+
+  afterEach(() => {
+    if (testEnv.tempDir) {
+      fs.rmSync(testEnv.tempDir, { recursive: true, force: true })
+    }
+  })
+
+  test('registers skills with systematic: prefix in command name', async () => {
+    const handler = createConfigHandler({
+      directory: testEnv.projectDir,
+      bundledSkillsDir: path.join(testEnv.bundledDir, 'skills'),
+      bundledAgentsDir: path.join(testEnv.bundledDir, 'agents'),
+      bundledCommandsDir: path.join(testEnv.bundledDir, 'commands'),
+    })
+
+    const config: Config = {}
+    await handler(config)
+
+    const commandNames = Object.keys(config.command || {})
+    expect(commandNames).toContain('systematic:test-skill')
+    expect(commandNames).not.toContain('test-skill')
+  })
+
+  test('adds (systematic - Skill) prefix to skill descriptions', async () => {
+    const handler = createConfigHandler({
+      directory: testEnv.projectDir,
+      bundledSkillsDir: path.join(testEnv.bundledDir, 'skills'),
+      bundledAgentsDir: path.join(testEnv.bundledDir, 'agents'),
+      bundledCommandsDir: path.join(testEnv.bundledDir, 'commands'),
+    })
+
+    const config: Config = {}
+    await handler(config)
+
+    const skillCommand = config.command?.['systematic:test-skill']
+    expect(skillCommand?.description).toMatch(/^\(systematic - Skill\) /)
+    expect(skillCommand?.description).toBe(
+      '(systematic - Skill) A skill for integration testing',
+    )
+  })
+
+  test('wraps skill template in <skill-instruction> tags', async () => {
+    const handler = createConfigHandler({
+      directory: testEnv.projectDir,
+      bundledSkillsDir: path.join(testEnv.bundledDir, 'skills'),
+      bundledAgentsDir: path.join(testEnv.bundledDir, 'agents'),
+      bundledCommandsDir: path.join(testEnv.bundledDir, 'commands'),
+    })
+
+    const config: Config = {}
+    await handler(config)
+
+    const skillCommand = config.command?.['systematic:test-skill']
+    expect(skillCommand?.template).toContain('<skill-instruction>')
+    expect(skillCommand?.template).toContain('</skill-instruction>')
+    expect(skillCommand?.template).toContain('Base directory for this skill:')
+    expect(skillCommand?.template).toContain('Integration test content.')
+  })
+
+  test('skill template does not contain frontmatter', async () => {
+    const handler = createConfigHandler({
+      directory: testEnv.projectDir,
+      bundledSkillsDir: path.join(testEnv.bundledDir, 'skills'),
+      bundledAgentsDir: path.join(testEnv.bundledDir, 'agents'),
+      bundledCommandsDir: path.join(testEnv.bundledDir, 'commands'),
+    })
+
+    const config: Config = {}
+    await handler(config)
+
+    const skillCommand = config.command?.['systematic:test-skill']
+    expect(skillCommand?.template).not.toContain('name: test-skill')
+    expect(skillCommand?.template).not.toContain(
+      'description: A skill for integration testing',
+    )
+  })
+})
+
 describe('opencode availability check', () => {
   test('reports opencode installation status', () => {
     console.log(`OpenCode available: ${OPENCODE_AVAILABLE}`)