revert(skills): --skills-dir supports multi-path append instead of override#1606
revert(skills): --skills-dir supports multi-path append instead of override#1606
Conversation
tempurai
changed the title
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR reverts the earlier change that made --skills-dir support multiple paths appended to the default skills discovery chain, restoring the single-path override behavior across CLI, runtime wiring, and tests.
Changes:
- Change
--skills-dirback to a single directory and treat it as an override (skipping user/project discovery). - Update runtime wiring (
KimiCLI/Runtime) and e2e helpers/tests to pass a singleskills_dir. - Adjust skill discovery precedence so later roots override earlier ones, and update/remove tests accordingly.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
tests_e2e/wire_helpers.py |
Updates wire test harness to accept a single skills_dir and emit one --skills-dir flag. |
tests_e2e/test_wire_skills_mcp.py |
Updates e2e tests to pass skills_dir (singular). |
tests/core/test_skill.py |
Updates skill discovery/root-resolution tests to match override behavior and later-root precedence. |
src/kimi_cli/soul/agent.py |
Threads skills_dir override into skill root resolution during runtime creation. |
src/kimi_cli/skill/__init__.py |
Reverts root-resolution API to skills_dir_override and switches dedup semantics to later-root-wins. |
src/kimi_cli/cli/__init__.py |
Reverts CLI option type to a single Path and maps it to a single KaosPath override. |
src/kimi_cli/app.py |
Wires the skills_dir override through KimiCLI.create() into Runtime.create(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Resolve layered skill roots in priority order. | ||
|
|
||
| Built-in skills load first when supported by the active KAOS backend, | ||
| followed by user/project discovery, then any extra directories supplied | ||
| via ``--skills-dir``, and finally plugin directories. Extra directories | ||
| are **appended** to the default discovery chain — they never replace | ||
| user or project skills. | ||
| Built-in skills load first when supported by the active KAOS backend. When an | ||
| override is provided, user/project discovery is skipped. | ||
| """ |
There was a problem hiding this comment.
resolve_skills_roots() says it returns roots "in priority order", but the effective precedence is determined by discover_skills_from_roots() (which now makes later roots override earlier ones). Please clarify this docstring to explicitly state that later roots take precedence when the same skill name exists in multiple roots, and how plugins/builtin fit into that ordering.
| file_okay=False, | ||
| dir_okay=True, | ||
| readable=True, | ||
| help="Additional skills directories (repeatable). Appended to default discovery.", | ||
| help="Path to the skills directory. Overrides discovery.", | ||
| ), |
There was a problem hiding this comment.
The --skills-dir option is now a single Path and described as overriding discovery, but existing user docs/reference pages describe it as repeatable and appended. Please update the docs (and any CLI reference tables) to match this reverted behavior, or consider keeping it repeatable to avoid a user-visible breaking change.
| dir_okay=True, | ||
| readable=True, | ||
| help="Additional skills directories (repeatable). Appended to default discovery.", | ||
| help="Path to the skills directory. Overrides discovery.", |
There was a problem hiding this comment.
The help text says "Overrides discovery", but resolve_skills_roots() still includes built-in skills (when supported) and always appends the plugins directory even when an override is provided. Consider clarifying the option help (e.g., overrides user/project discovery only) to avoid confusing users.
| help="Path to the skills directory. Overrides discovery.", | |
| help=( | |
| "Path to the skills directory. Overrides user/project discovery; " | |
| "built-in and plugin skills may still be used." | |
| ), |
src/kimi_cli/app.py
Outdated
| yolo, | ||
| extra_skills_dirs=extra_skills_dirs, | ||
| ) | ||
| runtime = await Runtime.create(config, oauth, llm, session, yolo, skills_dir) |
There was a problem hiding this comment.
Runtime.create(...) is being called with skills_dir as a positional argument. Since this is an optional parameter and the function signature has recently changed, using a keyword argument here would be more robust against future signature edits and improves readability.
| runtime = await Runtime.create(config, oauth, llm, session, yolo, skills_dir) | |
| runtime = await Runtime.create(config, oauth, llm, session, yolo, skills_dir=skills_dir) |
… of over…" This reverts commit a61f26a.
tempurai
force-pushed
the
revert-1578-fix/skills-dir-multi-path
branch
from
cbc41bc to
d9e0309
Compare
Reverts #1578