Skip to content

Commit ef5e8c1

Browse files
committed
fix: implement disabled skills functionality in skill resolution
1 parent 5c7eb02 commit ef5e8c1

File tree

8 files changed

+145
-25
lines changed

8 files changed

+145
-25
lines changed

src/agents/utils.test.ts

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ describe("createBuiltinAgents with model overrides", () => {
1010
// #given - no overrides, using systemDefaultModel
1111

1212
// #when
13-
const agents = await createBuiltinAgents([], {}, undefined, TEST_DEFAULT_MODEL)
13+
const agents = await createBuiltinAgents([], {}, undefined, TEST_DEFAULT_MODEL, undefined, undefined, [], undefined, undefined)
1414

1515
// #then
1616
expect(agents.sisyphus.model).toBe("anthropic/claude-opus-4-5")
@@ -25,7 +25,7 @@ describe("createBuiltinAgents with model overrides", () => {
2525
}
2626

2727
// #when
28-
const agents = await createBuiltinAgents([], overrides, undefined, TEST_DEFAULT_MODEL)
28+
const agents = await createBuiltinAgents([], overrides, undefined, TEST_DEFAULT_MODEL, undefined, undefined, [], undefined, undefined)
2929

3030
// #then
3131
expect(agents.sisyphus.model).toBe("github-copilot/gpt-5.2")
@@ -38,7 +38,7 @@ describe("createBuiltinAgents with model overrides", () => {
3838
const systemDefaultModel = "anthropic/claude-opus-4-5"
3939

4040
// #when
41-
const agents = await createBuiltinAgents([], {}, undefined, systemDefaultModel)
41+
const agents = await createBuiltinAgents([], {}, undefined, systemDefaultModel, undefined, undefined, [], undefined, undefined)
4242

4343
// #then - falls back to system default when no availability match
4444
expect(agents.sisyphus.model).toBe("anthropic/claude-opus-4-5")
@@ -50,7 +50,7 @@ describe("createBuiltinAgents with model overrides", () => {
5050
// #given - no available models simulates CI without model cache
5151

5252
// #when
53-
const agents = await createBuiltinAgents([], {}, undefined, TEST_DEFAULT_MODEL)
53+
const agents = await createBuiltinAgents([], {}, undefined, TEST_DEFAULT_MODEL, undefined, undefined, [], undefined, undefined)
5454

5555
// #then - uses first fallback entry (openai/gpt-5.2) instead of system default
5656
expect(agents.oracle.model).toBe("openai/gpt-5.2")
@@ -66,7 +66,7 @@ describe("createBuiltinAgents with model overrides", () => {
6666
}
6767

6868
// #when
69-
const agents = await createBuiltinAgents([], overrides, undefined, TEST_DEFAULT_MODEL)
69+
const agents = await createBuiltinAgents([], overrides, undefined, TEST_DEFAULT_MODEL, undefined, undefined, [], undefined, undefined)
7070

7171
// #then
7272
expect(agents.oracle.model).toBe("openai/gpt-5.2")
@@ -82,7 +82,7 @@ describe("createBuiltinAgents with model overrides", () => {
8282
}
8383

8484
// #when
85-
const agents = await createBuiltinAgents([], overrides, undefined, TEST_DEFAULT_MODEL)
85+
const agents = await createBuiltinAgents([], overrides, undefined, TEST_DEFAULT_MODEL, undefined, undefined, [], undefined, undefined)
8686

8787
// #then
8888
expect(agents.oracle.model).toBe("anthropic/claude-sonnet-4")
@@ -98,12 +98,25 @@ describe("createBuiltinAgents with model overrides", () => {
9898
}
9999

100100
// #when
101-
const agents = await createBuiltinAgents([], overrides, undefined, TEST_DEFAULT_MODEL)
101+
const agents = await createBuiltinAgents([], overrides, undefined, TEST_DEFAULT_MODEL, undefined, undefined, [], undefined, undefined)
102102

103103
// #then
104104
expect(agents.sisyphus.model).toBe("github-copilot/gpt-5.2")
105105
expect(agents.sisyphus.temperature).toBe(0.5)
106106
})
107+
108+
test("createBuiltinAgents excludes disabled skills from availableSkills", async () => {
109+
// #given
110+
const disabledSkills = new Set(["playwright"])
111+
112+
// #when
113+
const agents = await createBuiltinAgents([], {}, undefined, TEST_DEFAULT_MODEL, undefined, undefined, [], undefined, undefined, disabledSkills)
114+
115+
// #then
116+
expect(agents.sisyphus.prompt).not.toContain("playwright")
117+
expect(agents.sisyphus.prompt).toContain("frontend-ui-ux")
118+
expect(agents.sisyphus.prompt).toContain("git-master")
119+
})
107120
})
108121

109122
describe("buildAgent with category and skills", () => {

src/agents/utils.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,8 @@ export async function createBuiltinAgents(
149149
gitMasterConfig?: GitMasterConfig,
150150
discoveredSkills: LoadedSkill[] = [],
151151
client?: any,
152-
browserProvider?: BrowserAutomationProvider
152+
browserProvider?: BrowserAutomationProvider,
153+
disabledSkills?: Set<string>
153154
): Promise<Record<string, AgentConfig>> {
154155
if (!systemDefaultModel) {
155156
throw new Error("createBuiltinAgents requires systemDefaultModel")
@@ -170,7 +171,7 @@ export async function createBuiltinAgents(
170171
description: categories?.[name]?.description ?? CATEGORY_DESCRIPTIONS[name] ?? "General tasks",
171172
}))
172173

173-
const builtinSkills = createBuiltinSkills({ browserProvider })
174+
const builtinSkills = createBuiltinSkills({ browserProvider, disabledSkills })
174175
const builtinSkillNames = new Set(builtinSkills.map(s => s.name))
175176

176177
const builtinAvailable: AvailableSkill[] = builtinSkills.map((skill) => ({

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

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,4 +86,58 @@ describe("createBuiltinSkills", () => {
8686
expect(defaultSkills).toHaveLength(4)
8787
expect(agentBrowserSkills).toHaveLength(4)
8888
})
89+
90+
test("should exclude playwright when it is in disabledSkills", () => {
91+
// #given
92+
const options = { disabledSkills: new Set(["playwright"]) }
93+
94+
// #when
95+
const skills = createBuiltinSkills(options)
96+
97+
// #then
98+
expect(skills.map((s) => s.name)).not.toContain("playwright")
99+
expect(skills.map((s) => s.name)).toContain("frontend-ui-ux")
100+
expect(skills.map((s) => s.name)).toContain("git-master")
101+
expect(skills.map((s) => s.name)).toContain("dev-browser")
102+
expect(skills.length).toBe(3)
103+
})
104+
105+
test("should exclude multiple skills when they are in disabledSkills", () => {
106+
// #given
107+
const options = { disabledSkills: new Set(["playwright", "git-master"]) }
108+
109+
// #when
110+
const skills = createBuiltinSkills(options)
111+
112+
// #then
113+
expect(skills.map((s) => s.name)).not.toContain("playwright")
114+
expect(skills.map((s) => s.name)).not.toContain("git-master")
115+
expect(skills.map((s) => s.name)).toContain("frontend-ui-ux")
116+
expect(skills.map((s) => s.name)).toContain("dev-browser")
117+
expect(skills.length).toBe(2)
118+
})
119+
120+
test("should return an empty array when all skills are disabled", () => {
121+
// #given
122+
const options = {
123+
disabledSkills: new Set(["playwright", "frontend-ui-ux", "git-master", "dev-browser"]),
124+
}
125+
126+
// #when
127+
const skills = createBuiltinSkills(options)
128+
129+
// #then
130+
expect(skills.length).toBe(0)
131+
})
132+
133+
test("should return all skills when disabledSkills set is empty", () => {
134+
// #given
135+
const options = { disabledSkills: new Set<string>() }
136+
137+
// #when
138+
const skills = createBuiltinSkills(options)
139+
140+
// #then
141+
expect(skills.length).toBe(4)
142+
})
89143
})

src/features/builtin-skills/skills.ts

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1718,12 +1718,19 @@ EOF
17181718

17191719
export interface CreateBuiltinSkillsOptions {
17201720
browserProvider?: BrowserAutomationProvider
1721+
disabledSkills?: Set<string>
17211722
}
17221723

17231724
export function createBuiltinSkills(options: CreateBuiltinSkillsOptions = {}): BuiltinSkill[] {
1724-
const { browserProvider = "playwright" } = options
1725+
const { browserProvider = "playwright", disabledSkills } = options
17251726

17261727
const browserSkill = browserProvider === "agent-browser" ? agentBrowserSkill : playwrightSkill
17271728

1728-
return [browserSkill, frontendUiUxSkill, gitMasterSkill, devBrowserSkill]
1729+
const skills = [browserSkill, frontendUiUxSkill, gitMasterSkill, devBrowserSkill]
1730+
1731+
if (!disabledSkills) {
1732+
return skills
1733+
}
1734+
1735+
return skills.filter((skill) => !disabledSkills.has(skill.name))
17291736
}

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

Lines changed: 40 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -33,10 +33,12 @@ describe("resolveSkillContent", () => {
3333
expect(result).toBeNull()
3434
})
3535

36-
it("should return null for empty string", () => {
37-
// #given: builtin skills
38-
// #when: resolving content for empty string
39-
const result = resolveSkillContent("")
36+
it("should return null for disabled skill", () => {
37+
// #given: frontend-ui-ux skill disabled
38+
const options = { disabledSkills: new Set(["frontend-ui-ux"]) }
39+
40+
// #when: resolving content for disabled skill
41+
const result = resolveSkillContent("frontend-ui-ux", options)
4042

4143
// #then: returns null
4244
expect(result).toBeNull()
@@ -96,6 +98,20 @@ describe("resolveMultipleSkills", () => {
9698
expect(result.notFound).toEqual(["skill-one", "skill-two", "skill-three"])
9799
})
98100

101+
it("should treat disabled skills as not found", () => {
102+
// #given: frontend-ui-ux disabled, playwright not disabled
103+
const skillNames = ["frontend-ui-ux", "playwright"]
104+
const options = { disabledSkills: new Set(["frontend-ui-ux"]) }
105+
106+
// #when: resolving multiple skills with disabled one
107+
const result = resolveMultipleSkills(skillNames, options)
108+
109+
// #then: frontend-ui-ux in notFound, playwright resolved
110+
expect(result.resolved.size).toBe(1)
111+
expect(result.resolved.has("playwright")).toBe(true)
112+
expect(result.notFound).toEqual(["frontend-ui-ux"])
113+
})
114+
99115
it("should preserve skill order in resolved map", () => {
100116
// #given: list of skill names in specific order
101117
const skillNames = ["playwright", "frontend-ui-ux"]
@@ -122,10 +138,12 @@ describe("resolveSkillContentAsync", () => {
122138
expect(result).toContain("Role: Designer-Turned-Developer")
123139
})
124140

125-
it("should return null for non-existent skill", async () => {
126-
// #given: non-existent skill name
127-
// #when: resolving content async
128-
const result = await resolveSkillContentAsync("definitely-not-a-skill-12345")
141+
it("should return null for disabled skill", async () => {
142+
// #given: frontend-ui-ux disabled
143+
const options = { disabledSkills: new Set(["frontend-ui-ux"]) }
144+
145+
// #when: resolving content async for disabled skill
146+
const result = await resolveSkillContentAsync("frontend-ui-ux", options)
129147

130148
// #then: returns null
131149
expect(result).toBeNull()
@@ -160,6 +178,20 @@ describe("resolveMultipleSkillsAsync", () => {
160178
expect(result.resolved.get("playwright")).toContain("Playwright Browser Automation")
161179
})
162180

181+
it("should treat disabled skills as not found async", async () => {
182+
// #given: frontend-ui-ux disabled
183+
const skillNames = ["frontend-ui-ux", "playwright"]
184+
const options = { disabledSkills: new Set(["frontend-ui-ux"]) }
185+
186+
// #when: resolving multiple skills async with disabled one
187+
const result = await resolveMultipleSkillsAsync(skillNames, options)
188+
189+
// #then: frontend-ui-ux in notFound, playwright resolved
190+
expect(result.resolved.size).toBe(1)
191+
expect(result.resolved.has("playwright")).toBe(true)
192+
expect(result.notFound).toEqual(["frontend-ui-ux"])
193+
})
194+
163195
it("should NOT inject watermark when both options are disabled", async () => {
164196
// #given: git-master skill with watermark disabled
165197
const skillNames = ["git-master"]

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

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import type { GitMasterConfig, BrowserAutomationProvider } from "../../config/sc
88
export interface SkillResolutionOptions {
99
gitMasterConfig?: GitMasterConfig
1010
browserProvider?: BrowserAutomationProvider
11+
disabledSkills?: Set<string>
1112
}
1213

1314
const cachedSkillsByProvider = new Map<string, LoadedSkill[]>()
@@ -23,7 +24,12 @@ async function getAllSkills(options?: SkillResolutionOptions): Promise<LoadedSki
2324

2425
const [discoveredSkills, builtinSkillDefs] = await Promise.all([
2526
discoverSkills({ includeClaudeCodePaths: true }),
26-
Promise.resolve(createBuiltinSkills({ browserProvider: options?.browserProvider })),
27+
Promise.resolve(
28+
createBuiltinSkills({
29+
browserProvider: options?.browserProvider,
30+
disabledSkills: options?.disabledSkills,
31+
})
32+
),
2733
])
2834

2935
const builtinSkillsAsLoaded: LoadedSkill[] = builtinSkillDefs.map((skill) => ({
@@ -122,7 +128,10 @@ export function injectGitMasterConfig(template: string, config?: GitMasterConfig
122128
}
123129

124130
export function resolveSkillContent(skillName: string, options?: SkillResolutionOptions): string | null {
125-
const skills = createBuiltinSkills({ browserProvider: options?.browserProvider })
131+
const skills = createBuiltinSkills({
132+
browserProvider: options?.browserProvider,
133+
disabledSkills: options?.disabledSkills,
134+
})
126135
const skill = skills.find((s) => s.name === skillName)
127136
if (!skill) return null
128137

@@ -137,7 +146,10 @@ export function resolveMultipleSkills(skillNames: string[], options?: SkillResol
137146
resolved: Map<string, string>
138147
notFound: string[]
139148
} {
140-
const skills = createBuiltinSkills({ browserProvider: options?.browserProvider })
149+
const skills = createBuiltinSkills({
150+
browserProvider: options?.browserProvider,
151+
disabledSkills: options?.disabledSkills,
152+
})
141153
const skillMap = new Map(skills.map((s) => [s.name, s.template]))
142154

143155
const resolved = new Map<string, string>()

src/index.ts

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -308,8 +308,7 @@ const OhMyOpenCodePlugin: Plugin = async (ctx) => {
308308
});
309309
const disabledSkills = new Set(pluginConfig.disabled_skills ?? []);
310310
const systemMcpNames = getSystemMcpServerNames();
311-
const builtinSkills = createBuiltinSkills({ browserProvider }).filter((skill) => {
312-
if (disabledSkills.has(skill.name as never)) return false;
311+
const builtinSkills = createBuiltinSkills({ browserProvider, disabledSkills }).filter((skill) => {
313312
if (skill.mcpConfig) {
314313
for (const mcpName of Object.keys(skill.mcpConfig)) {
315314
if (systemMcpNames.has(mcpName)) return false;

src/plugin-handlers/config-handler.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,7 @@ export function createConfigHandler(deps: ConfigHandlerDeps) {
166166
];
167167

168168
const browserProvider = pluginConfig.browser_automation_engine?.provider ?? "playwright";
169+
const disabledSkills = new Set(pluginConfig.disabled_skills ?? []);
169170
const builtinAgents = await createBuiltinAgents(
170171
migratedDisabledAgents,
171172
pluginConfig.agents,
@@ -175,7 +176,8 @@ export function createConfigHandler(deps: ConfigHandlerDeps) {
175176
pluginConfig.git_master,
176177
allDiscoveredSkills,
177178
ctx.client,
178-
browserProvider
179+
browserProvider,
180+
disabledSkills
179181
);
180182

181183
// Claude Code agents: Do NOT apply permission migration

0 commit comments

Comments
 (0)