mirror of
https://github.com/Sea-Haven-Industries/open-swe.git
synced 2026-10-07 08:09:07 +00:00
feat: apply API standards skill in PR reviews for API changes (#1452)
Pull the api-standards skill from the LangSmith Context Hub at reviewer run start and inject it into the system prompt, gated on the PR adding or modifying an API surface. Best-effort: failures fall back to no supplement. Co-authored-by: GowriH-1 <218394553+GowriH-1@users.noreply.github.com> Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com>
This commit is contained in:
parent
b0cfa2cfad
commit
85049290a3
4 changed files with 130 additions and 0 deletions
|
|
@ -65,6 +65,7 @@ from .tools import (
|
||||||
web_search,
|
web_search,
|
||||||
)
|
)
|
||||||
from .utils.agents_md import fetch_agents_md
|
from .utils.agents_md import fetch_agents_md
|
||||||
|
from .utils.api_standards_skill import fetch_api_standards_skill
|
||||||
from .utils.github_app import get_github_app_installation_token_with_expiry
|
from .utils.github_app import get_github_app_installation_token_with_expiry
|
||||||
from .utils.github_token import cache_github_token_for_thread
|
from .utils.github_token import cache_github_token_for_thread
|
||||||
from .utils.model import DEFAULT_LLM_REASONING, make_model, provider_model_kwargs
|
from .utils.model import DEFAULT_LLM_REASONING, make_model, provider_model_kwargs
|
||||||
|
|
@ -278,6 +279,7 @@ def _reviewer_system_prompt(
|
||||||
org_guidelines: str | None = None,
|
org_guidelines: str | None = None,
|
||||||
repo_style_prompt: str | None = None,
|
repo_style_prompt: str | None = None,
|
||||||
agents_md_content: str | None = None,
|
agents_md_content: str | None = None,
|
||||||
|
api_standards_skill: str | None = None,
|
||||||
) -> str:
|
) -> str:
|
||||||
prompt = REVIEWER_PROMPT_TEMPLATE.format(
|
prompt = REVIEWER_PROMPT_TEMPLATE.format(
|
||||||
working_dir=working_dir,
|
working_dir=working_dir,
|
||||||
|
|
@ -322,6 +324,21 @@ def _reviewer_system_prompt(
|
||||||
f"{agents_md_content}\n"
|
f"{agents_md_content}\n"
|
||||||
"```"
|
"```"
|
||||||
)
|
)
|
||||||
|
if api_standards_skill:
|
||||||
|
prompt = (
|
||||||
|
f"{prompt}\n\n"
|
||||||
|
"# API standards skill\n\n"
|
||||||
|
"Apply this skill ONLY when the PR introduces a new API or modifies "
|
||||||
|
"an existing one (HTTP routes/handlers, RPC or GraphQL endpoints, "
|
||||||
|
"public SDK/library signatures, request/response schemas, status "
|
||||||
|
"codes, headers, or other API contracts). When the diff touches such "
|
||||||
|
"surfaces, verify the change against the best practices below and "
|
||||||
|
"file a finding when a changed line violates them and clears the "
|
||||||
|
"global bar above (anchored, concrete failure mode, in-diff). If the "
|
||||||
|
"PR does not change any API, ignore this section. Do not file "
|
||||||
|
"style-only nits or pre-existing violations outside the diff.\n\n"
|
||||||
|
f"{api_standards_skill}"
|
||||||
|
)
|
||||||
return prompt
|
return prompt
|
||||||
|
|
||||||
|
|
||||||
|
|
@ -788,6 +805,7 @@ async def get_reviewer_agent(config: RunnableConfig) -> Pregel:
|
||||||
repo_style_prompt,
|
repo_style_prompt,
|
||||||
agents_md_content,
|
agents_md_content,
|
||||||
org_guidelines,
|
org_guidelines,
|
||||||
|
api_standards_skill,
|
||||||
) = await asyncio.gather(
|
) = await asyncio.gather(
|
||||||
_fetch_diff_context(),
|
_fetch_diff_context(),
|
||||||
_fetch_pr_overview(),
|
_fetch_pr_overview(),
|
||||||
|
|
@ -795,6 +813,7 @@ async def get_reviewer_agent(config: RunnableConfig) -> Pregel:
|
||||||
_fetch_repo_style_prompt(),
|
_fetch_repo_style_prompt(),
|
||||||
_fetch_agents_md_context(),
|
_fetch_agents_md_context(),
|
||||||
_fetch_org_guidelines(),
|
_fetch_org_guidelines(),
|
||||||
|
fetch_api_standards_skill(),
|
||||||
)
|
)
|
||||||
pr_diff_text, pr_diff_line_set = diff_context
|
pr_diff_text, pr_diff_line_set = diff_context
|
||||||
pr_title, pr_body = pr_overview
|
pr_title, pr_body = pr_overview
|
||||||
|
|
@ -903,6 +922,7 @@ async def get_reviewer_agent(config: RunnableConfig) -> Pregel:
|
||||||
org_guidelines=org_guidelines,
|
org_guidelines=org_guidelines,
|
||||||
repo_style_prompt=repo_style_prompt,
|
repo_style_prompt=repo_style_prompt,
|
||||||
agents_md_content=agents_md_content,
|
agents_md_content=agents_md_content,
|
||||||
|
api_standards_skill=api_standards_skill,
|
||||||
)
|
)
|
||||||
if review_context:
|
if review_context:
|
||||||
system_prompt = f"{system_prompt}\n\n{review_context}"
|
system_prompt = f"{system_prompt}\n\n{review_context}"
|
||||||
|
|
|
||||||
52
agent/utils/api_standards_skill.py
Normal file
52
agent/utils/api_standards_skill.py
Normal file
|
|
@ -0,0 +1,52 @@
|
||||||
|
"""Fetch the API-standards skill from the LangSmith Context Hub.
|
||||||
|
|
||||||
|
The reviewer applies this skill when a PR adds or changes APIs, so it can
|
||||||
|
verify the changes against the team's API best practices. The skill lives in
|
||||||
|
the Context Hub as a skill repo (default handle ``api-standards``); we pull its
|
||||||
|
``SKILL.md`` at run start and inject it into the reviewer's system prompt.
|
||||||
|
|
||||||
|
Best-effort: any failure (missing handle, no API key, SDK error) returns
|
||||||
|
``None`` and the reviewer runs without the supplement.
|
||||||
|
"""
|
||||||
|
|
||||||
|
from __future__ import annotations
|
||||||
|
|
||||||
|
import asyncio
|
||||||
|
import logging
|
||||||
|
import os
|
||||||
|
|
||||||
|
logger = logging.getLogger(__name__)
|
||||||
|
|
||||||
|
API_STANDARDS_SKILL_HANDLE = os.environ.get("API_STANDARDS_SKILL_HANDLE", "api-standards")
|
||||||
|
|
||||||
|
|
||||||
|
def _pull_api_standards_skill_sync(handle: str) -> str | None:
|
||||||
|
from langsmith import Client as LangSmithClient
|
||||||
|
|
||||||
|
client = LangSmithClient()
|
||||||
|
skill = client.pull_skill(handle)
|
||||||
|
files = getattr(skill, "files", None) or {}
|
||||||
|
entry = files.get("SKILL.md")
|
||||||
|
content = getattr(entry, "content", None) if entry is not None else None
|
||||||
|
if isinstance(content, str) and content.strip():
|
||||||
|
return content.strip()
|
||||||
|
return None
|
||||||
|
|
||||||
|
|
||||||
|
async def fetch_api_standards_skill(handle: str | None = None) -> str | None:
|
||||||
|
"""Return the API-standards ``SKILL.md`` content, or ``None`` on any failure."""
|
||||||
|
resolved = handle or API_STANDARDS_SKILL_HANDLE
|
||||||
|
if not resolved:
|
||||||
|
return None
|
||||||
|
try:
|
||||||
|
content = await asyncio.to_thread(_pull_api_standards_skill_sync, resolved)
|
||||||
|
except Exception: # noqa: BLE001
|
||||||
|
logger.warning("Failed to pull API-standards skill '%s'", resolved, exc_info=True)
|
||||||
|
return None
|
||||||
|
if content:
|
||||||
|
logger.info(
|
||||||
|
"Loaded API-standards skill '%s' (%d chars) into reviewer prompt",
|
||||||
|
resolved,
|
||||||
|
len(content),
|
||||||
|
)
|
||||||
|
return content
|
||||||
35
tests/test_api_standards_skill.py
Normal file
35
tests/test_api_standards_skill.py
Normal file
|
|
@ -0,0 +1,35 @@
|
||||||
|
from __future__ import annotations
|
||||||
|
|
||||||
|
from unittest.mock import patch
|
||||||
|
|
||||||
|
import pytest
|
||||||
|
|
||||||
|
from agent.utils import api_standards_skill
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_fetch_returns_skill_content() -> None:
|
||||||
|
with patch.object(
|
||||||
|
api_standards_skill,
|
||||||
|
"_pull_api_standards_skill_sync",
|
||||||
|
return_value="Use /v1/ prefixes.",
|
||||||
|
):
|
||||||
|
content = await api_standards_skill.fetch_api_standards_skill("api-standards")
|
||||||
|
assert content == "Use /v1/ prefixes."
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_fetch_returns_none_on_error() -> None:
|
||||||
|
def _boom(_handle: str) -> str | None:
|
||||||
|
raise RuntimeError("no api key")
|
||||||
|
|
||||||
|
with patch.object(api_standards_skill, "_pull_api_standards_skill_sync", side_effect=_boom):
|
||||||
|
content = await api_standards_skill.fetch_api_standards_skill("api-standards")
|
||||||
|
assert content is None
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_fetch_returns_none_when_no_handle_configured() -> None:
|
||||||
|
with patch.object(api_standards_skill, "API_STANDARDS_SKILL_HANDLE", ""):
|
||||||
|
content = await api_standards_skill.fetch_api_standards_skill("")
|
||||||
|
assert content is None
|
||||||
|
|
@ -62,6 +62,29 @@ def test_reviewer_system_prompt_org_guidelines_precede_repo_style() -> None:
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def test_reviewer_system_prompt_includes_api_standards_section() -> None:
|
||||||
|
prompt = reviewer._reviewer_system_prompt(
|
||||||
|
"/workspace/repo",
|
||||||
|
repo_owner="acme",
|
||||||
|
repo_name="repo",
|
||||||
|
pr_number=42,
|
||||||
|
api_standards_skill="Always version your endpoints under /v1/.",
|
||||||
|
)
|
||||||
|
assert "API standards skill" in prompt
|
||||||
|
assert "Always version your endpoints under /v1/." in prompt
|
||||||
|
assert "introduces a new API or modifies" in prompt
|
||||||
|
|
||||||
|
|
||||||
|
def test_reviewer_system_prompt_omits_api_standards_when_absent() -> None:
|
||||||
|
prompt = reviewer._reviewer_system_prompt(
|
||||||
|
"/workspace/repo",
|
||||||
|
repo_owner="acme",
|
||||||
|
repo_name="repo",
|
||||||
|
pr_number=42,
|
||||||
|
)
|
||||||
|
assert "API standards skill" not in prompt
|
||||||
|
|
||||||
|
|
||||||
def test_finding_reply_context_wraps_reply_as_untrusted_data() -> None:
|
def test_finding_reply_context_wraps_reply_as_untrusted_data() -> None:
|
||||||
prompt = reviewer._build_finding_reply_context(
|
prompt = reviewer._build_finding_reply_context(
|
||||||
pr_url="https://github.com/acme/repo/pull/1",
|
pr_url="https://github.com/acme/repo/pull/1",
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue