From 222b7395011f22cb7451c0b80984c9c6c214feb0 Mon Sep 17 00:00:00 2001 From: "N. Taylor Mullen" Date: Wed, 14 Jan 2026 20:02:17 -0800 Subject: [PATCH] feat(skills): add conflict detection and warnings for skill overrides (#16709) --- packages/core/src/skills/skillManager.test.ts | 111 ++++++++++++++++++ packages/core/src/skills/skillManager.ts | 25 +++- 2 files changed, 133 insertions(+), 3 deletions(-) diff --git a/packages/core/src/skills/skillManager.test.ts b/packages/core/src/skills/skillManager.test.ts index 20cba08405..0171ca0f61 100644 --- a/packages/core/src/skills/skillManager.test.ts +++ b/packages/core/src/skills/skillManager.test.ts @@ -12,6 +12,8 @@ import { SkillManager } from './skillManager.js'; import { Storage } from '../config/storage.js'; import { type GeminiCLIExtension } from '../config/config.js'; import { loadSkillsFromDir, type SkillDefinition } from './skillLoader.js'; +import { coreEvents } from '../utils/events.js'; +import { debugLogger } from '../utils/debugLogger.js'; vi.mock('./skillLoader.js', async (importOriginal) => { const actual = await importOriginal(); @@ -263,4 +265,113 @@ body1`, expect(service.isAdminEnabled()).toBe(false); }); + + describe('Conflict Detection', () => { + it('should emit UI warning when a non-built-in skill is overridden', async () => { + const emitFeedbackSpy = vi.spyOn(coreEvents, 'emitFeedback'); + const userDir = path.join(testRootDir, 'user'); + const projectDir = path.join(testRootDir, 'workspace'); + await fs.mkdir(userDir, { recursive: true }); + await fs.mkdir(projectDir, { recursive: true }); + + const skillName = 'conflicting-skill'; + const userSkillPath = path.join(userDir, 'SKILL.md'); + const projectSkillPath = path.join(projectDir, 'SKILL.md'); + + vi.mocked(loadSkillsFromDir).mockImplementation(async (dir) => { + if (dir === userDir) { + return [ + { + name: skillName, + description: 'user-desc', + location: userSkillPath, + body: '', + }, + ]; + } + if (dir === projectDir) { + return [ + { + name: skillName, + description: 'project-desc', + location: projectSkillPath, + body: '', + }, + ]; + } + return []; + }); + + vi.spyOn(Storage, 'getUserSkillsDir').mockReturnValue(userDir); + const storage = new Storage('/dummy'); + vi.spyOn(storage, 'getProjectSkillsDir').mockReturnValue(projectDir); + + const service = new SkillManager(); + // @ts-expect-error accessing private method for testing + vi.spyOn(service, 'discoverBuiltinSkills').mockResolvedValue(undefined); + + await service.discoverSkills(storage, []); + + expect(emitFeedbackSpy).toHaveBeenCalledWith( + 'warning', + expect.stringContaining( + `Skill conflict detected: "${skillName}" from "${projectSkillPath}" is overriding the same skill from "${userSkillPath}".`, + ), + ); + }); + + it('should log warning but NOT emit UI warning when a built-in skill is overridden', async () => { + const emitFeedbackSpy = vi.spyOn(coreEvents, 'emitFeedback'); + const debugWarnSpy = vi.spyOn(debugLogger, 'warn'); + const userDir = path.join(testRootDir, 'user'); + await fs.mkdir(userDir, { recursive: true }); + + const skillName = 'builtin-skill'; + const userSkillPath = path.join(userDir, 'SKILL.md'); + const builtinSkillPath = 'builtin/loc'; + + vi.mocked(loadSkillsFromDir).mockImplementation(async (dir) => { + if (dir.endsWith('builtin')) { + return [ + { + name: skillName, + description: 'builtin-desc', + location: builtinSkillPath, + body: '', + isBuiltin: true, + }, + ]; + } + if (dir === userDir) { + return [ + { + name: skillName, + description: 'user-desc', + location: userSkillPath, + body: '', + }, + ]; + } + return []; + }); + + vi.spyOn(Storage, 'getUserSkillsDir').mockReturnValue(userDir); + const storage = new Storage('/dummy'); + vi.spyOn(storage, 'getProjectSkillsDir').mockReturnValue('/non-existent'); + + const service = new SkillManager(); + + await service.discoverSkills(storage, []); + + // UI warning should not be called + expect(emitFeedbackSpy).not.toHaveBeenCalled(); + + // Debug warning should be called + expect(debugWarnSpy).toHaveBeenCalledWith( + expect.stringContaining( + `Skill "${skillName}" from "${userSkillPath}" is overriding the built-in skill.`, + ), + ); + }); + }); }); diff --git a/packages/core/src/skills/skillManager.ts b/packages/core/src/skills/skillManager.ts index b2ec9e660e..d80202cd5b 100644 --- a/packages/core/src/skills/skillManager.ts +++ b/packages/core/src/skills/skillManager.ts @@ -9,6 +9,8 @@ import { fileURLToPath } from 'node:url'; import { Storage } from '../config/storage.js'; import { type SkillDefinition, loadSkillsFromDir } from './skillLoader.js'; import type { GeminiCLIExtension } from '../config/config.js'; +import { debugLogger } from '../utils/debugLogger.js'; +import { coreEvents } from '../utils/events.js'; export { type SkillDefinition }; @@ -86,10 +88,27 @@ export class SkillManager { } private addSkillsWithPrecedence(newSkills: SkillDefinition[]): void { - const skillMap = new Map(); - for (const skill of [...this.skills, ...newSkills]) { - skillMap.set(skill.name, skill); + const skillMap = new Map( + this.skills.map((s) => [s.name, s]), + ); + + for (const newSkill of newSkills) { + const existingSkill = skillMap.get(newSkill.name); + if (existingSkill && existingSkill.location !== newSkill.location) { + if (existingSkill.isBuiltin) { + debugLogger.warn( + `Skill "${newSkill.name}" from "${newSkill.location}" is overriding the built-in skill.`, + ); + } else { + coreEvents.emitFeedback( + 'warning', + `Skill conflict detected: "${newSkill.name}" from "${newSkill.location}" is overriding the same skill from "${existingSkill.location}".`, + ); + } + } + skillMap.set(newSkill.name, newSkill); } + this.skills = Array.from(skillMap.values()); }