From e3913eb6a469564d20f012b4541f9181f12da82a Mon Sep 17 00:00:00 2001 From: Suhaib Mujahid Date: Sat, 4 Jul 2026 16:36:00 -0400 Subject: [PATCH 01/13] hackbot-api: track per-action apply state (run_actions table) Adds a run_actions table (one row per recorded action, with status/result/ error/applied_at) and Run.finalized_at, the schema foundation a downstream apply step needs to record what it did to each action idempotently. --- ...a2f3e4b5d6_run_actions_and_finalized_at.py | 59 +++++++++++++++++++ services/hackbot-api/app/database/models.py | 35 ++++++++++- 2 files changed, 93 insertions(+), 1 deletion(-) create mode 100644 services/hackbot-api/alembic/versions/c1a2f3e4b5d6_run_actions_and_finalized_at.py diff --git a/services/hackbot-api/alembic/versions/c1a2f3e4b5d6_run_actions_and_finalized_at.py b/services/hackbot-api/alembic/versions/c1a2f3e4b5d6_run_actions_and_finalized_at.py new file mode 100644 index 0000000000..4bb285648a --- /dev/null +++ b/services/hackbot-api/alembic/versions/c1a2f3e4b5d6_run_actions_and_finalized_at.py @@ -0,0 +1,59 @@ +"""Run actions and finalized_at. + +Revision ID: c1a2f3e4b5d6 +Revises: b5b896e1ce12 +Create Date: 2026-07-01 00:00:00.000000 + +""" + +from typing import Sequence, Union + +import sqlalchemy as sa +from alembic import op +from sqlalchemy.dialects import postgresql + +# revision identifiers, used by Alembic. +revision: str = "c1a2f3e4b5d6" +down_revision: Union[str, Sequence[str], None] = "b5b896e1ce12" +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + """Upgrade schema.""" + op.add_column( + "runs", sa.Column("finalized_at", sa.DateTime(timezone=True), nullable=True) + ) + + op.create_table( + "run_actions", + sa.Column("id", sa.Integer(), autoincrement=True, nullable=False), + sa.Column("run_id", sa.UUID(), nullable=False), + sa.Column("idx", sa.Integer(), nullable=False), + sa.Column("type", sa.String(), nullable=False), + sa.Column("params", postgresql.JSONB(astext_type=sa.Text()), nullable=False), + sa.Column("ref", sa.String(), nullable=True), + sa.Column("status", sa.String(), nullable=False), + sa.Column("result", postgresql.JSONB(astext_type=sa.Text()), nullable=True), + sa.Column("error", sa.Text(), nullable=True), + sa.Column( + "created_at", + sa.DateTime(timezone=True), + server_default=sa.text("now()"), + nullable=False, + ), + sa.Column("applied_at", sa.DateTime(timezone=True), nullable=True), + sa.ForeignKeyConstraint(["run_id"], ["runs.run_id"]), + sa.PrimaryKeyConstraint("id"), + sa.UniqueConstraint("run_id", "idx", name="uq_run_actions_run_idx"), + ) + op.create_index( + op.f("ix_run_actions_run_id"), "run_actions", ["run_id"], unique=False + ) + + +def downgrade() -> None: + """Downgrade schema.""" + op.drop_index(op.f("ix_run_actions_run_id"), table_name="run_actions") + op.drop_table("run_actions") + op.drop_column("runs", "finalized_at") diff --git a/services/hackbot-api/app/database/models.py b/services/hackbot-api/app/database/models.py index 0f566fb035..ac13bfd4a2 100644 --- a/services/hackbot-api/app/database/models.py +++ b/services/hackbot-api/app/database/models.py @@ -1,7 +1,7 @@ from datetime import datetime from uuid import UUID -from sqlalchemy import DateTime, String, Text +from sqlalchemy import DateTime, ForeignKey, Integer, String, Text, UniqueConstraint from sqlalchemy.dialects.postgresql import JSONB from sqlalchemy.dialects.postgresql import UUID as PG_UUID from sqlalchemy.ext.asyncio import AsyncAttrs @@ -35,3 +35,36 @@ class Run(Base): summary: Mapped[dict | None] = mapped_column(JSONB, nullable=True) artifacts: Mapped[list] = mapped_column(JSONB, nullable=False, default=list) error: Mapped[str | None] = mapped_column(Text, nullable=True) + finalized_at: Mapped[datetime | None] = mapped_column( + DateTime(timezone=True), nullable=True + ) + + +class RunAction(Base): + """A single agent-recorded action from a run's summary.json, and its apply state. + + One row per entry in `summary.json["actions"]`, upserted by the action-applier + the first time it sees a run so replays (Pub/Sub at-least-once delivery) can + skip actions already `applied` and only retry `pending`/`failed` ones. + """ + + __tablename__ = "run_actions" + __table_args__ = (UniqueConstraint("run_id", "idx", name="uq_run_actions_run_idx"),) + + id: Mapped[int] = mapped_column(Integer, primary_key=True, autoincrement=True) + run_id: Mapped[UUID] = mapped_column( + PG_UUID(as_uuid=True), ForeignKey("runs.run_id"), nullable=False, index=True + ) + idx: Mapped[int] = mapped_column(Integer, nullable=False) + type: Mapped[str] = mapped_column(String, nullable=False) + params: Mapped[dict] = mapped_column(JSONB, nullable=False) + ref: Mapped[str | None] = mapped_column(String, nullable=True) + status: Mapped[str] = mapped_column(String, nullable=False, default="pending") + result: Mapped[dict | None] = mapped_column(JSONB, nullable=True) + error: Mapped[str | None] = mapped_column(Text, nullable=True) + created_at: Mapped[datetime] = mapped_column( + DateTime(timezone=True), server_default=func.now() + ) + applied_at: Mapped[datetime | None] = mapped_column( + DateTime(timezone=True), nullable=True + ) From 0a3ff4e24516738cf353e74efcd5f6cef15179ae Mon Sep 17 00:00:00 2001 From: Suhaib Mujahid Date: Sat, 4 Jul 2026 16:37:00 -0400 Subject: [PATCH 02/13] hackbot-runtime: apply-side handler framework + Bugzilla REST handlers Agents could already record bugzilla.* actions, but nothing turned a recorded action back into a real API call. Adds that apply side: an ActionHandler protocol + ApplyContext (handlers never touch storage directly, only the download_artifact the caller provides), a HANDLERS registry keyed by dotted action type, and Bugzilla REST implementations for update_bug/add_comment/ add_attachment/create_bug. Also adds an optional `ref` on recorded actions so a later action can reference an earlier one's apply-time result (consumed by the applier's placeholder resolution). Bugzilla is talked to directly via requests rather than bugbug.bugzilla/libmozdata, whose bulk futures-based read pipelines don't fit a single-action write. --- .../actions/handlers/__init__.py | 23 +++ .../hackbot_runtime/actions/handlers/base.py | 50 +++++++ .../actions/handlers/bugzilla_handler.py | 136 ++++++++++++++++++ .../actions/handlers/registry.py | 21 +++ .../hackbot_runtime/actions/recorder.py | 10 ++ .../tests/test_bugzilla_handler.py | 102 +++++++++++++ libs/hackbot-runtime/tests/test_recorder.py | 12 ++ 7 files changed, 354 insertions(+) create mode 100644 libs/hackbot-runtime/hackbot_runtime/actions/handlers/__init__.py create mode 100644 libs/hackbot-runtime/hackbot_runtime/actions/handlers/base.py create mode 100644 libs/hackbot-runtime/hackbot_runtime/actions/handlers/bugzilla_handler.py create mode 100644 libs/hackbot-runtime/hackbot_runtime/actions/handlers/registry.py create mode 100644 libs/hackbot-runtime/tests/test_bugzilla_handler.py diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/handlers/__init__.py b/libs/hackbot-runtime/hackbot_runtime/actions/handlers/__init__.py new file mode 100644 index 0000000000..702928eec7 --- /dev/null +++ b/libs/hackbot-runtime/hackbot_runtime/actions/handlers/__init__.py @@ -0,0 +1,23 @@ +"""Apply-side handlers for recorded actions. + +``actions/bugzilla.py`` and ``actions/phabricator.py`` (sibling package) let an +agent *record* an intent into ``summary.json``; the handlers here turn a +recorded action back into a real API call once a run has finished. Kept in the +same library so the set of action types an agent can request and the set this +package knows how to apply never drift apart. +""" + +from hackbot_runtime.actions.handlers.base import ( + ActionHandler, + ActionResult, + ApplyContext, +) +from hackbot_runtime.actions.handlers.registry import HANDLERS, get_handler + +__all__ = [ + "ActionHandler", + "ActionResult", + "ApplyContext", + "HANDLERS", + "get_handler", +] diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/handlers/base.py b/libs/hackbot-runtime/hackbot_runtime/actions/handlers/base.py new file mode 100644 index 0000000000..c5b0615024 --- /dev/null +++ b/libs/hackbot-runtime/hackbot_runtime/actions/handlers/base.py @@ -0,0 +1,50 @@ +from __future__ import annotations + +from collections.abc import Awaitable, Callable +from dataclasses import dataclass, field +from typing import Any, Protocol + + +@dataclass +class ApplyContext: + """Everything an :class:`ActionHandler` needs from the run. + + Scoped to a single action application (not the whole run), since only + ``attachments`` varies per action. Handlers never talk to GCS directly — + ``download_artifact`` is provided by the caller (hackbot-api's + ``/internal/events/apply-run-actions`` route) — so this package stays free of + a dependency on any particular storage backend. Async, matching + hackbot-api's own GCS wrappers and ``ActionHandler.apply`` itself. + """ + + run_id: str + download_artifact: Callable[[str], Awaitable[bytes]] + attachments: list[dict[str, str]] = field(default_factory=list) + + def artifact_key(self, name: str) -> str | None: + """The uploaded key for an attachment recorded under ``name``, if any.""" + for attachment in self.attachments: + if attachment.get("name") == name: + return attachment.get("uploaded_key") + return None + + +@dataclass +class ActionResult: + status: str # "applied" | "failed" + result: dict[str, Any] | None = None + error: str | None = None + + @classmethod + def ok(cls, result: dict[str, Any] | None = None) -> ActionResult: + return cls(status="applied", result=result) + + @classmethod + def failed(cls, error: str) -> ActionResult: + return cls(status="failed", error=error) + + +class ActionHandler(Protocol): + async def apply( + self, params: dict[str, Any], ctx: ApplyContext + ) -> ActionResult: ... diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/handlers/bugzilla_handler.py b/libs/hackbot-runtime/hackbot_runtime/actions/handlers/bugzilla_handler.py new file mode 100644 index 0000000000..678b493838 --- /dev/null +++ b/libs/hackbot-runtime/hackbot_runtime/actions/handlers/bugzilla_handler.py @@ -0,0 +1,136 @@ +"""Apply-side Bugzilla actions: turns a recorded intent into a real REST call. + +Pairs with the recording side in ``actions/bugzilla.py`` — one handler per +action type recorded there. Talks to Bugzilla's REST API directly (not via +``bugbug.bugzilla``/``libmozdata``, which are built around bulk, futures-based +read pipelines): a single-bug write from an event-driven handler doesn't fit +that shape, and a plain ``requests`` call is simpler to reason about and test. +""" + +from __future__ import annotations + +import base64 +import logging +import os +from functools import lru_cache +from typing import Any + +import requests + +from hackbot_runtime.actions.handlers.base import ActionResult, ApplyContext + +log = logging.getLogger(__name__) + +_DEFAULT_BUGZILLA_URL = "https://bugzilla.mozilla.org" +_TIMEOUT_SECONDS = 30 + + +@lru_cache(maxsize=1) +def _base_url() -> str: + return os.environ.get("BUGZILLA_URL", _DEFAULT_BUGZILLA_URL).rstrip("/") + "/rest" + + +def _bug_url(bug_id: int) -> str: + root = os.environ.get("BUGZILLA_URL", _DEFAULT_BUGZILLA_URL).rstrip("/") + return f"{root}/show_bug.cgi?id={bug_id}" + + +def _headers() -> dict[str, str]: + token = os.environ.get("BUGZILLA_TOKEN", "") + if not token: + raise RuntimeError("BUGZILLA_TOKEN is not configured") + return {"X-Bugzilla-API-Key": token, "Content-Type": "application/json"} + + +def _request(method: str, path: str, json_body: dict[str, Any]) -> dict[str, Any]: + response = requests.request( + method, + f"{_base_url()}/{path}", + json=json_body, + headers=_headers(), + timeout=_TIMEOUT_SECONDS, + ) + response.raise_for_status() + return response.json() + + +class UpdateBugHandler: + async def apply(self, params: dict[str, Any], ctx: ApplyContext) -> ActionResult: + bug_id = params["bug_id"] + try: + _request("PUT", f"bug/{bug_id}", params["changes"]) + except Exception as exc: + log.exception("Failed to update bug %s", bug_id) + return ActionResult.failed(str(exc)) + return ActionResult.ok({"bug_id": bug_id, "url": _bug_url(bug_id)}) + + +class AddCommentHandler: + async def apply(self, params: dict[str, Any], ctx: ApplyContext) -> ActionResult: + bug_id = params["bug_id"] + body = { + "comment": { + "body": params["text"], + "is_private": params.get("is_private", False), + } + } + try: + _request("PUT", f"bug/{bug_id}", body) + except Exception as exc: + log.exception("Failed to add comment to bug %s", bug_id) + return ActionResult.failed(str(exc)) + return ActionResult.ok({"bug_id": bug_id, "url": _bug_url(bug_id)}) + + +class AddAttachmentHandler: + async def apply(self, params: dict[str, Any], ctx: ApplyContext) -> ActionResult: + bug_id = params["bug_id"] + key = ctx.artifact_key("file") + if key is None: + return ActionResult.failed("No 'file' attachment recorded for this action") + + try: + content = await ctx.download_artifact(key) + except Exception as exc: + log.exception("Failed to download attachment artifact %s", key) + return ActionResult.failed(str(exc)) + + body: dict[str, Any] = { + "ids": [bug_id], + "data": base64.b64encode(content).decode("ascii"), + "file_name": params["file_name"], + "summary": params["summary"], + "content_type": params["content_type"], + "is_patch": params.get("is_patch", False), + } + if params.get("comment"): + body["comment"] = params["comment"] + + try: + data = _request("POST", f"bug/{bug_id}/attachment", body) + except Exception as exc: + log.exception("Failed to attach file to bug %s", bug_id) + return ActionResult.failed(str(exc)) + + attachment_ids = data.get("ids") or [] + return ActionResult.ok( + { + "bug_id": bug_id, + "url": _bug_url(bug_id), + "attachment_id": attachment_ids[0] if attachment_ids else None, + } + ) + + +class CreateBugHandler: + async def apply(self, params: dict[str, Any], ctx: ApplyContext) -> ActionResult: + try: + data = _request("POST", "bug", params) + except Exception as exc: + log.exception("Failed to create bug: %s", params.get("summary")) + return ActionResult.failed(str(exc)) + + bug_id = data.get("id") + return ActionResult.ok( + {"bug_id": bug_id, "url": _bug_url(bug_id) if bug_id else None} + ) diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/handlers/registry.py b/libs/hackbot-runtime/hackbot_runtime/actions/handlers/registry.py new file mode 100644 index 0000000000..847c7db785 --- /dev/null +++ b/libs/hackbot-runtime/hackbot_runtime/actions/handlers/registry.py @@ -0,0 +1,21 @@ +from hackbot_runtime.actions.handlers.base import ActionHandler +from hackbot_runtime.actions.handlers.bugzilla_handler import ( + AddAttachmentHandler, + AddCommentHandler, + CreateBugHandler, + UpdateBugHandler, +) + +# Maps a recorded action's dotted `type` to the handler that applies it. +# Adding a new action type later is a one-line addition here — the dispatch +# loop (see the apply-run-actions route) never changes. +HANDLERS: dict[str, ActionHandler] = { + "bugzilla.update_bug": UpdateBugHandler(), + "bugzilla.add_comment": AddCommentHandler(), + "bugzilla.add_attachment": AddAttachmentHandler(), + "bugzilla.create_bug": CreateBugHandler(), +} + + +def get_handler(action_type: str) -> ActionHandler | None: + return HANDLERS.get(action_type) diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/recorder.py b/libs/hackbot-runtime/hackbot_runtime/actions/recorder.py index d7bae0cadb..31710d8547 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/recorder.py +++ b/libs/hackbot-runtime/hackbot_runtime/actions/recorder.py @@ -32,6 +32,7 @@ def record( *, reasoning: str | None = None, attachments: dict[str, Path] | None = None, + ref: str | None = None, ) -> dict: """Record an intended action. @@ -44,6 +45,13 @@ def record( artifacts directory (so it is retrievable from compose/direct runs). The recorded action references it by that key; the original local path is not persisted (it disappears with the container). + + ``ref`` optionally labels this action so a *later* action in the same + run can reference its apply-time result (e.g. a Bugzilla comment's + text containing ``{{actions.patch.url}}`` after a + ``phabricator.submit_patch`` action recorded with ``ref="patch"``). + Resolved by the apply step, since the result doesn't exist yet at + record time. """ idx = len(self._actions) action: dict = { @@ -51,6 +59,8 @@ def record( "params": params, "reasoning": reasoning, } + if ref is not None: + action["ref"] = ref if attachments: recorded_attachments: list[dict] = [] diff --git a/libs/hackbot-runtime/tests/test_bugzilla_handler.py b/libs/hackbot-runtime/tests/test_bugzilla_handler.py new file mode 100644 index 0000000000..08560c5c96 --- /dev/null +++ b/libs/hackbot-runtime/tests/test_bugzilla_handler.py @@ -0,0 +1,102 @@ +"""Tests for the apply-side Bugzilla action handlers. + +Mocks the Bugzilla REST call each handler performs (`_request`) so these +exercise the handlers' own logic — request construction, result parsing, +error handling — without touching a network. +""" + +import base64 + +from hackbot_runtime.actions.handlers import ApplyContext, bugzilla_handler + + +def _ctx(attachments=None, artifacts=None): + artifacts = artifacts or {} + + async def download(key): + return artifacts[key] + + return ApplyContext( + run_id="run-1", download_artifact=download, attachments=attachments or [] + ) + + +async def test_update_bug_handler_success(monkeypatch): + calls = [] + monkeypatch.setattr( + bugzilla_handler, + "_request", + lambda m, p, b: calls.append((m, p, b)) or {"bugs": [{"id": 1}]}, + ) + result = await bugzilla_handler.UpdateBugHandler().apply( + {"bug_id": 1, "changes": {"status": "RESOLVED"}}, _ctx() + ) + assert result.status == "applied" + assert result.result["bug_id"] == 1 + assert calls == [("PUT", "bug/1", {"status": "RESOLVED"})] + + +async def test_update_bug_handler_failure(monkeypatch): + def _raise(*_args): + raise RuntimeError("boom") + + monkeypatch.setattr(bugzilla_handler, "_request", _raise) + result = await bugzilla_handler.UpdateBugHandler().apply( + {"bug_id": 1, "changes": {}}, _ctx() + ) + assert result.status == "failed" + assert "boom" in result.error + + +async def test_add_comment_handler_builds_comment_body(monkeypatch): + calls = [] + monkeypatch.setattr( + bugzilla_handler, "_request", lambda m, p, b: calls.append((m, p, b)) + ) + await bugzilla_handler.AddCommentHandler().apply( + {"bug_id": 5, "text": "hi", "is_private": True}, _ctx() + ) + assert calls == [("PUT", "bug/5", {"comment": {"body": "hi", "is_private": True}})] + + +async def test_add_attachment_handler_downloads_and_base64_encodes(monkeypatch): + seen = {} + monkeypatch.setattr( + bugzilla_handler, + "_request", + lambda m, p, b: (seen.update(b), {"ids": [99]})[1], + ) + ctx = _ctx( + attachments=[{"name": "file", "uploaded_key": "attachments/0/file"}], + artifacts={"attachments/0/file": b"diff content"}, + ) + result = await bugzilla_handler.AddAttachmentHandler().apply( + { + "bug_id": 5, + "file_name": "fix.patch", + "summary": "fix", + "content_type": "text/plain", + "is_patch": True, + }, + ctx, + ) + assert result.status == "applied" + assert result.result["attachment_id"] == 99 + assert base64.b64decode(seen["data"]) == b"diff content" + + +async def test_add_attachment_handler_missing_attachment(): + result = await bugzilla_handler.AddAttachmentHandler().apply( + {"bug_id": 5, "file_name": "x", "summary": "x", "content_type": "text/plain"}, + _ctx(), + ) + assert result.status == "failed" + + +async def test_create_bug_handler_success(monkeypatch): + monkeypatch.setattr(bugzilla_handler, "_request", lambda m, p, b: {"id": 42}) + result = await bugzilla_handler.CreateBugHandler().apply( + {"product": "Core", "component": "General", "summary": "s"}, _ctx() + ) + assert result.status == "applied" + assert result.result["bug_id"] == 42 diff --git a/libs/hackbot-runtime/tests/test_recorder.py b/libs/hackbot-runtime/tests/test_recorder.py index 17d4dbda3c..9359857cd8 100644 --- a/libs/hackbot-runtime/tests/test_recorder.py +++ b/libs/hackbot-runtime/tests/test_recorder.py @@ -76,3 +76,15 @@ def test_attachment_key_uses_action_index(tmp_path): rec.record("bugzilla.update_bug", {"bug_id": 1}) rec.record("bugzilla.add_attachment", {"bug_id": 1}, attachments={"file": src}) assert rec.actions[1]["attachments"][0]["uploaded_key"] == "attachments/1/file" + + +def test_ref_included_when_given(): + rec = ActionsRecorder() + rec.record("phabricator.submit_patch", {"bug_id": 1}, ref="patch") + assert rec.actions[0]["ref"] == "patch" + + +def test_ref_omitted_when_not_given(): + rec = ActionsRecorder() + rec.record("bugzilla.update_bug", {"bug_id": 1}) + assert "ref" not in rec.actions[0] From 218e6fdf36e56c9fca7a99a6865d7d33dc2d8d0b Mon Sep 17 00:00:00 2001 From: Suhaib Mujahid Date: Sat, 4 Jul 2026 16:37:49 -0400 Subject: [PATCH 03/13] hackbot-runtime: Phabricator patch submission via the Conduit API Records a phabricator.submit_patch action and, at apply time, submits it to Phabricator with no local repo, checkout, or moz-phab CLI needed. The submission payload is built during the agent's own run (build_phabricator_diff in changes.py, wired into publish_changes), reusing moz-phab's own diff-building code (mozphab.git/mozphab.diff, imported as a library via the new [phabricator] extra) against the checkout the agent already has. A synthetic commit squashes base..HEAD so moz-phab's single-commit diff works over the whole change; it carries an explicit git identity so it succeeds even in a hardened container. The result is published as the changes/phabricator_diff.json artifact. The apply-side handler is then a thin relay: download that artifact and make two Conduit calls (differential.creatediff + differential.revision.edit), create vs update keyed on whether an explicit revision_id was recorded. repositoryPHID is resolved apply-side since it's environment-specific. Nothing on the apply path imports git or moz-phab. --- .../bug-fix/hackbot_agents/bug_fix/config.py | 1 + agents/bug-fix/pyproject.toml | 2 +- .../hackbot_runtime/actions/__init__.py | 4 +- .../hackbot_runtime/actions/claude_sdk.py | 3 +- .../actions/handlers/phabricator_handler.py | 144 +++++++++++++++ .../actions/handlers/registry.py | 2 + .../hackbot_runtime/actions/phabricator.py | 101 +++++++++++ .../hackbot_runtime/changes.py | 99 ++++++++++- .../hackbot_runtime/context.py | 23 ++- libs/hackbot-runtime/pyproject.toml | 4 + libs/hackbot-runtime/tests/test_changes.py | 135 ++++++++++++++ libs/hackbot-runtime/tests/test_context.py | 59 +++++++ .../tests/test_phabricator_actions.py | 48 +++++ .../tests/test_phabricator_handler.py | 165 ++++++++++++++++++ uv.lock | 84 ++++++++- 15 files changed, 865 insertions(+), 9 deletions(-) create mode 100644 libs/hackbot-runtime/hackbot_runtime/actions/handlers/phabricator_handler.py create mode 100644 libs/hackbot-runtime/hackbot_runtime/actions/phabricator.py create mode 100644 libs/hackbot-runtime/tests/test_changes.py create mode 100644 libs/hackbot-runtime/tests/test_phabricator_actions.py create mode 100644 libs/hackbot-runtime/tests/test_phabricator_handler.py diff --git a/agents/bug-fix/hackbot_agents/bug_fix/config.py b/agents/bug-fix/hackbot_agents/bug_fix/config.py index 61524f36e2..8509d3ee9a 100644 --- a/agents/bug-fix/hackbot_agents/bug_fix/config.py +++ b/agents/bug-fix/hackbot_agents/bug_fix/config.py @@ -17,6 +17,7 @@ "bugzilla.add_comment", "bugzilla.add_attachment", "bugzilla.create_bug", + "phabricator.submit_patch", ] # Firefox build/test tools. diff --git a/agents/bug-fix/pyproject.toml b/agents/bug-fix/pyproject.toml index 492df37f36..061d789b3b 100644 --- a/agents/bug-fix/pyproject.toml +++ b/agents/bug-fix/pyproject.toml @@ -4,7 +4,7 @@ version = "0.1.0" description = "Cloud Run Job image that runs the bug-fix agent for hackbot-api" requires-python = ">=3.12" dependencies = [ - "hackbot-runtime[claude-sdk]", + "hackbot-runtime[claude-sdk,phabricator]", "agent-tools[bugzilla,firefox]", "bugsy", "claude-agent-sdk>=0.1.30", diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/__init__.py b/libs/hackbot-runtime/hackbot_runtime/actions/__init__.py index 829cdebeb6..e38385139b 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/__init__.py +++ b/libs/hackbot-runtime/hackbot_runtime/actions/__init__.py @@ -7,9 +7,9 @@ claude-sdk adapter is ``hackbot_runtime.actions.claude_sdk.actions_server_for``. """ -from hackbot_runtime.actions import bugzilla +from hackbot_runtime.actions import bugzilla, phabricator from hackbot_runtime.actions.recorder import ActionsRecorder ACTIONS_SERVER_NAME = "actions" -__all__ = ["ACTIONS_SERVER_NAME", "ActionsRecorder", "bugzilla"] +__all__ = ["ACTIONS_SERVER_NAME", "ActionsRecorder", "bugzilla", "phabricator"] diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/claude_sdk.py b/libs/hackbot-runtime/hackbot_runtime/actions/claude_sdk.py index a6cafc94d6..d52c73e778 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/claude_sdk.py +++ b/libs/hackbot-runtime/hackbot_runtime/actions/claude_sdk.py @@ -14,6 +14,7 @@ from hackbot_runtime.actions import ACTIONS_SERVER_NAME from hackbot_runtime.actions import bugzilla as _bugzilla +from hackbot_runtime.actions import phabricator as _phabricator from hackbot_runtime.actions.recorder import ActionsRecorder @@ -32,7 +33,7 @@ def actions_server_for( """ if recorder is None: recorder = ActionsRecorder(artifacts_dir=fallback_artifacts_dir) - tools = _bugzilla.TOOLS + tools = _bugzilla.TOOLS + _phabricator.TOOLS if types is not None: wanted = set(types) tools = [t for t in tools if t.dotted in wanted] diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/handlers/phabricator_handler.py b/libs/hackbot-runtime/hackbot_runtime/actions/handlers/phabricator_handler.py new file mode 100644 index 0000000000..c7a92f0b3a --- /dev/null +++ b/libs/hackbot-runtime/hackbot_runtime/actions/handlers/phabricator_handler.py @@ -0,0 +1,144 @@ +"""Apply-side Phabricator action: submits an already-built diff payload. + +Pairs with the recording side in ``actions/phabricator.py`` and the payload +built agent-side in ``hackbot_runtime.changes.build_phabricator_diff`` (while +the agent still has its own checkout — nothing here ever touches git, a +local repo, or ``moz-phab``). Talks to Phabricator's Conduit API directly +with a small ``requests``-based client, mirroring ``bugzilla_handler.py``'s +choice to avoid ``libmozdata``'s heavier, bulk/futures-oriented client for a +single lightweight call. +""" + +from __future__ import annotations + +import json +import logging +import os +from functools import lru_cache +from typing import Any + +import requests + +from hackbot_runtime.actions.handlers.base import ActionResult, ApplyContext + +log = logging.getLogger(__name__) + +_DEFAULT_PHABRICATOR_URL = "https://phabricator.services.mozilla.com" +_DIFF_ARTIFACT_KEY = "changes/phabricator_diff.json" +_TIMEOUT_SECONDS = 60 + + +def _base_url() -> str: + return os.environ.get("PHABRICATOR_URL", _DEFAULT_PHABRICATOR_URL).rstrip("/") + + +def _revision_url(revision_id: int) -> str: + return f"{_base_url()}/D{revision_id}" + + +def _api_key() -> str: + token = os.environ.get("PHABRICATOR_API_KEY", "") + if not token: + raise RuntimeError("PHABRICATOR_API_KEY is not configured") + return token + + +def _conduit_request(method: str, **payload: Any) -> dict: + payload["__conduit__"] = {"token": _api_key()} + response = requests.post( + f"{_base_url()}/api/{method}", + data={"params": json.dumps(payload), "output": "json"}, + timeout=_TIMEOUT_SECONDS, + ) + response.raise_for_status() + data = response.json() + if data.get("error_code"): + raise RuntimeError( + f"Conduit error {data['error_code']}: {data.get('error_info')}" + ) + return data["result"] + + +@lru_cache(maxsize=1) +def _repository_phid() -> str: + """The target repository's PHID, needed on every `differential.creatediff` call. + + Prefers an explicit `PHABRICATOR_REPOSITORY_PHID` (simplest, most robust — + the recommended way to configure this in production) and falls back to a + `diffusion.repository.search` lookup by short name + (`PHABRICATOR_REPOSITORY_NAME`, default "mozilla-central") otherwise. + """ + configured = os.environ.get("PHABRICATOR_REPOSITORY_PHID") + if configured: + return configured + + name = os.environ.get("PHABRICATOR_REPOSITORY_NAME", "mozilla-central") + result = _conduit_request("diffusion.repository.search") + for repository in result.get("data", []): + fields = repository.get("fields", {}) + if fields.get("shortName") == name or fields.get("name") == name: + return repository["phid"] + raise RuntimeError(f"Could not find a Phabricator repository named '{name}'") + + +class SubmitPatchHandler: + async def apply(self, params: dict[str, Any], ctx: ApplyContext) -> ActionResult: + bug_id = params["bug_id"] + revision_id = params.get("revision_id") + + try: + raw = await ctx.download_artifact(_DIFF_ARTIFACT_KEY) + diff_payload = json.loads(raw) + except Exception as exc: + log.exception( + "Failed to load Phabricator diff artifact for run %s", ctx.run_id + ) + return ActionResult.failed( + f"No Phabricator diff artifact for this run: {exc}" + ) + + try: + diff_result = _conduit_request( + "differential.creatediff", + repositoryPHID=_repository_phid(), + **diff_payload, + ) + diff_phid = diff_result["phid"] + + transactions: list[dict[str, Any]] = [ + {"type": "update", "value": diff_phid} + ] + if params.get("title"): + transactions.append({"type": "title", "value": params["title"]}) + if params.get("summary"): + transactions.append({"type": "summary", "value": params["summary"]}) + if params.get("reviewers"): + # Assumes Phabricator resolves these identifiers directly; + # if Mozilla's instance requires PHIDs instead of usernames + # for this transaction, a user.search-based resolution step + # needs adding here — not verified against a live instance. + transactions.append( + {"type": "reviewers.add", "value": params["reviewers"]} + ) + transactions.append({"type": "bugzilla.bug-id", "value": str(bug_id)}) + + edit_args: dict[str, Any] = {"transactions": transactions} + if revision_id: + edit_args["objectIdentifier"] = revision_id + revision_result = _conduit_request( + "differential.revision.edit", **edit_args + ) + except Exception as exc: + log.exception("Failed to submit Phabricator diff for bug %s", bug_id) + return ActionResult.failed(str(exc)) + + object_data = revision_result.get("object") or {} + new_revision_id = object_data.get("id") or revision_id + return ActionResult.ok( + { + "revision_id": new_revision_id, + "revision_url": ( + _revision_url(new_revision_id) if new_revision_id else None + ), + } + ) diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/handlers/registry.py b/libs/hackbot-runtime/hackbot_runtime/actions/handlers/registry.py index 847c7db785..d9835e9cf3 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/handlers/registry.py +++ b/libs/hackbot-runtime/hackbot_runtime/actions/handlers/registry.py @@ -5,6 +5,7 @@ CreateBugHandler, UpdateBugHandler, ) +from hackbot_runtime.actions.handlers.phabricator_handler import SubmitPatchHandler # Maps a recorded action's dotted `type` to the handler that applies it. # Adding a new action type later is a one-line addition here — the dispatch @@ -14,6 +15,7 @@ "bugzilla.add_comment": AddCommentHandler(), "bugzilla.add_attachment": AddAttachmentHandler(), "bugzilla.create_bug": CreateBugHandler(), + "phabricator.submit_patch": SubmitPatchHandler(), } diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/phabricator.py b/libs/hackbot-runtime/hackbot_runtime/actions/phabricator.py new file mode 100644 index 0000000000..d3106f9590 --- /dev/null +++ b/libs/hackbot-runtime/hackbot_runtime/actions/phabricator.py @@ -0,0 +1,101 @@ +"""Phabricator-domain recordable actions. + +Mirrors ``actions/bugzilla.py``'s shape: the handler records an intended +change (nothing is submitted to Phabricator here) and returns a short +confirmation string. See ``actions/handlers/phabricator_handler.py`` for the +apply side. +""" + +from __future__ import annotations + +from typing import Annotated, Any + +from agent_tools.registry import ToolError, tool, tools_in +from pydantic import Field + +from hackbot_runtime.actions.recorder import ActionsRecorder + + +def _confirm(recorder: ActionsRecorder, action_type: str) -> str: + return f"Recorded {action_type} (#{len(recorder.actions) - 1})." + + +@tool +async def submit_patch( + recorder: ActionsRecorder, + bug_id: Annotated[int, Field(description="Bug this patch fixes.")], + reasoning: Annotated[ + str, Field(description="Why you are submitting this patch (for audit log).") + ], + revision_id: Annotated[ + int | None, + Field( + default=None, + description=( + "An existing Phabricator revision to attach a new diff to. " + "Omit to create a brand-new revision instead — never inferred " + "automatically, so pass this explicitly whenever you intend " + "to update rather than create." + ), + ), + ] = None, + reviewers: Annotated[ + list[str] | None, + Field(default=None, description="Reviewers to request on the revision."), + ] = None, + title: Annotated[ + str | None, + Field( + default=None, + description="Revision title. Required when creating a new revision.", + ), + ] = None, + summary: Annotated[ + str | None, + Field(default=None, description="Revision summary/description."), + ] = None, + ref: Annotated[ + str | None, + Field( + default=None, + description=( + "Optional label for this action so a later action (e.g. a " + "bugzilla.add_comment in the same run) can reference its " + "result once applied, via {{actions..url}} in that " + "action's text." + ), + ), + ] = None, +) -> str: + """Record an intended Phabricator patch submission. + + Submits whatever the run's final source-tree changes turn out to be — the + same diff the runtime already collects into ``changes/changes.patch`` once + the agent finishes, not a separately-supplied file. There's no local patch + path to give here because that diff isn't final until after this call + returns (it's computed once from the checkout's end state). + + Recorded into the run summary for human review — does not submit to + Phabricator. Set revision_id to update an existing revision with a new + diff, or omit it (and provide a title) to create a new one. + """ + if revision_id is None and not title: + raise ToolError("title is required when creating a new revision") + + params: dict[str, Any] = { + "bug_id": bug_id, + "revision_id": revision_id, + "reviewers": reviewers or [], + "title": title, + "summary": summary, + } + recorder.record( + "phabricator.submit_patch", + params, + reasoning=reasoning, + ref=ref, + ) + return _confirm(recorder, "phabricator.submit_patch") + + +TOOLS = tools_in(__name__) diff --git a/libs/hackbot-runtime/hackbot_runtime/changes.py b/libs/hackbot-runtime/hackbot_runtime/changes.py index cc201e02c0..758c5fc868 100644 --- a/libs/hackbot-runtime/hackbot_runtime/changes.py +++ b/libs/hackbot-runtime/hackbot_runtime/changes.py @@ -117,13 +117,109 @@ def _commit_metadata(repo: Path, base: str) -> list[dict]: return commits -def collect(repo: Path, base: str) -> ChangeSet | None: +def _synthetic_commit(repo: Path, base: str) -> str: + """Create a detached commit object squashing ``base..HEAD``'s tree. + + Doesn't touch the working tree, index, or branch pointer — `commit-tree` + just writes one new commit object with `base` as its sole parent, giving + `build_phabricator_diff` a single commit to diff (moz-phab's diff-tree + based diffing only supports one commit vs. its immediate parent, no + range). + """ + tree = _git(repo, "rev-parse", "HEAD^{tree}").strip() + # Pass an explicit identity (as _wrap_uncommitted does): the synthetic + # commit's author is throwaway — only its tree diff is used — but + # `commit-tree` errors under `user.useConfigOnly=true` and otherwise + # invents a `user@hostname` author when the container has no git identity + # configured. A fixed identity keeps it deterministic and unconditional. + return _git( + repo, + "-c", + f"user.name={_WIP_NAME}", + "-c", + f"user.email={_WIP_EMAIL}", + "commit-tree", + tree, + "-p", + base, + "-m", + "hackbot: squashed changes for Phabricator diff", + ).strip() + + +def build_phabricator_diff(repo: Path, base: str, repo_url: str) -> dict | None: + """Build the payload for Phabricator's ``differential.creatediff`` API. + + Uses ``moz-phab``'s own diff-building code (``mozphab.git``/``mozphab.diff``, + imported as a library — requires the ``hackbot-runtime[phabricator]`` + extra) against ``repo``, which the agent already has fully checked out + for its own work — no separate clone or checkout happens here. Returns + ``None`` if building the diff fails for any reason (e.g. the checkout + lacks an ``.arcconfig``, or nothing actually changed) — this is + best-effort, gated by the caller on whether a `phabricator.submit_patch` + action was even recorded, so a failure here shouldn't break an otherwise + successful run. + + ``repositoryPHID`` is deliberately not included here — it's resolved by + the apply-side handler instead, since it's specific to which Phabricator + instance/environment (staging vs. prod) the diff actually gets submitted + to, and that shouldn't be baked into an artifact built at agent-run time. + """ + try: + from mozphab.args import parse_args + from mozphab.commits import Commit + from mozphab.git import Git + except ImportError: + log.warning( + "hackbot-runtime[phabricator] extra not installed; " + "cannot build a Phabricator diff" + ) + return None + + try: + node = _synthetic_commit(repo, base) + mozphab_repo = Git(str(repo)) + # `set_args` needs a fully-populated argparse.Namespace matching what + # moz-phab's own CLI would build (several unrelated code paths read + # attributes off it) — going through its real parser instead of + # hand-listing the handful of attributes get_diff() happens to touch + # today, which would silently bit-rot on a moz-phab upgrade. + mozphab_repo.set_args(parse_args(["submit", "--yes"])) + diff = mozphab_repo.get_diff(Commit(node=node)) + except Exception: + log.warning("Could not build Phabricator diff for %s", repo, exc_info=True) + return None + + changes_payload = [change.to_conduit(node) for change in diff.changes.values()] + if not changes_payload: + return None + + return { + "changes": changes_payload, + "sourceMachine": repo_url, + "sourcePath": str(repo), + "sourceControlBaseRevision": base, + "sourceControlPath": "/", + "sourceControlSystem": "git", + "branch": "HEAD", + "creationMethod": "hackbot", + "lintStatus": "none", + "unitStatus": "none", + } + + +def collect(repo: Path, base: str, repo_url: str) -> ChangeSet | None: """Collect changes in ``repo`` since ``base`` as a patch plus metadata. Returns ``None`` when the agent made no changes at all (nothing committed and a clean working tree). Otherwise returns a :class:`ChangeSet` whose ``patch`` is an mbox (``git format-patch`` output) applied with ``git am`` and whose ``metadata`` describes the base, the commits, and the files touched. + + ``repo_url`` is carried into the metadata (not derived from ``repo``, a local + path) so a later, out-of-process apply step — e.g. the Phabricator submit + handler, which needs to re-check-out this same base commit — knows where + to clone from without re-deriving agent-specific config. """ wrapped = _wrap_uncommitted(repo) patch = _git_bytes(repo, "format-patch", "--stdout", "--binary", f"{base}..HEAD") @@ -131,6 +227,7 @@ def collect(repo: Path, base: str) -> ChangeSet | None: return None metadata = { "base_commit": base, + "repo_url": repo_url, "wrapped_uncommitted": wrapped, "commits": _commit_metadata(repo, base), } diff --git a/libs/hackbot-runtime/hackbot_runtime/context.py b/libs/hackbot-runtime/hackbot_runtime/context.py index 8269de8fd1..2d268a6ca4 100644 --- a/libs/hackbot-runtime/hackbot_runtime/context.py +++ b/libs/hackbot-runtime/hackbot_runtime/context.py @@ -195,6 +195,7 @@ def publish_changes( self, patch_key: str = "changes/changes.patch", meta_key: str = "changes/changes.json", + phabricator_diff_key: str = "changes/phabricator_diff.json", ) -> str | None: """Collect the agent's source-tree changes and publish them as artifacts. @@ -202,10 +203,18 @@ def publish_changes( local commits and wraps the uncommitted remainder, plus a JSON summary. Returns the patch key, or ``None`` when the agent never prepared a source checkout or made no changes at all. + + If the agent recorded a ``phabricator.submit_patch`` action, also builds + and publishes the Phabricator submission payload here — while the + checkout the agent already has is still around — so the downstream + apply step never needs its own checkout (see + ``changes.build_phabricator_diff``). """ if self._source_base is None: return None - change_set = changes.collect(self.source_repo, self._source_base) + change_set = changes.collect( + self.source_repo, self._source_base, self._config.source.repo_url + ) if change_set is None: return None artifacts.publish_bytes( @@ -216,4 +225,16 @@ def publish_changes( "text/x-patch", ) self.publish_json(meta_key, change_set.metadata) + + wants_phabricator = any( + action["type"] == "phabricator.submit_patch" + for action in self.actions.actions + ) + if wants_phabricator: + diff_payload = changes.build_phabricator_diff( + self.source_repo, self._source_base, self._config.source.repo_url + ) + if diff_payload is not None: + self.publish_json(phabricator_diff_key, diff_payload) + return patch_key diff --git a/libs/hackbot-runtime/pyproject.toml b/libs/hackbot-runtime/pyproject.toml index 8186fc62e3..0aebabde58 100644 --- a/libs/hackbot-runtime/pyproject.toml +++ b/libs/hackbot-runtime/pyproject.toml @@ -12,6 +12,10 @@ dependencies = [ [project.optional-dependencies] claude-sdk = ["claude-agent-sdk>=0.1.30", "agent-tools[claude-sdk]"] +# Pinned exactly: mozphab.git/mozphab.diff are internal implementation +# classes of a CLI tool, not a published library API — a minor/patch bump +# could change their behavior without notice. +phabricator = ["MozPhab==2.15.3"] [tool.uv.sources] agent-tools = { workspace = true } diff --git a/libs/hackbot-runtime/tests/test_changes.py b/libs/hackbot-runtime/tests/test_changes.py new file mode 100644 index 0000000000..d0d261db50 --- /dev/null +++ b/libs/hackbot-runtime/tests/test_changes.py @@ -0,0 +1,135 @@ +"""Tests for building the Phabricator diff payload from a real git repo. + +`collect()` (the pre-existing git-am patch collector) has no test coverage +either way and is out of scope here — this covers the new +`_synthetic_commit`/`build_phabricator_diff`, which run against the agent's +already-checked-out repo (see hackbot_runtime.context.publish_changes). +""" + +import builtins + +from hackbot_runtime.changes import _git, _synthetic_commit, build_phabricator_diff + + +def _init_repo(repo, with_arcconfig=True): + _git(repo, "init", "-q") + _git(repo, "config", "user.email", "test@test.com") + _git(repo, "config", "user.name", "Test") + if with_arcconfig: + (repo / ".arcconfig").write_text( + '{"phabricator.uri": "https://phabricator.services.mozilla.com/"}' + ) + (repo / "file.txt").write_text("line1\nline2\nline3\n") + _git(repo, "add", "-A") + _git(repo, "commit", "-q", "-m", "base commit") + return _git(repo, "rev-parse", "HEAD").strip() + + +def _commit_change(repo, content, message="the fix"): + (repo / "file.txt").write_text(content) + _git(repo, "add", "-A") + _git(repo, "commit", "-q", "-m", message) + return _git(repo, "rev-parse", "HEAD").strip() + + +# --- _synthetic_commit ------------------------------------------------- # + + +def test_synthetic_commit_does_not_move_branch(tmp_path): + base = _init_repo(tmp_path) + head = _commit_change(tmp_path, "line1\nline2 modified\nline3\n") + + synthetic = _synthetic_commit(tmp_path, base) + + assert synthetic != head + assert _git(tmp_path, "rev-parse", "HEAD").strip() == head + + +def test_synthetic_commit_parent_is_base(tmp_path): + base = _init_repo(tmp_path) + _commit_change(tmp_path, "line1\nline2 modified\nline3\n") + + synthetic = _synthetic_commit(tmp_path, base) + + parent = _git(tmp_path, "rev-parse", f"{synthetic}^").strip() + assert parent == base + + +def test_synthetic_commit_tree_matches_head(tmp_path): + base = _init_repo(tmp_path) + _commit_change(tmp_path, "line1\nline2 modified\nline3\n") + + synthetic = _synthetic_commit(tmp_path, base) + + head_tree = _git(tmp_path, "rev-parse", "HEAD^{tree}").strip() + synthetic_tree = _git(tmp_path, "rev-parse", f"{synthetic}^{{tree}}").strip() + assert synthetic_tree == head_tree + + +def test_synthetic_commit_works_without_git_identity(tmp_path): + base = _init_repo(tmp_path) + _commit_change(tmp_path, "line1\nline2 modified\nline3\n") + # Simulate a hardened container that refuses to invent an identity — + # `commit-tree` would fail here if we didn't pass one explicitly. + _git(tmp_path, "config", "user.useConfigOnly", "true") + _git(tmp_path, "config", "--unset", "user.name") + _git(tmp_path, "config", "--unset", "user.email") + + synthetic = _synthetic_commit(tmp_path, base) + + assert _git(tmp_path, "rev-parse", f"{synthetic}^").strip() == base + + +# --- build_phabricator_diff --------------------------------------------- # + + +def test_build_phabricator_diff_with_real_change(tmp_path): + base = _init_repo(tmp_path) + _commit_change(tmp_path, "line1\nline2 modified\nline3\n") + + payload = build_phabricator_diff(tmp_path, base, "https://example.com/repo.git") + + assert payload is not None + assert payload["sourceControlBaseRevision"] == base + assert payload["sourceControlSystem"] == "git" + assert payload["sourceMachine"] == "https://example.com/repo.git" + assert len(payload["changes"]) == 1 + change = payload["changes"][0] + assert change["currentPath"] == "file.txt" + assert change["hunks"][0]["corpus"] == " line1\n-line2\n+line2 modified\n line3\n" + + +def test_build_phabricator_diff_without_arcconfig_returns_none(tmp_path): + base = _init_repo(tmp_path, with_arcconfig=False) + _commit_change(tmp_path, "line1\nline2 modified\nline3\n") + + payload = build_phabricator_diff(tmp_path, base, "https://example.com/repo.git") + + assert payload is None + + +def test_build_phabricator_diff_no_changes_returns_none(tmp_path): + base = _init_repo(tmp_path) + # No commits made after base -- HEAD == base, nothing to squash/diff. + + payload = build_phabricator_diff(tmp_path, base, "https://example.com/repo.git") + + assert payload is None + + +def test_build_phabricator_diff_missing_mozphab_returns_none(tmp_path, monkeypatch): + base = _init_repo(tmp_path) + _commit_change(tmp_path, "line1\nline2 modified\nline3\n") + + real_import = builtins.__import__ + + def fake_import(name, *args, **kwargs): + if name.startswith("mozphab"): + raise ImportError("mozphab not installed") + return real_import(name, *args, **kwargs) + + monkeypatch.setattr(builtins, "__import__", fake_import) + + payload = build_phabricator_diff(tmp_path, base, "https://example.com/repo.git") + + assert payload is None diff --git a/libs/hackbot-runtime/tests/test_context.py b/libs/hackbot-runtime/tests/test_context.py index 26ef0b4d1d..7a9a403487 100644 --- a/libs/hackbot-runtime/tests/test_context.py +++ b/libs/hackbot-runtime/tests/test_context.py @@ -1,9 +1,11 @@ """Tests for HackbotContext capabilities and results plumbing.""" +import json from pathlib import Path import pytest from hackbot_runtime import HackbotContext +from hackbot_runtime.changes import ChangeSet from hackbot_runtime.config import FirefoxConfig, HackbotConfig, SourceConfig @@ -101,3 +103,60 @@ def test_results_plumbing(tmp_path): hb.actions.record("bugzilla.update_bug", {"bug_id": 1}, reasoning="r") assert hb.actions.actions[0]["type"] == "bugzilla.update_bug" + + +def _hb_with_source(tmp_path, monkeypatch): + """Wire a context to publish changes without a real checkout. + + Sets a recorded source base and mocks changes.collect so + publish_changes() runs its body. + """ + cfg = HackbotConfig(source=SourceConfig(repo_url="https://example.com/r.git")) + hb = _hb(tmp_path, cfg) + hb._source_base = "basecommit" + # source_repo would normally clone; publish_changes only passes it through + # to the (mocked) changes helpers, so a bare path is enough here. + monkeypatch.setattr( + type(hb), "source_repo", property(lambda self: tmp_path / "src") + ) + monkeypatch.setattr( + "hackbot_runtime.context.changes.collect", + lambda repo, base, repo_url: ChangeSet(patch=b"x", metadata={"base": base}), + ) + return hb + + +def test_publish_changes_builds_phabricator_diff_when_action_recorded( + tmp_path, monkeypatch +): + hb = _hb_with_source(tmp_path, monkeypatch) + monkeypatch.setattr( + "hackbot_runtime.context.changes.build_phabricator_diff", + lambda repo, base, repo_url: {"changes": [], "sourceControlBaseRevision": base}, + ) + hb.actions.record("phabricator.submit_patch", {"bug_id": 1}, reasoning="r") + + hb.publish_changes() + + written = ( + tmp_path / "artifacts" / "local-test" / "changes" / "phabricator_diff.json" + ) + assert json.loads(written.read_text())["sourceControlBaseRevision"] == "basecommit" + + +def test_publish_changes_skips_phabricator_diff_without_action(tmp_path, monkeypatch): + hb = _hb_with_source(tmp_path, monkeypatch) + called = [] + monkeypatch.setattr( + "hackbot_runtime.context.changes.build_phabricator_diff", + lambda *a, **k: called.append(a) or {}, + ) + hb.actions.record("bugzilla.add_comment", {"bug_id": 1}, reasoning="r") + + hb.publish_changes() + + assert called == [] + written = ( + tmp_path / "artifacts" / "local-test" / "changes" / "phabricator_diff.json" + ) + assert not written.exists() diff --git a/libs/hackbot-runtime/tests/test_phabricator_actions.py b/libs/hackbot-runtime/tests/test_phabricator_actions.py new file mode 100644 index 0000000000..172bbc7e46 --- /dev/null +++ b/libs/hackbot-runtime/tests/test_phabricator_actions.py @@ -0,0 +1,48 @@ +"""Tests for the phabricator.submit_patch recording tool.""" + +import pytest +from agent_tools.registry import ToolError +from hackbot_runtime.actions import ActionsRecorder, phabricator + + +async def test_create_requires_title(): + rec = ActionsRecorder() + with pytest.raises(ToolError): + await phabricator.submit_patch(rec, bug_id=1, reasoning="r") + + +async def test_create_records_without_revision_id(): + rec = ActionsRecorder() + await phabricator.submit_patch( + rec, bug_id=1, reasoning="r", title="Fix the thing", summary="Details" + ) + action = rec.actions[0] + assert action["type"] == "phabricator.submit_patch" + assert action["params"] == { + "bug_id": 1, + "revision_id": None, + "reviewers": [], + "title": "Fix the thing", + "summary": "Details", + } + assert "ref" not in action + + +async def test_update_does_not_require_title(): + rec = ActionsRecorder() + await phabricator.submit_patch(rec, bug_id=1, reasoning="r", revision_id=12345) + assert rec.actions[0]["params"]["revision_id"] == 12345 + + +async def test_ref_is_recorded(): + rec = ActionsRecorder() + await phabricator.submit_patch( + rec, bug_id=1, reasoning="r", title="Fix", ref="patch" + ) + assert rec.actions[0]["ref"] == "patch" + + +async def test_reviewers_default_to_empty_list(): + rec = ActionsRecorder() + await phabricator.submit_patch(rec, bug_id=1, reasoning="r", title="Fix") + assert rec.actions[0]["params"]["reviewers"] == [] diff --git a/libs/hackbot-runtime/tests/test_phabricator_handler.py b/libs/hackbot-runtime/tests/test_phabricator_handler.py new file mode 100644 index 0000000000..b019f13b31 --- /dev/null +++ b/libs/hackbot-runtime/tests/test_phabricator_handler.py @@ -0,0 +1,165 @@ +"""Tests for the apply-side Phabricator action handler. + +Mocks the Conduit API calls (`_conduit_request`) so these exercise the +handler's own logic — payload relay, create-vs-update transaction building, +result parsing — without a network call. The handler itself does no git/ +subprocess work at all; that happens agent-side (see test_changes.py's +`build_phabricator_diff` tests). +""" + +import json + +from hackbot_runtime.actions.handlers import ApplyContext, phabricator_handler + +_DIFF_PAYLOAD = { + "changes": [{"currentPath": "file.txt"}], + "sourceControlBaseRevision": "abc123", + "sourceControlPath": "/", + "sourceControlSystem": "git", + "branch": "HEAD", +} + + +def _ctx(artifact=_DIFF_PAYLOAD): + async def download(key): + assert key == "changes/phabricator_diff.json" + return json.dumps(artifact).encode() + + return ApplyContext(run_id="run-1", download_artifact=download) + + +def _fake_conduit(responses): + calls = [] + + def fake(method, **payload): + calls.append((method, payload)) + return responses[method] + + return fake, calls + + +async def test_submit_patch_create_success(monkeypatch): + fake, calls = _fake_conduit( + { + "differential.creatediff": {"phid": "PHID-DIFF-1", "diffid": 1}, + "differential.revision.edit": {"object": {"id": 555, "phid": "PHID-REV-1"}}, + } + ) + monkeypatch.setattr(phabricator_handler, "_conduit_request", fake) + monkeypatch.setattr(phabricator_handler, "_repository_phid", lambda: "PHID-REPO-1") + + result = await phabricator_handler.SubmitPatchHandler().apply( + { + "bug_id": 1, + "revision_id": None, + "title": "Fix", + "summary": "s", + "reviewers": ["alice"], + }, + _ctx(), + ) + + assert result.status == "applied" + assert result.result == { + "revision_id": 555, + "revision_url": "https://phabricator.services.mozilla.com/D555", + } + + creatediff_call = next(c for c in calls if c[0] == "differential.creatediff") + assert creatediff_call[1]["repositoryPHID"] == "PHID-REPO-1" + assert creatediff_call[1]["changes"] == _DIFF_PAYLOAD["changes"] + + edit_call = next(c for c in calls if c[0] == "differential.revision.edit") + assert "objectIdentifier" not in edit_call[1] + transactions = {t["type"]: t["value"] for t in edit_call[1]["transactions"]} + assert transactions["update"] == "PHID-DIFF-1" + assert transactions["title"] == "Fix" + assert transactions["reviewers.add"] == ["alice"] + assert transactions["bugzilla.bug-id"] == "1" + + +async def test_submit_patch_update_sets_object_identifier(monkeypatch): + fake, calls = _fake_conduit( + { + "differential.creatediff": {"phid": "PHID-DIFF-2", "diffid": 2}, + "differential.revision.edit": {"object": {"id": 12345}}, + } + ) + monkeypatch.setattr(phabricator_handler, "_conduit_request", fake) + monkeypatch.setattr(phabricator_handler, "_repository_phid", lambda: "PHID-REPO-1") + + result = await phabricator_handler.SubmitPatchHandler().apply( + {"bug_id": 7, "revision_id": 12345}, _ctx() + ) + + assert result.status == "applied" + assert result.result["revision_id"] == 12345 + edit_call = next(c for c in calls if c[0] == "differential.revision.edit") + assert edit_call[1]["objectIdentifier"] == 12345 + + +async def test_submit_patch_falls_back_to_given_revision_id_when_edit_omits_object( + monkeypatch, +): + fake, _ = _fake_conduit( + { + "differential.creatediff": {"phid": "PHID-DIFF-3"}, + "differential.revision.edit": {"object": {}}, + } + ) + monkeypatch.setattr(phabricator_handler, "_conduit_request", fake) + monkeypatch.setattr(phabricator_handler, "_repository_phid", lambda: "PHID-REPO-1") + + result = await phabricator_handler.SubmitPatchHandler().apply( + {"bug_id": 7, "revision_id": 999}, _ctx() + ) + assert result.status == "applied" + assert result.result["revision_id"] == 999 + + +async def test_submit_patch_missing_artifact_fails(): + async def download(key): + raise KeyError(key) + + ctx = ApplyContext(run_id="run-1", download_artifact=download) + result = await phabricator_handler.SubmitPatchHandler().apply({"bug_id": 1}, ctx) + assert result.status == "failed" + + +async def test_submit_patch_conduit_error_fails(monkeypatch): + def fake(method, **payload): + raise RuntimeError("Conduit error ERR-CONDUIT-CORE: bad request") + + monkeypatch.setattr(phabricator_handler, "_conduit_request", fake) + monkeypatch.setattr(phabricator_handler, "_repository_phid", lambda: "PHID-REPO-1") + + result = await phabricator_handler.SubmitPatchHandler().apply( + {"bug_id": 1, "title": "x"}, _ctx() + ) + assert result.status == "failed" + assert "ERR-CONDUIT-CORE" in result.error + + +def test_repository_phid_prefers_env_var(monkeypatch): + monkeypatch.setenv("PHABRICATOR_REPOSITORY_PHID", "PHID-FROM-ENV") + phabricator_handler._repository_phid.cache_clear() + assert phabricator_handler._repository_phid() == "PHID-FROM-ENV" + phabricator_handler._repository_phid.cache_clear() + + +def test_repository_phid_looks_up_by_short_name(monkeypatch): + monkeypatch.delenv("PHABRICATOR_REPOSITORY_PHID", raising=False) + + def fake(method, **payload): + assert method == "diffusion.repository.search" + return { + "data": [ + {"phid": "PHID-OTHER", "fields": {"shortName": "other-repo"}}, + {"phid": "PHID-MC", "fields": {"shortName": "mozilla-central"}}, + ] + } + + monkeypatch.setattr(phabricator_handler, "_conduit_request", fake) + phabricator_handler._repository_phid.cache_clear() + assert phabricator_handler._repository_phid() == "PHID-MC" + phabricator_handler._repository_phid.cache_clear() diff --git a/uv.lock b/uv.lock index 0de875449c..7e0b7f4880 100644 --- a/uv.lock +++ b/uv.lock @@ -1506,6 +1506,15 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/05/7f/798705f5296a58ca505d600456748d1be48078eac8a7050d8a98bc9edb89/decorator-5.3.1-py3-none-any.whl", hash = "sha256:f47fe6fdbd2edd623ecfe36875d37aba411624e2670dd395dddae1358689bb3c", size = 10365, upload-time = "2026-05-18T06:03:26.517Z" }, ] +[[package]] +name = "diskcache" +version = "5.6.3" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/3f/21/1c1ffc1a039ddcc459db43cc108658f32c57d271d7289a2794e401d0fdb6/diskcache-5.6.3.tar.gz", hash = "sha256:2c3a3fa2743d8535d832ec61c2054a1641f41775aa7c556758a109941e33e4fc", size = 67916, upload-time = "2023-08-31T06:12:00.316Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/3f/27/4570e78fc0bf5ea0ca45eb1de3818a23787af9b390c0b0a0033a1b8236f9/diskcache-5.6.3-py3-none-any.whl", hash = "sha256:5e31b2d5fbad117cc363ebaf6b689474db18a1f6438bc82358b024abd4c2ca19", size = 45550, upload-time = "2023-08-31T06:11:58.822Z" }, +] + [[package]] name = "diskcache-weave" version = "5.6.3.post1" @@ -1980,6 +1989,43 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/20/7a/1c6e3562dfd8950adbb11ffbc65d21e7c89d01a6e4f137fa981056de25c5/gitpython-3.1.50-py3-none-any.whl", hash = "sha256:d352abe2908d07355014abdd21ddf798c2a961469239afec4962e9da884858f9", size = 212507, upload-time = "2026-05-06T04:01:23.799Z" }, ] +[[package]] +name = "glean-parser" +version = "17.3.0" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "click" }, + { name = "diskcache" }, + { name = "jinja2" }, + { name = "jsonschema" }, + { name = "platformdirs" }, + { name = "pyyaml" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/83/c3/24387435f0bf3def52922a70c3f5aab81c82ab6120ac4db719109f0c8b23/glean_parser-17.3.0.tar.gz", hash = "sha256:f70fb4496436068f81ef786029a1b369f61ae2f9327eeb2fa50335dba59ee214", size = 125596, upload-time = "2025-07-09T09:03:02.026Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/6d/31/8176e4e77144bae2493d9da708c15da01e403befbf3c9490db001a64da53/glean_parser-17.3.0-py3-none-any.whl", hash = "sha256:84d975d40333e97d9f6e0ce0f22db4e1e3d8e088b2da678e34cc7ab3361f071d", size = 115439, upload-time = "2025-07-09T09:03:00.784Z" }, +] + +[[package]] +name = "glean-sdk" +version = "65.2.3" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "glean-parser" }, + { name = "semver" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/bf/8a/a45f4ee0a98901cd474204587ab6ecf18f141803a11d3c091c1f035fbfed/glean_sdk-65.2.3.tar.gz", hash = "sha256:e8d17d1851b688b242c9de51c9125b41cd87a30a788a2a26be6cdf00e628d1be", size = 260945, upload-time = "2025-11-06T11:15:08.141Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/b6/43/fabb546efe0925ba11c6a1d776507f75c267cfa8401729391e14e2fc3fc6/glean_sdk-65.2.3-py3-none-macosx_10_12_x86_64.macosx_11_0_arm64.macosx_10_12_universal2.whl", hash = "sha256:fdb39d22e2a6c095ec8990b71b2eb833cbde4a807e8032a4ae218a2983eb23bd", size = 1686649, upload-time = "2025-11-06T11:12:57.204Z" }, + { url = "https://files.pythonhosted.org/packages/d4/fa/cd8d6b2f31e96c4fd4e3fe54ad3f3c2ed51bbba18b2a5ff4adeb94970af3/glean_sdk-65.2.3-py3-none-macosx_10_12_x86_64.whl", hash = "sha256:263d1a91f1048261013f11a78073d5e1b9d17f29065f9ef7e296dc995f2f846a", size = 898508, upload-time = "2025-11-06T11:12:59.447Z" }, + { url = "https://files.pythonhosted.org/packages/d9/ce/76677fa87e58abeb4b016b1870fe1b6a9b616ea857099dc6a95f504e8b99/glean_sdk-65.2.3-py3-none-macosx_11_0_arm64.whl", hash = "sha256:38b90604ae9f55fc522e67b47989ed547ccd35d7dbb871b5f78caffedc9cbae0", size = 866137, upload-time = "2025-11-06T11:13:00.771Z" }, + { url = "https://files.pythonhosted.org/packages/2c/7e/51f1c15418f7e173bde4d73c06ea153e8447fc10ec05d62fb052098b08cd/glean_sdk-65.2.3-py3-none-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:182eaa29eba7096c751353eac8faec1916ea6b81445ea688874b732d45727b7a", size = 821146, upload-time = "2025-11-06T11:14:59.985Z" }, + { url = "https://files.pythonhosted.org/packages/93/65/ed4c48130e3423de9386011796452a81ba552429a6b6f0d6ec14e5aaa14b/glean_sdk-65.2.3-py3-none-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:f8346bd5938218906ad572ab82c5fd4468481dda6a2ba41e5fbe67c9383bbcf9", size = 934818, upload-time = "2025-11-06T11:14:31.725Z" }, + { url = "https://files.pythonhosted.org/packages/1b/24/ee4285df4a19726be7941bad99daa6d1ef3c581fb1ca54f2fdd68d3ddc2d/glean_sdk-65.2.3-py3-none-manylinux_2_34_x86_64.whl", hash = "sha256:45d61656d4eead892cdc2cac7d8b2ba1185a9520a0ca50e8326e8a6c2566519e", size = 934533, upload-time = "2025-11-06T11:15:06.777Z" }, + { url = "https://files.pythonhosted.org/packages/68/d0/78b6e7d156aa0267b4d65dfc67746254a039d485f6943cce86b2091f9443/glean_sdk-65.2.3-py3-none-win_amd64.whl", hash = "sha256:9e603dc7fca2b6cd739a3a31195d4d564db542de5571d7afbd33a7ca8fb8fdde", size = 878415, upload-time = "2025-11-06T11:14:06.474Z" }, + { url = "https://files.pythonhosted.org/packages/37/a2/f8bf156045be612d9dbf44c8c63e889050f94da3205ecdb4a836b79948bb/glean_sdk-65.2.3-py3-none-win_arm64.whl", hash = "sha256:70920c20bbf393da807933056e85575598078f8124c44c68257df195b5d8d5f4", size = 741441, upload-time = "2025-11-06T11:15:20.785Z" }, +] + [[package]] name = "google-api-core" version = "2.30.3" @@ -2439,7 +2485,7 @@ dependencies = [ { name = "agent-tools", extra = ["bugzilla", "firefox"] }, { name = "bugsy" }, { name = "claude-agent-sdk" }, - { name = "hackbot-runtime", extra = ["claude-sdk"] }, + { name = "hackbot-runtime", extra = ["claude-sdk", "phabricator"] }, { name = "mcp" }, { name = "starlette" }, { name = "uvicorn" }, @@ -2450,7 +2496,7 @@ requires-dist = [ { name = "agent-tools", extras = ["bugzilla", "firefox"], editable = "libs/agent-tools" }, { name = "bugsy" }, { name = "claude-agent-sdk", specifier = ">=0.1.30" }, - { name = "hackbot-runtime", extras = ["claude-sdk"], editable = "libs/hackbot-runtime" }, + { name = "hackbot-runtime", extras = ["claude-sdk", "phabricator"], editable = "libs/hackbot-runtime" }, { name = "mcp", specifier = ">=1.0.0" }, { name = "starlette", specifier = ">=0.36.0" }, { name = "uvicorn", specifier = ">=0.27.0" }, @@ -2600,6 +2646,9 @@ claude-sdk = [ { name = "agent-tools", extra = ["claude-sdk"] }, { name = "claude-agent-sdk" }, ] +phabricator = [ + { name = "mozphab" }, +] [package.metadata] requires-dist = [ @@ -2607,10 +2656,11 @@ requires-dist = [ { name = "agent-tools", extras = ["claude-sdk"], marker = "extra == 'claude-sdk'", editable = "libs/agent-tools" }, { name = "claude-agent-sdk", marker = "extra == 'claude-sdk'", specifier = ">=0.1.30" }, { name = "google-auth", specifier = ">=2.0.0" }, + { name = "mozphab", marker = "extra == 'phabricator'", specifier = "==2.15.3" }, { name = "pydantic-settings", specifier = ">=2.1.0" }, { name = "requests", specifier = ">=2.32.0" }, ] -provides-extras = ["claude-sdk"] +provides-extras = ["claude-sdk", "phabricator"] [[package]] name = "hf-xet" @@ -4185,6 +4235,25 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/93/2d/5cd9787f930f2a0cf4ab3d105ce15edc39733328e7606d97124877b70c4e/mozInstall-2.1.0-py2.py3-none-any.whl", hash = "sha256:8abc26e37b1976eb3d815ee2a42bb6bbb10926e16620eabb6f8f1ed764b0c0fe", size = 6687, upload-time = "2023-07-28T13:43:23.953Z" }, ] +[[package]] +name = "mozphab" +version = "2.15.3" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "colorama" }, + { name = "distro" }, + { name = "glean-sdk" }, + { name = "packaging" }, + { name = "python-hglib" }, + { name = "sentry-sdk" }, + { name = "setuptools" }, + { name = "urllib3" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/d7/f8/af97c0db47ef85ea801d782fcc6a3f6cbf2ee7c06ef75946fdf40ed28e3c/mozphab-2.15.3.tar.gz", hash = "sha256:2eb9e3fb4b936acc8d340b49739905b4d8f2f5d7450e519cfdd2cebf13bb8226", size = 278633, upload-time = "2026-06-25T16:31:57.812Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/00/32/512ae30c4ef7fa2c606a24fc1f9a331a215988f4819328233d427f3269de/mozphab-2.15.3-py3-none-any.whl", hash = "sha256:3265eb9ddd03e08c44c8ea784912790f60a8ec0a1f104fdc64eea410f4dff23e", size = 124401, upload-time = "2026-06-25T16:31:55.998Z" }, +] + [[package]] name = "multidict" version = "6.7.1" @@ -6411,6 +6480,15 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/b7/46/f5af3402b579fd5e11573ce652019a67074317e18c1935cc0b4ba9b35552/secretstorage-3.5.0-py3-none-any.whl", hash = "sha256:0ce65888c0725fcb2c5bc0fdb8e5438eece02c523557ea40ce0703c266248137", size = 15554, upload-time = "2025-11-23T19:02:51.545Z" }, ] +[[package]] +name = "semver" +version = "3.0.4" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/72/d1/d3159231aec234a59dd7d601e9dd9fe96f3afff15efd33c1070019b26132/semver-3.0.4.tar.gz", hash = "sha256:afc7d8c584a5ed0a11033af086e8af226a9c0b206f313e0301f8dd7b6b589602", size = 269730, upload-time = "2025-01-24T13:19:27.617Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/a6/24/4d91e05817e92e3a61c8a21e08fd0f390f5301f1c448b137c57c4bc6e543/semver-3.0.4-py3-none-any.whl", hash = "sha256:9c824d87ba7f7ab4a1890799cec8596f15c1241cb473404ea1cb0c55e4b04746", size = 17912, upload-time = "2025-01-24T13:19:24.949Z" }, +] + [[package]] name = "sendgrid" version = "6.12.5" From cd4dcfdf04cafab347e21ade465d15750f94d547 Mon Sep 17 00:00:00 2001 From: Suhaib Mujahid Date: Sat, 4 Jul 2026 16:38:32 -0400 Subject: [PATCH 04/13] hackbot-api: event-driven run completion and action application MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces client-poll-only completion detection (GET /runs/{run_id} was the only thing that ran _reconcile) with a push-based pipeline of two internal event routes, named by role rather than by mechanism: - /internal/events/agent-run-finished — the platform-completion ingress. Fed by an Eventarc trigger on Cloud Run Jobs execution state today; the name and downstream stay valid for other execution platforms later, only the payload parsing (_cloud_run_execution_name) is platform-specific. It calls finalize_run, which persists the terminal Run row idempotently (Run.finalized_at) and publishes a run.completed event to the agent-run- events Pub/Sub topic. GET /runs/{run_id} becomes a plain read. - /internal/events/apply-run-actions — a consumer of run.completed, named for its job since the event will feed other consumers later. Applies the run's recorded actions via hackbot_runtime's handler registry, one run_actions row per action, idempotent on retry, with {{actions..}} placeholder resolution. Only succeeded runs' actions are applied (also enforced by the subscription filter); run.completed still fires for all terminal statuses so future consumers (e.g. failure notifications) can react. Events carry filterable attributes (event_type, agent, status, schema_version) so consumers select via subscription filters; topics follow a per-domain convention (-events, no hackbot- prefix in a hackbot-only project), this being the agent-run domain. Both routes verify the OIDC push token (require_push_auth), since the service is otherwise public. Fixes RunSummary silently dropping the recorded actions array on the way in. --- services/hackbot-api/app/actions_applier.py | 156 ++++++++++++++++++ services/hackbot-api/app/auth.py | 41 +++++ services/hackbot-api/app/config.py | 9 + services/hackbot-api/app/gcs.py | 17 ++ services/hackbot-api/app/main.py | 3 +- services/hackbot-api/app/pubsub.py | 88 ++++++++++ services/hackbot-api/app/routers/__init__.py | 3 +- services/hackbot-api/app/routers/events.py | 106 ++++++++++++ services/hackbot-api/app/routers/runs.py | 34 ++-- services/hackbot-api/app/schemas.py | 4 +- services/hackbot-api/pyproject.toml | 6 + .../hackbot-api/tests/test_actions_applier.py | 87 ++++++++++ services/hackbot-api/tests/test_events.py | 41 +++++ .../hackbot-api/tests/test_finalize_run.py | 141 ++++++++++++++++ services/hackbot-api/tests/test_pubsub.py | 60 +++++++ uv.lock | 26 +++ 16 files changed, 800 insertions(+), 22 deletions(-) create mode 100644 services/hackbot-api/app/actions_applier.py create mode 100644 services/hackbot-api/app/pubsub.py create mode 100644 services/hackbot-api/app/routers/events.py create mode 100644 services/hackbot-api/tests/test_actions_applier.py create mode 100644 services/hackbot-api/tests/test_events.py create mode 100644 services/hackbot-api/tests/test_finalize_run.py create mode 100644 services/hackbot-api/tests/test_pubsub.py diff --git a/services/hackbot-api/app/actions_applier.py b/services/hackbot-api/app/actions_applier.py new file mode 100644 index 0000000000..7e5ef32301 --- /dev/null +++ b/services/hackbot-api/app/actions_applier.py @@ -0,0 +1,156 @@ +"""Apply a run's recorded actions once it has finished. + +Triggered by the `agent-run-events` push subscription (see +`app/routers/events.py`), after `finalize_run` has already persisted the +run's terminal status and `summary.json`. Reads `summary["actions"]`, upserts +one `RunAction` row per entry, and runs each pending one through the handler +registry in `hackbot_runtime.actions.handlers` — idempotent per action, so a +Pub/Sub retry of the same message only re-attempts actions that didn't reach +`applied` last time. +""" + +from __future__ import annotations + +import logging +import re +from datetime import datetime, timezone +from typing import Any + +from hackbot_runtime.actions.handlers import ApplyContext, get_handler +from sqlalchemy import select +from sqlalchemy.ext.asyncio import AsyncSession + +from app import gcs +from app.database.models import Run, RunAction +from app.schemas import RunStatus + +log = logging.getLogger(__name__) + +_PLACEHOLDER_RE = re.compile(r"\{\{actions\.([^.}]+)\.([^}]+)\}\}") + + +def resolve_placeholders(value: Any, results_by_ref: dict[str, dict]) -> Any: + """Substitute `{{actions..}}` in `value` using prior results. + + Recurses through dicts/lists so a placeholder can appear anywhere in an + action's params, not just at the top level. A placeholder referencing a + ref that hasn't been applied yet (or lacks that field) is left as-is + rather than raising — the action then fails downstream with an error a + human can actually read, instead of a silent substitution glitch. + """ + if isinstance(value, str): + + def _sub(match: re.Match) -> str: + result = results_by_ref.get(match.group(1)) + if result is None or match.group(2) not in result: + return match.group(0) + return str(result[match.group(2)]) + + return _PLACEHOLDER_RE.sub(_sub, value) + if isinstance(value, dict): + return {k: resolve_placeholders(v, results_by_ref) for k, v in value.items()} + if isinstance(value, list): + return [resolve_placeholders(v, results_by_ref) for v in value] + return value + + +async def _ensure_rows( + db: AsyncSession, run: Run +) -> list[tuple[RunAction, list[dict]]]: + """Upsert one `RunAction` per recorded action. + + Returns each row paired with its (not persisted) attachments list from + summary.json. + """ + actions: list[dict] = (run.summary or {}).get("actions", []) + + result = await db.execute(select(RunAction).where(RunAction.run_id == run.run_id)) + existing = {row.idx: row for row in result.scalars()} + + rows: list[tuple[RunAction, list[dict]]] = [] + for idx, action in enumerate(actions): + row = existing.get(idx) + if row is None: + row = RunAction( + run_id=run.run_id, + idx=idx, + type=action["type"], + params=action.get("params", {}), + ref=action.get("ref"), + status="pending", + ) + db.add(row) + rows.append((row, action.get("attachments", []))) + await db.flush() + return rows + + +async def apply_pending_actions(db: AsyncSession, run: Run) -> None: + # Only a successful run's actions are applied. A failed/timed-out run may + # have recorded actions before it errored, but submitting a patch or + # posting a comment from a run that never reached a verified-good state + # isn't wanted. The applier's Pub/Sub subscription filters to + # status="succeeded" as the primary routing (failed runs never reach here), + # so this check is defense-in-depth: it keeps the applier correct when + # invoked directly (tests, future callers) and documents the policy in + # code. `RunCompleted` is still published for every terminal status (see + # finalize_run), so a future failure-notifier consumer can filter for the + # ones this applier ignores. + if run.status != RunStatus.succeeded.value: + log.info( + "Skipping action application for run %s (status=%s)", + run.run_id, + run.status, + ) + return + + rows = await _ensure_rows(db, run) + + results_by_ref: dict[str, dict] = { + row.ref: row.result + for row, _ in rows + if row.ref and row.status == "applied" and row.result is not None + } + + for row, attachments in rows: + if row.status == "applied": + continue + + handler = get_handler(row.type) + if handler is None: + row.status = "failed" + row.error = f"No handler registered for action type '{row.type}'" + await db.commit() + continue + + params = resolve_placeholders(row.params, results_by_ref) + ctx = ApplyContext( + run_id=str(run.run_id), + download_artifact=lambda key, run_id=str(run.run_id): ( + gcs.download_artifact_bytes(run_id, key) + ), + attachments=attachments, + ) + + try: + outcome = await handler.apply(params, ctx) + except Exception as exc: + log.exception( + "Handler for %s raised while applying run %s action #%d", + row.type, + run.run_id, + row.idx, + ) + row.status = "failed" + row.error = str(exc) + await db.commit() + continue + + row.status = outcome.status + row.result = outcome.result + row.error = outcome.error + row.applied_at = datetime.now(timezone.utc) + await db.commit() + + if row.status == "applied" and row.ref and row.result is not None: + results_by_ref[row.ref] = row.result diff --git a/services/hackbot-api/app/auth.py b/services/hackbot-api/app/auth.py index 8df0436456..25cd2bb858 100644 --- a/services/hackbot-api/app/auth.py +++ b/services/hackbot-api/app/auth.py @@ -1,9 +1,14 @@ import hmac +import logging from fastapi import Header, HTTPException, status +from google.auth.transport import requests as google_requests +from google.oauth2 import id_token from app.config import settings +log = logging.getLogger(__name__) + async def require_api_key(x_api_key: str | None = Header(default=None)) -> None: if not settings.external_api_key: @@ -18,3 +23,39 @@ async def require_api_key(x_api_key: str | None = Header(default=None)) -> None: status_code=status.HTTP_401_UNAUTHORIZED, detail="Invalid or missing X-API-Key", ) + + +async def require_push_auth(authorization: str | None = Header(default=None)) -> None: + """Verify a Google-signed OIDC token from an Eventarc/Pub/Sub push request. + + Cloud Run allows unauthenticated invocations for this service (that's how + `require_api_key` callers reach it at all), so platform-level IAM checks on + the push subscription/Eventarc trigger don't protect these routes on their + own — the token still needs verifying here, same as GCP's own docs recommend + for push endpoints on a service that isn't otherwise locked down. + """ + if not settings.push_auth_audience or not settings.push_auth_service_account: + raise HTTPException( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail="Push auth not configured", + ) + if authorization is None or not authorization.startswith("Bearer "): + raise HTTPException( + status_code=status.HTTP_401_UNAUTHORIZED, + detail="Missing bearer token", + ) + token = authorization.removeprefix("Bearer ") + try: + claims = id_token.verify_oauth2_token( + token, google_requests.Request(), audience=settings.push_auth_audience + ) + except ValueError: + log.warning("Rejected push request with invalid OIDC token") + raise HTTPException( + status_code=status.HTTP_401_UNAUTHORIZED, detail="Invalid token" + ) from None + if claims.get("email") != settings.push_auth_service_account: + raise HTTPException( + status_code=status.HTTP_401_UNAUTHORIZED, + detail="Token not from the expected service account", + ) diff --git a/services/hackbot-api/app/config.py b/services/hackbot-api/app/config.py index 8975be89d1..ec23ff261e 100644 --- a/services/hackbot-api/app/config.py +++ b/services/hackbot-api/app/config.py @@ -21,6 +21,15 @@ class Settings(BaseSettings): # API auth external_api_key: str = "" + # Internal event routes (Eventarc / Pub/Sub push targets). + # Event topics follow a per-domain convention, `-events` (the GCP + # project is hackbot-only, so no `hackbot-` prefix); this is the agent-run + # domain. New domains add their own topic + setting rather than overloading + # this one (see app/pubsub.py). + run_events_topic: str = "agent-run-events" + push_auth_audience: str = "" + push_auth_service_account: str = "" + # Server port: int = 8080 environment: str = "development" diff --git a/services/hackbot-api/app/gcs.py b/services/hackbot-api/app/gcs.py index 49e720eff8..689b767cb3 100644 --- a/services/hackbot-api/app/gcs.py +++ b/services/hackbot-api/app/gcs.py @@ -144,6 +144,23 @@ async def read_summary(run_id: str) -> RunSummary | None: return await asyncio.to_thread(_read_summary_sync, run_id) +def _download_artifact_bytes_sync(run_id: str, key: str) -> bytes: + bucket = _client().bucket(settings.results_bucket) + blob = bucket.blob(f"{run_prefix(run_id)}{key}") + return blob.download_as_bytes() + + +async def download_artifact_bytes(run_id: str, key: str) -> bytes: + """Fetch the raw bytes of one artifact under a run's prefix. + + Backs `hackbot_runtime.actions.handlers.base.ApplyContext.download_artifact` + for the action-applier — handlers ask for an artifact by its recorded key + (e.g. "attachments/0/file", "changes/changes.patch") without knowing GCS + is behind it. + """ + return await asyncio.to_thread(_download_artifact_bytes_sync, run_id, key) + + def _list_artifacts_sync(run_id: str) -> list[ArtifactRef]: bucket = _client().bucket(settings.results_bucket) prefix = run_prefix(run_id) diff --git a/services/hackbot-api/app/main.py b/services/hackbot-api/app/main.py index 8693303a50..2bb44e4de2 100644 --- a/services/hackbot-api/app/main.py +++ b/services/hackbot-api/app/main.py @@ -7,7 +7,7 @@ from app import __version__ from app.config import settings from app.database.connection import close_db, init_db -from app.routers import runs_router +from app.routers import events_router, runs_router if settings.sentry_dsn: sentry_sdk.init( @@ -40,6 +40,7 @@ async def lifespan(app: FastAPI): ) app.include_router(runs_router) +app.include_router(events_router) @app.get("/health") diff --git a/services/hackbot-api/app/pubsub.py b/services/hackbot-api/app/pubsub.py new file mode 100644 index 0000000000..17936be0d3 --- /dev/null +++ b/services/hackbot-api/app/pubsub.py @@ -0,0 +1,88 @@ +import asyncio +import json +import logging +from functools import lru_cache + +from google.cloud import pubsub_v1 + +from app.config import settings + +log = logging.getLogger(__name__) + +# Bump when the attribute set or payload shape changes incompatibly, so +# consumers can guard on `attributes.schema_version` if they need to. +EVENT_SCHEMA_VERSION = "1" + + +@lru_cache(maxsize=1) +def _publisher() -> pubsub_v1.PublisherClient: + return pubsub_v1.PublisherClient() + + +def _topic_path(topic: str) -> str: + if not settings.gcp_project: + raise RuntimeError("gcp_project not configured") + return _publisher().topic_path(settings.gcp_project, topic) + + +def _publish_sync(topic: str, data: bytes, attributes: dict[str, str]) -> str: + future = _publisher().publish(_topic_path(topic), data, **attributes) + return future.result() + + +def _build_event( + event_type: str, payload: dict, attributes: dict[str, str] +) -> tuple[bytes, dict[str, str]]: + """Serialize one domain event into (body, attributes). + + `event_type` (dotted ``.``, e.g. ``run.completed``) and + the caller's routing attributes are merged with the schema version into the + attribute map. Pub/Sub subscription filters can only match on *attributes*, + never the body, so every key a consumer might filter by has to live here; + the JSON body carries the fuller payload for consumers that need more than + the filter keys. Pure/synchronous so the attribute wiring is unit-testable + without touching the network. + """ + all_attrs = { + "event_type": event_type, + "schema_version": EVENT_SCHEMA_VERSION, + **attributes, + } + return json.dumps(payload).encode("utf-8"), all_attrs + + +async def _publish_event( + topic: str, event_type: str, payload: dict, attributes: dict[str, str] +) -> None: + """Publish one domain event to ``topic``. Failures are logged, not raised. + + The publish is best-effort by design: the Run row is already durably + persisted before any event is published, so a lost publish means a + delayed/missed downstream reaction, not lost primary state. + """ + data, all_attrs = _build_event(event_type, payload, attributes) + try: + await asyncio.to_thread(_publish_sync, topic, data, all_attrs) + except Exception: + log.exception("Failed to publish %s event", event_type) + + +async def publish_run_completed(run_id: str, agent: str, status: str) -> None: + """Publish a ``run.completed`` event to the run-domain topic. + + Topics follow a per-domain convention, ``-events`` (the GCP project + is hackbot-only, so no ``hackbot-`` prefix); this is the agent-run domain + (``settings.run_events_topic``, default ``agent-run-events``). A new domain + (e.g. subscriber notifications, or + inter-service coordination events) gets its OWN topic plus a small wrapper + like this one, rather than overloading this stream — keeping IAM, retention, + and schema separable per domain as the system grows. Finer selection within + a domain is done by consumers via subscription filters on the attributes + set here (`event_type`, `agent`, `status`). + """ + await _publish_event( + settings.run_events_topic, + event_type="run.completed", + payload={"run_id": run_id, "agent": agent, "status": status}, + attributes={"agent": agent, "status": status}, + ) diff --git a/services/hackbot-api/app/routers/__init__.py b/services/hackbot-api/app/routers/__init__.py index 185d634d4e..b73c765be9 100644 --- a/services/hackbot-api/app/routers/__init__.py +++ b/services/hackbot-api/app/routers/__init__.py @@ -1,3 +1,4 @@ +from app.routers.events import router as events_router from app.routers.runs import router as runs_router -__all__ = ["runs_router"] +__all__ = ["events_router", "runs_router"] diff --git a/services/hackbot-api/app/routers/events.py b/services/hackbot-api/app/routers/events.py new file mode 100644 index 0000000000..cf9e6f98fd --- /dev/null +++ b/services/hackbot-api/app/routers/events.py @@ -0,0 +1,106 @@ +import base64 +import json +import logging +import uuid + +from fastapi import APIRouter, Depends, Request +from sqlalchemy import select +from sqlalchemy.ext.asyncio import AsyncSession + +from app.actions_applier import apply_pending_actions +from app.auth import require_push_auth +from app.database.connection import get_db +from app.database.models import Run +from app.routers.runs import finalize_run + +log = logging.getLogger(__name__) + +router = APIRouter( + prefix="/internal/events", + dependencies=[Depends(require_push_auth)], +) + + +def _decode_pubsub_push_body(body: dict) -> dict: + """Decode a standard Pub/Sub push envelope's `message.data` as JSON. + + Both the Eventarc trigger below (agent-run-finished) and the + `agent-run-events` action-applier subscription deliver via this same + envelope shape. + """ + message = body.get("message") or {} + data = message.get("data") + if not data: + return {} + return json.loads(base64.b64decode(data)) + + +def _cloud_run_execution_name(audit_log_entry: dict) -> str | None: + """Cloud Run Jobs adapter: pull the Execution resource name from an audit log. + + Eventarc wraps the raw Audit Log entry as the CloudEvent payload; the + entry's `protoPayload.resourceName` is the full Execution resource name + (e.g. `projects/P/locations/L/jobs/J/executions/E`), matching what + `app.jobs.get_execution_status` and `Run.execution_name` already use. + + This is the platform-specific half of `agent_run_finished`. Running agents + on another platform (e.g. Cloud Batch) later means adding a sibling parser + that extracts that platform's job identifier from its event, without + touching the platform-neutral finalize path below. + """ + proto_payload = audit_log_entry.get("protoPayload") or audit_log_entry + return proto_payload.get("resourceName") + + +@router.post("/agent-run-finished", status_code=204) +async def agent_run_finished( + request: Request, db: AsyncSession = Depends(get_db) +) -> None: + """Ingress for 'an agent run's underlying execution reached a terminal state'. + + Named by the domain outcome, not the platform mechanism: it's currently + fed by an Eventarc trigger on Cloud Run Jobs execution state, but the name + and the finalize path stay valid if agents move to / add another execution + platform — only the payload parsing (see `_cloud_run_execution_name`) is + platform-specific. + """ + body = await request.json() + event = _decode_pubsub_push_body(body) + execution_name = _cloud_run_execution_name(event) + if not execution_name: + log.warning("agent-run-finished event missing resourceName: %s", event) + return + + result = await db.execute(select(Run).where(Run.execution_name == execution_name)) + run = result.scalar_one_or_none() + if run is None: + log.warning("No run found for execution %s", execution_name) + return + + await finalize_run(db, run) + + +@router.post("/apply-run-actions", status_code=204) +async def apply_run_actions( + request: Request, db: AsyncSession = Depends(get_db) +) -> None: + """Consumer of the `run.completed` event: apply the run's recorded actions. + + Named for what it does, not the event it consumes, because the same + `run.completed` event will feed other consumers later (notifications, + webhooks) — each its own route named after its own job. The subscription + feeding this one is filtered to succeeded runs (see deploy-events.sh). + """ + body = await request.json() + event = _decode_pubsub_push_body(body) + run_id = event.get("run_id") + if not run_id: + log.warning("apply-run-actions event missing run_id: %s", event) + return + + run = await db.get(Run, uuid.UUID(run_id)) + if run is None: + log.warning("No run found for run_id %s", run_id) + return + + await apply_pending_actions(db, run) diff --git a/services/hackbot-api/app/routers/runs.py b/services/hackbot-api/app/routers/runs.py index ff55d466e8..8ec1bbf549 100644 --- a/services/hackbot-api/app/routers/runs.py +++ b/services/hackbot-api/app/routers/runs.py @@ -1,12 +1,13 @@ import json import logging import uuid +from datetime import datetime, timezone from fastapi import APIRouter, Depends, HTTPException, Query, status from sqlalchemy import select from sqlalchemy.ext.asyncio import AsyncSession -from app import gcs, jobs +from app import gcs, jobs, pubsub from app.agents import AGENT_REGISTRY, AgentSpec, model_to_env from app.auth import require_api_key from app.config import settings @@ -14,7 +15,6 @@ from app.database.models import Run from app.jobs import ExecutionStatus from app.schemas import ( - TERMINAL_STATUSES, AgentDescriptor, RunDoc, RunRef, @@ -115,14 +115,12 @@ async def list_runs( @router.get("/runs/{run_id}", response_model=RunDoc) async def get_run(run_id: uuid.UUID, db: AsyncSession = Depends(get_db)) -> RunDoc: + # A plain DB read: completion is detected out-of-band by finalize_run, + # invoked from the Eventarc-triggered /internal/events/agent-run-finished + # route (see app/routers/events.py), not from this request. run = await db.get(Run, run_id) if run is None: raise HTTPException(status_code=404, detail="Run not found") - - if run.status in {s.value for s in TERMINAL_STATUSES} or run.execution_name is None: - return RunDoc.model_validate(run) - - await _reconcile(db, run) return RunDoc.model_validate(run) @@ -142,15 +140,6 @@ async def get_artifact_download_url( if run is None: raise HTTPException(status_code=404, detail="Run not found") - # Refresh artifacts for runs that may have completed since the last poll, - # so a freshly finished run's artifacts are visible here without first - # requiring a GET /runs/{run_id} call. - if ( - run.status not in {s.value for s in TERMINAL_STATUSES} - and run.execution_name is not None - ): - await _reconcile(db, run) - known = {a.get("name") for a in (run.artifacts or [])} if artifact_path not in known: raise HTTPException(status_code=404, detail="Artifact not found") @@ -162,7 +151,16 @@ async def get_artifact_download_url( return {"url": url} -async def _reconcile(db: AsyncSession, run: Run) -> None: +async def finalize_run(db: AsyncSession, run: Run) -> None: + """Bring `run` to its terminal state and publish RunCompleted, once. + + Invoked from the Eventarc-triggered agent-run-finished route instead + of from a client request. Idempotent via `finalized_at`, since Eventarc's + at-least-once delivery can call this more than once for the same run. + """ + if run.finalized_at is not None: + return + assert run.execution_name is not None try: exec_status = await jobs.get_execution_status(run.execution_name) @@ -190,8 +188,10 @@ async def _reconcile(db: AsyncSession, run: Run) -> None: run.summary = summary.model_dump(mode="json") if error is not None: run.error = error + run.finalized_at = datetime.now(timezone.utc) await db.commit() + await pubsub.publish_run_completed(str(run.run_id), run.agent, run.status) def _terminal_status( diff --git a/services/hackbot-api/app/schemas.py b/services/hackbot-api/app/schemas.py index 3b01e033cf..79a9f4a082 100644 --- a/services/hackbot-api/app/schemas.py +++ b/services/hackbot-api/app/schemas.py @@ -14,9 +14,6 @@ class RunStatus(str, Enum): timed_out = "timed_out" -TERMINAL_STATUSES = {RunStatus.succeeded, RunStatus.failed, RunStatus.timed_out} - - class ArtifactRef(BaseModel): name: str size: int @@ -27,6 +24,7 @@ class RunSummary(BaseModel): status: str error: str | None = None findings: dict[str, Any] = Field(default_factory=dict) + actions: list[dict[str, Any]] = Field(default_factory=list) class AgentDescriptor(BaseModel): diff --git a/services/hackbot-api/pyproject.toml b/services/hackbot-api/pyproject.toml index 69f555c7e1..10dd2a7927 100644 --- a/services/hackbot-api/pyproject.toml +++ b/services/hackbot-api/pyproject.toml @@ -14,12 +14,18 @@ dependencies = [ "alembic>=1.13.1", "google-cloud-storage>=2.16.0", "google-cloud-run>=0.10.0", + "google-cloud-pubsub>=2.21.0", + "google-auth>=2.29.0", "sentry-sdk>=2.51.0", + "hackbot-runtime", ] [project.optional-dependencies] dev = ["pytest>=8.0.0", "pytest-asyncio>=0.23.0", "httpx>=0.26.0"] +[tool.uv.sources] +hackbot-runtime = { workspace = true } + [build-system] requires = ["hatchling"] build-backend = "hatchling.build" diff --git a/services/hackbot-api/tests/test_actions_applier.py b/services/hackbot-api/tests/test_actions_applier.py new file mode 100644 index 0000000000..357de02a83 --- /dev/null +++ b/services/hackbot-api/tests/test_actions_applier.py @@ -0,0 +1,87 @@ +"""Tests for the action applier. + +Covers the {{actions..}} placeholder resolver (the mechanism that +lets a later action reference an earlier one's apply-time result) and the +succeeded-run-only gate — see app/actions_applier.py. +""" + +import uuid +from dataclasses import dataclass, field + +from app.actions_applier import apply_pending_actions, resolve_placeholders +from app.schemas import RunStatus + + +def test_resolves_known_ref_and_field(): + out = resolve_placeholders( + "Fix submitted: {{actions.patch.revision_url}}", + {"patch": {"revision_url": "https://phabricator.services.mozilla.com/D1"}}, + ) + assert out == "Fix submitted: https://phabricator.services.mozilla.com/D1" + + +def test_unknown_ref_left_as_is(): + out = resolve_placeholders("See {{actions.missing.url}}", {}) + assert out == "See {{actions.missing.url}}" + + +def test_unknown_field_left_as_is(): + out = resolve_placeholders( + "See {{actions.patch.nope}}", {"patch": {"revision_url": "x"}} + ) + assert out == "See {{actions.patch.nope}}" + + +def test_recurses_into_dict_and_list(): + value = { + "text": "{{actions.patch.revision_url}}", + "items": ["{{actions.patch.revision_id}}", "plain"], + } + out = resolve_placeholders( + value, {"patch": {"revision_url": "u", "revision_id": 5}} + ) + assert out == {"text": "u", "items": ["5", "plain"]} + + +def test_non_string_values_pass_through(): + assert resolve_placeholders(42, {}) == 42 + assert resolve_placeholders(None, {}) is None + assert resolve_placeholders(True, {}) is True + + +def test_multiple_placeholders_in_one_string(): + out = resolve_placeholders( + "{{actions.a.x}} and {{actions.b.y}}", + {"a": {"x": "1"}, "b": {"y": "2"}}, + ) + assert out == "1 and 2" + + +# --- succeeded-run-only gate ------------------------------------------- # + + +@dataclass +class _FakeRun: + status: str + run_id: uuid.UUID = field(default_factory=uuid.uuid4) + summary: dict | None = None + + +class _FakeDB: + """Fails loudly if touched — a skipped run must not query/write anything.""" + + async def execute(self, *a, **k): + raise AssertionError( + "apply_pending_actions must not touch the DB when skipping" + ) + + +async def test_skips_non_succeeded_runs(): + # An action recorded on a failed run must NOT be applied. _FakeDB raises + # if apply_pending_actions gets past the status gate to _ensure_rows. + for status in (RunStatus.failed.value, RunStatus.timed_out.value): + run = _FakeRun( + status=status, + summary={"actions": [{"type": "bugzilla.add_comment", "params": {}}]}, + ) + await apply_pending_actions(_FakeDB(), run) # must not raise diff --git a/services/hackbot-api/tests/test_events.py b/services/hackbot-api/tests/test_events.py new file mode 100644 index 0000000000..64a40230fc --- /dev/null +++ b/services/hackbot-api/tests/test_events.py @@ -0,0 +1,41 @@ +"""Tests for the Pub/Sub push envelope decoding used by both internal event routes (agent-run-finished, apply-run-actions).""" + +import base64 +import json + +from app.routers.events import _cloud_run_execution_name, _decode_pubsub_push_body + + +def _push_envelope(payload: dict) -> dict: + data = base64.b64encode(json.dumps(payload).encode()).decode() + return {"message": {"data": data, "messageId": "1"}, "subscription": "sub"} + + +def test_decode_pubsub_push_body_round_trips(): + body = _push_envelope({"run_id": "abc", "status": "succeeded"}) + assert _decode_pubsub_push_body(body) == {"run_id": "abc", "status": "succeeded"} + + +def test_decode_pubsub_push_body_missing_message(): + assert _decode_pubsub_push_body({}) == {} + + +def test_decode_pubsub_push_body_missing_data(): + assert _decode_pubsub_push_body({"message": {}}) == {} + + +def test_cloud_run_execution_name_from_proto_payload(): + event = { + "protoPayload": { + "resourceName": "projects/p/locations/l/jobs/j/executions/e", + "methodName": "google.cloud.run.v2.Executions.RunExecution", + } + } + assert ( + _cloud_run_execution_name(event) == "projects/p/locations/l/jobs/j/executions/e" + ) + + +def test_cloud_run_execution_name_missing(): + assert _cloud_run_execution_name({"protoPayload": {}}) is None + assert _cloud_run_execution_name({}) is None diff --git a/services/hackbot-api/tests/test_finalize_run.py b/services/hackbot-api/tests/test_finalize_run.py new file mode 100644 index 0000000000..03afef1ccb --- /dev/null +++ b/services/hackbot-api/tests/test_finalize_run.py @@ -0,0 +1,141 @@ +"""Tests for finalize_run. + +The Eventarc-triggered /internal/events/agent-run-finished route calls +this instead of a client's GET /runs/{run_id} triggering it (see +app/routers/runs.py). +""" + +import uuid +from dataclasses import dataclass, field +from datetime import datetime, timezone + +import pytest +from app import gcs, jobs, pubsub +from app.jobs import ExecutionStatus +from app.routers.runs import finalize_run +from app.schemas import ArtifactRef, RunStatus, RunSummary + + +@dataclass +class _FakeRun: + run_id: uuid.UUID = field(default_factory=uuid.uuid4) + agent: str = "bug-fix" + status: str = RunStatus.pending.value + execution_name: str | None = "projects/p/locations/l/jobs/j/executions/e" + artifacts: list = field(default_factory=list) + summary: dict | None = None + error: str | None = None + finalized_at: datetime | None = None + + +class _FakeDB: + def __init__(self): + self.commits = 0 + + async def commit(self): + self.commits += 1 + + +@pytest.fixture(autouse=True) +def _no_publish(monkeypatch): + published = [] + + async def fake_publish(run_id, agent, status): + published.append((run_id, agent, status)) + + monkeypatch.setattr(pubsub, "publish_run_completed", fake_publish) + return published + + +async def test_noop_when_already_finalized(monkeypatch): + run = _FakeRun(finalized_at=datetime.now(timezone.utc)) + db = _FakeDB() + + async def fail(*_a, **_k): + raise AssertionError("should not check execution status once finalized") + + monkeypatch.setattr(jobs, "get_execution_status", fail) + await finalize_run(db, run) + assert db.commits == 0 + + +def _async(value): + """An async callable that ignores its args and returns `value`.""" + + async def _fn(*_args, **_kwargs): + return value + + return _fn + + +async def test_transitions_pending_to_running(monkeypatch): + run = _FakeRun(status=RunStatus.pending.value) + db = _FakeDB() + monkeypatch.setattr(jobs, "get_execution_status", _async(ExecutionStatus.running)) + await finalize_run(db, run) + assert run.status == RunStatus.running.value + assert run.finalized_at is None + assert db.commits == 1 + + +async def test_finalizes_succeeded_run(monkeypatch, _no_publish): + run = _FakeRun() + db = _FakeDB() + monkeypatch.setattr(jobs, "get_execution_status", _async(ExecutionStatus.succeeded)) + monkeypatch.setattr(gcs, "read_summary", _async(RunSummary(status="ok"))) + monkeypatch.setattr( + gcs, "list_artifacts", _async([ArtifactRef(name="summary.json", size=10)]) + ) + + await finalize_run(db, run) + + assert run.status == RunStatus.succeeded.value + assert run.finalized_at is not None + assert run.artifacts == [{"name": "summary.json", "size": 10, "content_type": None}] + assert _no_publish == [(str(run.run_id), run.agent, RunStatus.succeeded.value)] + + +async def test_finalizes_as_failed_when_summary_missing(monkeypatch): + run = _FakeRun() + db = _FakeDB() + monkeypatch.setattr(jobs, "get_execution_status", _async(ExecutionStatus.succeeded)) + monkeypatch.setattr(gcs, "read_summary", _async(None)) + monkeypatch.setattr(gcs, "list_artifacts", _async([])) + + await finalize_run(db, run) + + assert run.status == RunStatus.failed.value + assert "summary.json" in run.error + assert run.finalized_at is not None + + +async def test_cancelled_execution_marks_timed_out(monkeypatch): + run = _FakeRun() + db = _FakeDB() + monkeypatch.setattr(jobs, "get_execution_status", _async(ExecutionStatus.cancelled)) + monkeypatch.setattr(gcs, "read_summary", _async(None)) + monkeypatch.setattr(gcs, "list_artifacts", _async([])) + + await finalize_run(db, run) + + assert run.status == RunStatus.timed_out.value + assert run.finalized_at is not None + + +async def test_second_call_is_noop_after_finalizing(monkeypatch): + run = _FakeRun() + db = _FakeDB() + calls = [] + + async def fake_status(name): + calls.append(name) + return ExecutionStatus.succeeded + + monkeypatch.setattr(jobs, "get_execution_status", fake_status) + monkeypatch.setattr(gcs, "read_summary", _async(RunSummary(status="ok"))) + monkeypatch.setattr(gcs, "list_artifacts", _async([])) + + await finalize_run(db, run) + await finalize_run(db, run) + + assert len(calls) == 1 diff --git a/services/hackbot-api/tests/test_pubsub.py b/services/hackbot-api/tests/test_pubsub.py new file mode 100644 index 0000000000..35f7fc3572 --- /dev/null +++ b/services/hackbot-api/tests/test_pubsub.py @@ -0,0 +1,60 @@ +"""Tests for event publishing: the attributes consumers filter on. + +Subscription filters match only on message attributes (never the body), so +these lock in the attribute set a run.completed event carries — the routing +keys the action-applier's filter (and future consumers') depend on. +""" + +import json + +from app import pubsub + + +def test_build_event_merges_event_type_and_schema_version(): + data, attrs = pubsub._build_event( + "run.completed", + {"run_id": "r1", "agent": "bug-fix", "status": "succeeded"}, + {"agent": "bug-fix", "status": "succeeded"}, + ) + assert attrs == { + "event_type": "run.completed", + "schema_version": pubsub.EVENT_SCHEMA_VERSION, + "agent": "bug-fix", + "status": "succeeded", + } + assert json.loads(data) == { + "run_id": "r1", + "agent": "bug-fix", + "status": "succeeded", + } + + +async def test_publish_run_completed_publishes_filterable_attributes(monkeypatch): + captured = {} + + def fake_sync(topic, data, attributes): + captured["topic"] = topic + captured["data"] = data + captured["attributes"] = attributes + return "msg-id" + + monkeypatch.setattr(pubsub, "_publish_sync", fake_sync) + + await pubsub.publish_run_completed("run-1", "bug-fix", "failed") + + assert captured["topic"] == pubsub.settings.run_events_topic + # The keys the applier subscription filter matches on must be present. + assert captured["attributes"]["event_type"] == "run.completed" + assert captured["attributes"]["status"] == "failed" + assert captured["attributes"]["agent"] == "bug-fix" + assert json.loads(captured["data"])["run_id"] == "run-1" + + +async def test_publish_failure_is_swallowed(monkeypatch): + def boom(*a, **k): + raise RuntimeError("pubsub down") + + monkeypatch.setattr(pubsub, "_publish_sync", boom) + # Best-effort: a publish failure must not propagate (run is already + # finalized before this is called). + await pubsub.publish_run_completed("run-1", "bug-fix", "succeeded") diff --git a/uv.lock b/uv.lock index 7e0b7f4880..b7a81cf85a 100644 --- a/uv.lock +++ b/uv.lock @@ -2079,6 +2079,26 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/84/4a/98da8930ab109c73d9a5d13782a9ebb81ea8c111f6d534a567b71d23e52b/google_cloud_core-2.6.0-py3-none-any.whl", hash = "sha256:6d63ac8e5eca6d9e4319d0a1e2265fadcd7f1049904378caecfa01cf52dd869e", size = 29390, upload-time = "2026-05-07T08:02:34.672Z" }, ] +[[package]] +name = "google-cloud-pubsub" +version = "2.39.0" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "google-api-core", extra = ["grpc"] }, + { name = "google-auth" }, + { name = "grpc-google-iam-v1" }, + { name = "grpcio" }, + { name = "grpcio-status" }, + { name = "opentelemetry-api" }, + { name = "opentelemetry-sdk" }, + { name = "proto-plus" }, + { name = "protobuf" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/11/2b/4bf2c17e319ff65340389565b0e1b4d72696d87802b2f5f94390fbefa73c/google_cloud_pubsub-2.39.0.tar.gz", hash = "sha256:eed65e25f57f95bf3e02d96d7ee171688b23922471f9f21b5a91ed90e1282c0f", size = 402096, upload-time = "2026-06-03T15:28:26.396Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/93/20/dd0b27d4ad4577c062e77ff968ca3e2d404186cd78c8a2a53a0ef5fe5389/google_cloud_pubsub-2.39.0-py3-none-any.whl", hash = "sha256:7210d691a46d7a66559696899ebe6eb731e63de29b624964b3be4dd2d12d3e19", size = 324665, upload-time = "2026-06-03T15:27:41.119Z" }, +] + [[package]] name = "google-cloud-run" version = "0.16.0" @@ -2595,8 +2615,11 @@ dependencies = [ { name = "asyncpg" }, { name = "cloud-sql-python-connector", extra = ["asyncpg"] }, { name = "fastapi" }, + { name = "google-auth" }, + { name = "google-cloud-pubsub" }, { name = "google-cloud-run" }, { name = "google-cloud-storage" }, + { name = "hackbot-runtime" }, { name = "pydantic" }, { name = "pydantic-settings" }, { name = "sentry-sdk" }, @@ -2617,8 +2640,11 @@ requires-dist = [ { name = "asyncpg", specifier = ">=0.29.0" }, { name = "cloud-sql-python-connector", extras = ["asyncpg"], specifier = ">=1.5.0" }, { name = "fastapi", specifier = ">=0.109.0" }, + { name = "google-auth", specifier = ">=2.29.0" }, + { name = "google-cloud-pubsub", specifier = ">=2.21.0" }, { name = "google-cloud-run", specifier = ">=0.10.0" }, { name = "google-cloud-storage", specifier = ">=2.16.0" }, + { name = "hackbot-runtime", editable = "libs/hackbot-runtime" }, { name = "httpx", marker = "extra == 'dev'", specifier = ">=0.26.0" }, { name = "pydantic", specifier = ">=2.6.0" }, { name = "pydantic-settings", specifier = ">=2.1.0" }, From 91f01d7c8cf8663cb8b3e919a7c2c7160835f1f5 Mon Sep 17 00:00:00 2001 From: Suhaib Mujahid Date: Sun, 5 Jul 2026 00:59:21 -0400 Subject: [PATCH 05/13] Parse Cloud Run completion logs and match Runs --- services/hackbot-api/app/routers/events.py | 75 +++++++++++++++------- services/hackbot-api/tests/test_events.py | 56 ++++++++++++---- 2 files changed, 98 insertions(+), 33 deletions(-) diff --git a/services/hackbot-api/app/routers/events.py b/services/hackbot-api/app/routers/events.py index cf9e6f98fd..9504fd4bb7 100644 --- a/services/hackbot-api/app/routers/events.py +++ b/services/hackbot-api/app/routers/events.py @@ -24,7 +24,7 @@ def _decode_pubsub_push_body(body: dict) -> dict: """Decode a standard Pub/Sub push envelope's `message.data` as JSON. - Both the Eventarc trigger below (agent-run-finished) and the + Both the completion-log push subscription feeding agent-run-finished and the `agent-run-events` action-applier subscription deliver via this same envelope shape. """ @@ -35,21 +35,51 @@ def _decode_pubsub_push_body(body: dict) -> dict: return json.loads(base64.b64decode(data)) -def _cloud_run_execution_name(audit_log_entry: dict) -> str | None: - """Cloud Run Jobs adapter: pull the Execution resource name from an audit log. +def _execution_name_from_completion_log(entry: dict) -> str | None: + """Cloud Run Jobs adapter: pull the Execution name from a completion LogEntry. - Eventarc wraps the raw Audit Log entry as the CloudEvent payload; the - entry's `protoPayload.resourceName` is the full Execution resource name - (e.g. `projects/P/locations/L/jobs/J/executions/E`), matching what - `app.jobs.get_execution_status` and `Run.execution_name` already use. + A logging sink routes the control-plane `system_event` audit log that fires + when an execution's `Completed` condition changes (success or failure); + Pub/Sub delivers that LogEntry as the message body. The execution resource + name can appear in a few places depending on the log format, so check the + likely ones. Correlation to `Run.execution_name` (set from the run_v2 + execution name) tolerates prefix differences via a suffix match in the + route, so returning any of these forms is fine. - This is the platform-specific half of `agent_run_finished`. Running agents - on another platform (e.g. Cloud Batch) later means adding a sibling parser - that extracts that platform's job identifier from its event, without - touching the platform-neutral finalize path below. + Only this half is Cloud-Run-specific: a future platform adds a sibling + parser feeding the same platform-neutral finalize path below. """ - proto_payload = audit_log_entry.get("protoPayload") or audit_log_entry - return proto_payload.get("resourceName") + proto_payload = entry.get("protoPayload") or {} + response = proto_payload.get("response") or {} + metadata = response.get("metadata") or {} + labels = entry.get("labels") or {} + return ( + proto_payload.get("resourceName") + or metadata.get("name") + or labels.get("run.googleapis.com/execution_name") + ) + + +async def _find_run_for_execution(db: AsyncSession, execution_name: str) -> Run | None: + """Find the Run for a completion event's execution name. + + Prefers an exact match on the stored execution_name; falls back to matching + the execution short-name (last path segment) as a suffix, so a v1 + (`namespaces/...`) vs v2 (`projects/.../executions/E`) prefix mismatch + between the log entry and what run_v2 stored doesn't break correlation. + """ + result = await db.execute(select(Run).where(Run.execution_name == execution_name)) + run = result.scalar_one_or_none() + if run is not None: + return run + + short = execution_name.rsplit("/", 1)[-1] + if short and short != execution_name: + result = await db.execute( + select(Run).where(Run.execution_name.like(f"%/{short}")) + ) + run = result.scalar_one_or_none() + return run @router.post("/agent-run-finished", status_code=204) @@ -58,21 +88,22 @@ async def agent_run_finished( ) -> None: """Ingress for 'an agent run's underlying execution reached a terminal state'. - Named by the domain outcome, not the platform mechanism: it's currently - fed by an Eventarc trigger on Cloud Run Jobs execution state, but the name - and the finalize path stay valid if agents move to / add another execution - platform — only the payload parsing (see `_cloud_run_execution_name`) is - platform-specific. + Named by the domain outcome, not the platform mechanism: it's fed by a + Cloud Logging sink on Cloud Run Jobs `system_event` completion logs (which + fire for success and failure alike, incl. OOM/crash). The name and the + finalize path stay valid if agents move to / add another execution platform + — only the payload parsing (see `_execution_name_from_completion_log`) is + platform-specific. `finalize_run` re-queries the authoritative status, so + this route just needs to identify which run finished. """ body = await request.json() event = _decode_pubsub_push_body(body) - execution_name = _cloud_run_execution_name(event) + execution_name = _execution_name_from_completion_log(event) if not execution_name: - log.warning("agent-run-finished event missing resourceName: %s", event) + log.warning("agent-run-finished event missing execution name: %s", event) return - result = await db.execute(select(Run).where(Run.execution_name == execution_name)) - run = result.scalar_one_or_none() + run = await _find_run_for_execution(db, execution_name) if run is None: log.warning("No run found for execution %s", execution_name) return diff --git a/services/hackbot-api/tests/test_events.py b/services/hackbot-api/tests/test_events.py index 64a40230fc..214ca8e912 100644 --- a/services/hackbot-api/tests/test_events.py +++ b/services/hackbot-api/tests/test_events.py @@ -1,9 +1,16 @@ -"""Tests for the Pub/Sub push envelope decoding used by both internal event routes (agent-run-finished, apply-run-actions).""" +"""Tests for the internal event routes (agent-run-finished, apply-run-actions). + +Covers the Pub/Sub push envelope decode and extracting the execution name from +a Cloud Run Jobs `system_event` completion LogEntry (routed via a logging sink). +""" import base64 import json -from app.routers.events import _cloud_run_execution_name, _decode_pubsub_push_body +from app.routers.events import ( + _decode_pubsub_push_body, + _execution_name_from_completion_log, +) def _push_envelope(payload: dict) -> dict: @@ -24,18 +31,45 @@ def test_decode_pubsub_push_body_missing_data(): assert _decode_pubsub_push_body({"message": {}}) == {} -def test_cloud_run_execution_name_from_proto_payload(): - event = { +def _completion_log(status: str) -> dict: + """A Cloud Run Jobs execution-completion system_event LogEntry.""" + return { "protoPayload": { "resourceName": "projects/p/locations/l/jobs/j/executions/e", - "methodName": "google.cloud.run.v2.Executions.RunExecution", + "response": { + "status": {"conditions": [{"type": "Completed", "status": status}]} + }, + }, + "resource": { + "type": "cloud_run_job", + "labels": {"job_name": "hackbot-agent-bug-fix"}, + }, + } + + +def test_execution_name_from_completion_log_success_and_failure(): + # Both terminal outcomes carry the same execution resourceName. + for status in ("True", "False"): + assert ( + _execution_name_from_completion_log(_completion_log(status)) + == "projects/p/locations/l/jobs/j/executions/e" + ) + + +def test_execution_name_falls_back_to_response_metadata_name(): + entry = { + "protoPayload": { + "response": {"metadata": {"name": "namespaces/p/executions/e"}} } } - assert ( - _cloud_run_execution_name(event) == "projects/p/locations/l/jobs/j/executions/e" - ) + assert _execution_name_from_completion_log(entry) == "namespaces/p/executions/e" + + +def test_execution_name_falls_back_to_labels(): + entry = {"labels": {"run.googleapis.com/execution_name": "e-123"}} + assert _execution_name_from_completion_log(entry) == "e-123" -def test_cloud_run_execution_name_missing(): - assert _cloud_run_execution_name({"protoPayload": {}}) is None - assert _cloud_run_execution_name({}) is None +def test_execution_name_missing(): + assert _execution_name_from_completion_log({"protoPayload": {}}) is None + assert _execution_name_from_completion_log({}) is None From 12c0348dd3a43d9328b1cda7eadf71d1e6a2fc5f Mon Sep 17 00:00:00 2001 From: Suhaib Mujahid Date: Thu, 9 Jul 2026 15:39:05 -0400 Subject: [PATCH 06/13] hackbot-api: per-agent opt-in for auto-applying actions + manual apply Auto-applying a run's recorded actions is now off by default and opt-in per agent (`AgentSpec.auto_apply_actions`, default False). On a succeeded run the actions are always recorded as run_actions rows so they're visible; they're applied automatically only if the agent opted in, otherwise left pending. Refactors the applier into ensure_action_rows + _apply_pending_rows, with on_run_completed (record + conditional auto-apply, from the apply-run-actions event) and apply_all_pending (manual). Adds GET /runs/{id}/actions and POST /runs/{id}/actions/apply (apply-all, idempotent) so the UI can list and apply pending actions, plus the RunActionDoc schema. --- services/hackbot-api/app/actions_applier.py | 98 +++++++++++++------ services/hackbot-api/app/agents.py | 4 + services/hackbot-api/app/routers/events.py | 6 +- services/hackbot-api/app/routers/runs.py | 37 ++++++- services/hackbot-api/app/schemas.py | 15 +++ .../hackbot-api/tests/test_actions_applier.py | 91 ++++++++++++++--- .../hackbot-api/tests/test_run_actions_api.py | 66 +++++++++++++ 7 files changed, 265 insertions(+), 52 deletions(-) create mode 100644 services/hackbot-api/tests/test_run_actions_api.py diff --git a/services/hackbot-api/app/actions_applier.py b/services/hackbot-api/app/actions_applier.py index 7e5ef32301..b64b4a81d7 100644 --- a/services/hackbot-api/app/actions_applier.py +++ b/services/hackbot-api/app/actions_applier.py @@ -1,12 +1,13 @@ -"""Apply a run's recorded actions once it has finished. - -Triggered by the `agent-run-events` push subscription (see -`app/routers/events.py`), after `finalize_run` has already persisted the -run's terminal status and `summary.json`. Reads `summary["actions"]`, upserts -one `RunAction` row per entry, and runs each pending one through the handler -registry in `hackbot_runtime.actions.handlers` — idempotent per action, so a -Pub/Sub retry of the same message only re-attempts actions that didn't reach -`applied` last time. +"""Record and (optionally) apply a run's actions once it has finished. + +On run completion the recorded actions from `summary["actions"]` are always +upserted as `run_actions` rows (one per entry) so they're visible and +manageable in the UI. Whether they're then applied *automatically* depends on +the agent's `auto_apply_actions` opt-in (see `app/agents.py`); either way they +can be applied on demand (manual apply-all from the UI). Application runs each +pending row through the handler registry in `hackbot_runtime.actions.handlers` +and is idempotent per action — an already-`applied` row is never re-applied, so +Pub/Sub retries and repeated manual applies are safe. """ from __future__ import annotations @@ -21,6 +22,7 @@ from sqlalchemy.ext.asyncio import AsyncSession from app import gcs +from app.agents import AGENT_REGISTRY from app.database.models import Run, RunAction from app.schemas import RunStatus @@ -54,13 +56,14 @@ def _sub(match: re.Match) -> str: return value -async def _ensure_rows( +async def ensure_action_rows( db: AsyncSession, run: Run ) -> list[tuple[RunAction, list[dict]]]: - """Upsert one `RunAction` per recorded action. + """Upsert one `RunAction` per recorded action (does not apply them). Returns each row paired with its (not persisted) attachments list from - summary.json. + summary.json. Idempotent: existing rows are reused, so this can run on + every completion and again on each manual apply. """ actions: list[dict] = (run.summary or {}).get("actions", []) @@ -85,27 +88,16 @@ async def _ensure_rows( return rows -async def apply_pending_actions(db: AsyncSession, run: Run) -> None: - # Only a successful run's actions are applied. A failed/timed-out run may - # have recorded actions before it errored, but submitting a patch or - # posting a comment from a run that never reached a verified-good state - # isn't wanted. The applier's Pub/Sub subscription filters to - # status="succeeded" as the primary routing (failed runs never reach here), - # so this check is defense-in-depth: it keeps the applier correct when - # invoked directly (tests, future callers) and documents the policy in - # code. `RunCompleted` is still published for every terminal status (see - # finalize_run), so a future failure-notifier consumer can filter for the - # ones this applier ignores. - if run.status != RunStatus.succeeded.value: - log.info( - "Skipping action application for run %s (status=%s)", - run.run_id, - run.status, - ) - return - - rows = await _ensure_rows(db, run) +async def _apply_pending_rows( + db: AsyncSession, run: Run, rows: list[tuple[RunAction, list[dict]]] +) -> None: + """Apply every not-yet-`applied` row in `rows`, committing per action. + Cross-action `{{actions..}}` placeholders resolve against rows + that are already `applied` (seeded from prior applies) plus ones applied + earlier in this pass, so a later (even manual) apply can still reference an + earlier action's result. + """ results_by_ref: dict[str, dict] = { row.ref: row.result for row, _ in rows @@ -154,3 +146,45 @@ async def apply_pending_actions(db: AsyncSession, run: Run) -> None: if row.status == "applied" and row.ref and row.result is not None: results_by_ref[row.ref] = row.result + + +async def on_run_completed(db: AsyncSession, run: Run) -> None: + """Record a completed run's actions, and auto-apply them if the agent opts in. + + Called from the `apply-run-actions` push route. Actions are always recorded + (so the UI can show/manually apply them); they're applied automatically only + when the run's agent has `auto_apply_actions=True`. + """ + # Defense-in-depth: only a succeeded run's actions are recorded/applied. A + # failed/timed-out run may have recorded actions before erroring, but acting + # on a run that never reached a verified-good state isn't wanted. The + # Pub/Sub subscription already filters to status="succeeded"; this keeps the + # function correct if invoked directly. + if run.status != RunStatus.succeeded.value: + log.info("Skipping actions for run %s (status=%s)", run.run_id, run.status) + return + + rows = await ensure_action_rows(db, run) + await db.commit() + + spec = AGENT_REGISTRY.get(run.agent) + if spec and spec.auto_apply_actions: + await _apply_pending_rows(db, run, rows) + else: + log.info( + "Recorded %d action(s) for run %s; auto-apply off for agent %s", + len(rows), + run.run_id, + run.agent, + ) + + +async def apply_all_pending(db: AsyncSession, run: Run) -> None: + """Apply all of a run's not-yet-`applied` actions on demand (manual). + + Ensures the rows exist first, so this works whether or not they were + recorded automatically on completion. + """ + rows = await ensure_action_rows(db, run) + await db.commit() + await _apply_pending_rows(db, run, rows) diff --git a/services/hackbot-api/app/agents.py b/services/hackbot-api/app/agents.py index 6ad1c5c00d..eb5aa804bb 100644 --- a/services/hackbot-api/app/agents.py +++ b/services/hackbot-api/app/agents.py @@ -22,6 +22,10 @@ class AgentSpec: # Optional override for the rare agent whose env vars don't map 1:1 from # its input schema. Defaults to ``model_to_env`` (field -> UPPER_SNAKE env). build_env: Callable[[BaseModel], dict[str, str]] | None = None + # Whether this agent's recorded actions are applied AUTOMATICALLY when a run + # succeeds. Off by default: actions are still recorded and can always be + # applied manually from the UI; only opted-in agents auto-apply. + auto_apply_actions: bool = False def model_to_env(inputs: BaseModel) -> dict[str, str]: diff --git a/services/hackbot-api/app/routers/events.py b/services/hackbot-api/app/routers/events.py index 9504fd4bb7..d46da7ab6d 100644 --- a/services/hackbot-api/app/routers/events.py +++ b/services/hackbot-api/app/routers/events.py @@ -7,7 +7,7 @@ from sqlalchemy import select from sqlalchemy.ext.asyncio import AsyncSession -from app.actions_applier import apply_pending_actions +from app.actions_applier import on_run_completed from app.auth import require_push_auth from app.database.connection import get_db from app.database.models import Run @@ -115,7 +115,7 @@ async def agent_run_finished( async def apply_run_actions( request: Request, db: AsyncSession = Depends(get_db) ) -> None: - """Consumer of the `run.completed` event: apply the run's recorded actions. + """Consumer of `run.completed`: record the run's actions, auto-apply if opted in. Named for what it does, not the event it consumes, because the same `run.completed` event will feed other consumers later (notifications, @@ -134,4 +134,4 @@ async def apply_run_actions( log.warning("No run found for run_id %s", run_id) return - await apply_pending_actions(db, run) + await on_run_completed(db, run) diff --git a/services/hackbot-api/app/routers/runs.py b/services/hackbot-api/app/routers/runs.py index 8ec1bbf549..083f5b0f7f 100644 --- a/services/hackbot-api/app/routers/runs.py +++ b/services/hackbot-api/app/routers/runs.py @@ -8,14 +8,16 @@ from sqlalchemy.ext.asyncio import AsyncSession from app import gcs, jobs, pubsub +from app.actions_applier import apply_all_pending from app.agents import AGENT_REGISTRY, AgentSpec, model_to_env from app.auth import require_api_key from app.config import settings from app.database.connection import get_db -from app.database.models import Run +from app.database.models import Run, RunAction from app.jobs import ExecutionStatus from app.schemas import ( AgentDescriptor, + RunActionDoc, RunDoc, RunRef, RunStatus, @@ -151,6 +153,39 @@ async def get_artifact_download_url( return {"url": url} +async def _list_actions(db: AsyncSession, run_id: uuid.UUID) -> list[RunActionDoc]: + result = await db.execute( + select(RunAction).where(RunAction.run_id == run_id).order_by(RunAction.idx) + ) + return [RunActionDoc.model_validate(r) for r in result.scalars()] + + +@router.get("/runs/{run_id}/actions", response_model=list[RunActionDoc]) +async def list_run_actions( + run_id: uuid.UUID, db: AsyncSession = Depends(get_db) +) -> list[RunActionDoc]: + run = await db.get(Run, run_id) + if run is None: + raise HTTPException(status_code=404, detail="Run not found") + return await _list_actions(db, run_id) + + +@router.post("/runs/{run_id}/actions/apply", response_model=list[RunActionDoc]) +async def apply_run_actions( + run_id: uuid.UUID, db: AsyncSession = Depends(get_db) +) -> list[RunActionDoc]: + """Manually apply all of a run's pending actions (apply-all). + + Idempotent — already-applied actions are skipped — so this is safe to + click again after a partial failure. Returns the actions' updated state. + """ + run = await db.get(Run, run_id) + if run is None: + raise HTTPException(status_code=404, detail="Run not found") + await apply_all_pending(db, run) + return await _list_actions(db, run_id) + + async def finalize_run(db: AsyncSession, run: Run) -> None: """Bring `run` to its terminal state and publish RunCompleted, once. diff --git a/services/hackbot-api/app/schemas.py b/services/hackbot-api/app/schemas.py index 79a9f4a082..057dc88c10 100644 --- a/services/hackbot-api/app/schemas.py +++ b/services/hackbot-api/app/schemas.py @@ -27,6 +27,21 @@ class RunSummary(BaseModel): actions: list[dict[str, Any]] = Field(default_factory=list) +class RunActionDoc(BaseModel): + """A recorded action and its apply state, as shown/driven by the UI.""" + + model_config = ConfigDict(from_attributes=True) + + idx: int + type: str + params: dict[str, Any] + ref: str | None = None + status: str + result: dict[str, Any] | None = None + error: str | None = None + applied_at: datetime | None = None + + class AgentDescriptor(BaseModel): name: str description: str diff --git a/services/hackbot-api/tests/test_actions_applier.py b/services/hackbot-api/tests/test_actions_applier.py index 357de02a83..5101165ffd 100644 --- a/services/hackbot-api/tests/test_actions_applier.py +++ b/services/hackbot-api/tests/test_actions_applier.py @@ -1,14 +1,20 @@ """Tests for the action applier. -Covers the {{actions..}} placeholder resolver (the mechanism that -lets a later action reference an earlier one's apply-time result) and the -succeeded-run-only gate — see app/actions_applier.py. +Covers the {{actions..}} placeholder resolver, the succeeded-run +gate + per-agent auto-apply opt-in in `on_run_completed`, and the manual +`apply_all_pending` path — see app/actions_applier.py. """ import uuid from dataclasses import dataclass, field - -from app.actions_applier import apply_pending_actions, resolve_placeholders +from types import SimpleNamespace + +from app import actions_applier +from app.actions_applier import ( + apply_all_pending, + on_run_completed, + resolve_placeholders, +) from app.schemas import RunStatus @@ -57,31 +63,84 @@ def test_multiple_placeholders_in_one_string(): assert out == "1 and 2" -# --- succeeded-run-only gate ------------------------------------------- # +# --- record / auto-apply gating --------------------------------------- # @dataclass class _FakeRun: status: str + agent: str = "bug-fix" run_id: uuid.UUID = field(default_factory=uuid.uuid4) summary: dict | None = None class _FakeDB: - """Fails loudly if touched — a skipped run must not query/write anything.""" + def __init__(self): + self.commits = 0 + + async def commit(self): + self.commits += 1 async def execute(self, *a, **k): raise AssertionError( - "apply_pending_actions must not touch the DB when skipping" + "ensure_action_rows should be monkeypatched in these tests" ) -async def test_skips_non_succeeded_runs(): - # An action recorded on a failed run must NOT be applied. _FakeDB raises - # if apply_pending_actions gets past the status gate to _ensure_rows. +def _patch_applier(monkeypatch, *, auto: bool | None): + """Stub ensure/apply and the registry; record what got called. + + `auto=None` means the agent isn't in the registry at all. + """ + calls = {"ensured": False, "applied": False} + + async def fake_ensure(db, run): + calls["ensured"] = True + return [("row", [])] + + async def fake_apply(db, run, rows): + calls["applied"] = True + + monkeypatch.setattr(actions_applier, "ensure_action_rows", fake_ensure) + monkeypatch.setattr(actions_applier, "_apply_pending_rows", fake_apply) + registry = ( + {} if auto is None else {"bug-fix": SimpleNamespace(auto_apply_actions=auto)} + ) + monkeypatch.setattr(actions_applier, "AGENT_REGISTRY", registry) + return calls + + +async def test_non_succeeded_run_records_nothing(monkeypatch): + calls = _patch_applier(monkeypatch, auto=True) for status in (RunStatus.failed.value, RunStatus.timed_out.value): - run = _FakeRun( - status=status, - summary={"actions": [{"type": "bugzilla.add_comment", "params": {}}]}, - ) - await apply_pending_actions(_FakeDB(), run) # must not raise + await on_run_completed(_FakeDB(), _FakeRun(status=status)) + assert calls == {"ensured": False, "applied": False} + + +async def test_succeeded_opted_in_agent_records_and_applies(monkeypatch): + calls = _patch_applier(monkeypatch, auto=True) + db = _FakeDB() + await on_run_completed(db, _FakeRun(status=RunStatus.succeeded.value)) + assert calls == {"ensured": True, "applied": True} + assert db.commits >= 1 + + +async def test_succeeded_non_opted_agent_records_but_does_not_apply(monkeypatch): + calls = _patch_applier(monkeypatch, auto=False) + db = _FakeDB() + await on_run_completed(db, _FakeRun(status=RunStatus.succeeded.value)) + assert calls == {"ensured": True, "applied": False} + assert db.commits >= 1 + + +async def test_succeeded_unknown_agent_does_not_apply(monkeypatch): + calls = _patch_applier(monkeypatch, auto=None) + await on_run_completed(_FakeDB(), _FakeRun(status=RunStatus.succeeded.value)) + assert calls == {"ensured": True, "applied": False} + + +async def test_apply_all_pending_always_applies(monkeypatch): + # Manual apply ignores the opt-in flag entirely. + calls = _patch_applier(monkeypatch, auto=False) + await apply_all_pending(_FakeDB(), _FakeRun(status=RunStatus.succeeded.value)) + assert calls == {"ensured": True, "applied": True} diff --git a/services/hackbot-api/tests/test_run_actions_api.py b/services/hackbot-api/tests/test_run_actions_api.py new file mode 100644 index 0000000000..605e17b359 --- /dev/null +++ b/services/hackbot-api/tests/test_run_actions_api.py @@ -0,0 +1,66 @@ +"""Tests for the run-actions HTTP endpoints (list + manual apply-all). + +Exercises the route handlers directly with a fake DB (matching this suite's +fake-based style), stubbing the applier/query helpers to keep the handlers' +own logic — 404 handling, calling apply_all_pending, returning the list — in +focus. +""" + +import uuid +from types import SimpleNamespace + +import pytest +from app.routers import runs as runs_router +from app.schemas import RunActionDoc +from fastapi import HTTPException + + +class _FakeDB: + def __init__(self, run): + self._run = run + + async def get(self, model, run_id): + return self._run + + +_ACTIONS = [ + RunActionDoc(idx=0, type="bugzilla.add_comment", params={}, status="pending") +] + + +async def test_list_run_actions_404(): + with pytest.raises(HTTPException) as exc: + await runs_router.list_run_actions(uuid.uuid4(), _FakeDB(None)) + assert exc.value.status_code == 404 + + +async def test_list_run_actions_returns_rows(monkeypatch): + async def fake_list(db, run_id): + return _ACTIONS + + monkeypatch.setattr(runs_router, "_list_actions", fake_list) + out = await runs_router.list_run_actions(uuid.uuid4(), _FakeDB(SimpleNamespace())) + assert out is _ACTIONS + + +async def test_apply_run_actions_404(): + with pytest.raises(HTTPException) as exc: + await runs_router.apply_run_actions(uuid.uuid4(), _FakeDB(None)) + assert exc.value.status_code == 404 + + +async def test_apply_run_actions_applies_then_returns(monkeypatch): + applied = {"called": False} + + async def fake_apply(db, run): + applied["called"] = True + + async def fake_list(db, run_id): + return _ACTIONS + + monkeypatch.setattr(runs_router, "apply_all_pending", fake_apply) + monkeypatch.setattr(runs_router, "_list_actions", fake_list) + + out = await runs_router.apply_run_actions(uuid.uuid4(), _FakeDB(SimpleNamespace())) + assert applied["called"] is True + assert out is _ACTIONS From 259c87791397675e68c572ce3d0bce043dfc0498 Mon Sep 17 00:00:00 2001 From: Suhaib Mujahid Date: Thu, 9 Jul 2026 15:39:27 -0400 Subject: [PATCH 07/13] hackbot-ui: show a run's actions and apply pending ones manually Adds an "Actions" section to the run detail view listing each recorded action with its apply status, plus an "Apply all pending actions" button (shown only when actions are pending) that calls the new apply endpoint and refreshes. Adds the RunAction type, listRunActions/applyRunActions client fns, and the /api/runs/[runId]/actions proxy route (GET + POST). --- .../app/api/runs/[runId]/actions/route.ts | 42 +++++++++++++ services/hackbot-ui/components/RunDetail.tsx | 60 ++++++++++++++++++- services/hackbot-ui/lib/hackbot.ts | 14 ++++- services/hackbot-ui/lib/types.ts | 15 +++++ 4 files changed, 129 insertions(+), 2 deletions(-) create mode 100644 services/hackbot-ui/app/api/runs/[runId]/actions/route.ts diff --git a/services/hackbot-ui/app/api/runs/[runId]/actions/route.ts b/services/hackbot-ui/app/api/runs/[runId]/actions/route.ts new file mode 100644 index 0000000000..353849c606 --- /dev/null +++ b/services/hackbot-ui/app/api/runs/[runId]/actions/route.ts @@ -0,0 +1,42 @@ +import { NextResponse } from "next/server"; + +import { applyRunActions, HackbotError, listRunActions } from "@/lib/hackbot"; +import { getAuthedEmail } from "@/lib/session"; + +export const dynamic = "force-dynamic"; + +// GET /api/runs/:runId/actions — proxy the run's recorded actions + apply state. +export async function GET( + _req: Request, + { params }: { params: Promise<{ runId: string }> } +) { + if (!(await getAuthedEmail())) { + return NextResponse.json({ error: "Unauthorized" }, { status: 401 }); + } + + const { runId } = await params; + try { + return NextResponse.json(await listRunActions(runId)); + } catch (err) { + const status = err instanceof HackbotError ? err.status : 500; + return NextResponse.json({ error: (err as Error).message }, { status }); + } +} + +// POST /api/runs/:runId/actions — manually apply all pending actions. +export async function POST( + _req: Request, + { params }: { params: Promise<{ runId: string }> } +) { + if (!(await getAuthedEmail())) { + return NextResponse.json({ error: "Unauthorized" }, { status: 401 }); + } + + const { runId } = await params; + try { + return NextResponse.json(await applyRunActions(runId)); + } catch (err) { + const status = err instanceof HackbotError ? err.status : 500; + return NextResponse.json({ error: (err as Error).message }, { status }); + } +} diff --git a/services/hackbot-ui/components/RunDetail.tsx b/services/hackbot-ui/components/RunDetail.tsx index e85828e2ed..42ca4486c8 100644 --- a/services/hackbot-ui/components/RunDetail.tsx +++ b/services/hackbot-ui/components/RunDetail.tsx @@ -4,7 +4,7 @@ import Link from "next/link"; import { useCallback, useEffect, useRef, useState } from "react"; import { updateRunStatus } from "@/lib/store"; -import { isTerminal, type RunDoc } from "@/lib/types"; +import { isTerminal, type RunAction, type RunDoc } from "@/lib/types"; import { FindingsView } from "./FindingsView"; import { StatusBadge } from "./StatusBadge"; @@ -40,6 +40,9 @@ export function RunDetail({ runId }: { runId: string }) { const [run, setRun] = useState(null); const [error, setError] = useState(null); const [polling, setPolling] = useState(true); + const [actions, setActions] = useState(null); + const [applying, setApplying] = useState(false); + const [applyError, setApplyError] = useState(null); const timer = useRef | null>(null); const fetchRun = useCallback(async () => { @@ -78,6 +81,40 @@ export function RunDetail({ runId }: { runId: string }) { }; }, [fetchRun]); + const fetchActions = useCallback(async () => { + try { + const res = await fetch(`/api/runs/${runId}/actions`); + const body = await res.json(); + if (!res.ok) + throw new Error(body?.error ?? `Request failed (${res.status})`); + setActions(body as RunAction[]); + } catch (err) { + // Non-fatal: the actions section just stays hidden. + setApplyError((err as Error).message); + } + }, [runId]); + + // Actions are recorded once the run completes; fetch them then. + useEffect(() => { + if (run && isTerminal(run.status)) fetchActions(); + }, [run, fetchActions]); + + const applyActions = useCallback(async () => { + setApplying(true); + setApplyError(null); + try { + const res = await fetch(`/api/runs/${runId}/actions`, { method: "POST" }); + const body = await res.json(); + if (!res.ok) + throw new Error(body?.error ?? `Request failed (${res.status})`); + setActions(body as RunAction[]); + } catch (err) { + setApplyError((err as Error).message); + } finally { + setApplying(false); + } + }, [runId]); + if (!run && error) { return
{error}
; } @@ -145,6 +182,27 @@ export function RunDetail({ runId }: { runId: string }) { {hasFindings && } + {actions && actions.length > 0 && ( +
+

Actions ({actions.length})

+ {applyError &&
{applyError}
} +
    + {actions.map((a) => ( +
  • + {a.status} + {a.type} + {a.error && {a.error}} +
    • + ))} +
+ {actions.some((a) => a.status === "pending") && ( + + )} +
+ )} +

Artifacts ({run.artifacts.length})

{run.artifacts.length === 0 ? ( diff --git a/services/hackbot-ui/lib/hackbot.ts b/services/hackbot-ui/lib/hackbot.ts index 9d28305028..05cb311d97 100644 --- a/services/hackbot-ui/lib/hackbot.ts +++ b/services/hackbot-ui/lib/hackbot.ts @@ -1,6 +1,6 @@ import "server-only"; -import type { AgentDescriptor, RunDoc, RunRef } from "./types"; +import type { AgentDescriptor, RunAction, RunDoc, RunRef } from "./types"; // Thin server-side client for the hackbot-api. The API key lives here and is // never exposed to the browser — every browser request goes through the @@ -91,6 +91,18 @@ export function listRuns(limit = 50): Promise { return request(`/runs?limit=${limit}`); } +export function listRunActions(runId: string): Promise { + return request(`/runs/${encodeURIComponent(runId)}/actions`); +} + +// Manually apply all of a run's pending actions; returns their updated state. +export function applyRunActions(runId: string): Promise { + return request( + `/runs/${encodeURIComponent(runId)}/actions/apply`, + { method: "POST" }, + ); +} + // Ask hackbot-api for a short-lived signed download URL for one artifact. // `artifactName` may contain slashes; each segment is encoded individually so // the upstream `{artifact_path:path}` route still sees the directory structure. diff --git a/services/hackbot-ui/lib/types.ts b/services/hackbot-ui/lib/types.ts index 033eb64dc0..a7a2a27366 100644 --- a/services/hackbot-ui/lib/types.ts +++ b/services/hackbot-ui/lib/types.ts @@ -36,6 +36,21 @@ export interface RunSummary { findings: Record; } +export type RunActionStatus = "pending" | "applied" | "failed"; + +// Mirror of RunActionDoc (services/hackbot-api/app/schemas.py): a recorded +// agent action and its apply state. +export interface RunAction { + idx: number; + type: string; + params: Record; + ref: string | null; + status: RunActionStatus; + result: Record | null; + error: string | null; + applied_at: string | null; +} + export interface RunRef { run_id: string; agent: string; From 6e8507782cb08536bf48448bef024643b3796f44 Mon Sep 17 00:00:00 2001 From: Suhaib Mujahid Date: Thu, 9 Jul 2026 16:20:35 -0400 Subject: [PATCH 08/13] hackbot: install phabricator extra in libs CI; stamp applied_at only on success The libs test job installed hackbot-runtime with only the claude-sdk extra, so mozphab was missing and build_phabricator_diff's real-diff test failed; add the phabricator extra so it runs. Also only set RunAction.applied_at when a handler actually succeeds, so a failed action isn't mistaken for an applied one. --- .taskcluster.yml | 2 +- services/hackbot-api/app/actions_applier.py | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/.taskcluster.yml b/.taskcluster.yml index 8ccb224aad..4d9f767a54 100644 --- a/.taskcluster.yml +++ b/.taskcluster.yml @@ -148,7 +148,7 @@ tasks: cd libs/agent-tools && uv run --locked --with pytest==9.1.0 --with pytest-cov==7.1.0 --with pytest-asyncio==1.4.0 --extra bugzilla --extra firefox --extra claude-sdk pytest --cov=agent_tools --cov-append tests/ && cd ../hackbot-runtime && - uv run --locked --with pytest==9.1.0 --with pytest-cov==7.1.0 --with pytest-asyncio==1.4.0 --extra claude-sdk pytest --cov=hackbot_runtime --cov-append tests/ && + uv run --locked --with pytest==9.1.0 --with pytest-cov==7.1.0 --with pytest-asyncio==1.4.0 --extra claude-sdk --extra phabricator pytest --cov=hackbot_runtime --cov-append tests/ && cd ../.. && bash <(curl -s https://codecov.io/bash)" metadata: diff --git a/services/hackbot-api/app/actions_applier.py b/services/hackbot-api/app/actions_applier.py index b64b4a81d7..17d85c1853 100644 --- a/services/hackbot-api/app/actions_applier.py +++ b/services/hackbot-api/app/actions_applier.py @@ -141,7 +141,10 @@ async def _apply_pending_rows( row.status = outcome.status row.result = outcome.result row.error = outcome.error - row.applied_at = datetime.now(timezone.utc) + # Only stamp applied_at on a real success, so a failed row isn't + # mistaken for one that was applied. + if outcome.status == "applied": + row.applied_at = datetime.now(timezone.utc) await db.commit() if row.status == "applied" and row.ref and row.result is not None: From 997afa7de712e69cfdcf0599b25ae5b9e52d3243 Mon Sep 17 00:00:00 2001 From: Suhaib Mujahid Date: Thu, 9 Jul 2026 16:33:04 -0400 Subject: [PATCH 09/13] hackbot-runtime: give moz-phab an ambient git identity when building a diff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit moz-phab's git client reads user.email from the ambient git config (not the target repo's local config) and refuses to run without it, so build_phabricator_diff failed in environments without a global identity — CI, and agent containers in production. Point GIT_CONFIG_GLOBAL at a throwaway config carrying a hackbot identity for the duration of the moz-phab call. --- .../hackbot_runtime/changes.py | 49 ++++++++++++++++--- 1 file changed, 41 insertions(+), 8 deletions(-) diff --git a/libs/hackbot-runtime/hackbot_runtime/changes.py b/libs/hackbot-runtime/hackbot_runtime/changes.py index 758c5fc868..620037aeb4 100644 --- a/libs/hackbot-runtime/hackbot_runtime/changes.py +++ b/libs/hackbot-runtime/hackbot_runtime/changes.py @@ -13,8 +13,12 @@ from __future__ import annotations +import contextlib import logging +import os import subprocess +import tempfile +from collections.abc import Iterator from pathlib import Path from typing import NamedTuple @@ -147,6 +151,34 @@ def _synthetic_commit(repo: Path, base: str) -> str: ).strip() +@contextlib.contextmanager +def _ambient_git_identity() -> Iterator[None]: + """Give moz-phab an ambient git identity for its `git config --list` check. + + moz-phab's git client reads ``user.email`` from the *ambient* git config + (not the target repo's local config) and refuses to run without it. Agent + containers and CI often have no global identity, so point + ``GIT_CONFIG_GLOBAL`` at a throwaway config carrying a hackbot identity for + the duration of the call (moz-phab copies ``os.environ`` when it builds its + git client). The identity is cosmetic — only the diff is used, never a + commit moz-phab would author. + """ + with tempfile.NamedTemporaryFile("w", suffix=".gitconfig") as gc: + gc.write(f"[user]\n\temail = {_WIP_EMAIL}\n\tname = {_WIP_NAME}\n") + gc.flush() + overrides = {"GIT_CONFIG_GLOBAL": gc.name, "GIT_CONFIG_SYSTEM": os.devnull} + previous = {k: os.environ.get(k) for k in overrides} + os.environ.update(overrides) + try: + yield + finally: + for key, prev in previous.items(): + if prev is None: + os.environ.pop(key, None) + else: + os.environ[key] = prev + + def build_phabricator_diff(repo: Path, base: str, repo_url: str) -> dict | None: """Build the payload for Phabricator's ``differential.creatediff`` API. @@ -178,14 +210,15 @@ def build_phabricator_diff(repo: Path, base: str, repo_url: str) -> dict | None: try: node = _synthetic_commit(repo, base) - mozphab_repo = Git(str(repo)) - # `set_args` needs a fully-populated argparse.Namespace matching what - # moz-phab's own CLI would build (several unrelated code paths read - # attributes off it) — going through its real parser instead of - # hand-listing the handful of attributes get_diff() happens to touch - # today, which would silently bit-rot on a moz-phab upgrade. - mozphab_repo.set_args(parse_args(["submit", "--yes"])) - diff = mozphab_repo.get_diff(Commit(node=node)) + with _ambient_git_identity(): + mozphab_repo = Git(str(repo)) + # `set_args` needs a fully-populated argparse.Namespace matching what + # moz-phab's own CLI would build (several unrelated code paths read + # attributes off it) — going through its real parser instead of + # hand-listing the handful of attributes get_diff() happens to touch + # today, which would silently bit-rot on a moz-phab upgrade. + mozphab_repo.set_args(parse_args(["submit", "--yes"])) + diff = mozphab_repo.get_diff(Commit(node=node)) except Exception: log.warning("Could not build Phabricator diff for %s", repo, exc_info=True) return None From 95b0ace6511ca1a1c3a94a12d9a08f4195f4d183 Mon Sep 17 00:00:00 2001 From: Suhaib Mujahid Date: Thu, 9 Jul 2026 17:52:53 -0400 Subject: [PATCH 10/13] hackbot-ui: allow retrying failed actions from the run detail view The apply endpoint already re-attempts any action that isn't already applied (it skips only applied rows), so failed actions were retryable server-side but the button only appeared when actions were still pending. Show the button whenever there are pending or failed actions and label it to match (apply, retry, or both), and add a unit test locking the retry-failed / skip-applied contract of _apply_pending_rows. --- .../hackbot-api/tests/test_actions_applier.py | 51 +++++++++++++++++++ services/hackbot-ui/components/RunDetail.tsx | 17 ++++++- 2 files changed, 66 insertions(+), 2 deletions(-) diff --git a/services/hackbot-api/tests/test_actions_applier.py b/services/hackbot-api/tests/test_actions_applier.py index 5101165ffd..850c77be78 100644 --- a/services/hackbot-api/tests/test_actions_applier.py +++ b/services/hackbot-api/tests/test_actions_applier.py @@ -144,3 +144,54 @@ async def test_apply_all_pending_always_applies(monkeypatch): calls = _patch_applier(monkeypatch, auto=False) await apply_all_pending(_FakeDB(), _FakeRun(status=RunStatus.succeeded.value)) assert calls == {"ensured": True, "applied": True} + + +# --- retry semantics: _apply_pending_rows re-attempts failed rows ------ # + + +class _RecordingHandler: + def __init__(self, outcome): + self.outcome = outcome + self.calls = [] + + async def apply(self, params, ctx): + self.calls.append(params) + return self.outcome + + +def _row(idx, status, **kw): + return SimpleNamespace( + idx=idx, + type="bugzilla.add_comment", + params={}, + ref=None, + status=status, + result=kw.get("result"), + error=kw.get("error"), + applied_at=kw.get("applied_at"), + ) + + +async def test_apply_pending_rows_retries_failed_and_skips_applied(monkeypatch): + # A manual re-apply retries a previously-failed action (this is what the + # UI's retry button relies on) while leaving already-applied rows alone. + handler = _RecordingHandler( + SimpleNamespace(status="applied", result={"ok": 1}, error=None) + ) + monkeypatch.setattr(actions_applier, "get_handler", lambda t: handler) + + applied = _row(0, "applied", result={"pre": 1}, applied_at="then") + failed = _row(1, "failed", error="boom") + pending = _row(2, "pending") + rows = [(applied, []), (failed, []), (pending, [])] + + await actions_applier._apply_pending_rows( + _FakeDB(), _FakeRun(status=RunStatus.succeeded.value), rows + ) + + # The already-applied row is untouched; its handler never runs. + assert applied.status == "applied" and applied.result == {"pre": 1} + # The failed and pending rows are both (re)applied, clearing the stale error. + assert len(handler.calls) == 2 + assert failed.status == "applied" and failed.error is None + assert pending.status == "applied" diff --git a/services/hackbot-ui/components/RunDetail.tsx b/services/hackbot-ui/components/RunDetail.tsx index 42ca4486c8..d406889b5b 100644 --- a/services/hackbot-ui/components/RunDetail.tsx +++ b/services/hackbot-ui/components/RunDetail.tsx @@ -126,6 +126,19 @@ export function RunDetail({ runId }: { runId: string }) { const findings = run.summary?.findings ?? {}; const hasFindings = Object.keys(findings).length > 0; + // Both pending and failed actions are (re)applied by the apply endpoint — it + // skips only already-applied ones — so one button covers applying and retry. + const pendingActions = + actions?.filter((a) => a.status === "pending").length ?? 0; + const failedActions = + actions?.filter((a) => a.status === "failed").length ?? 0; + const applyLabel = + pendingActions && failedActions + ? "Apply pending & retry failed actions" + : failedActions + ? "Retry failed actions" + : "Apply pending actions"; + return ( <>
@@ -195,9 +208,9 @@ export function RunDetail({ runId }: { runId: string }) { ))} - {actions.some((a) => a.status === "pending") && ( + {pendingActions + failedActions > 0 && ( )}
From f6a0edef2a37c0baee1e3a5f00b2fb00e2aa99b5 Mon Sep 17 00:00:00 2001 From: Suhaib Mujahid Date: Thu, 9 Jul 2026 19:50:16 -0400 Subject: [PATCH 11/13] hackbot: coalesce same-bug Bugzilla mutations into one PUT Applying a run's Bugzilla actions one at a time sends a separate PUT /bug/{id} per action, so an agent that both edits fields and adds a comment on one bug triggers multiple bugmail notifications and shows as several separate history entries. Bugzilla applies field changes plus a comment in a single PUT as one transaction (one bugmail, one entry). The applier now plans same-bug field changes to ride with the closest comment in one combined PUT; any other comments on that bug still apply separately (Bugzilla takes one comment object per request). Grouping lives in the runtime lib (plan_coalesced_groups / merge_resolved); UpdateBugHandler accepts an optional comment. The combined unit applies at the group's max idx so backward {{actions..}} placeholders still resolve, and only non-applied rows are grouped so retries never re-send an applied action. --- .../actions/handlers/__init__.py | 6 + .../actions/handlers/bugzilla_handler.py | 84 ++++++- .../tests/test_bugzilla_handler.py | 96 ++++++++ services/hackbot-api/app/actions_applier.py | 128 ++++++---- .../hackbot-api/tests/test_actions_applier.py | 226 +++++++++++++++++- 5 files changed, 492 insertions(+), 48 deletions(-) diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/handlers/__init__.py b/libs/hackbot-runtime/hackbot_runtime/actions/handlers/__init__.py index 702928eec7..c30fb5cc0a 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/handlers/__init__.py +++ b/libs/hackbot-runtime/hackbot_runtime/actions/handlers/__init__.py @@ -12,6 +12,10 @@ ActionResult, ApplyContext, ) +from hackbot_runtime.actions.handlers.bugzilla_handler import ( + merge_resolved, + plan_coalesced_groups, +) from hackbot_runtime.actions.handlers.registry import HANDLERS, get_handler __all__ = [ @@ -20,4 +24,6 @@ "ApplyContext", "HANDLERS", "get_handler", + "merge_resolved", + "plan_coalesced_groups", ] diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/handlers/bugzilla_handler.py b/libs/hackbot-runtime/hackbot_runtime/actions/handlers/bugzilla_handler.py index 678b493838..c78dd23942 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/handlers/bugzilla_handler.py +++ b/libs/hackbot-runtime/hackbot_runtime/actions/handlers/bugzilla_handler.py @@ -57,8 +57,15 @@ def _request(method: str, path: str, json_body: dict[str, Any]) -> dict[str, Any class UpdateBugHandler: async def apply(self, params: dict[str, Any], ctx: ApplyContext) -> ActionResult: bug_id = params["bug_id"] + # Field changes and a comment both go in one PUT /bug/{id} — Bugzilla + # applies them as a single transaction (one bugmail, one history entry). + # Copy `changes` so we never mutate the caller's dict when folding in a + # comment (the coalescer hands us params built from other rows). + body = dict(params.get("changes", {})) + if params.get("comment"): + body["comment"] = params["comment"] try: - _request("PUT", f"bug/{bug_id}", params["changes"]) + _request("PUT", f"bug/{bug_id}", body) except Exception as exc: log.exception("Failed to update bug %s", bug_id) return ActionResult.failed(str(exc)) @@ -134,3 +141,78 @@ async def apply(self, params: dict[str, Any], ctx: ApplyContext) -> ActionResult return ActionResult.ok( {"bug_id": bug_id, "url": _bug_url(bug_id) if bug_id else None} ) + + +_MERGEABLE_TYPES = ("bugzilla.update_bug", "bugzilla.add_comment") + + +def _closest_comment(update_idxs: list[int], comment_idxs: list[int]) -> int: + """Pick the comment nearest (in idx order) to the field updates. + + Distance is to the closest update; ties break toward the earliest comment. + """ + return min( + comment_idxs, + key=lambda c: (min(abs(c - u) for u in update_idxs), c), + ) + + +def plan_coalesced_groups( + actions: list[tuple[str, dict[str, Any]]], +) -> list[list[int]]: + """Return index groups of same-bug actions to apply as one ``PUT /bug/{id}``. + + ``actions`` is every pending ``(action_type, params)`` in idx order. Field + changes for a bug are merged together and ride with the single comment + *closest* to them (Bugzilla takes one ``comment`` object per PUT); any other + comments on the bug are left to apply on their own. So each returned group + is ``[update idxs..., one comment idx]`` (update + comment) or + ``[update idxs...]`` (changes-only). Only groups of >= 2 indices are + returned — a lone action needs no coalescing and applies as before. + Comment-only bugs return nothing: distinct comments stay distinct PUTs. + """ + updates: dict[Any, list[int]] = {} + comments: dict[Any, list[int]] = {} + for idx, (action_type, params) in enumerate(actions): + if action_type not in _MERGEABLE_TYPES: + continue + bug_id = params.get("bug_id") + if bug_id is None: + continue + bucket = updates if action_type == "bugzilla.update_bug" else comments + bucket.setdefault(bug_id, []).append(idx) + + groups: list[list[int]] = [] + for bug_id, update_idxs in updates.items(): + group = list(update_idxs) + bug_comments = comments.get(bug_id) + if bug_comments: + group.append(_closest_comment(update_idxs, bug_comments)) + if len(group) >= 2: + groups.append(sorted(group)) + return groups + + +def merge_resolved(entries: list[tuple[str, dict[str, Any]]]) -> dict[str, Any]: + """Build combined :class:`UpdateBugHandler` params from a planned group. + + ``entries`` are placeholder-resolved ``(action_type, params)`` tuples for a + group from :func:`plan_coalesced_groups`, in idx order: all the bug's field + changes merged (later writes win) plus at most one comment. + """ + bug_id = entries[0][1]["bug_id"] + changes: dict[str, Any] = {} + comment: dict[str, Any] | None = None + for action_type, params in entries: + if action_type == "bugzilla.update_bug": + changes.update(params.get("changes", {})) + elif action_type == "bugzilla.add_comment": + comment = { + "body": params["text"], + "is_private": bool(params.get("is_private", False)), + } + + combined: dict[str, Any] = {"bug_id": bug_id, "changes": changes} + if comment is not None: + combined["comment"] = comment + return combined diff --git a/libs/hackbot-runtime/tests/test_bugzilla_handler.py b/libs/hackbot-runtime/tests/test_bugzilla_handler.py index 08560c5c96..611f338e80 100644 --- a/libs/hackbot-runtime/tests/test_bugzilla_handler.py +++ b/libs/hackbot-runtime/tests/test_bugzilla_handler.py @@ -100,3 +100,99 @@ async def test_create_bug_handler_success(monkeypatch): ) assert result.status == "applied" assert result.result["bug_id"] == 42 + + +async def test_update_bug_handler_merges_changes_and_comment(monkeypatch): + calls = [] + monkeypatch.setattr( + bugzilla_handler, "_request", lambda m, p, b: calls.append((m, p, b)) + ) + changes = {"status": "RESOLVED"} + await bugzilla_handler.UpdateBugHandler().apply( + { + "bug_id": 7, + "changes": changes, + "comment": {"body": "done", "is_private": False}, + }, + _ctx(), + ) + assert calls == [ + ( + "PUT", + "bug/7", + {"status": "RESOLVED", "comment": {"body": "done", "is_private": False}}, + ) + ] + # The caller's `changes` dict must not be mutated by folding in the comment. + assert changes == {"status": "RESOLVED"} + + +async def test_update_bug_handler_comment_only(monkeypatch): + calls = [] + monkeypatch.setattr( + bugzilla_handler, "_request", lambda m, p, b: calls.append((m, p, b)) + ) + await bugzilla_handler.UpdateBugHandler().apply( + {"bug_id": 7, "changes": {}, "comment": {"body": "hi", "is_private": True}}, + _ctx(), + ) + assert calls == [("PUT", "bug/7", {"comment": {"body": "hi", "is_private": True}})] + + +def test_plan_coalesced_groups_update_plus_comment(): + actions = [ + ("bugzilla.update_bug", {"bug_id": 5, "changes": {"status": "RESOLVED"}}), + ("bugzilla.add_comment", {"bug_id": 5, "text": "done"}), + ] + assert bugzilla_handler.plan_coalesced_groups(actions) == [[0, 1]] + + +def test_plan_coalesced_groups_closest_comment_wins(): + # update@0 is nearer comment@1 than comment@3; comment@3 stays standalone. + actions = [ + ("bugzilla.update_bug", {"bug_id": 5, "changes": {}}), + ("bugzilla.add_comment", {"bug_id": 5, "text": "near"}), + ("bugzilla.add_comment", {"bug_id": 9, "text": "other bug"}), + ("bugzilla.add_comment", {"bug_id": 5, "text": "far"}), + ] + assert bugzilla_handler.plan_coalesced_groups(actions) == [[0, 1]] + + +def test_plan_coalesced_groups_multiple_updates_merge_without_comment(): + actions = [ + ("bugzilla.update_bug", {"bug_id": 5, "changes": {"a": 1}}), + ("bugzilla.update_bug", {"bug_id": 5, "changes": {"b": 2}}), + ] + assert bugzilla_handler.plan_coalesced_groups(actions) == [[0, 1]] + + +def test_plan_coalesced_groups_ignores_unmergeable_and_lonely(): + actions = [ + ("bugzilla.add_comment", {"bug_id": 5, "text": "lone comment"}), + ("bugzilla.update_bug", {"bug_id": 6, "changes": {}}), # lone update + ("bugzilla.add_attachment", {"bug_id": 6}), # different endpoint + ("bugzilla.create_bug", {"summary": "x"}), # POST, no bug_id + ("bugzilla.update_bug", {"changes": {}}), # missing bug_id + ] + assert bugzilla_handler.plan_coalesced_groups(actions) == [] + + +def test_merge_resolved_combines_changes_and_single_comment(): + entries = [ + ("bugzilla.update_bug", {"bug_id": 5, "changes": {"a": 1}}), + ("bugzilla.update_bug", {"bug_id": 5, "changes": {"a": 2, "b": 3}}), + ("bugzilla.add_comment", {"bug_id": 5, "text": "hi", "is_private": True}), + ] + assert bugzilla_handler.merge_resolved(entries) == { + "bug_id": 5, + "changes": {"a": 2, "b": 3}, # later update wins on conflict + "comment": {"body": "hi", "is_private": True}, + } + + +def test_merge_resolved_changes_only(): + entries = [("bugzilla.update_bug", {"bug_id": 5, "changes": {"a": 1}})] + assert bugzilla_handler.merge_resolved(entries) == { + "bug_id": 5, + "changes": {"a": 1}, + } diff --git a/services/hackbot-api/app/actions_applier.py b/services/hackbot-api/app/actions_applier.py index 17d85c1853..10d668a139 100644 --- a/services/hackbot-api/app/actions_applier.py +++ b/services/hackbot-api/app/actions_applier.py @@ -17,7 +17,13 @@ from datetime import datetime, timezone from typing import Any -from hackbot_runtime.actions.handlers import ApplyContext, get_handler +from hackbot_runtime.actions.handlers import ( + ActionResult, + ApplyContext, + get_handler, + merge_resolved, + plan_coalesced_groups, +) from sqlalchemy import select from sqlalchemy.ext.asyncio import AsyncSession @@ -88,11 +94,46 @@ async def ensure_action_rows( return rows +async def _dispatch( + run: Run, action_type: str, params: dict, attachments: list[dict] +) -> ActionResult: + """Run one handler call, converting failures into a failed `ActionResult`. + + A missing handler or a raised exception becomes a failed result so callers + can stamp the affected row(s) uniformly. + """ + handler = get_handler(action_type) + if handler is None: + return ActionResult.failed( + f"No handler registered for action type '{action_type}'" + ) + + ctx = ApplyContext( + run_id=str(run.run_id), + download_artifact=lambda key, run_id=str(run.run_id): ( + gcs.download_artifact_bytes(run_id, key) + ), + attachments=attachments, + ) + try: + return await handler.apply(params, ctx) + except Exception as exc: + log.exception( + "Handler for %s raised while applying run %s", action_type, run.run_id + ) + return ActionResult.failed(str(exc)) + + async def _apply_pending_rows( db: AsyncSession, run: Run, rows: list[tuple[RunAction, list[dict]]] ) -> None: """Apply every not-yet-`applied` row in `rows`, committing per action. + Same-bug Bugzilla field changes are coalesced with the closest comment into + a single `PUT /bug/{id}` so Bugzilla applies them as one transaction (one + bugmail, one history entry); any other comments on that bug still apply + separately. See `plan_coalesced_groups`/`merge_resolved` in the runtime lib. + Cross-action `{{actions..}}` placeholders resolve against rows that are already `applied` (seeded from prior applies) plus ones applied earlier in this pass, so a later (even manual) apply can still reference an @@ -104,51 +145,58 @@ async def _apply_pending_rows( if row.ref and row.status == "applied" and row.result is not None } - for row, attachments in rows: - if row.status == "applied": - continue - - handler = get_handler(row.type) - if handler is None: - row.status = "failed" - row.error = f"No handler registered for action type '{row.type}'" - await db.commit() - continue - - params = resolve_placeholders(row.params, results_by_ref) - ctx = ApplyContext( - run_id=str(run.run_id), - download_artifact=lambda key, run_id=str(run.run_id): ( - gcs.download_artifact_bytes(run_id, key) - ), - attachments=attachments, - ) + pending = [(row, att) for row, att in rows if row.status != "applied"] - try: - outcome = await handler.apply(params, ctx) - except Exception as exc: - log.exception( - "Handler for %s raised while applying run %s action #%d", - row.type, - run.run_id, - row.idx, + # Plan which pending rows coalesce into one bug PUT (indices into `pending`). + # Drop any group whose rows carry a `ref`: nothing should reference a + # coalesced member's result, and this keeps that invariant if a ref is ever + # added to a bug action. Everything else applies one row at a time as before. + groups = [ + group + for group in plan_coalesced_groups( + [(row.type, row.params) for row, _ in pending] + ) + if all(pending[i][0].ref is None for i in group) + ] + # Rows sit in idx order, so a group's last member is its max idx: apply the + # whole group there, once every earlier (backward) dependency is resolved. + anchor_of = {i: max(group) for group in groups for i in group} + group_at = {max(group): group for group in groups} + + for pos, (row, attachments) in enumerate(pending): + anchor = anchor_of.get(pos) + if anchor is not None and pos != anchor: + continue # non-anchor member: applied together with its anchor + + if anchor is not None: + member_rows = [pending[i][0] for i in group_at[anchor]] + entries = [ + (member.type, resolve_placeholders(member.params, results_by_ref)) + for member in member_rows + ] + outcome = await _dispatch( + run, "bugzilla.update_bug", merge_resolved(entries), [] ) - row.status = "failed" - row.error = str(exc) - await db.commit() - continue - - row.status = outcome.status - row.result = outcome.result - row.error = outcome.error + else: + member_rows = [row] + params = resolve_placeholders(row.params, results_by_ref) + outcome = await _dispatch(run, row.type, params, attachments) + # Only stamp applied_at on a real success, so a failed row isn't # mistaken for one that was applied. - if outcome.status == "applied": - row.applied_at = datetime.now(timezone.utc) + applied_at = datetime.now(timezone.utc) if outcome.status == "applied" else None + for member in member_rows: + member.status = outcome.status + member.result = outcome.result + member.error = outcome.error + if applied_at is not None: + member.applied_at = applied_at await db.commit() - if row.status == "applied" and row.ref and row.result is not None: - results_by_ref[row.ref] = row.result + if outcome.status == "applied" and outcome.result is not None: + for member in member_rows: + if member.ref: + results_by_ref[member.ref] = outcome.result async def on_run_completed(db: AsyncSession, run: Run) -> None: diff --git a/services/hackbot-api/tests/test_actions_applier.py b/services/hackbot-api/tests/test_actions_applier.py index 850c77be78..37e10eef5c 100644 --- a/services/hackbot-api/tests/test_actions_applier.py +++ b/services/hackbot-api/tests/test_actions_applier.py @@ -159,16 +159,26 @@ async def apply(self, params, ctx): return self.outcome -def _row(idx, status, **kw): +def _row( + idx, + status, + *, + action_type="bugzilla.add_comment", + params=None, + ref=None, + result=None, + error=None, + applied_at=None, +): return SimpleNamespace( idx=idx, - type="bugzilla.add_comment", - params={}, - ref=None, + type=action_type, + params=params if params is not None else {}, + ref=ref, status=status, - result=kw.get("result"), - error=kw.get("error"), - applied_at=kw.get("applied_at"), + result=result, + error=error, + applied_at=applied_at, ) @@ -195,3 +205,205 @@ async def test_apply_pending_rows_retries_failed_and_skips_applied(monkeypatch): assert len(handler.calls) == 2 assert failed.status == "applied" and failed.error is None assert pending.status == "applied" + + +# --- coalescing same-bug Bugzilla mutations into one PUT ---------------- # + + +async def test_coalesces_update_and_comment_into_one_put(monkeypatch): + handler = _RecordingHandler( + SimpleNamespace(status="applied", result={"bug_id": 5}, error=None) + ) + monkeypatch.setattr(actions_applier, "get_handler", lambda t: handler) + + update = _row( + 0, + "pending", + action_type="bugzilla.update_bug", + params={"bug_id": 5, "changes": {"status": "RESOLVED"}}, + ) + other = _row( + 1, + "pending", + action_type="bugzilla.add_comment", + params={"bug_id": 99, "text": "different bug"}, + ) + comment = _row( + 2, + "pending", + action_type="bugzilla.add_comment", + params={"bug_id": 5, "text": "done"}, + ) + rows = [(update, []), (other, []), (comment, [])] + + await actions_applier._apply_pending_rows( + _FakeDB(), _FakeRun(status=RunStatus.succeeded.value), rows + ) + + # Two calls: the standalone comment on bug 99, then ONE combined PUT for + # bug 5 (applied at the group's max idx) carrying field change + comment. + assert handler.calls == [ + {"bug_id": 99, "text": "different bug"}, + { + "bug_id": 5, + "changes": {"status": "RESOLVED"}, + "comment": {"body": "done", "is_private": False}, + }, + ] + assert update.status == "applied" and comment.status == "applied" + assert update.result == {"bug_id": 5} and comment.result == {"bug_id": 5} + + +async def test_extra_comments_applied_separately(monkeypatch): + handler = _RecordingHandler( + SimpleNamespace(status="applied", result={}, error=None) + ) + monkeypatch.setattr(actions_applier, "get_handler", lambda t: handler) + + update = _row( + 0, + "pending", + action_type="bugzilla.update_bug", + params={"bug_id": 5, "changes": {"status": "RESOLVED"}}, + ) + near = _row( + 1, + "pending", + action_type="bugzilla.add_comment", + params={"bug_id": 5, "text": "near"}, + ) + far = _row( + 2, + "pending", + action_type="bugzilla.add_comment", + params={"bug_id": 5, "text": "far"}, + ) + rows = [(update, []), (near, []), (far, [])] + + await actions_applier._apply_pending_rows( + _FakeDB(), _FakeRun(status=RunStatus.succeeded.value), rows + ) + + # Field change rides with the closest comment ("near"); "far" is its own PUT. + assert handler.calls == [ + { + "bug_id": 5, + "changes": {"status": "RESOLVED"}, + "comment": {"body": "near", "is_private": False}, + }, + {"bug_id": 5, "text": "far"}, + ] + + +async def test_lone_same_type_actions_on_different_bugs_not_merged(monkeypatch): + handler = _RecordingHandler( + SimpleNamespace(status="applied", result={}, error=None) + ) + monkeypatch.setattr(actions_applier, "get_handler", lambda t: handler) + + u5 = _row( + 0, + "pending", + action_type="bugzilla.update_bug", + params={"bug_id": 5, "changes": {"a": 1}}, + ) + u6 = _row( + 1, + "pending", + action_type="bugzilla.update_bug", + params={"bug_id": 6, "changes": {"b": 2}}, + ) + rows = [(u5, []), (u6, [])] + + await actions_applier._apply_pending_rows( + _FakeDB(), _FakeRun(status=RunStatus.succeeded.value), rows + ) + # Different bugs, one update each -> no coalescing, two raw PUTs. + assert handler.calls == [ + {"bug_id": 5, "changes": {"a": 1}}, + {"bug_id": 6, "changes": {"b": 2}}, + ] + + +async def test_coalesced_group_failure_marks_all_then_retries(monkeypatch): + failing = _RecordingHandler( + SimpleNamespace(status="failed", result=None, error="boom") + ) + monkeypatch.setattr(actions_applier, "get_handler", lambda t: failing) + + update = _row( + 0, + "pending", + action_type="bugzilla.update_bug", + params={"bug_id": 5, "changes": {"a": 1}}, + ) + comment = _row( + 1, + "pending", + action_type="bugzilla.add_comment", + params={"bug_id": 5, "text": "c"}, + ) + done = _row( + 2, + "applied", + action_type="bugzilla.add_comment", + params={"bug_id": 5, "text": "already"}, + result={"x": 1}, + applied_at="then", + ) + rows = [(update, []), (comment, []), (done, [])] + run = _FakeRun(status=RunStatus.succeeded.value) + + await actions_applier._apply_pending_rows(_FakeDB(), run, rows) + # One combined call; both members failed; the already-applied row untouched. + assert len(failing.calls) == 1 + assert update.status == "failed" and comment.status == "failed" + assert done.status == "applied" and done.result == {"x": 1} + + # Retry: only the still-failed members re-group; the applied one is skipped. + ok = _RecordingHandler( + SimpleNamespace(status="applied", result={"bug_id": 5}, error=None) + ) + monkeypatch.setattr(actions_applier, "get_handler", lambda t: ok) + await actions_applier._apply_pending_rows(_FakeDB(), run, rows) + assert len(ok.calls) == 1 + assert update.status == "applied" and comment.status == "applied" + + +async def test_backward_placeholder_resolves_in_coalesced_comment(monkeypatch): + handler = _RecordingHandler( + SimpleNamespace(status="applied", result={"url": "http://x/D1"}, error=None) + ) + monkeypatch.setattr(actions_applier, "get_handler", lambda t: handler) + + patch = _row( + 0, "pending", action_type="phabricator.submit_patch", params={}, ref="patch" + ) + update = _row( + 1, + "pending", + action_type="bugzilla.update_bug", + params={"bug_id": 5, "changes": {"a": 1}}, + ) + comment = _row( + 2, + "pending", + action_type="bugzilla.add_comment", + params={"bug_id": 5, "text": "see {{actions.patch.url}}"}, + ) + rows = [(patch, []), (update, []), (comment, [])] + + await actions_applier._apply_pending_rows( + _FakeDB(), _FakeRun(status=RunStatus.succeeded.value), rows + ) + + # The patch applies first (its own idx), seeding results_by_ref; the + # coalesced comment then resolves {{actions.patch.url}} at the group anchor. + assert handler.calls == [ + {}, + { + "bug_id": 5, + "changes": {"a": 1}, + "comment": {"body": "see http://x/D1", "is_private": False}, + }, + ] From aff59090220f288545cc35cfb92e07f94b29e132 Mon Sep 17 00:00:00 2001 From: Suhaib Mujahid Date: Thu, 9 Jul 2026 20:29:19 -0400 Subject: [PATCH 12/13] hackbot-runtime: read the Bugzilla API key from BUGZILLA_API_KEY Rename the env var the apply-side Bugzilla handlers authenticate with from BUGZILLA_TOKEN to BUGZILLA_API_KEY, matching that it holds a Bugzilla API key. --- .../hackbot_runtime/actions/handlers/bugzilla_handler.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/handlers/bugzilla_handler.py b/libs/hackbot-runtime/hackbot_runtime/actions/handlers/bugzilla_handler.py index c78dd23942..620364db80 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/handlers/bugzilla_handler.py +++ b/libs/hackbot-runtime/hackbot_runtime/actions/handlers/bugzilla_handler.py @@ -36,10 +36,10 @@ def _bug_url(bug_id: int) -> str: def _headers() -> dict[str, str]: - token = os.environ.get("BUGZILLA_TOKEN", "") - if not token: - raise RuntimeError("BUGZILLA_TOKEN is not configured") - return {"X-Bugzilla-API-Key": token, "Content-Type": "application/json"} + api_key = os.environ.get("BUGZILLA_API_KEY", "") + if not api_key: + raise RuntimeError("BUGZILLA_API_KEY is not configured") + return {"X-Bugzilla-API-Key": api_key, "Content-Type": "application/json"} def _request(method: str, path: str, json_body: dict[str, Any]) -> dict[str, Any]: From ac64bf828eb91898c566f4e9c5fd77fde3fded8e Mon Sep 17 00:00:00 2001 From: Suhaib Mujahid Date: Fri, 10 Jul 2026 00:05:19 -0400 Subject: [PATCH 13/13] bug-fix agent: submit fix patches as Phabricator revisions --- .../hackbot_agents/bug_fix/prompts/system.md | 5 ++-- .../hackbot_runtime/actions/phabricator.py | 24 ++++++++++++------- 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/agents/bug-fix/hackbot_agents/bug_fix/prompts/system.md b/agents/bug-fix/hackbot_agents/bug_fix/prompts/system.md index 8d3b9536d8..bb93876015 100644 --- a/agents/bug-fix/hackbot_agents/bug_fix/prompts/system.md +++ b/agents/bug-fix/hackbot_agents/bug_fix/prompts/system.md @@ -42,8 +42,7 @@ Follow these rules: to avoid spot fixes where a more general fix on a higher level or earlier is more appropriate. - Avoid adding too much defense in depth, especially in performance critical paths. While defense in depth is generally not a bad thing, unnecessary/redundant checks cost valuable performance. -- When creating patches, always modify the files to be patched, then use `git diff`, **never** write - a patch file directly as it often leads to corrupt patches. +- When you have a fix you are confident in, submit it with the `phabricator_submit_patch` action. - If a bug has been closed or a developer has already added a fix patch (even if you cannot download it), then don't create a fix and move on. - If you detect that a bug has already been fixed by another bug, don't create another fix patch @@ -66,7 +65,7 @@ When you spawn an investigator via the Task tool, write a complete, self-contain # Recording actions -The `actions` MCP tools (`bugzilla_update_bug`, `bugzilla_add_comment`, `bugzilla_add_attachment`, `bugzilla_create_bug`) do **not** mutate Bugzilla directly. They record an intended action into the run's `summary.json` for a human reviewer (or a downstream apply step) to enact. Treat each recorded action as a final, irrevocable proposal — once recorded it appears in the run output verbatim. +The `actions` MCP tools (`bugzilla_update_bug`, `bugzilla_add_comment`, `phabricator_submit_patch`) do **not** mutate Bugzilla or Phabricator directly. They record an intended action into the run's `summary.json` for a human reviewer (or a downstream apply step) to enact. Treat each recorded action as a final, irrevocable proposal — once recorded it appears in the run output verbatim. Before calling any action tool, state in your response: diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/phabricator.py b/libs/hackbot-runtime/hackbot_runtime/actions/phabricator.py index d3106f9590..f096ee23c2 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/phabricator.py +++ b/libs/hackbot-runtime/hackbot_runtime/actions/phabricator.py @@ -67,17 +67,23 @@ async def submit_patch( ), ] = None, ) -> str: - """Record an intended Phabricator patch submission. + """Submit your fix for review as a Phabricator revision. - Submits whatever the run's final source-tree changes turn out to be — the - same diff the runtime already collects into ``changes/changes.patch`` once - the agent finishes, not a separately-supplied file. There's no local patch - path to give here because that diff isn't final until after this call - returns (it's computed once from the checkout's end state). + This is how you deliver a code fix. Do not attach the patch to a bug: a + Phabricator revision is the correct destination for a fix, not a bug + attachment. - Recorded into the run summary for human review — does not submit to - Phabricator. Set revision_id to update an existing revision with a new - diff, or omit it (and provide a title) to create a new one. + You do not supply a patch file. Your final code changes in the working + directory are submitted as the revision's diff, so make and verify all your + edits first, then call this once you are done. Calling it records the + submission as a proposed action for review; it is not sent to Phabricator + during the run. + + To create a new revision, pass a `title` (and ideally a `summary`). To add a + new diff to an existing revision instead, pass that revision's `revision_id`. + Set `ref` if you want to reference the new revision's URL from another action + in the same run, written as `{{actions..url}}` (for example, inside a + bug comment). """ if revision_id is None and not title: raise ToolError("title is required when creating a new revision")