From a534e59cfb498a55452d69712d3e17a215f03642 Mon Sep 17 00:00:00 2001 From: Orla Date: Mon, 9 Nov 2026 16:17:57 -0500 Subject: [PATCH 1/2] Add a github.submit_patch action that opens a pull request The GitHub counterpart of phabricator.submit_patch. The agent records the submission during the run; when it is applied, hackbot-api pushes a branch carrying the run's commits to the source repository and opens a draft pull request against its default branch. hackbot-api has no git checkout to apply the run's patch in, so publish_changes also captures each commit (message, author and changed files) as changes/github_commits.json, from the published base. The handler recreates those commits through GitHub's Git Data API on a hackbot/ branch, using the GitHub App with a token narrowed to that repository. Retrying reuses the branch and any pull request already open from it. github.submit_patch counts as a patch action in hackbot-api, so a run that leaves changes without one is still reported, and the run page previews the pull request's title and body. Fixes #7057 --- docs/hackbot/actions.md | 10 + .../hackbot_runtime/actions/__init__.py | 2 + .../hackbot_runtime/actions/claude_sdk.py | 2 + .../hackbot_runtime/actions/github.py | 70 ++++ .../hackbot_runtime/changes.py | 84 +++++ .../hackbot_runtime/context.py | 27 +- libs/hackbot-runtime/tests/test_changes.py | 87 +++++ libs/hackbot-runtime/tests/test_context.py | 77 ++++ .../tests/test_github_actions.py | 39 ++ .../app/action_handlers/github_handler.py | 254 +++++++++++++ .../app/action_handlers/registry.py | 10 +- .../hackbot-api/tests/test_github_handler.py | 351 ++++++++++++++++++ services/hackbot-ui/components/RunDetail.tsx | 9 +- 13 files changed, 1018 insertions(+), 4 deletions(-) create mode 100644 libs/hackbot-runtime/hackbot_runtime/actions/github.py create mode 100644 libs/hackbot-runtime/tests/test_github_actions.py create mode 100644 services/hackbot-api/app/action_handlers/github_handler.py create mode 100644 services/hackbot-api/tests/test_github_handler.py diff --git a/docs/hackbot/actions.md b/docs/hackbot/actions.md index 98ca9f12f38..e0184eb31a2 100644 --- a/docs/hackbot/actions.md +++ b/docs/hackbot/actions.md @@ -44,6 +44,7 @@ triage run but swaps it for `phabricator.update_patch` on a follow-up. | `phabricator.submit_patch` | Deliver a fix as a **new** revision | `bug_id`, `title`, `summary`, `test_plan`, `parent_revision_id` | | `phabricator.update_patch` | Add a new diff to an **existing** revision | `revision_id`, `title`, `summary`, `test_plan` | | `phabricator.add_comment` | Reply on a revision without changing code | `revision_id`, `text` | +| `github.submit_patch` | Deliver a fix as a **new** pull request | `title`, `body` | | `testrail.submit_test_plan` | Submit a generated test plan to TestRail | the validated feature + test cases | | `slack.post_message` | Post a message to Slack | `channel`, `text` | | `email.send` | Email a report about the run | `to`, `subject`, `body_markdown` | @@ -79,6 +80,15 @@ Neither takes a patch file. The agent's final working-tree state _is_ the diff why the runtime builds the Phabricator payload during `publish_changes`, while the checkout still exists. +`github.submit_patch` is the GitHub counterpart of `phabricator.submit_patch`, for an agent +whose `[source].repo_url` is on GitHub. hackbot-api has no checkout to apply a patch in, so +`publish_changes` also captures each of the run's commits (message, author and changed +files) as `changes/github_commits.json`. The handler recreates those commits through +GitHub's Git Data API on a `hackbot/` branch of the source repository and opens a +draft pull request against its default branch, with the GitHub App's credentials (so the +App must be installed on that repository). Retrying reuses the branch and any pull request +already open from it. + ### Hooks `ActionsRecorder` supports per-type hooks that run before an action is appended. A hook may diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/__init__.py b/libs/hackbot-runtime/hackbot_runtime/actions/__init__.py index 2396f16dd75..bd021d1194e 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/__init__.py +++ b/libs/hackbot-runtime/hackbot_runtime/actions/__init__.py @@ -13,6 +13,7 @@ bugzilla, email, feedback, + github, phabricator, slack, testrail, @@ -31,6 +32,7 @@ "bugzilla", "email", "feedback", + "github", "phabricator", "slack", "testrail", diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/claude_sdk.py b/libs/hackbot-runtime/hackbot_runtime/actions/claude_sdk.py index b460943c211..5cdbfaf6f30 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/claude_sdk.py +++ b/libs/hackbot-runtime/hackbot_runtime/actions/claude_sdk.py @@ -15,6 +15,7 @@ from hackbot_runtime.actions import agents as _agents from hackbot_runtime.actions import bugzilla as _bugzilla from hackbot_runtime.actions import feedback as _feedback +from hackbot_runtime.actions import github as _github from hackbot_runtime.actions import phabricator as _phabricator from hackbot_runtime.actions import slack as _slack from hackbot_runtime.actions import testrail as _testrail @@ -50,6 +51,7 @@ def actions_server_for( + _agents.TOOLS + _bugzilla.TOOLS + _phabricator.TOOLS + + _github.TOOLS + _testrail.TOOLS + _slack.TOOLS + _try_server.TOOLS diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/github.py b/libs/hackbot-runtime/hackbot_runtime/actions/github.py new file mode 100644 index 00000000000..f52e4756dae --- /dev/null +++ b/libs/hackbot-runtime/hackbot_runtime/actions/github.py @@ -0,0 +1,70 @@ +"""GitHub-domain recordable actions. + +Mirrors ``actions/phabricator.py``: the handler records an intended change +(nothing is pushed to GitHub here) and returns a short confirmation string. See +hackbot-api's ``app/action_handlers/github_handler.py`` for the apply side, +which pushes a branch carrying the run's commits and opens a pull request. +""" + +from typing import Annotated + +from agent_tools.registry import tool, tools_in +from pydantic import Field + +from hackbot_runtime.actions.recorder import ActionsRecorder, confirmation + +SUBMIT_PATCH_ACTION_TYPE = "github.submit_patch" + +# Actions that deliver the working directory's changes to GitHub, gating the +# commits artifact built in ``context.publish_changes``. The GitHub counterpart +# of ``phabricator.PATCH_ACTION_TYPES``: kept separate so a GitHub submission +# does not also build a Phabricator diff, and vice versa. +PATCH_ACTION_TYPES = frozenset({SUBMIT_PATCH_ACTION_TYPE}) + + +@tool +async def submit_patch( + recorder: ActionsRecorder, + title: Annotated[ + str, + Field( + description=( + "Title for the new pull request: a single line describing the " + "fix, as you would write a commit message subject." + ) + ), + ], + reasoning: Annotated[ + str, Field(description="Why you are submitting this patch (for audit log).") + ], + body: Annotated[ + str | None, + Field( + description=( + "Pull request description (GitHub Markdown). Explain what the " + "change does and how you verified it. Keep it concise and do not " + "narrate the investigation." + ), + ), + ] = None, +) -> str: + """Submit your fix for review as a new GitHub pull request. + + This is how you deliver a code fix to a repository hosted on GitHub. The + pull request is opened from a new branch against the repository's default + branch, as a draft. + + You do not supply a patch file. Your final code changes in the working + directory (and any commits you made) are pushed as the pull request's + commits, 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; + nothing is sent to GitHub during the run. + + Later actions can reference its `url`. + """ + params: dict = {"title": title, "body": body} + action = recorder.record(SUBMIT_PATCH_ACTION_TYPE, params, reasoning=reasoning) + return confirmation(action) + + +TOOLS = tools_in(__name__) diff --git a/libs/hackbot-runtime/hackbot_runtime/changes.py b/libs/hackbot-runtime/hackbot_runtime/changes.py index 79526e9305e..e6cd534b461 100644 --- a/libs/hackbot-runtime/hackbot_runtime/changes.py +++ b/libs/hackbot-runtime/hackbot_runtime/changes.py @@ -335,6 +335,90 @@ def build_try_push(repo: Path, base: str) -> dict | None: } +# A submodule entry: its tree entry points at a commit, not a blob. +_GITLINK_MODE = "160000" + + +def _github_file_changes(repo: Path, parent: str, commit: str) -> list[dict]: + """The tree entries ``commit`` changes relative to ``parent``. + + Each entry is ``{"path", "mode"}`` plus one of ``"content"`` (the new blob, + base64-encoded so binary files survive JSON), ``"sha"`` (a submodule's new + commit) or ``"deleted": True``. Renames are reported as a deletion plus an + addition, which is how a git tree records them anyway. + """ + out = _git_bytes( + repo, "diff-tree", "-r", "-z", "--no-renames", "--no-commit-id", parent, commit + ) + # -z output is ": \0\0" per + # entry, with paths unquoted. GitHub only takes UTF-8 paths, so any other + # path raises here rather than being pushed mangled. + fields = out.split(b"\0") + entries = [] + for header, raw_path in zip(fields[::2], fields[1::2]): + src_mode, dst_mode, _src_sha, dst_sha, status = header.lstrip(b":").split() + path = raw_path.decode("utf-8") + if status == b"D": + entries.append({"path": path, "mode": src_mode.decode(), "deleted": True}) + elif dst_mode.decode() == _GITLINK_MODE: + entries.append( + {"path": path, "mode": _GITLINK_MODE, "sha": dst_sha.decode()} + ) + else: + blob = _git_bytes(repo, "cat-file", "blob", dst_sha.decode()) + entries.append( + { + "path": path, + "mode": dst_mode.decode(), + "content": base64.b64encode(blob).decode("ascii"), + } + ) + return entries + + +def build_github_commits(repo: Path, base: str, repo_url: str) -> dict | None: + """Build the artifact for pushing the agent's commits to GitHub. + + hackbot-api has no git checkout (or git binary) to ``git am`` the run's + patch into, so this captures each commit in ``base..HEAD`` in the shape + GitHub's Git Data API takes: the full message, the author, and the files it + changes with their new contents. The apply-side handler recreates the same + commits on top of ``base`` from that alone. + + ``base`` must be a commit the GitHub repository already has (the published + base, as for a try push), since the recreated commits are parented on it. + Returns ``None`` when there are no commits to push. + """ + # Normally already done by `collect`; repeated here (a no-op on a clean + # tree) so this does not drop uncommitted work if called on its own. + commit_all(repo, WIP_MESSAGE) + + # First parent only: each commit is replayed as the diff against the one + # before it, so a merge the agent made is flattened rather than lost. + revisions = _git( + repo, "rev-list", "--reverse", "--first-parent", f"{base}..HEAD" + ).split() + if not revisions: + return None + + fmt = _FIELD_SEP.join(["%an", "%ae", "%aI", "%B"]) + commits = [] + parent = base + for revision in revisions: + out = _git(repo, "show", "-s", f"--format={fmt}", revision) + name, email, date, message = out.split(_FIELD_SEP, 3) + commits.append( + { + "message": message.rstrip("\n"), + "author": {"name": name, "email": email, "date": date}, + "changes": _github_file_changes(repo, parent, revision), + } + ) + parent = revision + + return {"repo_url": repo_url, "base_commit": base, "commits": commits} + + def collect( repo: Path, base: str, repo_url: str, message: str = WIP_MESSAGE ) -> ChangeSet | None: diff --git a/libs/hackbot-runtime/hackbot_runtime/context.py b/libs/hackbot-runtime/hackbot_runtime/context.py index 4450834934a..49fde3930c3 100644 --- a/libs/hackbot-runtime/hackbot_runtime/context.py +++ b/libs/hackbot-runtime/hackbot_runtime/context.py @@ -23,6 +23,7 @@ from pydantic_settings import BaseSettings, SettingsConfigDict from hackbot_runtime import artifacts, changes +from hackbot_runtime.actions import github from hackbot_runtime.actions.phabricator import ( PATCH_ACTION_TYPES, SUBMIT_PATCH_ACTION_TYPE, @@ -53,7 +54,11 @@ def _default_run_id() -> str: def _wip_commit_message(actions: list[dict]) -> str: """The patch title, else the try push title, for the agent's uncommitted work.""" - for action_type in (SUBMIT_PATCH_ACTION_TYPE, TRY_PUSH_ACTION_TYPE): + for action_type in ( + SUBMIT_PATCH_ACTION_TYPE, + github.SUBMIT_PATCH_ACTION_TYPE, + TRY_PUSH_ACTION_TYPE, + ): for action in actions: if action["type"] == action_type and action["params"]["title"]: return action["params"]["title"] @@ -304,6 +309,7 @@ def publish_changes( meta_key: str = "changes/changes.json", phabricator_diff_key: str = "changes/phabricator_diff.json", try_push_key: str = "changes/try_push.json", + github_commits_key: str = "changes/github_commits.json", ) -> str | None: """Collect the agent's source-tree changes and publish them as artifacts.""" if self._source_base is None: @@ -334,6 +340,25 @@ def publish_changes( if diff_payload is not None: self.publish_json(phabricator_diff_key, diff_payload) + if recorded_types & github.PATCH_ACTION_TYPES: + # From the published base, like the try push: the commits are + # recreated on GitHub on top of it, so it has to be a commit the + # repository already has. + try: + github_payload = changes.build_github_commits( + self.repo_path, + self._published_base, + self._config.source.repo_url, + ) + except Exception: + # Best-effort, as for the Phabricator diff: without the + # artifact the action fails when applied, saying so, rather + # than failing an otherwise successful run here. + log.warning("Could not build the GitHub commits", exc_info=True) + github_payload = None + if github_payload is not None: + self.publish_json(github_commits_key, github_payload) + if recorded_types & TRY_ACTION_TYPES: # From the *published* base, not `_source_base`: Lando has to # resolve the base commit in its own clone, and it pushes the patch diff --git a/libs/hackbot-runtime/tests/test_changes.py b/libs/hackbot-runtime/tests/test_changes.py index 69c0577b4d1..8f08be8893d 100644 --- a/libs/hackbot-runtime/tests/test_changes.py +++ b/libs/hackbot-runtime/tests/test_changes.py @@ -13,6 +13,7 @@ from hackbot_runtime.changes import ( _git, _synthetic_commit, + build_github_commits, build_phabricator_diff, build_try_push, has_changes, @@ -211,6 +212,92 @@ def test_build_try_push_rejects_abbreviated_base(tmp_path): assert build_try_push(tmp_path, base[:12]) is None +# --- build_github_commits ------------------------------------------------ # + +_REPO_URL = "https://github.com/owner/repo.git" + + +def _changes_by_path(commit): + return {change["path"]: change for change in commit["changes"]} + + +def test_build_github_commits_one_entry_per_commit(tmp_path): + base = _init_repo(tmp_path) + _commit_change(tmp_path, "one\n", message="First\n\nWith a body.") + _commit_change(tmp_path, "two\n", message="Second") + + payload = build_github_commits(tmp_path, base, _REPO_URL) + + assert payload["repo_url"] == _REPO_URL + assert payload["base_commit"] == base + first, second = payload["commits"] + assert first["message"] == "First\n\nWith a body." + assert second["message"] == "Second" + assert first["author"]["name"] == "Test" + assert first["author"]["email"] == "test@test.com" + assert first["author"]["date"] + # Each commit carries the file as it is at that commit, not at HEAD. + assert base64.b64decode(first["changes"][0]["content"]) == b"one\n" + assert base64.b64decode(second["changes"][0]["content"]) == b"two\n" + assert second["changes"][0] == { + "path": "file.txt", + "mode": "100644", + "content": base64.b64encode(b"two\n").decode(), + } + + +def test_build_github_commits_includes_uncommitted_work(tmp_path): + base = _init_repo(tmp_path) + (tmp_path / "file.txt").write_text("edited\n") + (tmp_path / "new.txt").write_text("untracked\n") + + payload = build_github_commits(tmp_path, base, _REPO_URL) + + (commit,) = payload["commits"] + assert commit["message"] == "Uncommitted agent changes" + assert set(_changes_by_path(commit)) == {"file.txt", "new.txt"} + + +def test_build_github_commits_records_deletions_modes_and_binaries(tmp_path): + base = _init_repo(tmp_path) + (tmp_path / "file.txt").unlink() + script = tmp_path / "dir with space" / "run.sh" + script.parent.mkdir() + script.write_text("#!/bin/sh\n") + script.chmod(0o755) + (tmp_path / "image.bin").write_bytes(b"\x00\xff\x10") + + payload = build_github_commits(tmp_path, base, _REPO_URL) + + changes = _changes_by_path(payload["commits"][0]) + assert changes["file.txt"] == { + "path": "file.txt", + "mode": "100644", + "deleted": True, + } + assert changes["dir with space/run.sh"]["mode"] == "100755" + assert base64.b64decode(changes["image.bin"]["content"]) == b"\x00\xff\x10" + + +def test_build_github_commits_reports_a_rename_as_delete_and_add(tmp_path): + base = _init_repo(tmp_path) + _git(tmp_path, "mv", "file.txt", "renamed.txt") + + payload = build_github_commits(tmp_path, base, _REPO_URL) + + changes = _changes_by_path(payload["commits"][0]) + assert changes["file.txt"]["deleted"] is True + assert base64.b64decode(changes["renamed.txt"]["content"]) == ( + b"line1\nline2\nline3\n" + ) + + +def test_build_github_commits_no_changes_returns_none(tmp_path): + base = _init_repo(tmp_path) + + assert build_github_commits(tmp_path, base, _REPO_URL) is None + + # --- has_changes ------------------------------------------------------- # diff --git a/libs/hackbot-runtime/tests/test_context.py b/libs/hackbot-runtime/tests/test_context.py index 626f5f6a9d6..439212cbbd5 100644 --- a/libs/hackbot-runtime/tests/test_context.py +++ b/libs/hackbot-runtime/tests/test_context.py @@ -440,3 +440,80 @@ def test_a_parent_with_no_artifacts_is_not_mistaken_for_a_local_run( ) hb.fetch_parent_artifacts() assert hb.parent_artifacts_dir != tmp_path / "artifacts" / "parent" + + +def test_publish_changes_builds_github_commits_from_the_published_base( + tmp_path, monkeypatch +): + hb = _hb_with_source(tmp_path, monkeypatch) + calls = {} + + def _github_commits(repo, base, repo_url): + calls["github"] = (base, repo_url) + return {"repo_url": repo_url, "base_commit": base, "commits": []} + + def _phabricator_diff(*args): + calls["phabricator"] = args + return None + + monkeypatch.setattr( + "hackbot_runtime.context.changes.build_github_commits", _github_commits + ) + monkeypatch.setattr( + "hackbot_runtime.context.changes.build_phabricator_diff", _phabricator_diff + ) + monkeypatch.setattr( + "hackbot_runtime.context.changes.base_commit", lambda repo: "localseededsha" + ) + hb.record_source_base() + hb.actions.record("github.submit_patch", {"title": "Fix"}, reasoning="r") + + hb.publish_changes() + + payload = json.loads( + ( + tmp_path / "artifacts" / "local-test" / "changes" / "github_commits.json" + ).read_text() + ) + # GitHub parents the recreated commits on the base, so it must be a commit + # the repository has, not one seeded locally. + assert payload["base_commit"] == "basecommit" + assert calls["github"] == ("basecommit", hb.config.source.repo_url) + # A GitHub submission does not build a Phabricator diff. + assert "phabricator" not in calls + + +def test_publish_changes_survives_a_failed_github_commits_build(tmp_path, monkeypatch): + hb = _hb_with_source(tmp_path, monkeypatch) + + def _boom(*args): + raise UnicodeDecodeError("utf-8", b"\xff", 0, 1, "bad path") + + monkeypatch.setattr("hackbot_runtime.context.changes.build_github_commits", _boom) + hb.actions.record("github.submit_patch", {"title": "Fix"}, reasoning="r") + + assert hb.publish_changes() == "changes/changes.patch" + assert not ( + tmp_path / "artifacts" / "local-test" / "changes" / "github_commits.json" + ).exists() + + +def test_publish_changes_names_the_agents_commit_after_a_github_title( + tmp_path, monkeypatch +): + hb = _hb_with_source(tmp_path, monkeypatch) + messages = [] + + def _collect(repo, base, repo_url, message): + messages.append(message) + return ChangeSet(patch=b"x", metadata={"base": base}) + + monkeypatch.setattr("hackbot_runtime.context.changes.collect", _collect) + monkeypatch.setattr( + "hackbot_runtime.context.changes.build_github_commits", lambda *a: None + ) + hb.actions.record("github.submit_patch", {"title": "Fix it"}, reasoning="r") + + hb.publish_changes() + + assert messages == ["Fix it"] diff --git a/libs/hackbot-runtime/tests/test_github_actions.py b/libs/hackbot-runtime/tests/test_github_actions.py new file mode 100644 index 00000000000..5b7eec81230 --- /dev/null +++ b/libs/hackbot-runtime/tests/test_github_actions.py @@ -0,0 +1,39 @@ +"""Tests for the GitHub recording tools.""" + +from hackbot_runtime.actions import ActionsRecorder, github, phabricator +from hackbot_runtime.actions.claude_sdk import actions_to_tool_names + + +async def test_submit_patch_records_title_and_body(): + rec = ActionsRecorder() + + reply = await github.submit_patch( + rec, title="Fix the thing", body="Details", reasoning="r" + ) + + action = rec.actions[0] + assert action["type"] == "github.submit_patch" + assert action["params"] == {"title": "Fix the thing", "body": "Details"} + assert action["reasoning"] == "r" + assert reply.startswith("Recorded github.submit_patch (ID: ") + + +async def test_submit_patch_body_is_optional(): + rec = ActionsRecorder() + + await github.submit_patch(rec, title="Fix", reasoning="r") + + assert rec.actions[0]["params"] == {"title": "Fix", "body": None} + + +def test_submit_patch_is_exposed_as_an_action_tool(): + assert [t.dotted for t in github.TOOLS] == ["github.submit_patch"] + assert actions_to_tool_names(["github.submit_patch"]) == [ + "mcp__actions__github_submit_patch" + ] + + +def test_github_patch_types_do_not_overlap_phabricator_ones(): + # Each gates building its own artifact in publish_changes. + assert {"github.submit_patch"} == github.PATCH_ACTION_TYPES + assert not github.PATCH_ACTION_TYPES & phabricator.PATCH_ACTION_TYPES diff --git a/services/hackbot-api/app/action_handlers/github_handler.py b/services/hackbot-api/app/action_handlers/github_handler.py new file mode 100644 index 00000000000..6899e8b9529 --- /dev/null +++ b/services/hackbot-api/app/action_handlers/github_handler.py @@ -0,0 +1,254 @@ +"""Apply-side GitHub actions: push the run's commits and open a pull request. + +Pairs with the recording side in ``hackbot_runtime/actions/github.py`` and the +payload built agent-side in ``hackbot_runtime.changes.build_github_commits``. +This service has no checkout (or git binary), so the commits are recreated +through GitHub's Git Data API: a blob per changed file, a tree per commit on top +of its parent's, then the commit itself. Commits made this way are attributed to +their original author and committed (and signed) by the GitHub App. + +The pull request goes to the repository the agent's source was checked out +from, which must have the GitHub App installed. +""" + +import json +import logging +import re +from functools import lru_cache +from typing import Any + +from githubkit import AppInstallationAuthStrategy, GitHub +from githubkit.exception import RequestFailed + +from app.action_handlers.base import ActionResult, ApplyContext +from app.config import settings +from app.github import READ_WRITE_PERMISSIONS, GitHubApp + +log = logging.getLogger(__name__) + +_COMMITS_ARTIFACT_KEY = "changes/github_commits.json" + +# The forms a ``[source].repo_url`` on GitHub takes. +_GITHUB_REPO_URL_RE = re.compile( + r"^(?:https://github\.com/|git@github\.com:|ssh://git@github\.com/)" + r"(?P[A-Za-z0-9-]+)/(?P[A-Za-z0-9._-]+?)(?:\.git)?/?$" +) + +_GITLINK_MODE = "160000" + + +@lru_cache(maxsize=1) +def _github_app() -> GitHubApp: + if settings.github is None: + raise RuntimeError("The GitHub App is not configured (GITHUB_* settings)") + return GitHubApp(settings.github) + + +def parse_repo_url(repo_url: str) -> tuple[str, str] | None: + """``(owner, repo)`` for a GitHub repository URL, else ``None``.""" + match = _GITHUB_REPO_URL_RE.match(repo_url.strip()) + if match is None: + return None + return match["owner"], match["repo"] + + +def branch_name(run_id: str) -> str: + """The branch a run's pull request is opened from. + + One per run: every submission in a run carries the same commits (there is + one changes artifact per run), so a second one, or a retry, reuses it. + """ + return f"hackbot/{run_id}" + + +async def _installation_client( + owner: str, repo: str +) -> GitHub[AppInstallationAuthStrategy]: + """A client for ``owner/repo``, with a token narrowed to that repository.""" + github_app = _github_app() + # githubkit's ``rest`` namespace only holds a weak reference to its client, + # so the client must outlive the call. + app_client = github_app.as_app() + installation = await app_client.rest.apps.async_get_repo_installation(owner, repo) + return github_app.as_installation( + installation.json()["id"], + permissions=READ_WRITE_PERMISSIONS, + repositories=[repo], + ) + + +async def _tree_entry( + github: GitHub, owner: str, repo: str, change: dict[str, Any] +) -> dict[str, Any]: + """The Git Data API tree entry for one changed path.""" + mode = change["mode"] + entry_type = "commit" if mode == _GITLINK_MODE else "blob" + if change.get("deleted"): + sha = None + elif "sha" in change: + sha = change["sha"] + else: + blob = await github.rest.git.async_create_blob( + owner, repo, data={"content": change["content"], "encoding": "base64"} + ) + sha = blob.json()["sha"] + return {"path": change["path"], "mode": mode, "type": entry_type, "sha": sha} + + +async def _push_commits( + github: GitHub, owner: str, repo: str, submission: dict[str, Any] +) -> str: + """Recreate the run's commits on top of its base; return the new head sha. + + Requests are made one at a time, as GitHub asks of content-creating calls to + stay clear of its secondary rate limits. + """ + parent = submission["base_commit"] + base_commit = await github.rest.git.async_get_commit(owner, repo, parent) + tree = base_commit.json()["tree"]["sha"] + + for commit in submission["commits"]: + entries = [ + await _tree_entry(github, owner, repo, change) + for change in commit["changes"] + ] + if entries: + new_tree = await github.rest.git.async_create_tree( + owner, repo, data={"base_tree": tree, "tree": entries} + ) + tree = new_tree.json()["sha"] + new_commit = await github.rest.git.async_create_commit( + owner, + repo, + data={ + "message": commit["message"], + "tree": tree, + "parents": [parent], + "author": commit["author"], + }, + ) + parent = new_commit.json()["sha"] + return parent + + +async def _point_branch( + github: GitHub, owner: str, repo: str, branch: str, sha: str +) -> None: + """Create ``branch`` at ``sha``, or move it there if it already exists. + + It only exists already if an earlier attempt at this run's submission got + this far, since the name is the run's own. + """ + try: + await github.rest.git.async_create_ref( + owner, repo, data={"ref": f"refs/heads/{branch}", "sha": sha} + ) + except RequestFailed as exc: + if exc.response.status_code != 422: + raise + await github.rest.git.async_update_ref( + owner, repo, f"heads/{branch}", data={"sha": sha, "force": True} + ) + + +async def _open_pull_request( + github: GitHub, + owner: str, + repo: str, + *, + branch: str, + base: str, + title: str, + body: str | None, +) -> dict[str, Any]: + """Open a draft pull request from ``branch``, or return the one already open.""" + data: dict[str, Any] = { + "title": title, + "head": branch, + "base": base, + # Everything hackbot submits is a draft, as on Phabricator, so a human + # decides when it is ready for review. + "draft": True, + } + if body: + data["body"] = body + try: + response = await github.rest.pulls.async_create(owner, repo, data=data) + except RequestFailed as exc: + if exc.response.status_code != 422: + raise + existing = await github.rest.pulls.async_list( + owner, repo, state="open", head=f"{owner}:{branch}" + ) + pull_requests = existing.json() + if not pull_requests: + raise + return pull_requests[0] + return response.json() + + +class SubmitPatchHandler: + """Applies ``github.submit_patch``: the run's commits become a pull request. + + The branch is pushed to the source repository itself and the pull request + targets its default branch. Retrying after a partial failure reuses the + branch and any pull request already open from it. + """ + + async def apply(self, params: dict[str, Any], ctx: ApplyContext) -> ActionResult: + try: + raw = await ctx.download_artifact(_COMMITS_ARTIFACT_KEY) + submission = json.loads(raw) + except Exception as exc: + log.exception( + "Failed to load GitHub commits artifact for run %s", ctx.run_id + ) + return ActionResult.failed( + f"Could not load the GitHub commits artifact for this run: {exc}" + ) + + repo_url = submission["repo_url"] + parsed = parse_repo_url(repo_url) + if parsed is None: + return ActionResult.failed( + f"The agent's source repository {repo_url!r} is not on GitHub" + ) + if not submission["commits"]: + return ActionResult.failed("This run has no commits to submit") + owner, repo = parsed + branch = branch_name(ctx.run_id) + + try: + github = await _installation_client(owner, repo) + repository = await github.rest.repos.async_get(owner, repo) + default_branch = repository.json()["default_branch"] + head = await _push_commits(github, owner, repo, submission) + await _point_branch(github, owner, repo, branch, head) + pull_request = await _open_pull_request( + github, + owner, + repo, + branch=branch, + base=default_branch, + title=params["title"].strip(), + body=params.get("body"), + ) + except Exception as exc: + log.exception( + "Failed to open a pull request on %s/%s for run %s", + owner, + repo, + ctx.run_id, + ) + return ActionResult.failed(str(exc)) + + return ActionResult.ok( + { + "pull_request_number": pull_request["number"], + # `url` is the field a `{{actions..url}}` placeholder + # reads, so it is the page a human would want. + "url": pull_request["html_url"], + "repository": f"{owner}/{repo}", + "branch": branch, + } + ) diff --git a/services/hackbot-api/app/action_handlers/registry.py b/services/hackbot-api/app/action_handlers/registry.py index a0905eb6752..43598e5ce48 100644 --- a/services/hackbot-api/app/action_handlers/registry.py +++ b/services/hackbot-api/app/action_handlers/registry.py @@ -8,6 +8,9 @@ ) from app.action_handlers.email_handler import SendEmailHandler from app.action_handlers.feedback_handler import ReportProblemHandler +from app.action_handlers.github_handler import ( + SubmitPatchHandler as GitHubSubmitPatchHandler, +) from app.action_handlers.phabricator_handler import ( AddCommentHandler as PhabricatorAddCommentHandler, ) @@ -19,8 +22,10 @@ from app.action_handlers.testrail_handler import SubmitTestPlanHandler from app.action_handlers.try_server_handler import PushHandler -# Actions that submit source changes to Phabricator. -PATCH_ACTION_TYPES = frozenset({"phabricator.submit_patch", "phabricator.update_patch"}) +# Actions that submit source changes for review, on Phabricator or GitHub. +PATCH_ACTION_TYPES = frozenset( + {"phabricator.submit_patch", "phabricator.update_patch", "github.submit_patch"} +) # 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 @@ -33,6 +38,7 @@ "phabricator.submit_patch": SubmitPatchHandler(), "phabricator.update_patch": UpdatePatchHandler(), "phabricator.add_comment": PhabricatorAddCommentHandler(), + "github.submit_patch": GitHubSubmitPatchHandler(), "testrail.submit_test_plan": SubmitTestPlanHandler(), "slack.post_message": PostMessageHandler(), "email.send": SendEmailHandler(), diff --git a/services/hackbot-api/tests/test_github_handler.py b/services/hackbot-api/tests/test_github_handler.py new file mode 100644 index 00000000000..af98afe9692 --- /dev/null +++ b/services/hackbot-api/tests/test_github_handler.py @@ -0,0 +1,351 @@ +"""Tests for the apply-side GitHub action handler. + +Drives the real githubkit client against a fake api.github.com (an +``httpx.MockTransport``), so these check the Git Data and pull request calls +the handler actually makes. The commits artifact it reads is built agent-side +(see hackbot-runtime's test_changes.py ``build_github_commits`` tests). +""" + +import json +from datetime import UTC, datetime, timedelta + +import httpx +import pytest +from app.action_handlers import ApplyContext, github_handler +from app.action_handlers.registry import PATCH_ACTION_TYPES, get_handler +from app.config import GitHubAppSettings +from app.github import GitHubApp +from cryptography.hazmat.primitives import serialization +from cryptography.hazmat.primitives.asymmetric import rsa + +OWNER = "owner" +REPO = "repo" +INSTALLATION_ID = 789 +RUN_ID = "run-1" +BRANCH = "hackbot/run-1" +PR_URL = f"https://github.com/{OWNER}/{REPO}/pull/7" + +_SUBMISSION = { + "repo_url": f"https://github.com/{OWNER}/{REPO}.git", + "base_commit": "base", + "commits": [ + { + "message": "First\n\nBody.", + "author": { + "name": "Dev", + "email": "dev@example.com", + "date": "2026-11-09T10:00:00+00:00", + }, + "changes": [ + {"path": "a.txt", "mode": "100644", "content": "YQo="}, + {"path": "old.txt", "mode": "100644", "deleted": True}, + ], + }, + { + "message": "Second", + "author": { + "name": "Hackbot", + "email": "hackbot@mozilla.tld", + "date": "2026-11-09T11:00:00+00:00", + }, + "changes": [ + {"path": "run.sh", "mode": "100755", "content": "IyEK"}, + {"path": "third_party/lib", "mode": "160000", "sha": "c0ffee"}, + ], + }, + ], +} + + +@pytest.fixture(scope="module") +def pem() -> str: + key = rsa.generate_private_key(public_exponent=65537, key_size=2048) + return key.private_bytes( + serialization.Encoding.PEM, + serialization.PrivateFormat.TraditionalOpenSSL, + serialization.NoEncryption(), + ).decode() + + +class _FakeGitHub: + """Stands in for api.github.com, numbering every object it creates.""" + + def __init__(self, *, branch_exists=False, pull_request_exists=False): + self.branch_exists = branch_exists + self.pull_request_exists = pull_request_exists + self.requests: list[tuple[str, str, dict | None]] = [] + self.created = 0 + + def calls(self, method: str, path: str) -> list[dict | None]: + return [body for m, p, body in self.requests if (m, p) == (method, path)] + + def _new_sha(self, kind: str) -> str: + self.created += 1 + return f"{kind}{self.created}" + + async def __call__(self, request: httpx.Request) -> httpx.Response: + method = request.method + # Undecoded, to catch a ref path whose slashes get escaped. + path = request.url.raw_path.decode().partition("?")[0] + body = json.loads(request.content) if request.content else None + self.requests.append((method, path, body)) + repo = f"/repos/{OWNER}/{REPO}" + + if path.endswith("/access_tokens"): + expires_at = datetime.now(UTC) + timedelta(hours=1) + return httpx.Response( + 201, + json={ + "token": "ghs_test", + "expires_at": expires_at.strftime("%Y-%m-%dT%H:%M:%SZ"), + }, + ) + match method, path.removeprefix(repo): + case "GET", "/installation": + return httpx.Response(200, json={"id": INSTALLATION_ID}) + case "GET", "": + return httpx.Response(200, json={"default_branch": "main"}) + case "GET", "/git/commits/base": + return httpx.Response( + 200, json={"sha": "base", "tree": {"sha": "basetree"}} + ) + case "POST", "/git/blobs": + return httpx.Response(201, json={"sha": self._new_sha("blob")}) + case "POST", "/git/trees": + return httpx.Response(201, json={"sha": self._new_sha("tree")}) + case "POST", "/git/commits": + return httpx.Response(201, json={"sha": self._new_sha("commit")}) + case "POST", "/git/refs": + if self.branch_exists: + return httpx.Response( + 422, json={"message": "Reference already exists"} + ) + return httpx.Response(201, json={"ref": body["ref"]}) + case "PATCH", "/git/refs/heads/hackbot/run-1": + return httpx.Response(200, json={"ref": f"refs/heads/{BRANCH}"}) + case "POST", "/pulls": + if self.pull_request_exists: + return httpx.Response( + 422, json={"message": "A pull request already exists"} + ) + return httpx.Response(201, json={"number": 7, "html_url": PR_URL}) + case "GET", "/pulls": + return httpx.Response(200, json=[{"number": 7, "html_url": PR_URL}]) + return httpx.Response(404, json={"message": f"unexpected {method} {path}"}) + + +@pytest.fixture +def fake_github(monkeypatch, pem): + def install(**kwargs) -> _FakeGitHub: + fake = _FakeGitHub(**kwargs) + app = GitHubApp( + GitHubAppSettings( + app_id=123, private_key=pem, webhook_secret="test-webhook-secret" + ), + async_transport=httpx.MockTransport(fake), + http_cache=False, + auto_retry=False, + ) + monkeypatch.setattr(github_handler, "_github_app", lambda: app) + return fake + + return install + + +def _ctx(submission=_SUBMISSION): + async def download(key, max_bytes=None): + assert key == "changes/github_commits.json" + if submission is None: + raise FileNotFoundError(key) + return json.dumps(submission).encode() + + return ApplyContext( + run_id=RUN_ID, + agent="test-agent", + download_artifact=download, + hackbot_client=object(), + ) + + +_PARAMS = {"title": " Fix the thing ", "body": "What and why."} + + +def test_submit_patch_is_registered_as_a_patch_action(): + assert isinstance( + get_handler("github.submit_patch"), github_handler.SubmitPatchHandler + ) + assert "github.submit_patch" in PATCH_ACTION_TYPES + + +@pytest.mark.parametrize( + ("url", "expected"), + [ + ("https://github.com/mozilla/bugbug.git", ("mozilla", "bugbug")), + ("https://github.com/mozilla/bugbug", ("mozilla", "bugbug")), + ( + "https://github.com/mozilla-firefox/firefox.git/", + ("mozilla-firefox", "firefox"), + ), + ("git@github.com:mozilla/bugbug.git", ("mozilla", "bugbug")), + ("ssh://git@github.com/mozilla/web.compat.git", ("mozilla", "web.compat")), + ("https://hg.mozilla.org/mozilla-central", None), + ("https://github.com.evil.example/mozilla/bugbug", None), + ("https://github.com/mozilla/bugbug/tree/main", None), + ], +) +def test_parse_repo_url(url, expected): + assert github_handler.parse_repo_url(url) == expected + + +async def test_submit_patch_recreates_the_commits_and_opens_a_draft_pr(fake_github): + fake = fake_github() + + result = await github_handler.SubmitPatchHandler().apply(_PARAMS, _ctx()) + + assert result.status == "applied", result.error + assert result.result == { + "pull_request_number": 7, + "url": PR_URL, + "repository": f"{OWNER}/{REPO}", + "branch": BRANCH, + } + + repo = f"/repos/{OWNER}/{REPO}" + # One blob per changed file with content; deletions and submodules need none. + assert fake.calls("POST", f"{repo}/git/blobs") == [ + {"content": "YQo=", "encoding": "base64"}, + {"content": "IyEK", "encoding": "base64"}, + ] + first_tree, second_tree = fake.calls("POST", f"{repo}/git/trees") + assert first_tree == { + "base_tree": "basetree", + "tree": [ + {"path": "a.txt", "mode": "100644", "type": "blob", "sha": "blob1"}, + {"path": "old.txt", "mode": "100644", "type": "blob", "sha": None}, + ], + } + # Each commit's tree builds on the one before it. + assert second_tree == { + "base_tree": "tree2", + "tree": [ + {"path": "run.sh", "mode": "100755", "type": "blob", "sha": "blob4"}, + { + "path": "third_party/lib", + "mode": "160000", + "type": "commit", + "sha": "c0ffee", + }, + ], + } + first_commit, second_commit = fake.calls("POST", f"{repo}/git/commits") + assert first_commit == { + "message": "First\n\nBody.", + "tree": "tree2", + "parents": ["base"], + # githubkit writes a UTC offset as "Z"; any other offset is kept. + "author": { + "name": "Dev", + "email": "dev@example.com", + "date": "2026-11-09T10:00:00Z", + }, + } + assert second_commit["parents"] == ["commit3"] + assert second_commit["tree"] == "tree5" + # No committer: GitHub commits (and signs) as the App. + assert "committer" not in second_commit + + assert fake.calls("POST", f"{repo}/git/refs") == [ + {"ref": f"refs/heads/{BRANCH}", "sha": "commit6"} + ] + assert fake.calls("POST", f"{repo}/pulls") == [ + { + "title": "Fix the thing", + "head": BRANCH, + "base": "main", + "draft": True, + "body": "What and why.", + } + ] + # The token is narrowed to the one repository, with write access. + (token_request,) = fake.calls( + "POST", f"/app/installations/{INSTALLATION_ID}/access_tokens" + ) + assert token_request["repositories"] == [REPO] + assert token_request["permissions"] == { + "contents": "write", + "pull_requests": "write", + } + + +async def test_submit_patch_omits_an_empty_body(fake_github): + fake = fake_github() + + result = await github_handler.SubmitPatchHandler().apply( + {"title": "Fix", "body": None}, _ctx() + ) + + assert result.status == "applied", result.error + (pull_request,) = fake.calls("POST", f"/repos/{OWNER}/{REPO}/pulls") + assert "body" not in pull_request + + +async def test_retry_reuses_the_runs_branch_and_open_pull_request(fake_github): + # An earlier attempt pushed the branch and opened the PR, then failed + # before its result was recorded. + fake = fake_github(branch_exists=True, pull_request_exists=True) + + result = await github_handler.SubmitPatchHandler().apply(_PARAMS, _ctx()) + + assert result.status == "applied", result.error + assert result.result["url"] == PR_URL + repo = f"/repos/{OWNER}/{REPO}" + assert fake.calls("PATCH", f"{repo}/git/refs/heads/hackbot/run-1") == [ + {"sha": "commit6", "force": True} + ] + listing = [r for r in fake.requests if r[:2] == ("GET", f"{repo}/pulls")] + assert len(listing) == 1 + + +async def test_submit_patch_fails_for_a_repository_not_on_github(fake_github): + fake = fake_github() + submission = {**_SUBMISSION, "repo_url": "https://hg.mozilla.org/try"} + + result = await github_handler.SubmitPatchHandler().apply(_PARAMS, _ctx(submission)) + + assert result.status == "failed" + assert "not on GitHub" in result.error + assert fake.requests == [] + + +async def test_submit_patch_fails_without_the_artifact(fake_github): + fake = fake_github() + + result = await github_handler.SubmitPatchHandler().apply(_PARAMS, _ctx(None)) + + assert result.status == "failed" + assert "GitHub commits artifact" in result.error + assert fake.requests == [] + + +async def test_submit_patch_fails_when_github_rejects_a_call(fake_github): + fake = fake_github() + submission = {**_SUBMISSION, "base_commit": "unknown"} + + result = await github_handler.SubmitPatchHandler().apply(_PARAMS, _ctx(submission)) + + assert result.status == "failed" + # Nothing is pushed on top of a base GitHub doesn't have. + assert not fake.calls("POST", f"/repos/{OWNER}/{REPO}/git/refs") + + +async def test_submit_patch_fails_when_the_github_app_is_not_configured( + monkeypatch, +): + monkeypatch.setattr(github_handler.settings, "github", None) + github_handler._github_app.cache_clear() + + result = await github_handler.SubmitPatchHandler().apply(_PARAMS, _ctx()) + + github_handler._github_app.cache_clear() + assert result.status == "failed" + assert "not configured" in result.error diff --git a/services/hackbot-ui/components/RunDetail.tsx b/services/hackbot-ui/components/RunDetail.tsx index f8b613beea4..6cc1cc5ff2c 100644 --- a/services/hackbot-ui/components/RunDetail.tsx +++ b/services/hackbot-ui/components/RunDetail.tsx @@ -22,7 +22,8 @@ import { parseTestPlan, TestPlanView } from "./TestPlanView"; // approves the actual text and not just an action type. A comment carries its // body in params.text; a Phabricator submission carries the title and summary of // the revision it would open for the patch (previewed by PatchView), and an -// update carries them only when it rewords the existing revision's. +// update carries them only when it rewords the existing revision's. A GitHub +// submission carries the title and body of the pull request it would open. function actionPreview( action: RunAction ): { label: string; text: string } | null { @@ -41,6 +42,12 @@ function actionPreview( .join("\n\n"); return body ? { label: "Revision preview", text: body } : null; } + if (action.type === "github.submit_patch") { + const body = [text(action.params?.title), text(action.params?.body)] + .filter(Boolean) + .join("\n\n"); + return body ? { label: "Pull request preview", text: body } : null; + } return null; } From a86edabd14013da40abe48ecee71149abcf524af Mon Sep 17 00:00:00 2001 From: Orla Date: Thu, 12 Nov 2026 16:35:44 -0500 Subject: [PATCH 2/2] Pair diff-tree fields strictly when building GitHub commits Drop the trailing NUL explicitly and pair headers with paths using itertools.batched(strict=True), so output that doesn't hold one path per entry raises instead of being silently mispaired. Handle a commit that changes no files, where diff-tree prints nothing. --- .../hackbot_runtime/changes.py | 12 ++++--- libs/hackbot-runtime/tests/test_changes.py | 35 +++++++++++++++++++ 2 files changed, 43 insertions(+), 4 deletions(-) diff --git a/libs/hackbot-runtime/hackbot_runtime/changes.py b/libs/hackbot-runtime/hackbot_runtime/changes.py index e6cd534b461..baba075603a 100644 --- a/libs/hackbot-runtime/hackbot_runtime/changes.py +++ b/libs/hackbot-runtime/hackbot_runtime/changes.py @@ -13,6 +13,7 @@ import base64 import contextlib +import itertools import logging import os import re @@ -351,11 +352,14 @@ def _github_file_changes(repo: Path, parent: str, commit: str) -> list[dict]: repo, "diff-tree", "-r", "-z", "--no-renames", "--no-commit-id", parent, commit ) # -z output is ": \0\0" per - # entry, with paths unquoted. GitHub only takes UTF-8 paths, so any other - # path raises here rather than being pushed mangled. - fields = out.split(b"\0") + # entry, with paths unquoted. --no-renames keeps it to one path per entry (a + # rename or copy would carry two), and strict pairing raises if that ever + # stops holding. A commit that changes no files prints nothing at all. GitHub + # only takes UTF-8 paths, so any other path raises here rather than being + # pushed mangled. + fields = out.removesuffix(b"\0").split(b"\0") if out else [] entries = [] - for header, raw_path in zip(fields[::2], fields[1::2]): + for header, raw_path in itertools.batched(fields, 2, strict=True): src_mode, dst_mode, _src_sha, dst_sha, status = header.lstrip(b":").split() path = raw_path.decode("utf-8") if status == b"D": diff --git a/libs/hackbot-runtime/tests/test_changes.py b/libs/hackbot-runtime/tests/test_changes.py index 8f08be8893d..63b5085edbc 100644 --- a/libs/hackbot-runtime/tests/test_changes.py +++ b/libs/hackbot-runtime/tests/test_changes.py @@ -292,6 +292,41 @@ def test_build_github_commits_reports_a_rename_as_delete_and_add(tmp_path): ) +def test_build_github_commits_pairs_each_path_with_its_own_entry(tmp_path): + # Paths with spaces, next to plain ones, land on the right header: each + # path gets its own content and mode, not a neighbour's. + base = _init_repo(tmp_path) + (tmp_path / "a file.txt").write_text("spaced\n") + (tmp_path / "plain.txt").write_text("plain\n") + nested = tmp_path / "some dir" / "two spaces.sh" + nested.parent.mkdir() + nested.write_text("#!/bin/sh\n") + nested.chmod(0o755) + + payload = build_github_commits(tmp_path, base, _REPO_URL) + + (commit,) = payload["commits"] + assert [ + (c["path"], c["mode"], base64.b64decode(c["content"])) + for c in commit["changes"] + ] == [ + ("a file.txt", "100644", b"spaced\n"), + ("plain.txt", "100644", b"plain\n"), + ("some dir/two spaces.sh", "100755", b"#!/bin/sh\n"), + ] + + +def test_build_github_commits_keeps_a_commit_that_changes_no_files(tmp_path): + base = _init_repo(tmp_path) + _git(tmp_path, "commit", "-q", "--allow-empty", "-m", "Empty") + + payload = build_github_commits(tmp_path, base, _REPO_URL) + + (commit,) = payload["commits"] + assert commit["message"] == "Empty" + assert commit["changes"] == [] + + def test_build_github_commits_no_changes_returns_none(tmp_path): base = _init_repo(tmp_path)