From 87acd3e7021745db697924518554db2e39cf7a66 Mon Sep 17 00:00:00 2001 From: minhn4 Date: Thu, 6 Aug 2026 07:58:20 +0700 Subject: [PATCH] fix(opencode): resolve duplicate skill names deterministically, preferring .claude MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Skills are parsed with unbounded concurrency and the loser of a name collision was decided by whichever parse finished first, so which copy won varied run to run. Assign each discovered SKILL.md a source rank (.claude external > .agents external > config dir > skills.paths > skills.urls) and resolve collisions by that rank with a lexicographic path tie-break, both order-independent. A Claude Code skill now deterministically shadows an .agents copy of the same name — the common case being the very same skill reached through a symlink. Co-Authored-By: Claude Fable 5 --- packages/opencode/src/skill/index.ts | 89 +++++++++++++++++----- packages/opencode/test/skill/skill.test.ts | 43 +++++++++++ 2 files changed, 115 insertions(+), 17 deletions(-) diff --git a/packages/opencode/src/skill/index.ts b/packages/opencode/src/skill/index.ts index d382b6a5cc..674e1ffbd7 100644 --- a/packages/opencode/src/skill/index.ts +++ b/packages/opencode/src/skill/index.ts @@ -24,6 +24,27 @@ const EXTERNAL_SKILL_PATTERN = "skills/**/SKILL.md" const CODEV_SKILL_PATTERN = "{skill,skills}/**/SKILL.md" const SKILL_PATTERN = "**/SKILL.md" +// When two SKILL.md files declare the same `name`, the winner is chosen by +// this rank (higher wins), NOT by which parse finishes first — skills are +// parsed with unbounded concurrency, so a completion-order tie-break would be +// non-deterministic run to run. `.claude` outranks `.agents` so a Claude Code +// skill deterministically shadows an `.agents` copy of the same name (the two +// are frequently the very same skill reached through a symlink); the config +// dir, `skills.paths` and `skills.urls` fall below the external dirs. Equal +// ranks tie-break on the lexicographically smaller path, which is also +// order-independent — so the resolved set is identical on every run. +const SkillPriority = { + claudeExternal: 50, + agentsExternal: 40, + configDir: 30, + skillsPath: 20, + skillsUrl: 10, +} as const + +function externalDirPriority(dir: string): number { + return dir === CLAUDE_EXTERNAL_DIR ? SkillPriority.claudeExternal : SkillPriority.agentsExternal +} + // Built-in skill that ships with opencode. The model's intuition for what an // codev.json should look like is often wrong, and opencode hard-fails on // invalid config, so users hit cryptic startup errors. Loading this skill @@ -81,16 +102,27 @@ export class NotFoundError extends Schema.TaggedErrorClass()("Ski type State = { skills: Record + // name -> priority of the skill currently occupying skills[name]; lets a + // later, higher-ranked match replace an earlier one regardless of the order + // the concurrent parses complete in. + priorities: Record dirs: Set } +type Match = { + path: string + priority: number +} + type DiscoveryState = { - matches: string[] + matches: Match[] dirs: string[] } type ScanState = { - matches: Set + // path -> highest priority it was scanned at (a path reached from two roots + // keeps the stronger claim). + matches: Map dirs: Set } @@ -102,7 +134,12 @@ export interface Interface { readonly available: (agent?: Agent.Info) => Effect.Effect } -const add = Effect.fnUntraced(function* (state: State, match: string, events: EventV2Bridge.Service["Service"]) { +const add = Effect.fnUntraced(function* ( + state: State, + match: string, + priority: number, + events: EventV2Bridge.Service["Service"], +) { const md = yield* Effect.tryPromise({ try: () => ConfigMarkdown.parse(match), catch: (err) => err, @@ -122,15 +159,26 @@ const add = Effect.fnUntraced(function* (state: State, match: string, events: Ev if (!isSkillFrontmatter(md.data)) return - if (state.skills[md.data.name]) { + state.dirs.add(path.dirname(match)) + + const existing = state.skills[md.data.name] + if (existing) { + // A built-in skill (registered before disk discovery) has no priority + // entry; treat it as the weakest so any disk skill overrides it. + const existingPriority = state.priorities[md.data.name] ?? Number.NEGATIVE_INFINITY + // Deterministic winner, independent of parse-completion order: higher + // rank wins; equal rank keeps the lexicographically smaller path. + const keepExisting = + priority < existingPriority || (priority === existingPriority && existing.location <= match) yield* Effect.logWarning("duplicate skill name", { name: md.data.name, - existing: state.skills[md.data.name].location, - duplicate: match, + kept: keepExisting ? existing.location : match, + dropped: keepExisting ? match : existing.location, }) + if (keepExisting) return } - state.dirs.add(path.dirname(match)) + state.priorities[md.data.name] = priority state.skills[md.data.name] = { name: md.data.name, description: md.data.description, @@ -143,6 +191,7 @@ const scan = Effect.fnUntraced(function* ( state: ScanState, root: string, pattern: string, + priority: number, opts?: { dot?: boolean; scope?: string }, ) { const matches = yield* Effect.tryPromise({ @@ -165,7 +214,8 @@ const scan = Effect.fnUntraced(function* ( ) for (const match of matches) { - state.matches.add(match) + const prev = state.matches.get(match) + state.matches.set(match, prev === undefined ? priority : Math.max(prev, priority)) state.dirs.add(path.dirname(match)) } }) @@ -180,7 +230,7 @@ const discoverSkills = Effect.fnUntraced(function* ( directory: string, worktree: string, ) { - const state: ScanState = { matches: new Set(), dirs: new Set() } + const state: ScanState = { matches: new Map(), dirs: new Set() } const externalDirs: string[] = [] if (!disableExternalSkills) { @@ -190,7 +240,7 @@ const discoverSkills = Effect.fnUntraced(function* ( for (const dir of externalDirs) { const root = path.join(global.home, dir) if (!(yield* fsys.isDir(root))) continue - yield* scan(state, root, EXTERNAL_SKILL_PATTERN, { dot: true, scope: "global" }) + yield* scan(state, root, EXTERNAL_SKILL_PATTERN, externalDirPriority(dir), { dot: true, scope: "global" }) } const upDirs = yield* fsys @@ -198,13 +248,18 @@ const discoverSkills = Effect.fnUntraced(function* ( .pipe(Effect.catch(() => Effect.succeed([] as string[]))) for (const root of upDirs) { - yield* scan(state, root, EXTERNAL_SKILL_PATTERN, { dot: true, scope: "project" }) + // up() returns each matched dir as `/`, so its + // basename is the external dir name that fixes its priority. + yield* scan(state, root, EXTERNAL_SKILL_PATTERN, externalDirPriority(path.basename(root)), { + dot: true, + scope: "project", + }) } } const configDirs = yield* config.directories() for (const dir of configDirs) { - yield* scan(state, dir, CODEV_SKILL_PATTERN) + yield* scan(state, dir, CODEV_SKILL_PATTERN, SkillPriority.configDir) } const cfg = yield* config.get() @@ -216,18 +271,18 @@ const discoverSkills = Effect.fnUntraced(function* ( continue } - yield* scan(state, dir, SKILL_PATTERN) + yield* scan(state, dir, SKILL_PATTERN, SkillPriority.skillsPath) } for (const url of cfg.skills?.urls ?? []) { const pulledDirs = yield* discovery.pull(url) for (const dir of pulledDirs) { - yield* scan(state, dir, SKILL_PATTERN) + yield* scan(state, dir, SKILL_PATTERN, SkillPriority.skillsUrl) } } return { - matches: Array.from(state.matches), + matches: Array.from(state.matches, ([path, priority]) => ({ path, priority })), dirs: Array.from(state.dirs), } }) @@ -237,7 +292,7 @@ const loadSkills = Effect.fnUntraced(function* ( discovered: DiscoveryState, events: EventV2Bridge.Service["Service"], ) { - yield* Effect.forEach(discovered.matches, (match) => add(state, match, events), { + yield* Effect.forEach(discovered.matches, (match) => add(state, match.path, match.priority, events), { concurrency: "unbounded", discard: true, }) @@ -272,7 +327,7 @@ const layer = Layer.effect( ) const state = yield* InstanceState.make( Effect.fn("Skill.state")(function* () { - const s: State = { skills: {}, dirs: new Set() } + const s: State = { skills: {}, priorities: {}, dirs: new Set() } // Register the built-in skill BEFORE disk discovery so a user-disk // skill with the same name can override it. s.skills[CUSTOMIZE_CODEV_SKILL_NAME] = { diff --git a/packages/opencode/test/skill/skill.test.ts b/packages/opencode/test/skill/skill.test.ts index 772180eb30..2265e0291a 100644 --- a/packages/opencode/test/skill/skill.test.ts +++ b/packages/opencode/test/skill/skill.test.ts @@ -443,6 +443,49 @@ description: A skill in the .agents/skills directory. ), ) + it.live("prefers the .claude copy when the same skill name is in .claude and .agents", () => + provideTmpdirInstance( + (dir) => + Effect.gen(function* () { + yield* Effect.promise(() => + Promise.all([ + Bun.write( + path.join(dir, ".claude", "skills", "dup-skill", "SKILL.md"), + `--- +name: dup-skill +description: The .claude copy. +--- + +# Claude copy +`, + ), + Bun.write( + path.join(dir, ".agents", "skills", "dup-skill", "SKILL.md"), + `--- +name: dup-skill +description: The .agents copy. +--- + +# Agents copy +`, + ), + ]), + ) + + const skill = yield* Skill.Service + const list = (yield* skill.all()).filter((s) => s.location !== "") + // A single winner (the duplicate name collapses to one entry), and + // it is deterministically the .claude copy — never the .agents one, + // regardless of parse-completion order. + expect(list.filter((s) => s.name === "dup-skill")).toHaveLength(1) + const resolved = list.find((s) => s.name === "dup-skill")! + expect(resolved.location).toContain(path.join(".claude", "skills", "dup-skill", "SKILL.md")) + expect(resolved.description).toBe("The .claude copy.") + }), + { git: true }, + ), + ) + itWithoutClaudeCodeSkills.live("skips Claude Code skills when disabled", () => provideTmpdirInstance( (dir) =>