From 2ea7f9f0e2d2268d7a9896d1dfcac19a0f0b942a Mon Sep 17 00:00:00 2001 From: Pavel Rykov Date: Tue, 14 Jul 2026 13:49:23 +0300 Subject: [PATCH 01/14] docs(skills): design spec for skills subsystem Layered design for Agent Skills (SKILL.md, progressive disclosure): loader/model, registry+config, system-prompt auto-registration, use_skill invocation tool, and command surfaces (ACP available_commands, optional MCP prompts). Grounded in Anthropic Agent Skills best practices. Co-Authored-By: Claude Opus 4.8 --- docs/design/skills.md | 179 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 179 insertions(+) create mode 100644 docs/design/skills.md diff --git a/docs/design/skills.md b/docs/design/skills.md new file mode 100644 index 00000000..869d5510 --- /dev/null +++ b/docs/design/skills.md @@ -0,0 +1,179 @@ +# Skills subsystem — design & specification + +Status: draft (feat/skills) +Author: feat/skills work +Layered per `.cursor/rules/architecture.mdc` (bottom-up, one class at a time, TDD). + +## 1. Motivation + +Add first-class **Agent Skills** to SGR Agent Core, modeled on the Anthropic +"Agent Skills" pattern (a `SKILL.md` file with YAML frontmatter + markdown body, +progressive disclosure). Two capabilities are required: + +1. **Autonomous invocation** — the agent automatically *registers* available + skills into its system prompt (name + description only, "progressive + disclosure" level 1) so the LLM can decide, on its own, to invoke a skill. + Invoking a skill loads its full body (level 2) into the conversation. +2. **Skills as commands** — skills are surfaced as user-visible **commands** + through the protocol layers: ACP `available_commands` (slash commands in ACP + clients such as Zed) and an optional MCP prompts server. This mirrors how + codex / Claude Code / Cursor expose reusable prompts/skills as commands. + +## 2. What a skill is (Anthropic model) + +A skill is a directory containing a `SKILL.md`: + +``` +skills/ + pdf-processing/ + SKILL.md # required: frontmatter + body + scripts/ # optional bundled resources (level 3) + references/ +``` + +`SKILL.md` frontmatter (YAML). The portable open standard requires exactly two +fields — `name` and `description`; the rest are optional extensions. + +| field | required | validation / notes | +|-----------------|----------|--------------------------------------------------------------------| +| `name` | yes | ≤ 64 chars; only lowercase letters, digits, hyphens; no XML tags; not "anthropic"/"claude" | +| `description` | yes | non-empty; ≤ 1024 chars; no XML tags; third person; *what it does* + *when to use* | +| `license` | no | SPDX id or text | +| `allowed-tools` | no | advisory allowlist of tool names the skill uses (soft, documented) | +| `metadata` | no | free-form dict (version, author, ...) | + +Validation is enforced at parse time (Anthropic spec rules). Body: markdown +instructions (level-2 content, keep under ~500 lines / ~5k tokens); push long +material into reference files loaded on demand. + +### Progressive disclosure (three levels) + +- **Level 1 — metadata**: `name` + `description` always injected into the system + prompt so the model knows the skill exists and when to reach for it. +- **Level 2 — body**: the `SKILL.md` markdown body, loaded into context only when + the skill is invoked. +- **Level 3 — bundled files**: scripts/resources under the skill dir, referenced + from the body and read/run only as needed. + +## 3. Discovery + +Skill roots are scanned (in order; later roots override same-named skills): + +1. Packaged/builtin skills dir (optional, shipped with an example). +2. User dir: `~/.sgr/skills` (personal). +3. Project dir: `./.sgr/skills` relative to `config_dir`. +4. Explicit dirs from config (`skills.paths`) and per-agent `skills:` selection. + +A skill dir is any immediate subdirectory that contains a `SKILL.md`. + +## 4. Architecture (layered) + +### Layer 1 — data model + loader (no deps) + +- `sgr_agent_core/skills/models.py` + - `SkillMetadata(BaseModel)`: `name`, `description`, `license: str | None`, + `allowed_tools: list[str]`, `metadata: dict`. + - `Skill(BaseModel)`: `metadata: SkillMetadata`, `body: str`, `path: Path`. + Convenience props: `name`, `description`. +- `sgr_agent_core/skills/loader.py` + - `SkillLoader.load_skill(dir: Path) -> Skill` — parse `SKILL.md` + (frontmatter via a tiny YAML front-matter splitter; no new dependency — + PyYAML already present). Validate required fields; raise `SkillError` + on malformed/missing. + - `SkillLoader.discover(root: Path) -> list[Skill]` — scan subdirs. + +### Layer 2 — registry + config + +- `sgr_agent_core/skills/registry.py` + - `SkillRegistry` — runtime catalog (dict name -> Skill). Unlike ToolRegistry + it is populated by scanning dirs (skills are data, not subclasses). + Methods: `register`, `get`, `list_items`, `load_from_paths(paths)`, + `clear`. +- Config: `SkillsConfig(BaseModel)` added to `AgentConfig` + (`skills: SkillsConfig | None`), with: + - `paths: list[str]` — extra skill roots. + - `enabled: bool = True`. + - `include: list[str] | None` / `exclude: list[str] | None` — filter by name. + Per-agent selection via `AgentDefinition.skills: list[str]` (names) — resolved + in a validator mirroring `agent_level_tools_validator`. + +### Layer 3 — prompt injection + invocation tool + +- `PromptLoader.get_system_prompt(..., available_skills=...)` — render a + `{available_skills}` block (numbered `name: description`). Default + `system_prompt.txt` gains an `` section. Always pass the + kwarg (empty string when no skills) so custom templates using the placeholder + never KeyError, and templates without it are unaffected (`str.format` ignores + extra kwargs). +- `sgr_agent_core/tools/skill_tool.py` + - `SkillTool(SystemBaseTool)` — `tool_name = "use_skill"`. Field: + `skill_name: str`. `__call__` looks up the skill (from + `context.available_skills` / SkillRegistry), returns the level-2 body as a + string (which the agent appends to conversation). Unknown skill -> helpful + error string listing available skills. This is the autonomous-invocation + vehicle and flows through both function-calling and SGR union paths via the + registry — same pattern as `SearchToolsTool`. + +### Layer 4 — agent + factory wiring + +- `AgentContext` gains `available_skills: list[Skill]` (default empty) so tools + can resolve skills without global state. +- `BaseAgent._prepare_context()` passes + `available_skills=self.available_skills` to `PromptLoader.get_system_prompt`. + `BaseAgent.__init__` accepts `skills: list[Skill] | None`. +- `AgentFactory.create` resolves skills (from `SkillRegistry` + + `agent_def.skills` + config paths), injects `SkillTool` into the toolkit when + skills are present, and threads the skill list into the agent + context. + +### Layer 5 — protocol command surfaces + +- **ACP**: `SGRACPBridge` sends an `AvailableCommandsUpdate` session update + after `new_session` (and on agent switch), advertising each skill as an + `AvailableCommand(name, description, input=UnstructuredCommandInput(...))`. + A prompt of the form `/skill-name args...` is mapped to invoking that skill + (prepend the skill body / run `use_skill`). +- **MCP (optional/stretch)**: a `SkillsMCPServer` (FastMCP) exposing each skill + as an MCP **prompt** (`@mcp.prompt`) — the MCP primitive clients render as + slash commands (`prompts/list`, `prompts/get`). Symmetric inverse of + `MCP2ToolConverter`. +- **CLI**: `sgrsh --list-skills` prints discovered skills; `/skill` handling in + chat loop (nice-to-have). +- **Server**: `GET /v1/skills` lists discovered skills (OpenAI-adjacent). + +## 5. Testing plan (TDD, red first) + +- `tests/test_skill_loader.py` — frontmatter parsing, required-field validation, + discovery, malformed handling. +- `tests/test_skill_registry.py` — register/get/list, path loading, override. +- `tests/test_skills_config.py` — config parse, per-agent selection validator. +- `tests/test_prompts.py` (extend) — `{available_skills}` rendering. +- `tests/test_skill_tool.py` — `use_skill` returns body / errors. +- `tests/test_agent_factory.py` (extend) — factory injects SkillTool + skills. +- `tests/test_acp_bridge.py` (extend) — available_commands advertised; `/name` + routes to skill. +- `tests/test_skills_e2e.py` (`@pytest.mark.e2e`) — end-to-end autonomous + invocation with a mocked LLM choosing `use_skill`. +- Mode checks: ACP, CLI, OpenAI server. + +## 5a. Listing budget & invocation semantics + +- **Listing budget (level 1).** Injecting many skills must not blow the system + prompt. Each rendered entry is `name: description` with the description + truncated to a per-entry cap (`SkillsConfig.max_desc_chars`, default 500). + This mirrors Claude Code's skill-listing budget (~1% context, ~1536-char + per-entry cap). The name is always shown in full. +- **Invocation = prompt injection.** When `use_skill` runs, the level-2 body is + returned as the tool result and appended to the conversation, where it + persists for the rest of the run (matching Anthropic semantics). No isolated + subagent in v1. +- **ACP push mechanism.** The bridge advertises commands via + `client.session_update(session_id, AvailableCommandsUpdate(available_commands=[ + AvailableCommand(name, description, input=UnstructuredCommandInput(hint=...))]))` + — same `session_update` channel the streaming generator already uses. + +## 6. Non-goals / constraints + +- No new runtime dependency (reuse PyYAML, fastmcp, acp already present). +- Backward compatible: agents without skills behave exactly as before + (`{available_skills}` optional, SkillTool only added when skills exist). +- English comments; 120-char lines; ruff clean. From 630f20768c3708e07d8aab84afe121dd2adcc9be Mon Sep 17 00:00:00 2001 From: Pavel Rykov Date: Tue, 14 Jul 2026 13:51:50 +0300 Subject: [PATCH 02/14] =?UTF-8?q?feat(skills):=20Layer=201=20=E2=80=94=20S?= =?UTF-8?q?kill=20model=20and=20SKILL.md=20loader?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add sgr_agent_core.skills package: - SkillMetadata: validated frontmatter (name/description spec rules, allowed-tools alias, metadata) per Anthropic Agent Skills. - Skill: metadata + markdown body + source path. - SkillLoader: parse SKILL.md frontmatter/body, load_skill from a dir (defaults name to dir), discover skills under a root (skips malformed). Tests: tests/test_skill_loader.py (19 tests). Full suite 516 passed. Co-Authored-By: Claude Opus 4.8 --- sgr_agent_core/skills/__init__.py | 18 ++++ sgr_agent_core/skills/loader.py | 133 ++++++++++++++++++++++++++++++ sgr_agent_core/skills/models.py | 90 ++++++++++++++++++++ tests/test_skill_loader.py | 131 +++++++++++++++++++++++++++++ 4 files changed, 372 insertions(+) create mode 100644 sgr_agent_core/skills/__init__.py create mode 100644 sgr_agent_core/skills/loader.py create mode 100644 sgr_agent_core/skills/models.py create mode 100644 tests/test_skill_loader.py diff --git a/sgr_agent_core/skills/__init__.py b/sgr_agent_core/skills/__init__.py new file mode 100644 index 00000000..a62d7b19 --- /dev/null +++ b/sgr_agent_core/skills/__init__.py @@ -0,0 +1,18 @@ +"""Agent Skills subsystem for SGR Agent Core. + +Skills are directories containing a ``SKILL.md`` file (YAML frontmatter plus a +markdown body). They are auto-registered into the agent system prompt (name + +description) so the agent can invoke them autonomously, and can be surfaced as +commands over ACP/MCP. +""" + +from sgr_agent_core.skills.loader import SKILL_FILE, SkillLoader +from sgr_agent_core.skills.models import Skill, SkillError, SkillMetadata + +__all__ = [ + "Skill", + "SkillError", + "SkillMetadata", + "SkillLoader", + "SKILL_FILE", +] diff --git a/sgr_agent_core/skills/loader.py b/sgr_agent_core/skills/loader.py new file mode 100644 index 00000000..eea5e8c6 --- /dev/null +++ b/sgr_agent_core/skills/loader.py @@ -0,0 +1,133 @@ +"""Loader that parses ``SKILL.md`` files and discovers skills on disk.""" + +from __future__ import annotations + +import logging +from pathlib import Path + +import yaml +from pydantic import ValidationError + +from sgr_agent_core.skills.models import Skill, SkillError, SkillMetadata + +logger = logging.getLogger(__name__) + +SKILL_FILE = "SKILL.md" + + +class SkillLoader: + """Parse and discover skills from the filesystem. + + A skill lives in its own directory that contains a ``SKILL.md`` file with + YAML frontmatter and a markdown body (progressive disclosure level 2). + """ + + @staticmethod + def _split_frontmatter(text: str) -> tuple[dict, str]: + """Split raw ``SKILL.md`` text into a frontmatter dict and a body. + + Args: + text: Full ``SKILL.md`` content. + + Returns: + Tuple of (frontmatter mapping, markdown body). + + Raises: + SkillError: If the leading YAML frontmatter block is missing or + unterminated, or is not a mapping. + """ + stripped = text.lstrip("") + if not stripped.startswith("---"): + raise SkillError("SKILL.md must start with a YAML frontmatter block delimited by '---'") + + lines = stripped.splitlines() + end_index: int | None = None + for i in range(1, len(lines)): + if lines[i].strip() == "---": + end_index = i + break + if end_index is None: + raise SkillError("SKILL.md frontmatter block is not terminated by '---'") + + frontmatter_text = "\n".join(lines[1:end_index]) + body = "\n".join(lines[end_index + 1 :]).strip() + try: + data = yaml.safe_load(frontmatter_text) or {} + except yaml.YAMLError as exc: + raise SkillError(f"Invalid YAML in SKILL.md frontmatter: {exc}") from exc + if not isinstance(data, dict): + raise SkillError("SKILL.md frontmatter must be a YAML mapping") + return data, body + + @classmethod + def parse(cls, text: str, *, path: Path | None = None, default_name: str | None = None) -> Skill: + """Parse ``SKILL.md`` text into a validated :class:`Skill`. + + Args: + text: Full ``SKILL.md`` content. + path: Optional source path stored on the skill. + default_name: Name to use when frontmatter omits ``name`` + (typically the skill directory name). + + Returns: + A validated :class:`Skill`. + + Raises: + SkillError: If frontmatter is missing, malformed, or fails validation. + """ + data, body = cls._split_frontmatter(text) + if "name" not in data and default_name is not None: + data["name"] = default_name + try: + metadata = SkillMetadata.model_validate(data) + except ValidationError as exc: + raise SkillError(f"Invalid skill metadata: {exc}") from exc + return Skill(metadata=metadata, body=body, path=path) + + @classmethod + def load_skill(cls, directory: Path) -> Skill: + """Load a single skill from a directory containing ``SKILL.md``. + + Args: + directory: Directory holding the ``SKILL.md`` file. + + Returns: + The loaded :class:`Skill`. + + Raises: + SkillError: If ``SKILL.md`` is missing or invalid. + """ + directory = Path(directory) + skill_file = directory / SKILL_FILE + if not skill_file.is_file(): + raise SkillError(f"No {SKILL_FILE} found in {directory}") + text = skill_file.read_text(encoding="utf-8") + return cls.parse(text, path=directory, default_name=directory.name) + + @classmethod + def discover(cls, root: Path) -> list[Skill]: + """Discover all skills in immediate subdirectories of ``root``. + + Malformed skills are logged and skipped so one bad skill never breaks + discovery of the rest. + + Args: + root: Directory whose immediate subdirectories may be skills. + + Returns: + List of successfully loaded skills, sorted by name. + """ + root = Path(root) + if not root.is_dir(): + return [] + skills: list[Skill] = [] + for entry in sorted(root.iterdir()): + if not entry.is_dir(): + continue + if not (entry / SKILL_FILE).is_file(): + continue + try: + skills.append(cls.load_skill(entry)) + except SkillError as exc: + logger.warning("Skipping malformed skill in %s: %s", entry, exc) + return skills diff --git a/sgr_agent_core/skills/models.py b/sgr_agent_core/skills/models.py new file mode 100644 index 00000000..01a36894 --- /dev/null +++ b/sgr_agent_core/skills/models.py @@ -0,0 +1,90 @@ +"""Data model for Agent Skills (Anthropic SKILL.md model). + +A skill is a directory containing a ``SKILL.md`` file with YAML frontmatter +(``name`` + ``description`` required) and a markdown body. Frontmatter is +validated against the Anthropic Agent Skills spec rules. +""" + +from __future__ import annotations + +import re +from pathlib import Path + +from pydantic import BaseModel, ConfigDict, Field, field_validator + +NAME_MAX_LENGTH = 64 +DESCRIPTION_MAX_LENGTH = 1024 +NAME_PATTERN = re.compile(r"^[a-z0-9-]+$") +RESERVED_WORDS = ("anthropic", "claude") + + +class SkillError(Exception): + """Raised when a skill cannot be parsed, validated, or loaded.""" + + +class SkillMetadata(BaseModel): + """Validated frontmatter of a ``SKILL.md`` file. + + Enforces the Anthropic Agent Skills spec: ``name`` is lowercase letters, + digits and hyphens up to 64 chars (no XML tags, no reserved words) and + ``description`` is a non-empty string up to 1024 chars with no XML tags. + """ + + model_config = ConfigDict(populate_by_name=True) + + name: str + description: str + license: str | None = None + allowed_tools: list[str] = Field(default_factory=list, alias="allowed-tools") + metadata: dict = Field(default_factory=dict) + + @field_validator("name") + @classmethod + def _validate_name(cls, value: str) -> str: + if not value: + raise ValueError("Skill 'name' must be non-empty") + if len(value) > NAME_MAX_LENGTH: + raise ValueError(f"Skill 'name' must be at most {NAME_MAX_LENGTH} characters") + if "<" in value or ">" in value: + raise ValueError("Skill 'name' cannot contain XML tags") + if not NAME_PATTERN.match(value): + raise ValueError("Skill 'name' must contain only lowercase letters, digits and hyphens") + lowered = value.lower() + for word in RESERVED_WORDS: + if word in lowered: + raise ValueError(f"Skill 'name' cannot contain the reserved word '{word}'") + return value + + @field_validator("description") + @classmethod + def _validate_description(cls, value: str) -> str: + if not value or not value.strip(): + raise ValueError("Skill 'description' must be non-empty") + if len(value) > DESCRIPTION_MAX_LENGTH: + raise ValueError(f"Skill 'description' must be at most {DESCRIPTION_MAX_LENGTH} characters") + if "<" in value or ">" in value: + raise ValueError("Skill 'description' cannot contain XML tags") + return value + + +class Skill(BaseModel): + """A loaded skill: validated metadata plus the markdown body and source path.""" + + metadata: SkillMetadata + body: str = "" + path: Path | None = None + + @property + def name(self) -> str: + """Skill name (from metadata).""" + return self.metadata.name + + @property + def description(self) -> str: + """Skill description (from metadata).""" + return self.metadata.description + + @property + def allowed_tools(self) -> list[str]: + """Advisory list of tool names the skill uses.""" + return self.metadata.allowed_tools diff --git a/tests/test_skill_loader.py b/tests/test_skill_loader.py new file mode 100644 index 00000000..09d7bbf9 --- /dev/null +++ b/tests/test_skill_loader.py @@ -0,0 +1,131 @@ +"""Tests for the skills data model and loader (Layer 1).""" + +from pathlib import Path + +import pytest + +from sgr_agent_core.skills import Skill, SkillError, SkillLoader, SkillMetadata + + +def _write_skill(root: Path, name: str, frontmatter: str, body: str = "Body text") -> Path: + """Create a skill directory with a SKILL.md and return the directory.""" + skill_dir = root / name + skill_dir.mkdir(parents=True, exist_ok=True) + (skill_dir / "SKILL.md").write_text(f"---\n{frontmatter}\n---\n\n{body}\n", encoding="utf-8") + return skill_dir + + +class TestSkillMetadata: + """Validation rules for skill frontmatter metadata.""" + + def test_valid_metadata(self): + meta = SkillMetadata(name="pdf-processing", description="Process PDF files. Use when working with PDFs.") + assert meta.name == "pdf-processing" + assert meta.description.startswith("Process PDF") + assert meta.allowed_tools == [] + assert meta.metadata == {} + + def test_name_too_long_rejected(self): + with pytest.raises((ValueError, SkillError)): + SkillMetadata(name="a" * 65, description="ok") + + def test_name_invalid_chars_rejected(self): + with pytest.raises((ValueError, SkillError)): + SkillMetadata(name="Bad_Name", description="ok") + + def test_name_reserved_word_rejected(self): + with pytest.raises((ValueError, SkillError)): + SkillMetadata(name="claude-helper", description="ok") + + def test_name_xml_tag_rejected(self): + with pytest.raises((ValueError, SkillError)): + SkillMetadata(name="foo", description="ok") + + def test_description_empty_rejected(self): + with pytest.raises((ValueError, SkillError)): + SkillMetadata(name="foo", description="") + + def test_description_too_long_rejected(self): + with pytest.raises((ValueError, SkillError)): + SkillMetadata(name="foo", description="x" * 1025) + + def test_description_xml_tag_rejected(self): + with pytest.raises((ValueError, SkillError)): + SkillMetadata(name="foo", description="uses