feat(skills): agent-skills (SKILL.md) format support for review/improve/describe#2385
Open
IsmaelMartinez wants to merge 6 commits into
Open
feat(skills): agent-skills (SKILL.md) format support for review/improve/describe#2385IsmaelMartinez wants to merge 6 commits into
IsmaelMartinez wants to merge 6 commits into
Conversation
Pointer file for Claude Code: high-level prompt-building hot path, settings/Dynaconf flow, git-provider abstraction, and the unit-test command. Defers to AGENTS.md for the full repo guidelines.
Discover SKILL.md files from configured filesystem paths, parse YAML frontmatter, and inject the combined name/description/body block into the review, improve, and describe prompts as a separate skills_context variable (rendered above extra_instructions so user-supplied instructions still take precedence). Activation is description-based: every discovered skill is included with its "Use when..." description, and the model decides which guidance to apply. A token budget (skills.max_skills_tokens, default 4000) caps the injected block, dropping skills from the end if exceeded. Disabled by default; enable via [skills] in configuration. No new dependencies (uses stdlib yaml). Refs The-PR-Agent#2384.
Apply post-review fixes to the agent-skills loader:
* Inline non-SKILL.md resources. Every *.md file in the skill directory
tree (including references/ subdirectories) is now gathered and
appended to the skill body in the prompt. This is the closest analogue
to the standard's progressive-disclosure model under PR-Agent's
single-shot prompt architecture, where the model has no opportunity
to Read files mid-turn. scripts/ and assets/ subdirectories remain
excluded -- the implementation supports text-only skills, and skills
that depend on script execution will not work here. Documented in
the loader docstring and configuration.toml.
* Treat nested SKILL.md files as independent skills: their subtree is
not absorbed into the outer skill's resources.
* Switch get_skills_context to attribute-style settings access for
consistency with the rest of the codebase, narrow the catch to only
the int() coercion (programmer errors now surface), and expand
$ENV_VAR alongside ~ in configured paths.
* Bump default max_skills_tokens from 4000 to 8000 (a typical multi-skill
setup needed lifting; still user-overridable per repo).
* Clean up format_skills_context truncation (drop dead variable, slice
cleanly against the budget).
* Add tests for: Jinja2 syntax in skill bodies (confirms substitution is
single-pass and {% raw %} wrapping is unnecessary), env-var and ~
path expansion, sibling .md and references/ resource gathering,
scripts/ + assets/ exclusion, nested SKILL.md isolation, resource
rendering, and resource-aware budget enforcement.
Contributor
Review Summary by QodoAdd agent-skills (SKILL.md) format support for review/improve/describe tools
WalkthroughsDescription• Implements agent-skills (SKILL.md) format support for /review, /improve, and /describe tools • Discovers and parses SKILL.md files from configured filesystem paths with YAML frontmatter • Inlines sibling *.md resources (including references/ subdirectories) into skill context • Injects formatted skills into prompts via skills_context variable, positioned above extra_instructions • Disabled by default; gated behind [skills] enabled = true configuration with token budget control Diagramflowchart LR
A["Configured filesystem paths"] -->|discover_skills| B["SKILL.md files"]
B -->|parse frontmatter| C["Skill metadata<br/>name + description"]
B -->|gather resources| D["Sibling *.md files<br/>references/"]
C --> E["Format skills context"]
D --> E
E -->|inject via skills_context| F["Review/Improve/Describe<br/>prompts"]
F -->|render with Jinja2| G["Model receives<br/>formatted skills"]
File Changes1. pr_agent/algo/skills_loader.py
|
Contributor
Code Review by Qodo
Context used 1. No skills.installed support
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+254
to
+277
| 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.
1. All skills always injected 📎 Requirement gap ➹ Performance
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
## Issue description
Skills are injected unconditionally (all discovered skills), instead of activating only a relevant subset based on PR diff/context using the skill `description` field.
## Issue Context
Compliance requires description-based activation tied to PR context/diff (e.g., keyword/file-type prefiltering or a selection pass that uses the descriptions to choose a subset).
## Fix Focus Areas
- pr_agent/algo/skills_loader.py[254-277]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Author
There was a problem hiding this comment.
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 (max_skills_tokens, default 8000) caps the inlined block and skills past the cap are dropped with a warning.
* Cache get_skills_context() via starlette_context so the three tools
(review/improve/describe) share one discovery + parse + format per
request, mirroring the apply_repo_settings cache pattern.
* Replace the 4-chars-per-token heuristic with TokenEncoder and
clip_tokens (already in pr_agent.algo) so the budget reflects actual
model tokens.
* Log when remaining skills are dropped after the first-skill
truncation path (operational visibility).
* Remove unused Skill.path field; pass file_path directly to
_gather_resources at construction.
* Drop dead inner sorted(files) in _gather_resources -- the final
resources.sort() is authoritative.
* Fix asymmetric Jinja whitespace strip in pr_reviewer_prompts.toml
and pr_description_prompts.toml ({% endif %} -> {%- endif %}).
* Trim WHAT-narrating comments per code-review pass.
Contributor
Code Review by QodoNew Review StartedThis review has been superseded by a new analysis |
Address Qodo review findings on PR The-PR-Agent#2385: * Security: apply_repo_settings now refuses to apply the [skills] section from a repo's .pr_agent.toml. skills.paths walks the host filesystem and inlines file contents into the LLM prompt; without this gate, a malicious repo could set paths = ["/etc"] or ["~/.ssh"] in its repo settings and exfiltrate sensitive host files. The skills section is now host-level configuration only (global settings, env vars, CLI). * Reliability: per-resource file size cap (256 KB). _gather_resources now stats each candidate before opening; oversized files are skipped with a warning log rather than read into memory. Defends against pathological skill directories or accidental inclusion of generated docs that would spike memory during tool init. * Tests for both behaviours. The third Qodo finding — that get_skills_context injects every skill unconditionally rather than selecting a relevant subset based on PR context — is the documented progressive-disclosure limitation called out in the PR description's Limitations section and detailed in the out-of-band design note. A two-pass selector is the architecturally correct follow-up and is intentionally out of scope here.
Contributor
|
Persistent review updated to latest commit 10f3591 |
Comment on lines
+303
to
+317
| 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 "" |
Contributor
There was a problem hiding this comment.
1. No skills.installed support 📎 Requirement gap ≡ Correctness
Skills discovery only reads settings.skills.paths and the default config defines no skills.installed list, so centrally installed skill directories cannot be configured/scanned as required. This breaks the configured discovery contract for agent-skills and makes deployments unable to separate repo-local vs installed paths.
Agent Prompt
## Issue description
The skills feature does not support the required `[skills].installed` configuration (and only scans `[skills].paths`). This prevents configuring centrally installed skill directories.
## Issue Context
Compliance requires scanning both repo-local paths and installed organization paths.
## Fix Focus Areas
- pr_agent/algo/skills_loader.py[303-317]
- pr_agent/settings/configuration.toml[355-367]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Author
There was a problem hiding this comment.
Not applied — after the host-only gating landed in 10f35914 ([skills] is now refused from repo .pr_agent.toml), the rationale for splitting paths and installed collapses: both lists would come from the same host-level configuration source, so the two-list distinction adds config surface without enabling anything new. Multiple host-level skill directories can still be configured by adding them to paths (it is a list). The original issue #2384 sketched installed as a separate key on the assumption that repo settings could opt-in to skills; that assumption is no longer compatible with the security model on this branch. Happy to revisit if a maintainer signals the two-list shape is required.
Comment on lines
+109
to
+112
| if size > _MAX_RESOURCE_FILE_BYTES: | ||
| get_logger().warning( | ||
| f"Skill resource skipped (exceeds {_MAX_RESOURCE_FILE_BYTES} bytes): {full} ({size} bytes)" | ||
| ) |
Contributor
There was a problem hiding this comment.
2. Over-120 char lines added 📘 Rule violation ⚙ Maintainability
New code introduces long lines that likely violate the repository's Ruff line-length = 120 constraint, which can fail pre-commit/CI linting. These should be wrapped/split to satisfy style requirements.
Agent Prompt
## Issue description
Some newly added lines exceed the configured Ruff line length (120), which can cause lint failures.
## Issue Context
The repo enforces `line-length = 120` via Ruff configuration.
## Fix Focus Areas
- pr_agent/algo/skills_loader.py[109-112]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Author
There was a problem hiding this comment.
Not applied — false positive. Verified with awk 'length > 120' pr_agent/algo/skills_loader.py (empty) and git diff main...HEAD | grep ^+ | awk 'length > 121' (empty). All lines added by this PR are within the Ruff line-length = 120 constraint. The long lines reported by other tools elsewhere in the repo are pre-existing in files not touched by this diff.
UnicodeDecodeError is not a subclass of OSError, so a single non-UTF-8 SKILL.md or sibling resource would propagate out of get_skills_context() and break /review, /improve, /describe whenever skills were enabled. Both reads now catch (OSError, UnicodeDecodeError); the offending file is logged and skipped. Tests cover both SKILL.md and resource paths. Addresses Qodo finding The-PR-Agent#3 on PR The-PR-Agent#2385.
Contributor
|
Persistent review updated to latest commit 33f6e27 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements agent-skills (SKILL.md) format support for
/review,/improve, and/describe, addressing #2384. Skills are discovered from configured filesystem paths, parsed for YAML frontmatter (name+description), and injected into prompts alongsideextra_instructions. Sibling*.mdfiles in the skill directory tree (includingreferences/subdirectories) are inlined as part of the same skill;scripts/andassets/subdirectories are skipped because PR-Agent's single-shot prompt architecture cannot execute scripts or load binary assets on demand. Disabled by default; gated behind[skills] enabled = true.The value proposition is org-wide curated skill libraries distributed at the PR-Agent host level — install one set of skills, reuse across many repos. This is distinct from per-repo guidance checked into version control, which is the focus of related work in flight (see "Relationship to in-flight work" below).
Scope
Parsing, discovery, formatting, and injection of text-only agent skills from filesystem paths. Every discovered skill's body and inlined
*.mdresources are added to every prompt when the feature is enabled. Thedescriptionfield is preserved and rendered as a section header in the injected context but does not currently drive selective loading.Limitations
PR-Agent dispatches single-shot model calls — there is no tool-use loop in which the model could
Reada file mid-turn — so the agent-skills standard's progressive-disclosure model (model seesname + descriptionat startup, reads SKILL.md only on selection, readsreferences/*.mdonly on demand) is not implementable on the current architecture. Every text byte of every enabled skill is loaded on every PR. Themax_skills_tokensbudget (default 8000, user-configurable) caps the combined block; skills past the cap are dropped from the end with a warning. Skills that depend on script execution or binary assets will not work — those subdirectories are excluded from inlining and PR-Agent has no mechanism to surface them.A draft design for a two-pass selector — a cheap weak-model call that picks relevant skills before the main tool prompt — is in
AGENT_SKILLS_DESIGN.mdon this branch. That is the architecturally correct way to honor progressive disclosure within the current single-shot flow. Long-term, the in-flight MCP PRs (#2348, #2356) would offer a cleaner path: with MCP tool calling, the model could invoke aread_skill_bodytool on demand, removing the need for a separate selector pass entirely. Either follow-up is out of scope for this PR.Relationship to in-flight work
Several adjacent PRs and issues touch the broader "let OSS users supply richer custom guidance to PR-Agent" gap. This PR is intended to complement, not replace, them.
#2382(load repobest_practices.mdin OSS) addresses #2377 by reading a single file from the PR's own repository via a newGitProvider.get_pr_agent_repo_custom_file()method. Its scope is per-repo guidance, checked into version control, scoped to/improve. This PR's scope is host-level skill libraries, configured outside any one repo, available to/review,/improve, and/describe. The two are complementary: a team could use #2382 for repo-specific rules and this for shared org standards in the same deployment. There is some file-level overlap (pr_code_suggestions.py,pr_reviewer.py,configuration.toml, three prompt TOMLs add different blocks in different sections); whichever lands second will need a small rebase but not a redesign.#2304(asbach —add_repo_metadatafor AGENTS.md / QODO.md / claude.md) takes a similar shape to #2382: read named files from the PR's repo and inject. If it lands, a follow-up to this PR could optionally use the sameget_repo_file()plumbing to discover skills inside the PR's repo (for example, a.pr_agent_skills/directory checked into version control), in addition to host-level paths. This branch deliberately did not depend on #2304 to keep review surface independent.#2348and#2356(MCP tool orchestration / registry) introduce tool-use capability to PR-Agent. As noted under Limitations, this is the architecturally cleanest substrate for the progressive-disclosure half of the agent-skills standard, and is the natural home for any future "selectively load skill content" work.Issue references for context: #2384 (this proposal), #2311 (the original "skills support" inquiry), #1766 (the long-running "custom rules" thread, where the workaround pattern is to stuff guidance into
extra_instructions).Configuration
Repos can override these in their own
.pr_agent.tomlper the existing Dynaconf merge logic.Test plan
PYTHONPATH=. ./.venv/bin/pytest tests/unittest -v(363 passed locally; 31 new tests intests/unittest/test_skills_loader.pycovering parsing, discovery, formatting, budget enforcement, env-var path expansion, Jinja-syntax safety, sibling-md gathering, references/ inlining, scripts/+assets/ exclusion, and nested-SKILL.md isolation)[skills], pointpathsat a directory containing one or more SKILL.md files, runpython -m pr_agent.cli --pr_url <url> reviewagainst a real PR, confirmskills_contextreaches the rendered promptenabled = false, prompts are byte-identical to pre-feature output (the{%- if skills_context %}block produces nothing)[truncated]marker without raisingRefs: #2384, #2311, #1766. Complements: #2382, #2304, #2348, #2356.