mirror of
https://github.com/NCBM/plyngent.git
synced 2026-11-07 23:47:54 +08:00
tools/skills: create and modify skills behind a grant
skill_create writes a SKILL.md (name, description, body) plus whatever bundled files the model lists, and skill_edit changes one file of an existing skill either by literal replacement, by appending, or by rewriting it. Both resolve every path inside the skill directory first, so a bad argument is answered before anything is written and before the human is asked anything; a rewritten SKILL.md is re-parsed so a skill that lost its description (or its frontmatter) says so instead of failing quietly. The store is reloaded after a write, which makes a new skill visible to the very next skill_list. Writing is a host policy, not a directory-access grant: the denylist wins first, then a session grant, then [skills].allow_write, then the host's confirm hook — asked once per skill directory, so one answer covers its files. A host without a hook denies, which keeps the library default safe; the confirm can also answer 'session' to cover the rest of the chat.
This commit is contained in:
@@ -868,6 +868,41 @@ async def test_pretty_skill_tools(capsys: pytest.CaptureFixture[str]) -> None:
|
||||
assert "* Skill Search 'nope' (no matches)" in out
|
||||
|
||||
|
||||
async def test_pretty_skill_create_and_edit(capsys: pytest.CaptureFixture[str]) -> None:
|
||||
await render_events(
|
||||
_aiter(
|
||||
[
|
||||
_pretty_call("skill_create", '{"name": "acme", "description": "d", "body": "b"}'),
|
||||
_result("created skill 'acme' at /skills/acme\nnotes: SKILL.md has no frontmatter description"),
|
||||
]
|
||||
)
|
||||
)
|
||||
out = capsys.readouterr().out
|
||||
assert "* Skill Create 'acme' (created) — SKILL.md has no frontmatter description" in out
|
||||
|
||||
await render_events(
|
||||
_aiter(
|
||||
[
|
||||
_pretty_call("skill_edit", '{"name": "acme"}'),
|
||||
_result("updated skill 'acme' (SKILL.md: replaced 1 of 1 occurrence(s))"),
|
||||
]
|
||||
)
|
||||
)
|
||||
out = capsys.readouterr().out
|
||||
assert "* Skill Edit 'acme' (SKILL.md: replaced 1 of 1 occurrence(s))" in out
|
||||
|
||||
await render_events(
|
||||
_aiter(
|
||||
[
|
||||
_pretty_call("skill_edit", '{"name": "acme", "file": "scripts/run.sh"}'),
|
||||
_result("error: skill write to /skills/acme denied (declined or timed out after 30s)"),
|
||||
]
|
||||
)
|
||||
)
|
||||
out = capsys.readouterr().out
|
||||
assert "* Skill Edit 'acme' scripts/run.sh (error: skill write to /skills/acme denied" in out
|
||||
|
||||
|
||||
async def test_pretty_prefix_shown_while_call_runs(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
capsys: pytest.CaptureFixture[str],
|
||||
|
||||
@@ -5,9 +5,20 @@ from typing import TYPE_CHECKING
|
||||
|
||||
from plyngent.config.models import SkillsConfig
|
||||
from plyngent.skills import SKILL_FILE, SkillStore, build_roots
|
||||
from plyngent.tools.context import InstanceState, bind_instance
|
||||
from plyngent.tools.skills import SKILL_TOOL_NAMES, format_skill_table, skill_list, skill_read, skill_search
|
||||
from plyngent.tools.workspace import set_static_read_roots, set_workspace_root
|
||||
from plyngent.tools.context import InstanceState, SessionState, bind_instance, bind_session
|
||||
from plyngent.tools.skills import (
|
||||
SKILL_TOOL_NAMES,
|
||||
SkillWriteDecision,
|
||||
SkillWritePolicy,
|
||||
format_skill_table,
|
||||
set_skill_write_policy,
|
||||
skill_create,
|
||||
skill_edit,
|
||||
skill_list,
|
||||
skill_read,
|
||||
skill_search,
|
||||
)
|
||||
from plyngent.tools.workspace import set_path_denylist, set_static_read_roots, set_workspace_root
|
||||
|
||||
if TYPE_CHECKING:
|
||||
from collections.abc import Generator
|
||||
@@ -26,7 +37,13 @@ def _write_skill(root: Path, name: str, *, description: str | None = "A test ski
|
||||
|
||||
|
||||
@contextmanager
|
||||
def _bound(tmp_path: Path, *roots: Path, skills: SkillStore | None = None) -> Generator[InstanceState]:
|
||||
def _bound(
|
||||
tmp_path: Path,
|
||||
*roots: Path,
|
||||
skills: SkillStore | None = None,
|
||||
session: bool = True,
|
||||
policy: SkillWritePolicy | None = None,
|
||||
) -> Generator[InstanceState]:
|
||||
"""Bind an instance whose skill store scans *roots*, like the CLI does."""
|
||||
workspace = tmp_path / "project"
|
||||
workspace.mkdir(exist_ok=True)
|
||||
@@ -40,12 +57,30 @@ def _bound(tmp_path: Path, *roots: Path, skills: SkillStore | None = None) -> Ge
|
||||
)
|
||||
)
|
||||
instance = InstanceState(workspace_root=workspace, skills=store)
|
||||
with bind_instance(instance):
|
||||
session_state = SessionState(session_id=1) if session else None
|
||||
with bind_instance(instance), bind_session(session_state):
|
||||
_ = set_workspace_root(workspace)
|
||||
_ = set_static_read_roots(roots)
|
||||
if policy is not None:
|
||||
set_skill_write_policy(policy, instance=instance)
|
||||
yield instance
|
||||
|
||||
|
||||
def _write_root(tmp_path: Path) -> Path:
|
||||
"""An empty user-level skills directory to create skills in."""
|
||||
root = tmp_path / "ours"
|
||||
root.mkdir(parents=True, exist_ok=True)
|
||||
return root
|
||||
|
||||
|
||||
def _allow(*_args: object) -> SkillWriteDecision:
|
||||
return SkillWriteDecision(allow=True)
|
||||
|
||||
|
||||
def _deny(*_args: object) -> bool:
|
||||
return False
|
||||
|
||||
|
||||
async def test_skill_list_without_a_store_returns_an_error(tmp_path: Path) -> None:
|
||||
with _bound(tmp_path, skills=None) as instance:
|
||||
instance.skills = None
|
||||
@@ -188,7 +223,181 @@ def test_format_skill_table_lists_roots_in_priority_order(tmp_path: Path) -> Non
|
||||
|
||||
|
||||
def test_skill_tool_names_cover_the_group() -> None:
|
||||
expected = ("skill_list", "skill_read", "skill_search")
|
||||
expected = ("skill_list", "skill_read", "skill_search", "skill_create", "skill_edit")
|
||||
for name in expected:
|
||||
assert name in SKILL_TOOL_NAMES
|
||||
assert len(SKILL_TOOL_NAMES) == len(expected)
|
||||
|
||||
|
||||
async def test_skill_create_writes_skill_md_and_bundled_files(tmp_path: Path) -> None:
|
||||
root = _write_root(tmp_path)
|
||||
with _bound(tmp_path, root, policy=SkillWritePolicy(allow_all=True)):
|
||||
out = await skill_create.handler(
|
||||
"acme-deploy",
|
||||
"Deploy the acme stack",
|
||||
"# Steps\n\n1. Build.\n",
|
||||
files={"scripts/deploy.sh": "#!/bin/sh\necho deploy\n"},
|
||||
)
|
||||
assert out.startswith("created skill 'acme-deploy' at ")
|
||||
assert (root / "acme-deploy" / SKILL_FILE).is_file()
|
||||
assert (root / "acme-deploy" / "scripts" / "deploy.sh").read_text(encoding="utf-8").startswith("#!/bin/sh")
|
||||
# The new skill is visible right away, without a restart.
|
||||
assert "acme-deploy [config-user]" in await skill_list.handler()
|
||||
read = await skill_read.handler("acme-deploy")
|
||||
assert "description: Deploy the acme stack" in read
|
||||
assert "1. Build." in read
|
||||
|
||||
|
||||
async def test_skill_create_quotes_a_frontmatter_description_that_needs_it(tmp_path: Path) -> None:
|
||||
root = _write_root(tmp_path)
|
||||
with _bound(tmp_path, root, policy=SkillWritePolicy(allow_all=True)):
|
||||
_ = await skill_create.handler("quoted", "Use: colons, {braces}", "Body.")
|
||||
skill = SkillStore(
|
||||
build_roots(SkillsConfig(paths=[str(root)], discover=[]), workspace=tmp_path, user_config_dir=tmp_path)
|
||||
).skills[0]
|
||||
assert skill.description == "Use: colons, {braces}"
|
||||
|
||||
|
||||
async def test_skill_create_rejects_a_bad_name_and_a_taken_one(tmp_path: Path) -> None:
|
||||
root = _write_root(tmp_path)
|
||||
_ = _write_skill(root, "taken")
|
||||
with _bound(tmp_path, root, policy=SkillWritePolicy(allow_all=True)):
|
||||
assert (await skill_create.handler("Not-A-Name", "d", "b")).startswith("error: skill name")
|
||||
assert "already exists" in await skill_create.handler("taken", "d", "b")
|
||||
assert (await skill_create.handler("ok-name", "d", "b")).startswith("created skill")
|
||||
|
||||
|
||||
async def test_skill_create_rejects_a_bundled_file_outside_the_skill(tmp_path: Path) -> None:
|
||||
root = _write_root(tmp_path)
|
||||
with _bound(tmp_path, root, policy=SkillWritePolicy(allow_all=True)):
|
||||
out = await skill_create.handler("acme", "d", "b", files={"../escape.sh": "x"})
|
||||
assert out == "error: bundled file '../escape.sh' is outside skill 'acme'"
|
||||
assert not (tmp_path / "escape.sh").exists()
|
||||
|
||||
|
||||
async def test_skill_write_needs_a_grant(tmp_path: Path) -> None:
|
||||
"""Without a hook the write is denied, and the message says how to open it."""
|
||||
root = _write_root(tmp_path)
|
||||
with _bound(tmp_path, root):
|
||||
out = await skill_create.handler("acme", "d", "b")
|
||||
assert out == (
|
||||
"error: writing skills needs a grant (no confirm hook is installed; denied); "
|
||||
"set [skills].allow_write = true to allow skill writes"
|
||||
)
|
||||
assert not (root / "acme").exists()
|
||||
|
||||
|
||||
async def test_skill_write_prompt_denial_is_reported(tmp_path: Path) -> None:
|
||||
root = _write_root(tmp_path)
|
||||
_ = _write_skill(root, "acme", body="Before.")
|
||||
with _bound(tmp_path, root, policy=SkillWritePolicy(confirm=_deny, timeout=3.0)):
|
||||
out = await skill_edit.handler("acme", old_string="Before.", new_string="After.")
|
||||
assert out.startswith("error: skill write to ")
|
||||
assert out.endswith("denied (declined or timed out after 3s)")
|
||||
assert "After." not in (root / "acme" / SKILL_FILE).read_text(encoding="utf-8")
|
||||
|
||||
|
||||
async def test_skill_write_confirm_sees_the_action_and_the_skill(tmp_path: Path) -> None:
|
||||
root = _write_root(tmp_path)
|
||||
seen: list[tuple[Path, str, str, float]] = []
|
||||
|
||||
def hook(target: Path, action: str, detail: str, timeout: float) -> bool:
|
||||
seen.append((target, action, detail, timeout))
|
||||
return True
|
||||
|
||||
with _bound(tmp_path, root, policy=SkillWritePolicy(confirm=hook, timeout=5.0)):
|
||||
_ = await skill_create.handler("acme", "d", "b")
|
||||
assert seen == [(root / "acme", "create", f"create skill 'acme' at {root / 'acme'} (SKILL.md)", 5.0)]
|
||||
|
||||
|
||||
async def test_skill_write_session_grant_is_asked_once(tmp_path: Path) -> None:
|
||||
"""A 'session' answer covers the same skill for the rest of the chat."""
|
||||
root = _write_root(tmp_path)
|
||||
calls: list[Path] = []
|
||||
|
||||
def hook(target: Path, action: str, detail: str, timeout: float) -> SkillWriteDecision:
|
||||
calls.append(target)
|
||||
return SkillWriteDecision(allow=True, session=True)
|
||||
|
||||
with _bound(tmp_path, root, policy=SkillWritePolicy(confirm=hook)):
|
||||
_ = await skill_create.handler("acme", "d", "b")
|
||||
_ = await skill_edit.handler("acme", append="\nMore.\n")
|
||||
_ = await skill_edit.handler("acme", file="notes.md", new_string="note\n")
|
||||
# One prompt for the skill directory, covering SKILL.md and later files.
|
||||
assert calls == [root / "acme"]
|
||||
assert (root / "acme" / "notes.md").read_text(encoding="utf-8") == "note\n"
|
||||
|
||||
|
||||
async def test_skill_write_denylist_wins_over_a_grant(tmp_path: Path) -> None:
|
||||
root = _write_root(tmp_path)
|
||||
with _bound(tmp_path, root, policy=SkillWritePolicy(allow_all=True)):
|
||||
set_path_denylist(["acme"])
|
||||
try:
|
||||
out = await skill_create.handler("acme", "d", "b")
|
||||
finally:
|
||||
set_path_denylist(())
|
||||
assert out.startswith("error: path denied by policy (matched 'acme')")
|
||||
assert not (root / "acme").exists()
|
||||
|
||||
|
||||
async def test_skill_edit_forms(tmp_path: Path) -> None:
|
||||
root = _write_root(tmp_path)
|
||||
_ = _write_skill(root, "acme", body="Line one.\nLine two.")
|
||||
with _bound(tmp_path, root, policy=SkillWritePolicy(allow_all=True)):
|
||||
out = await skill_edit.handler("acme", old_string="Line two.", new_string="Line two, edited.")
|
||||
assert out.startswith("updated skill 'acme' (SKILL.md: replaced 1 of 1 occurrence(s))")
|
||||
|
||||
out = await skill_edit.handler("acme", append="\nAppended tail.")
|
||||
assert "appended 15 characters" in out
|
||||
|
||||
out = await skill_edit.handler("acme", file="scripts/run.sh", new_string="#!/bin/sh\n")
|
||||
assert "rewrote 10 characters" in out
|
||||
assert (root / "acme" / "scripts" / "run.sh").is_file()
|
||||
text = (root / "acme" / SKILL_FILE).read_text(encoding="utf-8")
|
||||
assert "Line two, edited." in text
|
||||
assert text.endswith("Appended tail.")
|
||||
|
||||
|
||||
async def test_skill_edit_replace_all_and_missing_match(tmp_path: Path) -> None:
|
||||
root = _write_root(tmp_path)
|
||||
directory = _write_skill(root, "acme", body="hit\nhit\nhit")
|
||||
with _bound(tmp_path, root, policy=SkillWritePolicy(allow_all=True)):
|
||||
out = await skill_edit.handler("acme", old_string="hit", new_string="done", replace_all=True)
|
||||
assert "replaced 3 of 3 occurrence(s)" in out
|
||||
missing = await skill_edit.handler("acme", old_string="nope", new_string="x")
|
||||
assert missing == "error: old_string not found in SKILL.md of skill 'acme'"
|
||||
assert (directory / SKILL_FILE).read_text(encoding="utf-8").count("done") == 3
|
||||
|
||||
|
||||
async def test_skill_edit_rejects_conflicting_forms(tmp_path: Path) -> None:
|
||||
root = _write_root(tmp_path)
|
||||
_ = _write_skill(root, "acme")
|
||||
with _bound(tmp_path, root, policy=SkillWritePolicy(allow_all=True)):
|
||||
assert (await skill_edit.handler("acme")).startswith("error: nothing to write")
|
||||
assert (await skill_edit.handler("acme", old_string="a", new_string="b", append="c")).startswith(
|
||||
"error: use either"
|
||||
)
|
||||
assert (await skill_edit.handler("acme", file="nope.md", append="x")) == (
|
||||
"error: no such file in skill 'acme': nope.md"
|
||||
)
|
||||
assert (await skill_edit.handler("acme", file="../x.md", new_string="x")).startswith("error: file")
|
||||
|
||||
|
||||
async def test_skill_edit_notes_a_rewritten_frontmatter(tmp_path: Path) -> None:
|
||||
"""A SKILL.md that loses its description (or breaks) is reported, not repaired."""
|
||||
root = _write_root(tmp_path)
|
||||
_ = _write_skill(root, "acme")
|
||||
with _bound(tmp_path, root, policy=SkillWritePolicy(allow_all=True)):
|
||||
dropped = await skill_edit.handler("acme", new_string="Only a body, no frontmatter.\n")
|
||||
broken = await skill_edit.handler("acme", new_string="---\nname: acme\nbroken line\n---\n\nBody.\n")
|
||||
assert dropped.endswith("notes: SKILL.md has no frontmatter description; the first paragraph will be used")
|
||||
assert "notes: SKILL.md frontmatter not parsed" in broken
|
||||
|
||||
|
||||
async def test_skill_tools_reject_writes_when_no_session_is_bound(tmp_path: Path) -> None:
|
||||
"""A host without session state still denies instead of crashing on the grant."""
|
||||
root = _write_root(tmp_path)
|
||||
_ = _write_skill(root, "acme", body="Before.")
|
||||
with _bound(tmp_path, root, session=False, policy=SkillWritePolicy(confirm=_allow)):
|
||||
out = await skill_edit.handler("acme", old_string="Before.", new_string="After.")
|
||||
assert out.startswith("updated skill 'acme'")
|
||||
|
||||
Reference in New Issue
Block a user