From 96722aa67eb20fbbe3424ddff2aa2762a2958e64 Mon Sep 17 00:00:00 2001 From: yiliang114 <1204183885@qq.com> Date: Mon, 18 May 2026 19:00:51 +0800 Subject: [PATCH] fix(skills): scope priority to /skills listing only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Earlier in this PR, `skill.priority` was mapped into `SlashCommand.completionPriority` on both bundled and non-bundled skill loaders, so a high-priority skill also bubbled up in the slash-completion menu and the `/help` custom-commands tab. That was broader than intended — the design goal is for `priority:` to control the `/skills` listing only, with everything else (typing `/`, mid-input completion, `/help`) staying purely alphabetical so a skill can't reorder built-in commands. Changes: - BundledSkillLoader / SkillCommandLoader: drop the `completionPriority: skill.priority` mapping. Skill commands now have no `completionPriority`, falling back to alphabetical+recency in the shared completion comparator. - Help.tsx: revert the per-group sort to `localeCompare` and remove the `compareCommandsForHelp` helper. `/help` is again purely alphabetical within each group. - Tests: - Both loader tests assert `completionPriority` is `undefined` when a skill has a `priority` set, locking the non-leakage in. - Help.test.tsx's "orders by completionPriority" case is replaced with "orders alphabetically regardless of completionPriority", so a future change that re-introduces the leak fails the test. - Extension-skill validation also normalizes `skill.priority` to 0 (in addition to the existing sort-time normalization) so downstream consumers see a clean value matching the emitted warning. Validation: - 177/177 unit tests pass across the 5 affected test files - core typecheck clean - bundled CLI built (`npm run bundle`) and exercised via tmux E2E: E1 /skills sorted by priority, E2 / completion menu unaffected, E3 mid-input alphabetical, E4 invalid priority warns + skill loads, E5 order stable across restart — all 5 pass. --- .../src/services/BundledSkillLoader.test.ts | 17 +++++++++++++++-- .../cli/src/services/BundledSkillLoader.ts | 1 - .../src/services/SkillCommandLoader.test.ts | 18 ++++++++++++++++-- .../cli/src/services/SkillCommandLoader.ts | 1 - packages/cli/src/ui/components/Help.test.tsx | 7 +++++-- packages/cli/src/ui/components/Help.tsx | 9 +-------- packages/core/src/skills/skill-manager.test.ts | 4 ++++ packages/core/src/skills/skill-manager.ts | 17 +++++++++++++---- 8 files changed, 54 insertions(+), 20 deletions(-) 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(