diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000000..711057ba3f --- /dev/null +++ b/CLAUDE.md @@ -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 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_.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/` or `fix/`. +- Don't reformat or reorder unrelated lines in prompt/config files — diffs in those files are reviewed closely and small noise is rejected. diff --git a/pr_agent/algo/skills_loader.py b/pr_agent/algo/skills_loader.py new file mode 100644 index 0000000000..3aee220526 --- /dev/null +++ b/pr_agent/algo/skills_loader.py @@ -0,0 +1,319 @@ +""" +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 starlette_context import context + +from pr_agent.algo.token_handler import TokenEncoder +from pr_agent.algo.utils import clip_tokens +from pr_agent.config_loader import get_settings +from pr_agent.log import get_logger + +_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"}) +_CONTEXT_CACHE_KEY = "skills_context" +# Per-resource-file size cap. Defence-in-depth against pathological skill +# directories (large markdown dumps, accidental inclusion of generated docs, +# or a misconfigured paths entry pointing at a directory with huge files). +_MAX_RESOURCE_FILE_BYTES = 256 * 1024 + + +@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 + resources: Tuple[SkillResource, ...] = field(default_factory=tuple) + + +def _count_tokens(text: str) -> int: + return len(TokenEncoder.get_token_encoder().encode(text)) + + +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): + dirs[:] = [d for d in dirs if d not in _EXCLUDED_RESOURCE_DIRS] + if root != skill_dir and "SKILL.md" in files: + dirs[:] = [] + continue + for filename in files: + if not filename.endswith(".md"): + continue + if root == skill_dir and filename == "SKILL.md": + continue + full = os.path.join(root, filename) + try: + size = os.path.getsize(full) + except OSError as e: + get_logger().warning(f"Skill resource unreadable: {full} ({e})") + continue + if size > _MAX_RESOURCE_FILE_BYTES: + get_logger().warning( + f"Skill resource skipped (exceeds {_MAX_RESOURCE_FILE_BYTES} bytes): {full} ({size} bytes)" + ) + continue + try: + with open(full, "r", encoding="utf-8") as fh: + content = fh.read() + except (OSError, UnicodeDecodeError) 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, UnicodeDecodeError) as e: + get_logger().warning(f"Skill file unreadable: {file_path} ({e})") + return None + + 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, + 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")) + + 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 token count would exceed the + budget, remaining skills are dropped. If the first skill alone exceeds the + budget, its formatted text is clipped via ``clip_tokens`` and a marker is + appended. Returns an empty string when nothing fits. + """ + if not skills: + return "" + if max_tokens is None or max_tokens <= 0: + return "" + + truncate_marker = "\n\n[truncated]" + separator = "\n\n---\n\n" + sep_tokens = _count_tokens(separator) + marker_tokens = _count_tokens(truncate_marker) + pieces: List[str] = [] + used = 0 + for skill in skills: + formatted = _format_skill(skill) + tokens = _count_tokens(formatted) + addition = (sep_tokens if pieces else 0) + tokens + if used + addition > max_tokens: + if not pieces: + budget = max(1, max_tokens - marker_tokens) + truncated = clip_tokens(formatted, budget, add_three_dots=False) + pieces.append(truncated + truncate_marker) + if len(skills) > 1: + get_logger().info( + f"First skill exceeded budget; truncated and dropped {len(skills) - 1} skill(s)" + ) + else: + get_logger().info( + f"Skills context budget reached; dropping {len(skills) - len(pieces)} skill(s)" + ) + break + pieces.append(formatted) + used += addition + + return separator.join(pieces).strip() + + +def _get_cached_context() -> Optional[str]: + try: + return context.get(_CONTEXT_CACHE_KEY, None) + except Exception: + return None + + +def _set_cached_context(value: str) -> None: + try: + context[_CONTEXT_CACHE_KEY] = value + except Exception: + pass + + +def get_skills_context() -> str: + """Read settings, discover skills, and format them for prompt injection. + + Memoised per request via ``starlette_context`` so the three tools that + inject ``skills_context`` (review, improve, describe) share a single + discovery + parse + format. Returns ``''`` when skills are disabled, no + paths are configured, or no skills are found. + """ + cached = _get_cached_context() + if cached is not None: + return cached + + settings = get_settings() + if not settings.skills.enabled: + _set_cached_context("") + 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) + out = format_skills_context(skills, max_tokens) if skills else "" + _set_cached_context(out) + return out diff --git a/pr_agent/git_providers/utils.py b/pr_agent/git_providers/utils.py index 1e64b9578d..4036203683 100644 --- a/pr_agent/git_providers/utils.py +++ b/pr_agent/git_providers/utils.py @@ -10,6 +10,12 @@ from pr_agent.git_providers import get_git_provider_with_context from pr_agent.log import get_logger +# Sections whose values come from host-level configuration only. Repo-supplied +# .pr_agent.toml MUST NOT be able to set these, because they expose +# filesystem-touching capabilities (e.g. skills.paths) that a malicious repo +# could otherwise abuse to read sensitive host files into the LLM prompt. +_HOST_ONLY_SETTINGS_SECTIONS = frozenset({"skills"}) + def apply_repo_settings(pr_url): os.environ["AUTO_CAST_FOR_DYNACONF"] = "false" @@ -64,6 +70,12 @@ def apply_repo_settings(pr_url): # Skip excluded items, such as forbidden to load env. get_logger().debug(f"Skipping a section: {section} which is not allowed") continue + if section.lower() in _HOST_ONLY_SETTINGS_SECTIONS: + get_logger().warning( + f"Refusing to apply host-only section [{section}] from repo settings; " + f"this section can only be configured at the host level" + ) + continue section_dict = copy.deepcopy(get_settings().as_dict().get(section, {})) for key, value in contents.items(): section_dict[key] = value diff --git a/pr_agent/settings/code_suggestions/pr_code_suggestions_prompts.toml b/pr_agent/settings/code_suggestions/pr_code_suggestions_prompts.toml index 36b4d0dcf6..e0383cf6f2 100644 --- a/pr_agent/settings/code_suggestions/pr_code_suggestions_prompts.toml +++ b/pr_agent/settings/code_suggestions/pr_code_suggestions_prompts.toml @@ -73,6 +73,15 @@ Specific guidelines for generating code suggestions: - Be aware that your input consists only of partial code segments (PR diff code), not the complete codebase. Therefore, avoid making suggestions that might duplicate existing functionality, and refrain from questioning code elements (such as variable declarations or import statements) that may be defined elsewhere in the codebase. - When mentioning code elements (variables, names, or files) in your response, surround them with backticks (`). For example: "verify that `user_id` is..." +{%- if skills_context %} + + +Organizational standards and review skills (apply the ones relevant to this PR): +====== +{{ skills_context }} +====== +{%- endif %} + {%- if extra_instructions %} diff --git a/pr_agent/settings/code_suggestions/pr_code_suggestions_prompts_not_decoupled.toml b/pr_agent/settings/code_suggestions/pr_code_suggestions_prompts_not_decoupled.toml index 6178ee23c0..5a9552a26c 100644 --- a/pr_agent/settings/code_suggestions/pr_code_suggestions_prompts_not_decoupled.toml +++ b/pr_agent/settings/code_suggestions/pr_code_suggestions_prompts_not_decoupled.toml @@ -62,6 +62,15 @@ Specific guidelines for generating code suggestions: - Note that you will only see partial code segments that were changed (diff hunks in a PR code), and not the entire codebase. Avoid suggestions that might duplicate existing functionality of the outer codebase. In addition, the absence of a definition, declaration, import, or initialization for any entity in the PR code is NEVER a basis for a suggestion. - Also note that if the code ends at an opening brace or statement that begins a new scope (like 'if', 'for', 'try'), don't treat it as incomplete. Instead, acknowledge the visible scope boundary and analyze only the code shown. +{%- if skills_context %} + + +Organizational standards and review skills (apply the ones relevant to this PR): +====== +{{ skills_context }} +====== +{%- endif %} + {%- if extra_instructions %} diff --git a/pr_agent/settings/configuration.toml b/pr_agent/settings/configuration.toml index c2533b54ec..7924383264 100644 --- a/pr_agent/settings/configuration.toml +++ b/pr_agent/settings/configuration.toml @@ -352,6 +352,19 @@ uri = "./lancedb" # url = "https://YOUR-QDRANT-URL" # api_key = "..." +[skills] +# Agent skills (SKILL.md) support: discovers SKILL.md files from the configured +# filesystem paths and injects their content into review/improve/describe prompts. +# Sibling *.md files in the skill directory tree (e.g. references/guide.md) are +# inlined alongside SKILL.md. PR-Agent supports text-only skills: scripts/ and +# assets/ subdirectories are skipped because PR-Agent uses a single-shot model +# call (no tool-use loop) and cannot execute scripts or load binary assets on +# demand. Skills that depend on script execution will not work here. +# See https://github.com/The-PR-Agent/pr-agent/issues/2384 +enabled = false +paths = [] # directories to scan recursively for "*/SKILL.md"; supports ~ and $VAR +max_skills_tokens = 8000 # token budget for the combined skills_context block + [best_practices] content = "" organization_name = "" diff --git a/pr_agent/settings/pr_description_prompts.toml b/pr_agent/settings/pr_description_prompts.toml index 2627401ea9..a6d921d922 100644 --- a/pr_agent/settings/pr_description_prompts.toml +++ b/pr_agent/settings/pr_description_prompts.toml @@ -8,6 +8,14 @@ Your task is to provide a full description for the PR content: type, description - When quoting variables, names or file paths from the code, use backticks (`) instead of single quote ('). - When needed, use '- ' as bullets +{%- if skills_context %} + +Organizational standards and review skills (apply the ones relevant to this PR): +===== +{{ skills_context }} +===== +{%- endif %} + {%- if extra_instructions %} Extra instructions from the user: diff --git a/pr_agent/settings/pr_reviewer_prompts.toml b/pr_agent/settings/pr_reviewer_prompts.toml index bbe6c6d04c..89eff5513e 100644 --- a/pr_agent/settings/pr_reviewer_prompts.toml +++ b/pr_agent/settings/pr_reviewer_prompts.toml @@ -60,6 +60,15 @@ Constructing comments: - Keep each issue description concise. Write so the reader grasps the point immediately without close reading. - Use a matter-of-fact, helpful tone. Avoid accusatory language, excessive praise, or filler phrases like 'Great job', 'Thanks for'. +{%- if skills_context %} + + +Organizational standards and review skills (apply the ones relevant to this PR): +====== +{{ skills_context }} +====== +{%- endif %} + {%- if extra_instructions %} diff --git a/pr_agent/tools/pr_code_suggestions.py b/pr_agent/tools/pr_code_suggestions.py index bbdf58e46d..5e1ef04d7a 100644 --- a/pr_agent/tools/pr_code_suggestions.py +++ b/pr_agent/tools/pr_code_suggestions.py @@ -17,6 +17,7 @@ from pr_agent.algo.pr_processing import (add_ai_metadata_to_diff_files, get_pr_diff, get_pr_multi_diffs, retry_with_fallback_models) +from pr_agent.algo.skills_loader import get_skills_context from pr_agent.algo.token_handler import TokenHandler from pr_agent.algo.utils import (ModelType, load_yaml, replace_code_tags, show_relevant_configurations, get_max_tokens, clip_tokens, get_model) @@ -66,6 +67,7 @@ def __init__(self, pr_url: str, cli_mode=False, args: list = None, "diff_no_line_numbers": "", # empty diff for initial calculation "num_code_suggestions": num_code_suggestions, "extra_instructions": get_settings().pr_code_suggestions.extra_instructions, + "skills_context": get_skills_context(), "commit_messages_str": self.git_provider.get_commit_messages(), "relevant_best_practices": "", "is_ai_metadata": get_settings().get("config.enable_ai_metadata", False), diff --git a/pr_agent/tools/pr_description.py b/pr_agent/tools/pr_description.py index 26ea5d190a..2214e95e11 100644 --- a/pr_agent/tools/pr_description.py +++ b/pr_agent/tools/pr_description.py @@ -14,6 +14,7 @@ get_pr_diff, get_pr_diff_multiple_patchs, retry_with_fallback_models) +from pr_agent.algo.skills_loader import get_skills_context from pr_agent.algo.token_handler import TokenHandler from pr_agent.algo.utils import (ModelType, PRDescriptionHeader, clip_tokens, get_max_tokens, get_user_labels, load_yaml, @@ -67,6 +68,7 @@ def __init__(self, pr_url: str, args: list = None, "language": self.main_pr_language, "diff": "", # empty diff for initial calculation "extra_instructions": get_settings().pr_description.extra_instructions, + "skills_context": get_skills_context(), "commit_messages_str": self.git_provider.get_commit_messages(), "enable_custom_labels": get_settings().config.enable_custom_labels, "custom_labels_class": "", # will be filled if necessary in 'set_custom_labels' function diff --git a/pr_agent/tools/pr_reviewer.py b/pr_agent/tools/pr_reviewer.py index c4917f3597..2995b7691d 100644 --- a/pr_agent/tools/pr_reviewer.py +++ b/pr_agent/tools/pr_reviewer.py @@ -12,6 +12,7 @@ from pr_agent.algo.pr_processing import (add_ai_metadata_to_diff_files, get_pr_diff, retry_with_fallback_models) +from pr_agent.algo.skills_loader import get_skills_context from pr_agent.algo.token_handler import TokenHandler from pr_agent.algo.utils import (ModelType, PRReviewHeader, convert_to_markdown_v2, github_action_output, @@ -92,6 +93,7 @@ def __init__(self, pr_url: str, is_answer: bool = False, is_auto: bool = False, 'question_str': question_str, 'answer_str': answer_str, "extra_instructions": get_settings().pr_reviewer.extra_instructions, + "skills_context": get_skills_context(), "commit_messages_str": self.git_provider.get_commit_messages(), "custom_labels": "", "enable_custom_labels": get_settings().config.enable_custom_labels, diff --git a/tests/unittest/test_skills_loader.py b/tests/unittest/test_skills_loader.py new file mode 100644 index 0000000000..1a7721bbaa --- /dev/null +++ b/tests/unittest/test_skills_loader.py @@ -0,0 +1,369 @@ +"""Unit tests for the agent skills loader.""" +import os +import textwrap +from pathlib import Path + +from jinja2 import Environment, StrictUndefined + +from pr_agent.algo.skills_loader import (Skill, _parse_skill_file, + discover_skills, + format_skills_context, + get_skills_context) + + +def _write_skill(directory: Path, name: str, body: str = "Body content."): + skill_dir = directory / name + skill_dir.mkdir(parents=True, exist_ok=True) + skill_file = skill_dir / "SKILL.md" + skill_file.write_text(textwrap.dedent(f"""\ + --- + name: {name} + description: Use when reviewing {name} code. + --- + + {body} + """)) + return skill_file + + +class TestParseSkillFile: + def test_parses_valid_frontmatter_and_body(self, tmp_path): + skill_file = _write_skill(tmp_path, "terraform-standards", + body="# Terraform Review\n- check tags") + skill = _parse_skill_file(str(skill_file)) + assert skill is not None + assert skill.name == "terraform-standards" + assert skill.description == "Use when reviewing terraform-standards code." + assert "Terraform Review" in skill.body + assert "- check tags" in skill.body + + def test_missing_opening_delimiter_returns_none(self, tmp_path): + f = tmp_path / "SKILL.md" + f.write_text("no frontmatter here\nname: x\n") + assert _parse_skill_file(str(f)) is None + + def test_missing_closing_delimiter_returns_none(self, tmp_path): + f = tmp_path / "SKILL.md" + f.write_text("---\nname: x\ndescription: y\nstill in frontmatter\n") + assert _parse_skill_file(str(f)) is None + + def test_invalid_yaml_returns_none(self, tmp_path): + f = tmp_path / "SKILL.md" + f.write_text("---\nname: [unclosed\n---\nbody\n") + assert _parse_skill_file(str(f)) is None + + def test_missing_required_fields_returns_none(self, tmp_path): + f = tmp_path / "SKILL.md" + f.write_text("---\nname: only-name\n---\nbody\n") + assert _parse_skill_file(str(f)) is None + + f2 = tmp_path / "SKILL2.md" + f2.write_text("---\ndescription: only desc\n---\nbody\n") + assert _parse_skill_file(str(f2)) is None + + def test_body_with_inner_dashes_preserved(self, tmp_path): + f = tmp_path / "SKILL.md" + f.write_text(textwrap.dedent("""\ + --- + name: with-dashes + description: Use when X. + --- + + # Heading + --- + section after rule + """)) + skill = _parse_skill_file(str(f)) + assert skill is not None + assert "section after rule" in skill.body + assert "---" in skill.body + + +class TestDiscoverSkills: + def test_finds_nested_skill_md_files(self, tmp_path): + _write_skill(tmp_path / "a", "alpha") + _write_skill(tmp_path / "b" / "nested", "bravo") + (tmp_path / "c").mkdir() + (tmp_path / "c" / "README.md").write_text("not a skill") + + skills = discover_skills([str(tmp_path)]) + names = {s.name for s in skills} + assert names == {"alpha", "bravo"} + + def test_skips_missing_paths_without_raising(self, tmp_path): + skills = discover_skills([str(tmp_path / "does-not-exist")]) + assert skills == [] + + def test_accepts_direct_path_to_skill_file(self, tmp_path): + skill_file = _write_skill(tmp_path, "direct") + skills = discover_skills([str(skill_file)]) + assert len(skills) == 1 + assert skills[0].name == "direct" + + def test_deduplicates_overlapping_paths(self, tmp_path): + _write_skill(tmp_path / "x", "xray") + skills = discover_skills([str(tmp_path), str(tmp_path / "x")]) + assert len(skills) == 1 + + def test_skips_malformed_files_but_returns_others(self, tmp_path): + _write_skill(tmp_path / "good", "good") + bad_dir = tmp_path / "bad" + bad_dir.mkdir() + (bad_dir / "SKILL.md").write_text("no frontmatter\n") + skills = discover_skills([str(tmp_path)]) + names = [s.name for s in skills] + assert names == ["good"] + + def test_ignores_empty_and_non_string_path_entries(self, tmp_path): + _write_skill(tmp_path, "only") + skills = discover_skills([str(tmp_path), "", None]) # type: ignore[list-item] + assert len(skills) == 1 + + +class TestFormatSkillsContext: + def _mk(self, name: str, body: str = "guidance body") -> Skill: + return Skill(name=name, description=f"Use when {name}", body=body) + + def test_returns_empty_when_no_skills(self): + assert format_skills_context([], 4000) == "" + + def test_returns_empty_when_budget_zero(self): + assert format_skills_context([self._mk("a")], 0) == "" + + def test_includes_name_description_and_body(self): + out = format_skills_context([self._mk("alpha", body="step one\nstep two")], 4000) + assert "Skill: alpha" in out + assert "When to use: Use when alpha" in out + assert "step one" in out + assert "step two" in out + + def test_drops_skills_beyond_budget(self): + skills = [self._mk(f"s{i}", body="x " * 500) for i in range(5)] + out = format_skills_context(skills, max_tokens=300) + assert "Skill: s0" in out + assert "Skill: s4" not in out + + def test_truncates_when_first_skill_exceeds_budget(self): + huge = self._mk("huge", body="y " * 5000) + out = format_skills_context([huge], max_tokens=50) + assert "[truncated]" in out + + def test_separator_between_multiple_skills(self): + out = format_skills_context( + [self._mk("a", body="A"), self._mk("b", body="B")], max_tokens=4000 + ) + assert out.count("---") >= 1 + assert out.index("Skill: a") < out.index("Skill: b") + + +class TestGetSkillsContext: + def test_disabled_returns_empty(self, tmp_path, monkeypatch): + from pr_agent.config_loader import get_settings + get_settings().set("skills", {"enabled": False, "paths": [str(tmp_path)], + "max_skills_tokens": 4000}) + assert get_skills_context() == "" + + def test_enabled_with_no_paths_returns_empty(self, monkeypatch): + from pr_agent.config_loader import get_settings + get_settings().set("skills", {"enabled": True, "paths": [], + "max_skills_tokens": 4000}) + assert get_skills_context() == "" + + def test_enabled_with_skills_returns_formatted(self, tmp_path): + _write_skill(tmp_path, "demo", body="check the thing") + from pr_agent.config_loader import get_settings + get_settings().set("skills", {"enabled": True, "paths": [str(tmp_path)], + "max_skills_tokens": 4000}) + out = get_skills_context() + assert "Skill: demo" in out + assert "check the thing" in out + + def test_invalid_max_tokens_falls_back_to_default(self, tmp_path): + _write_skill(tmp_path, "demo", body="check the thing") + from pr_agent.config_loader import get_settings + get_settings().set("skills", {"enabled": True, "paths": [str(tmp_path)], + "max_skills_tokens": "not-a-number"}) + # Should not raise; should still produce skills_context using the default budget. + out = get_skills_context() + assert "Skill: demo" in out + + +class TestJinjaSafety: + """Skills bodies often contain {{ }} or {% %} (Helm/Ansible/Terraform). + + Confirm that Jinja2 substitution is single-pass: the rendered template + contains the literal characters from the substituted variable, not a + re-evaluation of them. + """ + + def test_jinja_syntax_in_skill_body_renders_as_literal(self, tmp_path): + body = "Use {{ unknown_var }} and {% if foo %}bar{% endif %} here." + _write_skill(tmp_path, "helm", body=body) + skills = discover_skills([str(tmp_path)]) + out = format_skills_context(skills, max_tokens=4000) + + # Mirror the prompt-template injection site: a guarded {{ skills_context }}. + template = "before\n{%- if skills_context %}{{ skills_context }}{% endif %}\nafter" + env = Environment(undefined=StrictUndefined) + rendered = env.from_string(template).render(skills_context=out) + + assert "{{ unknown_var }}" in rendered + assert "{% if foo %}" in rendered + + +class TestPathExpansion: + def test_env_var_in_path_is_expanded(self, tmp_path, monkeypatch): + _write_skill(tmp_path, "envtest") + monkeypatch.setenv("SKILLS_TEST_DIR", str(tmp_path)) + skills = discover_skills(["$SKILLS_TEST_DIR"]) + assert [s.name for s in skills] == ["envtest"] + + def test_tilde_in_path_is_expanded(self, tmp_path, monkeypatch): + _write_skill(tmp_path, "homestest") + monkeypatch.setenv("HOME", str(tmp_path)) + skills = discover_skills(["~"]) + assert [s.name for s in skills] == ["homestest"] + + +class TestResourceGathering: + def test_sibling_md_file_is_inlined_as_resource(self, tmp_path): + _write_skill(tmp_path, "withrefs", body="main body") + skill_dir = tmp_path / "withrefs" + (skill_dir / "examples.md").write_text("# Examples\n- one\n- two\n") + + skills = discover_skills([str(tmp_path)]) + assert len(skills) == 1 + names = [r.relative_path for r in skills[0].resources] + assert names == ["examples.md"] + assert "- one" in skills[0].resources[0].content + + def test_references_subdirectory_is_inlined(self, tmp_path): + _write_skill(tmp_path, "withdir") + refs = tmp_path / "withdir" / "references" + refs.mkdir() + (refs / "guide.md").write_text("guide content") + (refs / "deep" / "nested").mkdir(parents=True) + (refs / "deep" / "nested" / "more.md").write_text("deeper content") + + skills = discover_skills([str(tmp_path)]) + rels = sorted(r.relative_path for r in skills[0].resources) + # Use os.sep-agnostic comparison + rels_normalised = [r.replace(os.sep, "/") for r in rels] + assert rels_normalised == ["references/deep/nested/more.md", "references/guide.md"] + + def test_scripts_and_assets_directories_are_excluded(self, tmp_path): + _write_skill(tmp_path, "secure") + skill_dir = tmp_path / "secure" + (skill_dir / "scripts").mkdir() + (skill_dir / "scripts" / "run.py").write_text("print('hi')") + (skill_dir / "scripts" / "notes.md").write_text("script notes (should be excluded)") + (skill_dir / "assets").mkdir() + (skill_dir / "assets" / "data.md").write_text("asset data (should be excluded)") + (skill_dir / "assets" / "img.svg").write_text("") + + skills = discover_skills([str(tmp_path)]) + rels = [r.relative_path for r in skills[0].resources] + assert rels == [] + + def test_nested_skill_directory_is_treated_independently(self, tmp_path): + _write_skill(tmp_path, "outer", body="outer body") + # Nested skill inside the outer skill's directory. + inner_dir = tmp_path / "outer" / "inner" + inner_dir.mkdir() + (inner_dir / "SKILL.md").write_text(textwrap.dedent("""\ + --- + name: inner + description: Use when inner. + --- + + inner body + """)) + (inner_dir / "extra.md").write_text("extra inner content") + + skills = discover_skills([str(tmp_path)]) + by_name = {s.name: s for s in skills} + assert set(by_name) == {"inner", "outer"} + # outer must not absorb the inner skill's files + outer_rels = [r.relative_path for r in by_name["outer"].resources] + assert outer_rels == [] + # inner picks up only its own sibling + inner_rels = [r.relative_path for r in by_name["inner"].resources] + assert inner_rels == ["extra.md"] + + def test_repo_settings_cannot_override_host_only_skills_section(self, monkeypatch): + """A malicious repo's .pr_agent.toml must not be able to enable skills + or set skills.paths — that would allow host-file exfiltration to the LLM. + """ + from pr_agent.config_loader import get_settings + from pr_agent.git_providers import utils as gp_utils + + get_settings().unset("skills") + get_settings().set("skills", {"enabled": False, "paths": [], + "max_skills_tokens": 8000}) + get_settings().config.use_repo_settings_file = True + + repo_toml = b'[skills]\nenabled = true\npaths = ["/etc/pwned"]\n' + + class FakeGitProvider: + def __init__(self, *a, **kw): + pass + + def get_repo_settings(self): + return repo_toml + + monkeypatch.setattr(gp_utils, "get_git_provider_with_context", + lambda _url: FakeGitProvider()) + gp_utils.apply_repo_settings("https://example.com/owner/repo/pull/1") + + assert get_settings().skills.enabled is False, \ + "Repo settings must not be able to enable skills" + assert "/etc/pwned" not in list(get_settings().skills.paths), \ + "Repo settings must not be able to inject paths" + + def test_format_skills_context_includes_resource_content(self, tmp_path): + _write_skill(tmp_path, "doc") + (tmp_path / "doc" / "checklist.md").write_text("- item one\n- item two") + skills = discover_skills([str(tmp_path)]) + out = format_skills_context(skills, max_tokens=4000) + assert "#### checklist.md" in out + assert "- item one" in out + + def test_non_utf8_skill_md_is_skipped_without_crashing(self, tmp_path): + bad = tmp_path / "broken" + bad.mkdir() + (bad / "SKILL.md").write_bytes(b"---\nname: x\ndescription: y\n---\n\n\xff\xfe invalid utf-8") + _write_skill(tmp_path, "good") + skills = discover_skills([str(tmp_path)]) + assert [s.name for s in skills] == ["good"] + + def test_non_utf8_resource_file_is_skipped_without_crashing(self, tmp_path): + _write_skill(tmp_path, "mixed") + (tmp_path / "mixed" / "good.md").write_text("readable content") + (tmp_path / "mixed" / "bad.md").write_bytes(b"\xff\xfe binary garbage") + skills = discover_skills([str(tmp_path)]) + rels = [r.relative_path for r in skills[0].resources] + assert "good.md" in rels + assert "bad.md" not in rels + + def test_oversized_resource_file_is_skipped(self, tmp_path, caplog): + _write_skill(tmp_path, "huge-res") + huge = tmp_path / "huge-res" / "huge.md" + huge.write_text("a" * (300 * 1024)) # 300 KB, above the 256 KB cap + (tmp_path / "huge-res" / "fine.md").write_text("small content") + + skills = discover_skills([str(tmp_path)]) + rels = [r.relative_path for r in skills[0].resources] + assert "fine.md" in rels + assert "huge.md" not in rels + + def test_huge_resource_is_dropped_when_skill_already_consumed_budget(self, tmp_path): + # Two skills; the second has a huge resource. Budget fits skill 1 plus + # SKILL.md of skill 2 only — so skill 2 is dropped entirely (not partially). + _write_skill(tmp_path, "first", body="first body") + _write_skill(tmp_path, "second", body="second body") + (tmp_path / "second" / "huge.md").write_text("z" * 50_000) + + skills = discover_skills([str(tmp_path)]) + out = format_skills_context(skills, max_tokens=200) # 800-char budget + assert "Skill: first" in out + assert "Skill: second" not in out