feat(skills): support priority field in SKILL.md for sorting skill display order#4155
Merged
Conversation
…splay order Closes #4136
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
- /skills was re-sorting alphabetically after listSkills(), masking the new priority order. Drop the redundant sort and reuse the manager's output directly. - Treat missing priority as 0 instead of -Infinity so an explicit negative priority (e.g. -1) sorts below unset skills, which matches user intent.
Collaborator
Author
|
测试了一下，行为符合预期： 
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an optional priority numeric field to SKILL.md frontmatter so skill authors can influence ordering (higher priority first), and propagates that signal into CLI slash-command completion ranking.
Changes:
- Extend
SkillConfigwithpriority?: number. - Parse
priorityfromSKILL.mdfrontmatter and sortSkillManager.listSkills()bypriority(desc) thenname(asc). - Bridge skill
priorityinto CLI slash commands viacompletionPriority, and preservelistSkills()ordering in/skillsoutput.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/skills/types.ts | Adds priority?: number to skill config type and documents intended ordering semantics. |
| packages/core/src/skills/skill-manager.ts | Parses priority from frontmatter and updates skill listing sort order. |
| packages/cli/src/ui/commands/skillsCommand.ts | Stops re-sorting; relies on SkillManager.listSkills() ordering. |
| packages/cli/src/services/SkillCommandLoader.ts | Maps skill.priority into slash command completionPriority. |
| packages/cli/src/services/BundledSkillLoader.ts | Maps skill.priority into slash command completionPriority for bundled skills. |
Comments suppressed due to low confidence (2)
packages/core/src/skills/skill-manager.ts:690
priorityparsing usesNumber(frontmatter['priority']), which will silently coerce non-numeric YAML values (e.g.priority:→ null → 0,priority: true→ 1). This can make an accidentally empty/mistyped field affect ordering. Consider treatingnull/empty as “unset”, and otherwise requiretypeof raw === 'number'(or a numeric string) and reject booleans/objects explicitly before converting.
// Optional `priority` frontmatter: higher values sort first.
let priority: number | undefined;
if (frontmatter['priority'] !== undefined) {
priority = Number(frontmatter['priority']);
if (!Number.isFinite(priority)) {
throw new Error('"priority" must be a finite number');
}
}
packages/core/src/skills/skill-manager.ts:237
- New behavior (priority parsing + priority-desc sorting) doesn’t appear to be covered by unit tests. Since this file already has
SkillManagertests, please add cases that (1) parse numericprioritycorrectly, (2) reject invalid values, and (3) assert list ordering is priority-desc then name-asc with missing priority treated as 0.
// Sort by priority desc (unspecified treated as 0), then by name.
skills.sort(
(a, b) =>
(b.priority ?? 0) - (a.priority ?? 0) || a.name.localeCompare(b.name),
);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
wenshao
requested changes
May 16, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
[Critical] packages/core/src/skills/skill-load.ts — Extension skills missing priority parsing
skill-load.ts has its own parseSkillContent() used by extensions, but the PR only updated skill-manager.ts's parseSkillContent. Any extension SKILL.md with priority in frontmatter will silently drop it. The same pattern previously affected whenToUse and disable-model-invocation, which were eventually synced to both parsers (see skill-load.ts:151-157 comment).
Suggested fix: Add the same priority extraction logic in skill-load.ts's parseSkillContent() (after paths parsing, before config construction).
[Suggestion] packages/core/src/skills/skill-load.ts:212 — validateConfig() doesn't validate priority field. Consider adding a comment noting priority validation is done inline in parseSkillFromMd, or add a range/type check.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
wenshao
reviewed
May 16, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Second-opinion review with glm-5.1 (previous review used DeepSeek/deepseek-v4-pro)
All previously flagged issues (extension skills missing priority parsing in skill-load.ts, Number() coercion, test coverage gaps for sort order and completionPriority mapping) are still valid and should be addressed.
New finding:
[Suggestion] priority frontmatter field has no user-facing documentation
docs/users/features/skills.md documents name, description, paths, disable-model-invocation, argument-hint, and when_to_use, but the new priority field is absent. Users cannot discover this feature without reading source code. Consider adding a section documenting the field, its default value (0), and that higher values sort first.
No new Critical issues found beyond what the previous review already identified.
— glm-5.1 via Qwen Code /review
wenshao
requested changes
May 17, 2026
wenshao
requested changes
May 17, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
[Critical] Extension skills bypass validateConfig and parsePriorityField — no warning for invalid priority values.
In listSkillsAtLevel (~line 897), extension-provided skills are spread directly without any validation:
extension.skills?.forEach((skill) => {
skills.push({ ...skill, extensionName: extension.name });
});Project/user/bundled skills flow through parseSkillContent → parsePriorityField → validateConfig, which catches invalid values and emits warnings. An extension author writing priority: "high" (string, not number) will see their skill silently sort at the bottom with zero diagnostic output — normalizeSkillPriority silently coerces it to 0.
Suggested fix: Add a validation check before pushing extension skills, or run them through validateConfig:
extension.skills?.forEach((skill) => {
if (
skill.priority !== undefined &&
(typeof skill.priority !== 'number' || !Number.isFinite(skill.priority))
) {
debugLogger.warn(
`Extension "${extension.name}" skill "${skill.name}" has invalid priority`,
);
}
skills.push({ ...skill, extensionName: extension.name });
});[Suggestion] parseSkillContent is duplicated across skill-manager.ts and skill-load.ts (~100 lines of shared config construction). When the next frontmatter field is added, updating only one copy will cause silent divergence. Consider extracting a shared buildSkillConfig(frontmatter, overrides) factory.
[Suggestion] Exported functions parsePriorityField and normalizeSkillPriority in skill-load.ts lack direct unit tests — only indirectly covered through integration tests. Inputs like NaN, Infinity, "string", true are not directly validated.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Extension-provided skills bypass parseSkillContent / validateConfig, so a non-number `priority` was silently normalized to 0 in the sort with zero diagnostic. Match the SKILL.md author signal: warn at load time so the extension author can see and fix the typo. Addresses PR #4155 review (the extension-bypass-validation point).
…killPriority Both helpers are exported but previously had no direct tests — coverage came only via parseSkillContent and listSkills. Adds inputs the integration paths can't surface cleanly: -0 / NaN / Infinity, numeric strings, objects, arrays, and the boolean coercion regression that motivated the strict typecheck. Also adds a NOTE on parsePriorityField warning future contributors that SKILL.md frontmatter parsing lives in two places (parseSkillContent here and SkillManager.parseSkillContent), so any new field must be wired into both — the same regression that previously hit whenToUse, disable-model-invocation, paths, and priority. Full dedup of the two parseSkillContent bodies is left as a follow-up refactor. Addresses the remaining two [Suggestion] items from PR #4155 review.
wenshao
reviewed
May 17, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
wenshao
reviewed
May 17, 2026
| if ( | ||
| skill.priority !== undefined && | ||
| (typeof skill.priority !== 'number' || | ||
| !Number.isFinite(skill.priority)) |
Collaborator
There was a problem hiding this comment.
[Suggestion] The new debugLogger.warn for invalid extension skill priority has no spy assertion in tests.
The test should normalize non-number extension priorities during sorting exercises this code path but never asserts that the warning was logged. If the warning is removed or refactored, no test would catch it.
Suggested change
| !Number.isFinite(skill.priority)) | |
| // In the test: | |
| const warnSpy = vi.spyOn(debugLogger, 'warn'); | |
| // ... after listSkills(): | |
| expect(warnSpy).toHaveBeenCalledWith( | |
| expect.stringContaining('invalid priority') | |
| ); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Holding off on the spy assertion. The debugLogger here is module-private and writes to a session log file rather than console.warn, so vi.spyOn(debugLogger, 'warn') would need us to either export the logger or mock createDebugLogger — both feel heavier than the value of locking in a single-line warning. The "treating as 0" behavior is asserted via expect(badSkill?.priority).toBe(0) in the same test, so a refactor that removes the normalization (the part that actually matters for sort correctness) still fails. Open to revisiting if you have a lighter pattern in mind.
wenshao
reviewed
May 17, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Second-opinion review with DeepSeek/deepseek-v4-pro (previous review used mimo-v2.5-pro)
All previously flagged issues are still present and have existing inline comments:
completionPrioritynormalization gap inSkillCommandLoader.ts:97/BundledSkillLoader.ts:74compareCommandsForHelpusing?? 0instead ofnormalizeSkillPriorityinHelp.tsx:506- Missing test file for
skillsCommand.ts
One additional observation not yet covered:
[Suggestion] The listSkills() sort change (alphabetical → priority-based) propagates to the model-facing <available_skills> XML block via SkillTool.refreshSkills(). This means skills are now presented to the model in priority order rather than alphabetical order. The PR description and docs only mention user-facing display changes (/skills, slash-command completion, /help). If this model-facing behavioral change is intentional, it should be documented; if unintentional, SkillTool should sort independently rather than inheriting listSkills() ordering.
Verdict: No new Critical findings. The implementation is structurally sound with good test coverage (tsc + eslint clean, 4 pre-existing flaky test failures unrelated). The existing inline comments from the prior review cover all actionable items.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
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.
`parsePriorityField` previously hardcoded `debugLogger.warn` from skill-load, so a warning emitted from `SkillManager.parseSkillContent` (project / user / bundled skills) was tagged `[SKILL_LOAD]` instead of `[SKILL_MANAGER]`. Annoying for log filtering and slightly misleading about which parse path actually surfaced the bad priority. Added an optional `warn` callback parameter; the existing extension call site keeps the default skill-load logger, while skill-manager passes its own. Behavior is otherwise unchanged.
Earlier doc said priority sorts "in /skills, slash-command completion, and the /help custom commands view." After the scope-narrowing in 96722aa, priority only affects /skills. Updating the doc to match the actual behavior so readers don't expect cross-surface ordering.
wenshao
approved these changes
May 18, 2026
|
|
||
| // Sort by name for consistent ordering | ||
| skills.sort((a, b) => a.name.localeCompare(b.name)); | ||
| // Sort by priority desc (unspecified treated as 0), then by name. |
Collaborator
There was a problem hiding this comment.
[Suggestion] listSkills() 优先级排序影响范围超出 /skills 列表
listSkills() 现在为所有调用方按优先级排序，但文档声明优先级仅用于 /skills 展示。SkillTool（packages/core/src/tools/skill.ts:112）调用 listSkills() 构建发送给模型的 <available_skills> XML，这意味着模型端工具描述的顺序也受优先级影响。
当前实际影响有限（LLM 不会因 XML 条目位置可靠地偏好某技能），但代码行为与文档不一致，未来可能导致混淆。
两个修复方向：
- 更新文档，承认优先级也影响模型端排序
- 将排序限定在展示层（在
skillsCommand.ts中排序而非listSkills()中）
Suggested change
| // Sort by priority desc (unspecified treated as 0), then by name. | |
| // Consider: either document that priority affects model-facing ordering too, | |
| // or move the priority sort to the display layer (skillsCommand.ts) so | |
| // listSkills() returns pure alphabetical order for all consumers. |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
wenshao
requested changes
May 18, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
The core skill-manager test file fails deterministically. Running cd packages/core && npx vitest run src/skills/skill-manager.test.ts fails because the bundled-skill tests can no longer find the mocked review skill (skills.some((s) => s.name === 'review') is false, and loadSkill('review') returns null). This means the PR currently breaks the affected package test command and should not merge until the test setup or implementation is corrected.
— gpt-5.5 via Qwen Code /review
… display `listSkills()` previously returned priority-desc order for every consumer, including `SkillTool.refreshSkills()` which builds the model-facing `<available_skills>` description. That contradicted the stated design goal (`priority:` controls the `/skills` listing only) and the user docs, which say everything outside `/skills` stays alphabetical. - skill-manager.ts: `listSkills()` now sorts name-asc only, giving all programmatic consumers (SkillTool, contextCommand, loaders) a stable alphabetical order unaffected by `priority:`. - skillsCommand.ts: apply the priority-desc, name-asc sort at the display layer using the shared `normalizeSkillPriority`. - skills/index.ts: export `normalizeSkillPriority` for the CLI display sort. - Tests: core tests now lock in that `listSkills()` stays alphabetical regardless of priority; new skillsCommand.test.ts covers the display sort.
Collaborator
Author
|
Ran the exact command on the current PR head ( The bundled-skill tests resolve the mocked I think this review ran against a checkout before |
wenshao
requested changes
May 19, 2026
| @@ -91,11 +92,13 @@ Qwen Code currently validates that: | |||
|
|
|||
Collaborator
There was a problem hiding this comment.
[Critical] The documentation now says priority only affects the /skills listing and explicitly does not affect slash completion or /help, but the linked issue requests priority ordering for the model-facing <available_skills> list, /skills, /help, and Tab completion, and the PR description still promises the slash-completion mapping. The implementation was changed to keep listSkills() alphabetical and to avoid setting completionPriority, so the documented behavior no longer satisfies the feature this PR is supposed to close.
Either restore priority propagation to the requested surfaces (listSkills() / SkillTool, completionPriority, and /help), or update the PR/issue resolution so it does not claim to close the broader request.
— gpt-5.5 via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Updated the PR description to accurately reflect the current scope. The implementation intentionally scopes priority to the /skills listing only — /help, Tab completion, and model-facing <available_skills> stay alphabetical. This was a deliberate design decision: a high-priority extension skill should not reorder built-in commands in completion/help, and reordering model-facing lists would change model behavior unpredictably.
Changed the PR from "Closes #4136" to "Partially addresses #4136" since only the /skills surface is covered.
LaZzyMan
previously approved these changes
May 19, 2026
LaZzyMan
left a comment
Collaborator
There was a problem hiding this comment.
Review
Small, contained feature that adds an optional priority field to SKILL.md frontmatter and uses it as the primary sort key (priority desc, then name asc) across both the /skills listing and slash-command completion ranking. The parsing is well-defended: strict typeof === 'number' && Number.isFinite rejection with explicit regression tests for the Number(true) === 1 footgun, priority: high (string) silently failing open, and NaN from the YAML parser also rejected. normalizeSkillPriority(undefined) === 0 keeps backward compat clean (skills without priority sort alphabetically among themselves; negatives sort below unset). The doc comment on parsePriorityField is especially good — it names the two-place SKILL.md parsing problem and references the same regression class that previously hit whenToUse and paths. Test coverage extends to the extension-skill bypass path (runtime warning for non-numeric priority), and skillsCommand correctly drops its now-redundant sort with an explicit "preserve listSkills() ordering" comment.
Verdict
APPROVE — defensive parsing, clear documentation, and regression-guard tests for the obvious footguns.
wenshao
requested changes
May 19, 2026
| 'argument-hint': '[topic]', | ||
| }; | ||
| } | ||
| if (/^priority:/m.test(yamlString)) { |
Collaborator
There was a problem hiding this comment.
[Critical] This new mockImplementation condition for priority: causes a regression in the bundled skill tests below.
Running cd packages/core && npx vitest run src/skills/skill-manager.test.ts produces 2 failures:
should load bundled skills in listSkills—skills.some((s) => s.name === 'review')returnsfalseshould fall back to bundled level in loadSkill—skill!.namethrows TypeError (skillisnull)
The base branch passes all 67 tests; this PR branch fails 2 of 135. The setupReviewSkillMocks() helper below calls mockParseYaml.mockReturnValue(...), which likely doesn't fully override the mockImplementation set by the priority-aware tests that run before it in the same describe block.
Suggested fix: Add mockParseYaml.mockReset() at the start of setupReviewSkillMocks() (or use mockReturnValueOnce per-call) to ensure the bundled skill tests get a clean mock state.
— qwen-latest-series-invite-beta-v30 via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Investigated: the test failures are caused by stale compiled .js files in packages/core/src/skills/ (not tracked in git). When present, vitest resolves import from './skill-load.js' to the stale compiled file which lacks parsePriorityField. On a clean checkout (CI), all 72 tests pass.
This is a local environment issue, not a code regression. Running find packages/core/src -name '*.js' -delete resolves it. The CI run on this branch confirms all tests pass.
| * field, so a typo in this single key shouldn't make a previously-working | ||
| * skill silently disappear from the listing. | ||
| */ | ||
| export function parsePriorityField( |
Collaborator
There was a problem hiding this comment.
[Suggestion] parsePriorityField accepts an optional warn callback (used by SkillManager.parseSkillContent to tag warnings as [SKILL_MANAGER] instead of [SKILL_LOAD]), but no test verifies that the custom warn is actually invoked on invalid input.
If the warn(msg) call is accidentally moved above the validation check, or the parameter is dropped during a refactor, the diagnostic signal is silently lost — no test would catch it.
Suggested fix:
it('invokes the custom warn callback on invalid input', () => {
const warn = vi.fn();
parsePriorityField({ priority: true }, filePath, warn);
expect(warn).toHaveBeenCalledOnce();
expect(warn).toHaveBeenCalledWith(
expect.stringContaining('Ignoring invalid priority'),
);
});— qwen-latest-series-invite-beta-v30 via Qwen Code /review
wenshao
approved these changes
May 21, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM! ✅ — gpt-5.5 via Qwen Code /review
DragonnZhang
pushed a commit
that referenced
this pull request
May 21, 2026
…splay order (#4155) * feat(skills): support priority field in SKILL.md for sorting skill display order Closes #4136 * fix(skills): make /skills respect priority and treat unset as 0 - /skills was re-sorting alphabetically after listSkills(), masking the new priority order. Drop the redundant sort and reuse the manager's output directly. - Treat missing priority as 0 instead of -Infinity so an explicit negative priority (e.g. -1) sorts below unset skills, which matches user intent. * fix(skills): harden priority parsing and ordering * fix(skills): warn when extension supplies invalid priority Extension-provided skills bypass parseSkillContent / validateConfig, so a non-number `priority` was silently normalized to 0 in the sort with zero diagnostic. Match the SKILL.md author signal: warn at load time so the extension author can see and fix the typo. Addresses PR #4155 review (the extension-bypass-validation point). * test(skills): direct unit tests for parsePriorityField and normalizeSkillPriority Both helpers are exported but previously had no direct tests — coverage came only via parseSkillContent and listSkills. Adds inputs the integration paths can't surface cleanly: -0 / NaN / Infinity, numeric strings, objects, arrays, and the boolean coercion regression that motivated the strict typecheck. Also adds a NOTE on parsePriorityField warning future contributors that SKILL.md frontmatter parsing lives in two places (parseSkillContent here and SkillManager.parseSkillContent), so any new field must be wired into both — the same regression that previously hit whenToUse, disable-model-invocation, paths, and priority. Full dedup of the two parseSkillContent bodies is left as a follow-up refactor. Addresses the remaining two [Suggestion] items from PR #4155 review. * fix(skills): scope priority to /skills listing only 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. * fix(skills): tag priority warning with calling module's namespace `parsePriorityField` previously hardcoded `debugLogger.warn` from skill-load, so a warning emitted from `SkillManager.parseSkillContent` (project / user / bundled skills) was tagged `[SKILL_LOAD]` instead of `[SKILL_MANAGER]`. Annoying for log filtering and slightly misleading about which parse path actually surfaced the bad priority. Added an optional `warn` callback parameter; the existing extension call site keeps the default skill-load logger, while skill-manager passes its own. Behavior is otherwise unchanged. * docs(skills): correct priority scope description Earlier doc said priority sorts "in /skills, slash-command completion, and the /help custom commands view." After the scope-narrowing in 96722aa, priority only affects /skills. Updating the doc to match the actual behavior so readers don't expect cross-surface ordering. * fix(skills): keep listSkills() alphabetical, sort priority at /skills display `listSkills()` previously returned priority-desc order for every consumer, including `SkillTool.refreshSkills()` which builds the model-facing `<available_skills>` description. That contradicted the stated design goal (`priority:` controls the `/skills` listing only) and the user docs, which say everything outside `/skills` stays alphabetical. - skill-manager.ts: `listSkills()` now sorts name-asc only, giving all programmatic consumers (SkillTool, contextCommand, loaders) a stable alphabetical order unaffected by `priority:`. - skillsCommand.ts: apply the priority-desc, name-asc sort at the display layer using the shared `normalizeSkillPriority`. - skills/index.ts: export `normalizeSkillPriority` for the CLI display sort. - Tests: core tests now lock in that `listSkills()` stays alphabetical regardless of priority; new skillsCommand.test.ts covers the display sort. * fix: correct copyright year 2025 -> 2026 in new file [skip ci]
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add optional
priorityfield in SKILL.md frontmatter to let skill authors control the display order in the/skillslisting.Partially addresses #4136 (scoped to
/skillslisting only — see Design Decision below).Design Decision
The original issue requested priority across 4 surfaces (
<available_skills>,/skills,/help, Tab completion). After review, we intentionally scopedpriorityto the/skillslisting only:/helpand Tab completion stay alphabetical — a high-priority extension skill should not push built-in commands around in these UIs.<available_skills>stays alphabetical — reordering here would change model behavior in unpredictable ways./skillslisting — this is the display surface where priority ordering makes the most sense: users browsing available skills benefit from seeing important ones first.Changes
packages/core/src/skills/types.tspriority?: numbertoSkillConfigpackages/core/src/skills/skill-load.tsparsePriorityField+normalizeSkillPriorityhelperspackages/core/src/skills/skill-manager.tspriorityfrom frontmatter, keeplistSkills()alphabeticalpackages/cli/src/ui/commands/skillsCommand.ts/skillsdisplay by priority desc, then name ascdocs/users/features/skills.mdpriorityfield and its scopeUsage
/skillslisting only/help, Tab completion, and model-facing skill list remain alphabeticalTest plan
/skillsdisplay layercompletionPriorityis NOT set fromskill.priority