feat(skills): support extends: bundled to allow extending bundled skills without full override#2380
Open
feat(skills): support extends: bundled to allow extending bundled skills without full override#2380
extends: bundled to allow extending bundled skills without full override#2380Conversation
… bundled skills (#2379) Add a generic `extends: bundled` mechanism to the skill system, allowing users to append custom content to bundled skills without replacing them entirely. This enables adding custom review dimensions to /review by creating `.qwen/skills/review/SKILL.md` with `extends: bundled`. - Add `extends?: SkillLevel` field to `SkillConfig` type - Parse `extends` from frontmatter in both `skill-load.ts` and `skill-manager.ts` - Add `resolveExtends()` in `SkillManager` that merges base + extending body - Inherit `allowedTools` from base skill when extending skill omits them - Resolve extends in both `loadSkill` and `listSkills` paths - Update `BundledSkillLoader` to resolve skills at invocation time with graceful fallback on errors - Update review SKILL.md to use dynamic agent count wording - Add `extends` validation in `validateConfig` - Add comprehensive tests covering merge, ordering, inheritance, error paths, and resilience Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
wenshao
requested review from
DennisYu07,
DragonnZhang,
LaZzyMan,
Mingholy,
gwinthis,
pomelo-nwu and
tanzhenxin
as code owners
Contributor
📋 Review SummaryThis PR implements a well-designed 🔍 General Feedback
🎯 Specific Feedback🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
…d-level guard Extract duplicated extends field parsing into shared parseExtendsField(), narrow extends type from SkillLevel to 'bundled', add validation that bundled-level skills cannot declare extends, remove dead code in resolveExtends, and clarify BundledSkillLoader resolution flow comments. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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. |
…fy loadSkill, add tests Clear `extends` on fallback in listSkills to prevent leaking unresolved field to consumers, remove misleading type cast in parseExtendsField, extract findFirstSkillByName to simplify loadSkill, and add user-level extends and fallback clearing tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Collaborator
|
Overall this direction makes sense to me, especially for
|
Collaborator
Author
The existing skill system already has a layered precedence model (bundled → extension → user → project) where higher levels fully replace lower ones. extends: bundled simply adds an "append to base" option alongside the existing "replace base" behavior — it's a natural extension of the level hierarchy rather than a new concept. The naming uses the literal SkillLevel value to keep it grounded in existing semantics.
Yes, intentional v1 tradeoff. Each appended agent section is structurally self-contained (own heading, own focus areas), so the coupling between base and extension is loose — upstream prompt refinements flow through automatically, which is a feature rather than a bug for most users. The alternative (pinning to a snapshot) would require versioning and upgrade tooling for a problem that doesn't exist yet. If a future bundled skill change is structural enough to break extensions, we'd handle it through release notes as with any breaking change.
To clarify the current behavior: if the bundled base is missing, resolveExtends() throws, listSkills() catches it with a debug warning, and the skill falls back to the user's own body only. So it degrades gracefully but silently. I agree a user-visible warning here would be better — happy to add that in a follow-up.
Intentionally constrained. The full override path (project-level SKILL.md without extends) still handles subtractive and replacement cases, so users aren't locked into append-only. extends is an additive shortcut for the common case, not the only mechanism. If we see real demand for more granular composition (disable/replace specific sections), we can evolve toward a slot or section-marker model — but I'd rather let concrete use cases drive that design. |
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
2 participants
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.
feat(skills): support
extends: bundledto allow extending bundled skills without full overrideBody:
Summary
extends: bundledmechanism to the skill system, enabling users to append custom content to bundled skills instead of replacing them entirely/reviewwithout forking the full SKILL.mdCloses #2379
Motivation
The bundled
/reviewskill ships with 4 fixed review dimensions. Teams with specific review needs (accessibility, API compatibility, compliance, etc.) previously had to copy the entire SKILL.md to.qwen/skills/review/SKILL.md— losing future updates to the bundled version.extends: bundledsolves this by allowing surgical additions.Usage
Create
.qwen/skills/review/SKILL.md:The
/reviewcommand will now run 5 parallel review agents (4 bundled + 1 custom).Design
extends: bundledis a generic skill system enhancement, not review-specific:SkillConfiggains an optionalextends?: SkillLevelfield (v1: only'bundled'supported)SkillManager.resolveExtends()mergesbaseSkill.body + "\n\n" + extendingSkill.bodyallowedToolsinherited from base when extending skill omits themBundledSkillLoaderresolves at invocation time with graceful fallback on errorsError handling
extends: bundledbut bundled skill not foundSkillErrorthrown, BundledSkillLoader falls back to bundled versionextends: project(unsupported value)loadSkillthrows during invocationlistSkillsresolve failsextendsfield (existing behavior)Test plan
npx vitest run packages/core/src/skills/skill-load.test.ts— 21 tests (parsing, validation)npx vitest run packages/core/src/skills/skill-manager.test.ts— 48 tests (resolve, merge order, inheritance, error paths)npx vitest run packages/cli/src/services/BundledSkillLoader.test.ts— 10 tests (invocation-time resolve, fallback).qwen/skills/review/SKILL.mdwithextends: bundledand verify/reviewincludes custom dimensionsTLDR
Dive Deeper
Reviewer Test Plan
Testing Matrix
Linked issues / bugs