From 2df0eabd35f0764cf442d0dc34cd07094d7414ac Mon Sep 17 00:00:00 2001 From: Johannes du Plessis Date: Thu, 18 Jun 2026 11:13:58 -0700 Subject: [PATCH] feat: reviewer enforces AGENTS.md/CLAUDE.md repo rules as mandatory pass (#1569) * feat: reviewer enforces AGENTS.md/CLAUDE.md repo rules as mandatory pass The reviewer already fetched AGENTS.md but treated violations as optional candidate findings. Now the reviewer runs a dedicated compliance pass that checks every changed hunk against each rule in AGENTS.md (or CLAUDE.md as fallback), treating violations as mandatory findings rather than style nits. Co-authored-by: open-swe[bot] * fix: oversized AGENTS.md returns None instead of falling back to CLAUDE.md Only a 404 (file absent) triggers fallback to CLAUDE.md. Oversize, HTTP errors, and unexpected status codes now return None immediately so the reviewer does not enforce stale rules from a secondary file. Co-authored-by: open-swe[bot] --------- Co-authored-by: open-swe[bot] --- agent/reviewer.py | 40 +++++++++++--- agent/utils/agents_md.py | 92 +++++++++++++++++++------------- tests/test_agents_md.py | 110 +++++++++++++++++++++++++++++++++++++++ tests/test_reviewer.py | 59 ++++++++++++++++++++- 4 files changed, 256 insertions(+), 45 deletions(-) create mode 100644 tests/test_agents_md.py diff --git a/agent/reviewer.py b/agent/reviewer.py index 1bc232f8..9771dfa8 100644 --- a/agent/reviewer.py +++ b/agent/reviewer.py @@ -215,6 +215,16 @@ carefully before reaching for unchanged code. 6. **Verify library / framework usage you're not certain of.** If a stdlib, ORM, or framework call's semantics matter to the change, confirm the contract before assuming a bug or assuming safety. +7. **Repository conventions compliance.** If a Repository conventions + (AGENTS.md / CLAUDE.md) section appears in this prompt, run a dedicated + pass that checks every changed hunk against each rule listed there. For + each rule, ask: *does this PR's diff violate it?* Common violations + include failing to update docs that describe changed behavior, using a + forbidden import or pattern, skipping a required test/changelog step, or + ignoring naming/architecture mandates. File a finding for each violation + that is anchored to a changed line — these are mandatory repo rules, not + style nits, so a violation is a legitimate finding even when it would + otherwise look like a convention nit. Use `add_finding` to record each candidate. Every finding must include a concise generated `title` that names the failure mode in roughly 4-10 words; @@ -390,14 +400,28 @@ def _reviewer_system_prompt( if agents_md_content: prompt = ( f"{prompt}\n\n" - "# Repository conventions (AGENTS.md)\n\n" - "The following is the `AGENTS.md` file from the target branch " - "(the PR's base), not from the PR head. It documents the " - "project's conventions, architecture, and rules. Treat " - "violations of these conventions as candidate findings when " - "they meet the global bar above (anchored to a changed line, " - "concrete failure mode, in-diff). Do not file findings for " - "pre-existing violations outside the diff.\n\n" + "# Repository conventions (AGENTS.md / CLAUDE.md)\n\n" + "The following is the `AGENTS.md` or `CLAUDE.md` file from the target " + "branch (the PR's base), not from the PR head. It documents the " + "project's conventions, architecture, and rules. These rules are " + "**mandatory** — the project enforces them on every contributor and " + "they are not optional style preferences. When a changed line " + "violates one of these rules, file a finding for it (still anchored " + "to the changed line, still a concrete failure mode, still in-diff). " + "Do not file findings for pre-existing violations outside the diff.\n\n" + "Common rule categories to check:\n" + "- **Documentation sync rules** — many repos require docs/ to be " + "updated when behavior changes. If the PR changes behavior a doc " + "describes and the doc is not updated, that is a finding.\n" + "- **Naming / convention rules** — if the repo mandates specific " + "naming, patterns, or helpers and the PR uses the wrong one, that " + "is a finding (not a style nit — it violates an explicit repo rule).\n" + "- **Architecture / layering rules** — if the repo forbids certain " + "imports, cross-layer calls, or patterns and the PR introduces one, " + "that is a finding.\n" + "- **Process / CI rules** — if the repo requires tests, changelog " + "entries, or specific CI steps for certain changes and the PR skips " + "them, that is a finding.\n\n" "```\n" f"{agents_md_content}\n" "```" diff --git a/agent/utils/agents_md.py b/agent/utils/agents_md.py index f57a4b5e..576f5e09 100644 --- a/agent/utils/agents_md.py +++ b/agent/utils/agents_md.py @@ -1,4 +1,4 @@ -"""Fetch ``AGENTS.md`` from a GitHub repo so it can be inlined into prompts. +"""Fetch ``AGENTS.md`` (or ``CLAUDE.md`` fallback) from a GitHub repo. Used by the reviewer to deterministically load repo conventions into context without the model having to clone the repo and read the file itself. @@ -12,10 +12,15 @@ import httpx logger = logging.getLogger(__name__) -# Cap the inlined content. AGENTS.md is meant to be a short conventions doc; -# anything larger is probably accidental and would bloat every reviewer prompt. +# Cap the inlined content. AGENTS.md / CLAUDE.md is meant to be a short +# conventions doc; anything larger is probably accidental and would bloat +# every reviewer prompt. _MAX_AGENTS_MD_BYTES = 64 * 1024 +# Filenames tried in order of preference. AGENTS.md is the cross-tool standard; +# CLAUDE.md is the legacy Anthropic-specific filename still used by many repos. +_AGENT_DOC_FILENAMES = ("AGENTS.md", "CLAUDE.md") + async def fetch_agents_md( owner: str, @@ -25,15 +30,17 @@ async def fetch_agents_md( token: str | None, timeout: float = 10.0, ) -> str | None: - """Fetch ``AGENTS.md`` at ``ref`` from ``owner/repo``. + """Fetch ``AGENTS.md`` (or ``CLAUDE.md`` fallback) at ``ref`` from ``owner/repo``. - Returns the raw file contents, or ``None`` if the file is missing, the - request fails, or the file exceeds the size cap. + Returns the raw file contents of the first matching file, or ``None`` if no + file is found, a fetch fails, or the file exceeds the size cap. Only a 404 + (file absent) triggers fallback to the next filename; any other condition + (oversize, HTTP error, unexpected status) returns ``None`` immediately so + the reviewer does not enforce stale rules from a secondary file. """ if not owner or not repo or not ref: return None - url = f"https://api.github.com/repos/{owner}/{repo}/contents/AGENTS.md" headers = { "Accept": "application/vnd.github.raw", "X-GitHub-Api-Version": "2022-11-28", @@ -41,33 +48,48 @@ async def fetch_agents_md( if token: headers["Authorization"] = f"Bearer {token}" - try: - async with httpx.AsyncClient(timeout=timeout) as client: - response = await client.get(url, headers=headers, params={"ref": ref}) - except httpx.HTTPError: - logger.exception("Failed to fetch AGENTS.md from %s/%s@%s", owner, repo, ref) - return None + async with httpx.AsyncClient(timeout=timeout) as client: + for filename in _AGENT_DOC_FILENAMES: + url = f"https://api.github.com/repos/{owner}/{repo}/contents/{filename}" + try: + response = await client.get(url, headers=headers, params={"ref": ref}) + except httpx.HTTPError: + logger.exception("Failed to fetch %s from %s/%s@%s", filename, owner, repo, ref) + return None - if response.status_code == 404: - return None - if response.status_code != 200: - logger.warning( - "Unexpected status %s fetching AGENTS.md from %s/%s@%s", - response.status_code, - owner, - repo, - ref, - ) - return None + if response.status_code == 404: + continue + if response.status_code != 200: + logger.warning( + "Unexpected status %s fetching %s from %s/%s@%s", + response.status_code, + filename, + owner, + repo, + ref, + ) + return None - content = response.text - if len(content.encode("utf-8")) > _MAX_AGENTS_MD_BYTES: - logger.info( - "AGENTS.md in %s/%s@%s exceeds %d bytes; skipping inline", - owner, - repo, - ref, - _MAX_AGENTS_MD_BYTES, - ) - return None - return content + content = response.text + if len(content.encode("utf-8")) > _MAX_AGENTS_MD_BYTES: + logger.info( + "%s in %s/%s@%s exceeds %d bytes; skipping inline", + filename, + owner, + repo, + ref, + _MAX_AGENTS_MD_BYTES, + ) + return None + + logger.info( + "Loaded %s (%d chars) from %s/%s@%s", + filename, + len(content), + owner, + repo, + ref, + ) + return content + + return None diff --git a/tests/test_agents_md.py b/tests/test_agents_md.py new file mode 100644 index 00000000..474c40e5 --- /dev/null +++ b/tests/test_agents_md.py @@ -0,0 +1,110 @@ +from __future__ import annotations + +from unittest.mock import AsyncMock, MagicMock, patch + +import httpx +import pytest + +from agent.utils import agents_md + + +def _make_response(status: int, text: str = "") -> MagicMock: + resp = MagicMock() + resp.status_code = status + resp.text = text + return resp + + +@pytest.mark.asyncio +async def test_fetch_agents_md_returns_content() -> None: + with patch("httpx.AsyncClient") as mock_client_cls: + client = MagicMock() + client.get = AsyncMock(return_value=_make_response(200, "# AGENTS.md\nrules")) + mock_client_cls.return_value.__aenter__ = AsyncMock(return_value=client) + mock_client_cls.return_value.__aexit__ = AsyncMock(return_value=None) + result = await agents_md.fetch_agents_md("acme", "repo", "main", token="tok") + assert result == "# AGENTS.md\nrules" + + +@pytest.mark.asyncio +async def test_fetch_agents_md_falls_back_to_claude_md() -> None: + with patch("httpx.AsyncClient") as mock_client_cls: + client = MagicMock() + client.get = AsyncMock( + side_effect=[ + _make_response(404), + _make_response(200, "# CLAUDE.md\nrules"), + ] + ) + mock_client_cls.return_value.__aenter__ = AsyncMock(return_value=client) + mock_client_cls.return_value.__aexit__ = AsyncMock(return_value=None) + result = await agents_md.fetch_agents_md("acme", "repo", "main", token="tok") + assert result == "# CLAUDE.md\nrules" + assert client.get.await_count == 2 + + +@pytest.mark.asyncio +async def test_fetch_agents_md_returns_none_when_both_missing() -> None: + with patch("httpx.AsyncClient") as mock_client_cls: + client = MagicMock() + client.get = AsyncMock( + side_effect=[ + _make_response(404), + _make_response(404), + ] + ) + mock_client_cls.return_value.__aenter__ = AsyncMock(return_value=client) + mock_client_cls.return_value.__aexit__ = AsyncMock(return_value=None) + result = await agents_md.fetch_agents_md("acme", "repo", "main", token="tok") + assert result is None + + +@pytest.mark.asyncio +async def test_fetch_agents_md_skips_oversized_file() -> None: + big = "x" * (agents_md._MAX_AGENTS_MD_BYTES + 1) + with patch("httpx.AsyncClient") as mock_client_cls: + client = MagicMock() + client.get = AsyncMock(return_value=_make_response(200, big)) + mock_client_cls.return_value.__aenter__ = AsyncMock(return_value=client) + mock_client_cls.return_value.__aexit__ = AsyncMock(return_value=None) + result = await agents_md.fetch_agents_md("acme", "repo", "main", token="tok") + assert result is None + + +@pytest.mark.asyncio +async def test_fetch_agents_md_oversized_agents_md_does_not_fall_back_to_claude_md() -> None: + big = "x" * (agents_md._MAX_AGENTS_MD_BYTES + 1) + with patch("httpx.AsyncClient") as mock_client_cls: + client = MagicMock() + client.get = AsyncMock( + side_effect=[ + _make_response(200, big), + _make_response(200, "# CLAUDE.md\nrules"), + ] + ) + mock_client_cls.return_value.__aenter__ = AsyncMock(return_value=client) + mock_client_cls.return_value.__aexit__ = AsyncMock(return_value=None) + result = await agents_md.fetch_agents_md("acme", "repo", "main", token="tok") + assert result is None + assert client.get.await_count == 1 + + +@pytest.mark.asyncio +async def test_fetch_agents_md_handles_http_error() -> None: + with patch("httpx.AsyncClient") as mock_client_cls: + client = MagicMock() + client.get = AsyncMock(side_effect=httpx.HTTPError("boom")) + mock_client_cls.return_value.__aenter__ = AsyncMock(return_value=client) + mock_client_cls.return_value.__aexit__ = AsyncMock(return_value=None) + result = await agents_md.fetch_agents_md("acme", "repo", "main", token="tok") + assert result is None + + +@pytest.mark.asyncio +async def test_fetch_agents_md_returns_none_for_missing_params() -> None: + result = await agents_md.fetch_agents_md("", "repo", "main", token="tok") + assert result is None + result = await agents_md.fetch_agents_md("acme", "", "main", token="tok") + assert result is None + result = await agents_md.fetch_agents_md("acme", "repo", "", token="tok") + assert result is None diff --git a/tests/test_reviewer.py b/tests/test_reviewer.py index 7afca801..fd7766bc 100644 --- a/tests/test_reviewer.py +++ b/tests/test_reviewer.py @@ -496,8 +496,10 @@ def test_reviewer_system_prompt_includes_agents_md_section() -> None: pr_number=42, agents_md_content="Use snake_case for all Python identifiers.", ) - assert "Repository conventions (AGENTS.md)" in prompt + assert "Repository conventions (AGENTS.md / CLAUDE.md)" in prompt assert "Use snake_case for all Python identifiers." in prompt + assert "Repository conventions compliance" in prompt + assert "mandatory repo rules" in prompt @pytest.mark.asyncio @@ -548,10 +550,63 @@ async def test_reviewer_inlines_agents_md_into_system_prompt() -> None: await reviewer.get_reviewer_agent(config) mock_fetch_agents_md.assert_awaited_once_with("acme", "repo", "base-sha-xyz", token="gh-token") - assert "Repository conventions (AGENTS.md)" in captured["system_prompt"] + assert "Repository conventions (AGENTS.md / CLAUDE.md)" in captured["system_prompt"] assert "Always use the design system IconButton." in captured["system_prompt"] +@pytest.mark.asyncio +async def test_reviewer_inlines_claude_md_when_agents_md_absent() -> None: + config: RunnableConfig = { + "configurable": { + "__is_for_execution__": True, + "thread_id": "reviewer-thread-id", + "source": "github", + "repo": {"owner": "acme", "name": "repo"}, + "pr_number": 7, + "pr_url": "https://github.com/acme/repo/pull/7", + "base_sha": "base-sha-xyz", + "head_sha": "head-sha-abc", + }, + "metadata": {}, + } + captured: dict[str, str] = {} + + def fake_create_deep_agent(*, system_prompt: str, **kwargs: object) -> _DummyAgent: + captured["system_prompt"] = system_prompt + return _DummyAgent() + + with ( + patch( + "agent.reviewer.get_github_app_installation_token_with_expiry", + new_callable=AsyncMock, + return_value=("gh-token", None), + ), + patch( + "agent.reviewer.ensure_sandbox_for_thread", + new_callable=AsyncMock, + return_value=MagicMock(), + ), + patch( + "agent.reviewer.aresolve_sandbox_work_dir", + new_callable=AsyncMock, + return_value="/workspace", + ), + patch( + "agent.reviewer.fetch_agents_md", + new_callable=AsyncMock, + return_value="# CLAUDE.md\nUse semantic tokens only.", + ) as mock_fetch_agents_md, + patch("agent.reviewer.make_model", return_value=MagicMock()), + patch("agent.reviewer.create_deep_agent", side_effect=fake_create_deep_agent), + ): + await reviewer.get_reviewer_agent(config) + + mock_fetch_agents_md.assert_awaited_once_with("acme", "repo", "base-sha-xyz", token="gh-token") + assert "Repository conventions (AGENTS.md / CLAUDE.md)" in captured["system_prompt"] + assert "Use semantic tokens only." in captured["system_prompt"] + assert "Repository conventions compliance" in captured["system_prompt"] + + def test_format_pr_review_threads_renders_resolved_and_open_threads() -> None: block = reviewer._format_pr_review_threads( [