diff --git a/src/plyngent/cli/app.py b/src/plyngent/cli/app.py index 1c11819..a8aa5d2 100644 --- a/src/plyngent/cli/app.py +++ b/src/plyngent/cli/app.py @@ -387,6 +387,9 @@ async def _run_chat( # noqa: C901, PLR0912, PLR0915 — chat orchestration if isinstance(state_obj, ReplState): state_obj.instance_state.workspace.policy_confirm_hook = None state_obj.instance_state.workspace.policy_allowed_commands.clear() + from plyngent.tools.skills import set_skill_write_policy + + set_skill_write_policy(None, instance=state_obj.instance_state) from plyngent.tools.net import ( clear_private_grants, clear_ssrf_assume_public_cidrs, diff --git a/src/plyngent/cli/limits.py b/src/plyngent/cli/limits.py index bc7548a..84a74ce 100644 --- a/src/plyngent/cli/limits.py +++ b/src/plyngent/cli/limits.py @@ -21,6 +21,7 @@ from plyngent.prompting import ( ) from plyngent.tools.access import AccessDecision from plyngent.tools.process.pty_session import PtyManager +from plyngent.tools.skills import SkillWriteDecision from plyngent.tools.workspace import AccessMode, parse_access_mode if TYPE_CHECKING: @@ -347,6 +348,54 @@ def prompt_policy_fetch_confirm( return False +def prompt_skill_write_confirm( + skill_dir: Path, + action: str, + detail: str, + timeout_seconds: float, +) -> SkillWriteDecision: + """Ask whether to write into *skill_dir* (timed; default deny). + + ``o``/``y`` allows this write; ``s`` allows this skill directory for the rest + of the chat; anything else (or a timeout, cancel, or non-interactive stdin) + denies. Independent of YOLO: a skill directory is not a path grant, so the + human is asked even with ``--yes`` unless ``[skills].allow_write`` is set. + """ + lines = [ + f"action: {action} skill {skill_dir.name!r}", + f"path: {skill_dir}", + f"change: {detail}", + f"(timeout {timeout_seconds:g}s defaults to DENY; not skipped by YOLO)", + ] + try: + with pause_task_cancel_for_prompt(): + backend = get_prompt_backend() + if not backend.is_interactive(): + return SkillWriteDecision(allow=False) + backend.echo() + backend.secho(format_tool_confirm_box(f"skills {action}", "\n".join(lines)), fg="yellow") + backend.echo() + prompt = ( + f"[skills] allow {action} in {skill_dir.name!r}? [o]nce/[s]ession/[n]o (timeout {timeout_seconds:g}s): " + ) + with contextlib.suppress(OSError): + _ = sys.stderr.write(prompt) + _ = sys.stderr.flush() + raw = _read_yes_no_line_with_timeout(timeout_seconds) + if raw is None: + with contextlib.suppress(OSError, NonInteractiveError): + backend.secho(f"[skills] timed out after {timeout_seconds:g}s — denied", fg="red", err=True) + return SkillWriteDecision(allow=False) + token = raw.strip().lower() + if token in {"s", "session"}: + return SkillWriteDecision(allow=True, session=True) + if token in {"o", "once", "y", "yes"}: + return SkillWriteDecision(allow=True) + return SkillWriteDecision(allow=False) + except NonInteractiveError, KeyboardInterrupt, EOFError: + return SkillWriteDecision(allow=False) + + def prompt_directory_access_confirm( path: Path, mode: AccessMode, diff --git a/src/plyngent/cli/slash.py b/src/plyngent/cli/slash.py index b608085..8c7ff64 100644 --- a/src/plyngent/cli/slash.py +++ b/src/plyngent/cli/slash.py @@ -590,6 +590,7 @@ def status_cmd(state: ReplState) -> None: f"rounds={last_rounds}\n" f"usage_session={session_u.format_line(billed=True)}\n" f"grants={len(access_grant_rows(instance=state.instance_state, session=state.session_state))} " + f"skills={len(state.skills.skills) if state.skills is not None else 'off'} " f"workspace={state.workspace}" ) @@ -1250,6 +1251,54 @@ def mcp_slash_cmd(state: ReplState, action: str | None) -> None: _print_mcp_status(state) +@slash.command("skills") +@click.argument("action", required=False, type=click.Choice(["list", "search", "read", "reload"])) +@click.argument("target", required=False) +@click.option("--file", "file", default="", help="File inside the skill (with ``read``).") +@click.pass_obj +def skills_slash_cmd(state: ReplState, action: str | None, target: str | None, file: str) -> None: + """List skills, search or read them, or rescan the roots. + + ``/skills`` or ``/skills list`` — discovered skills with their source root. + ``/skills search PATTERN`` — regex over every skill's contents. + ``/skills read NAME [--file F]`` — print a skill's SKILL.md (or one file). + ``/skills reload`` — rescan the roots after an external edit. + """ + from plyngent.tools.context import bind_tool_context + from plyngent.tools.skills import SKILL_TOOL_NAMES, format_skill_table, skill_read, skill_search + + if state.skills is None: + click.echo("skills: disabled (set [skills].enabled = true)") + return + act = (action or "list").lower() + if act == "reload": + state.reload_skills() + _slash_rebuild_tools_if_on(state) + click.echo(f"skills reloaded: {len(state.skills.skills)} visible; tools rebuilt") + return + # The tool handlers read the bound instance (the registry binds it for a + # model call; a slash command has to do it itself). + with bind_tool_context(instance=state.instance_state, session=state.session_state): + if act == "search": + if not target: + msg = "/skills search requires a pattern" + raise click.UsageError(msg) + click.echo(_await(skill_search.handler(target))) + return + if act == "read": + if not target: + msg = "/skills read requires a skill name" + raise click.UsageError(msg) + click.echo(_await(skill_read.handler(target, file or "SKILL.md"))) + return + click.echo(format_skill_table(state.skills)) + if not state.tools_enabled: + click.secho( + f"note: tools are off, so {', '.join(sorted(SKILL_TOOL_NAMES))} are not offered to the model", + fg="bright_black", + ) + + @slash.command("grants") @click.argument("action", required=False, type=click.Choice(["list", "revoke"])) @click.argument("target", required=False) diff --git a/src/plyngent/cli/state.py b/src/plyngent/cli/state.py index 16327f8..2b043de 100644 --- a/src/plyngent/cli/state.py +++ b/src/plyngent/cli/state.py @@ -35,6 +35,7 @@ if TYPE_CHECKING: from plyngent.memory import MemoryStore from plyngent.memory.database.schema import Session as SessionRow from plyngent.runtime.mcp_client import McpManager + from plyngent.skills import SkillStore from plyngent.tools.access import AccessDecision from plyngent.tools.workspace import AccessMode @@ -88,6 +89,8 @@ class ReplState: session_state: SessionState = field(default_factory=SessionState) # MCP connections started by the host app before this state was built. mcp_manager: McpManager | None = None + # Discovered skills (None when ``[skills].enabled`` is false). + skills: SkillStore | None = field(default=None, init=False, repr=False) _todo_persist_tasks: set[object] = field(default_factory=set, init=False, repr=False) # Session ids for Tab complete (updated when listing/creating/resuming). _session_id_cache: list[int] = field(default_factory=list, init=False, repr=False) @@ -103,6 +106,7 @@ class ReplState: self.instance_state.workspace_root = self.workspace self.instance_state.workspace.root = self.workspace self._install_config_access() + self._install_skills() from plyngent.cli.limits import set_auto_continue_default set_auto_continue_default(enabled=self.auto_continue_limits) @@ -302,6 +306,71 @@ class ReplState: err=True, ) + def _install_skills(self) -> None: + """Build the skill store and expose its roots read-only to path tools. + + Roots follow ``[skills]`` (explicit paths first, then our own user + directory and other harnesses' directories, with the project-scope ones + tied to the current workspace). Writing is *not* part of this: the roots + are read-only, and skill writes go through the write policy installed + below, so a skill directory never becomes a general write grant. + """ + from plyngent.cli.limits import prompt_skill_write_confirm + from plyngent.config.path import get_default_path + from plyngent.skills import SkillStore, build_roots + from plyngent.tools.skills import SkillWritePolicy, set_skill_write_policy + from plyngent.tools.workspace import set_static_read_roots + + cfg = self.config.skills_config + set_skill_write_policy( + SkillWritePolicy(allow_all=cfg.allow_write, confirm=prompt_skill_write_confirm), + instance=self.instance_state, + ) + if not cfg.enabled: + self.skills = None + self.instance_state.skills = None + _ = set_static_read_roots([], instance=self.instance_state) + return + roots = build_roots( + cfg, + workspace=self.workspace, + user_config_dir=get_default_path().parent, + home=Path.home(), + ) + self.skills = SkillStore(roots) + self.instance_state.skills = self.skills + _ = set_static_read_roots([root.path for root in roots], instance=self.instance_state) + + def reload_skills(self) -> None: + """Rescan the roots (after an external edit or a config change).""" + if self.skills is not None: + self.skills.reload() + + def _skills_text(self) -> str: + """Fold the discovered skills into the system prompt (like MCP instructions). + + Only names, sources, and descriptions: the bodies stay on disk so a turn + pays for the skill it actually reads. Notes and shadowed copies are left + to ``skill_list``, which the model can call when something looks off. + """ + cfg = self.config.skills_config + if self.skills is None or not self.tools_enabled or not cfg.inject_catalog: + return "" + skills = self.skills.skills + if not skills: + return "" + limit = max(0, cfg.max_catalog_skills) + shown = skills[:limit] + lines = [ + "### Skills", + "Available skills (read a skill's SKILL.md with `skill_read` before following it):", + ] + lines.extend(f"- {skill.name} ({skill.root.label}): {skill.description}" for skill in shown) + if len(skills) > len(shown): + lines.append(f"- … and {len(skills) - len(shown)} more; use `skill_search` or `skill_list`") + lines.append("Search all skills with `skill_search`; create or update one with `skill_create` / `skill_edit`.") + return "\n".join(lines) + def _tool_registry(self) -> ToolRegistry | None: if not self.tools_enabled: return None @@ -309,6 +378,7 @@ class ReplState: from plyngent.tools.catalog import register_builtin_tools from plyngent.tools.danger import classify_danger from plyngent.tools.plugins import load_plugin_tools + from plyngent.tools.skills import SKILL_TOOL_NAMES plugins_cfg = self.config.plugins_config catalog = register_builtin_tools() @@ -323,7 +393,9 @@ class ReplState: # Connections were started by the host; registration is sync-only. _ = register_mcp_tools(self.mcp_manager, self.config.mcp_config) # Local surface: builtins + allowlisted plugins (import registers into catalog). - tools = catalog.select(surface="local") + # ``[skills].enabled = false`` drops the skill tools from the surface. + excluded = None if self.config.skills_config.enabled else set(SKILL_TOOL_NAMES) + tools = catalog.select(surface="local", exclude_names=excluded) yolo = self.effective_yolo() != "off" # Always attach soft-confirm path so non-YOLO tools still prompt under YOLO mode. return ToolRegistry( @@ -357,6 +429,7 @@ class ReplState: system_prompt = compose_agent_system_content( agent_cfg.system_prompt, agent_cfg.tool_directives, + self._skills_text(), mcp_text, ) on_limit = prompt_continue_limit_async if (self.interactive_limits or auto_continue_enabled()) else None @@ -539,6 +612,8 @@ class ReplState: self.config.reload() self.instance_state.workspace.path_denylist = tuple(self.config.agent_config.path_denylist or ()) + # Roots and their switches came from the file that was just re-read. + self._install_skills() selectable = self.config.selectable_providers() preferred_provider = self.provider_name if self.provider_name in selectable else None @@ -576,6 +651,8 @@ class ReplState: self.workspace = resolved self.instance_state.workspace_root = resolved self.instance_state.workspace.root = resolved + # Project-scope skill roots follow the workspace. + self._install_skills() if hasattr(self, "agent") and self.agent.tools is not None: self.agent.tools.set_instance_state(self.instance_state) diff --git a/tests/test_cli/test_skills_cmd.py b/tests/test_cli/test_skills_cmd.py new file mode 100644 index 0000000..d9f2223 --- /dev/null +++ b/tests/test_cli/test_skills_cmd.py @@ -0,0 +1,290 @@ +"""CLI surface for skills: ``/skills``, the system-prompt catalog, and writes.""" + +from __future__ import annotations + +from typing import TYPE_CHECKING + +import pytest +import tomlkit + +from plyngent.cli.slash import handle_slash +from plyngent.cli.state import ReplState +from plyngent.config.models import DatabaseConfig, OpenAIProvider +from plyngent.config.store import ConfigStore +from plyngent.memory import MemoryStore +from plyngent.prompting import NonInteractiveBackend, temporary_backend +from plyngent.skills import SKILL_FILE +from tests.test_prompting import ScriptedBackend + +if TYPE_CHECKING: + from collections.abc import AsyncIterator + from pathlib import Path + + +def _write_skill(root: Path, name: str, *, description: str = "A test skill", body: str = "Body.") -> Path: + directory = root / name + directory.mkdir(parents=True, exist_ok=True) + _ = (directory / SKILL_FILE).write_text( + f"---\nname: {name}\ndescription: {description}\n---\n\n{body}\n", + encoding="utf-8", + ) + return directory + + +def _config_doc(tmp_path: Path, *, paths: list[Path] | None = None, extra: str = "") -> tomlkit.TOMLDocument: + lines = ["[skills]", "discover = []"] + if paths: + rendered = ", ".join(f'"{path}"' for path in paths) + lines.append(f"paths = [{rendered}]") + if extra: + lines.append(extra) + return tomlkit.parse("\n".join(lines) + "\n") + + +async def _state( + tmp_path: Path, + *, + skills_dir: Path | None = None, + tools_enabled: bool = False, + extra: str = "", +) -> ReplState: + memory = await MemoryStore.open(DatabaseConfig()) + provider = OpenAIProvider(access_key_or_token="sk-test") + document = _config_doc(tmp_path, paths=[skills_dir] if skills_dir is not None else None, extra=extra) + config = ConfigStore(path=tmp_path / "plyngent.toml", document=document) + config.providers = {"local": provider} + # The skills root sits outside the workspace: reading it works through the + # static read-only roots, writing must stay denied by the path policy. + workspace = tmp_path / "project" + workspace.mkdir(exist_ok=True) + state = ReplState( + config=config, + memory=memory, + workspace=workspace, + provider_name="local", + provider=provider, + model="gpt-test", + tools_enabled=tools_enabled, + quiet=True, + ) + await state.new_session("t") + return state + + +@pytest.fixture +async def skills_state(tmp_path: Path) -> AsyncIterator[ReplState]: + root = tmp_path / "ours" + root.mkdir() + _ = _write_skill(root, "acme-deploy", description="Deploy the acme stack", body="Use pdftotext.") + state = await _state(tmp_path, skills_dir=root, tools_enabled=True) + yield state + await state.memory.close() + + +async def test_slash_status_counts_skills(skills_state: ReplState, capsys: pytest.CaptureFixture[str]) -> None: + assert await handle_slash(skills_state, "/status") is True + assert "skills=1" in capsys.readouterr().out + + +async def test_skills_are_discovered_from_a_configured_root(tmp_path: Path) -> None: + root = tmp_path / "ours" + root.mkdir() + _ = _write_skill(root, "acme-deploy") + state = await _state(tmp_path, skills_dir=root) + try: + assert state.skills is not None + assert [skill.name for skill in state.skills.skills] == ["acme-deploy"] + assert state.instance_state.skills is state.skills + finally: + await state.memory.close() + + +async def test_slash_skills_lists_search_and_reads( + skills_state: ReplState, + capsys: pytest.CaptureFixture[str], +) -> None: + assert await handle_slash(skills_state, "/skills") is True + out = capsys.readouterr().out + assert "skills: 1" in out + assert "acme-deploy" in out + assert "Deploy the acme stack" in out + + assert await handle_slash(skills_state, "/skills search pdftotext") is True + assert "acme-deploy/SKILL.md:" in capsys.readouterr().out + + assert await handle_slash(skills_state, "/skills read acme-deploy") is True + assert "description: Deploy the acme stack" in capsys.readouterr().out + + assert await handle_slash(skills_state, "/skills read acme-deploy --file SKILL.md") is True + assert "acme-deploy" in capsys.readouterr().out + + +async def test_slash_skills_reload_picks_up_a_new_skill( + tmp_path: Path, + capsys: pytest.CaptureFixture[str], +) -> None: + root = tmp_path / "ours" + root.mkdir() + state = await _state(tmp_path, skills_dir=root, tools_enabled=True) + try: + assert await handle_slash(state, "/skills") is True + assert "skills: 0" in capsys.readouterr().out + _ = _write_skill(root, "added-later") + assert await handle_slash(state, "/skills reload") is True + assert "skills reloaded: 1 visible" in capsys.readouterr().out + assert await handle_slash(state, "/skills") is True + assert "added-later" in capsys.readouterr().out + finally: + await state.memory.close() + + +async def test_slash_skills_reports_a_disabled_feature(tmp_path: Path, capsys: pytest.CaptureFixture[str]) -> None: + state = await _state(tmp_path, extra="enabled = false") + try: + assert await handle_slash(state, "/skills") is True + assert "skills: disabled" in capsys.readouterr().out + assert state.skills is None + assert state.instance_state.skills is None + finally: + await state.memory.close() + + +async def test_slash_skills_requires_a_target(skills_state: ReplState, capsys: pytest.CaptureFixture[str]) -> None: + for command in ("/skills search", "/skills read"): + assert await handle_slash(skills_state, command) is True + assert "requires" in capsys.readouterr().err + + +async def test_skill_tools_are_offered_and_can_read_skill_files( + skills_state: ReplState, +) -> None: + """The roots are read-only roots: path tools read them, writes are denied.""" + assert skills_state.agent.tools is not None + assert skills_state.agent.tools.get("skill_list") is not None + + skill_file = skills_state.skills.skills[0].skill_file if skills_state.skills else None + assert skill_file is not None + read = await skills_state.agent.tools.execute("read_file", f'{{"path": "{skill_file}"}}') + assert "Deploy the acme stack" in read + + denied = await skills_state.agent.tools.execute( + "write_file", + f'{{"path": "{skill_file}", "content": "x"}}', + ) + assert denied.startswith("error:") + assert "escapes workspace root" in denied + assert "Deploy the acme stack" in skill_file.read_text(encoding="utf-8") + + +async def test_skill_tools_are_absent_when_disabled(tmp_path: Path) -> None: + root = tmp_path / "ours" + root.mkdir() + _ = _write_skill(root, "acme") + state = await _state(tmp_path, skills_dir=root, tools_enabled=True, extra="enabled = false") + try: + assert state.agent.tools is not None + assert state.agent.tools.get("skill_list") is None + assert state.agent.tools.get("read_file") is not None + finally: + await state.memory.close() + + +async def test_skills_catalog_is_folded_into_the_system_prompt(tmp_path: Path) -> None: + root = tmp_path / "ours" + root.mkdir() + _ = _write_skill(root, "acme-deploy", description="Deploy the acme stack") + state = await _state(tmp_path, skills_dir=root, tools_enabled=True) + try: + prompt = state.agent.system_prompt or "" + assert "### Skills" in prompt + assert "- acme-deploy (config-user): Deploy the acme stack" in prompt + assert "`skill_read`" in prompt + # The body stays on disk: only the catalog goes into the prompt. + assert "Body." not in prompt + finally: + await state.memory.close() + + +async def test_skills_catalog_respects_the_cap(tmp_path: Path) -> None: + root = tmp_path / "ours" + root.mkdir() + for name in ("alpha", "beta", "gamma"): + _ = _write_skill(root, name) + state = await _state(tmp_path, skills_dir=root, tools_enabled=True, extra="max_catalog_skills = 1") + try: + prompt = state.agent.system_prompt or "" + assert "alpha" in prompt + assert "beta" not in prompt + assert "… and 2 more" in prompt + finally: + await state.memory.close() + + +async def test_skills_catalog_can_be_switched_off(tmp_path: Path) -> None: + root = tmp_path / "ours" + root.mkdir() + _ = _write_skill(root, "acme") + state = await _state(tmp_path, skills_dir=root, tools_enabled=True, extra="inject_catalog = false") + try: + assert "### Skills" not in (state.agent.system_prompt or "") + # The tools are still offered; only the prompt block is gone. + assert state.agent.tools is not None + assert state.agent.tools.get("skill_list") is not None + finally: + await state.memory.close() + + +async def test_skills_catalog_needs_tools(tmp_path: Path) -> None: + root = tmp_path / "ours" + root.mkdir() + _ = _write_skill(root, "acme") + state = await _state(tmp_path, skills_dir=root, tools_enabled=False) + try: + assert state.agent.tools is None + assert "### Skills" not in (state.agent.system_prompt or "") + finally: + await state.memory.close() + + +async def test_slash_skills_notes_when_tools_are_off(tmp_path: Path, capsys: pytest.CaptureFixture[str]) -> None: + root = tmp_path / "ours" + root.mkdir() + _ = _write_skill(root, "acme") + state = await _state(tmp_path, skills_dir=root, tools_enabled=False) + try: + assert state.tools_enabled is False + assert await handle_slash(state, "/skills list") is True + assert "tools are off" in capsys.readouterr().out + finally: + await state.memory.close() + + +def test_prompt_skill_write_confirm_noninteractive_denies() -> None: + from pathlib import Path + + from plyngent.cli.limits import prompt_skill_write_confirm + + with temporary_backend(NonInteractiveBackend()): + decision = prompt_skill_write_confirm(Path("/skills/acme"), "create", "create skill", 1.0) + assert decision.allow is False + + +def test_prompt_skill_write_confirm_answers(monkeypatch: pytest.MonkeyPatch) -> None: + from pathlib import Path + + from plyngent.cli import limits + + target = Path("/skills/acme") + with temporary_backend(ScriptedBackend([])): + monkeypatch.setattr(limits, "_read_yes_no_line_with_timeout", lambda _timeout: "o\n") + once = limits.prompt_skill_write_confirm(target, "create", "create skill", 30.0) + monkeypatch.setattr(limits, "_read_yes_no_line_with_timeout", lambda _timeout: "s\n") + session = limits.prompt_skill_write_confirm(target, "create", "create skill", 30.0) + monkeypatch.setattr(limits, "_read_yes_no_line_with_timeout", lambda _timeout: "n\n") + denied = limits.prompt_skill_write_confirm(target, "create", "create skill", 30.0) + monkeypatch.setattr(limits, "_read_yes_no_line_with_timeout", lambda _timeout: None) + timed_out = limits.prompt_skill_write_confirm(target, "create", "create skill", 30.0) + assert (once.allow, once.session) == (True, False) + assert (session.allow, session.session) == (True, True) + assert denied.allow is False + assert timed_out.allow is False