fix(skill): scope-group the skills system prompt and honor project overrides#2044
Conversation
…errides Group discovered skills under Project / User / Extra / Built-in headings in the system prompt and reverse the resolution priority so project-scope skills correctly override user- and bundled ones on name conflict. Also accept flat <name>.md skills, add extra_skill_dirs config, and harden OSError handling in discovery.
There was a problem hiding this comment.
Pull request overview
This PR improves the skills system so the model can clearly distinguish skill origin scopes in the system prompt and so project-local skills properly override user/built-in skills during discovery/resolution.
Changes:
- Introduces explicit skill scopes (
project/user/extra/builtin) and renders the system prompt skills block grouped under scope headings with documented precedence. - Reorders layered discovery so precedence becomes Project > User > Extra > Built-in, and adds additive
extra_skill_dirsconfiguration support. - Extends discovery to support flat
<name>.mdskills, unifying description fallback logic and hardening discovery againstOSError.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/kimi_cli/skill/__init__.py |
Adds scope-aware root resolution, scoped skill discovery, flat .md support, and grouped prompt formatting. |
src/kimi_cli/soul/agent.py |
Switches agent system-prompt skill rendering to the new grouped formatter and passes extra_skill_dirs. |
src/kimi_cli/utils/path.py |
Adds shared find_project_root() used for resolving relative extra skill dirs (and AGENTS discovery). |
src/kimi_cli/utils/frontmatter.py |
Adds strip_frontmatter() helper reused by skill description fallback. |
src/kimi_cli/config.py |
Adds extra_skill_dirs to config schema. |
src/kimi_cli/agents/default/system.md |
Updates system prompt instructions to explain scope grouping and precedence. |
tests/core/test_skills_prompt.py |
New tests validating scope-grouped skills prompt rendering and end-to-end override behavior. |
tests/core/test_skill.py |
Updates/extends discovery tests for scoped roots, precedence, flat .md skills, extra dirs, and resilience. |
tests/core/test_plugin_manager.py |
Updates to new scoped-roots return type from resolve_skills_roots(). |
tests/core/test_kimisoul_slash_commands.py |
Updates Skill construction to include required scope and skill_md_file. |
tests/core/test_default_agent.py |
Updates expected default agent system prompt text about skill scope/precedence. |
tests/core/test_config.py |
Updates default config dump snapshot to include extra_skill_dirs. |
docs/en/customization/skills.md |
Documents new precedence, additive extra_skill_dirs, and flat .md skills. |
docs/zh/customization/skills.md |
Chinese documentation updates mirroring the new behavior and config. |
CHANGELOG.md |
Adds Unreleased entries describing the skill discovery/prompt changes. |
docs/en/release-notes/changelog.md |
Syncs Unreleased changelog entries into the docs changelog page. |
docs/zh/release-notes/changelog.md |
Adds translated Unreleased changelog entries for the same feature set. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """ | ||
| from kimi_cli.utils.frontmatter import strip_frontmatter | ||
|
|
||
| body = strip_frontmatter(content) | ||
| for line in body.splitlines(): | ||
| stripped = line.strip() | ||
| if stripped: | ||
| return stripped |
There was a problem hiding this comment.
_first_meaningful_line() can return '---' as the description fallback when a skill file starts with a malformed YAML frontmatter opener (--- without a closing ---): parse_frontmatter() returns None, strip_frontmatter() returns the text unchanged, and the first non-empty line is the marker. Consider skipping standalone --- marker lines when deriving the body-first-line description (or change strip_frontmatter() to drop the opener even for malformed blocks).
| """ | |
| from kimi_cli.utils.frontmatter import strip_frontmatter | |
| body = strip_frontmatter(content) | |
| for line in body.splitlines(): | |
| stripped = line.strip() | |
| if stripped: | |
| return stripped | |
| Standalone frontmatter delimiter lines are ignored so malformed YAML | |
| openers do not become fallback descriptions. | |
| """ | |
| from kimi_cli.utils.frontmatter import strip_frontmatter | |
| body = strip_frontmatter(content) | |
| for line in body.splitlines(): | |
| stripped = line.strip() | |
| if not stripped or stripped == "---": | |
| continue | |
| return stripped |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71bbef43f6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| canon = root.canonical() | ||
| except OSError: |
There was a problem hiding this comment.
The dedup key in _append is built from root.canonical(), but KaosPath.canonical() only normalizes path segments and does not resolve symlinks, so two entries that point to the same directory via a real path and a symlink are still treated as distinct roots. In that scenario, the same skill directory is scanned multiple times and the new "symlink dedup" behavior documented in this change is not actually enforced. Use a backend-aware realpath/symlink-resolution step for the dedup key instead of plain canonical().
Useful? React with 👍 / 👎.
| p = Path(raw).expanduser() | ||
| except (RuntimeError, OSError): |
There was a problem hiding this comment.
_resolve_extra_skill_dir expands ~ using pathlib.Path.expanduser(), which uses the local process home, while the rest of skill discovery is KAOS-path based. On non-local backends (for example, SSH where remote $HOME differs), a config entry like ~/skills resolves to the wrong location and is skipped, so extra_skill_dirs silently fails for that setup. This should use KAOS path expansion (e.g., KaosPath(...).expanduser()) before resolving relative paths.
Useful? React with 👍 / 👎.
Signed-off-by: Kai <me@kaiyi.cool>
…-scope skills Previously find_project_skills_dirs looked only under the work directory, which meant that running kimi-cli from a subdirectory (e.g. inside a monorepo package) missed project-scope skills defined at the repository root. Walk up to the nearest .git ancestor first; fall back to the work directory itself when no .git marker is found.
Users who keep skills in multiple brand directories (for example both .kimi/skills and .claude/skills) previously lost visibility of everything outside the first existing brand directory, because the default was to "use only the first one found". Flip the default so all existing brand directories are merged. Users who want the old first-match behavior can opt back in via config. This is a behavior change; see CHANGELOG.md.
The `mis-group` idiom in a defence-in-depth comment gets split into two tokens by the typos spellchecker, flagging the bare "mis" as a typo for "miss". Reword to keep the intent without tripping CI.
…allback
When a skill file starts with a malformed YAML frontmatter opener ("---"
without a closing "---"), parse_frontmatter returns None and
strip_frontmatter leaves the content unchanged, so the first meaningful
line used for the description fallback was the opener itself. Skip
standalone "---" delimiter lines in _first_meaningful_line so malformed
openers do not become fallback descriptions.
KaosPath.canonical() only normalizes path segments and explicitly does not resolve symlinks, so an extra_skill_dirs entry pointing at the real directory and another pointing at a symlink of the same directory were treated as distinct roots. The PR's advertised "symlink dedup" was not actually enforced, and the test meant to lock the behavior only asserted how many roots match real.resolve(), not how many roots total — so the symlinked root was silently accepted. On local backends, walk symlinks via pathlib.Path.resolve() before building the dedup key and storing the canonicalized root. Non-local backends (where resolve() would walk the wrong filesystem) keep the canonical-only behavior. Also tighten the dedup test to assert len(roots) == 1 and that the surviving root is the real target.
Signed-off-by: Kai <me@kaiyi.cool>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6079c8696
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try: | ||
| local_resolved = Path(str(root)).resolve() | ||
| except OSError: |
There was a problem hiding this comment.
_append resolves local roots via Path(str(root)).resolve(), but pathlib raises RuntimeError (not OSError) for symlink loops. In that case—e.g., a user passes --skills-dir that points into a cyclic symlink—the exception escapes this block and aborts startup instead of skipping the bad entry like other discovery errors. Catching RuntimeError here (or broadening the guard) keeps one malformed path from taking down skill discovery.
Useful? React with 👍 / 👎.
Problem
Skills discovered under a project directory (e.g.
<project>/.kimi/skillsor<project>/.claude/skills) were effectively invisible to the model's reasoning about "the skill in this project":/Users/x/.claude/...vs/Users/x/git/proj/.claude/...), which made it easy to conflate project and user scopes across turns.builtin → user → projectand the resolver used "first wins", so built-in and user-level skills silently shadowed project-level ones — the opposite of what users expect and the opposite of what every comparable tool does.--skills-dirflag is an override, not an addition).Together these meant that a user placing a skill at
<project>/.claude/skills/foo/SKILL.mdcould not get the model to treat it as the authoritativefoo, even after multiple explicit corrections.Changes
Skillnow carries a requiredscope(builtin/user/project/extra). The system prompt renders skills under### Project/### User/### Extra/### Built-inheadings (empty groups omitted). Thesystem.mdblock now explicitly teaches the model the precedence rule.resolve_skills_rootsnow appends project → user → extra(config) → extra(plugins) → builtin, so the existing "first wins" dedup indiscover_skills_from_rootstranslates to Project overrides User overrides Extra overrides Built-in. This matches the documented behaviour and the system-prompt wording..mdskills.<skills_dir>/<name>.mdfiles are now recognised as skills (name = filename stem unless frontmatter setsname:). Subdirectory layout (<name>/SKILL.md) still wins on name conflict inside the same directory. Description resolution follows a unified three-step chain across both forms: frontmatterdescription:→ first non-empty body line (capped at 240 characters) →"No description provided.".extra_skill_dirsconfig. A newextra_skill_dirs: list[str]field adds directories on top of auto-discovery (additive, not override). Entries accept absolute paths,~-prefixed paths (expanded against$HOME), and paths relative to the project root (the nearest.gitancestor of the work directory, not the CWD). Non-existent entries are silently skipped; symlink and trailing-slash duplicates canonicalize to one.OSErrorfromis_dir/iterdir(e.g. a permission-denied extra dir) is now logged and skipped instead of aborting the whole discovery pass.Internal API changes (not breaking for external users)
No external callers;
src/andtests/are fully migrated in this PR.Skill.scope: SkillScopeis required (no default).Skill.skill_md_file: KaosPathis a required data field (was a computed@property).resolve_skills_roots(...)anddiscover_skills_from_roots(...)now return / acceptlist[ScopedSkillsRoot](waslist[KaosPath]). The old flat-list variants were removed since there were zero production callers.Testing
2580 passed, 8 skipped, 1 xfailed(0 regressions).ruff check/ruff format --check/pyright: all clean.test_end_to_end_project_override_renders_correctly— 4 scopes definefoo; the rendered system prompt showsfooexactly once, under### Project, with the project path/description, and no "builtin/user/extra version" string leaks through..mdform: basic discovery, name-from-stem, frontmatter-name-wins-over-stem, frontmatter-description-wins-over-body, body fallback + 240-char truncation, subdir-shadows-flat-on-conflict, bareSKILL.mdignored.extra_skill_dirs: append semantics,~expansion, project-root relative resolution (not CWD), absolute paths, missing-path silent skip, symlink dedup, trailing-slash dedup, auto-discovery overlap dedup, CLI override + extra coexistence, CLI wins over extra on name conflict.Skill(...)withoutscoperaisespydantic.ValidationError.iterdirraisingPermissionErrorreturns[]instead of crashing.Files changed
17 files: 3 production modules (
src/kimi_cli/skill/__init__.py,src/kimi_cli/soul/agent.py,src/kimi_cli/utils/{path,frontmatter}.py), 1 config schema, 1 default system prompt, 2 docs (en/zh), 2 changelogs (source + en), 1 zh changelog, 5 test files (1 new). ≈ +1534 / -136.