From 2bb8e381d045a621582ae1ab2b540505805e196d Mon Sep 17 00:00:00 2001 From: Evgeny Pavlov Date: Wed, 9 Sep 2026 15:07:12 -0700 Subject: [PATCH] feat(build-repair): Submit the fix for review on Phabricator --- agents/build-repair/README.md | 28 ++- .../hackbot_agents/build_repair/__main__.py | 7 + .../hackbot_agents/build_repair/agent.py | 127 ++++++++-- .../hackbot_agents/build_repair/config.py | 7 + .../hackbot_agents/build_repair/notify.py | 17 +- .../hackbot_agents/build_repair/prompts.py | 10 +- .../hackbot_agents/build_repair/resolve.py | 37 ++- agents/build-repair/pyproject.toml | 2 +- agents/build-repair/tests/test_notify.py | 15 ++ agents/build-repair/tests/test_reporting.py | 238 ++++++++++++++++++ services/hackbot-api/app/agents.py | 5 +- .../hackbot-api/tests/test_actions_applier.py | 17 +- uv.lock | 4 +- 13 files changed, 471 insertions(+), 43 deletions(-) create mode 100644 agents/build-repair/tests/test_reporting.py diff --git a/agents/build-repair/README.md b/agents/build-repair/README.md index bc228c9071..cfb81ed2fb 100644 --- a/agents/build-repair/README.md +++ b/agents/build-repair/README.md @@ -18,7 +18,11 @@ It also optionally bootstraps Firefox build if needed. reaches `CHECKOUT_DEPTH` commits back so the agent can find a culprit in an earlier push when the failing job did not run there. - `GIT_COMMIT` - Optional override for the failure commit (skips the hg->git lookup). -- `BUG_ID` - Optional Bugzilla bug id. +- `BUG_ID` - Optional override, normally unset. The bug is resolved from the push: the + pushlog lookup already returns each changeset's description, whose first line names it + (`Bug 123 - ...`). The failure commit's bug gives the analysis stage its Bugzilla + context; the fix is filed against the _blamed_ commit's bug, known once stage 1 picks + the culprit. ## Output @@ -38,11 +42,29 @@ Second stage - fixing: The result reports `blamed_commit` so the caller can attribute the failure to a developer. +## Submitting the fix + +Once a bug is known, the fix stage records a `phabricator.submit_patch` action in +`summary.json` -- a new WIP revision carrying the fix, whose diff the runtime builds +from the agent's own checkout into `changes/phabricator_diff.json`. Nothing is posted +to the bug, and nothing reaches Phabricator during the run. + +A developer reviews the patch (`changes/changes.patch`) in the Hackbot UI and applies +the action from there; the full review then happens on the revision. Unlike the email +below, this one waits for a human -- only `email.send` auto-applies, see +[`agents.py`](../../services/hackbot-api/app/agents.py). Eval runs pass no actions +recorder, so they never record it. + +A run whose blamed commit names no bug (a "No bug" commit, a backout) produces the fix +but records no revision -- one has to be filed against a bug. The agent log says so +when that happens. + ## Email notification A run that produced a patch records an `email.send` action carrying the analysis, the -blamed commit and the patch, addressed to that commit's author and to the developer who -pushed the failing change (the hackbot team is copied apply-side). A run that proposed no +blamed commit, the patch and -- when a revision is pending -- the steps to review and +apply it, addressed to that commit's author and to the developer who pushed the failing +change (the hackbot team is copied apply-side). A run that proposed no patch is a transient or not-to-blame failure and is not emailed -- see `NOTIFY_ONLY_WITH_PATCH` in [config.py](hackbot_agents/build_repair/config.py). diff --git a/agents/build-repair/hackbot_agents/build_repair/__main__.py b/agents/build-repair/hackbot_agents/build_repair/__main__.py index 6fd11e2584..59e03f8a8c 100644 --- a/agents/build-repair/hackbot_agents/build_repair/__main__.py +++ b/agents/build-repair/hackbot_agents/build_repair/__main__.py @@ -2,6 +2,7 @@ from hackbot_runtime import HackbotContext, run_async from hackbot_runtime.actions.email import record_email +from hackbot_runtime.actions.phabricator import PATCH_ACTION_TYPES from pydantic_settings import BaseSettings, SettingsConfigDict from .agent import BuildRepairResult, run_build_repair @@ -56,6 +57,7 @@ async def main(ctx: HackbotContext) -> BuildRepairResult: source_repo=ctx.repo_path, fx_ctx=ctx.firefox, bug_id=inputs.bug_id, + commit_bugs=push.commit_bugs, git_commits=git_commits, project=push.project, hg_revision=push.hg_revision, @@ -66,6 +68,7 @@ async def main(ctx: HackbotContext) -> BuildRepairResult: log=ctx.log_path, verbose=True, publish_file=ctx.publish_file, + actions_recorder=ctx.actions, ) try: @@ -85,12 +88,16 @@ def _record_analysis_email( return blamed_author = resolve_author_email(ctx.repo_path, result.blamed_commit) + revision_pending = any( + action["type"] in PATCH_ACTION_TYPES for action in ctx.actions.actions + ) subject, body = build_email( result, push, task_id=task_id, run_id=ctx.run_id, has_patch=has_patch, + revision_pending=revision_pending, blamed_author=blamed_author, ) record_email( diff --git a/agents/build-repair/hackbot_agents/build_repair/agent.py b/agents/build-repair/hackbot_agents/build_repair/agent.py index ec03492628..bcf3d6621b 100644 --- a/agents/build-repair/hackbot_agents/build_repair/agent.py +++ b/agents/build-repair/hackbot_agents/build_repair/agent.py @@ -16,6 +16,7 @@ import json import re +import subprocess import sys import tempfile from collections.abc import Callable @@ -35,7 +36,9 @@ UserMessage, ) from hackbot_agents.build_repair.try_push import TRY_TOOLS -from hackbot_runtime import AgentError, HackbotAgentResult +from hackbot_runtime import ActionsRecorder, AgentError, HackbotAgentResult +from hackbot_runtime.actions import ACTIONS_SERVER_NAME +from hackbot_runtime.actions.claude_sdk import actions_server_for, actions_to_tool_names from hackbot_runtime.claude import Reporter from .config import ( @@ -45,6 +48,7 @@ BUGZILLA_READ_TOOLS, BUILD_TOOL, CHECKOUT_DEPTH, + ENABLED_ACTION_TYPES, FIREFOX_TOOLS, FIX_MODEL, TRY_PUSH_TOOL, @@ -58,12 +62,13 @@ FIX_TEMPLATE, PUSH_COMMIT_LINE, PUSH_CONTEXT, + REPORT_INSTRUCTIONS, SINGLE_COMMIT_CONTEXT, TREEHERDER_STEP, TREEHERDER_STEP_NO_PUSH, TRY_PUSH_INSTRUCTIONS, ) -from .resolve import task_push +from .resolve import _bug_from_desc, task_push TARGET_SOFTWARE = "Mozilla Firefox" @@ -148,6 +153,7 @@ async def run_build_repair( source_repo: Path, fx_ctx: FirefoxContext, bug_id: int | None = None, + commit_bugs: dict[str, int] | None = None, git_commits: list[str], project: str | None = None, hg_revision: str | None = None, @@ -158,19 +164,40 @@ async def run_build_repair( verbose: bool = False, log: Path | None = None, publish_file: Callable[[str, Path, str | None], str] | None = None, + actions_recorder: ActionsRecorder | None = None, ) -> BuildRepairResult: """Analyze a build failure and implement a fix in ``source_repo``. Returns a :class:`BuildRepairResult`; raises :class:`AgentError` if a stage ends in an error or produces no result. + + ``commit_bugs`` maps each push commit to the bug it landed for (from the + pushlog, see ``resolve_push``). A run is never told which bug a failure + belongs to, so that mapping is what an unset ``bug_id`` falls back to. + + Pass ``actions_recorder`` to let the fix stage submit the fix for review as a + Phabricator revision, recorded as a proposed action rather than submitted + here. Runs without a recorder, or with no bug for the blamed commit, only + produce the fix in the source tree. """ if not git_commits: raise AgentError("git_commits must contain at least one commit") failure_commit = git_commits[0] - label = f"bug {bug_id}" if bug_id is not None else f"commit {failure_commit[:12]}" + commit_bugs = commit_bugs or {} + # The failure commit's bug gives stage 1 its Bugzilla context. Which bug the + # *fix* is filed against is only settled once stage 1 picks the culprit out + # of the push, so it is resolved again below. + push_bug_id = bug_id if bug_id is not None else commit_bugs.get(failure_commit) + label = ( + f"bug {push_bug_id}" + if push_bug_id is not None + else f"commit {failure_commit[:12]}" + ) print(f"[build_repair] repairing {label} at {failure_commit}", file=sys.stderr) - scratch_dir = Path(tempfile.mkdtemp(prefix=f"build-repair-{bug_id or 'nobug'}-")) + scratch_dir = Path( + tempfile.mkdtemp(prefix=f"build-repair-{push_bug_id or 'nobug'}-") + ) scratch_out = scratch_dir / "out" scratch_out.mkdir(parents=True, exist_ok=True) @@ -213,19 +240,16 @@ async def run_build_repair( treeherder_step=treeherder_step, blame_step=_blame_step(git_commits, scratch_out), scratch_out=scratch_out, - bug_context=BUG_CONTEXT.format(bug_id=bug_id) if bug_id is not None else "", - bug_step=BUG_ANALYSIS_STEP.format(bug_id=bug_id) if bug_id is not None else "", - logs_num=3 if bug_id is not None else 2, - ) - fix_prompt = FIX_TEMPLATE.format( - target_software=TARGET_SOFTWARE, - source_repo=source_repo, - scratch_out=scratch_out, - try_push=( - TRY_PUSH_INSTRUCTIONS.format(task_name=task_name) if run_try_push else "" + bug_context=( + BUG_CONTEXT.format(bug_id=push_bug_id) if push_bug_id is not None else "" ), + bug_step=( + BUG_ANALYSIS_STEP.format(bug_id=push_bug_id) + if push_bug_id is not None + else "" + ), + logs_num=3 if push_bug_id is not None else 2, ) - total_cost = 0.0 total_turns = 0 # Last JSON result of each tracked tool, keyed by tool name. Lets us report @@ -253,17 +277,67 @@ async def run_build_repair( total_cost += result_msg.total_cost_usd or 0.0 total_turns += result_msg.num_turns or 0 + # Which bug the fix belongs to is only settled now: when the push has + # several commits, stage 1 is what picks the culprit, and it is the + # culprit's bug a revision has to be filed against. + blamed_commit = _resolve_blame(scratch_out, git_commits) + resolved_bug_id = bug_id + if resolved_bug_id is None and blamed_commit is not None: + # A culprit from an earlier push is not in the pushlog mapping, but + # the checkout reaches CHECKOUT_DEPTH commits back, so its subject + # still names the bug. + resolved_bug_id = commit_bugs.get(blamed_commit) or _bug_from_commit( + source_repo, blamed_commit + ) + + # Reporting is confined to the fix stage: the analysis stage must not + # submit anything before there is a verified fix. + report = actions_recorder is not None and resolved_bug_id is not None + fix_mcp_servers = mcp_servers + fix_allowed_tools = allowed_tools + if report: + _, actions_server = actions_server_for( + actions_recorder, types=ENABLED_ACTION_TYPES + ) + fix_mcp_servers = {**mcp_servers, ACTIONS_SERVER_NAME: actions_server} + fix_allowed_tools = [ + *allowed_tools, + *actions_to_tool_names(ENABLED_ACTION_TYPES), + ] + elif actions_recorder is not None: + print( + f"[build_repair] no bug for blamed commit {blamed_commit}: the fix " + "will be produced but not submitted for review, since a " + "Phabricator revision is filed against a bug", + file=sys.stderr, + ) + + fix_prompt = FIX_TEMPLATE.format( + target_software=TARGET_SOFTWARE, + source_repo=source_repo, + scratch_out=scratch_out, + try_push=( + TRY_PUSH_INSTRUCTIONS.format(task_name=task_name) + if run_try_push + else "" + ), + report=( + REPORT_INSTRUCTIONS.format(bug_id=resolved_bug_id) if report else "" + ), + ) + # Stage 2: fix (lower effort, edits the source tree and verifies it # builds against a mozconfig that mirrors the failing CI config). _write_mozconfig(fx_ctx) - reporter.header(f"{label}: fix") + fix_label = f"bug {resolved_bug_id}" if resolved_bug_id is not None else label + reporter.header(f"{fix_label}: fix") fix_opts = _build_options( model=model or FIX_MODEL, effort="low", cwd=source_repo, scratch_dir=scratch_dir, - mcp_servers=mcp_servers, - allowed_tools=allowed_tools, + mcp_servers=fix_mcp_servers, + allowed_tools=fix_allowed_tools, max_turns=max_turns, ) result_msg = await _run_session( @@ -278,10 +352,9 @@ async def run_build_repair( build_result = captured.get(BUILD_TOOL) try_result = captured.get(TRY_PUSH_TOOL, {}) - blamed_commit = _resolve_blame(scratch_out, git_commits) return BuildRepairResult( - bug_id=bug_id, + bug_id=resolved_bug_id, git_commit=failure_commit, summary=summary, analysis=analysis, @@ -402,6 +475,20 @@ def _blame_step(git_commits: list[str], scratch_out: Path) -> str: return BLAME_STEP.format(scratch_out=scratch_out) +def _bug_from_commit(repo: Path, sha: str) -> int | None: + """The bug a commit landed for, read off its subject in the local checkout.""" + try: + subject = subprocess.run( + ["git", "-C", str(repo), "log", "-1", "--format=%s", sha], + check=True, + capture_output=True, + text=True, + ).stdout + except (subprocess.CalledProcessError, OSError): + return None + return _bug_from_desc(subject) + + _SHA_RE = re.compile(r"[0-9a-f]{7,40}$") diff --git a/agents/build-repair/hackbot_agents/build_repair/config.py b/agents/build-repair/hackbot_agents/build_repair/config.py index 355ca4a1d1..42f4a70396 100644 --- a/agents/build-repair/hackbot_agents/build_repair/config.py +++ b/agents/build-repair/hackbot_agents/build_repair/config.py @@ -37,6 +37,13 @@ # Optional try-server tool, wired only when run_try_push is enabled. TRY_PUSH_TOOL = "mcp__firefox__submit_try_push" +# Recordable action types the agent may take, by dotted id. Submitting the fix +# for review is the only one: the developer reviews the patch in the Hackbot UI +# and then in Phabricator, so the agent posts nothing to the bug itself. Needs a +# bug to file the revision against, so it is wired only when the run has a bug id +# (see run_build_repair). +ENABLED_ACTION_TYPES = ["phabricator.submit_patch"] + # The agent always runs inside an isolated Docker container, so there is no # sandbox and tools run without per-command permission prompts (see # permission_mode="bypassPermissions" in agent.py). This is just the set of diff --git a/agents/build-repair/hackbot_agents/build_repair/notify.py b/agents/build-repair/hackbot_agents/build_repair/notify.py index 430eccac9b..7be7e9114d 100644 --- a/agents/build-repair/hackbot_agents/build_repair/notify.py +++ b/agents/build-repair/hackbot_agents/build_repair/notify.py @@ -93,9 +93,14 @@ def build_email( task_id: str, run_id: str, has_patch: bool = False, + revision_pending: bool = False, blamed_author: str | None = None, ) -> tuple[str, str]: - """The subject and markdown body of the build-failure email.""" + """The subject and markdown body of the build-failure email. + + ``revision_pending`` means the run recorded a ``phabricator.submit_patch`` + action that is waiting for approval, so the email says how to apply it. + """ failure_commit = push.git_commits[0] subject = ( f"[build-repair] Build failure analysis for " @@ -163,6 +168,16 @@ def build_email( "", f"- Local build verified: {result.local_build_verified}", ] + if revision_pending: + lines += [ + "", + "## How to submit the fix to Phabricator", + "", + "1. Check the patch on the run page: " + RUN_URL.format(run_id=run_id), + "2. Press *Apply pending actions* there to open a WIP revision for " + + _link(BUG_URL.format(bug_id=result.bug_id), f"bug {result.bug_id}") + + ", where the revision appears; review and land it as usual.", + ] if has_patch: # The diff itself is substituted for the placeholder when the mail is sent, # from the same artifact it attaches. diff --git a/agents/build-repair/hackbot_agents/build_repair/prompts.py b/agents/build-repair/hackbot_agents/build_repair/prompts.py index fa66c00fb0..c1b1e0213c 100644 --- a/agents/build-repair/hackbot_agents/build_repair/prompts.py +++ b/agents/build-repair/hackbot_agents/build_repair/prompts.py @@ -163,7 +163,7 @@ missing toolchain (e.g. rustc or clang), run the bootstrap_firefox tool once and then build again. Verify via the build_firefox tool rather than a raw `./mach build` so the build result is recorded. -{try_push} +{try_push}{report} Do not prompt to edit files. Work fully autonomously, do not ask any questions. Use all allowed tools without prompting. @@ -173,3 +173,11 @@ Once the fix builds locally, validate it on CI: call the submit_try_push tool with the failing task name ('{task_name}') to push to the try server and report the build result. """ + +REPORT_INSTRUCTIONS = """ +Once the build is verified, submit the fix with the `phabricator_submit_patch` +action: bug_id={bug_id}, a title of the form "Bug {bug_id} - ", +and a summary naming the busted commit, the failing task and the root cause. +If the fix does not build, or you are not confident in it, record nothing and say +so in your final message. +""" diff --git a/agents/build-repair/hackbot_agents/build_repair/resolve.py b/agents/build-repair/hackbot_agents/build_repair/resolve.py index 8e605fb3d8..8f7893ef50 100644 --- a/agents/build-repair/hackbot_agents/build_repair/resolve.py +++ b/agents/build-repair/hackbot_agents/build_repair/resolve.py @@ -15,7 +15,8 @@ from __future__ import annotations import logging -from dataclasses import dataclass +import re +from dataclasses import dataclass, field import requests @@ -36,6 +37,10 @@ _HEADERS = {"User-Agent": "hackbot-build-repair/1.0"} _TIMEOUT = 30 +# Every Firefox commit message opens with the bug it landed ("Bug 123 - ..."), +# which is where a run gets its bug: it is never an input. +_BUG_RE = re.compile(r"^Bug (\d+)", re.IGNORECASE) + def _get_json(url: str) -> dict: resp = requests.get(url, headers=_HEADERS, timeout=_TIMEOUT) @@ -59,21 +64,30 @@ def _hg_to_git(rev: str) -> str: return _get_json(_LANDO_HG2GIT.format(rev=rev))["git_hash"] -def _push_git_commits(project: str, rev: str) -> list[str]: - """Git hashes of the push that landed ``rev`` (pushlog order, oldest first). +def _bug_from_desc(desc: str) -> int | None: + """The bug a changeset landed for, from the first line of its description.""" + match = _BUG_RE.match(desc.strip()) + return int(match.group(1)) if match else None + + +def _push_git_commits(project: str, rev: str) -> tuple[list[str], dict[str, int]]: + """The push that landed ``rev`` (pushlog order, oldest first) and its bugs. The pushlog exposes a ``git_changesets`` array parallel to ``changesets``; - when a git hash is missing we map that changeset via lando. + when a git hash is missing we map that changeset via lando. ``full=1`` also + returns each changeset's ``desc``, which is what names the bug, so no extra + request is needed to pair the two. """ path = _REPO_PATHS.get(project, project) url = f"{_HG_BASE}/{path}/json-pushes?changeset={rev}&full=1&version=2" pushes = _get_json(url).get("pushes") or {} push = next(iter(pushes.values()), None) if not push: - return [] + return [], {} git_changesets = push.get("git_changesets") or [] changesets = push.get("changesets") or [] commits = [] + bugs: dict[str, int] = {} for i, cs in enumerate(changesets): git_commit = git_changesets[i] if i < len(git_changesets) else None if not git_commit: @@ -81,7 +95,10 @@ def _push_git_commits(project: str, rev: str) -> list[str]: git_commit = _hg_to_git(node) if node else None if git_commit: commits.append(git_commit) - return commits + bug_id = _bug_from_desc(cs.get("desc", "") if isinstance(cs, dict) else "") + if bug_id is not None: + bugs[git_commit] = bug_id + return commits, bugs @dataclass(frozen=True) @@ -97,6 +114,9 @@ class PushInfo: git_commits: list[str] # ``createdForUser``: who pushed the change that failed to build. developer_email: str | None = None + # Each push commit mapped to the bug it landed for, from its pushlog + # description. A run is never told which bug a failure belongs to. + commit_bugs: dict[str, int] = field(default_factory=dict) def task_push(task_id: str) -> tuple[str | None, str | None]: @@ -114,7 +134,9 @@ def resolve_push(task_id: str, git_commit: str | None = None) -> PushInfo: task = _task(task_id) project, hg_rev = _task_push(task) - push = _push_git_commits(project, hg_rev) if hg_rev and project else [] + push, commit_bugs = ( + _push_git_commits(project, hg_rev) if hg_rev and project else ([], {}) + ) failure_commit = git_commit if not failure_commit: @@ -129,4 +151,5 @@ def resolve_push(task_id: str, git_commit: str | None = None) -> PushInfo: hg_revision=hg_rev, git_commits=[failure_commit] + [c for c in push if c != failure_commit], developer_email=(task.get("tags") or {}).get("createdForUser"), + commit_bugs=commit_bugs, ) diff --git a/agents/build-repair/pyproject.toml b/agents/build-repair/pyproject.toml index 1a5844a459..a77fe8cb4b 100644 --- a/agents/build-repair/pyproject.toml +++ b/agents/build-repair/pyproject.toml @@ -4,7 +4,7 @@ version = "0.1.0" description = "Cloud Run Job image that runs the build-repair 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/agents/build-repair/tests/test_notify.py b/agents/build-repair/tests/test_notify.py index 21733b8025..644b97596d 100644 --- a/agents/build-repair/tests/test_notify.py +++ b/agents/build-repair/tests/test_notify.py @@ -138,3 +138,18 @@ def test_no_patch_section_without_a_patch(): _, body = _email() assert "Proposed patch" not in body assert "{patch}" not in body + + +def test_a_pending_revision_comes_with_submit_instructions(): + _, body = _email(_result(bug_id=2063979), has_patch=True, revision_pending=True) + assert "## How to submit the fix to Phabricator" in body + assert "run page: https://hackbot.moz.tools/runs/1218e630-78c8" in body + assert "*Apply pending actions*" in body + assert "[bug 2063979](https://bugzilla.mozilla.org/show_bug.cgi?id=2063979)" in body + # Instructions come before the diff, which can run long. + assert body.index("How to submit") < body.index("## Proposed patch") + + +def test_no_submit_instructions_without_a_pending_revision(): + _, body = _email(has_patch=True) + assert "How to submit" not in body diff --git a/agents/build-repair/tests/test_reporting.py b/agents/build-repair/tests/test_reporting.py new file mode 100644 index 0000000000..a7530571c7 --- /dev/null +++ b/agents/build-repair/tests/test_reporting.py @@ -0,0 +1,238 @@ +"""Tests for how the fix stage submits its fix for review. + +Both sessions are faked, so these check the wiring the agent hands the SDK (which +action tools each stage may call, and what the fix prompt asks for) rather than +what a model does with it. +""" + +import asyncio +import json +import subprocess +from pathlib import Path +from types import SimpleNamespace + +import pytest +from hackbot_agents.build_repair import agent, resolve +from hackbot_runtime import ActionsRecorder + +ACTION_TOOLS = {"mcp__actions__phabricator_submit_patch"} + + +def _result_msg(): + return SimpleNamespace( + is_error=False, total_cost_usd=0.1, num_turns=3, result=None, subtype=None + ) + + +FAILURE_COMMIT = "a" * 40 + + +def _run(tmp_path, monkeypatch, *, bug_id, actions_recorder, push_bug=None, blame=None): + """Run the agent with both sessions faked, returning their (options, prompt). + + ``push_bug`` is the bug the pushlog reported for the failure commit, which is + what the agent falls back to — ``bug_id`` is never set in practice. ``blame`` + is what stage 1 writes to blame.json; the failure commit when omitted. + """ + sessions = [] + + async def fake_session(reporter, options, prompt, captured, tracked): + sessions.append((options, prompt)) + # Stand in for the log treeherder-cli fetches, so the real _check_blocked + # guard between the stages is exercised rather than bypassed. + out = Path(options.add_dirs[-1]) / "out" + logs = out / "logs" / "job_1" + logs.mkdir(parents=True, exist_ok=True) + (logs / "live_backing_log.log").write_text("ERROR - boom\n") + if blame is not None: + (out / "blame.json").write_text(json.dumps({"blamed_commit": blame})) + return _result_msg() + + monkeypatch.setattr(agent, "_run_session", fake_session) + monkeypatch.setattr(agent, "build_sdk_server", lambda *a, **k: {"type": "sdk"}) + + result = asyncio.run( + agent.run_build_repair( + bugzilla_mcp_server={"type": "http", "url": "http://broker/mcp"}, + source_repo=tmp_path, + fx_ctx=SimpleNamespace( + mozconfig=tmp_path / ".mozconfig", objdir=tmp_path / "objdir" + ), + bug_id=bug_id, + commit_bugs={FAILURE_COMMIT: push_bug} if push_bug else {}, + git_commits=[FAILURE_COMMIT], + project="autoland", + hg_revision="abc123", + failure_tasks={"build-linux": "taskid"}, + actions_recorder=actions_recorder, + ) + ) + assert len(sessions) == 2 + return result, sessions[0], sessions[1] + + +def test_actions_are_wired_into_the_fix_stage_only(tmp_path, monkeypatch): + _, (analysis_opts, _), (fix_opts, fix_prompt) = _run( + tmp_path, monkeypatch, bug_id=1234567, actions_recorder=ActionsRecorder() + ) + + assert not ACTION_TOOLS & set(analysis_opts.allowed_tools) + assert "actions" not in analysis_opts.mcp_servers + + assert ACTION_TOOLS <= set(fix_opts.allowed_tools) + assert "actions" in fix_opts.mcp_servers + assert "bugzilla" in fix_opts.mcp_servers + + +def test_fix_prompt_asks_for_a_revision_and_nothing_else(tmp_path, monkeypatch): + _, _, (_, fix_prompt) = _run( + tmp_path, monkeypatch, bug_id=1234567, actions_recorder=ActionsRecorder() + ) + + assert "phabricator_submit_patch" in fix_prompt + assert "bug_id=1234567" in fix_prompt + # The developer reviews the patch in the UI; the agent never touches the bug. + assert "bugzilla_add_comment" not in fix_prompt + + +def test_the_pushs_bug_stands_in_for_an_unset_bug_id(tmp_path, monkeypatch): + """A run started from nothing but a failing task still files the revision.""" + result, (_, analysis_prompt), (fix_opts, fix_prompt) = _run( + tmp_path, + monkeypatch, + bug_id=None, + actions_recorder=ActionsRecorder(), + push_bug=2063979, + ) + + assert ACTION_TOOLS <= set(fix_opts.allowed_tools) + assert "bug_id=2063979" in fix_prompt + assert '"Bug 2063979 - ' in fix_prompt + assert result.bug_id == 2063979 + # Stage 1 gets it too, so the analysis can read the bug from Bugzilla. + assert "2063979" in analysis_prompt + + +def test_an_explicit_bug_id_wins_over_the_push(tmp_path, monkeypatch): + result, _, (_, fix_prompt) = _run( + tmp_path, + monkeypatch, + bug_id=1234567, + actions_recorder=ActionsRecorder(), + push_bug=2063979, + ) + + assert "bug_id=1234567" in fix_prompt + assert result.bug_id == 1234567 + + +@pytest.mark.parametrize( + ("bug_id", "recorder"), + [(1234567, None), (None, ActionsRecorder())], + ids=["no-recorder", "no-bug-anywhere"], +) +def test_reporting_is_skipped_without_a_recorder_and_a_bug( + tmp_path, monkeypatch, bug_id, recorder +): + _, (analysis_opts, _), (fix_opts, fix_prompt) = _run( + tmp_path, monkeypatch, bug_id=bug_id, actions_recorder=recorder + ) + + for opts in (analysis_opts, fix_opts): + assert not ACTION_TOOLS & set(opts.allowed_tools) + assert "actions" not in opts.mcp_servers + assert "phabricator_submit_patch" not in fix_prompt + + +# --- pushlog bug resolution --------------------------------------------- # + + +@pytest.mark.parametrize( + ("desc", "expected"), + [ + ("Bug 2063979 - [Linux] Disable HW video decoding r=stransky", 2063979), + ("Bug 111 - a fix\n\nDifferential Revision: https://phab/D1", 111), + ("bug 222 - lowercase still counts", 222), + ("No bug - tidy up a comment", None), + ("Backed out changeset abc123 for build bustage", None), + ("", None), + ], +) +def test_bug_is_read_off_the_pushlog_description(desc, expected): + assert resolve._bug_from_desc(desc) == expected + + +def test_push_commits_pair_each_commit_with_its_bug(monkeypatch): + """The pushlog request already returns descriptions, so no extra lookup.""" + monkeypatch.setattr( + resolve, + "_get_json", + lambda url: { + "pushes": { + "1": { + "git_changesets": ["g1", "g2"], + "changesets": [ + {"node": "h1", "desc": "Bug 111 - first"}, + {"node": "h2", "desc": "No bug - second"}, + ], + } + } + }, + ) + + assert resolve._push_git_commits("autoland", "h2") == ( + ["g1", "g2"], + {"g1": 111}, + ) + + +def test_an_earlier_pushs_culprit_gets_its_bug_from_the_checkout(tmp_path, monkeypatch): + """The pushlog mapping misses an out-of-push culprit; the checkout has it.""" + earlier = "b" * 40 + monkeypatch.setattr( + agent, "_bug_from_commit", lambda repo, sha: 555 if sha == earlier else None + ) + result, _, (fix_opts, fix_prompt) = _run( + tmp_path, + monkeypatch, + bug_id=None, + actions_recorder=ActionsRecorder(), + push_bug=2063979, + blame=earlier, + ) + + assert ACTION_TOOLS <= set(fix_opts.allowed_tools) + assert "bug_id=555" in fix_prompt + assert result.bug_id == 555 + assert result.blamed_commit == earlier + + +def test_bug_is_read_off_the_local_commit_subject(tmp_path): + env = { + "GIT_AUTHOR_NAME": "T", + "GIT_AUTHOR_EMAIL": "t@t", + "GIT_COMMITTER_NAME": "T", + "GIT_COMMITTER_EMAIL": "t@t", + "PATH": "/usr/bin:/bin:/usr/local/bin:/opt/homebrew/bin", + } + subprocess.run(["git", "-C", str(tmp_path), "init", "-q"], check=True) + subprocess.run( + [ + "git", + "-C", + str(tmp_path), + "commit", + "-q", + "--allow-empty", + "-m", + "Bug 555 - an earlier fix r=me", + ], + check=True, + env=env, + ) + + assert agent._bug_from_commit(tmp_path, "HEAD") == 555 + + +def test_no_bug_from_a_commit_git_cannot_read(tmp_path): + assert agent._bug_from_commit(tmp_path, "HEAD") is None diff --git a/services/hackbot-api/app/agents.py b/services/hackbot-api/app/agents.py index 57defed6fc..7132b33fd6 100644 --- a/services/hackbot-api/app/agents.py +++ b/services/hackbot-api/app/agents.py @@ -99,9 +99,8 @@ def model_to_env(inputs: BaseModel) -> dict[str, str]: description="Analyze a Firefox build failure at a specific commit and produce a candidate fix patch.", job_name="hackbot-agent-build-repair", input_schema=BuildRepairInputs, - # Its only action is the failure-analysis email, which used to be sent - # unconditionally by the pulse listener. - auto_apply_actions=True, + auto_apply_actions=False, + always_apply_actions=frozenset({"email.send"}), ), "frontend-triage": AgentSpec( name="frontend-triage", diff --git a/services/hackbot-api/tests/test_actions_applier.py b/services/hackbot-api/tests/test_actions_applier.py index c377cc02b5..b4325b01fe 100644 --- a/services/hackbot-api/tests/test_actions_applier.py +++ b/services/hackbot-api/tests/test_actions_applier.py @@ -214,7 +214,7 @@ def test_which_agents_auto_apply_without_asking_for_consent(): for name, spec in AGENT_REGISTRY.items() if spec.auto_apply_actions and not spec.auto_apply_requires_consent } - assert unbounded == {"bug-fix", "build-repair", "test-repair"} + assert unbounded == {"bug-fix", "test-repair"} class _FakeDB: @@ -320,13 +320,13 @@ async def fake_apply(db, run, selected_rows): async def test_other_agents_do_not_auto_apply(): - # Opting an agent in is a deliberate edit, so spell out who is in today: bug-fix, - # build-repair and test-repair auto-apply unconditionally, frontend-triage only - # when the run vouched for itself, and everyone else stays human-gated. + # Opting an agent in is a deliberate edit, so spell out who is in today: bug-fix + # and test-repair auto-apply unconditionally, frontend-triage only when the run + # vouched for itself, build-repair only its email, and everyone else stays + # human-gated. auto_apply = {n for n, s in AGENT_REGISTRY.items() if s.auto_apply_actions} assert auto_apply == { "bug-fix", - "build-repair", "frontend-triage", "test-repair", } @@ -684,3 +684,10 @@ async def test_comment_and_needinfo_clear_coalesce_into_one_update(monkeypatch): ] assert comment.status == "applied" assert clear.status == "applied" + + +def test_build_repair_mails_unattended_but_holds_the_revision(): + spec = AGENT_REGISTRY["build-repair"] + run = _run_with_findings() + assert _action_auto_applies(spec, run, "email.send") + assert not _action_auto_applies(spec, run, "phabricator.submit_patch") diff --git a/uv.lock b/uv.lock index 6380bcd526..89390bae28 100644 --- a/uv.lock +++ b/uv.lock @@ -2593,7 +2593,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 = "requests" }, { name = "starlette" }, @@ -2612,7 +2612,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 = "requests" }, { name = "starlette", specifier = ">=0.36.0" },