diff --git a/docs/hackbot/actions.md b/docs/hackbot/actions.md index 98ca9f12f38..1299d763f90 100644 --- a/docs/hackbot/actions.md +++ b/docs/hackbot/actions.md @@ -35,20 +35,22 @@ record in its `config.py` and passes them to `actions_server_for`, which builds carrying only those. `bug-fix`, for instance, allows `phabricator.submit_patch` on a fresh triage run but swaps it for `phabricator.update_patch` on a follow-up. -| Action type | Records the intent to… | Params | -| --------------------------- | ------------------------------------------ | --------------------------------------------------------------- | -| `bugzilla.update_bug` | Change a bug's fields | `bug_id`, `changes` | -| `bugzilla.add_comment` | Comment on a bug | `bug_id`, `text`, `is_private` | -| `bugzilla.add_attachment` | Attach a file to a bug | `bug_id`, + a `file` attachment | -| `bugzilla.create_bug` | File a new bug | the new bug's fields, `ref` | -| `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` | -| `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` | -| `feedback.report_problem` | Report a problem to the Hackbot developers | `severity`, `title`, `message` | -| `agents.start_run` | Start another agent on this run's output | `agent` | +| Action type | Records the intent to… | Params | +| ---------------------------- | ------------------------------------------ | --------------------------------------------------------------- | +| `bugzilla.update_bug` | Change a bug's fields | `bug_id`, `changes` | +| `bugzilla.add_comment` | Comment on a bug | `bug_id`, `text`, `is_private` | +| `bugzilla.add_attachment` | Attach a file to a bug | `bug_id`, + a `file` attachment | +| `bugzilla.create_bug` | File a new bug | the new bug's fields, `ref` | +| `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.add_comment` | Comment on a pull request conversation | `repo`, `pull_number`, `text` | +| `github.add_inline_comments` | Review a pull request with inline comments | `repo`, `pull_number`, `comments`, `summary` | +| `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` | +| `feedback.report_problem` | Report a problem to the Hackbot developers | `severity`, `title`, `message` | +| `agents.start_run` | Start another agent on this run's output | `agent` | All but `testrail.submit_test_plan` take a **`reasoning`** argument — a free-text audit trail stored on the action and shown in the UI beside the proposed change. Each tool returns the @@ -165,6 +167,18 @@ applied at its last member's index, once every earlier dependency has resolved, group with a member another pending action references is excluded (that action could run before the group is applied). +## GitHub review grouping + +When general and inline comments for the same GitHub PR are pending in the same +apply pass, they are submitted together as one review. General comment text and +existing review summaries are joined with blank lines in action order. General +comments without inline comments still post to the PR conversation. + +Only actions selected for that apply pass are grouped. Already-applied actions +are skipped, and groups containing an action referenced by another pending +action are excluded. Each grouped action retains its own status; general +comments return the review URL as their `comment_url`. + ## Where to look | Concern | File | 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..98ac50c163f --- /dev/null +++ b/libs/hackbot-runtime/hackbot_runtime/actions/github.py @@ -0,0 +1,153 @@ +"""GitHub-domain recordable actions. + +Mirrors ``actions/phabricator.py``'s shape: the tools record an intended comment +(nothing is posted to GitHub here) and return a short confirmation string. See +hackbot-api's ``app/action_handlers/github_handler.py`` for the apply side. +""" + +from typing import Annotated, Any + +from agent_tools.registry import ToolError, tool, tools_in +from pydantic import ( + BaseModel, + Field, + StringConstraints, + ValidationError, + model_validator, +) + +from hackbot_runtime.actions.recorder import ActionsRecorder, confirmation + +ADD_COMMENT_ACTION_TYPE = "github.add_comment" +ADD_INLINE_COMMENTS_ACTION_TYPE = "github.add_inline_comments" + +_Text = Annotated[str, StringConstraints(strip_whitespace=True, min_length=1)] + + +class InlineComment(BaseModel): + path: Annotated[ + _Text, Field(description="Path of the file in the repository, e.g. src/a.py.") + ] + line: Annotated[ + int, + Field( + ge=1, + description=( + "Line in the new version of the file. It must appear in the pull " + "request's diff, as a changed line or the context around one." + ), + ), + ] + body: Annotated[_Text, Field(description="Comment body (Markdown supported).")] + start_line: Annotated[ + int | None, + Field( + ge=1, + description="First line of a multi-line comment ending at `line`.", + ), + ] = None + + @model_validator(mode="after") + def _start_line_before_line(self) -> InlineComment: + if self.start_line is not None and self.start_line >= self.line: + raise ValueError("start_line must be before line") + return self + + +# The tool adapters pass the agent's arguments through unvalidated, so these +# models are the enforcement; the tool signatures below are the agent's schema. +class _PullRequest(BaseModel): + # GitHub's own rules for owner and repository names. + repo: Annotated[str, Field(pattern=r"^[A-Za-z0-9-]+/[A-Za-z0-9._-]+$")] + pull_number: Annotated[int, Field(ge=1)] + + +class _Comment(_PullRequest): + text: _Text + + +class _InlineComments(_PullRequest): + comments: Annotated[list[InlineComment], Field(min_length=1)] + summary: _Text | None = None + + +def _validated(model: type[BaseModel], **values: Any) -> dict[str, Any]: + try: + validated = model.model_validate(values) + except ValidationError as exc: + raise ToolError( + "invalid GitHub action", + payload={ + "error": "invalid GitHub action", + "details": exc.errors(include_url=False), + }, + ) from exc + return validated.model_dump(mode="json", exclude_none=True) + + +_Repo = Annotated[ + str, + Field(description='The repository, as "owner/name" (e.g. mozilla/bugbug).'), +] +_PullNumber = Annotated[int, Field(description="The pull request number.")] +_Reasoning = Annotated[ + str, Field(description="Why you are recording this comment (for audit log).") +] + + +@tool +async def add_comment( + recorder: ActionsRecorder, + repo: _Repo, + pull_number: _PullNumber, + text: Annotated[str, Field(description="Comment body (Markdown supported).")], + reasoning: _Reasoning, +) -> str: + """Record an intended comment on a GitHub pull request's conversation. + + Use this to reply on a pull request, for example to answer a question, or for + feedback that is not about particular lines; for those, use + ``add_inline_comments``. Recorded into the run summary for human review; + nothing is posted to GitHub during the run. + """ + params = _validated(_Comment, repo=repo, pull_number=pull_number, text=text) + action = recorder.record(ADD_COMMENT_ACTION_TYPE, params, reasoning=reasoning) + return confirmation(action) + + +@tool +async def add_inline_comments( + recorder: ActionsRecorder, + repo: _Repo, + pull_number: _PullNumber, + comments: Annotated[ + list[InlineComment], + Field(min_length=1, description="The line comments, at least one."), + ], + reasoning: _Reasoning, + summary: Annotated[ + str | None, + Field(description="Optional overall comment shown above the line comments."), + ] = None, +) -> str: + """Record an intended review of a GitHub pull request: comments on its lines. + + The comments are posted together as one review, so pass every line comment + for the pull request in a single call; each call becomes a separate review. + Recorded into the run summary for human review; nothing is posted to GitHub + during the run. + """ + params = _validated( + _InlineComments, + repo=repo, + pull_number=pull_number, + comments=comments, + summary=summary, + ) + action = recorder.record( + ADD_INLINE_COMMENTS_ACTION_TYPE, params, reasoning=reasoning + ) + return confirmation(action) + + +TOOLS = tools_in(__name__) 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..22dc411cc12 --- /dev/null +++ b/libs/hackbot-runtime/tests/test_github_actions.py @@ -0,0 +1,124 @@ +"""Tests for the GitHub recording tools (PR comments and inline review comments).""" + +import pytest +from agent_tools.registry import ToolError +from hackbot_runtime.actions import ActionsRecorder, github + +REPO = "mozilla-mobile/firefox-ios" + + +async def test_add_comment_records_the_pull_request_and_text(): + rec = ActionsRecorder() + await github.add_comment( + rec, repo=REPO, pull_number=7, text=" Here is the answer. ", reasoning="r" + ) + [action] = rec.actions + assert action["type"] == "github.add_comment" + assert action["params"] == { + "repo": REPO, + "pull_number": 7, + "text": "Here is the answer.", + } + assert action["reasoning"] == "r" + + +async def test_add_inline_comments_records_one_action_for_all_comments(): + rec = ActionsRecorder() + await github.add_inline_comments( + rec, + repo=REPO, + pull_number=7, + comments=[ + {"path": "a.py", "line": 3, "body": "Off by one?"}, + {"path": "b.py", "line": 12, "start_line": 10, "body": "Extract this."}, + ], + summary="Two things.", + reasoning="r", + ) + [action] = rec.actions + assert action["type"] == "github.add_inline_comments" + assert action["params"] == { + "repo": REPO, + "pull_number": 7, + "comments": [ + {"path": "a.py", "line": 3, "body": "Off by one?"}, + {"path": "b.py", "line": 12, "start_line": 10, "body": "Extract this."}, + ], + "summary": "Two things.", + } + + +async def test_add_inline_comments_summary_is_optional(): + rec = ActionsRecorder() + await github.add_inline_comments( + rec, + repo=REPO, + pull_number=7, + comments=[{"path": "a.py", "line": 3, "body": "Nit."}], + reasoning="r", + ) + assert "summary" not in rec.actions[0]["params"] + + +@pytest.mark.parametrize( + ("repo", "pull_number"), + [("firefox-ios", 7), ("mozilla-mobile/firefox-ios/x", 7), (REPO, 0)], + ids=["no-owner", "extra-segment", "pull-number"], +) +async def test_a_malformed_pull_request_is_refused(repo, pull_number): + rec = ActionsRecorder() + with pytest.raises(ToolError): + await github.add_comment( + rec, repo=repo, pull_number=pull_number, text="x", reasoning="r" + ) + with pytest.raises(ToolError): + await github.add_inline_comments( + rec, + repo=repo, + pull_number=pull_number, + comments=[{"path": "a.py", "line": 1, "body": "x"}], + reasoning="r", + ) + assert rec.actions == [] + + +async def test_a_blank_comment_is_refused(): + rec = ActionsRecorder() + with pytest.raises(ToolError): + await github.add_comment(rec, repo=REPO, pull_number=7, text=" ", reasoning="r") + assert rec.actions == [] + + +@pytest.mark.parametrize( + "comments", + [ + [], + [{"path": "a.py", "line": 0, "body": "x"}], + [{"path": "a.py", "line": 3, "body": " "}], + [{"path": "", "line": 3, "body": "x"}], + [{"path": "a.py", "line": 3, "start_line": 3, "body": "x"}], + ], + ids=["none", "line-zero", "blank-body", "blank-path", "start-not-before-end"], +) +async def test_invalid_inline_comments_are_refused(comments): + rec = ActionsRecorder() + with pytest.raises(ToolError) as exc: + await github.add_inline_comments( + rec, repo=REPO, pull_number=7, comments=comments, reasoning="r" + ) + assert exc.value.payload["details"] + assert rec.actions == [] + + +def test_tool_schemas(): + tools = {t.name: t for t in github.TOOLS} + assert set(tools) == {"add_comment", "add_inline_comments"} + assert set(tools["add_comment"].input_schema["required"]) == { + "repo", + "pull_number", + "text", + "reasoning", + } + inline = tools["add_inline_comments"].input_schema + assert set(inline["required"]) == {"repo", "pull_number", "comments", "reasoning"} + assert "summary" in inline["properties"] 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..798536544fd --- /dev/null +++ b/services/hackbot-api/app/action_handlers/github_handler.py @@ -0,0 +1,163 @@ +"""Apply-side GitHub actions: comment on pull requests as the hackbot GitHub App. + +Pairs with the recording side in ``hackbot_runtime/actions/github.py``. Each +action gets an installation token for its own repository only, with nothing +but ``pull_requests: write``: enough to comment on a pull request, not to touch +its code (or to comment on a plain issue, which needs ``issues: write``). +""" + +import logging +from functools import lru_cache +from types import MappingProxyType +from typing import Any + +from githubkit import GitHub +from githubkit.exception import RequestFailed + +from app.action_handlers.base import ActionResult, ApplyContext +from app.config import settings +from app.github import GitHubApp + +log = logging.getLogger(__name__) + +_COMMENT_PERMISSIONS = MappingProxyType({"pull_requests": "write"}) + + +def plan_review_groups(actions: list[tuple[str, dict[str, Any]]]) -> list[list[int]]: + """Group same-PR actions only when both general and inline comments exist.""" + by_pr: dict[tuple[str, int], list[int]] = {} + for idx, (action_type, params) in enumerate(actions): + if action_type in {"github.add_comment", "github.add_inline_comments"}: + repo, number = params.get("repo"), params.get("pull_number") + if not isinstance(repo, str) or not isinstance(number, int): + continue + key = (repo, number) + by_pr.setdefault(key, []).append(idx) + return [ + indices + for indices in by_pr.values() + if len({actions[i][0] for i in indices}) == 2 + ] + + +def merge_review(entries: list[tuple[str, dict[str, Any]]]) -> dict[str, Any]: + """Preserve comment order, using general comments as review summary text.""" + first = entries[0][1] + comments = [] + summaries = [] + for action_type, params in entries: + if action_type == "github.add_comment": + summaries.append(params["text"]) + else: + comments.extend(params["comments"]) + if summary := params.get("summary"): + summaries.append(summary) + return { + "repo": first["repo"], + "pull_number": first["pull_number"], + "comments": comments, + "summary": "\n\n".join(summaries), + } + + +class GitHubAppNotConfiguredError(Exception): + pass + + +@lru_cache(maxsize=1) +def _app() -> GitHubApp: + if settings.github is None: + raise GitHubAppNotConfiguredError( + "The GitHub App is not configured (GITHUB_APP_ID, GITHUB_PRIVATE_KEY)" + ) + return GitHubApp(settings.github) + + +async def _client_for(owner: str, repo: str) -> GitHub: + app = _app() + # githubkit's ``rest`` namespace only holds a weak reference to its client. + github = app.as_app() + installation = await github.rest.apps.async_get_repo_installation(owner, repo) + return app.as_installation( + installation.json()["id"], + permissions=_COMMENT_PERMISSIONS, + repositories=[repo], + ) + + +def _describe(exc: Exception) -> str: + """GitHub's own explanation of a failed request, which ``str()`` leaves out.""" + if not isinstance(exc, RequestFailed): + return str(exc) + response = exc.response + try: + payload = response.json() + except ValueError: + return f"GitHub API error {response.status_code}" + detail = payload.get("message", "") + if errors := payload.get("errors"): + detail += f" ({'; '.join(map(str, errors))})" + return f"GitHub API error {response.status_code}: {detail}" + + +class AddCommentHandler: + async def apply(self, params: dict[str, Any], ctx: ApplyContext) -> ActionResult: + repo, number = params["repo"], params["pull_number"] + owner, name = repo.split("/") + try: + github = await _client_for(owner, name) + # PR conversation comments go through the issues API. + response = await github.rest.issues.async_create_comment( + owner, name, number, body=params["text"] + ) + except Exception as exc: + log.exception("Failed to comment on %s#%s", repo, number) + return ActionResult.failed(_describe(exc)) + return ActionResult.ok( + { + "repo": repo, + "pull_number": number, + "comment_url": response.json()["html_url"], + } + ) + + +class AddInlineCommentsHandler: + async def apply(self, params: dict[str, Any], ctx: ApplyContext) -> ActionResult: + repo, number = params["repo"], params["pull_number"] + owner, name = repo.split("/") + review: dict[str, Any] = { + "event": "COMMENT", + "comments": [_review_comment(c) for c in params["comments"]], + } + if summary := params.get("summary"): + review["body"] = summary + try: + github = await _client_for(owner, name) + response = await github.rest.pulls.async_create_review( + owner, name, number, data=review + ) + except Exception as exc: + log.exception("Failed to review %s#%s", repo, number) + return ActionResult.failed(_describe(exc)) + return ActionResult.ok( + { + "repo": repo, + "pull_number": number, + "review_url": response.json()["html_url"], + } + ) + + +def _review_comment(comment: dict[str, Any]) -> dict[str, Any]: + # Always the new version of the file ("RIGHT"), which is what the agent + # has checked out. + review_comment = { + "path": comment["path"], + "line": comment["line"], + "side": "RIGHT", + "body": comment["body"], + } + if start_line := comment.get("start_line"): + review_comment |= {"start_line": start_line, "start_side": "RIGHT"} + return review_comment diff --git a/services/hackbot-api/app/action_handlers/registry.py b/services/hackbot-api/app/action_handlers/registry.py index a0905eb6752..2342c864ecb 100644 --- a/services/hackbot-api/app/action_handlers/registry.py +++ b/services/hackbot-api/app/action_handlers/registry.py @@ -8,6 +8,10 @@ ) from app.action_handlers.email_handler import SendEmailHandler from app.action_handlers.feedback_handler import ReportProblemHandler +from app.action_handlers.github_handler import ( + AddCommentHandler as GitHubAddCommentHandler, +) +from app.action_handlers.github_handler import AddInlineCommentsHandler from app.action_handlers.phabricator_handler import ( AddCommentHandler as PhabricatorAddCommentHandler, ) @@ -33,6 +37,8 @@ "phabricator.submit_patch": SubmitPatchHandler(), "phabricator.update_patch": UpdatePatchHandler(), "phabricator.add_comment": PhabricatorAddCommentHandler(), + "github.add_comment": GitHubAddCommentHandler(), + "github.add_inline_comments": AddInlineCommentsHandler(), "testrail.submit_test_plan": SubmitTestPlanHandler(), "slack.post_message": PostMessageHandler(), "email.send": SendEmailHandler(), diff --git a/services/hackbot-api/app/actions_applier.py b/services/hackbot-api/app/actions_applier.py index bbfd3495cb9..3c05093f9b3 100644 --- a/services/hackbot-api/app/actions_applier.py +++ b/services/hackbot-api/app/actions_applier.py @@ -28,6 +28,7 @@ merge_resolved, plan_coalesced_groups, ) +from app.action_handlers.github_handler import merge_review, plan_review_groups from app.agents import AGENT_REGISTRY, AgentSpec from app.database.models import Run, RunAction from app.schemas import RunStatus @@ -253,7 +254,8 @@ async def _apply_pending_rows( 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 `app.action_handlers`. + separately. General and inline GitHub comments on the same PR are submitted + together as one review when both are pending in this apply pass. Cross-action `{{actions..}}` placeholders resolve against rows that are already `applied` (seeded from prior applies) plus ones applied @@ -274,7 +276,7 @@ async def _apply_pending_rows( pending = [(row, att) for row, att in rows if row.status != "applied"] - # Plan which pending rows coalesce into one bug PUT (indices into `pending`). + # Plan which pending rows share one API request (indices into `pending`). # Drop any group with a member another pending action references: a group # applies at its last member, possibly after the action that needs the # result. Everything else applies one row at a time as before. @@ -283,11 +285,10 @@ async def _apply_pending_rows( for row, _ in pending for match in _PLACEHOLDER_RE.finditer(json.dumps(row.params)) } + actions = [(row.type, row.params) for row, _ in pending] groups = [ group - for group in plan_coalesced_groups( - [(row.type, row.params) for row, _ in pending] - ) + for group in plan_coalesced_groups(actions) + plan_review_groups(actions) if all(pending[i][0].ref not in referenced for i in group) ] # Rows sit in idx order, so a group's last member is its max idx: apply the @@ -306,13 +307,22 @@ async def _apply_pending_rows( (member.type, resolve_placeholders(member.params, results_by_ref)) for member in member_rows ] - outcome = await _dispatch( - run, - "bugzilla.update_bug", - merge_resolved(entries), - [], - hackbot_client, - ) + if row.type.startswith("github."): + outcome = await _dispatch( + run, + "github.add_inline_comments", + merge_review(entries), + [], + hackbot_client, + ) + else: + outcome = await _dispatch( + run, + "bugzilla.update_bug", + merge_resolved(entries), + [], + hackbot_client, + ) else: member_rows = [row] params = resolve_placeholders(row.params, results_by_ref) @@ -326,6 +336,17 @@ async def _apply_pending_rows( for member in member_rows: member.status = outcome.status member.result = outcome.result + if ( + anchor is not None + and member.type == "github.add_comment" + and outcome.result is not None + and outcome.status == "applied" + ): + member.result = { + "repo": outcome.result["repo"], + "pull_number": outcome.result["pull_number"], + "comment_url": outcome.result["review_url"], + } member.error = outcome.error if applied_at is not None: member.applied_at = applied_at @@ -334,7 +355,7 @@ async def _apply_pending_rows( 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 + results_by_ref[member.ref] = member.result async def on_run_completed( diff --git a/services/hackbot-api/tests/test_actions_applier.py b/services/hackbot-api/tests/test_actions_applier.py index ee7a8bd96ee..8cbb3374388 100644 --- a/services/hackbot-api/tests/test_actions_applier.py +++ b/services/hackbot-api/tests/test_actions_applier.py @@ -11,6 +11,7 @@ from datetime import datetime, timezone from types import SimpleNamespace +import pytest from app import actions_applier from app.actions_applier import ( apply_all_pending, @@ -24,6 +25,216 @@ _CLIENT = object() +def _github_comment_rows(): + return [ + ( + _row( + 0, + "pending", + action_type="github.add_comment", + params={"repo": "o/r", "pull_number": 7, "text": "Overall feedback."}, + ref="general", + ), + [], + ), + ( + _row( + 1, + "pending", + action_type="github.add_inline_comments", + params={ + "repo": "o/r", + "pull_number": 7, + "comments": [{"path": "a.py", "line": 3, "body": "Check this."}], + "summary": "Review summary.", + }, + ref="inline", + ), + [], + ), + ] + + +async def test_github_review_keeps_multiple_summaries_and_inline_lists(monkeypatch): + handler = _RecordingHandler( + SimpleNamespace( + status="applied", + result={ + "repo": "o/r", + "pull_number": 7, + "review_url": "https://example.com/review", + }, + error=None, + ) + ) + monkeypatch.setattr(actions_applier, "get_handler", lambda t: handler) + general, inline = _github_comment_rows() + extra = _row( + 2, + "pending", + action_type="github.add_inline_comments", + params={ + "repo": "o/r", + "pull_number": 7, + "comments": [{"path": "b.py", "line": 5, "body": "Another finding."}], + "summary": "More feedback.", + }, + ) + inline[0].idx, extra.idx, general[0].idx = 0, 1, 2 + await actions_applier._apply_pending_rows( + _FakeDB(), + _FakeRun(status=RunStatus.succeeded.value), + [inline, (extra, []), general], + None, + ) + assert len(handler.calls) == 1 + assert ( + handler.calls[0]["summary"] + == "Review summary.\n\nMore feedback.\n\nOverall feedback." + ) + assert ( + handler.calls[0]["comments"] + == inline[0].params["comments"] + extra.params["comments"] + ) + + +async def test_github_general_comments_without_inline_comments_stay_separate( + monkeypatch, +): + handler = _RecordingHandler( + SimpleNamespace(status="applied", result={}, error=None) + ) + monkeypatch.setattr(actions_applier, "get_handler", lambda t: handler) + general, _ = _github_comment_rows() + another = _row( + 1, + "pending", + action_type="github.add_comment", + params={"repo": "o/r", "pull_number": 7, "text": "Another reply."}, + ) + await actions_applier._apply_pending_rows( + _FakeDB(), + _FakeRun(status=RunStatus.succeeded.value), + [general, (another, [])], + None, + ) + assert handler.calls == [general[0].params, another.params] + + +async def test_github_comments_post_as_one_review_and_keep_result_urls(monkeypatch): + result = { + "repo": "o/r", + "pull_number": 7, + "review_url": "https://example.com/review", + } + handler = _RecordingHandler( + SimpleNamespace(status="applied", result=result, error=None) + ) + dispatched = [] + + def get_handler(action_type): + dispatched.append(action_type) + return handler + + monkeypatch.setattr(actions_applier, "get_handler", get_handler) + rows = _github_comment_rows() + db = _FakeDB() + run = _FakeRun(status=RunStatus.succeeded.value) + await actions_applier._apply_pending_rows(db, run, rows, None) + assert dispatched == ["github.add_inline_comments"] + assert handler.calls == [ + { + "repo": "o/r", + "pull_number": 7, + "comments": [{"path": "a.py", "line": 3, "body": "Check this."}], + "summary": "Overall feedback.\n\nReview summary.", + } + ] + assert all(row.status == "applied" for row, _ in rows) + assert rows[0][0].result == { + "repo": "o/r", + "pull_number": 7, + "comment_url": result["review_url"], + } + assert rows[1][0].result == result + assert db.commits == 2 # claim, then apply the combined review + await actions_applier._apply_pending_rows(db, run, rows, None) + assert len(handler.calls) == 1 + + +@pytest.mark.parametrize("separation", ["repo", "pull_number", "applied", "dependent"]) +async def test_github_comments_stay_separate_when_needed(monkeypatch, separation): + handler = _RecordingHandler( + SimpleNamespace(status="applied", result={}, error=None) + ) + monkeypatch.setattr(actions_applier, "get_handler", lambda t: handler) + rows = _github_comment_rows() + if separation == "repo": + rows[1][0].params["repo"] = "o/another" + elif separation == "pull_number": + rows[1][0].params["pull_number"] = 8 + elif separation == "applied": + rows[1][0].status = "applied" + else: + rows[1][0].params["summary"] = "See {{actions.general.comment_url}}" + await actions_applier._apply_pending_rows( + _FakeDB(), _FakeRun(status=RunStatus.succeeded.value), rows, None + ) + assert handler.calls[0] == rows[0][0].params + assert len(handler.calls) == (1 if separation == "applied" else 2) + + +async def test_failed_github_review_retries_both_actions(monkeypatch): + handler = _RecordingHandler( + SimpleNamespace(status="failed", result=None, error="rejected") + ) + monkeypatch.setattr(actions_applier, "get_handler", lambda t: handler) + rows = _github_comment_rows() + run = _FakeRun(status=RunStatus.succeeded.value) + await actions_applier._apply_pending_rows(_FakeDB(), run, rows, None) + assert len(handler.calls) == 1 + assert all(row.status == "failed" and row.applied_at is None for row, _ in rows) + handler.outcome = SimpleNamespace( + status="applied", + result={ + "repo": "o/r", + "pull_number": 7, + "review_url": "https://example.com/review", + }, + error=None, + ) + await actions_applier._apply_pending_rows(_FakeDB(), run, rows, None) + assert len(handler.calls) == 2 + assert all(row.status == "applied" and row.error is None for row, _ in rows) + + +@pytest.mark.parametrize( + "held_type", ["github.add_comment", "github.add_inline_comments"] +) +async def test_github_review_grouping_respects_auto_apply_policy( + monkeypatch, held_type +): + rows = _github_comment_rows() + + async def ensure(db, run): + return rows + + monkeypatch.setattr(actions_applier, "ensure_action_rows", ensure) + monkeypatch.setattr( + actions_applier, + "AGENT_REGISTRY", + {"bug-fix": _spec(never=frozenset({held_type}))}, + ) + handler = _RecordingHandler( + SimpleNamespace(status="applied", result={}, error=None) + ) + monkeypatch.setattr(actions_applier, "get_handler", lambda t: handler) + await on_run_completed(_FakeDB(), _FakeRun(status=RunStatus.succeeded.value), None) + assert len(handler.calls) == 1 + for row, _ in rows: + assert row.status == ("pending" if row.type == held_type else "applied") + + def test_resolves_known_ref_and_field(): out = resolve_placeholders( "Fix submitted: {{actions.patch.url}}", 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..eeb1c3ca03b --- /dev/null +++ b/services/hackbot-api/tests/test_github_handler.py @@ -0,0 +1,284 @@ +"""Tests for the GitHub action handlers (PR comments and inline reviews).""" + +import json +from datetime import UTC, datetime, timedelta +from functools import partial +from types import SimpleNamespace +from unittest.mock import AsyncMock + +import httpx +import pytest +from app.action_handlers import github_handler +from app.action_handlers.base import ApplyContext +from app.action_handlers.registry import get_handler +from app.actions_applier import _apply_pending_rows +from app.config import GitHubAppSettings +from app.github import GitHubApp +from cryptography.hazmat.primitives import serialization +from cryptography.hazmat.primitives.asymmetric import rsa + +INSTALLATION_ID = 99 +CTX = ApplyContext( + run_id="run-1", agent="test-agent", download_artifact=None, hackbot_client=None +) + + +async def test_general_and_inline_actions_send_one_github_review(fake): + rows = [ + ( + SimpleNamespace( + id=1, + idx=0, + type="github.add_comment", + status="pending", + ref=None, + params={"repo": "o/r", "pull_number": 7, "text": "Overall feedback."}, + ), + [], + ), + ( + SimpleNamespace( + id=2, + idx=1, + type="github.add_inline_comments", + status="pending", + ref=None, + params={ + "repo": "o/r", + "pull_number": 7, + "comments": [{"path": "a.py", "line": 3, "body": "Check this."}], + "summary": "One thing.", + }, + ), + [], + ), + ] + db = AsyncMock() + db.execute.return_value = SimpleNamespace(scalars=lambda: [1, 2]) + await _apply_pending_rows( + db, SimpleNamespace(run_id="run-1", agent="test-agent"), rows, None + ) + posts = [ + r + for r in fake.requests + if r.method == "POST" and r.url.path.startswith("/repos/") + ] + assert len(posts) == 1 + assert posts[0].url.path == "/repos/o/r/pulls/7/reviews" + assert json.loads(posts[0].content) == { + "event": "COMMENT", + "body": "Overall feedback.\n\nOne thing.", + "comments": [ + {"path": "a.py", "line": 3, "side": "RIGHT", "body": "Check this."} + ], + } + assert all(row.status == "applied" for row, _ in rows) + + +@pytest.fixture(scope="module") +def pem() -> str: + return ( + rsa.generate_private_key(public_exponent=65537, key_size=2048) + .private_bytes( + serialization.Encoding.PEM, + serialization.PrivateFormat.TraditionalOpenSSL, + serialization.NoEncryption(), + ) + .decode() + ) + + +class _FakeGitHub: + """Stands in for api.github.com; ``fail`` maps a path to an error response.""" + + def __init__(self, fail: dict[str, httpx.Response] | None = None): + self.fail = fail or {} + self.requests: list[httpx.Request] = [] + + def request_to(self, path: str) -> httpx.Request: + return next(r for r in self.requests if r.url.path == path) + + def __call__(self, request: httpx.Request) -> httpx.Response: + self.requests.append(request) + path = request.url.path + if path in self.fail: + return self.fail[path] + if path.endswith("/installation"): + return httpx.Response(200, json={"id": INSTALLATION_ID}) + 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"), + }, + ) + if path.endswith("/comments"): + return httpx.Response( + 201, json={"html_url": "https://github.com/o/r/pull/7#issuecomment-1"} + ) + if path.endswith("/reviews"): + return httpx.Response( + 200, + json={"html_url": "https://github.com/o/r/pull/7#pullrequestreview-1"}, + ) + return httpx.Response(404, json={"message": "Not Found"}) + + +@pytest.fixture +def fake(monkeypatch, pem): + fake = _FakeGitHub() + monkeypatch.setattr( + github_handler.settings, + "github", + GitHubAppSettings(app_id=123, private_key=pem, webhook_secret="test-secret"), + ) + monkeypatch.setattr( + github_handler, + "GitHubApp", + partial(GitHubApp, async_transport=httpx.MockTransport(fake), http_cache=False), + ) + github_handler._app.cache_clear() + try: + yield fake + finally: + github_handler._app.cache_clear() + + +async def test_add_comment_posts_with_a_comment_only_token(fake): + result = await get_handler("github.add_comment").apply( + {"repo": "o/r", "pull_number": 7, "text": "Answering your question."}, CTX + ) + + assert result.status == "applied" + assert result.result == { + "repo": "o/r", + "pull_number": 7, + "comment_url": "https://github.com/o/r/pull/7#issuecomment-1", + } + lookup = fake.request_to("/repos/o/r/installation") + assert lookup.headers["Authorization"].startswith("Bearer ") + mint = fake.request_to(f"/app/installations/{INSTALLATION_ID}/access_tokens") + assert json.loads(mint.content) == { + "permissions": {"pull_requests": "write"}, + "repositories": ["r"], + } + comment = fake.request_to("/repos/o/r/issues/7/comments") + assert comment.headers["Authorization"] == "token ghs_test" + assert json.loads(comment.content) == {"body": "Answering your question."} + + +async def test_add_inline_comments_posts_one_review(fake): + result = await get_handler("github.add_inline_comments").apply( + { + "repo": "o/r", + "pull_number": 7, + "comments": [ + {"path": "a.py", "line": 3, "body": "Off by one?"}, + {"path": "b.py", "line": 12, "start_line": 10, "body": "Extract."}, + ], + "summary": "Two things.", + }, + CTX, + ) + + assert result.status == "applied" + assert result.result["review_url"].endswith("#pullrequestreview-1") + review = fake.request_to("/repos/o/r/pulls/7/reviews") + assert json.loads(review.content) == { + "event": "COMMENT", + "body": "Two things.", + "comments": [ + {"path": "a.py", "line": 3, "side": "RIGHT", "body": "Off by one?"}, + { + "path": "b.py", + "line": 12, + "side": "RIGHT", + "start_line": 10, + "start_side": "RIGHT", + "body": "Extract.", + }, + ], + } + + +async def test_add_inline_comments_without_a_summary_has_no_review_body(fake): + await github_handler.AddInlineCommentsHandler().apply( + { + "repo": "o/r", + "pull_number": 7, + "comments": [{"path": "a.py", "line": 3, "body": "Nit."}], + }, + CTX, + ) + review = fake.request_to("/repos/o/r/pulls/7/reviews") + assert "body" not in json.loads(review.content) + + +async def test_github_rejecting_a_review_fails_with_its_reason(fake): + fake.fail["/repos/o/r/pulls/7/reviews"] = httpx.Response( + 422, + json={ + "message": "Unprocessable Entity", + "errors": ["Line could not be resolved"], + }, + ) + + result = await github_handler.AddInlineCommentsHandler().apply( + { + "repo": "o/r", + "pull_number": 7, + "comments": [{"path": "a.py", "line": 50, "body": "x"}], + }, + CTX, + ) + + assert result.status == "failed" + assert result.error == ( + "GitHub API error 422: Unprocessable Entity (Line could not be resolved)" + ) + + +async def test_a_repo_without_the_app_installed_fails(fake): + fake.fail["/repos/o/r/installation"] = httpx.Response( + 404, json={"message": "Not Found"} + ) + + result = await github_handler.AddCommentHandler().apply( + {"repo": "o/r", "pull_number": 7, "text": "x"}, CTX + ) + + assert result.status == "failed" + assert result.error == "GitHub API error 404: Not Found" + assert not any(r.url.path.endswith("/comments") for r in fake.requests) + + +@pytest.mark.parametrize( + ("action_type", "params"), + [ + ("github.add_comment", {"repo": "o/r", "pull_number": 7, "text": "x"}), + ( + "github.add_inline_comments", + { + "repo": "o/r", + "pull_number": 7, + "comments": [{"path": "a.py", "line": 3, "body": "x"}], + }, + ), + ], +) +async def test_an_unconfigured_app_fails_without_breaking_the_service( + monkeypatch, action_type, params +): + monkeypatch.setattr(github_handler.settings, "github", None) + github_handler._app.cache_clear() + try: + result = await get_handler(action_type).apply(params, CTX) + finally: + github_handler._app.cache_clear() + + assert result.status == "failed" + assert result.error.startswith("The GitHub App is not configured") + assert "GITHUB_APP_ID" in result.error + assert "GITHUB_PRIVATE_KEY" in result.error