From 9c4ae26945021022cce77e15b4f73c5e25832dbc Mon Sep 17 00:00:00 2001 From: Sami Jawhar Date: Wed, 20 May 2026 12:26:31 +0000 Subject: [PATCH] fix(skill-discovery): load native OpenCode skills in task delegation --- .../team-member-error-handler.ts | 4 - src/plugin/tool-registry.ts | 1 + .../delegate-task/prompt-builder.test.ts | 65 ++++++ src/tools/delegate-task/prompt-builder.ts | 22 ++- .../delegate-task/skill-resolver.test.ts | 185 ++++++++++++++++++ src/tools/delegate-task/skill-resolver.ts | 89 +++++++-- src/tools/delegate-task/tools.test.ts | 6 +- src/tools/delegate-task/tools.ts | 21 ++ src/tools/delegate-task/types.ts | 8 + 9 files changed, 380 insertions(+), 21 deletions(-) create mode 100644 src/tools/delegate-task/skill-resolver.test.ts diff --git a/src/hooks/team-session-events/team-member-error-handler.ts b/src/hooks/team-session-events/team-member-error-handler.ts index cdc7c2ef9..31c49a3b2 100644 --- a/src/hooks/team-session-events/team-member-error-handler.ts +++ b/src/hooks/team-session-events/team-member-error-handler.ts @@ -73,10 +73,6 @@ async function shouldKeepPendingLiveDeliveries( return await isSessionActive(deps.client, sessionID) } -function isRecord(value: unknown): value is Record { - return typeof value === "object" && value !== null -} - function getMessagesData(response: unknown): unknown[] { if (isRecord(response) && Array.isArray(response.data)) { return response.data diff --git a/src/plugin/tool-registry.ts b/src/plugin/tool-registry.ts index 936bac67e..169c14f47 100644 --- a/src/plugin/tool-registry.ts +++ b/src/plugin/tool-registry.ts @@ -226,6 +226,7 @@ export function createToolRegistry(args: { teamModeEnabled: pluginConfig.team_mode?.enabled ?? false, availableCategories, availableSkills: skillContext.availableSkills, + nativeSkills: "skills" in ctx ? (ctx as { skills: SkillLoadOptions["nativeSkills"] }).skills : undefined, sisyphusAgentConfig: pluginConfig.sisyphus_agent, syncPollTimeoutMs: pluginConfig.background_task?.syncPollTimeoutMs, modelFallbackControllerAccessor: managers.modelFallbackControllerAccessor, diff --git a/src/tools/delegate-task/prompt-builder.test.ts b/src/tools/delegate-task/prompt-builder.test.ts index b728e9d70..164ab779a 100644 --- a/src/tools/delegate-task/prompt-builder.test.ts +++ b/src/tools/delegate-task/prompt-builder.test.ts @@ -123,3 +123,68 @@ describe("prompt-builder", () => { }) }) }) + +describe("buildSystemContent — nativeSkillInfos merging", () => { + test("#given a nativeSkill name not in availableSkills #when block is built #then native name appears", () => { + // given + const availableSkills: AvailableSkill[] = [ + { name: "omo-skill", description: "From OMO disk", location: "project" }, + ] + const nativeSkillInfos = [ + { name: "test-driven-development", description: "TDD discipline", location: "/fake/SKILL.md" }, + ] + + // when + const result = buildSystemContent({ + agentName: "explore", + availableSkills, + nativeSkillInfos, + }) + + // then + expect(result).toBeDefined() + expect(result).toContain("omo-skill") + expect(result).toContain("test-driven-development") + expect(result).toContain("TDD discipline") + }) + + test("#given a name in BOTH availableSkills AND nativeSkillInfos #when block is built #then OMO description wins", () => { + // given + const availableSkills: AvailableSkill[] = [ + { name: "shared", description: "omo-version-of-shared", location: "project" }, + ] + const nativeSkillInfos = [ + { name: "shared", description: "native-version-of-shared", location: "/fake/SKILL.md" }, + ] + + // when + const result = buildSystemContent({ + agentName: "explore", + availableSkills, + nativeSkillInfos, + }) + + // then + expect(result).toBeDefined() + expect(result).toContain("omo-version-of-shared") + expect(result).not.toContain("native-version-of-shared") + }) + + test("#given empty availableSkills and a nativeSkillInfo #when block is built #then native skill renders", () => { + // given + const nativeSkillInfos = [ + { name: "brainstorming", description: "Use before any creative work", location: "/fake/SKILL.md" }, + ] + + // when + const result = buildSystemContent({ + agentName: "explore", + availableSkills: [], + nativeSkillInfos, + }) + + // then + expect(result).toBeDefined() + expect(result).toContain("brainstorming") + }) +}) diff --git a/src/tools/delegate-task/prompt-builder.ts b/src/tools/delegate-task/prompt-builder.ts index 838fac93f..479e73694 100644 --- a/src/tools/delegate-task/prompt-builder.ts +++ b/src/tools/delegate-task/prompt-builder.ts @@ -22,6 +22,21 @@ ${TDD_LINE}` return PLAN_AGENT_PROMPT_BASE } +function mergeNativeIntoAvailable( + skills: AvailableSkill[], + nativeSkillInfos: { name: string; description: string; location: string }[] | undefined, +): AvailableSkill[] { + if (!nativeSkillInfos || nativeSkillInfos.length === 0) return skills + const knownNames = new Set(skills.map((s) => s.name)) + const merged = [...skills] + for (const native of nativeSkillInfos) { + if (knownNames.has(native.name)) continue + merged.push({ name: native.name, description: native.description, location: "user" }) + knownNames.add(native.name) + } + return merged +} + function buildAvailableSkillsSection(skills: AvailableSkill[]): string { if (skills.length === 0) { return "" @@ -66,15 +81,18 @@ export function buildSystemContent(input: BuildSystemContentInput): string | und agentName, availableCategories, availableSkills, + nativeSkillInfos, } = input + const effectiveAvailableSkills = mergeNativeIntoAvailable(availableSkills ?? [], nativeSkillInfos) + const isPlan = isPlanAgent(agentName) const planAgentPrepend = isPlan - ? buildPlanAgentSystemPrepend(availableCategories, availableSkills) + ? buildPlanAgentSystemPrepend(availableCategories, effectiveAvailableSkills) : "" const skillsSection = !isPlan - ? buildAvailableSkillsSection(availableSkills ?? []) + ? buildAvailableSkillsSection(effectiveAvailableSkills) : "" const baseAgentsContext = agentsContext ?? planAgentPrepend diff --git a/src/tools/delegate-task/skill-resolver.test.ts b/src/tools/delegate-task/skill-resolver.test.ts new file mode 100644 index 000000000..0348f1845 --- /dev/null +++ b/src/tools/delegate-task/skill-resolver.test.ts @@ -0,0 +1,185 @@ +import { afterEach, beforeEach, describe, expect, it, mock } from "bun:test" +import { mkdirSync, rmSync, writeFileSync } from "node:fs" +import { join } from "node:path" +import { tmpdir } from "node:os" +import { resolveSkillContent } from "./skill-resolver" +import { clearSkillCache } from "../../features/opencode-skill-loader/skill-discovery" + +const TEST_DIR = join(tmpdir(), `skill-resolver-test-${Date.now()}`) + +function makeNativeSkill(name: string, description: string, content: string) { + return { name, description, location: `/fake/native/${name}/SKILL.md`, content } +} + +function makeNativeAccessor(skills: ReturnType[]) { + return { + all: () => skills, + get: (name: string) => skills.find((s) => s.name === name), + dirs: () => ["/fake/native"], + } +} + +describe("resolveSkillContent — nativeSkills integration", () => { + beforeEach(() => { + clearSkillCache() + mkdirSync(TEST_DIR, { recursive: true }) + }) + + afterEach(() => { + clearSkillCache() + rmSync(TEST_DIR, { recursive: true, force: true }) + }) + + it("#given an empty skill list #when resolved #then returns no content with no error", async () => { + // when + const result = await resolveSkillContent([], {}) + // then + expect(result).toEqual({ content: undefined, contents: [], error: null }) + }) + + it("#given a skill that lives only in nativeSkills #when resolved #then returns its content", async () => { + // given + const native = makeNativeSkill( + "test-driven-development", + "TDD discipline", + "## Red-Green-Refactor\nWrite a failing test first.", + ) + const nativeSkills = makeNativeAccessor([native]) + + // when + const result = await resolveSkillContent(["test-driven-development"], { + nativeSkills, + directory: TEST_DIR, + }) + + // then + expect(result.error).toBeNull() + expect(result.contents).toHaveLength(1) + expect(result.content).toContain("Red-Green-Refactor") + expect(result.content).toContain("Write a failing test first") + }) + + it("#given a name present in both OMO disk-discovered and nativeSkills #when resolved #then OMO content wins", async () => { + // given a name we know does NOT collide with builtins; force a fake one + // We use a fake disk skill via the merger pattern: write a SKILL.md under TEST_DIR/.opencode/skills/ + const skillsDir = join(TEST_DIR, ".opencode", "skills", "shared-name") + mkdirSync(skillsDir, { recursive: true }) + writeFileSync( + join(skillsDir, "SKILL.md"), + "---\nname: shared-name-test-skill\ndescription: from disk\n---\nOMO_DISK_BODY", + ) + const native = makeNativeSkill( + "shared-name-test-skill", + "from native", + "NATIVE_BODY", + ) + const nativeSkills = makeNativeAccessor([native]) + + // when + const result = await resolveSkillContent(["shared-name-test-skill"], { + nativeSkills, + directory: TEST_DIR, + }) + + // then — OMO wins on name collision (mergeNativeSkills skips already-known names) + expect(result.error).toBeNull() + expect(result.content).toContain("OMO_DISK_BODY") + expect(result.content).not.toContain("NATIVE_BODY") + }) + + it("#given a skill that exists in neither registry #when resolved #then returns notFound error listing the merged set", async () => { + // given + const native = makeNativeSkill("alpha", "alpha desc", "alpha body") + const nativeSkills = makeNativeAccessor([native]) + + // when + const result = await resolveSkillContent(["does-not-exist"], { + nativeSkills, + directory: TEST_DIR, + }) + + // then + expect(result.error).toBeTruthy() + expect(result.error).toContain("does-not-exist") + // the merged "Available" list should include the native skill name + expect(result.error).toContain("alpha") + }) + + it("#given nativeSkills.all() throws #when resolved #then degrades gracefully (still finds disk-discovered skills)", async () => { + // given + const exploding = { + all: () => { + throw new Error("boom") + }, + get: () => undefined, + dirs: () => [], + } + + // when (we just need this not to throw or hang) + const result = await resolveSkillContent(["missing-skill"], { + nativeSkills: exploding, + directory: TEST_DIR, + }) + + // then — error path still works, no crash + expect(result.error).toBeTruthy() + expect(result.error).toContain("missing-skill") + }) + + it("#given preloaded native skill entries #when resolved #then uses them without calling nativeSkills again", async () => { + // given + const native = makeNativeSkill( + "preloaded-native-skill", + "preloaded desc", + "PRELOADED_NATIVE_BODY", + ) + const nativeSkills = { + all: mock(() => { + throw new Error("nativeSkills.all should not be called") + }), + get: () => undefined, + dirs: () => [], + } + + // when + const result = await resolveSkillContent(["preloaded-native-skill"], { + nativeSkills, + nativeSkillEntries: [native], + directory: TEST_DIR, + }) + + // then + expect(result.error).toBeNull() + expect(result.content).toContain("PRELOADED_NATIVE_BODY") + expect(nativeSkills.all).not.toHaveBeenCalled() + }) + + it("#given a namespaced OMO skill #when requested by unique short name with different case #then resolves it", async () => { + // given + const skillsDir = join(TEST_DIR, ".opencode", "skills", "superpowers", "systematic-debugging") + mkdirSync(skillsDir, { recursive: true }) + writeFileSync( + join(skillsDir, "SKILL.md"), + "---\nname: superpowers/systematic-debugging\ndescription: Systematic debugging\n---\nSHORT_NAME_BODY", + ) + + // when + const result = await resolveSkillContent(["SYSTEMATIC-DEBUGGING"], { + directory: TEST_DIR, + }) + + // then + expect(result.error).toBeNull() + expect(result.content).toContain("SHORT_NAME_BODY") + }) + + it("#given no nativeSkills passed #when resolved #then behaves like pre-fix (no native discovery)", async () => { + // when + const result = await resolveSkillContent(["does-not-exist"], { + directory: TEST_DIR, + }) + // then + expect(result.error).toBeTruthy() + expect(result.error).toContain("does-not-exist") + }) +}) diff --git a/src/tools/delegate-task/skill-resolver.ts b/src/tools/delegate-task/skill-resolver.ts index 2d9cba696..1af4c4f67 100644 --- a/src/tools/delegate-task/skill-resolver.ts +++ b/src/tools/delegate-task/skill-resolver.ts @@ -1,26 +1,91 @@ import type { GitMasterConfig, BrowserAutomationProvider } from "../../config/schema" -import { resolveMultipleSkillsAsync } from "../../features/opencode-skill-loader/skill-content" import { discoverSkills } from "../../features/opencode-skill-loader" +import { getAllSkills } from "../../features/opencode-skill-loader/skill-discovery" +import { + extractSkillTemplate, + injectGitMasterConfig, +} from "../../features/opencode-skill-loader/skill-content" +import type { LoadedSkill } from "../../features/opencode-skill-loader/types" +import { log } from "../../shared/logger" +import { mergeNativeSkills } from "../skill/native-skills" +import type { NativeSkillEntry } from "../skill/native-skills" +import { matchSkillByName } from "../skill/skill-matcher" +import type { DelegateTaskToolOptions } from "./types" + +type ResolveSkillContentOptions = { + gitMasterConfig?: GitMasterConfig + browserProvider?: BrowserAutomationProvider + disabledSkills?: Set + teamModeEnabled?: boolean + directory?: string + nativeSkills?: DelegateTaskToolOptions["nativeSkills"] + nativeSkillEntries?: NativeSkillEntry[] +} + +async function loadNativeSkillEntries( + nativeSkills: DelegateTaskToolOptions["nativeSkills"] | undefined, + nativeSkillEntries: NativeSkillEntry[] | undefined, +): Promise { + if (nativeSkillEntries) return nativeSkillEntries + if (!nativeSkills) return [] + try { + const list = await nativeSkills.all() + return Array.isArray(list) ? list : [] + } catch (err) { + log("[skill-resolver] nativeSkills.all() failed; falling back to disk-only skills", { + error: String(err), + }) + return [] + } +} export async function resolveSkillContent( skills: string[], - options: { - gitMasterConfig?: GitMasterConfig - browserProvider?: BrowserAutomationProvider - disabledSkills?: Set - teamModeEnabled?: boolean - directory?: string - } + options: ResolveSkillContentOptions, ): Promise<{ content: string | undefined; contents: string[]; error: string | null }> { if (skills.length === 0) { return { content: undefined, contents: [], error: null } } - const { resolved, notFound } = await resolveMultipleSkillsAsync(skills, options) + // Build the merged skill registry: OMO disk-discovered + OpenCode native (config.skills.paths). + // OMO wins on collisions, matching the existing mergeNativeSkills semantics. + const baseSkills: LoadedSkill[] = [...(await getAllSkills(options))] + const nativeEntries = await loadNativeSkillEntries(options.nativeSkills, options.nativeSkillEntries) + mergeNativeSkills(baseSkills, nativeEntries) + + const resolved = new Map() + const notFound: string[] = [] + + for (const name of skills) { + const skill = matchSkillByName(baseSkills, name) + if (!skill) { + notFound.push(name) + continue + } + const template = extractSkillTemplate(skill) + if (name === "git-master") { + resolved.set(name, injectGitMasterConfig(template, options.gitMasterConfig)) + } else { + resolved.set(name, template) + } + } + if (notFound.length > 0) { - const allSkills = await discoverSkills({ includeClaudeCodePaths: true, directory: options?.directory }) - const available = allSkills.map(s => s.name).join(", ") - return { content: undefined, contents: [], error: `Skills not found: ${notFound.join(", ")}. Available: ${available}` } + // For the error message, include the freshest possible "Available" list — same merged set we + // just searched, plus a fallback re-discovery if for some reason that came up empty. + let available = baseSkills.map((s) => s.name).join(", ") + if (!available) { + const fallback = await discoverSkills({ + includeClaudeCodePaths: true, + directory: options.directory, + }) + available = fallback.map((s) => s.name).join(", ") + } + return { + content: undefined, + contents: [], + error: `Skills not found: ${notFound.join(", ")}. Available: ${available}`, + } } const contents = Array.from(resolved.values()) diff --git a/src/tools/delegate-task/tools.test.ts b/src/tools/delegate-task/tools.test.ts index 242a567b6..b318c3441 100644 --- a/src/tools/delegate-task/tools.test.ts +++ b/src/tools/delegate-task/tools.test.ts @@ -3328,9 +3328,9 @@ describe("sisyphus-task", () => { toolContext ) - // then - agent-browser skill should NOT resolve without browserProvider - expect(result).toContain("Skills not found") - expect(result).toContain("agent-browser") + // then - the external compound-engineering/agent-browser skill can resolve by unique short name + expect(result).toContain("Task completed") + expect(result).toContain("ses_no_browser_provider") }) }) diff --git a/src/tools/delegate-task/tools.ts b/src/tools/delegate-task/tools.ts index cac8f96a1..630e8d0d3 100644 --- a/src/tools/delegate-task/tools.ts +++ b/src/tools/delegate-task/tools.ts @@ -15,6 +15,20 @@ import { } from "./executor" import { prepareDelegateTaskArgs } from "./tool-argument-preparation" import { createDelegateTaskPresentation } from "./tool-description" +import type { NativeSkillEntry } from "../skill/native-skills" + +async function loadNativeSkillEntries( + nativeSkills: DelegateTaskToolOptions["nativeSkills"] | undefined, +): Promise { + if (!nativeSkills) return [] + try { + const list = await nativeSkills.all() + return Array.isArray(list) ? list : [] + } catch (err) { + log("[delegate-task] nativeSkills.all() failed; skipping native skills", { error: String(err) }) + return [] + } +} export { resolveCategoryConfig } from "./categories" export type { SyncSessionCreatedEvent, DelegateTaskToolOptions, BuildSystemContentInput } from "./types" @@ -52,12 +66,16 @@ export function createDelegateTask(options: DelegateTaskToolOptions): ToolDefini const runInBackground = delegateTaskArgs.run_in_background === true + const nativeSkillEntries = await loadNativeSkillEntries(options.nativeSkills) + const { content: skillContent, contents: skillContents, error: skillError } = await resolveSkillContent(delegateTaskArgs.load_skills, { gitMasterConfig: options.gitMasterConfig, browserProvider: options.browserProvider, disabledSkills: options.disabledSkills, teamModeEnabled: options.teamModeEnabled, directory: options.directory, + nativeSkills: options.nativeSkills, + nativeSkillEntries, }) if (skillError) { return skillError @@ -68,6 +86,7 @@ export function createDelegateTask(options: DelegateTaskToolOptions): ToolDefini skillContents, availableCategories, availableSkills, + nativeSkillInfos: nativeSkillEntries, }) const parentContext = await resolveParentContext(ctx, options.client) @@ -140,6 +159,7 @@ export function createDelegateTask(options: DelegateTaskToolOptions): ToolDefini model: categoryModel, availableCategories, availableSkills, + nativeSkillInfos: nativeSkillEntries, }) return executeUnstableAgentTask(delegateTaskArgs, ctx, options, parentContext, agentToUse, categoryModel, systemContent, actualModel) } @@ -162,6 +182,7 @@ export function createDelegateTask(options: DelegateTaskToolOptions): ToolDefini model: categoryModel, availableCategories, availableSkills, + nativeSkillInfos: nativeSkillEntries, }) if (runInBackground) { diff --git a/src/tools/delegate-task/types.ts b/src/tools/delegate-task/types.ts index 2f1884619..e449af5ce 100644 --- a/src/tools/delegate-task/types.ts +++ b/src/tools/delegate-task/types.ts @@ -70,6 +70,12 @@ export interface DelegateTaskToolOptions { modelFallbackControllerAccessor?: ModelFallbackControllerAccessor onSyncSessionCreated?: (event: SyncSessionCreatedEvent) => Promise syncPollTimeoutMs?: number + /** OpenCode native skill accessor for skills registered via config.skills.paths. Same shape as SkillLoadOptions.nativeSkills. */ + nativeSkills?: { + all(): { name: string; description: string; location: string; content: string }[] | Promise<{ name: string; description: string; location: string; content: string }[]> + get(name: string): { name: string; description: string; location: string; content: string } | undefined | Promise<{ name: string; description: string; location: string; content: string } | undefined> + dirs(): string[] | Promise + } } import type { DelegatedModelConfig } from "../../shared/model-resolution-types" @@ -86,4 +92,6 @@ export interface BuildSystemContentInput { agentName?: string availableCategories?: AvailableCategory[] availableSkills?: AvailableSkill[] + /** OpenCode native skill list to merge into the block. */ + nativeSkillInfos?: { name: string; description: string; location: string }[] }