Merge pull request #1370 from misyuari/fix/refactor-skills

fix: update skill resolution to support disabled skills functionality

src/agents/utils.test.ts

@@ -41,7 +41,7 @@ describe("createBuiltinAgents with model overrides", () => {
     }
 
     // #when
-    const agents = await createBuiltinAgents([], overrides, undefined, TEST_DEFAULT_MODEL)
+    const agents = await createBuiltinAgents([], overrides, undefined, TEST_DEFAULT_MODEL, undefined, undefined, [], undefined, undefined)
 
     // #then
     expect(agents.sisyphus.model).toBe("github-copilot/gpt-5.2")
@@ -103,7 +103,7 @@ describe("createBuiltinAgents with model overrides", () => {
      const cacheSpy = spyOn(connectedProvidersCache, "readConnectedProvidersCache").mockReturnValue(["openai"])
 
      // #when
-     const agents = await createBuiltinAgents([], {}, undefined, TEST_DEFAULT_MODEL)
+     const agents = await createBuiltinAgents([], {}, undefined, TEST_DEFAULT_MODEL, undefined, undefined, [], undefined, undefined)
 
      // #then - oracle resolves via connected cache fallback to openai/gpt-5.2 (not system default)
      expect(agents.oracle.model).toBe("openai/gpt-5.2")
@@ -132,7 +132,7 @@ describe("createBuiltinAgents with model overrides", () => {
     }
 
     // #when
-    const agents = await createBuiltinAgents([], overrides, undefined, TEST_DEFAULT_MODEL)
+    const agents = await createBuiltinAgents([], overrides, undefined, TEST_DEFAULT_MODEL, undefined, undefined, [], undefined, undefined)
 
     // #then
     expect(agents.oracle.model).toBe("openai/gpt-5.2")
@@ -148,7 +148,7 @@ describe("createBuiltinAgents with model overrides", () => {
     }
 
     // #when
-    const agents = await createBuiltinAgents([], overrides, undefined, TEST_DEFAULT_MODEL)
+    const agents = await createBuiltinAgents([], overrides, undefined, TEST_DEFAULT_MODEL, undefined, undefined, [], undefined, undefined)
 
     // #then
     expect(agents.oracle.model).toBe("anthropic/claude-sonnet-4")
@@ -164,12 +164,25 @@ describe("createBuiltinAgents with model overrides", () => {
      }
 
      // #when
-     const agents = await createBuiltinAgents([], overrides, undefined, TEST_DEFAULT_MODEL)
+     const agents = await createBuiltinAgents([], overrides, undefined, TEST_DEFAULT_MODEL, undefined, undefined, [], undefined, undefined)
 
      // #then
      expect(agents.sisyphus.model).toBe("github-copilot/gpt-5.2")
      expect(agents.sisyphus.temperature).toBe(0.5)
    })
+
+  test("createBuiltinAgents excludes disabled skills from availableSkills", async () => {
+    // #given
+    const disabledSkills = new Set(["playwright"])
+
+    // #when
+    const agents = await createBuiltinAgents([], {}, undefined, TEST_DEFAULT_MODEL, undefined, undefined, [], undefined, undefined, undefined, disabledSkills)
+
+    // #then
+    expect(agents.sisyphus.prompt).not.toContain("playwright")
+    expect(agents.sisyphus.prompt).toContain("frontend-ui-ux")
+    expect(agents.sisyphus.prompt).toContain("git-master")
+  })
 })
 
 describe("createBuiltinAgents without systemDefaultModel", () => {

src/agents/utils.ts

@@ -57,7 +57,8 @@ export function buildAgent(
   model: string,
   categories?: CategoriesConfig,
   gitMasterConfig?: GitMasterConfig,
-  browserProvider?: BrowserAutomationProvider
+  browserProvider?: BrowserAutomationProvider,
+  disabledSkills?: Set<string>
 ): AgentConfig {
   const base = isFactory(source) ? source(model) : source
   const categoryConfigs: Record<string, CategoryConfig> = categories
@@ -81,7 +82,7 @@ export function buildAgent(
   }
 
   if (agentWithCategory.skills?.length) {
-    const { resolved } = resolveMultipleSkills(agentWithCategory.skills, { gitMasterConfig, browserProvider })
+    const { resolved } = resolveMultipleSkills(agentWithCategory.skills, { gitMasterConfig, browserProvider, disabledSkills })
     if (resolved.size > 0) {
       const skillContent = Array.from(resolved.values()).join("\n\n")
       base.prompt = skillContent + (base.prompt ? "\n\n" + base.prompt : "")
@@ -234,7 +235,8 @@ export async function createBuiltinAgents(
   discoveredSkills: LoadedSkill[] = [],
   client?: any,
   browserProvider?: BrowserAutomationProvider,
-  uiSelectedModel?: string
+  uiSelectedModel?: string,
+  disabledSkills?: Set<string>
 ): Promise<Record<string, AgentConfig>> {
   const connectedProviders = readConnectedProvidersCache()
   // IMPORTANT: Do NOT pass client to fetchAvailableModels during plugin initialization.
@@ -258,7 +260,7 @@ export async function createBuiltinAgents(
     description: categories?.[name]?.description ?? CATEGORY_DESCRIPTIONS[name] ?? "General tasks",
   }))
 
-  const builtinSkills = createBuiltinSkills({ browserProvider })
+  const builtinSkills = createBuiltinSkills({ browserProvider, disabledSkills })
   const builtinSkillNames = new Set(builtinSkills.map(s => s.name))
 
   const builtinAvailable: AvailableSkill[] = builtinSkills.map((skill) => ({
@@ -311,7 +313,7 @@ export async function createBuiltinAgents(
     if (!resolution) continue
     const { model, variant: resolvedVariant } = resolution
 
-    let config = buildAgent(source, model, mergedCategories, gitMasterConfig, browserProvider)
+    let config = buildAgent(source, model, mergedCategories, gitMasterConfig, browserProvider, disabledSkills)
     
     // Apply resolved variant from model fallback chain
     if (resolvedVariant) {

src/features/builtin-skills/skills.test.ts

@@ -86,4 +86,58 @@ describe("createBuiltinSkills", () => {
 		expect(defaultSkills).toHaveLength(4)
 		expect(agentBrowserSkills).toHaveLength(4)
 	})
+
+	test("should exclude playwright when it is in disabledSkills", () => {
+		// #given
+		const options = { disabledSkills: new Set(["playwright"]) }
+
+		// #when
+		const skills = createBuiltinSkills(options)
+
+		// #then
+		expect(skills.map((s) => s.name)).not.toContain("playwright")
+		expect(skills.map((s) => s.name)).toContain("frontend-ui-ux")
+		expect(skills.map((s) => s.name)).toContain("git-master")
+		expect(skills.map((s) => s.name)).toContain("dev-browser")
+		expect(skills.length).toBe(3)
+	})
+
+	test("should exclude multiple skills when they are in disabledSkills", () => {
+		// #given
+		const options = { disabledSkills: new Set(["playwright", "git-master"]) }
+
+		// #when
+		const skills = createBuiltinSkills(options)
+
+		// #then
+		expect(skills.map((s) => s.name)).not.toContain("playwright")
+		expect(skills.map((s) => s.name)).not.toContain("git-master")
+		expect(skills.map((s) => s.name)).toContain("frontend-ui-ux")
+		expect(skills.map((s) => s.name)).toContain("dev-browser")
+		expect(skills.length).toBe(2)
+	})
+
+	test("should return an empty array when all skills are disabled", () => {
+		// #given
+		const options = {
+			disabledSkills: new Set(["playwright", "frontend-ui-ux", "git-master", "dev-browser"]),
+		}
+
+		// #when
+		const skills = createBuiltinSkills(options)
+
+		// #then
+		expect(skills.length).toBe(0)
+	})
+
+	test("should return all skills when disabledSkills set is empty", () => {
+		// #given
+		const options = { disabledSkills: new Set<string>() }
+
+		// #when
+		const skills = createBuiltinSkills(options)
+
+		// #then
+		expect(skills.length).toBe(4)
+	})
 })

src/features/builtin-skills/skills.ts

@@ -11,12 +11,19 @@ import {
 
 export interface CreateBuiltinSkillsOptions {
   browserProvider?: BrowserAutomationProvider
+  disabledSkills?: Set<string>
 }
 
 export function createBuiltinSkills(options: CreateBuiltinSkillsOptions = {}): BuiltinSkill[] {
-  const { browserProvider = "playwright" } = options
+  const { browserProvider = "playwright", disabledSkills } = options
 
   const browserSkill = browserProvider === "agent-browser" ? agentBrowserSkill : playwrightSkill
 
-  return [browserSkill, frontendUiUxSkill, gitMasterSkill, devBrowserSkill]
+  const skills = [browserSkill, frontendUiUxSkill, gitMasterSkill, devBrowserSkill]
+
+  if (!disabledSkills) {
+    return skills
+  }
+
+  return skills.filter((skill) => !disabledSkills.has(skill.name))
 }

src/features/opencode-skill-loader/skill-content.test.ts

@@ -33,10 +33,12 @@ describe("resolveSkillContent", () => {
 		expect(result).toBeNull()
 	})
 
-	it("should return null for empty string", () => {
+	it("should return null for disabled skill", () => {
-		// given: builtin skills
+		// given: frontend-ui-ux skill disabled
-		// when: resolving content for empty string
+		const options = { disabledSkills: new Set(["frontend-ui-ux"]) }
-		const result = resolveSkillContent("")
+
+		// when: resolving content for disabled skill
+		const result = resolveSkillContent("frontend-ui-ux", options)
 
 		// then: returns null
 		expect(result).toBeNull()
@@ -96,6 +98,20 @@ describe("resolveMultipleSkills", () => {
 		expect(result.notFound).toEqual(["skill-one", "skill-two", "skill-three"])
 	})
 
+	it("should treat disabled skills as not found", () => {
+		// #given: frontend-ui-ux disabled, playwright not disabled
+		const skillNames = ["frontend-ui-ux", "playwright"]
+		const options = { disabledSkills: new Set(["frontend-ui-ux"]) }
+
+		// #when: resolving multiple skills with disabled one
+		const result = resolveMultipleSkills(skillNames, options)
+
+		// #then: frontend-ui-ux in notFound, playwright resolved
+		expect(result.resolved.size).toBe(1)
+		expect(result.resolved.has("playwright")).toBe(true)
+		expect(result.notFound).toEqual(["frontend-ui-ux"])
+	})
+
 	it("should preserve skill order in resolved map", () => {
 		// given: list of skill names in specific order
 		const skillNames = ["playwright", "frontend-ui-ux"]
@@ -111,21 +127,24 @@ describe("resolveMultipleSkills", () => {
 })
 
 describe("resolveSkillContentAsync", () => {
-	it("should return template for builtin skill", async () => {
+	it("should return template for builtin skill async", async () => {
 		// given: builtin skill 'frontend-ui-ux'
 		// when: resolving content async
-		const result = await resolveSkillContentAsync("frontend-ui-ux")
+		const options = { disabledSkills: new Set(["frontend-ui-ux"]) }
+		const result = await resolveSkillContentAsync("git-master", options)
 
 		// then: returns template string
 		expect(result).not.toBeNull()
 		expect(typeof result).toBe("string")
-		expect(result).toContain("Role: Designer-Turned-Developer")
+		expect(result).toContain("Git Master Agent")
 	})
 
-	it("should return null for non-existent skill", async () => {
+	it("should return null for disabled skill async", async () => {
-		// given: non-existent skill name
+		// given: frontend-ui-ux disabled
-		// when: resolving content async
+		const options = { disabledSkills: new Set(["frontend-ui-ux"]) }
-		const result = await resolveSkillContentAsync("definitely-not-a-skill-12345")
+
+		// when: resolving content async for disabled skill
+		const result = await resolveSkillContentAsync("frontend-ui-ux", options)
 
 		// then: returns null
 		expect(result).toBeNull()
@@ -133,9 +152,9 @@ describe("resolveSkillContentAsync", () => {
 })
 
 describe("resolveMultipleSkillsAsync", () => {
-	it("should resolve builtin skills", async () => {
+	it("should resolve builtin skills async", async () => {
 		// given: builtin skill names
-		const skillNames = ["playwright", "frontend-ui-ux"]
+		const skillNames = ["playwright", "git-master"]
 
 		// when: resolving multiple skills async
 		const result = await resolveMultipleSkillsAsync(skillNames)
@@ -144,10 +163,10 @@ describe("resolveMultipleSkillsAsync", () => {
 		expect(result.resolved.size).toBe(2)
 		expect(result.notFound).toEqual([])
 		expect(result.resolved.get("playwright")).toContain("Playwright Browser Automation")
-		expect(result.resolved.get("frontend-ui-ux")).toContain("Designer-Turned-Developer")
+		expect(result.resolved.get("git-master")).toContain("Git Master Agent")
 	})
 
-	it("should handle partial success with non-existent skills", async () => {
+	it("should handle partial success with non-existent skills async", async () => {
 		// given: mix of existing and non-existing skills
 		const skillNames = ["playwright", "nonexistent-skill-12345"]
 
@@ -160,6 +179,20 @@ describe("resolveMultipleSkillsAsync", () => {
 		expect(result.resolved.get("playwright")).toContain("Playwright Browser Automation")
 	})
 
+	it("should treat disabled skills as not found async", async () => {
+		// #given: frontend-ui-ux disabled
+		const skillNames = ["frontend-ui-ux", "playwright"]
+		const options = { disabledSkills: new Set(["frontend-ui-ux"]) }
+
+		// #when: resolving multiple skills async with disabled one
+		const result = await resolveMultipleSkillsAsync(skillNames, options)
+
+		// #then: frontend-ui-ux in notFound, playwright resolved
+		expect(result.resolved.size).toBe(1)
+		expect(result.resolved.has("playwright")).toBe(true)
+		expect(result.notFound).toEqual(["frontend-ui-ux"])
+	})
+
 	it("should NOT inject watermark when both options are disabled", async () => {
 		// given: git-master skill with watermark disabled
 		const skillNames = ["git-master"]

src/features/opencode-skill-loader/skill-content.ts

@@ -8,6 +8,7 @@ import type { GitMasterConfig, BrowserAutomationProvider } from "../../config/sc
 export interface SkillResolutionOptions {
 	gitMasterConfig?: GitMasterConfig
 	browserProvider?: BrowserAutomationProvider
+	disabledSkills?: Set<string>
 }
 
 const cachedSkillsByProvider = new Map<string, LoadedSkill[]>()
@@ -23,7 +24,12 @@ async function getAllSkills(options?: SkillResolutionOptions): Promise<LoadedSki
 
 	const [discoveredSkills, builtinSkillDefs] = await Promise.all([
 		discoverSkills({ includeClaudeCodePaths: true }),
-		Promise.resolve(createBuiltinSkills({ browserProvider: options?.browserProvider })),
+		Promise.resolve(
+			createBuiltinSkills({
+				browserProvider: options?.browserProvider,
+				disabledSkills: options?.disabledSkills,
+			})
+		),
 	])
 
 	const builtinSkillsAsLoaded: LoadedSkill[] = builtinSkillDefs.map((skill) => ({
@@ -122,7 +128,10 @@ export function injectGitMasterConfig(template: string, config?: GitMasterConfig
 }
 
 export function resolveSkillContent(skillName: string, options?: SkillResolutionOptions): string | null {
-	const skills = createBuiltinSkills({ browserProvider: options?.browserProvider })
+	const skills = createBuiltinSkills({
+		browserProvider: options?.browserProvider,
+		disabledSkills: options?.disabledSkills,
+	})
 	const skill = skills.find((s) => s.name === skillName)
 	if (!skill) return null
 
@@ -137,7 +146,10 @@ export function resolveMultipleSkills(skillNames: string[], options?: SkillResol
 	resolved: Map<string, string>
 	notFound: string[]
 } {
-	const skills = createBuiltinSkills({ browserProvider: options?.browserProvider })
+	const skills = createBuiltinSkills({
+		browserProvider: options?.browserProvider,
+		disabledSkills: options?.disabledSkills,
+	})
 	const skillMap = new Map(skills.map((s) => [s.name, s.template]))
 
 	const resolved = new Map<string, string>()

src/index.ts

@@ -388,6 +388,7 @@ const OhMyOpenCodePlugin: Plugin = async (ctx) => {
   const lookAt = isMultimodalLookerEnabled ? createLookAt(ctx) : null;
   const browserProvider =
     pluginConfig.browser_automation_engine?.provider ?? "playwright";
+  const disabledSkills = new Set<string>(pluginConfig.disabled_skills ?? []);
   const delegateTask = createDelegateTask({
     manager: backgroundManager,
     client: ctx.client,
@@ -396,6 +397,7 @@ const OhMyOpenCodePlugin: Plugin = async (ctx) => {
     gitMasterConfig: pluginConfig.git_master,
     sisyphusJuniorModel: pluginConfig.agents?.["sisyphus-junior"]?.model,
     browserProvider,
+    disabledSkills,
     onSyncSessionCreated: async (event) => {
       log("[index] onSyncSessionCreated callback", {
         sessionID: event.sessionID,
@@ -414,11 +416,8 @@ const OhMyOpenCodePlugin: Plugin = async (ctx) => {
       });
     },
   });
-  const disabledSkills = new Set(pluginConfig.disabled_skills ?? []);
   const systemMcpNames = getSystemMcpServerNames();
-  const builtinSkills = createBuiltinSkills({ browserProvider }).filter(
+  const builtinSkills = createBuiltinSkills({ browserProvider, disabledSkills }).filter((skill) => {
-    (skill) => {
-      if (disabledSkills.has(skill.name as never)) return false;
       if (skill.mcpConfig) {
         for (const mcpName of Object.keys(skill.mcpConfig)) {
           if (systemMcpNames.has(mcpName)) return false;
@@ -450,6 +449,7 @@ const OhMyOpenCodePlugin: Plugin = async (ctx) => {
     mcpManager: skillMcpManager,
     getSessionID: getSessionIDForMcp,
     gitMasterConfig: pluginConfig.git_master,
+    disabledSkills
   });
   const skillMcpTool = createSkillMcpTool({
     manager: skillMcpManager,

src/plugin-handlers/config-handler.ts

@@ -157,6 +157,7 @@ export function createConfigHandler(deps: ConfigHandlerDeps) {
     // config.model represents the currently active model in OpenCode (including UI selection)
     // Pass it as uiSelectedModel so it takes highest priority in model resolution
     const currentModel = config.model as string | undefined;
+    const disabledSkills = new Set<string>(pluginConfig.disabled_skills ?? []);
     const builtinAgents = await createBuiltinAgents(
       migratedDisabledAgents,
       pluginConfig.agents,
@@ -167,7 +168,8 @@ export function createConfigHandler(deps: ConfigHandlerDeps) {
       allDiscoveredSkills,
       ctx.client,
       browserProvider,
-      currentModel // uiSelectedModel - takes highest priority
+      currentModel, // uiSelectedModel - takes highest priority
+      disabledSkills
     );
 
     // Claude Code agents: Do NOT apply permission migration
@@ -358,7 +360,8 @@ export function createConfigHandler(deps: ConfigHandlerDeps) {
         : {};
 
       const planDemoteConfig = shouldDemotePlan
-        ? { mode: "subagent" as const }
+           ? { mode: "subagent" as const
+          }
         : undefined;
 
       config.agent = {

src/tools/delegate-task/executor.ts

@@ -44,7 +44,7 @@ interface SessionMessage {
 
 export async function resolveSkillContent(
   skills: string[],
-  options: { gitMasterConfig?: GitMasterConfig; browserProvider?: BrowserAutomationProvider }
+  options: { gitMasterConfig?: GitMasterConfig; browserProvider?: BrowserAutomationProvider, disabledSkills?: Set<string> }
 ): Promise<{ content: string | undefined; error: string | null }> {
   if (skills.length === 0) {
     return { content: undefined, error: null }

src/tools/delegate-task/tools.ts

@@ -83,6 +83,7 @@ Prompts MUST be in English.`
       const { content: skillContent, error: skillError } = await resolveSkillContent(args.load_skills, {
         gitMasterConfig: options.gitMasterConfig,
         browserProvider: options.browserProvider,
+        disabledSkills: options.disabledSkills,
       })
       if (skillError) {
         return skillError

src/tools/delegate-task/types.ts

@@ -41,6 +41,7 @@ export interface DelegateTaskToolOptions {
   gitMasterConfig?: GitMasterConfig
   sisyphusJuniorModel?: string
   browserProvider?: BrowserAutomationProvider
+  disabledSkills?: Set<string>
   onSyncSessionCreated?: (event: SyncSessionCreatedEvent) => Promise<void>
 }
 

src/tools/skill/tools.ts

@@ -133,7 +133,7 @@ export function createSkillTool(options: SkillLoadOptions = {}): ToolDefinition
   const getSkills = async (): Promise<LoadedSkill[]> => {
     if (options.skills) return options.skills
     if (cachedSkills) return cachedSkills
-    cachedSkills = await getAllSkills()
+    cachedSkills = await getAllSkills({disabledSkills: options?.disabledSkills})
     return cachedSkills
   }
 

src/tools/skill/types.ts

@@ -28,4 +28,5 @@ export interface SkillLoadOptions {
   getSessionID?: () => string
   /** Git master configuration for watermark/co-author settings */
   gitMasterConfig?: GitMasterConfig
+  disabledSkills?: Set<string>
 }