From d9e030932db9215e5cfca3a68c845cc83a79f391 Mon Sep 17 00:00:00 2001 From: Tempura <4588539+tempurai@users.noreply.github.com> Date: Fri, 27 Mar 2026 16:34:22 +0800 Subject: [PATCH] =?UTF-8?q?Revert=20"feat(skills):=20--skills-dir=20suppor?= =?UTF-8?q?ts=20multi-path=20append=20instead=20of=20over=E2=80=A6"?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit a61f26a1cdf4d0321bb6b692f65de7493c70d16e. --- src/kimi_cli/app.py | 15 ++---- src/kimi_cli/cli/__init__.py | 12 ++--- src/kimi_cli/skill/__init__.py | 28 +++++------ src/kimi_cli/soul/agent.py | 7 +-- tests/core/test_skill.py | 81 ++++--------------------------- tests_e2e/test_wire_skills_mcp.py | 4 +- tests_e2e/wire_helpers.py | 6 +-- 7 files changed, 39 insertions(+), 114 deletions(-) diff --git a/src/kimi_cli/app.py b/src/kimi_cli/app.py index 8dfa0f489..52ba0b2bd 100644 --- a/src/kimi_cli/app.py +++ b/src/kimi_cli/app.py @@ -84,7 +84,7 @@ async def create( # Extensions agent_file: Path | None = None, mcp_configs: list[MCPConfig] | list[dict[str, Any]] | None = None, - extra_skills_dirs: list[KaosPath] | None = None, + skills_dir: KaosPath | None = None, # Loop control max_steps_per_turn: int | None = None, max_retries_per_step: int | None = None, @@ -105,8 +105,8 @@ async def create( agent_file (Path | None, optional): Path to the agent file. Defaults to None. mcp_configs (list[MCPConfig | dict[str, Any]] | None, optional): MCP configs to load MCP tools from. Defaults to None. - extra_skills_dirs (list[KaosPath] | None, optional): Additional skills directories - appended to default discovery. Defaults to None. + skills_dir (KaosPath | None, optional): Override skills directory discovery. Defaults + to None. max_steps_per_turn (int | None, optional): Maximum number of steps in one turn. Defaults to None. max_retries_per_step (int | None, optional): Maximum number of retries in one step. @@ -186,14 +186,7 @@ async def create( if startup_progress is not None: startup_progress("Scanning workspace...") - runtime = await Runtime.create( - config, - oauth, - llm, - session, - yolo, - extra_skills_dirs=extra_skills_dirs, - ) + runtime = await Runtime.create(config, oauth, llm, session, yolo, skills_dir_override=skills_dir) runtime.notifications.recover() runtime.background_tasks.reconcile() _cleanup_stale_foreground_subagents(runtime) diff --git a/src/kimi_cli/cli/__init__.py b/src/kimi_cli/cli/__init__.py index d28e2a57b..7b29aff8c 100644 --- a/src/kimi_cli/cli/__init__.py +++ b/src/kimi_cli/cli/__init__.py @@ -280,14 +280,14 @@ def kimi( ), ] = None, local_skills_dir: Annotated[ - list[Path] | None, + Path | None, typer.Option( "--skills-dir", exists=True, 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.", ), ] = None, # Loop control @@ -475,9 +475,9 @@ def _emit_fatal_error(message: str) -> None: except json.JSONDecodeError as e: raise typer.BadParameter(f"Invalid JSON: {e}", param_hint="--mcp-config") from e - extra_skills_dirs: list[KaosPath] | None = None - if local_skills_dir: - extra_skills_dirs = [KaosPath.unsafe_from_local_path(p) for p in local_skills_dir] + skills_dir: KaosPath | None = None + if local_skills_dir is not None: + skills_dir = KaosPath.unsafe_from_local_path(local_skills_dir) work_dir = KaosPath.unsafe_from_local_path(local_work_dir) if local_work_dir else KaosPath.cwd() @@ -552,7 +552,7 @@ async def _run(session_id: str | None) -> tuple[Session, int]: yolo=yolo or (ui == "print"), # print mode implies yolo agent_file=agent_file, mcp_configs=mcp_configs, - extra_skills_dirs=extra_skills_dirs, + skills_dir=skills_dir, max_steps_per_turn=max_steps_per_turn, max_retries_per_step=max_retries_per_step, max_ralph_iterations=max_ralph_iterations, diff --git a/src/kimi_cli/skill/__init__.py b/src/kimi_cli/skill/__init__.py index 942e9b374..26584a5d3 100644 --- a/src/kimi_cli/skill/__init__.py +++ b/src/kimi_cli/skill/__init__.py @@ -2,7 +2,7 @@ from __future__ import annotations -from collections.abc import Callable, Iterable, Iterator, Sequence +from collections.abc import Callable, Iterable, Iterator from pathlib import Path from typing import Literal @@ -85,29 +85,27 @@ async def find_project_skills_dir(work_dir: KaosPath) -> KaosPath | None: async def resolve_skills_roots( work_dir: KaosPath, *, - extra_skills_dirs: Sequence[KaosPath] | None = None, + skills_dir_override: KaosPath | None = None, ) -> list[KaosPath]: """ 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. """ from kimi_cli.plugin.manager import get_plugins_dir roots: list[KaosPath] = [] if _supports_builtin_skills(): roots.append(KaosPath.unsafe_from_local_path(get_builtin_skills_dir())) - if user_dir := await find_user_skills_dir(): - roots.append(user_dir) - if project_dir := await find_project_skills_dir(work_dir): - roots.append(project_dir) - if extra_skills_dirs: - roots.extend(extra_skills_dirs) - # Plugins are always discoverable + if skills_dir_override is not None: + roots.append(skills_dir_override) + else: + if user_dir := await find_user_skills_dir(): + roots.append(user_dir) + if project_dir := await find_project_skills_dir(work_dir): + roots.append(project_dir) + # Plugins are always discoverable, even when --skills-dir is set plugins_path = get_plugins_dir() if plugins_path.is_dir(): roots.append(KaosPath.unsafe_from_local_path(plugins_path)) @@ -131,7 +129,7 @@ async def discover_skills_from_roots(skills_dirs: Iterable[KaosPath]) -> list[Sk skills_by_name: dict[str, Skill] = {} for skills_dir in skills_dirs: for skill in await discover_skills(skills_dir): - skills_by_name.setdefault(normalize_skill_name(skill.name), skill) + skills_by_name[normalize_skill_name(skill.name)] = skill return sorted(skills_by_name.values(), key=lambda s: s.name) diff --git a/src/kimi_cli/soul/agent.py b/src/kimi_cli/soul/agent.py index 0c0d8ea4f..9bc13d4c5 100644 --- a/src/kimi_cli/soul/agent.py +++ b/src/kimi_cli/soul/agent.py @@ -116,7 +116,7 @@ async def create( llm: LLM | None, session: Session, yolo: bool, - extra_skills_dirs: list[KaosPath] | None = None, + skills_dir_override: KaosPath | None = None, ) -> Runtime: ls_output, agents_md, environment = await asyncio.gather( list_directory(session.work_dir), @@ -125,10 +125,7 @@ async def create( ) # Discover and format skills - skills_roots = await resolve_skills_roots( - session.work_dir, - extra_skills_dirs=extra_skills_dirs, - ) + skills_roots = await resolve_skills_roots(session.work_dir, skills_dir_override=skills_dir_override) # Canonicalize so symlinked skill directories match resolved paths skills_roots_canonical = [r.canonical() for r in skills_roots] skills = await discover_skills_from_roots(skills_roots) diff --git a/tests/core/test_skill.py b/tests/core/test_skill.py index 866bca023..4808fe00b 100644 --- a/tests/core/test_skill.py +++ b/tests/core/test_skill.py @@ -117,7 +117,7 @@ async def test_discover_skills_flow_parse_failure_falls_back(tmp_path): @pytest.mark.asyncio -async def test_discover_skills_from_roots_prefers_earlier_dirs(tmp_path): +async def test_discover_skills_from_roots_prefers_later_dirs(tmp_path): root = tmp_path / "root" system_dir = root / "system" user_dir = root / "user" @@ -157,9 +157,9 @@ async def test_discover_skills_from_roots_prefers_earlier_dirs(tmp_path): [ Skill( name="shared", - description="System version", + description="User version", type="standard", - dir=KaosPath.unsafe_from_local_path(Path("/path/to/system/shared")), + dir=KaosPath.unsafe_from_local_path(Path("/path/to/user/shared")), flow=None, ) ] @@ -190,83 +190,20 @@ async def test_resolve_skills_roots_uses_layers(monkeypatch, tmp_path): @pytest.mark.asyncio -async def test_resolve_skills_roots_appends_extra_dirs(tmp_path, monkeypatch): - """Extra dirs are appended after user/project, not replacing them.""" - home_dir = tmp_path / "home" - user_dir = home_dir / ".config" / "agents" / "skills" - user_dir.mkdir(parents=True) - monkeypatch.setattr(Path, "home", lambda: home_dir) - +async def test_resolve_skills_roots_respects_override(tmp_path, monkeypatch): work_dir = tmp_path / "project" - project_dir = work_dir / ".agents" / "skills" - project_dir.mkdir(parents=True) - - extra_a = tmp_path / "extra_a" - extra_a.mkdir() - extra_b = tmp_path / "extra_b" - extra_b.mkdir() + override_dir = tmp_path / "override" + override_dir.mkdir() + # Redirect share dir to tmp so ~/.kimi/plugins/ doesn't interfere monkeypatch.setenv("KIMI_SHARE_DIR", str(tmp_path / "share")) roots = await resolve_skills_roots( KaosPath.unsafe_from_local_path(work_dir), - extra_skills_dirs=[ - KaosPath.unsafe_from_local_path(extra_a), - KaosPath.unsafe_from_local_path(extra_b), - ], + skills_dir_override=KaosPath.unsafe_from_local_path(override_dir), ) assert roots == [ KaosPath.unsafe_from_local_path(get_builtin_skills_dir()), - KaosPath.unsafe_from_local_path(user_dir), - KaosPath.unsafe_from_local_path(project_dir), - KaosPath.unsafe_from_local_path(extra_a), - KaosPath.unsafe_from_local_path(extra_b), + KaosPath.unsafe_from_local_path(override_dir), ] - - -@pytest.mark.asyncio -async def test_resolve_skills_roots_empty_extra_dirs(tmp_path, monkeypatch): - """Empty extra_skills_dirs behaves same as None.""" - monkeypatch.setenv("KIMI_SHARE_DIR", str(tmp_path / "share")) - - roots_none = await resolve_skills_roots( - KaosPath.unsafe_from_local_path(tmp_path), - extra_skills_dirs=None, - ) - roots_empty = await resolve_skills_roots( - KaosPath.unsafe_from_local_path(tmp_path), - extra_skills_dirs=[], - ) - - assert roots_none == roots_empty - - -@pytest.mark.asyncio -async def test_discover_skills_from_roots_first_wins(tmp_path): - """When the same skill name appears in multiple roots, the first root wins.""" - # Root A has skill "greet" with description "A" - root_a = tmp_path / "root_a" / "greet" - root_a.mkdir(parents=True) - (root_a / "SKILL.md").write_text( - "---\nname: greet\ndescription: A\n---\nHello from A", - encoding="utf-8", - ) - - # Root B has skill "greet" with description "B" - root_b = tmp_path / "root_b" / "greet" - root_b.mkdir(parents=True) - (root_b / "SKILL.md").write_text( - "---\nname: greet\ndescription: B\n---\nHello from B", - encoding="utf-8", - ) - - skills = await discover_skills_from_roots( - [ - KaosPath.unsafe_from_local_path(tmp_path / "root_a"), - KaosPath.unsafe_from_local_path(tmp_path / "root_b"), - ] - ) - - assert len(skills) == 1 - assert skills[0].description == "A" diff --git a/tests_e2e/test_wire_skills_mcp.py b/tests_e2e/test_wire_skills_mcp.py index 88b49376d..2b4d3afce 100644 --- a/tests_e2e/test_wire_skills_mcp.py +++ b/tests_e2e/test_wire_skills_mcp.py @@ -78,7 +78,7 @@ def test_skill_prompt_injects_skill_text(tmp_path) -> None: config_text=None, work_dir=work_dir, home_dir=home_dir, - skills_dirs=[skill_dir], + skills_dir=skill_dir, extra_args=["--session", session_id], yolo=True, ) @@ -165,7 +165,7 @@ def test_flow_skill(tmp_path) -> None: config_text=None, work_dir=work_dir, home_dir=home_dir, - skills_dirs=[skill_dir], + skills_dir=skill_dir, yolo=True, ) try: diff --git a/tests_e2e/wire_helpers.py b/tests_e2e/wire_helpers.py index 2b14695d7..51ef94e9e 100644 --- a/tests_e2e/wire_helpers.py +++ b/tests_e2e/wire_helpers.py @@ -254,7 +254,7 @@ def start_wire( extra_args: list[str] | None = None, yolo: bool = False, mcp_config_path: Path | None = None, - skills_dirs: list[Path] | None = None, + skills_dir: Path | None = None, agent_file: Path | None = None, ) -> WireProcess: cmd = _wire_base_command() @@ -266,8 +266,8 @@ def start_wire( cmd.extend(["--config", config_text]) if mcp_config_path is not None: cmd.extend(["--mcp-config-file", str(mcp_config_path)]) - for sd in skills_dirs or []: - cmd.extend(["--skills-dir", str(sd)]) + if skills_dir is not None: + cmd.extend(["--skills-dir", str(skills_dir)]) if agent_file is not None: cmd.extend(["--agent-file", str(agent_file)]) if extra_args: