diff --git a/packages/coding-agent/src/core/package-manager.ts b/packages/coding-agent/src/core/package-manager.ts index 5890ec6c..39c2751d 100644 --- a/packages/coding-agent/src/core/package-manager.ts +++ b/packages/coding-agent/src/core/package-manager.ts @@ -118,6 +118,24 @@ interface ResourceAccumulator { themes: Map; } +/** + * Compute a numeric precedence rank for a resource based on its metadata. + * Lower rank = higher precedence. Used to sort resolved resources so that + * name-collision resolution ("first wins") produces the correct outcome. + * + * Precedence (highest to lowest): + * 0 project + settings entry (source: "local", scope: "project") + * 1 project + auto-discovered (source: "auto", scope: "project") + * 2 user + settings entry (source: "local", scope: "user") + * 3 user + auto-discovered (source: "auto", scope: "user") + * 4 package resource (origin: "package") + */ +function resourcePrecedenceRank(m: PathMetadata): number { + if (m.origin === "package") return 4; + const scopeBase = m.scope === "project" ? 0 : 2; + return scopeBase + (m.source === "local" ? 0 : 1); +} + interface PackageFilter { extensions?: string[]; skills?: string[]; @@ -2146,11 +2164,13 @@ export class DefaultPackageManager implements PackageManager { private toResolvedPaths(accumulator: ResourceAccumulator): ResolvedPaths { const toResolved = (entries: Map): ResolvedResource[] => { - return Array.from(entries.entries()).map(([path, { metadata, enabled }]) => ({ + const resolved = Array.from(entries.entries()).map(([path, { metadata, enabled }]) => ({ path, enabled, metadata, })); + resolved.sort((a, b) => resourcePrecedenceRank(a.metadata) - resourcePrecedenceRank(b.metadata)); + return resolved; }; return { diff --git a/packages/coding-agent/src/core/resource-loader.ts b/packages/coding-agent/src/core/resource-loader.ts index 095cf870..9450c9ed 100644 --- a/packages/coding-agent/src/core/resource-loader.ts +++ b/packages/coding-agent/src/core/resource-loader.ts @@ -413,7 +413,7 @@ export class DefaultResourceLoader implements ResourceLoader { const skillPaths = this.noSkills ? this.mergePaths(cliEnabledSkills, this.additionalSkillPaths) - : this.mergePaths([...enabledSkills, ...cliEnabledSkills], this.additionalSkillPaths); + : this.mergePaths([...cliEnabledSkills, ...enabledSkills], this.additionalSkillPaths); this.lastSkillPaths = skillPaths; this.updateSkillsFromPaths(skillPaths, metadataByPath); @@ -425,7 +425,7 @@ export class DefaultResourceLoader implements ResourceLoader { const promptPaths = this.noPromptTemplates ? this.mergePaths(cliEnabledPrompts, this.additionalPromptTemplatePaths) - : this.mergePaths([...enabledPrompts, ...cliEnabledPrompts], this.additionalPromptTemplatePaths); + : this.mergePaths([...cliEnabledPrompts, ...enabledPrompts], this.additionalPromptTemplatePaths); this.lastPromptPaths = promptPaths; this.updatePromptsFromPaths(promptPaths, metadataByPath); @@ -437,7 +437,7 @@ export class DefaultResourceLoader implements ResourceLoader { const themePaths = this.noThemes ? this.mergePaths(cliEnabledThemes, this.additionalThemePaths) - : this.mergePaths([...enabledThemes, ...cliEnabledThemes], this.additionalThemePaths); + : this.mergePaths([...cliEnabledThemes, ...enabledThemes], this.additionalThemePaths); this.lastThemePaths = themePaths; this.updateThemesFromPaths(themePaths, metadataByPath); diff --git a/packages/coding-agent/test/suite/regressions/2781-skill-collision-precedence.test.ts b/packages/coding-agent/test/suite/regressions/2781-skill-collision-precedence.test.ts new file mode 100644 index 00000000..e7e17f58 --- /dev/null +++ b/packages/coding-agent/test/suite/regressions/2781-skill-collision-precedence.test.ts @@ -0,0 +1,120 @@ +import { mkdirSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { DefaultResourceLoader } from "../../../src/core/resource-loader.js"; + +describe("issue #2781 skill collision precedence: user skills should override package skills", () => { + let tempDir: string; + let agentDir: string; + let cwd: string; + + beforeEach(() => { + tempDir = join(tmpdir(), `pi-2781-${Date.now()}-${Math.random().toString(36).slice(2)}`); + agentDir = join(tempDir, "agent"); + cwd = join(tempDir, "project"); + mkdirSync(agentDir, { recursive: true }); + mkdirSync(cwd, { recursive: true }); + }); + + afterEach(() => { + rmSync(tempDir, { recursive: true, force: true }); + }); + + function createPackageWithSkill(name: string, description: string): string { + const pkgDir = join(tempDir, `fake-package-${name}`); + const skillDir = join(pkgDir, "skills", name); + mkdirSync(skillDir, { recursive: true }); + writeFileSync( + join(pkgDir, "package.json"), + JSON.stringify({ name: `fake-pkg-${name}`, version: "1.0.0", pi: { skills: [`skills/${name}`] } }, null, 2), + ); + writeFileSync( + join(skillDir, "SKILL.md"), + `---\nname: ${name}\ndescription: ${description}\n---\nPackage skill content`, + ); + return pkgDir; + } + + function createUserSkill(name: string, description: string): string { + const skillDir = join(agentDir, "skills", name); + mkdirSync(skillDir, { recursive: true }); + const skillPath = join(skillDir, "SKILL.md"); + writeFileSync(skillPath, `---\nname: ${name}\ndescription: ${description}\n---\nUser skill content`); + return skillPath; + } + + function createProjectSkill(name: string, description: string): string { + const skillDir = join(cwd, ".pi", "skills", name); + mkdirSync(skillDir, { recursive: true }); + const skillPath = join(skillDir, "SKILL.md"); + writeFileSync(skillPath, `---\nname: ${name}\ndescription: ${description}\n---\nProject skill content`); + return skillPath; + } + + function createSettingsWithPackage(pkgDir: string, scope: "user" | "project"): void { + const settingsDir = scope === "user" ? agentDir : join(cwd, ".pi"); + mkdirSync(settingsDir, { recursive: true }); + writeFileSync(join(settingsDir, "settings.json"), JSON.stringify({ packages: [pkgDir] }, null, 2)); + } + + it("user auto-discovered skill should override package skill with same name", async () => { + const pkgDir = createPackageWithSkill("web-fetch", "Package web-fetch skill"); + const userSkillPath = createUserSkill("web-fetch", "User web-fetch override"); + createSettingsWithPackage(pkgDir, "user"); + + const loader = new DefaultResourceLoader({ cwd, agentDir }); + await loader.reload(); + + const { skills } = loader.getSkills(); + const webFetch = skills.find((s) => s.name === "web-fetch"); + expect(webFetch).toBeDefined(); + expect(webFetch!.filePath).toBe(userSkillPath); + expect(webFetch!.description).toBe("User web-fetch override"); + }); + + it("project auto-discovered skill should override package skill with same name", async () => { + const pkgDir = createPackageWithSkill("web-fetch", "Package web-fetch skill"); + const projectSkillPath = createProjectSkill("web-fetch", "Project web-fetch override"); + createSettingsWithPackage(pkgDir, "user"); + + const loader = new DefaultResourceLoader({ cwd, agentDir }); + await loader.reload(); + + const { skills } = loader.getSkills(); + const webFetch = skills.find((s) => s.name === "web-fetch"); + expect(webFetch).toBeDefined(); + expect(webFetch!.filePath).toBe(projectSkillPath); + expect(webFetch!.description).toBe("Project web-fetch override"); + }); + + it("project skill should override user skill which should override package skill", async () => { + const pkgDir = createPackageWithSkill("web-fetch", "Package web-fetch skill"); + createUserSkill("web-fetch", "User web-fetch override"); + const projectSkillPath = createProjectSkill("web-fetch", "Project web-fetch override"); + createSettingsWithPackage(pkgDir, "user"); + + const loader = new DefaultResourceLoader({ cwd, agentDir }); + await loader.reload(); + + const { skills } = loader.getSkills(); + const webFetch = skills.find((s) => s.name === "web-fetch"); + expect(webFetch).toBeDefined(); + expect(webFetch!.filePath).toBe(projectSkillPath); + expect(webFetch!.description).toBe("Project web-fetch override"); + }); + + it("collision diagnostics should report package skill as loser when user skill wins", async () => { + const pkgDir = createPackageWithSkill("web-fetch", "Package web-fetch skill"); + createUserSkill("web-fetch", "User web-fetch override"); + createSettingsWithPackage(pkgDir, "user"); + + const loader = new DefaultResourceLoader({ cwd, agentDir }); + await loader.reload(); + + const { diagnostics } = loader.getSkills(); + const collision = diagnostics.find((d) => d.type === "collision" && d.collision?.name === "web-fetch"); + expect(collision).toBeDefined(); + expect(collision!.collision!.loserPath).toContain("fake-package"); + }); +});