Add agent tools to read revisions from Phabricator

agents/bug-fix/compose.yml

@@ -20,7 +20,7 @@ services:
     environment:
       - RUN_ID
       - BUG_ID=${BUG_ID:?error}
-      - BUGZILLA_MCP_URL=http://bug-fix-broker:8765/mcp
+      - BUGZILLA_MCP_URL=http://bug-fix-broker:8765/bugzilla/mcp
       - PHABRICATOR_BROKER_URL=http://bug-fix-broker:8765
       - REVISION_ID
       - COMMENT

agents/bug-fix/hackbot_agents/bug_fix/__main__.py

@@ -10,23 +10,22 @@ class AgentInputs(BaseSettings):
     bugzilla_mcp_url: str
     revision_id: int | None = None
     comment: str | None = None
-    phabricator_broker_url: str | None = None
+    phabricator_broker_url: str
     model: str | None = None
     max_turns: int | None = None
     effort: str | None = None
 
     model_config = SettingsConfigDict(extra="ignore")
 
-    @model_validator(mode="after")
-    def _broker_url_required_for_follow_up(self) -> "AgentInputs":
-        # A follow-up (revision_id set) must be able to fetch the revision's
-        # patch from the broker to check it out.
-        if self.revision_id is not None and not self.phabricator_broker_url:
-            raise ValueError(
-                "phabricator_broker_url (PHABRICATOR_BROKER_URL) is required when "
-                "revision_id is set, to check out the revision"
-            )
-        return self
+    @property
+    def phabricator_mcp_url(self) -> str:
+        """The broker's Phabricator MCP endpoint.
+
+        Derived from the broker URL rather than taken as its own input: both
+        endpoints are served by the same sidecar, so there is nothing for a
+        caller to configure independently.
+        """
+        return f"{self.phabricator_broker_url.rstrip('/')}/phabricator/mcp"
 
     @model_validator(mode="after")
     def _follow_up_with_comment(self) -> "AgentInputs":
@@ -53,6 +52,10 @@ async def main(ctx: HackbotContext) -> BugFixResult:
             "type": "http",
             "url": inputs.bugzilla_mcp_url,
         },
+        phabricator_mcp_server={
+            "type": "http",
+            "url": inputs.phabricator_mcp_url,
+        },
         source_repo=ctx.repo_path,
         fx_ctx=ctx.firefox,
         bug=inputs.bug_id,

agents/bug-fix/hackbot_agents/bug_fix/agent.py

@@ -30,6 +30,7 @@
     BUGZILLA_READ_TOOLS,
     FIREFOX_TOOLS,
     PHABRICATOR_FOLLOW_UP_ACTIONS,
+    PHABRICATOR_READ_TOOLS,
     SOURCE_WRITE_TOOLS,
     TRIAGE_AND_FIX_ACTIONS,
 )
@@ -84,6 +85,7 @@ def make_investigator() -> AgentDefinition:
 async def run_bug_fix(
     *,
     bugzilla_mcp_server: McpServerConfig,
+    phabricator_mcp_server: McpServerConfig,
     source_repo: Path,
     fx_ctx: FirefoxContext,
     bug: int,
@@ -137,6 +139,7 @@ async def run_bug_fix(
         system_prompt=system_prompt,
         mcp_servers={
             "bugzilla": bugzilla_mcp_server,
+            "phabricator": phabricator_mcp_server,
             "firefox": firefox_server,
             ACTIONS_SERVER_NAME: actions_server,
         },
@@ -152,6 +155,7 @@ async def run_bug_fix(
             "Task",
             *SOURCE_WRITE_TOOLS,
             *BUGZILLA_READ_TOOLS,
+            *PHABRICATOR_READ_TOOLS,
             *enabled_action_tools,
             *FIREFOX_TOOLS,
         ],

agents/bug-fix/hackbot_agents/bug_fix/broker.py

@@ -5,7 +5,11 @@
 reaches us at `127.0.0.1:<port>`. The agent container itself binds no
 credentials:
 
-- Bugzilla: the `bugzilla` MCP tools over `/mcp` (read-only, live during the run).
+- Bugzilla: the `bugzilla` MCP tools over `/bugzilla/mcp` (read-only, live
+  during the run).
+- Phabricator: the read-only `phabricator` MCP tools over `/phabricator/mcp`, so
+  a follow-up run can read the revision it was called on: its metadata, the
+  full comment thread, and where each inline comment sits.
 - Phabricator: `GET /phabricator/revision/{id}/patch` returns a revision's base
   commit + raw diff, so the agent can check its source tree out at the revision
   before running (see ``revision.checkout_revision``).
@@ -17,8 +21,10 @@
 import bugsy
 import uvicorn
 from agent_tools import bugzilla
+from agent_tools import phabricator as phabricator_tools
 from agent_tools.bugzilla import BugzillaContext
 from agent_tools.claude_sdk import build_sdk_server
+from agent_tools.phabricator import PhabricatorContext
 from mcp.server.streamable_http_manager import StreamableHTTPSessionManager
 from phabricator_client import PhabricatorClient, PhabricatorSettings
 from pydantic_settings import BaseSettings, SettingsConfigDict
@@ -32,15 +38,18 @@
 class BrokerInputs(BaseSettings):
     bugzilla_api_url: str
     bugzilla_api_key: str
-    phabricator_url: str
-    phabricator_api_key: str
+    phabricator: PhabricatorSettings
     host: str = "0.0.0.0"
     port: int = 8765
 
-    model_config = SettingsConfigDict(extra="ignore")
+    model_config = SettingsConfigDict(
+        extra="ignore",
+        env_nested_delimiter="_",
+        env_nested_max_split=1,
+    )
 
 
-def _phabricator_route(settings: PhabricatorSettings) -> Route:
+def _patch_endpoint(client: PhabricatorClient):
     """A read-only endpoint returning a revision's base commit + raw diff.
 
     The broker holds the Conduit key; the agent only ever sees this loopback URL,
@@ -49,7 +58,6 @@ def _phabricator_route(settings: PhabricatorSettings) -> Route:
 
     async def get_patch(request):
         revision_id = int(request.path_params["revision_id"])
-        client = PhabricatorClient(settings)
         diff = await client.query_latest_diff(revision_id)
         if diff is None:
             return JSONResponse(
@@ -66,39 +74,58 @@ async def get_patch(request):
         base_commit = await client.resolve_commit(diff.base_commit) or diff.base_commit
         return JSONResponse({"base_commit": base_commit, "raw_diff": raw_diff})
 
-    return Route("/phabricator/revision/{revision_id:int}/patch", get_patch)
+    return get_patch
+
+
+def _mcp_endpoint(manager: StreamableHTTPSessionManager):
+    """An ASGI app serving one MCP server over streamable HTTP."""
+
+    async def handler(scope, receive, send):
+        await manager.handle_request(scope, receive, send)
+
+    return handler
 
 
 def build_app(inputs: BrokerInputs) -> Starlette:
     client = bugsy.Bugsy(
         api_key=inputs.bugzilla_api_key, bugzilla_url=inputs.bugzilla_api_url
     )
-    ctx = BugzillaContext(client=client)
-    sdk_config = build_sdk_server("bugzilla", ctx, bugzilla.TOOLS)
-    mcp_server = sdk_config["instance"]
+    bugzilla_config = build_sdk_server(
+        "bugzilla", BugzillaContext(client=client), bugzilla.TOOLS
+    )
+    bugzilla_manager = StreamableHTTPSessionManager(
+        app=bugzilla_config["instance"], stateless=True
+    )
 
-    manager = StreamableHTTPSessionManager(app=mcp_server, stateless=True)
+    phabricator_client = PhabricatorClient(inputs.phabricator)
+    phabricator_config = build_sdk_server(
+        "phabricator",
+        PhabricatorContext(client=phabricator_client),
+        phabricator_tools.TOOLS,
+    )
+    phabricator_manager = StreamableHTTPSessionManager(
+        app=phabricator_config["instance"], stateless=True
+    )
 
     @asynccontextmanager
     async def lifespan(app):
-        async with manager.run():
+        async with bugzilla_manager.run(), phabricator_manager.run():
             log.info(
-                "broker ready on %s:%d (bugzilla read-only + phabricator patch)",
+                "broker ready on %s:%d (bugzilla + phabricator read-only, "
+                "phabricator patch)",
                 inputs.host,
                 inputs.port,
             )
             yield
 
-    async def mcp_handler(scope, receive, send):
-        await manager.handle_request(scope, receive, send)
-
-    phabricator_settings = PhabricatorSettings(
-        url=inputs.phabricator_url, api_key=inputs.phabricator_api_key
-    )
     return Starlette(
         routes=[
-            Mount("/mcp", app=mcp_handler),
-            _phabricator_route(phabricator_settings),
+            Mount("/bugzilla/mcp", app=_mcp_endpoint(bugzilla_manager)),
+            Mount("/phabricator/mcp", app=_mcp_endpoint(phabricator_manager)),
+            Route(
+                "/phabricator/revision/{revision_id:int}/patch",
+                _patch_endpoint(phabricator_client),
+            ),
         ],
         lifespan=lifespan,
     )

agents/bug-fix/hackbot_agents/bug_fix/config.py

@@ -10,6 +10,13 @@
     "mcp__bugzilla__download_attachment",
 ]
 
+# Phabricator MCP tool names as exposed to the agent (mcp__<server>__<tool>).
+PHABRICATOR_READ_TOOLS = [
+    "mcp__phabricator__get_revision",
+    "mcp__phabricator__get_revision_comments",
+    "mcp__phabricator__get_revision_diff",
+]
+
 # Action types that the agent may record during triage/fix runs.
 TRIAGE_AND_FIX_ACTIONS = [
     "bugzilla.update_bug",

agents/bug-fix/hackbot_agents/bug_fix/prompts/follow-up.md

@@ -6,7 +6,9 @@ Respond only to the comments quoted below. Ignore any earlier mentions of you el
 {comment}
 </comments>
 
-First investigate to understand what they are asking for, then address each one by taking the matching path:
+A quoted comment rarely stands alone: an inline one only makes sense next to the code it sits on. Before acting, use the `phabricator` tools to read D{revision_id} and its thread, and to locate the comment context so you can understand it. Your tree is at the revision's latest diff, so a comment on an older `diff_id` may point at code that has since changed.
+
+Then address each quoted comment by taking the matching path:
 
 - If it requests a code change (a fix, tweak, or follow-up to the patch): make the necessary source changes, verify them, and call phabricator_update_patch with revision_id={revision_id} so the existing revision D{revision_id} is updated.
 - If it is only a question or a request for clarification (no code change is warranted): do not edit the source or submit a patch. Investigate, then reply on the revision by calling phabricator_add_comment with revision_id={revision_id}. This posts on D{revision_id} itself; do not answer via a Bugzilla comment.

agents/bug-fix/tests/test_broker.py

@@ -1,22 +1,26 @@
-"""Tests for the broker's Phabricator patch route."""
+"""Tests for the broker's Phabricator patch route and MCP mounts."""
 
 from unittest.mock import AsyncMock
 
+import pytest
 from hackbot_agents.bug_fix import broker
 from phabricator_client import PhabricatorDiff, PhabricatorSettings
+from pydantic import ValidationError
 from starlette.applications import Starlette
+from starlette.routing import Mount, Route
 from starlette.testclient import TestClient
 
 VALID_TOKEN = "api-" + "a" * 28
 
 
-def _client(monkeypatch, fake) -> TestClient:
-    monkeypatch.setattr(broker, "PhabricatorClient", lambda settings: fake)
-    route = broker._phabricator_route(PhabricatorSettings(api_key=VALID_TOKEN))
+def _client(fake) -> TestClient:
+    route = Route(
+        "/phabricator/revision/{revision_id:int}/patch", broker._patch_endpoint(fake)
+    )
     return TestClient(Starlette(routes=[route]))
 
 
-def test_patch_route_returns_base_and_diff(monkeypatch):
+def test_patch_route_returns_base_and_diff():
     fake = AsyncMock()
     fake.query_latest_diff = AsyncMock(
         return_value=PhabricatorDiff(id=9, base_commit="base9")
@@ -25,7 +29,7 @@ def test_patch_route_returns_base_and_diff(monkeypatch):
     # The abbreviated base is expanded to a full, fetchable hash.
     fake.resolve_commit = AsyncMock(return_value="base9full")
 
-    resp = _client(monkeypatch, fake).get("/phabricator/revision/42/patch")
+    resp = _client(fake).get("/phabricator/revision/42/patch")
 
     assert resp.status_code == 200
     assert resp.json() == {
@@ -36,35 +40,88 @@ def test_patch_route_returns_base_and_diff(monkeypatch):
     fake.resolve_commit.assert_awaited_once_with("base9")
 
 
-def test_patch_route_falls_back_to_raw_base_when_unresolved(monkeypatch):
+def test_patch_route_falls_back_to_raw_base_when_unresolved():
     fake = AsyncMock()
     fake.query_latest_diff = AsyncMock(
         return_value=PhabricatorDiff(id=9, base_commit="base9")
     )
     fake.get_raw_diff = AsyncMock(return_value="diff --git a/f b/f\n")
     fake.resolve_commit = AsyncMock(return_value=None)
 
-    resp = _client(monkeypatch, fake).get("/phabricator/revision/42/patch")
+    resp = _client(fake).get("/phabricator/revision/42/patch")
 
     assert resp.status_code == 200
     assert resp.json()["base_commit"] == "base9"
 
 
-def test_patch_route_404_when_no_diff(monkeypatch):
+def test_patch_route_404_when_no_diff():
     fake = AsyncMock()
     fake.query_latest_diff = AsyncMock(return_value=None)
 
-    resp = _client(monkeypatch, fake).get("/phabricator/revision/42/patch")
+    resp = _client(fake).get("/phabricator/revision/42/patch")
 
     assert resp.status_code == 404
 
 
-def test_patch_route_404_when_no_base_commit(monkeypatch):
+def test_patch_route_404_when_no_base_commit():
     fake = AsyncMock()
     fake.query_latest_diff = AsyncMock(
         return_value=PhabricatorDiff(id=9, base_commit=None)
     )
 
-    resp = _client(monkeypatch, fake).get("/phabricator/revision/42/patch")
+    resp = _client(fake).get("/phabricator/revision/42/patch")
 
     assert resp.status_code == 404
+
+
+def _app() -> Starlette:
+    return broker.build_app(
+        broker.BrokerInputs(
+            bugzilla_api_url="https://bugzilla.example.com/rest",
+            bugzilla_api_key="bz-key",
+            phabricator=PhabricatorSettings(
+                url="https://phab.example.com", api_key=VALID_TOKEN
+            ),
+        )
+    )
+
+
+def test_inputs_embed_phabricator_config_from_flat_env_names(monkeypatch):
+    # env_nested_max_split=1 splits only on the first underscore, so
+    # PHABRICATOR_API_KEY lands on phabricator.api_key, not phabricator.api.key,
+    # and flat fields keep binding to their own exact env names.
+    monkeypatch.setenv("BUGZILLA_API_URL", "https://bugzilla.example.com/rest")
+    monkeypatch.setenv("BUGZILLA_API_KEY", "bz-key")
+    monkeypatch.setenv("PHABRICATOR_URL", "https://phab.example.com")
+    monkeypatch.setenv("PHABRICATOR_API_KEY", VALID_TOKEN)
+    monkeypatch.setenv("PHABRICATOR_TIMEOUT_SECONDS", "15")
+
+    inputs = broker.BrokerInputs()
+
+    assert inputs.bugzilla_api_url == "https://bugzilla.example.com/rest"
+    assert inputs.bugzilla_api_key == "bz-key"
+    assert inputs.phabricator.url == "https://phab.example.com"
+    assert inputs.phabricator.api_key == VALID_TOKEN
+    assert inputs.phabricator.timeout_seconds == 15
+
+
+def test_inputs_require_phabricator_config(monkeypatch):
+    # Fail at startup rather than serve tools that 401 on every call.
+    monkeypatch.setenv("BUGZILLA_API_URL", "https://bugzilla.example.com/rest")
+    monkeypatch.setenv("BUGZILLA_API_KEY", "bz-key")
+    monkeypatch.delenv("PHABRICATOR_URL", raising=False)
+    monkeypatch.delenv("PHABRICATOR_API_KEY", raising=False)
+
+    with pytest.raises(ValidationError, match="phabricator"):
+        broker.BrokerInputs()
+
+
+def test_app_serves_both_mcp_endpoints():
+    # One MCP server per domain, both wired here so no token leaves the broker.
+    mounts = {r.path for r in _app().routes if isinstance(r, Mount)}
+    assert mounts == {"/bugzilla/mcp", "/phabricator/mcp"}
+
+
+def test_app_serves_the_patch_route():
+    paths = {r.path for r in _app().routes if isinstance(r, Route)}
+    assert "/phabricator/revision/{revision_id:int}/patch" in paths