-
Notifications
You must be signed in to change notification settings - Fork 1.5k
feat(skills): agent-skills (SKILL.md) format support for review/improve/describe #2385
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 3 commits
6725358
8479dae
a801454
49bc3d2
10f3591
33f6e27
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| # CLAUDE.md | ||
|
|
||
| This file provides guidance to Claude Code (claude.ai/code) when working with code in this repository. | ||
|
|
||
| See `AGENTS.md` for the full repository guidelines (dos/don'ts, coding style, safety, security). The notes below are the high-leverage subset for navigating PR-Agent quickly. | ||
|
|
||
| ## Common commands | ||
|
|
||
| Run from the repo root with the virtualenv activated: | ||
|
|
||
| - Single unit test: `PYTHONPATH=. ./.venv/bin/pytest tests/unittest/test_fix_json_escape_char.py -q` | ||
| - Full unit suite: `PYTHONPATH=. ./.venv/bin/pytest tests/unittest -v` | ||
| - Pytest auto-discovery is configured in `pyproject.toml` (`asyncio_mode = "auto"`, `testpaths = ["tests"]`); always set `PYTHONPATH=.` to avoid import errors. | ||
| - Local CLI run: `python -m pr_agent.cli --pr_url <url> review` | ||
| - Lint: project uses Ruff with `line-length = 120` (config in `pyproject.toml`); pre-commit hooks live in `.pre-commit-config.yaml`. | ||
| - Docker test target (mirror of CI): `docker build -f docker/Dockerfile --target test .` | ||
| - E2E (`tests/e2e_tests/`) and health (`tests/health_test/`) suites require provider tokens (`TOKEN_GITHUB`, `TOKEN_GITLAB`, `BITBUCKET_USERNAME`/`PASSWORD`) and are slow — only run when configured. | ||
|
|
||
| Python ≥ 3.12 is required (see `pyproject.toml`). | ||
|
|
||
| ## Architecture | ||
|
|
||
| PR-Agent is a CLI/server that runs AI-powered tools (`/review`, `/describe`, `/improve`, `/ask`, etc.) against a pull request on GitHub, GitLab, Bitbucket, Azure DevOps, Gitea, Gerrit, or local. The dispatch flow is `pr_agent/agent/pr_agent.py` → `command2class` map → tool class in `pr_agent/tools/`. Each tool is responsible for fetching the PR via a git provider, building a Jinja2 prompt, calling the model, and publishing the result. | ||
|
|
||
| ### Prompt building (the hot path) | ||
|
|
||
| Every tool follows the same shape: in `__init__` it constructs a `self.vars` dict, then passes it together with system/user prompt strings to a `TokenHandler`. At run time the prompts are rendered with `jinja2.Environment(undefined=StrictUndefined)` against `self.vars`. Adding new context to a prompt means: extend `self.vars` in the tool, then add a `{%- if my_var %}` block in the matching prompt TOML. Because templates use `StrictUndefined`, every variable referenced in the template must be present in `vars` (use `{%- if … %}` guards, never optional Jinja lookups). | ||
|
|
||
| System/user prompt strings live as TOML in `pr_agent/settings/`, loaded via Dynaconf as part of `global_settings` in `pr_agent/config_loader.py`. The mapping between tool and prompt file follows naming conventions: `pr_reviewer.py` ↔ `pr_reviewer_prompts.toml`, `pr_description.py` ↔ `pr_description_prompts.toml`, `pr_code_suggestions.py` ↔ `code_suggestions/pr_code_suggestions_prompts.toml` (and the `_not_decoupled` variant). New prompt files must also be registered in the `settings_files=[...]` list in `config_loader.py` to be loaded into `global_settings`. | ||
|
|
||
| ### Settings and runtime config | ||
|
|
||
| `get_settings()` from `pr_agent/config_loader.py` is the single accessor for configuration. It returns either a request-scoped Dynaconf object stored in `starlette_context` (server flows) or the module-level `global_settings`. Defaults live in `pr_agent/settings/configuration.toml`; per-repo overrides come from the repo's `.pr_agent.toml`, merged in `pr_agent/git_providers/utils.py::apply_repo_settings` (called once per request before tool dispatch). When introducing a new config section, add it to `configuration.toml` with comments — that file is the authoritative listing of options, and `apply_repo_settings` does a per-section merge so partial overrides work. | ||
|
|
||
| Sensitive values (API keys, tokens) come from environment variables or `.secrets.toml` (gitignored); `apply_secrets_manager_config()` optionally pulls from AWS Secrets Manager. | ||
|
|
||
| ### Git providers | ||
|
|
||
| `pr_agent/git_providers/` contains one provider per platform (GitHub, GitLab, Bitbucket variants, Azure DevOps, Gitea, Gerrit, Codecommit, local). They share the `GitProvider` interface in `git_providers/git_provider.py` (capabilities probed via `is_supported("feature")`) and are selected via `config.git_provider`. Tools should never branch on `isinstance(provider, GithubProvider)` for behavior — query `is_supported(...)` instead, since providers may stub or override features. Some prompt features (e.g. semantic file types in `/describe`) are gated on `gfm_markdown` support. | ||
|
|
||
| ### Servers and entrypoints | ||
|
|
||
| `pr_agent/servers/` hosts the webhook entrypoints (`github_app.py`, `gitlab_webhook.py`, `bitbucket_app.py`, etc.) that translate webhooks into `PRAgent.handle_request(pr_url, command)` calls. The CLI entry point is `pr_agent/cli.py` (registered as the `pr-agent` console script). | ||
|
|
||
| ### Tests | ||
|
|
||
| Unit tests in `tests/unittest/` are the right place for helpers in `pr_agent/algo/`, prompt-building logic, and provider adapters; mirror the file naming pattern (`test_<module>.py`). Use `parametrize` where the surrounding files do. The health test (`tests/health_test/main.py`) exercises `/describe`, `/review`, `/improve` against real PRs and is the canary for prompt regressions — update its expected artifacts when prompts change meaningfully. | ||
|
|
||
| ## Conventions to keep in mind | ||
|
|
||
| - Prompt and configuration TOMLs are single sources of truth. When changing behavior, update the prompts and the config defaults together; don't fork values across files. | ||
| - Conventional Commit messages; feature branches as `feature/<name>` or `fix/<issue>`. | ||
| - Don't reformat or reorder unrelated lines in prompt/config files — diffs in those files are reviewed closely and small noise is rejected. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,277 @@ | ||
| """ | ||
| Agent skills loader. | ||
|
|
||
| Discovers ``SKILL.md`` files from configured filesystem paths, parses their YAML | ||
| frontmatter, and formats them as prompt context for review/improve/describe tools. | ||
|
|
||
| A skill is a directory containing a ``SKILL.md`` file with the structure: | ||
|
|
||
| --- | ||
| name: terraform-standards | ||
| description: Use when reviewing Terraform code... | ||
| --- | ||
|
|
||
| # Terraform Review Guidance | ||
| ... | ||
|
|
||
| Activation is description-based: every discovered skill is included with its | ||
| name, description, and body. The model decides which guidance applies based on | ||
| the descriptions. | ||
|
|
||
| Resources alongside SKILL.md | ||
| ---------------------------- | ||
| The agent-skills standard supports bundled files for progressive disclosure: | ||
| ``references/`` (markdown context loaded on demand), ``scripts/`` (executables | ||
| the agent can invoke), and ``assets/`` (templates / images / data). PR-Agent | ||
| runs single-shot model calls and has no tool-use loop, so progressive disclosure | ||
| is not implementable here. Instead, this loader inlines every text resource | ||
| directly into the prompt: | ||
|
|
||
| * All ``*.md`` files in the skill directory tree (including ``references/``) | ||
| are gathered and appended after the SKILL.md body. | ||
| * ``scripts/`` and ``assets/`` subdirectories are skipped: scripts are | ||
| executables we cannot safely run from a one-shot prompt, and assets are | ||
| typically binary. Skills that depend on script execution will not work. | ||
|
|
||
| In short, this implementation supports **text-only** agent skills. | ||
| """ | ||
| from __future__ import annotations | ||
|
|
||
| import os | ||
| from dataclasses import dataclass, field | ||
| from typing import List, Optional, Tuple | ||
|
|
||
| import yaml | ||
|
|
||
| from pr_agent.config_loader import get_settings | ||
| from pr_agent.log import get_logger | ||
|
|
||
| # Approximate characters-per-token used to keep the skills block under budget. | ||
| _CHARS_PER_TOKEN = 4 | ||
| _FRONTMATTER_DELIMITER = "---" | ||
| _DEFAULT_MAX_SKILLS_TOKENS = 8000 | ||
| # Subdirectories whose contents are intentionally excluded from inlining, | ||
| # matching the agent-skills standard's executable/binary conventions. | ||
| _EXCLUDED_RESOURCE_DIRS = frozenset({"scripts", "assets"}) | ||
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class SkillResource: | ||
| """A non-SKILL.md text file bundled with a skill (e.g. references/guide.md).""" | ||
| relative_path: str | ||
| content: str | ||
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class Skill: | ||
| name: str | ||
| description: str | ||
| body: str | ||
| path: str | ||
| resources: Tuple[SkillResource, ...] = field(default_factory=tuple) | ||
|
|
||
|
|
||
| def _gather_resources(skill_md_path: str) -> Tuple[SkillResource, ...]: | ||
| """Walk the skill's directory tree and collect sibling ``*.md`` files. | ||
|
|
||
| SKILL.md itself is excluded. Subdirectories named ``scripts`` or ``assets`` | ||
| are skipped wholesale. If a nested directory contains its own SKILL.md it | ||
| is treated as a separate skill and not descended into. | ||
| """ | ||
| skill_dir = os.path.dirname(skill_md_path) | ||
| resources: List[SkillResource] = [] | ||
|
|
||
| for root, dirs, files in os.walk(skill_dir): | ||
| # Prune executable / binary subtrees per the agent-skills convention. | ||
| dirs[:] = [d for d in dirs if d not in _EXCLUDED_RESOURCE_DIRS] | ||
| # A nested skill directory is independent; do not absorb its files. | ||
| if root != skill_dir and "SKILL.md" in files: | ||
| dirs[:] = [] | ||
| continue | ||
| for filename in sorted(files): | ||
| if not filename.endswith(".md"): | ||
| continue | ||
| if root == skill_dir and filename == "SKILL.md": | ||
| continue | ||
| full = os.path.join(root, filename) | ||
| try: | ||
| with open(full, "r", encoding="utf-8") as fh: | ||
| content = fh.read() | ||
| except OSError as e: | ||
| get_logger().warning(f"Skill resource unreadable: {full} ({e})") | ||
| continue | ||
| rel = os.path.relpath(full, skill_dir) | ||
| resources.append(SkillResource(relative_path=rel, content=content)) | ||
|
|
||
| resources.sort(key=lambda r: r.relative_path) | ||
| return tuple(resources) | ||
|
|
||
|
|
||
| def _parse_skill_file(file_path: str) -> Optional[Skill]: | ||
| """Parse a single SKILL.md file. Returns None and logs a warning on malformed input.""" | ||
| try: | ||
| with open(file_path, "r", encoding="utf-8") as f: | ||
| content = f.read() | ||
| except OSError as e: | ||
| get_logger().warning(f"Skill file unreadable: {file_path} ({e})") | ||
| return None | ||
|
qodo-free-for-open-source-projects[bot] marked this conversation as resolved.
|
||
|
|
||
| lines = content.splitlines() | ||
| if not lines or lines[0].strip() != _FRONTMATTER_DELIMITER: | ||
| get_logger().warning(f"Skill file missing opening frontmatter delimiter: {file_path}") | ||
| return None | ||
| end_idx = None | ||
| for i in range(1, len(lines)): | ||
| if lines[i].strip() == _FRONTMATTER_DELIMITER: | ||
| end_idx = i | ||
| break | ||
| if end_idx is None: | ||
| get_logger().warning(f"Skill file missing closing frontmatter delimiter: {file_path}") | ||
| return None | ||
|
|
||
| frontmatter_text = "\n".join(lines[1:end_idx]) | ||
| body = "\n".join(lines[end_idx + 1 :]).strip() | ||
|
|
||
| try: | ||
| meta = yaml.safe_load(frontmatter_text) or {} | ||
| except yaml.YAMLError as e: | ||
| get_logger().warning(f"Skill frontmatter is not valid YAML: {file_path} ({e})") | ||
| return None | ||
|
|
||
| if not isinstance(meta, dict): | ||
| get_logger().warning(f"Skill frontmatter must be a mapping: {file_path}") | ||
| return None | ||
|
|
||
| name = meta.get("name") | ||
| description = meta.get("description") | ||
| if not isinstance(name, str) or not name.strip(): | ||
| get_logger().warning(f"Skill missing required 'name' field: {file_path}") | ||
| return None | ||
| if not isinstance(description, str) or not description.strip(): | ||
| get_logger().warning(f"Skill missing required 'description' field: {file_path}") | ||
| return None | ||
|
|
||
| return Skill( | ||
| name=name.strip(), | ||
| description=description.strip(), | ||
| body=body, | ||
| path=file_path, | ||
| resources=_gather_resources(file_path), | ||
| ) | ||
|
|
||
|
|
||
| def discover_skills(paths: List[str]) -> List[Skill]: | ||
| """Scan the given filesystem paths for ``*/SKILL.md`` files. | ||
|
|
||
| Each entry in ``paths`` may be either a directory containing skill | ||
| subdirectories (recursive search) or a path to a SKILL.md file directly. | ||
| Environment variables and ``~`` are expanded. Missing paths are skipped | ||
| with a warning. | ||
| """ | ||
| skills: List[Skill] = [] | ||
| seen: set = set() | ||
|
|
||
| for raw_path in paths or []: | ||
| if not isinstance(raw_path, str) or not raw_path.strip(): | ||
| continue | ||
| expanded = os.path.expanduser(os.path.expandvars(raw_path.strip())) | ||
| if not os.path.exists(expanded): | ||
| get_logger().warning(f"Skills path does not exist: {expanded}") | ||
| continue | ||
|
|
||
| if os.path.isfile(expanded): | ||
| candidates = [expanded] if os.path.basename(expanded) == "SKILL.md" else [] | ||
| else: | ||
| candidates = [] | ||
| for root, _dirs, files in os.walk(expanded): | ||
| if "SKILL.md" in files: | ||
| candidates.append(os.path.join(root, "SKILL.md")) | ||
|
qodo-free-for-open-source-projects[bot] marked this conversation as resolved.
|
||
|
|
||
| for candidate in candidates: | ||
| real = os.path.realpath(candidate) | ||
| if real in seen: | ||
| continue | ||
| seen.add(real) | ||
| skill = _parse_skill_file(candidate) | ||
| if skill is not None: | ||
| skills.append(skill) | ||
|
|
||
| skills.sort(key=lambda s: s.name) | ||
| return skills | ||
|
|
||
|
|
||
| def _format_skill(skill: Skill) -> str: | ||
| """Render a skill (and its inlined resources) as a prompt-ready string.""" | ||
| parts = [ | ||
| f"### Skill: {skill.name}", | ||
| f"When to use: {skill.description}", | ||
| "", | ||
| skill.body.rstrip(), | ||
| ] | ||
| for resource in skill.resources: | ||
| parts.append("") | ||
| parts.append(f"#### {resource.relative_path}") | ||
| parts.append(resource.content.rstrip()) | ||
| return "\n".join(parts).rstrip() | ||
|
|
||
|
|
||
| def format_skills_context(skills: List[Skill], max_tokens: int) -> str: | ||
| """Format skills into a prompt-ready string under a token budget. | ||
|
|
||
| Skills are emitted in order; once the running character count would exceed | ||
| the budget (estimated as ``max_tokens * 4`` characters), remaining skills | ||
| are dropped. If even the first skill exceeds the budget, its formatted text | ||
| is truncated and a marker appended. Returns an empty string when nothing fits. | ||
| """ | ||
| if not skills: | ||
| return "" | ||
| if max_tokens is None or max_tokens <= 0: | ||
| return "" | ||
|
|
||
| char_budget = max_tokens * _CHARS_PER_TOKEN | ||
| truncate_marker = "\n\n[truncated]" | ||
| separator = "\n\n---\n\n" | ||
| pieces: List[str] = [] | ||
| used = 0 | ||
| for skill in skills: | ||
| formatted = _format_skill(skill) | ||
| addition_len = (len(separator) if pieces else 0) + len(formatted) | ||
| if used + addition_len > char_budget: | ||
| if not pieces: | ||
| available = max(0, char_budget - len(truncate_marker)) | ||
| pieces.append(formatted[:available] + truncate_marker) | ||
| else: | ||
| get_logger().info( | ||
| f"Skills context budget reached; dropping {len(skills) - len(pieces)} skill(s)" | ||
| ) | ||
| break | ||
| pieces.append(formatted) | ||
| used += addition_len | ||
|
|
||
| return separator.join(pieces).strip() | ||
|
|
||
|
|
||
| def get_skills_context() -> str: | ||
| """Read settings, discover skills, and format them for prompt injection. | ||
|
|
||
| Returns ``''`` when skills are disabled, no paths are configured, or no | ||
| skills are found. The only swallowed error is a non-numeric override of | ||
| ``skills.max_skills_tokens``; everything else surfaces normally so genuine | ||
| bugs are not masked. | ||
| """ | ||
| settings = get_settings() | ||
| if not settings.skills.enabled: | ||
| return "" | ||
| paths = list(settings.skills.paths or []) | ||
| raw_max = settings.skills.max_skills_tokens | ||
| try: | ||
| max_tokens = int(raw_max) | ||
| except (TypeError, ValueError): | ||
| get_logger().warning( | ||
| f"Invalid skills.max_skills_tokens={raw_max!r}; falling back to {_DEFAULT_MAX_SKILLS_TOKENS}" | ||
| ) | ||
| max_tokens = _DEFAULT_MAX_SKILLS_TOKENS | ||
| skills = discover_skills(paths) | ||
| if not skills: | ||
| return "" | ||
| return format_skills_context(skills, max_tokens) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 1. All skills always injected get_skills_context() always discovers and injects every skill from skills.paths (subject only to a token cap), without selecting a relevant subset based on the PR diff/context. This violates the requirement to activate only relevant skills using the description field as the activation signal, and can bloat prompts and dilute guidance quality. Agent Prompt
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not applied — this is a documented architectural limitation of the current PR. PR-Agent dispatches single-shot model calls; there is no tool-use loop in which the model could selectively read SKILL.md based on the description field, so progressive disclosure is not implementable on the current architecture. The PR description's Limitations section calls this out, and the out-of-band design note sketches a two-pass selector (cheap weak-model call routing the diff to relevant skills, then main call with only those bodies) as the natural follow-up — intentionally out of scope here. The token budget ( |
||
Uh oh!
There was an error while loading. Please reload this page.