diff --git a/packages/cli/src/services/BundledSkillLoader.test.ts b/packages/cli/src/services/BundledSkillLoader.test.ts index 8e3b8cc46..1e3349dda 100644 --- a/packages/cli/src/services/BundledSkillLoader.test.ts +++ b/packages/cli/src/services/BundledSkillLoader.test.ts @@ -80,7 +80,7 @@ describe('BundledSkillLoader', () => { }); it('should load bundled skills as slash commands', async () => { - const skill = makeSkill({ priority: 42 }); + const skill = makeSkill(); mockSkillManager.listSkills.mockResolvedValue([skill]); const loader = new BundledSkillLoader(mockConfig); @@ -90,12 +90,25 @@ describe('BundledSkillLoader', () => { expect(commands[0].name).toBe('review'); expect(commands[0].description).toBe('Review code changes'); expect(commands[0].kind).toBe(CommandKind.SKILL); - expect(commands[0].completionPriority).toBe(42); expect(mockSkillManager.listSkills).toHaveBeenCalledWith({ level: 'bundled', }); }); + it('does not propagate skill.priority to completionPriority', async () => { + // Priority is intentionally scoped to the `/skills` listing (sorted in + // SkillManager.listSkills) and must NOT leak into the slash-completion + // menu / `/help` ordering — typing `/` should keep its prior behavior + // regardless of any skill's priority value. + const skill = makeSkill({ priority: 42 }); + mockSkillManager.listSkills.mockResolvedValue([skill]); + + const loader = new BundledSkillLoader(mockConfig); + const commands = await loader.loadCommands(signal); + + expect(commands[0].completionPriority).toBeUndefined(); + }); + it('should submit skill body as prompt without args', async () => { const skill = makeSkill(); mockSkillManager.listSkills.mockResolvedValue([skill]); diff --git a/packages/cli/src/services/BundledSkillLoader.ts b/packages/cli/src/services/BundledSkillLoader.ts index cd31f2658..1b53208be 100644 --- a/packages/cli/src/services/BundledSkillLoader.ts +++ b/packages/cli/src/services/BundledSkillLoader.ts @@ -71,7 +71,6 @@ export class BundledSkillLoader implements ICommandLoader { modelInvocable: !skill.disableModelInvocation, argumentHint: skill.argumentHint, whenToUse: skill.whenToUse, - completionPriority: skill.priority, action: async (context, _args): Promise => { // Resolve template variables in skill body let body = skill.body; diff --git a/packages/cli/src/services/SkillCommandLoader.test.ts b/packages/cli/src/services/SkillCommandLoader.test.ts index 10f2178c1..a8135d7ac 100644 --- a/packages/cli/src/services/SkillCommandLoader.test.ts +++ b/packages/cli/src/services/SkillCommandLoader.test.ts @@ -84,7 +84,7 @@ describe('SkillCommandLoader', () => { }); it('should load user skill as slash command with correct properties', async () => { - const skill = makeSkill({ level: 'user', priority: 42 }); + const skill = makeSkill({ level: 'user' }); mockSkillManager.listSkills.mockImplementation( ({ level }: { level: string }) => Promise.resolve(level === 'user' ? [skill] : []), @@ -102,7 +102,21 @@ describe('SkillCommandLoader', () => { expect(cmd.sourceLabel).toBe('User'); expect(cmd.sourceDetail).toBe('user'); expect(cmd.modelInvocable).toBe(true); - expect(cmd.completionPriority).toBe(42); + }); + + it('does not propagate skill.priority to completionPriority', async () => { + // Priority is scoped to the `/skills` listing only; slash-completion / + // `/help` ordering should be independent of any skill's priority value. + const skill = makeSkill({ level: 'user', priority: 42 }); + mockSkillManager.listSkills.mockImplementation( + ({ level }: { level: string }) => + Promise.resolve(level === 'user' ? [skill] : []), + ); + + const loader = new SkillCommandLoader(mockConfig); + const commands = await loader.loadCommands(signal); + + expect(commands[0].completionPriority).toBeUndefined(); }); it('should load project skill with sourceLabel "Project"', async () => { diff --git a/packages/cli/src/services/SkillCommandLoader.ts b/packages/cli/src/services/SkillCommandLoader.ts index 18e9d3dd6..204e57b2a 100644 --- a/packages/cli/src/services/SkillCommandLoader.ts +++ b/packages/cli/src/services/SkillCommandLoader.ts @@ -94,7 +94,6 @@ export class SkillCommandLoader implements ICommandLoader { modelInvocable, argumentHint: skill.argumentHint, whenToUse: skill.whenToUse, - completionPriority: skill.priority, action: async (context, _args): Promise => { const body = skill.body; diff --git a/packages/cli/src/ui/components/Help.test.tsx b/packages/cli/src/ui/components/Help.test.tsx index c314010b5..e99565f86 100644 --- a/packages/cli/src/ui/components/Help.test.tsx +++ b/packages/cli/src/ui/components/Help.test.tsx @@ -186,7 +186,10 @@ describe('Help Component', () => { expect(output).not.toContain('/test'); }); - it('orders help commands by completionPriority before name', () => { + it('orders help commands alphabetically regardless of completionPriority', () => { + // Skill priority is scoped to the /skills listing; /help intentionally + // stays alphabetical so a high-priority skill can't push a built-in + // command around in the help view. const commands: SlashCommand[] = [ { name: 'alpha', @@ -217,8 +220,8 @@ describe('Help Component', () => { ); const output = lastFrame() ?? ''; - expect(output.indexOf('/zeta')).toBeLessThan(output.indexOf('/alpha')); expect(output.indexOf('/alpha')).toBeLessThan(output.indexOf('/beta')); + expect(output.indexOf('/beta')).toBeLessThan(output.indexOf('/zeta')); }); it('switches tabs with Tab and Shift+Tab when interactive', () => { diff --git a/packages/cli/src/ui/components/Help.tsx b/packages/cli/src/ui/components/Help.tsx index b6bb1d90e..b7fe57c06 100644 --- a/packages/cli/src/ui/components/Help.tsx +++ b/packages/cli/src/ui/components/Help.tsx @@ -497,17 +497,10 @@ function groupCommands( .sort((a, b) => a.order - b.order) .map((group) => ({ ...group, - commands: group.commands.sort(compareCommandsForHelp), + commands: group.commands.sort((a, b) => a.name.localeCompare(b.name)), })); } -function compareCommandsForHelp(a: SlashCommand, b: SlashCommand): number { - return ( - (b.completionPriority ?? 0) - (a.completionPriority ?? 0) || - a.name.localeCompare(b.name) - ); -} - function truncateText(text: string, maxLength: number): string { if (maxLength <= 1 || text.length <= maxLength) return text; return `${text.slice(0, maxLength - 1)}…`; diff --git a/packages/core/src/skills/skill-manager.test.ts b/packages/core/src/skills/skill-manager.test.ts index 727eea4b6..ed80bc026 100644 --- a/packages/core/src/skills/skill-manager.test.ts +++ b/packages/core/src/skills/skill-manager.test.ts @@ -828,6 +828,10 @@ Body`); 'high-priority', 'bad-priority', ]); + // The non-number priority should also be normalized on the skill itself, + // not just at sort time, so downstream consumers see a clean value. + const badSkill = skills.find((s) => s.name === 'bad-priority'); + expect(badSkill?.priority).toBe(0); }); it('should deduplicate same-name skills across provider dirs within a level', async () => { diff --git a/packages/core/src/skills/skill-manager.ts b/packages/core/src/skills/skill-manager.ts index 0931ac490..76b866fcd 100644 --- a/packages/core/src/skills/skill-manager.ts +++ b/packages/core/src/skills/skill-manager.ts @@ -898,16 +898,25 @@ export class SkillManager { // non-number `priority` would silently sort at the bottom (via // normalizeSkillPriority) with no diagnostic. Warn here so the // extension author sees the same signal a SKILL.md author would. - if ( + const hasInvalidPriority = skill.priority !== undefined && (typeof skill.priority !== 'number' || - !Number.isFinite(skill.priority)) - ) { + !Number.isFinite(skill.priority)); + if (hasInvalidPriority) { debugLogger.warn( `Extension "${extension.name}" skill "${skill.name}" has invalid priority (${typeof skill.priority}: ${String(skill.priority)}); treating as 0.`, ); } - skills.push({ ...skill, extensionName: extension.name }); + skills.push({ + ...skill, + extensionName: extension.name, + // Normalize so downstream consumers reading `skill.priority` + // observe the same value reflected by the warning above and by + // sort-time normalization. + priority: hasInvalidPriority + ? 0 + : (skill.priority as number | undefined), + }); }); } debugLogger.debug(