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/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/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/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/__init__.py b/libs/hackbot-runtime/hackbot_runtime/actions/handlers/__init__.py new file mode 100644 index 0000000000..c30fb5cc0a --- /dev/null +++ b/libs/hackbot-runtime/hackbot_runtime/actions/handlers/__init__.py @@ -0,0 +1,29 @@ +"""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.bugzilla_handler import ( + merge_resolved, + plan_coalesced_groups, +) +from hackbot_runtime.actions.handlers.registry import HANDLERS, get_handler + +__all__ = [ + "ActionHandler", + "ActionResult", + "ApplyContext", + "HANDLERS", + "get_handler", + "merge_resolved", + "plan_coalesced_groups", +] 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..620364db80 --- /dev/null +++ b/libs/hackbot-runtime/hackbot_runtime/actions/handlers/bugzilla_handler.py @@ -0,0 +1,218 @@ +"""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]: + 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]: + 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"] + # 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}", body) + 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} + ) + + +_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/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 new file mode 100644 index 0000000000..d9835e9cf3 --- /dev/null +++ b/libs/hackbot-runtime/hackbot_runtime/actions/handlers/registry.py @@ -0,0 +1,23 @@ +from hackbot_runtime.actions.handlers.base import ActionHandler +from hackbot_runtime.actions.handlers.bugzilla_handler import ( + AddAttachmentHandler, + AddCommentHandler, + 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 +# 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(), + "phabricator.submit_patch": SubmitPatchHandler(), +} + + +def get_handler(action_type: str) -> ActionHandler | None: + return HANDLERS.get(action_type) 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..f096ee23c2 --- /dev/null +++ b/libs/hackbot-runtime/hackbot_runtime/actions/phabricator.py @@ -0,0 +1,107 @@ +"""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: + """Submit your fix for review as a Phabricator revision. + + 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. + + 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") + + 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/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/hackbot_runtime/changes.py b/libs/hackbot-runtime/hackbot_runtime/changes.py index cc201e02c0..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 @@ -117,13 +121,138 @@ 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() + + +@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. + + 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) + 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 + + 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 +260,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_bugzilla_handler.py b/libs/hackbot-runtime/tests/test_bugzilla_handler.py new file mode 100644 index 0000000000..611f338e80 --- /dev/null +++ b/libs/hackbot-runtime/tests/test_bugzilla_handler.py @@ -0,0 +1,198 @@ +"""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 + + +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/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/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] 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/actions_applier.py b/services/hackbot-api/app/actions_applier.py new file mode 100644 index 0000000000..10d668a139 --- /dev/null +++ b/services/hackbot-api/app/actions_applier.py @@ -0,0 +1,241 @@ +"""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 + +import logging +import re +from datetime import datetime, timezone +from typing import Any + +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 + +from app import gcs +from app.agents import AGENT_REGISTRY +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_action_rows( + db: AsyncSession, run: Run +) -> list[tuple[RunAction, list[dict]]]: + """Upsert one `RunAction` per recorded action (does not apply them). + + Returns each row paired with its (not persisted) attachments list from + 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", []) + + 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 _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 + earlier action's result. + """ + 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 + } + + pending = [(row, att) for row, att in rows if row.status != "applied"] + + # 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), [] + ) + 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. + 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 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: + """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/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/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 + ) 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..d46da7ab6d --- /dev/null +++ b/services/hackbot-api/app/routers/events.py @@ -0,0 +1,137 @@ +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 on_run_completed +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 completion-log push subscription feeding 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 _execution_name_from_completion_log(entry: dict) -> str | None: + """Cloud Run Jobs adapter: pull the Execution name from a completion LogEntry. + + 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. + + Only this half is Cloud-Run-specific: a future platform adds a sibling + parser feeding the same platform-neutral finalize path below. + """ + 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) +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 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 = _execution_name_from_completion_log(event) + if not execution_name: + log.warning("agent-run-finished event missing execution name: %s", event) + return + + run = await _find_run_for_execution(db, execution_name) + 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 `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, + 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 on_run_completed(db, run) diff --git a/services/hackbot-api/app/routers/runs.py b/services/hackbot-api/app/routers/runs.py index ff55d466e8..083f5b0f7f 100644 --- a/services/hackbot-api/app/routers/runs.py +++ b/services/hackbot-api/app/routers/runs.py @@ -1,21 +1,23 @@ 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.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 ( - TERMINAL_STATUSES, AgentDescriptor, + RunActionDoc, RunDoc, RunRef, RunStatus, @@ -115,14 +117,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 +142,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 +153,49 @@ async def get_artifact_download_url( return {"url": url} -async def _reconcile(db: AsyncSession, run: Run) -> None: +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. + + 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 +223,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..057dc88c10 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,22 @@ 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 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): 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..37e10eef5c --- /dev/null +++ b/services/hackbot-api/tests/test_actions_applier.py @@ -0,0 +1,409 @@ +"""Tests for the action applier. + +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 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 + + +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" + + +# --- 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: + def __init__(self): + self.commits = 0 + + async def commit(self): + self.commits += 1 + + async def execute(self, *a, **k): + raise AssertionError( + "ensure_action_rows should be monkeypatched in these tests" + ) + + +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): + 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} + + +# --- 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, + *, + action_type="bugzilla.add_comment", + params=None, + ref=None, + result=None, + error=None, + applied_at=None, +): + return SimpleNamespace( + idx=idx, + type=action_type, + params=params if params is not None else {}, + ref=ref, + status=status, + result=result, + error=error, + applied_at=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" + + +# --- 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}, + }, + ] diff --git a/services/hackbot-api/tests/test_events.py b/services/hackbot-api/tests/test_events.py new file mode 100644 index 0000000000..214ca8e912 --- /dev/null +++ b/services/hackbot-api/tests/test_events.py @@ -0,0 +1,75 @@ +"""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 ( + _decode_pubsub_push_body, + _execution_name_from_completion_log, +) + + +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 _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", + "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 _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_execution_name_missing(): + assert _execution_name_from_completion_log({"protoPayload": {}}) is None + assert _execution_name_from_completion_log({}) 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/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 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..d406889b5b 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}
; } @@ -89,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 ( <>
@@ -145,6 +195,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}} +
    • + ))} +
+ {pendingActions + failedActions > 0 && ( + + )} +
+ )} +

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; diff --git a/uv.lock b/uv.lock index 0de875449c..b7a81cf85a 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" @@ -2033,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" @@ -2439,7 +2505,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 +2516,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" }, @@ -2549,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" }, @@ -2571,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" }, @@ -2600,6 +2672,9 @@ claude-sdk = [ { name = "agent-tools", extra = ["claude-sdk"] }, { name = "claude-agent-sdk" }, ] +phabricator = [ + { name = "mozphab" }, +] [package.metadata] requires-dist = [ @@ -2607,10 +2682,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 +4261,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 +6506,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"