fix(skill-loader): support unambiguous short skill names
This commit is contained in:
@@ -3,6 +3,7 @@
|
|||||||
import { describe, it, expect, beforeEach, afterEach } from "bun:test"
|
import { describe, it, expect, beforeEach, afterEach } from "bun:test"
|
||||||
import { join } from "node:path"
|
import { join } from "node:path"
|
||||||
import { tmpdir } from "node:os"
|
import { tmpdir } from "node:os"
|
||||||
|
import { mkdirSync, writeFileSync } from "node:fs"
|
||||||
import {
|
import {
|
||||||
clearSkillCache,
|
clearSkillCache,
|
||||||
resolveSkillContent,
|
resolveSkillContent,
|
||||||
@@ -11,6 +12,13 @@ import {
|
|||||||
resolveMultipleSkillsAsync,
|
resolveMultipleSkillsAsync,
|
||||||
} from "./skill-content"
|
} from "./skill-content"
|
||||||
|
|
||||||
|
function createNestedSkill(baseDir: string, namespace: string, name: string, content: string): void {
|
||||||
|
const dir = join(baseDir, "skills", namespace, name)
|
||||||
|
mkdirSync(dir, { recursive: true })
|
||||||
|
const yaml = `---\nname: ${name}\ndescription: ${namespace}/${name} skill\n---\n${content}`
|
||||||
|
writeFileSync(join(dir, "SKILL.md"), yaml)
|
||||||
|
}
|
||||||
|
|
||||||
let originalEnv: Record<string, string | undefined>
|
let originalEnv: Record<string, string | undefined>
|
||||||
let testConfigDir: string
|
let testConfigDir: string
|
||||||
|
|
||||||
@@ -185,6 +193,58 @@ describe("resolveSkillContentAsync", () => {
|
|||||||
// then: returns null
|
// then: returns null
|
||||||
expect(result).toBeNull()
|
expect(result).toBeNull()
|
||||||
})
|
})
|
||||||
|
|
||||||
|
it("resolves nested skill by unique short name async", async () => {
|
||||||
|
// given: a discovered nested skill superpowers/systematic-debugging
|
||||||
|
createNestedSkill(testConfigDir, "superpowers", "systematic-debugging", "Short name test content")
|
||||||
|
|
||||||
|
// when: resolving by short name
|
||||||
|
const result = await resolveSkillContentAsync("systematic-debugging")
|
||||||
|
|
||||||
|
// then: finds the nested skill
|
||||||
|
expect(result).not.toBeNull()
|
||||||
|
expect(result).toContain("Short name test content")
|
||||||
|
})
|
||||||
|
|
||||||
|
it("returns null for ambiguous short name async", async () => {
|
||||||
|
// given: two skills with same short name in different namespaces
|
||||||
|
createNestedSkill(testConfigDir, "superpowers", "debugging", "superpowers content")
|
||||||
|
createNestedSkill(testConfigDir, "utils", "debugging", "utils content")
|
||||||
|
|
||||||
|
// when: resolving by ambiguous short name
|
||||||
|
const result = await resolveSkillContentAsync("debugging")
|
||||||
|
|
||||||
|
// then: ambiguous => null
|
||||||
|
expect(result).toBeNull()
|
||||||
|
})
|
||||||
|
|
||||||
|
it("prefers exact match over short name match async", async () => {
|
||||||
|
// given: an exact skill name "debugging" and a nested "superpowers/debugging"
|
||||||
|
createNestedSkill(testConfigDir, "superpowers", "debugging", "nested debugging")
|
||||||
|
// Also create the exact match by placing it at dir root
|
||||||
|
const dir = join(testConfigDir, "skills")
|
||||||
|
mkdirSync(dir, { recursive: true })
|
||||||
|
writeFileSync(join(dir, "debugging.md"), "---\nname: debugging\ndescription: exact debugging\n---\nexact match content")
|
||||||
|
|
||||||
|
// when: resolving by name "debugging"
|
||||||
|
const result = await resolveSkillContentAsync("debugging")
|
||||||
|
|
||||||
|
// then: prefers exact match over the nested one
|
||||||
|
expect(result).not.toBeNull()
|
||||||
|
expect(result).toContain("exact match content")
|
||||||
|
})
|
||||||
|
|
||||||
|
it("is case-insensitive for short name matching async", async () => {
|
||||||
|
// given: a nested skill with lowercase name
|
||||||
|
createNestedSkill(testConfigDir, "superpowers", "systematic-debugging", "case insensitive match")
|
||||||
|
|
||||||
|
// when: resolving by uppercase short name
|
||||||
|
const result = await resolveSkillContentAsync("Systematic-Debugging")
|
||||||
|
|
||||||
|
// then: finds it case-insensitively
|
||||||
|
expect(result).not.toBeNull()
|
||||||
|
expect(result).toContain("case insensitive match")
|
||||||
|
})
|
||||||
})
|
})
|
||||||
|
|
||||||
describe("resolveMultipleSkillsAsync", () => {
|
describe("resolveMultipleSkillsAsync", () => {
|
||||||
@@ -377,6 +437,50 @@ describe("resolveMultipleSkillsAsync", () => {
|
|||||||
expect(result.resolved.size).toBe(0)
|
expect(result.resolved.size).toBe(0)
|
||||||
expect(result.notFound).toEqual([])
|
expect(result.notFound).toEqual([])
|
||||||
})
|
})
|
||||||
|
|
||||||
|
it("resolves nested skill by unique short name in mixed batch", async () => {
|
||||||
|
// given: nested skill and builtin skill
|
||||||
|
createNestedSkill(testConfigDir, "superpowers", "systematic-debugging", "short name resolved")
|
||||||
|
|
||||||
|
// when: mixing short name with full builtin name
|
||||||
|
const result = await resolveMultipleSkillsAsync(["systematic-debugging", "playwright"])
|
||||||
|
|
||||||
|
// then: both resolved
|
||||||
|
expect(result.resolved.size).toBe(2)
|
||||||
|
expect(result.notFound).toEqual([])
|
||||||
|
expect(result.resolved.get("systematic-debugging")).toContain("short name resolved")
|
||||||
|
expect(result.resolved.get("playwright")).toContain("Playwright Browser Automation")
|
||||||
|
})
|
||||||
|
|
||||||
|
it("does not resolve ambiguous short name in batch", async () => {
|
||||||
|
// given: two skills with same short name
|
||||||
|
createNestedSkill(testConfigDir, "superpowers", "debugging", "sp content")
|
||||||
|
createNestedSkill(testConfigDir, "utils", "debugging", "utils content")
|
||||||
|
|
||||||
|
// when: resolving ambiguous short name with builtin
|
||||||
|
const result = await resolveMultipleSkillsAsync(["debugging", "playwright"])
|
||||||
|
|
||||||
|
// then: debugging not found, playwright resolved
|
||||||
|
expect(result.resolved.size).toBe(1)
|
||||||
|
expect(result.resolved.has("playwright")).toBe(true)
|
||||||
|
expect(result.notFound).toContain("debugging")
|
||||||
|
})
|
||||||
|
|
||||||
|
it("prefers exact match over short name in batch", async () => {
|
||||||
|
// given: an exact skill and a nested skill with same base name
|
||||||
|
const dir = join(testConfigDir, "skills")
|
||||||
|
mkdirSync(dir, { recursive: true })
|
||||||
|
writeFileSync(join(dir, "debugging.md"), "---\nname: debugging\ndescription: exact debugging\n---\nexact match content")
|
||||||
|
createNestedSkill(testConfigDir, "superpowers", "debugging", "nested content")
|
||||||
|
|
||||||
|
// when: resolving "debugging" in batch
|
||||||
|
const result = await resolveMultipleSkillsAsync(["debugging", "playwright"])
|
||||||
|
|
||||||
|
// then: exact match wins
|
||||||
|
expect(result.resolved.size).toBe(2)
|
||||||
|
expect(result.notFound).toEqual([])
|
||||||
|
expect(result.resolved.get("debugging")).toContain("exact match content")
|
||||||
|
})
|
||||||
})
|
})
|
||||||
|
|
||||||
describe("resolveSkillContent with browserProvider", () => {
|
describe("resolveSkillContent with browserProvider", () => {
|
||||||
|
|||||||
@@ -1,9 +1,9 @@
|
|||||||
|
import { matchSkillByName } from "../../tools/skill/skill-matcher"
|
||||||
import { createBuiltinSkills } from "../builtin-skills/skills"
|
import { createBuiltinSkills } from "../builtin-skills/skills"
|
||||||
import type { LoadedSkill } from "./types"
|
|
||||||
import type { SkillResolutionOptions } from "./skill-resolution-options"
|
|
||||||
import { injectGitMasterConfig } from "./git-master-template-injection"
|
import { injectGitMasterConfig } from "./git-master-template-injection"
|
||||||
import { getAllSkills } from "./skill-discovery"
|
|
||||||
import { extractSkillTemplate } from "./loaded-skill-template-extractor"
|
import { extractSkillTemplate } from "./loaded-skill-template-extractor"
|
||||||
|
import { getAllSkills } from "./skill-discovery"
|
||||||
|
import type { SkillResolutionOptions } from "./skill-resolution-options"
|
||||||
|
|
||||||
export function resolveSkillContent(skillName: string, options?: SkillResolutionOptions): string | null {
|
export function resolveSkillContent(skillName: string, options?: SkillResolutionOptions): string | null {
|
||||||
const skills = createBuiltinSkills({
|
const skills = createBuiltinSkills({
|
||||||
@@ -56,7 +56,7 @@ export async function resolveSkillContentAsync(
|
|||||||
options?: SkillResolutionOptions
|
options?: SkillResolutionOptions
|
||||||
): Promise<string | null> {
|
): Promise<string | null> {
|
||||||
const allSkills = await getAllSkills(options)
|
const allSkills = await getAllSkills(options)
|
||||||
const skill = allSkills.find((loadedSkill) => loadedSkill.name === skillName)
|
const skill = matchSkillByName(allSkills, skillName)
|
||||||
if (!skill) return null
|
if (!skill) return null
|
||||||
|
|
||||||
const template = await extractSkillTemplate(skill)
|
const template = await extractSkillTemplate(skill)
|
||||||
@@ -73,16 +73,12 @@ export async function resolveMultipleSkillsAsync(
|
|||||||
options?: SkillResolutionOptions
|
options?: SkillResolutionOptions
|
||||||
): Promise<{ resolved: Map<string, string>; notFound: string[] }> {
|
): Promise<{ resolved: Map<string, string>; notFound: string[] }> {
|
||||||
const allSkills = await getAllSkills(options)
|
const allSkills = await getAllSkills(options)
|
||||||
const skillMap = new Map<string, LoadedSkill>()
|
|
||||||
for (const skill of allSkills) {
|
|
||||||
skillMap.set(skill.name, skill)
|
|
||||||
}
|
|
||||||
|
|
||||||
const resolved = new Map<string, string>()
|
const resolved = new Map<string, string>()
|
||||||
const notFound: string[] = []
|
const notFound: string[] = []
|
||||||
|
|
||||||
for (const name of skillNames) {
|
for (const name of skillNames) {
|
||||||
const skill = skillMap.get(name)
|
const skill = matchSkillByName(allSkills, name)
|
||||||
if (skill) {
|
if (skill) {
|
||||||
const template = await extractSkillTemplate(skill)
|
const template = await extractSkillTemplate(skill)
|
||||||
if (name === "git-master") {
|
if (name === "git-master") {
|
||||||
|
|||||||
@@ -3208,7 +3208,90 @@ describe("sisyphus-task", () => {
|
|||||||
})
|
})
|
||||||
})
|
})
|
||||||
|
|
||||||
describe("buildSystemContent", () => {
|
describe("delegate task with short skill name", () => {
|
||||||
|
let envCleanup: Record<string, string | undefined>
|
||||||
|
|
||||||
|
beforeEach(() => {
|
||||||
|
envCleanup = {
|
||||||
|
CLAUDE_CONFIG_DIR: process.env.CLAUDE_CONFIG_DIR,
|
||||||
|
OPENCODE_CONFIG_DIR: process.env.OPENCODE_CONFIG_DIR,
|
||||||
|
}
|
||||||
|
})
|
||||||
|
|
||||||
|
afterEach(() => {
|
||||||
|
for (const [key, value] of Object.entries(envCleanup)) {
|
||||||
|
if (value !== undefined) {
|
||||||
|
process.env[key] = value
|
||||||
|
} else {
|
||||||
|
delete process.env[key]
|
||||||
|
}
|
||||||
|
}
|
||||||
|
})
|
||||||
|
|
||||||
|
test("resolves short named discovered skill without reporting not found", async () => {
|
||||||
|
// given: a nested discovered skill under a temp config dir
|
||||||
|
const { join } = require("node:path")
|
||||||
|
const { tmpdir } = require("node:os")
|
||||||
|
const { mkdirSync, writeFileSync } = require("node:fs")
|
||||||
|
const unique = `delegate-shortname-${Date.now()}-${Math.random().toString(16).slice(2)}`
|
||||||
|
const testConfigDir = join(tmpdir(), unique)
|
||||||
|
process.env.CLAUDE_CONFIG_DIR = testConfigDir
|
||||||
|
process.env.OPENCODE_CONFIG_DIR = testConfigDir
|
||||||
|
const skillDir = join(testConfigDir, "skills", "superpowers", "systematic-debugging")
|
||||||
|
mkdirSync(skillDir, { recursive: true })
|
||||||
|
writeFileSync(
|
||||||
|
join(skillDir, "SKILL.md"),
|
||||||
|
"---\nname: systematic-debugging\ndescription: Nested debug skill\n---\nDebug instructions"
|
||||||
|
)
|
||||||
|
clearSkillCache()
|
||||||
|
|
||||||
|
const { createDelegateTask } = require("./tools")
|
||||||
|
const mockManager = { launch: async () => ({}) }
|
||||||
|
const mockClient = {
|
||||||
|
app: { agents: async () => ({ data: [] }) },
|
||||||
|
config: { get: async () => ({ data: { model: SYSTEM_DEFAULT_MODEL } }) },
|
||||||
|
session: {
|
||||||
|
get: async () => ({ data: { directory: "/project" } }),
|
||||||
|
create: async () => ({ data: { id: "ses_shortname_test" } }),
|
||||||
|
prompt: async () => ({ data: {} }),
|
||||||
|
promptAsync: async () => ({ data: {} }),
|
||||||
|
messages: async () => ({
|
||||||
|
data: [{ info: { role: "assistant" }, parts: [{ type: "text", text: "Done" }] }],
|
||||||
|
}),
|
||||||
|
status: async () => ({ data: {} }),
|
||||||
|
},
|
||||||
|
}
|
||||||
|
|
||||||
|
const tool = createDelegateTask({
|
||||||
|
manager: mockManager,
|
||||||
|
client: mockClient,
|
||||||
|
})
|
||||||
|
|
||||||
|
const toolContext = {
|
||||||
|
sessionID: "parent-session",
|
||||||
|
messageID: "parent-message",
|
||||||
|
agent: "sisyphus",
|
||||||
|
abort: new AbortController().signal,
|
||||||
|
}
|
||||||
|
|
||||||
|
// when: using short name in load_skills
|
||||||
|
const result = await tool.execute(
|
||||||
|
{
|
||||||
|
description: "Test short name resolution",
|
||||||
|
prompt: "Do something",
|
||||||
|
category: "ultrabrain",
|
||||||
|
run_in_background: false,
|
||||||
|
load_skills: ["systematic-debugging"],
|
||||||
|
},
|
||||||
|
toolContext
|
||||||
|
)
|
||||||
|
|
||||||
|
// then: should NOT report "Skills not found"
|
||||||
|
expect(result).not.toContain("Skills not found")
|
||||||
|
})
|
||||||
|
})
|
||||||
|
|
||||||
|
describe("buildSystemContent", () => {
|
||||||
test("returns undefined when no skills and no category promptAppend", () => {
|
test("returns undefined when no skills and no category promptAppend", () => {
|
||||||
// given
|
// given
|
||||||
const { buildSystemContent } = require("./tools")
|
const { buildSystemContent } = require("./tools")
|
||||||
|
|||||||
Reference in New Issue
Block a user