From eb9b133acef135af9955d1122e9da09109e58a1d Mon Sep 17 00:00:00 2001 From: Connor Sheehan Date: Thu, 17 Sep 2026 22:30:54 -0400 Subject: [PATCH 1/9] Bug 2051452: Add uplift source models and agent config An uplift run is an ordered list of sources applied onto a stable branch, so a stack is simply several sources. `kind` discriminates the two shapes a source can take, letting one run mix them. Each kind knows two things about itself: how to materialize whatever the agent needs on disk (`fetch_diff`) and how to describe itself as a work item in the prompt (`render_work_item`). Keeping both on the model means a third source kind touches this module only. A Phabricator source carries only `revision_id` plus an optional `diff_id`, not the diff text. Run inputs reach the Cloud Run Job as environment variables, so an inlined diff would be bounded by the env size limit. `diff_id` defaults to the revision's latest, but a caller that knows which diff it landed should pin it: otherwise a revision updated between request and run resolves to different code. Diffs are named on disk by diff as well as revision, since sources are all fetched up front and two pins on one revision would otherwise share a file. A fetched diff carries its author and its base commit. The author because the uplift commit must keep it -- Lando refuses a patch authored by hackbot -- and the base commit because `git apply --3way` needs it fetched to find the blobs the diff names. moz-phab abbreviates that hash for a repo the size of firefox and `git fetch` refuses an abbreviation, so it is expanded before use. `RequestedSource` is the record of what a run was asked to uplift and what the fetch pinned each source down to, which for an unpinned source is not derivable from the input. It is not evidence anything was applied. `Report` is the schema of the `report.json` the agent writes, which is model output and so validated rather than trusted. Every field defaults, so a partial report still parses. What validation buys is catching a plausible but wrong value: `"resolved": "false"` is a string `bool()` reads as `True`, and `confidence` is what a caller gates on. Issue: https://github.com/mozilla/bugbug/issues/6865 --- .../__init__.py | 0 .../uplift_merge_conflict_resolver/config.py | 240 ++++++++++++++++++ 2 files changed, 240 insertions(+) create mode 100644 agents/uplift/hackbot_agents/uplift_merge_conflict_resolver/__init__.py create mode 100644 agents/uplift/hackbot_agents/uplift_merge_conflict_resolver/config.py diff --git a/agents/uplift/hackbot_agents/uplift_merge_conflict_resolver/__init__.py b/agents/uplift/hackbot_agents/uplift_merge_conflict_resolver/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/agents/uplift/hackbot_agents/uplift_merge_conflict_resolver/config.py b/agents/uplift/hackbot_agents/uplift_merge_conflict_resolver/config.py new file mode 100644 index 0000000000..36f289547b --- /dev/null +++ b/agents/uplift/hackbot_agents/uplift_merge_conflict_resolver/config.py @@ -0,0 +1,240 @@ +"""Models and source types for the uplift agent. + +Each source kind knows how to fetch what it needs (`fetch_diff`) and how to +describe itself in the prompt (`render_work_item`), so a new kind touches only +this module. +""" + +from __future__ import annotations + +import logging +from dataclasses import dataclass +from pathlib import Path +from typing import Annotated, Literal, Union + +from hackbot_runtime import AgentError +from phabricator_client import ( + PhabricatorClient, + PhabricatorDiff, + UnresolvedCommitError, +) +from pydantic import BaseModel, ConfigDict, Field + +logger = logging.getLogger(__name__) + +MODEL = "claude-opus-5" + + +@dataclass(frozen=True) +class FetchedDiff: + """A source's diff on disk, and what the agent needs to apply it.""" + + path: Path + + # ``"Name "``, or ``None`` when Phabricator recorded none. The + # uplift commit must keep it: Lando refuses a patch authored by hackbot. + author: str | None + + # The commit the diff was built on. Until it is fetched the blobs the diff + # names are missing from the shallow checkout, and `--3way` silently + # degrades to a direct apply that leaves no conflict markers. + base_commit: str | None = None + + # The diff actually fetched, which an unpinned source does not name. + diff_id: int | None = None + + +class GitSource(BaseModel): + """A patch to uplift, identified by a git commit in the Firefox repo.""" + + kind: Literal["git"] = "git" + commit: str = Field(description="Full git commit SHA to cherry-pick.") + + async def fetch_diff( + self, client: PhabricatorClient, scratch_out: Path + ) -> FetchedDiff | None: + """Nothing to fetch: the commit is already in the repo to cherry-pick.""" + return None + + def render_work_item(self, index: int, fetched: FetchedDiff | None) -> str: + """Describe this source as a numbered work item for the prompt.""" + return ( + f"{index}. git commit `{self.commit}` — cherry-pick it, which keeps " + f"the original author." + ) + + +class PhabricatorSource(BaseModel): + """A patch to uplift, identified by a Phabricator revision. + + The diff is fetched through the broker rather than passed in: run inputs + reach the job as environment variables, which a large diff will not fit. + """ + + kind: Literal["phabricator"] = "phabricator" + revision_id: int = Field(description="Phabricator revision id (the D-number).") + + # Pin the exact diff. Lando knows which one it landed; without a pin, a + # revision updated since the request resolves to different code. + diff_id: int | None = Field( + default=None, + description="Diff to uplift; defaults to the revision's latest.", + ) + + def diff_path(self, scratch_out: Path, diff_id: int) -> Path: + """The on-disk path a given diff of this revision is written to. + + Named by diff too: sources are all fetched up front, so two pins on one + revision would otherwise share a file and lose one of them. + """ + return scratch_out / f"D{self.revision_id}-{diff_id}.diff" + + async def resolve_diff(self, client: PhabricatorClient) -> PhabricatorDiff: + """The diff to uplift: the pinned one, else the revision's latest. + + Raises :class:`AgentError` for a revision with no diffs, or a pin that + does not belong to it, rather than uplifting other code. + """ + result = await client.conduit_request( + "differential.querydiffs", revisionIDs=[self.revision_id] + ) + by_id = {int(raw["id"]): raw for raw in (result or {}).values()} + if not by_id: + raise AgentError(f"D{self.revision_id} has no diffs to uplift") + + if self.diff_id is None: + raw = by_id[max(by_id)] + elif self.diff_id in by_id: + raw = by_id[self.diff_id] + else: + raise AgentError( + f"diff {self.diff_id} does not belong to D{self.revision_id}" + ) + + return PhabricatorDiff.model_validate(raw) + + async def fetch_diff( + self, client: PhabricatorClient, scratch_out: Path + ) -> FetchedDiff | None: + """Write this revision's diff to disk and return what to apply it with.""" + diff = await self.resolve_diff(client) + path = self.diff_path(scratch_out, diff.id) + path.write_text(await client.get_raw_diff(diff.id)) + return FetchedDiff( + path=path, + author=diff.author, + base_commit=await self.resolve_base(client, diff), + diff_id=diff.id, + ) + + async def resolve_base( + self, client: PhabricatorClient, diff: PhabricatorDiff + ) -> str | None: + """The diff's base commit as a full hash, the only kind git will fetch. + + moz-phab abbreviates it for a repo the size of firefox. One that cannot + be expanded is reported as none: it is a hint, and the prompt covers + going without. + """ + if diff.base_commit is None: + return None + try: + return await client.resolve_commit(diff.base_commit) + except UnresolvedCommitError: + logger.warning( + "could not expand base commit %s of D%s to a full hash", + diff.base_commit, + self.revision_id, + ) + return None + + def render_work_item(self, index: int, fetched: FetchedDiff | None) -> str: + """Describe this source as a numbered work item for the prompt. + + ``fetched`` is this revision's own :meth:`fetch_diff` result and so is + never ``None``; the type is optional only because a git source fetches + nothing. Fail rather than name a path no diff was written to. + """ + if fetched is None: + raise AgentError( + f"D{self.revision_id}: diff must be fetched before it is rendered" + ) + return ( + f"{index}. Phabricator revision D{self.revision_id} — the diff is at " + f"`{fetched.path}`, {self.render_base(fetched)}. Commit it " + f"{self.render_author(fetched)}." + ) + + def render_base(self, fetched: FetchedDiff) -> str: + """The base commit to fetch before applying, or that there is none.""" + if fetched.base_commit is None: + return "and Phabricator recorded no base commit for it" + return f"built on base commit `{fetched.base_commit}`" + + def render_author(self, fetched: FetchedDiff) -> str: + """How the agent must attribute this source's commit. + + Phabricator records no author for a diff uploaded through the web UI. + Say so, rather than let the agent commit as the container. + """ + if fetched.author is None: + return ( + "with `--author` set to the revision's author, which Phabricator " + "did not record — read it off the originating bug or revision, " + "and say so in your report if you cannot" + ) + return f'with `--author="{fetched.author}"`' + + +# Sources are applied in order, and a run may mix the two kinds. +UpliftSource = Annotated[ + Union[GitSource, PhabricatorSource], Field(discriminator="kind") +] + + +class RequestedSource(BaseModel): + """What one requested source resolved to, before the agent touched it. + + A record of what was asked for and what the fetch pinned it down to, not + evidence any of it was applied -- the report and the checks on the checkout + speak to that. + """ + + # The source as supplied. A raw mapping, to stay agnostic about the kinds. + source: dict + + # The diff actually used, which differs from the input when nothing was + # pinned. Phabricator sources only. + diff_id: int | None = None + + base_commit: str | None = None + author: str | None = None + + +class ConflictReport(BaseModel): + """One file the agent resolved, as it reports it. + + Tolerant on purpose: a malformed entry should not cost the whole report. + """ + + model_config = ConfigDict(extra="ignore") + + file: str = "" + resolution: str = "" + + +class Report(BaseModel): + """The agent's ``report.json``, in the shape the system prompt specifies. + + Every field defaults, so a partial report still parses. Validation is what + catches a plausible but wrong value: `"false"` is a string `bool()` reads + as `True`, and a caller gates on `confidence`. + """ + + model_config = ConfigDict(extra="ignore") + + resolved: bool = False + confidence: Literal["high", "medium", "low"] | None = None + summary: str = "" + conflicts: list[ConflictReport] = [] + unresolved: list[str] = [] From 4f1697e6d5a1f9b9229d11907c804dbbc5383bb5 Mon Sep 17 00:00:00 2001 From: Connor Sheehan Date: Thu, 17 Sep 2026 22:30:54 -0400 Subject: [PATCH 2/9] Bug 2051452: Add the uplift agent prompts Split the way every other agent splits them: `system.md` is how to resolve an uplift and holds no per-run detail, `task.md` is what this run is uplifting, onto what, and where to report it. `system.md` covers how to apply each kind of source, how to resolve a conflict without dropping the patch's functional change, which Mozilla MCP to consult when the intent is unclear, and the report shape. Four things it insists on: confidence is graded conservatively, since a wrong uplift on a stable branch is expensive and the grade is what a human triages on; every commit keeps the original patch author, because Lando refuses a patch authored by hackbot; a Phabricator diff's base commit is fetched before applying it, without which the three-way merge finds no blobs and leaves nothing to resolve; and the checkout has to agree with the report, since that is checked mechanically. Keeping the run out of `system.md` leaves an identical prefix on every run for prompt caching to reuse, and keeps Phabricator-supplied strings -- an author name reaches the prompt through `--author` -- out of the most privileged part of the context. Issue: https://github.com/mozilla/bugbug/issues/6865 --- .../prompts/system.md | 109 ++++++++++++++++++ .../prompts/task.md | 11 ++ 2 files changed, 120 insertions(+) create mode 100644 agents/uplift/hackbot_agents/uplift_merge_conflict_resolver/prompts/system.md create mode 100644 agents/uplift/hackbot_agents/uplift_merge_conflict_resolver/prompts/task.md diff --git a/agents/uplift/hackbot_agents/uplift_merge_conflict_resolver/prompts/system.md b/agents/uplift/hackbot_agents/uplift_merge_conflict_resolver/prompts/system.md new file mode 100644 index 0000000000..6ad0409e59 --- /dev/null +++ b/agents/uplift/hackbot_agents/uplift_merge_conflict_resolver/prompts/system.md @@ -0,0 +1,109 @@ +You are a Firefox engineer who is working to uplift code to a release train. + +The source checkout is already on the target uplift branch. Your job is to apply +the patches you are given onto that branch, resolve merge conflicts, and return +the final patch as an artifact. + +## How to apply each patch + +Apply the sources one at a time, in the order given. The checkout is shallow, so +commits you need are not present locally until you fetch them. + +For a **Git** source (a commit SHA): + +1. Fetch the commit and its parent so cherry-pick can three-way merge: + `git fetch --depth=2 origin ` +2. `git cherry-pick -x ` +3. If it conflicts, resolve the conflicts and commit the fixed patch. + +For a **Phabricator** source (a revision id): + +1. Its diff has already been fetched for you, at the path your task lists for + it. Do not try to download it yourself. +2. Fetch the base commit listed for it: `git fetch --depth=1 origin `. + Do not skip this. The diff names the blobs it was built from, and until that + commit is local they are missing, so `git apply --3way` reports "repository + lacks the necessary blob", silently falls back to a direct apply, and fails + leaving you no conflict markers at all. +3. Apply it with a three-way merge so conflicts surface as markers: + `git apply --3way ` +4. Resolve any conflicts and commit the fixed patch. + +If no base commit is listed, Phabricator never recorded one. Try the apply +anyway; should it fail with no markers, find the base yourself (the revision or +the bug will name it) and fetch it rather than hand-applying a patch that did +not apply. + +## Resolving a conflict + +Resolve conflicts while preserving the *intent* of the original patch while +fitting the code as it exists on the target branch: + +- When the intent is not obvious from the diff alone, consult the originating + bug (`get_bugzilla_bug`) and the Phabricator revision (`get_phabricator_revision`) + through the bugbug MCP. Read both before resolving a non-trivial conflict. +- Use `read_fx_doc_section` when you need Firefox architecture or workflow + context. +- Never drop the patch's functional change to make a conflict "go away", and + never invent behavior the patch did not have. If a hunk simply does not apply + because the surrounding feature is absent on the branch, that is a real signal + — record it rather than forcing a resolution. + +## Tools available to you + +You work like a local Claude Code session: `Read`/`Grep`/`Glob`/`Edit`/`Write`, +`Bash` for git, and `Task` sub-agents for parallel investigation. On top of the +built-ins, the following Mozilla MCP servers are wired in: + +- **searchfox** — search mozilla-central by identifier, text, or definition, and + read blame. Use it to understand how the code the patch touches evolved on the + branch, and to find the modern location of code the patch expected. +- **mozilla_vcs** — read a changeset's diff, metadata, and file history from + hg.mozilla.org. Use it to see exactly what landed between the patch's base and + the target branch. + +You cannot build or run Firefox. Where you would otherwise reach for a build to +settle a question, read the code instead and say what you could not verify in +your report -- a reviewer compiles the resolved patch. + +## Confidence level in the patch + +A wrong uplift on a stable branch is expensive. Be conservative: + +- `high`: conflicts were mechanical (imports, adjacent edits, context shifts) + and the resolution is unambiguous. +- `medium`: you had to make a judgment call, but the bug/revision context + supports it. +- `low`: the branch has diverged enough that you are unsure, or you could not + fully resolve a hunk. A human must look closely. + +## When you are done + +Leave all resolved patches committed in the working tree (the platform collects +them as the output patch). Commit everything: uncommitted work is swept into a +synthetic commit authored by this container, which loses the original author. + +Then write `report.json`, in the output directory your task names, with exactly +this shape: + +```json +{ + "resolved": true, + "confidence": "high", + "summary": "One short paragraph for the developer: what conflicted and how you resolved it.", + "conflicts": [ + {"file": "path/to/file", "resolution": "what you did and why"} + ], + "unresolved": ["path/to/file: why it could not be resolved"] +} +``` + +Set `resolved` to `false` if any source could not be applied or any conflict was +left unresolved, and list those in `unresolved`. Before you finish, check that +the checkout agrees with what you are about to claim: no conflicted paths left +in the index, no cherry-pick or merge still open, nothing uncommitted, and +commits that actually change something. All of that is verified mechanically +after you stop, and a `resolved` that disagrees with it is overruled. + +Also write a short, human-readable `summary.md` beside the report, covering the +same ground for the reviewer. diff --git a/agents/uplift/hackbot_agents/uplift_merge_conflict_resolver/prompts/task.md b/agents/uplift/hackbot_agents/uplift_merge_conflict_resolver/prompts/task.md new file mode 100644 index 0000000000..7be7a41ed3 --- /dev/null +++ b/agents/uplift/hackbot_agents/uplift_merge_conflict_resolver/prompts/task.md @@ -0,0 +1,11 @@ +Uplift the patches below onto `{target_branch}` and resolve every conflict. +Follow the workflow in your instructions. + +## The patches to uplift (apply in this exact order) + +{sources_block} + +{bug_block}## Where to write your report + +Write `{scratch_out}/report.json` and `{scratch_out}/summary.md`, in the shapes +your instructions describe. From 102add86f5be87667353b73996cf0d6f6bb557cf Mon Sep 17 00:00:00 2001 From: Connor Sheehan Date: Thu, 17 Sep 2026 22:31:13 -0400 Subject: [PATCH 3/9] Bug 2051452: Add the mechanical checks on a finished uplift The agent grades its own work, and a caller gates landing on `resolved`. Taken on trust that accepts a report claiming success while listing unresolved hunks, a tree with conflict markers still in the index, a cherry-pick left open, work left uncommitted, or a claimed resolution with no patch behind it at all. None of those are judgment calls, so they are checked against the repository. Confidence stays the agent's own grade, which only it can give. "There is a patch" is checked the way the runtime collects one, since HEAD merely differing from the commit the run started at does not mean there is something to collect: HEAD can move backwards, and an empty commit moves it while changing nothing. So the base has to remain an ancestor, the range has to hold commits, and those commits have to change something. Tested against real repositories, including a genuinely conflicted cherry-pick: these are statements about git's own state, and a stub could only assert we asked for it. The fixtures neutralise global and system git config, since a developer's commit signing or hooks path would otherwise reach these repositories -- and the runtime's own git calls too. Issue: https://github.com/mozilla/bugbug/issues/6865 --- .../uplift_merge_conflict_resolver/verify.py | 80 ++++++++++ agents/uplift/tests/conftest.py | 88 +++++++++++ agents/uplift/tests/test_uplift_verify.py | 142 ++++++++++++++++++ 3 files changed, 310 insertions(+) create mode 100644 agents/uplift/hackbot_agents/uplift_merge_conflict_resolver/verify.py create mode 100644 agents/uplift/tests/conftest.py create mode 100644 agents/uplift/tests/test_uplift_verify.py diff --git a/agents/uplift/hackbot_agents/uplift_merge_conflict_resolver/verify.py b/agents/uplift/hackbot_agents/uplift_merge_conflict_resolver/verify.py new file mode 100644 index 0000000000..546e27c382 --- /dev/null +++ b/agents/uplift/hackbot_agents/uplift_merge_conflict_resolver/verify.py @@ -0,0 +1,80 @@ +"""Mechanical checks on a finished uplift. + +The agent grades its own work and a caller gates landing on `resolved`, so it +is checked against the repository rather than trusted. Confidence stays the +agent's own judgment; none of these are. +""" + +import subprocess +from pathlib import Path + +from .config import Report + + +def git_output(repo: Path, *args: str) -> str: + """Run a read-only git command in ``repo`` and return its stripped stdout.""" + return subprocess.run( + ["git", "-C", str(repo), *args], + check=True, + capture_output=True, + text=True, + ).stdout.strip() + + +def head_commit(repo: Path) -> str: + """The commit ``repo`` is currently on.""" + return git_output(repo, "rev-parse", "HEAD") + + +def verify_uplift(repo: Path, base_commit: str, report: Report) -> list[str]: + """What the checkout and the report say is wrong, whatever the agent claimed. + + One line per problem, empty when the run is fit to hand on. + """ + problems = [] + + if report.resolved and report.unresolved: + problems.append( + f"report claims resolved while listing {len(report.unresolved)} " + f"unresolved item(s)" + ) + + unmerged = git_output(repo, "diff", "--name-only", "--diff-filter=U") + if unmerged: + problems.append(f"unresolved conflicts still in the index: {unmerged.split()}") + + # Mid-operation the tree looks plausible, but the patch would be a fragment. + git_dir = Path(git_output(repo, "rev-parse", "--absolute-git-dir")) + for marker, operation in ( + ("CHERRY_PICK_HEAD", "cherry-pick"), + ("MERGE_HEAD", "merge"), + ("rebase-merge", "rebase"), + ("rebase-apply", "rebase"), + ): + if (git_dir / marker).exists(): + problems.append(f"{operation} left unfinished in the checkout") + break + + # Uncommitted work is swept into a container-authored commit, losing the + # original author. + dirty = git_output(repo, "status", "--porcelain") + if dirty: + problems.append(f"{len(dirty.splitlines())} path(s) left uncommitted") + + # A claimed resolution has to leave a patch behind it, and HEAD simply + # differing from the base does not mean it does: HEAD can move backwards, + # and an empty commit moves it while changing nothing. This is what the + # runtime will collect, so it is checked the way the runtime collects it. + # A run reporting failure is exempt -- having nothing to collect is honest. + if report.resolved: + if git_output(repo, "merge-base", base_commit, "HEAD") != base_commit: + problems.append( + f"HEAD no longer descends from the commit the run started at " + f"({base_commit[:12]})" + ) + elif not git_output(repo, "rev-list", f"{base_commit}..HEAD"): + problems.append("report claims resolved but no commits were produced") + elif not git_output(repo, "diff", "--name-only", base_commit, "HEAD"): + problems.append("report claims resolved but the commits change nothing") + + return problems diff --git a/agents/uplift/tests/conftest.py b/agents/uplift/tests/conftest.py new file mode 100644 index 0000000000..07f0299d22 --- /dev/null +++ b/agents/uplift/tests/conftest.py @@ -0,0 +1,88 @@ +"""Shared fixtures for the uplift agent tests.""" + +from __future__ import annotations + +import os +import subprocess +from collections.abc import Callable +from pathlib import Path + +import pytest + + +@pytest.fixture(autouse=True) +def isolated_git_config(monkeypatch) -> None: + """Keep the developer's own git config out of the repositories these build. + + Set in the environment rather than passed per call, because the runtime + makes git calls of its own: with global commit signing or a hooks path + configured, those would fail too. Identity still comes from each + repository's local config. + """ + monkeypatch.setenv("GIT_CONFIG_GLOBAL", os.devnull) + monkeypatch.setenv("GIT_CONFIG_SYSTEM", os.devnull) + monkeypatch.setenv("GIT_CONFIG_NOSYSTEM", "1") + monkeypatch.setenv("GIT_TERMINAL_PROMPT", "0") + + +@pytest.fixture +def git_in() -> Callable[..., str]: + """Run git in a repository and return its stripped stdout. + + ``check=False`` is for commands expected to fail, like a cherry-pick that + conflicts on purpose. + """ + + def run(repo: Path, *args: str, check: bool = True) -> str: + return subprocess.run( + ["git", "-C", str(repo), *args], + check=check, + capture_output=True, + text=True, + ).stdout.strip() + + return run + + +@pytest.fixture +def repo(tmp_path, git_in) -> Path: + """A git repository at one commit, standing in for the prepared checkout. + + Real rather than stubbed: what the run checks afterwards are facts about + git's own state, which a stub could only assert we asked for. + """ + path = tmp_path / "firefox" + path.mkdir() + git_in(path, "init", "-q") + git_in(path, "config", "user.email", "agent@example.com") + git_in(path, "config", "user.name", "Agent") + (path / "f.txt").write_text("line1\nline2\nline3\n") + git_in(path, "add", "-A") + git_in(path, "commit", "-qm", "base") + return path + + +class RecordingPublisher: + """A ``publish_file`` callable that records calls instead of uploading. + + Bodies are read when the call is made, as a real uploader reads them, so a + later write to the same path cannot change what a test sees published. + """ + + def __init__(self) -> None: + self.calls: list[tuple[str, Path, str | None]] = [] + self.bodies: dict[str, str] = {} + + def __call__(self, name: str, path: Path, content_type: str | None) -> str: + self.calls.append((name, Path(path), content_type)) + self.bodies[name] = Path(path).read_text() + return f"published://{name}" + + @property + def names(self) -> list[str]: + return [name for name, _, _ in self.calls] + + +@pytest.fixture +def publisher() -> RecordingPublisher: + return RecordingPublisher() diff --git a/agents/uplift/tests/test_uplift_verify.py b/agents/uplift/tests/test_uplift_verify.py new file mode 100644 index 0000000000..1b12da311f --- /dev/null +++ b/agents/uplift/tests/test_uplift_verify.py @@ -0,0 +1,142 @@ +"""Tests for the mechanical checks applied to a finished uplift. + +Real repositories throughout: every check is a statement about git's own +state, which a stub could only assert we asked for. +""" + +from __future__ import annotations + +from pathlib import Path + +from hackbot_agents.uplift_merge_conflict_resolver.config import Report +from hackbot_agents.uplift_merge_conflict_resolver.verify import verify_uplift + +RESOLVED = Report(resolved=True, confidence="high", summary="done") + + +def commit(git_in, repo: Path, content: str, message: str = "uplifted") -> None: + (repo / "f.txt").write_text(content) + git_in(repo, "add", "-A") + git_in(repo, "commit", "-qm", message) + + +def conflicted_cherry_pick(git_in, repo: Path) -> str: + """Leave ``repo`` mid-cherry-pick with a conflict, and return the base. + + The state a real failed uplift arrives in. + """ + base = git_in(repo, "rev-parse", "HEAD") + git_in(repo, "checkout", "-q", "-b", "theirs") + commit(git_in, repo, "line1\nline2 theirs\nline3\n", "theirs") + git_in(repo, "checkout", "-q", base) + git_in(repo, "checkout", "-q", "-b", "ours") + commit(git_in, repo, "line1\nline2 ours\nline3\n", "ours") + git_in(repo, "cherry-pick", "theirs", check=False) + return base + + +def test_a_committed_resolution_passes(repo, git_in): + base = git_in(repo, "rev-parse", "HEAD") + commit(git_in, repo, "line1\nline2 uplifted\nline3\n") + + assert verify_uplift(repo, base, RESOLVED) == [], ( + "A clean tree with a new commit is what a good run leaves behind." + ) + + +def test_a_failed_run_with_nothing_to_show_passes(repo, git_in): + base = git_in(repo, "rev-parse", "HEAD") + + assert verify_uplift(repo, base, Report(resolved=False)) == [], ( + "A run reporting failure that produced nothing is self-consistent; " + "there is no patch to guard." + ) + + +def test_a_claimed_resolution_with_no_commits_fails(repo, git_in): + base = git_in(repo, "rev-parse", "HEAD") + + problems = verify_uplift(repo, base, RESOLVED) + + assert any("no commits were produced" in problem for problem in problems), ( + "A resolution with an unmoved HEAD has no patch behind it." + ) + + +def test_a_rewound_head_fails(repo, git_in): + """The runtime collects `base..HEAD`, which is empty once HEAD moves back.""" + commit(git_in, repo, "line1\nline2 second\nline3\n", "second") + base = git_in(repo, "rev-parse", "HEAD") + git_in(repo, "reset", "--hard", "-q", "HEAD~") + + problems = verify_uplift(repo, base, RESOLVED) + + assert any("no longer descends" in problem for problem in problems), ( + "HEAD differing from the base is not enough: it can differ by moving " + "backwards, which collects to no patch at all." + ) + + +def test_an_empty_commit_fails(repo, git_in): + base = git_in(repo, "rev-parse", "HEAD") + git_in(repo, "commit", "-q", "--allow-empty", "-m", "nothing at all") + + problems = verify_uplift(repo, base, RESOLVED) + + assert any("change nothing" in problem for problem in problems), ( + "An empty commit moves HEAD without uplifting anything." + ) + + +def test_uncommitted_work_fails(repo, git_in): + base = git_in(repo, "rev-parse", "HEAD") + commit(git_in, repo, "line1\nline2 uplifted\nline3\n") + (repo / "f.txt").write_text("line1\nline2 forgotten\nline3\n") + + problems = verify_uplift(repo, base, RESOLVED) + + assert any("uncommitted" in problem for problem in problems), ( + "The runtime sweeps uncommitted work into a commit owned by the " + "container, which loses the author and is refused at landing time." + ) + + +def test_a_conflicted_index_fails(repo, git_in): + base = conflicted_cherry_pick(git_in, repo) + + problems = verify_uplift(repo, base, RESOLVED) + + assert any("unresolved conflicts" in problem for problem in problems), ( + "A path still in a conflicted state must not pass as resolved." + ) + assert any("cherry-pick left unfinished" in problem for problem in problems), ( + "The tree is mid-operation rather than at a finished commit." + ) + + +def test_an_unfinished_cherry_pick_fails_even_with_a_clean_index(repo, git_in): + """Conflicts staged but never committed: the index is clean, the pick is not.""" + base = conflicted_cherry_pick(git_in, repo) + (repo / "f.txt").write_text("line1\nline2 resolved\nline3\n") + git_in(repo, "add", "-A") + + problems = verify_uplift(repo, base, RESOLVED) + + assert any("cherry-pick left unfinished" in problem for problem in problems), ( + "Staging a resolution is not committing it; the pick is still open." + ) + + +def test_a_report_claiming_resolved_while_listing_unresolved_fails(repo, git_in): + base = git_in(repo, "rev-parse", "HEAD") + commit(git_in, repo, "line1\nline2 uplifted\nline3\n") + + problems = verify_uplift( + repo, + base, + Report(resolved=True, unresolved=["a.cpp: feature absent on the branch"]), + ) + + assert any("claims resolved while listing" in problem for problem in problems), ( + "`resolved` and a non-empty `unresolved` contradict each other." + ) From cfa08d06d8b283aa88758216853942f6c3b6f9fa Mon Sep 17 00:00:00 2001 From: Connor Sheehan Date: Thu, 17 Sep 2026 22:31:13 -0400 Subject: [PATCH 4/9] Bug 2051452: Add uplift agent orchestration and pure helpers A single-stage claude-agent-sdk run: ask each source to materialize what it needs on disk, render the task against the result, drive the session, then check the checkout and report what happened. The runtime collects the resolved commits separately into `changes/changes.patch`. The per-kind differences live on the source models, so this module only zips sources against their fetch results and lets each render itself. Diffs come from the broker's read-only Conduit proxy rather than arriving as input. The result records `base_commit` and `requested_sources`, so a reviewer can see what the run was applied onto and what each source resolved to without re-deriving it. A `resolved` the checks disagree with is overruled to `false`, with the reasons in `verification_failures`. `report.json` is published with that verified outcome rather than the agent's claim, which would otherwise contradict the run summary; what the agent wrote is published verbatim beside it as `report.unverified.json`. No Firefox build tools: resolving a conflict is a source-level judgment, and a build would need the `mach bootstrap` toolchain `build-repair` carries. `setting_sources` loads `project` so the agent reads the checkout's own `CLAUDE.md` and in-tree skills; `local` names a gitignored file a fresh clone cannot have. `effort` is omitted unless a run supplies one, since the API already defaults to `high`. `run_session` is a seam, so `run_uplift` is driven end to end against a real checkout the stand-in session commits into -- including once all the way through, resolving a genuine conflict and reapplying the collected patch to a clean clone, since a patch that will not land is the one way the run can look successful and be useless. Issue: https://github.com/mozilla/bugbug/issues/6865 --- .../uplift_merge_conflict_resolver/agent.py | 416 ++++++++++++++++++ agents/uplift/tests/test_uplift_diffs.py | 244 ++++++++++ agents/uplift/tests/test_uplift_options.py | 101 +++++ .../tests/test_uplift_patch_roundtrip.py | 198 +++++++++ agents/uplift/tests/test_uplift_prompt.py | 170 +++++++ agents/uplift/tests/test_uplift_report.py | 93 ++++ agents/uplift/tests/test_uplift_run.py | 210 +++++++++ 7 files changed, 1432 insertions(+) create mode 100644 agents/uplift/hackbot_agents/uplift_merge_conflict_resolver/agent.py create mode 100644 agents/uplift/tests/test_uplift_diffs.py create mode 100644 agents/uplift/tests/test_uplift_options.py create mode 100644 agents/uplift/tests/test_uplift_patch_roundtrip.py create mode 100644 agents/uplift/tests/test_uplift_prompt.py create mode 100644 agents/uplift/tests/test_uplift_report.py create mode 100644 agents/uplift/tests/test_uplift_run.py diff --git a/agents/uplift/hackbot_agents/uplift_merge_conflict_resolver/agent.py b/agents/uplift/hackbot_agents/uplift_merge_conflict_resolver/agent.py new file mode 100644 index 0000000000..7e7f95d139 --- /dev/null +++ b/agents/uplift/hackbot_agents/uplift_merge_conflict_resolver/agent.py @@ -0,0 +1,416 @@ +"""Uplift conflict-resolution agent. + +A single-stage claude-agent-sdk agent that reproduces a failed uplift +cherry-pick on a stable branch and resolves the conflicts. The entrypoint +checks the tree out and the runtime collects the resolved commits into +``changes.patch``; this module orchestrates the session and reports what +happened for human review. +""" + +import json +import logging +import tempfile +from collections.abc import Callable +from pathlib import Path + +from agent_tools import mozilla_vcs, searchfox +from agent_tools.claude_sdk import build_sdk_server +from agent_tools.mozilla_vcs import MozillaVcsContext +from agent_tools.searchfox import SearchfoxContext +from claude_agent_sdk import ( + ClaudeAgentOptions, + ClaudeSDKClient, + McpServerConfig, + ResultMessage, +) +from hackbot_runtime import AgentError, HackbotAgentResult +from hackbot_runtime.claude import Reporter +from phabricator_client import PhabricatorClient, PhabricatorSettings +from searchfox import AsyncSearchfoxClient + +from .config import ( + MODEL, + ConflictReport, + FetchedDiff, + Report, + RequestedSource, + UpliftSource, +) +from .verify import head_commit, verify_uplift + +logger = logging.getLogger(__name__) + +HERE = Path(__file__).resolve().parent + +# Where the broker mounts its read-only Conduit proxy. +PROXY_MOUNT = "/phabricator" + +# Not a secret: the proxy discards it and substitutes the real Conduit key. +# Sized to the 32 characters `PhabricatorSettings` requires. +PROXY_API_TOKEN = "hackbot-broker-proxy-placeholder" + + +class UpliftResult(HackbotAgentResult): + """Outcome of an uplift run, saved to ``summary.json`` under ``findings``. + + The patch itself is collected separately into ``changes.patch``. + """ + + # The stable branch the patches were uplifted onto. + target_branch: str + + # The commit they were applied onto. A branch name moves, so this is what + # makes the run reproducible. + base_commit: str = "" + + # Originating Bugzilla bug, when one was provided as context. + bug_id: int | None = None + + # The sources as requested, in order, and what each resolved to. + requested_sources: list[RequestedSource] = [] + + # Whether every source applied and every conflict was resolved. + resolved: bool = False + + # The agent's own grade: high/medium/low. + confidence: str | None = None + + # Developer-facing summary of what conflicted and how it was resolved. + summary: str = "" + + conflicts: list[ConflictReport] = [] + unresolved: list[str] = [] + + # What the checks found wrong regardless of what the agent claimed. + # Non-empty means `resolved` was forced to `False`. + verification_failures: list[str] = [] + + +async def run_uplift( + *, + bugbug_mcp_server: McpServerConfig, + broker_url: str, + source_repo: Path, + target_branch: str, + sources: list[UpliftSource], + target_commit: str | None = None, + bug_id: int | None = None, + model: str | None = None, + max_turns: int | None = None, + effort: str | None = None, + verbose: bool = False, + log: Path | None = None, + publish_file: Callable[[str, Path, str | None], str] | None = None, +) -> UpliftResult: + """Resolve the uplift merge conflicts for ``sources`` onto ``target_branch``. + + Returns an :class:`UpliftResult`; raises :class:`AgentError` if the agent + ends in an error or produces no result message. + """ + if not sources: + raise AgentError("no sources to uplift") + + logger.info("resolving uplift of %d source(s) onto %s", len(sources), target_branch) + + # Where the agent starts: what the checks compare against afterwards. + base_commit = head_commit(source_repo) + if target_commit and base_commit != target_commit: + raise AgentError( + f"checkout is at {base_commit}, not the requested {target_commit}" + ) + + scratch_dir = Path(tempfile.mkdtemp(prefix="uplift-")) + scratch_out = scratch_dir / "out" + scratch_out.mkdir(parents=True, exist_ok=True) + + # Materialize the diffs first, so the prompt names the paths they landed at. + fetched = await fetch_source_diffs( + build_phabricator_client(broker_url), sources, scratch_out + ) + user_prompt = build_user_prompt( + target_branch=target_branch, + sources=sources, + bug_id=bug_id, + scratch_out=scratch_out, + fetched=fetched, + ) + options = build_options( + system_prompt=load_system_prompt(), + source_repo=source_repo, + scratch_out=scratch_out, + mcp_servers=build_mcp_servers(bugbug_mcp_server), + model=model, + effort=effort, + max_turns=max_turns, + ) + + with Reporter(verbose=verbose, log_path=log) as reporter: + reporter.header(f"uplift onto {target_branch}") + result_msg = await run_session(reporter, options, user_prompt) + + check_result(result_msg, target_branch) + + report = read_agent_report(scratch_out, publish_file) + + # `Report`'s fields are exactly the result's report-derived ones. + report_fields = report.model_dump() + failures = verify_uplift(source_repo, base_commit, report) + if failures: + logger.warning( + "uplift onto %s did not pass verification: %s", + target_branch, + "; ".join(failures), + ) + report_fields["resolved"] = False + + result = UpliftResult( + target_branch=target_branch, + base_commit=base_commit, + bug_id=bug_id, + requested_sources=describe_requested(sources, fetched), + num_turns=result_msg.num_turns, + total_cost_usd=result_msg.total_cost_usd, + verification_failures=failures, + **report_fields, + ) + publish_verified_report(scratch_out, publish_file, result) + return result + + +def describe_requested( + sources: list[UpliftSource], fetched: list[FetchedDiff | None] +) -> list[RequestedSource]: + """What each requested source resolved to, in order. + + Reads the lists the prompt was rendered from, so it names the diff that was + actually fetched -- which an unpinned source does not. + """ + return [ + RequestedSource( + source=source.model_dump(), + diff_id=diff.diff_id if diff is not None else None, + base_commit=diff.base_commit if diff is not None else None, + author=diff.author if diff is not None else None, + ) + for source, diff in zip(sources, fetched, strict=True) + ] + + +def build_mcp_servers( + bugbug_mcp_server: McpServerConfig, +) -> dict[str, McpServerConfig]: + """The bugbug server, plus in-process Searchfox and HGMO lookups. + + No Firefox build tools: resolving a conflict is a source-level judgment, + and a build would need the toolchain image `build-repair` carries. + """ + return { + "bugbug": bugbug_mcp_server, + "searchfox": build_sdk_server( + "searchfox", + SearchfoxContext(client=AsyncSearchfoxClient()), + searchfox.TOOLS, + ), + "mozilla_vcs": build_sdk_server( + "mozilla_vcs", MozillaVcsContext(), mozilla_vcs.TOOLS + ), + } + + +def build_options( + *, + system_prompt: str, + source_repo: Path, + scratch_out: Path, + mcp_servers: dict[str, McpServerConfig], + model: str | None = None, + effort: str | None = None, + max_turns: int | None = None, +) -> ClaudeAgentOptions: + """Assemble the agent's SDK options to mirror a local Claude Code session. + + The container is the sandbox, so the agent runs unattended with every + built-in tool; only `AskUserQuestion` goes, since nobody can answer. + `setting_sources` loads `project` for the checkout's own `CLAUDE.md` and + in-tree skills, but not `local`, which a fresh clone cannot have. + """ + return ClaudeAgentOptions( + system_prompt=system_prompt, + model=model or MODEL, + cwd=str(source_repo), + add_dirs=[str(scratch_out)], + mcp_servers=mcp_servers, + disallowed_tools=["AskUserQuestion"], + permission_mode="bypassPermissions", + max_turns=max_turns, + setting_sources=["project"], + # Omitted rather than defaulted, as in `bug-fix`: the API's own default + # effort is `high`, so naming it here would only duplicate it. + **({"effort": effort} if effort else {}), + ) + + +def load_system_prompt() -> str: + """How to resolve an uplift, which is the same for every run. + + Deliberately free of per-run detail: that belongs in the task, and it keeps + this an identical prefix across runs for prompt caching to reuse. + """ + return (HERE / "prompts" / "system.md").read_text() + + +def build_user_prompt( + *, + target_branch: str, + sources: list[UpliftSource], + bug_id: int | None, + scratch_out: Path, + fetched: list[FetchedDiff | None], +) -> str: + """The run's own task: what to uplift, onto what, and where to report it.""" + template = (HERE / "prompts" / "task.md").read_text() + return template.format( + target_branch=target_branch, + sources_block=render_sources(sources, fetched), + bug_block=render_bug_block(bug_id), + scratch_out=str(scratch_out), + ) + + +def render_sources( + sources: list[UpliftSource], fetched: list[FetchedDiff | None] +) -> str: + """Describe each source as a numbered work item, in order. + + ``fetched`` is positional: one entry per source, ``None`` for a kind with + nothing to fetch. + """ + return "\n".join( + source.render_work_item(index, diff) + for index, (source, diff) in enumerate( + zip(sources, fetched, strict=True), start=1 + ) + ) + + +def render_bug_block(bug_id: int | None) -> str: + """The optional originating-bug section of the prompt; empty when no bug.""" + if bug_id is None: + return "" + return ( + f"## Originating bug\n\n" + f"These patches belong to bug {bug_id}. Consult it with `get_bugzilla_bug` " + f"when a conflict's intent is unclear.\n\n" + ) + + +def build_phabricator_client(broker_url: str) -> PhabricatorClient: + """A Conduit client pointed at the broker's read-only proxy.""" + return PhabricatorClient( + PhabricatorSettings( + url=f"{broker_url.rstrip('/')}{PROXY_MOUNT}", + api_key=PROXY_API_TOKEN, + ) + ) + + +async def fetch_source_diffs( + client: PhabricatorClient, + sources: list[UpliftSource], + scratch_out: Path, +) -> list[FetchedDiff | None]: + """Materialize whatever each source needs on disk, one entry per source. + + A git source is cherry-picked from the repo and contributes ``None``. + """ + fetched: list[FetchedDiff | None] = [] + for source in sources: + diff = await source.fetch_diff(client, scratch_out) + if diff is not None: + logger.info("fetched %s", diff.path.name) + fetched.append(diff) + return fetched + + +async def run_session( + reporter: Reporter, options: ClaudeAgentOptions, prompt: str +) -> ResultMessage | None: + """Drive one agent session to completion and return its result message. + + Separate from :func:`run_uplift` so a test can stand in for the session. + """ + result_msg: ResultMessage | None = None + async with ClaudeSDKClient(options=options) as client: + await client.query(prompt) + async for msg in client.receive_response(): + reporter.message(msg) + if isinstance(msg, ResultMessage): + result_msg = msg + return result_msg + + +def check_result(result_msg: ResultMessage | None, target_branch: str) -> None: + """Raise :class:`AgentError` if the agent produced no result or errored.""" + if result_msg is None: + raise AgentError(f"uplift onto {target_branch}: agent produced no result") + if result_msg.is_error: + raise AgentError( + f"uplift onto {target_branch} failed: " + f"{result_msg.result or result_msg.subtype}" + ) + + +def read_agent_report( + scratch_out: Path, + publish_file: Callable[[str, Path, str | None], str] | None, +) -> Report: + """Parse the report the agent wrote, publishing it as it wrote it. + + Published as ``report.unverified.json``, since its `resolved` is the + agent's claim and the checks may overrule it. A missing or unparseable + report is an unresolved run rather than a crash; the patch is collected + separately and is worth surfacing either way. + """ + summary = scratch_out / "summary.md" + if publish_file is not None and summary.exists(): + publish_file("summary.md", summary, "text/markdown") + + written = scratch_out / "report.json" + if not written.exists(): + logger.warning("agent wrote no report.json") + return Report() + if publish_file is not None: + publish_file("report.unverified.json", written, "application/json") + try: + return Report.model_validate_json(written.read_text()) + except ValueError as exc: + logger.warning("report.json did not validate: %s", exc) + return Report() + + +def publish_verified_report( + scratch_out: Path, + publish_file: Callable[[str, Path, str | None], str] | None, + result: UpliftResult, +) -> None: + """Publish ``report.json``, whose `resolved` is the one the checks allow. + + This is the report a consumer reads, so it has to agree with the run + summary rather than repeat a claim the checks rejected. The agent's own + file stays published beside it, unchanged. + """ + if publish_file is None: + return + body = result.model_dump( + include={ + "resolved", + "confidence", + "summary", + "conflicts", + "unresolved", + "verification_failures", + } + ) + path = scratch_out / "report.verified.json" + path.write_text(json.dumps(body, indent=2)) + publish_file("report.json", path, "application/json") diff --git a/agents/uplift/tests/test_uplift_diffs.py b/agents/uplift/tests/test_uplift_diffs.py new file mode 100644 index 0000000000..9fa0d6684d --- /dev/null +++ b/agents/uplift/tests/test_uplift_diffs.py @@ -0,0 +1,244 @@ +"""Tests for the Phabricator diff fetch, the one filesystem side effect of setup. + +Conduit is stubbed and the filesystem is real: the point is which diff is +asked for and where it lands, not HTTP. +""" + +from __future__ import annotations + +import pytest +from hackbot_agents.uplift_merge_conflict_resolver.agent import ( + build_phabricator_client, + describe_requested, + fetch_source_diffs, +) +from hackbot_agents.uplift_merge_conflict_resolver.config import ( + GitSource, + PhabricatorSource, +) +from hackbot_runtime import AgentError +from phabricator_client import UnresolvedCommitError + + +class StubConduit: + """A `PhabricatorClient` stand-in over a fake `differential.querydiffs`. + + Stubbed at the client level; `phabricator-client` already tests the call + against real envelopes. The envelope *shape* is faithful, though -- keyed + by stringified id, with `id` a string too -- which is what makes the + `int()` coercion in `resolve_diff` load-bearing. ``diffs`` maps a revision + id to the diff ids it has, newest last. + """ + + def __init__( + self, + diffs: dict[int, list[int]] | None = None, + without_author: set[int] | None = None, + unresolvable: set[str] | None = None, + ) -> None: + self.diffs = diffs or {} + self.without_author = without_author or set() + self.unresolvable = unresolvable or set() + self.raw_diff_calls: list[int] = [] + self.resolve_calls: list[str] = [] + + async def conduit_request(self, method: str, **payload): + assert method == "differential.querydiffs", ( + "The agent should resolve diffs through `differential.querydiffs`." + ) + (revision_id,) = payload["revisionIDs"] + return { + str(diff_id): self._raw(diff_id) + for diff_id in self.diffs.get(revision_id, []) + } + + def _raw(self, diff_id: int) -> dict: + raw: dict = { + "id": str(diff_id), + "sourceControlBaseRevision": f"base{diff_id}", + } + if diff_id not in self.without_author: + raw["authorName"] = f"Author {diff_id}" + raw["authorEmail"] = f"author{diff_id}@example.com" + return raw + + async def resolve_commit(self, ref: str) -> str: + """Expand an abbreviated base hash, as `diffusion.querycommits` does.""" + self.resolve_calls.append(ref) + if ref in self.unresolvable: + raise UnresolvedCommitError(f"cannot resolve {ref}") + return ref.ljust(40, "0") + + async def get_raw_diff(self, diff_id: int) -> str: + self.raw_diff_calls.append(diff_id) + return f"diff for {diff_id}" + + +async def test_writes_fetched_diff_to_expected_path(tmp_path): + client = StubConduit(diffs={9: [400]}) + source = PhabricatorSource(revision_id=9) + + fetched = await fetch_source_diffs(client, [source], tmp_path) + + expected = source.diff_path(tmp_path, 400) + assert [entry.path for entry in fetched] == [expected], ( + "The written path should match the source's own `diff_path`." + ) + assert expected.read_text() == "diff for 400", ( + "The file should hold the diff Conduit returned." + ) + + +async def test_two_pins_on_one_revision_do_not_share_a_file(tmp_path): + """Every source is fetched up front, so a shared filename loses a pin.""" + client = StubConduit(diffs={9: [10, 20]}) + sources = [ + PhabricatorSource(revision_id=9, diff_id=10), + PhabricatorSource(revision_id=9, diff_id=20), + ] + + fetched = await fetch_source_diffs(client, sources, tmp_path) + + assert [entry.path.read_text() for entry in fetched] == [ + "diff for 10", + "diff for 20", + ], "Each source must read the diff it pinned, not whichever was written last." + + +async def test_pinned_diff_id_is_used_verbatim(tmp_path): + client = StubConduit(diffs={9: [317, 400]}) + + await fetch_source_diffs( + client, [PhabricatorSource(revision_id=9, diff_id=317)], tmp_path + ) + + assert client.raw_diff_calls == [317], ( + "A pinned `diff_id` should be fetched instead of the revision's latest." + ) + + +async def test_missing_diff_id_falls_back_to_the_latest(): + client = StubConduit(diffs={9: [317, 400]}) + + diff = await PhabricatorSource(revision_id=9).resolve_diff(client) + + assert diff.id == 400, "Without a pin, the revision's newest diff is used." + + +async def test_revision_without_any_diff_is_an_agent_error(): + client = StubConduit(diffs={}) + + with pytest.raises(AgentError, match="D9 has no diffs"): + await PhabricatorSource(revision_id=9).resolve_diff(client) + + +async def test_pinned_diff_from_another_revision_is_an_agent_error(): + client = StubConduit(diffs={9: [400]}) + + with pytest.raises(AgentError, match="does not belong to D9"): + await PhabricatorSource(revision_id=9, diff_id=123).resolve_diff(client) + + +async def test_author_travels_with_the_diff(tmp_path): + client = StubConduit(diffs={9: [400]}) + + fetched = await fetch_source_diffs( + client, [PhabricatorSource(revision_id=9)], tmp_path + ) + + assert fetched[0].author == "Author 400 ", ( + "The uplift commit needs the original author, as `Name `." + ) + + +async def test_base_commit_is_expanded_to_a_full_hash(tmp_path): + client = StubConduit(diffs={9: [400]}) + + fetched = await fetch_source_diffs( + client, [PhabricatorSource(revision_id=9)], tmp_path + ) + + assert client.resolve_calls == ["base400"], ( + "The recorded base should be expanded through Conduit before being used." + ) + assert fetched[0].base_commit == "base400".ljust(40, "0"), ( + "moz-phab records an abbreviated base and `git fetch` refuses one, so " + "the full hash is what reaches the prompt." + ) + + +async def test_an_unexpandable_base_commit_is_reported_as_none(tmp_path): + client = StubConduit(diffs={9: [400]}, unresolvable={"base400"}) + + fetched = await fetch_source_diffs( + client, [PhabricatorSource(revision_id=9)], tmp_path + ) + + assert fetched[0].base_commit is None, ( + "A base that cannot be expanded should send the agent down the prompt's " + "fallback, not fail the run over a three-way merge hint." + ) + + +async def test_requested_sources_record_what_each_input_resolved_to(tmp_path): + """An unpinned input does not say which diff ran, so the result records it.""" + client = StubConduit(diffs={9: [317, 400]}) + sources = [GitSource(commit="abc"), PhabricatorSource(revision_id=9)] + + requested = describe_requested( + sources, await fetch_source_diffs(client, sources, tmp_path) + ) + + assert [entry.source for entry in requested] == [ + {"kind": "git", "commit": "abc"}, + {"kind": "phabricator", "revision_id": 9, "diff_id": None}, + ], "Each source should be recorded as the caller gave it." + assert requested[1].diff_id == 400, ( + "The diff actually fetched should be recorded, not the empty pin." + ) + assert requested[1].base_commit == "base400".ljust(40, "0"), ( + "The expanded base commit belongs in the record a reviewer reads." + ) + assert requested[1].author == "Author 400 ", ( + "Who the uplift is attributed to should be recorded too." + ) + assert (requested[0].diff_id, requested[0].base_commit) == (None, None), ( + "A git source has no diff to resolve; it is cherry-picked as-is." + ) + + +async def test_git_sources_fetch_nothing(tmp_path): + client = StubConduit() + + fetched = await fetch_source_diffs(client, [GitSource(commit="abc")], tmp_path) + + assert fetched == [None], ( + "A git source is cherry-picked, so it holds a slot but fetches nothing." + ) + assert client.raw_diff_calls == [], "A git source should not reach Conduit." + + +async def test_a_stack_is_fetched_in_order(tmp_path): + client = StubConduit(diffs={1: [10], 2: [88, 99]}) + sources = [ + GitSource(commit="aaa"), + PhabricatorSource(revision_id=1), + PhabricatorSource(revision_id=2, diff_id=88), + ] + + fetched = await fetch_source_diffs(client, sources, tmp_path) + + assert [entry is None for entry in fetched] == [True, False, False], ( + "The result is positional, so the git source keeps its slot." + ) + assert client.raw_diff_calls == [10, 88], ( + "Each revision resolves to its own diff, pinned or latest, in stack order." + ) + + +def test_client_points_at_the_brokers_proxy_mount(): + client = build_phabricator_client("http://uplift-broker:8765/") + + assert client.base_url == "http://uplift-broker:8765/phabricator", ( + "The client should target the broker's proxy mount, with no double slash." + ) diff --git a/agents/uplift/tests/test_uplift_options.py b/agents/uplift/tests/test_uplift_options.py new file mode 100644 index 0000000000..f7fb6d7a75 --- /dev/null +++ b/agents/uplift/tests/test_uplift_options.py @@ -0,0 +1,101 @@ +"""Tests for ``build_options`` and ``check_result``. + +Both are pure: the options object is asserted on without running anything, and +the result guard is fed lightweight stand-in result objects. +""" + +from __future__ import annotations + +from pathlib import Path +from types import SimpleNamespace + +import pytest +from hackbot_agents.uplift_merge_conflict_resolver.agent import ( + build_options, + check_result, +) +from hackbot_agents.uplift_merge_conflict_resolver.config import MODEL + +MCP_SERVERS = {"bugbug": {"type": "http", "url": "http://localhost:8080/mcp"}} + + +def make_options(**overrides): + kwargs = dict( + system_prompt="SYSTEM", + source_repo=Path("/repo"), + scratch_out=Path("/scratch"), + mcp_servers=MCP_SERVERS, + ) + kwargs.update(overrides) + return build_options(**kwargs) + + +def test_options_default_model(): + options = make_options() + assert options.model == MODEL, "Without an override, the configured model is used." + + +def test_options_leave_effort_to_the_api_default(): + options = make_options() + assert options.effort is None, ( + "Without an override, `effort` is omitted so the API's own default applies." + ) + + +def test_options_accept_overrides(): + options = make_options(model="custom-model", effort="low", max_turns=12) + assert options.model == "custom-model", ( + "An explicit model should override the default." + ) + assert options.effort == "low", "An explicit effort should override the default." + assert options.max_turns == 12, "`max_turns` should be passed through." + + +def test_options_load_project_settings_like_a_local_session(): + options = make_options() + assert options.setting_sources == ["project"], ( + "`project` loads `CLAUDE.md` and the in-tree skills; `local` would only " + "name a gitignored file a fresh clone cannot have." + ) + + +def test_options_run_unattended_with_every_tool_but_questions(): + options = make_options() + assert options.permission_mode == "bypassPermissions", ( + "Nobody is present to approve a tool call; the container is the sandbox." + ) + assert "AskUserQuestion" in options.disallowed_tools, ( + "Interactive questions should be disabled in an unattended run." + ) + assert "Task" not in options.disallowed_tools, ( + "Sub-agents stay enabled, like a local Claude Code session." + ) + + +def test_check_result_raises_when_no_result(): + with pytest.raises(Exception) as excinfo: + check_result(None, "beta") + assert "no result" in str(excinfo.value), "A missing result should explain itself." + + +def test_check_result_raises_on_error_with_message(): + failed = SimpleNamespace(is_error=True, result="boom", subtype="error_max_turns") + with pytest.raises(Exception) as excinfo: + check_result(failed, "beta") + assert "boom" in str(excinfo.value), ( + "The failure message should surface in the error." + ) + + +def test_check_result_falls_back_to_subtype(): + failed = SimpleNamespace(is_error=True, result=None, subtype="error_max_turns") + with pytest.raises(Exception) as excinfo: + check_result(failed, "beta") + assert "error_max_turns" in str(excinfo.value), ( + "With no message, the subtype should be reported instead." + ) + + +def test_check_result_passes_on_success(): + ok = SimpleNamespace(is_error=False, result="done", subtype="success") + check_result(ok, "beta") # Should not raise. diff --git a/agents/uplift/tests/test_uplift_patch_roundtrip.py b/agents/uplift/tests/test_uplift_patch_roundtrip.py new file mode 100644 index 0000000000..4fdf1c745f --- /dev/null +++ b/agents/uplift/tests/test_uplift_patch_roundtrip.py @@ -0,0 +1,198 @@ +"""End-to-end check that a resolved uplift survives being turned into a patch. + +Everything downstream consumes `changes.patch`, not the checkout, so the run is +only useful if that patch reapplies to a clean target and keeps the original +author -- Lando refuses a patch authored by hackbot. Driven against real +repositories: a genuine conflict, a committed resolution, then `collect` and +`git am` onto a fresh clone. +""" + +from __future__ import annotations + +import json +from pathlib import Path +from types import SimpleNamespace + +from hackbot_agents.uplift_merge_conflict_resolver import agent +from hackbot_agents.uplift_merge_conflict_resolver.config import GitSource +from hackbot_runtime import changes + +AUTHOR = "Original Author " +REPO_URL = "https://example.invalid/firefox.git" + +# What the patch does on trunk, what the stable branch did instead, and the +# resolution that has to keep the patch's change while fitting the branch. +ON_TRUNK = "line1\nline2 patched\nline3\n" +ON_STABLE = "line1\nline2 diverged\nline3\nline4\n" +RESOLVED = "line1\nline2 patched\nline3\nline4\n" + + +def build_upstream(git_in, path: Path) -> str: + """A remote with a patch on trunk and a diverged stable branch. + + Returns the commit to uplift. + """ + path.mkdir() + git_in(path, "init", "-q") + git_in(path, "config", "user.email", "trunk@example.com") + git_in(path, "config", "user.name", "Trunk Dev") + (path / "f.txt").write_text("line1\nline2\nline3\n") + git_in(path, "add", "-A") + git_in(path, "commit", "-qm", "base") + base = git_in(path, "rev-parse", "HEAD") + + (path / "f.txt").write_text(ON_TRUNK) + git_in(path, "add", "-A") + git_in(path, "commit", "-qm", "Bug 1: patch to uplift", f"--author={AUTHOR}") + to_uplift = git_in(path, "rev-parse", "HEAD") + + git_in(path, "checkout", "-q", "-b", "stable", base) + (path / "f.txt").write_text(ON_STABLE) + git_in(path, "add", "-A") + git_in(path, "commit", "-qm", "unrelated change on stable") + git_in(path, "checkout", "-q", "stable") + return to_uplift + + +def clone_stable(git_in, upstream: Path, path: Path) -> Path: + """A checkout of the stable branch, as the runtime prepares one.""" + git_in(path.parent, "clone", "-q", "--branch", "stable", str(upstream), path.name) + git_in(path, "config", "user.email", "agent@example.com") + git_in(path, "config", "user.name", "Hackbot Agent") + return path + + +def test_a_resolved_uplift_reapplies_to_a_clean_target(tmp_path, git_in): + upstream = tmp_path / "upstream" + to_uplift = build_upstream(git_in, upstream) + checkout = clone_stable(git_in, upstream, tmp_path / "checkout") + base = git_in(checkout, "rev-parse", "HEAD") + + # The conflict the agent is spun up to deal with. + git_in(checkout, "fetch", "-q", "origin", to_uplift) + git_in(checkout, "cherry-pick", "-x", to_uplift, check=False) + assert git_in(checkout, "diff", "--name-only", "--diff-filter=U") == "f.txt", ( + "The setup should leave a real conflict, or this proves nothing." + ) + + # The session's work: resolve, then finish the pick, which keeps the author. + (checkout / "f.txt").write_text(RESOLVED) + git_in(checkout, "add", "-A") + git_in(checkout, "-c", "core.editor=true", "cherry-pick", "--continue") + + change_set = changes.collect(checkout, base, REPO_URL) + assert change_set is not None, "A resolved uplift should collect to a patch." + assert change_set.metadata["wrapped_uncommitted"] is False, ( + "The resolution was committed, so nothing should need wrapping into a " + "synthetic commit owned by the container." + ) + + # What a reviewer, or Lando, does with the artifact. + target = clone_stable(git_in, upstream, tmp_path / "target") + mbox = tmp_path / "changes.patch" + mbox.write_bytes(change_set.patch) + git_in(target, "am", str(mbox)) + + assert (target / "f.txt").read_text() == RESOLVED, ( + "Reapplying the collected patch should reproduce the resolution." + ) + assert git_in(target, "log", "-1", "--format=%an <%ae>") == AUTHOR, ( + "The uplift has to keep the original author: Lando refuses a patch " + "authored by hackbot." + ) + + +def test_an_uncommitted_resolution_is_flagged_as_wrapped(tmp_path, git_in): + """What the prompt warns about: uncommitted work loses the original author.""" + upstream = tmp_path / "upstream" + to_uplift = build_upstream(git_in, upstream) + checkout = clone_stable(git_in, upstream, tmp_path / "checkout") + base = git_in(checkout, "rev-parse", "HEAD") + + git_in(checkout, "fetch", "-q", "origin", to_uplift) + git_in(checkout, "cherry-pick", "-x", to_uplift, check=False) + (checkout / "f.txt").write_text(RESOLVED) + + change_set = changes.collect(checkout, base, REPO_URL) + + assert change_set is not None, "There is still work to collect." + assert change_set.metadata["wrapped_uncommitted"] is True, ( + "Left uncommitted, the resolution is swept into a container-authored " + "commit -- which is why the run refuses to call this resolved." + ) + + +async def test_a_run_uplift_resolution_reapplies( + tmp_path, git_in, monkeypatch, publisher +): + """The whole path in one go: run the agent, then land what it produced. + + The stand-in session does what the prompt asks for real -- fetch, pick, + resolve, finish the pick -- so the run's own checks, the base commit it + records, and the patch collected from it are all exercised together. + """ + upstream = tmp_path / "upstream" + to_uplift = build_upstream(git_in, upstream) + checkout = clone_stable(git_in, upstream, tmp_path / "checkout") + + async def session(reporter, options, prompt): + repo = Path(options.cwd) + git_in(repo, "fetch", "-q", "origin", to_uplift) + git_in(repo, "cherry-pick", "-x", to_uplift, check=False) + assert git_in(repo, "diff", "--name-only", "--diff-filter=U") == "f.txt", ( + "The uplift should conflict, or the run has nothing to resolve." + ) + (repo / "f.txt").write_text(RESOLVED) + git_in(repo, "add", "-A") + git_in(repo, "-c", "core.editor=true", "cherry-pick", "--continue") + Path(options.add_dirs[0], "report.json").write_text( + json.dumps( + { + "resolved": True, + "confidence": "high", + "summary": "Kept the patch's change alongside the branch's.", + } + ) + ) + return SimpleNamespace( + is_error=False, + result="done", + subtype="success", + num_turns=3, + total_cost_usd=0.1, + ) + + monkeypatch.setattr(agent, "run_session", session) + result = await agent.run_uplift( + bugbug_mcp_server={"type": "http", "url": "http://localhost:8080/mcp"}, + broker_url="http://localhost:8765", + source_repo=checkout, + target_branch="stable", + sources=[GitSource(commit=to_uplift)], + publish_file=publisher, + ) + + assert (result.resolved, result.verification_failures) == (True, []), ( + "A real resolution, committed, should pass the checks." + ) + assert json.loads(publisher.bodies["report.json"])["resolved"] is True, ( + "The published report should agree with it." + ) + + change_set = changes.collect(checkout, result.base_commit, REPO_URL) + assert change_set is not None, ( + "The base commit the run recorded should be what the patch is collected " + "against." + ) + + target = clone_stable(git_in, upstream, tmp_path / "target") + mbox = tmp_path / "changes.patch" + mbox.write_bytes(change_set.patch) + git_in(target, "am", str(mbox)) + + assert (target / "f.txt").read_text() == RESOLVED, ( + "The run's patch should reproduce its resolution on a clean target." + ) + assert git_in(target, "log", "-1", "--format=%an <%ae>") == AUTHOR, ( + "And keep the original author, which Lando refuses a patch without." + ) diff --git a/agents/uplift/tests/test_uplift_prompt.py b/agents/uplift/tests/test_uplift_prompt.py new file mode 100644 index 0000000000..ca9eedb211 --- /dev/null +++ b/agents/uplift/tests/test_uplift_prompt.py @@ -0,0 +1,170 @@ +"""Tests for prompt rendering. + +All pure: these turn inputs into prompt text and never touch the filesystem, so +``scratch_out`` is passed as a value. Diff writing is covered in +``test_uplift_diffs``. +""" + +from __future__ import annotations + +import re +from pathlib import Path + +import pytest +from hackbot_agents.uplift_merge_conflict_resolver.agent import ( + build_user_prompt, + load_system_prompt, + render_bug_block, + render_sources, +) +from hackbot_agents.uplift_merge_conflict_resolver.config import ( + FetchedDiff, + GitSource, + PhabricatorSource, +) +from hackbot_runtime import AgentError + +SCRATCH = Path("/scratch/out") +BASE = "bbb2222" + + +def fetched_for( + *sources, + author: str | None = "Dev ", + base_commit: str | None = BASE, +): + """One entry per source, as `render_sources` reads them. + + A `GitSource` has nothing to fetch, so it contributes `None`. + """ + return [ + None + if isinstance(source, GitSource) + else FetchedDiff( + path=source.diff_path(SCRATCH, source.diff_id or 1), + author=author, + base_commit=base_commit, + ) + for source in sources + ] + + +def test_render_sources_describes_a_mixed_stack(): + """One work item per source, in order, each with what its kind needs.""" + sources = [ + GitSource(commit="aaa1111"), + PhabricatorSource(revision_id=88, diff_id=500), + ] + + git_line, phab_line = render_sources(sources, fetched_for(*sources)).splitlines() + + assert git_line.startswith("1."), "Work items are numbered in the order applied." + assert "`aaa1111`" in git_line, "A git source names the commit to pick." + assert "cherry-pick" in git_line, "A git source is applied as a cherry-pick." + assert "--author" not in git_line, ( + "`git cherry-pick` carries the author across, so no override is needed." + ) + + assert phab_line.startswith("2."), "Numbering follows input order." + assert "D88" in phab_line, "A Phabricator source names its revision." + assert str(SCRATCH / "D88-500.diff") in phab_line, ( + "It points at the path its diff was actually written to." + ) + assert f"`{BASE}`" in phab_line, ( + "It names the base commit, which `git apply --3way` needs fetched to " + "find the blobs the diff was built from." + ) + assert '--author="Dev "' in phab_line, ( + "It names the author to commit as, which the uplift has to keep." + ) + + +def test_render_sources_says_what_phabricator_did_not_record(): + """Neither field is guaranteed, and guessing either one lands a bad patch.""" + sources = [PhabricatorSource(revision_id=88)] + + text = render_sources(sources, fetched_for(*sources, author=None, base_commit=None)) + + assert "no base commit" in text, ( + "Without a base commit the agent is told, not left to wonder why the " + "three-way merge found nothing." + ) + assert "--author" in text and "did not record" in text, ( + "Without an author the agent is still told to set one, rather than " + "committing as the container." + ) + + +def test_render_sources_refuses_a_phabricator_source_that_was_not_fetched(): + # Rendering a path the diff was never written to would point the agent at a + # file that does not exist, so this is a failure rather than a guess. + with pytest.raises(AgentError, match="must be fetched"): + render_sources([PhabricatorSource(revision_id=88)], [None]) + + +def test_render_bug_block_is_present_only_when_there_is_a_bug(): + assert render_bug_block(None) == "", ( + "With no bug id, the originating-bug block should be empty." + ) + + block = render_bug_block(1500000) + assert "1500000" in block, "The bug block should name the bug id." + assert "get_bugzilla_bug" in block, "The bug block should point at the MCP tool." + + +def test_the_system_prompt_holds_no_per_run_detail(): + """Identical every run, which is what prompt caching reuses.""" + prompt = load_system_prompt() + + assert re.search(r"\{[a-z_]+\}", prompt) is None, ( + "An unrendered placeholder means per-run detail leaked into the prefix." + ) + assert prompt == load_system_prompt(), "The system prompt should not vary." + assert '"resolved": true' in prompt, ( + "The report shape is stable guidance, so it belongs here -- and with " + "nothing to `.format`, its braces need no escaping." + ) + assert "How to apply each patch" in prompt, ( + "How to apply a source is guidance, not a detail of one run." + ) + + +def test_the_task_prompt_holds_the_run(tmp_path): + sources = [GitSource(commit="abc1234"), PhabricatorSource(revision_id=77)] + + prompt = build_user_prompt( + target_branch="release", + sources=sources, + bug_id=42, + scratch_out=tmp_path, + fetched=fetched_for(*sources), + ) + + assert "release" in prompt, "The task should name the branch to uplift onto." + assert "`abc1234`" in prompt and "D77" in prompt, ( + "Both sources should reach the task, which is what the agent works from." + ) + assert "bug 42" in prompt, "The originating bug belongs with the task." + assert f"{tmp_path}/report.json" in prompt, ( + "The task names where to write the report, since the path is per-run." + ) + assert f"{tmp_path}/summary.md" in prompt, "And the summary beside it." + + +def test_the_task_prompt_omits_the_bug_section_without_a_bug(tmp_path): + sources = [GitSource(commit="abc1234")] + + prompt = build_user_prompt( + target_branch="beta", + sources=sources, + bug_id=None, + scratch_out=tmp_path, + fetched=fetched_for(*sources), + ) + + assert "Originating bug" not in prompt, ( + "With no bug there should be no empty section left behind." + ) + assert "\n\n\n\n" not in prompt, ( + "And no run of blank lines where the section would have been." + ) diff --git a/agents/uplift/tests/test_uplift_report.py b/agents/uplift/tests/test_uplift_report.py new file mode 100644 index 0000000000..a5f661244c --- /dev/null +++ b/agents/uplift/tests/test_uplift_report.py @@ -0,0 +1,93 @@ +"""Tests for reading and publishing what the agent wrote. + +`report.json` is model output, so what matters is that a plausible but wrong +value cannot get through, and that a bad report degrades to an unresolved run +rather than losing the whole one. +""" + +from __future__ import annotations + +import json + +import pytest +from hackbot_agents.uplift_merge_conflict_resolver.agent import read_agent_report +from hackbot_agents.uplift_merge_conflict_resolver.config import Report + +REPORT = { + "resolved": True, + "confidence": "high", + "summary": "Rebased the import block.", + "conflicts": [{"file": "a.cpp", "resolution": "kept both edits"}], + "unresolved": [], +} + + +def write(scratch_out, report=None, summary=None): + if report is not None: + (scratch_out / "report.json").write_text(report) + if summary is not None: + (scratch_out / "summary.md").write_text(summary) + + +def test_the_agents_report_is_published_as_unverified(tmp_path, publisher): + write(tmp_path, report=json.dumps(REPORT), summary="# resolved") + + result = read_agent_report(tmp_path, publisher) + + assert result == Report(**REPORT), ( + "The parsed report should match what was written." + ) + assert publisher.calls == [ + ("summary.md", tmp_path / "summary.md", "text/markdown"), + ("report.unverified.json", tmp_path / "report.json", "application/json"), + ], ( + "The agent's own report is published under a name that says the checks " + "have not been applied to it yet." + ) + + +@pytest.mark.parametrize( + "body, reason", + [ + ("{not json", "not JSON at all"), + ("[1, 2, 3]", "a list rather than an object"), + ('"a string"', "a bare string"), + ("", "empty"), + ('{"resolved": "false"}', "a stringified `false` `bool()` would read as true"), + ('{"resolved": true, "confidence": "very high"}', "an invented confidence"), + ], +) +def test_a_bad_report_is_never_read_as_resolved(tmp_path, body, reason): + write(tmp_path, report=body) + + assert read_agent_report(tmp_path, None) == Report(), ( + f"A report that is {reason} should fall back to unresolved." + ) + + +def test_a_report_that_cannot_be_parsed_is_still_published(tmp_path, publisher): + write(tmp_path, report="{broken") + + result = read_agent_report(tmp_path, publisher) + + assert result == Report(), "An unparseable report should fall back." + assert publisher.bodies["report.unverified.json"] == "{broken", ( + "The raw report is still published, verbatim, so a human can inspect it." + ) + + +def test_a_missing_report_is_an_unresolved_run(tmp_path, publisher): + result = read_agent_report(tmp_path, publisher) + + assert result == Report(), "A missing `report.json` should yield a bare report." + assert publisher.calls == [], ( + "Nothing should be published when nothing was written." + ) + + +def test_reading_works_without_a_publisher(tmp_path): + write(tmp_path, report='{"resolved": false}', summary="# nothing doing") + + assert read_agent_report(tmp_path, None) == Report(resolved=False), ( + "A standalone run has no uploader, and reading should not depend on one." + ) diff --git a/agents/uplift/tests/test_uplift_run.py b/agents/uplift/tests/test_uplift_run.py new file mode 100644 index 0000000000..427b196bf9 --- /dev/null +++ b/agents/uplift/tests/test_uplift_run.py @@ -0,0 +1,210 @@ +"""Tests for ``run_uplift``, the orchestration around the agent session. + +``run_session`` is stood in for, so these cover the assembly around it: the +servers and options the session is handed, and what the run makes of the tree +and report it leaves behind. The stand-in commits into a real checkout, like a +real session is told to, so the mechanical checks run rather than being +bypassed. +""" + +from __future__ import annotations + +import json +from pathlib import Path +from types import SimpleNamespace + +import pytest +from hackbot_agents.uplift_merge_conflict_resolver import agent +from hackbot_agents.uplift_merge_conflict_resolver.config import GitSource +from hackbot_runtime import AgentError + +BUGBUG_MCP = {"type": "http", "url": "http://localhost:8080/mcp"} + +REPORT = { + "resolved": True, + "confidence": "medium", + "summary": "Rebased the import block.", + "conflicts": [{"file": "a.cpp", "resolution": "kept both edits"}], + "unresolved": [], +} + + +def fake_session( + git_in, report: dict | None, *, commits: bool = True, dirty: bool = False +): + """A ``run_session`` stand-in that resolves, commits, and writes ``report``. + + The scratch directory is read off the options, which is how the real agent + learns it too. ``commits`` and ``dirty`` produce the two ways a session can + claim success while leaving the checkout unfit to land. + """ + captured: dict = {} + + async def session(reporter, options, prompt): + captured["options"] = options + captured["prompt"] = prompt + scratch_out = Path(options.add_dirs[0]) + if report is not None: + (scratch_out / "report.json").write_text(json.dumps(report)) + (scratch_out / "summary.md").write_text("# resolved") + repo = Path(options.cwd) + if commits: + (repo / "f.txt").write_text("line1\nline2 uplifted\nline3\n") + git_in(repo, "add", "-A") + git_in(repo, "commit", "-qm", "uplift the patch") + if dirty: + (repo / "f.txt").write_text("line1\nline2 forgotten\nline3\n") + return SimpleNamespace( + is_error=False, + result="done", + subtype="success", + num_turns=4, + total_cost_usd=0.25, + ) + + return session, captured + + +async def run(monkeypatch, session, repo, **overrides): + kwargs = dict( + bugbug_mcp_server=BUGBUG_MCP, + broker_url="http://uplift-broker:8765", + source_repo=repo, + target_branch="beta", + sources=[GitSource(commit="a" * 40)], + ) + kwargs.update(overrides) + monkeypatch.setattr(agent, "run_session", session) + return await agent.run_uplift(**kwargs) + + +async def test_run_uplift_reports_what_the_session_resolved( + monkeypatch, publisher, repo, git_in +): + session, _ = fake_session(git_in, REPORT) + + result = await run(monkeypatch, session, repo, publish_file=publisher) + + assert (result.resolved, result.confidence) == (True, "medium"), ( + "The report the session wrote should decide the run's outcome." + ) + assert result.verification_failures == [], ( + "A session that committed a clean resolution has nothing to flag." + ) + assert result.target_branch == "beta", "The run should name the branch it targeted." + assert result.base_commit == git_in(repo, "rev-parse", "HEAD~"), ( + "The commit the patches were applied onto is what makes a run " + "reproducible, since a branch name moves." + ) + assert [entry.source for entry in result.requested_sources] == [ + {"kind": "git", "commit": "a" * 40} + ], "The result should record which sources were requested, not just how many." + assert (result.num_turns, result.total_cost_usd) == (4, 0.25), ( + "The session's run metadata should reach the result." + ) + assert [conflict.file for conflict in result.conflicts] == ["a.cpp"], ( + "Per-file conflict resolutions should reach the result." + ) + assert publisher.names == [ + "summary.md", + "report.unverified.json", + "report.json", + ], ( + "What the agent wrote is published, and `report.json` beside it is the " + "verified one a consumer reads." + ) + + +async def test_run_uplift_refuses_a_checkout_that_is_not_the_pinned_commit( + monkeypatch, repo, git_in +): + session, _ = fake_session(git_in, REPORT) + + with pytest.raises(AgentError, match="not the requested"): + await run(monkeypatch, session, repo, target_commit="f" * 40) + + +async def test_run_uplift_wires_every_mozilla_mcp_server(monkeypatch, repo, git_in): + session, captured = fake_session(git_in, REPORT) + + await run(monkeypatch, session, repo) + + assert set(captured["options"].mcp_servers) == { + "bugbug", + "searchfox", + "mozilla_vcs", + }, "The session should get the bugbug server plus the in-process Mozilla ones." + assert "beta" in captured["prompt"], ( + "The session's prompt should name the branch being uplifted onto." + ) + + +async def test_run_uplift_refuses_to_call_a_dirty_tree_resolved( + monkeypatch, repo, git_in +): + session, _ = fake_session(git_in, REPORT, dirty=True) + + result = await run(monkeypatch, session, repo) + + assert result.resolved is False, ( + "Uncommitted work loses the patch's author, so the claim is overruled." + ) + assert any("uncommitted" in failure for failure in result.verification_failures), ( + "The reason for overruling the claim should be recorded on the result." + ) + + +async def test_the_published_report_agrees_with_the_verified_result( + monkeypatch, publisher, repo, git_in +): + """An overruled claim must not survive in the report a consumer reads.""" + session, _ = fake_session(git_in, REPORT, dirty=True) + + result = await run(monkeypatch, session, repo, publish_file=publisher) + + assert result.resolved is False, "The checks overruled the claim." + assert json.loads(publisher.bodies["report.json"]) == { + "resolved": False, + "confidence": "medium", + "summary": REPORT["summary"], + "conflicts": REPORT["conflicts"], + "unresolved": [], + "verification_failures": result.verification_failures, + }, "`report.json` should carry the verified outcome and why it was overruled." + assert json.loads(publisher.bodies["report.unverified.json"])["resolved"] is True, ( + "The agent's own claim is kept verbatim, under a name that says so." + ) + + +async def test_run_uplift_refuses_a_resolution_with_no_commits( + monkeypatch, repo, git_in +): + session, _ = fake_session(git_in, REPORT, commits=False) + + result = await run(monkeypatch, session, repo) + + assert result.resolved is False, ( + "A claimed resolution with no commits has no patch behind it." + ) + assert any("no commits" in failure for failure in result.verification_failures), ( + "The result should say why the claim was not accepted." + ) + + +async def test_run_uplift_survives_a_session_that_wrote_no_report( + monkeypatch, repo, git_in +): + session, _ = fake_session(git_in, None) + + result = await run(monkeypatch, session, repo) + + assert result.resolved is False, ( + "A session that wrote no report is an unresolved run, not a crash." + ) + + +async def test_run_uplift_refuses_a_run_with_no_sources(monkeypatch, repo, git_in): + session, _ = fake_session(git_in, REPORT) + + with pytest.raises(AgentError, match="no sources"): + await run(monkeypatch, session, repo, sources=[]) From edaf2bc6b3a2209a7633af47687546e9e64220b7 Mon Sep 17 00:00:00 2001 From: Connor Sheehan Date: Thu, 17 Sep 2026 22:31:14 -0400 Subject: [PATCH 5/9] Bug 2051452: Add the uplift agent entrypoint `AgentInputs` reads the per-run inputs the platform passes as environment variables, then `main` prepares the checkout and hands off to `run_uplift`. The checkout is pinned before anything else reads the tree, to `target_commit` when the caller supplied one and the branch tip otherwise: a branch name moves, so pinning is what reproduces the checkout a failed uplift was seen on. Empty strings count as absent, since compose passes every unset optional input as `${VAR:-}`. Issue: https://github.com/mozilla/bugbug/issues/6865 --- .../__main__.py | 77 ++++++++++ agents/uplift/tests/test_uplift_main.py | 134 ++++++++++++++++++ 2 files changed, 211 insertions(+) create mode 100644 agents/uplift/hackbot_agents/uplift_merge_conflict_resolver/__main__.py create mode 100644 agents/uplift/tests/test_uplift_main.py diff --git a/agents/uplift/hackbot_agents/uplift_merge_conflict_resolver/__main__.py b/agents/uplift/hackbot_agents/uplift_merge_conflict_resolver/__main__.py new file mode 100644 index 0000000000..c75aa14382 --- /dev/null +++ b/agents/uplift/hackbot_agents/uplift_merge_conflict_resolver/__main__.py @@ -0,0 +1,77 @@ +import logging + +from hackbot_runtime import HackbotContext, run_async +from pydantic_settings import BaseSettings, SettingsConfigDict + +from .agent import UpliftResult, run_uplift +from .config import UpliftSource + +logger = logging.getLogger(__name__) + + +class AgentInputs(BaseSettings): + """The per-run inputs, read from the environment the platform sets. + + Mirrors `UpliftInputs` in hackbot-api, which upper-cases each field name + and JSON-encodes lists. Deploy-time constants arrive the same way, from the + Job's static env rather than the caller. + """ + + # The stable branch ref to uplift onto, e.g. `release`, `beta`, `esr128`. + target_branch: str + + # The exact commit to uplift onto. A branch name moves, so a caller + # reproducing a specific uplift should pin it; otherwise the tip is used. + target_commit: str | None = None + + # The patches to apply onto the branch, in this order; a stack is several. + sources: list[UpliftSource] + + # The bugbug MCP, serving the bug, revision and fx-docs tools. + bugbug_mcp_url: str + + # The broker sidecar, whose Conduit proxy diffs are fetched through. + broker_url: str + + # The originating bug, for context when a conflict's intent is unclear. + bug_id: int | None = None + + # Overrides; `None` leaves the SDK's or the API's own default in place. + model: str | None = None + max_turns: int | None = None + effort: str | None = None + + # Compose passes unset inputs as empty strings (``${BUG_ID:-}``). + model_config = SettingsConfigDict(extra="ignore", env_ignore_empty=True) + + +async def main(ctx: HackbotContext) -> UpliftResult: + inputs = AgentInputs() + + # Pin the checkout before anything else reads the tree. + ref = inputs.target_commit or inputs.target_branch + logger.debug("Preparing the source checkout at %s.", ref) + source_repo = await ctx.prepare_repo(ref=ref) + + return await run_uplift( + bugbug_mcp_server={ + "type": "http", + "url": inputs.bugbug_mcp_url, + }, + broker_url=inputs.broker_url, + source_repo=source_repo, + target_branch=inputs.target_branch, + target_commit=inputs.target_commit, + sources=inputs.sources, + bug_id=inputs.bug_id, + model=inputs.model, + max_turns=inputs.max_turns, + effort=inputs.effort, + log=ctx.log_path, + verbose=True, + publish_file=ctx.publish_file, + ) + + +if __name__ == "__main__": + run_async(main) diff --git a/agents/uplift/tests/test_uplift_main.py b/agents/uplift/tests/test_uplift_main.py new file mode 100644 index 0000000000..4375288d93 --- /dev/null +++ b/agents/uplift/tests/test_uplift_main.py @@ -0,0 +1,134 @@ +"""Tests for the agent entrypoint's handoff to the runtime. + +``main`` is the seam between ``HackbotContext`` and ``run_uplift``. The context +is stubbed rather than mocked, so the contract is pinned by assertions. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest +from hackbot_agents.uplift_merge_conflict_resolver import __main__ as entrypoint +from hackbot_agents.uplift_merge_conflict_resolver.config import ( + GitSource, + PhabricatorSource, +) + +REPO_PATH = Path("/workspace/firefox") + + +class StubContext: + """A stand-in ``HackbotContext`` recording how source is prepared.""" + + def __init__(self) -> None: + self.prepare_calls: list[dict] = [] + self.log_path = Path("/artifacts/agent.log") + + async def prepare_repo(self, ref: str | None = None, depth: int | None = None): + self.prepare_calls.append({"ref": ref, "depth": depth}) + return REPO_PATH + + def publish_file(self, name: str, path: Path, content_type: str | None) -> str: + return f"published://{name}" + + +@pytest.fixture +def recorded_run(monkeypatch) -> dict: + """Capture the keyword arguments the entrypoint hands to ``run_uplift``.""" + captured: dict = {} + + async def fake_run_uplift(**kwargs): + captured.update(kwargs) + return "result" + + monkeypatch.setattr(entrypoint, "run_uplift", fake_run_uplift) + return captured + + +@pytest.fixture +def agent_env(monkeypatch) -> None: + monkeypatch.setenv("TARGET_BRANCH", "beta") + monkeypatch.setenv("SOURCES", '[{"kind": "git", "commit": "' + "a" * 40 + '"}]') + monkeypatch.setenv("BUGBUG_MCP_URL", "http://localhost:8080/mcp") + monkeypatch.setenv("BROKER_URL", "http://localhost:8765") + for name in ( + "BUG_ID", + "MODEL", + "MAX_TURNS", + "EFFORT", + "SOURCE_REF", + "TARGET_COMMIT", + ): + monkeypatch.delenv(name, raising=False) + + +async def test_the_handoff_to_the_agent(agent_env, recorded_run): + """One prepare, pinned to the branch, and the agent runs against its path.""" + ctx = StubContext() + + await entrypoint.main(ctx) + + assert ctx.prepare_calls == [{"ref": "beta", "depth": None}], ( + "The checkout is prepared exactly once, pinned to `target_branch`." + ) + assert recorded_run["source_repo"] == REPO_PATH, ( + "`run_uplift` receives the path `prepare_repo` returned." + ) + assert recorded_run["target_branch"] == "beta", ( + "`target_branch` is forwarded so the prompt names the branch." + ) + assert [source.commit for source in recorded_run["sources"]] == ["a" * 40], ( + "Sources parsed from the environment reach the agent in order." + ) + + +async def test_a_pinned_target_commit_is_what_gets_checked_out( + monkeypatch, agent_env, recorded_run +): + """A branch name moves, so a caller reproducing an uplift can pin the commit.""" + monkeypatch.setenv("TARGET_COMMIT", "f" * 40) + ctx = StubContext() + + await entrypoint.main(ctx) + + assert ctx.prepare_calls == [{"ref": "f" * 40, "depth": None}], ( + "The pinned commit should be checked out instead of the branch tip." + ) + assert recorded_run["target_branch"] == "beta", ( + "The branch is still reported, since it is what the uplift targets." + ) + + +async def test_inputs_treat_compose_empty_strings_as_absent(monkeypatch, agent_env): + """A local `docker compose` run passes every unset optional input as ``""``.""" + for name in ("BUG_ID", "MODEL", "MAX_TURNS", "EFFORT"): + monkeypatch.setenv(name, "") + + inputs = entrypoint.AgentInputs() + + assert (inputs.bug_id, inputs.model, inputs.max_turns, inputs.effort) == ( + None, + None, + None, + None, + ), "An empty env var should fall back to the default, not fail validation." + + +async def test_inputs_parse_mixed_sources_from_the_environment(monkeypatch, agent_env): + """The real input path: one env var holding a JSON list of mixed sources.""" + monkeypatch.setenv( + "SOURCES", + '[{"kind": "git", "commit": "abc"}, ' + '{"kind": "phabricator", "revision_id": 99, "diff_id": 500}]', + ) + monkeypatch.setenv("BUG_ID", "1234567") + + inputs = entrypoint.AgentInputs() + + assert inputs.bug_id == 1234567, "`BUG_ID` should map to `bug_id`." + assert [type(source) for source in inputs.sources] == [ + GitSource, + PhabricatorSource, + ], "`SOURCES` should parse to the discriminated source kinds, in order." + assert inputs.sources[1].diff_id == 500, "A pinned `diff_id` should survive." From 8ab32db626fa31156333ecc8a41917cc6f0fe879 Mon Sep 17 00:00:00 2001 From: Connor Sheehan Date: Thu, 17 Sep 2026 22:31:31 -0400 Subject: [PATCH 6/9] Bug 2051452: Add a read-only Phabricator broker for the uplift agent The agent container is the least-trusted component in the system and binds no durable credential, so the Phabricator key lives in a sidecar that serves `phabricator_proxy`'s read-only Conduit mount over loopback and substitutes the real key. The agent sends a placeholder token it never has to hold. Only `/phabricator/api` is mounted, unlike `bug-fix`'s broker: this agent's model-facing Bugzilla and Phabricator tools come from the bugbug MCP, and the broker exists solely so agent code can fetch each source's diff before the prompt is rendered. The methods that needs are already on the proxy's allow list. The security docs gain that Conduit proxy mount, a third shape of broker capability they did not describe, alongside the agents running a broker. Issue: https://github.com/mozilla/bugbug/issues/6865 --- .../uplift_merge_conflict_resolver/broker.py | 52 +++++++++++++++++++ docs/hackbot/security.md | 6 ++- 2 files changed, 57 insertions(+), 1 deletion(-) create mode 100644 agents/uplift/hackbot_agents/uplift_merge_conflict_resolver/broker.py diff --git a/agents/uplift/hackbot_agents/uplift_merge_conflict_resolver/broker.py b/agents/uplift/hackbot_agents/uplift_merge_conflict_resolver/broker.py new file mode 100644 index 0000000000..40adc04dd1 --- /dev/null +++ b/agents/uplift/hackbot_agents/uplift_merge_conflict_resolver/broker.py @@ -0,0 +1,52 @@ +"""Read-only Phabricator Conduit broker. + +Sidecar holding the Phabricator API key, reached by the agent over loopback in +the same Cloud Run Job task. The agent container binds no credential. + +Only `/phabricator/api` is mounted: the model's Bugzilla and Phabricator tools +come from the bugbug MCP, and the broker exists solely so agent code can fetch +each source's diff before the prompt is rendered. `phabricator_proxy` +allow-lists the read methods that needs and substitutes the real key. +""" + +import logging + +import uvicorn +from phabricator_client import PhabricatorClient, PhabricatorSettings +from phabricator_proxy import create_app as conduit_proxy +from pydantic_settings import BaseSettings, SettingsConfigDict +from starlette.applications import Starlette +from starlette.routing import Mount + +log = logging.getLogger("uplift-broker") + + +class BrokerInputs(BaseSettings): + phabricator: PhabricatorSettings + host: str = "0.0.0.0" + port: int = 8765 + + model_config = SettingsConfigDict( + extra="ignore", + env_nested_delimiter="_", + env_nested_max_split=1, + ) + + +def build_app(inputs: BrokerInputs) -> Starlette: + client = PhabricatorClient(inputs.phabricator) + log.info("broker serving the read-only Conduit proxy on %s", inputs.port) + return Starlette(routes=[Mount("/phabricator/api", app=conduit_proxy(client))]) + + +def main() -> None: + logging.basicConfig( + level=logging.INFO, + format="%(asctime)s - %(name)s - %(levelname)s - %(message)s", + ) + inputs = BrokerInputs() + uvicorn.run(build_app(inputs), host=inputs.host, port=inputs.port, log_config=None) + + +if __name__ == "__main__": + main() diff --git a/docs/hackbot/security.md b/docs/hackbot/security.md index 56f6285e7d..5ece09ac91 100644 --- a/docs/hackbot/security.md +++ b/docs/hackbot/security.md @@ -25,13 +25,17 @@ exposes them as capabilities over loopback (`BROKER_URL`, e.g. `http://127.0.0.1 - `/{bugzilla,phabricator}/mcp` — read-only MCP tool servers, live during the run. - `GET /phabricator/revision/{id}/patch` — a revision's base commit and raw diff, so a follow-up run can reproduce the revision's tree without a Conduit key. +- `/phabricator/api` — a Conduit proxy allow-listing a few read methods and substituting + the real key, for agent code that has to call Conduit directly (`uplift` fetches each + source's raw diff this way before rendering its prompt). It exposes only what a run legitimately needs, and only reads — every write goes through the recorded-actions path instead. **Per-execution env overrides target the `agent` container by name**, which is what stops a run's inputs from reaching or altering the broker's environment. -Today `bug-fix`, `build-repair`, `frontend-triage` and `autowebcompat-repro` run a broker. +Today `bug-fix`, `build-repair`, `frontend-triage`, `autowebcompat-repro`, +`autowebcompat-diagnosis` and `uplift-merge-conflict-resolver` run a broker. `test-repair` reaches an MCP server via an injected `BUGZILLA_MCP_URL` instead, and `test-plan-generator` needs no credentialed reads at all. The invariant holds in every case: **the key is never in the agent container.** From 0ded70bd9e7fdcdf7438f6d9ac6efaf2e69f0e01 Mon Sep 17 00:00:00 2001 From: Connor Sheehan Date: Thu, 17 Sep 2026 22:31:31 -0400 Subject: [PATCH 7/9] Bug 2051452: Add uplift agent packaging, Docker and compose One image with two targets, following `bug-fix`: `agent` runs the resolver, `broker` the Conduit proxy. `hackbot.toml` asks the platform for a Firefox checkout, the one capability the agent needs prepared -- no `[firefox]` table, since it never builds, and so no ffmpeg either. Compose runs the agent against the bugbug MCP and the broker so a local run takes the same path as a deployed one, minus the uploader. Both sidecars hold the credentials; the agent service binds none. Issue: https://github.com/mozilla/bugbug/issues/6865 --- agents/uplift/Dockerfile | 54 ++++++++++++++++++++++++++++++++ agents/uplift/compose.yml | 60 ++++++++++++++++++++++++++++++++++++ agents/uplift/hackbot.toml | 8 +++++ agents/uplift/pyproject.toml | 41 ++++++++++++++++++++++++ docker-compose.yml | 1 + uv.lock | 39 +++++++++++++++++++++++ 6 files changed, 203 insertions(+) create mode 100644 agents/uplift/Dockerfile create mode 100644 agents/uplift/compose.yml create mode 100644 agents/uplift/hackbot.toml create mode 100644 agents/uplift/pyproject.toml diff --git a/agents/uplift/Dockerfile b/agents/uplift/Dockerfile new file mode 100644 index 0000000000..f96045419a --- /dev/null +++ b/agents/uplift/Dockerfile @@ -0,0 +1,54 @@ +FROM python:3.12 AS builder + +COPY --from=ghcr.io/astral-sh/uv:latest /uv /uvx /bin/ + +ENV UV_PROJECT_ENVIRONMENT=/opt/venv + +WORKDIR /app + +# Install external deps without building workspace members. +RUN --mount=type=cache,target=/root/.cache/uv \ + --mount=type=bind,source=pyproject.toml,target=pyproject.toml \ + --mount=type=bind,source=uv.lock,target=uv.lock \ + --mount=type=bind,source=VERSION,target=VERSION \ + uv sync --frozen --no-dev --no-install-workspace --package hackbot-agent-uplift + +RUN --mount=type=cache,target=/root/.cache/uv \ + --mount=type=bind,target=/app,rw \ + uv sync --locked --no-dev --no-editable --package hackbot-agent-uplift + +FROM python:3.12 AS base + +COPY --from=builder /opt/venv /opt/venv +WORKDIR /app + +ENV PYTHONUNBUFFERED=1 +ENV PYTHONDONTWRITEBYTECODE=1 +ENV PATH="/opt/venv/bin:$PATH" + +FROM base AS agent + +# No ffmpeg: the other agents carry it for the codecs a Firefox they build or +# run needs, and this agent does neither. + +# hackbot.toml lives at the agent root (not inside the package), so copy it into +# the working dir; the runtime discovers it there (cwd) at startup. +COPY agents/uplift/hackbot.toml /app/hackbot.toml + +RUN useradd --create-home --shell /bin/bash agent \ + && mkdir -p /workspace \ + && chown agent:agent /workspace + +USER agent + +CMD ["python", "-m", "hackbot_agents.uplift_merge_conflict_resolver"] + +FROM base AS broker + +RUN useradd --create-home --shell /bin/bash broker + +USER broker + +EXPOSE 8765 + +CMD ["python", "-m", "hackbot_agents.uplift_merge_conflict_resolver.broker"] diff --git a/agents/uplift/compose.yml b/agents/uplift/compose.yml new file mode 100644 index 0000000000..9a835e5015 --- /dev/null +++ b/agents/uplift/compose.yml @@ -0,0 +1,60 @@ +services: + # The bugbug MCP server, run as a sidecar so the agent reaches the originating + # bug and Phabricator revision over HTTP. It holds the Bugzilla/Phabricator + # credentials; the agent container binds none. In production the agent points + # at the deployed bugbug MCP instead via BUGBUG_MCP_URL. + uplift-bugbug-mcp: + build: + context: ../.. + dockerfile: services/mcp/Dockerfile + target: base + environment: + PORT: "8080" + BUGZILLA_API_KEY: ${BUGZILLA_API_KEY:-} + BUGZILLA_URL: ${BUGZILLA_URL:-https://bugzilla.mozilla.org} + PHABRICATOR_API_KEY: ${PHABRICATOR_API_KEY:-} + PHABRICATOR_URL: ${PHABRICATOR_URL:-https://phabricator.services.mozilla.com} + expose: + - "8080" + + # Holds the Phabricator key so the agent can fetch each source's raw diff + # over the read-only Conduit proxy without binding a credential itself. + uplift-broker: + build: + context: ../.. + dockerfile: agents/uplift/Dockerfile + target: broker + environment: + PHABRICATOR_URL: ${PHABRICATOR_URL:-https://phabricator.services.mozilla.com} + PHABRICATOR_API_KEY: ${PHABRICATOR_API_KEY:-} + expose: + - "8765" + + uplift-merge-conflict-resolver: + build: + context: ../.. + dockerfile: agents/uplift/Dockerfile + target: agent + environment: + - RUN_ID + - TARGET_BRANCH=${TARGET_BRANCH:-} + - SOURCES=${SOURCES:-} + - BUG_ID=${BUG_ID:-} + - BUGBUG_MCP_URL=http://uplift-bugbug-mcp:8080/mcp + - BROKER_URL=http://uplift-broker:8765 + - SOURCE_REPO=/workspace/firefox + - ANTHROPIC_API_KEY=${ANTHROPIC_API_KEY:?error} + # No uploader locally: summary/logs/artifacts are written under + # /artifacts/, bind-mounted to the host's ~/hackbot/artifacts. + - ARTIFACTS_DIR=/artifacts + volumes: + - workspace:/workspace + - ${HOME}/hackbot/artifacts:/artifacts + depends_on: + uplift-bugbug-mcp: + condition: service_started + uplift-broker: + condition: service_started + +volumes: + workspace: diff --git a/agents/uplift/hackbot.toml b/agents/uplift/hackbot.toml new file mode 100644 index 0000000000..3795fddbd1 --- /dev/null +++ b/agents/uplift/hackbot.toml @@ -0,0 +1,8 @@ +[source] +repo_url = "https://github.com/mozilla-firefox/firefox.git" +checkout_path = "/workspace/firefox" +# The checkout is pinned per-run to the target uplift branch (from +# `target_branch`); the agent fetches and cherry-picks the sources onto it. + +# No [firefox] table: the agent resolves conflicts at the source level and +# never builds Firefox, so the build toolchain is not prepared. diff --git a/agents/uplift/pyproject.toml b/agents/uplift/pyproject.toml new file mode 100644 index 0000000000..0fd7cf5b94 --- /dev/null +++ b/agents/uplift/pyproject.toml @@ -0,0 +1,41 @@ +[project] +name = "hackbot-agent-uplift" +version = "0.1.0" +description = "Cloud Run Job image that runs the uplift conflict-resolution agent for hackbot-api" +requires-python = ">=3.12" +dependencies = [ + "hackbot-runtime[claude-sdk]", + "agent-tools[searchfox,vcs]", + "claude-agent-sdk>=0.1.30", + "mcp>=1.0.0", + "phabricator-client", + "phabricator-proxy", + "pydantic-settings", + "starlette", + "uvicorn", +] + +[project.optional-dependencies] +test = [ + "pytest>=8", + "pytest-asyncio>=0.23.0", +] + +[tool.uv.sources] +hackbot-runtime = { workspace = true } +agent-tools = { workspace = true } +phabricator-client = { workspace = true } +phabricator-proxy = { workspace = true } + +[tool.pytest.ini_options] +asyncio_mode = "auto" +testpaths = ["tests"] + +[build-system] +requires = ["hatchling"] +build-backend = "hatchling.build" + +# `hackbot_agents` is a PEP 420 namespace package, shared with the other +# agents. Hatchling ships everything under it, prompts included. +[tool.hatch.build.targets.wheel] +packages = ["hackbot_agents"] diff --git a/docker-compose.yml b/docker-compose.yml index c0e69b9d50..b0c36868e4 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -10,6 +10,7 @@ include: - path: agents/test-repair/compose.yml - path: agents/frontend-triage/compose.yml - path: agents/test-plan-generator/compose.yml + - path: agents/uplift/compose.yml services: bugbug-base: diff --git a/uv.lock b/uv.lock index a7ce179433..8902abd9fa 100644 --- a/uv.lock +++ b/uv.lock @@ -29,6 +29,7 @@ members = [ "hackbot-agent-frontend-triage", "hackbot-agent-test-plan-generator", "hackbot-agent-test-repair", + "hackbot-agent-uplift", "hackbot-api", "hackbot-client", "hackbot-pulse-listener", @@ -2703,6 +2704,44 @@ requires-dist = [ { name = "zstandard", specifier = "~=0.25.0" }, ] +[[package]] +name = "hackbot-agent-uplift" +version = "0.1.0" +source = { editable = "agents/uplift" } +dependencies = [ + { name = "agent-tools", extra = ["searchfox", "vcs"] }, + { name = "claude-agent-sdk" }, + { name = "hackbot-runtime", extra = ["claude-sdk"] }, + { name = "mcp" }, + { name = "phabricator-client" }, + { name = "phabricator-proxy" }, + { name = "pydantic-settings" }, + { name = "starlette" }, + { name = "uvicorn" }, +] + +[package.optional-dependencies] +test = [ + { name = "pytest" }, + { name = "pytest-asyncio" }, +] + +[package.metadata] +requires-dist = [ + { name = "agent-tools", extras = ["searchfox", "vcs"], editable = "libs/agent-tools" }, + { name = "claude-agent-sdk", specifier = ">=0.1.30" }, + { name = "hackbot-runtime", extras = ["claude-sdk"], editable = "libs/hackbot-runtime" }, + { name = "mcp", specifier = ">=1.0.0" }, + { name = "phabricator-client", editable = "libs/phabricator-client" }, + { name = "phabricator-proxy", editable = "libs/phabricator-proxy" }, + { name = "pydantic-settings" }, + { name = "pytest", marker = "extra == 'test'", specifier = ">=8" }, + { name = "pytest-asyncio", marker = "extra == 'test'", specifier = ">=0.23.0" }, + { name = "starlette" }, + { name = "uvicorn" }, +] +provides-extras = ["test"] + [[package]] name = "hackbot-api" version = "0.1.0" From cbb2d6ced8859c0646e2a878d912bc7631b71190 Mon Sep 17 00:00:00 2001 From: Connor Sheehan Date: Thu, 17 Sep 2026 22:31:31 -0400 Subject: [PATCH 8/9] Bug 2051452: Register the uplift agent in hackbot-api and the UI `UpliftInputs` is the agent's public contract; env vars derive from it, so no `build_env` is needed. `target_commit` is optional, so Lando can pin the checkout it failed on once it knows to. The registry entry leaves `auto_apply_actions` off: the agent records no actions, and its output is a patch for a human to review. The UI gets the agent in its shared list, which backs both the trigger form and the run filter, plus the fields a run actually needs -- without them the form falls through to its bug-id-only default and submits without `target_branch` or `sources`. `parseUpliftSources` validates the stack against the same shapes `UpliftSource` accepts, so a malformed entry is a message beside the field rather than a 422 after submitting. Issue: https://github.com/mozilla/bugbug/issues/6865 --- docs/hackbot/agents.md | 1 + services/hackbot-api/app/agents.py | 12 +++ services/hackbot-api/app/schemas.py | 71 +++++++++++- services/hackbot-api/tests/test_agents.py | 82 ++++++++++++++ .../hackbot-ui/components/TriggerForm.tsx | 73 ++++++++++++- services/hackbot-ui/lib/agents.ts | 4 + services/hackbot-ui/lib/uplift.test.ts | 101 ++++++++++++++++++ services/hackbot-ui/lib/uplift.ts | 75 +++++++++++++ 8 files changed, 414 insertions(+), 5 deletions(-) create mode 100644 services/hackbot-ui/lib/uplift.test.ts create mode 100644 services/hackbot-ui/lib/uplift.ts diff --git a/docs/hackbot/agents.md b/docs/hackbot/agents.md index 4ac09aa8f9..baefe298f5 100644 --- a/docs/hackbot/agents.md +++ b/docs/hackbot/agents.md @@ -58,6 +58,7 @@ or code. | `frontend-triage` | Read-only root-cause analysis and fix plan for a desktop frontend bug. | yes | no | no | | `autowebcompat-repro` | Reproduce a web-compatibility report in headless Firefox via DevTools MCP. | no | no | no | | `test-plan-generator` | Generate Firefox QA test cases, run them through DevTools MCP, report results. | no | no | no | +| `uplift-merge-conflict-resolver` | Resolve the merge conflicts from cherry-picking patches onto a stable uplift branch, and return the resolved patch with a confidence level. | yes | no | no | Two shapes recur. **Source agents** (`bug-fix`, `test-repair`, `build-repair`) check out Firefox, often build it, edit the tree, and let the runtime capture the diff. **Browser diff --git a/services/hackbot-api/app/agents.py b/services/hackbot-api/app/agents.py index 7132b33fd6..abb42c0f4c 100644 --- a/services/hackbot-api/app/agents.py +++ b/services/hackbot-api/app/agents.py @@ -12,6 +12,7 @@ FrontendTriageInputs, TestPlanGeneratorInputs, TestRepairInputs, + UpliftInputs, ) @@ -136,4 +137,15 @@ def model_to_env(inputs: BaseModel) -> dict[str, str]: job_name="hackbot-agent-test-plan-generator", input_schema=TestPlanGeneratorInputs, ), + # The first of the uplift/backport sub-agents; more may join under this area. + "uplift-merge-conflict-resolver": AgentSpec( + name="uplift-merge-conflict-resolver", + description=( + "Resolve the merge conflicts from cherry-picking patches (git commits " + "and/or Phabricator revisions) onto a stable uplift branch, and return " + "the resolved patch with a confidence level for human review." + ), + job_name="hackbot-agent-uplift-merge-conflict-resolver", + input_schema=UpliftInputs, + ), } diff --git a/services/hackbot-api/app/schemas.py b/services/hackbot-api/app/schemas.py index aa466dfe8e..7aac6c695c 100644 --- a/services/hackbot-api/app/schemas.py +++ b/services/hackbot-api/app/schemas.py @@ -1,6 +1,6 @@ from datetime import datetime from enum import Enum -from typing import Annotated, Any +from typing import Annotated, Any, Literal, Union from uuid import UUID from pydantic import BaseModel, ConfigDict, Field, model_validator @@ -175,3 +175,72 @@ class TestPlanGeneratorInputs(BaseModel): model: str | None = None max_turns: int | None = None effort: str | None = None + + +class GitUpliftSource(BaseModel): + """A patch to uplift, identified by a commit already in the Firefox repo.""" + + # Discriminator tag selecting this variant in the `UpliftSource` union. + kind: Literal["git"] = "git" + + # Full git commit SHA the agent fetches and cherry-picks onto the branch. + commit: str = Field(description="Full git commit SHA to cherry-pick.") + + +class PhabricatorUpliftSource(BaseModel): + """A patch to uplift, identified by a Phabricator revision. + + The agent fetches the diff through its broker; inputs reach the job as + environment variables, which a raw diff would not fit. + """ + + # Discriminator tag selecting this variant in the `UpliftSource` union. + kind: Literal["phabricator"] = "phabricator" + + # Phabricator revision to uplift, used for context and commit text. + revision_id: int = Field(description="Phabricator revision id (the D-number).") + + # Pin the exact diff. Without one, a revision updated since the request + # resolves to different code. + diff_id: int | None = Field( + default=None, + description="Diff to uplift; defaults to the revision's latest.", + ) + + +# `kind` discriminates the two, so one run can mix both. +UpliftSource = Annotated[ + Union[GitUpliftSource, PhabricatorUpliftSource], Field(discriminator="kind") +] + + +class UpliftInputs(BaseModel): + """Inputs for the uplift conflict-resolution agent.""" + + # The stable branch ref to uplift onto, e.g. `release`, `beta`, `esr128`. + target_branch: str + + # The exact commit to uplift onto. Branch names move, so a caller + # reproducing a specific uplift should pin it; otherwise the tip is used. + target_commit: str | None = None + + # Ordered patches to apply onto the branch; applied in this sequence. + sources: list[UpliftSource] + + # Originating Bugzilla bug, supplied as extra context for the agent. + bug_id: int | None = None + + # Override the agent's default Claude model id. + model: str | None = None + + # Cap on agent turns; `None` leaves the agent's own default in place. + max_turns: int | None = None + + # Override the agent's default reasoning effort (e.g. `high`). + effort: str | None = None + + @model_validator(mode="after") + def require_sources(self) -> "UpliftInputs": + if not self.sources: + raise ValueError("provide at least one source to uplift") + return self diff --git a/services/hackbot-api/tests/test_agents.py b/services/hackbot-api/tests/test_agents.py index 432e067ab9..5008cae2cb 100644 --- a/services/hackbot-api/tests/test_agents.py +++ b/services/hackbot-api/tests/test_agents.py @@ -8,6 +8,7 @@ BugFixInputs, BuildRepairInputs, TestRepairInputs, + UpliftInputs, ) from app.schemas import ( TestPlanGeneratorInputs as PlanGeneratorInputs, @@ -156,3 +157,84 @@ def test_test_repair_env_serialization(): def test_test_repair_inputs_require_failure_tasks(): with pytest.raises(ValidationError): TestRepairInputs(model="claude-opus-4-8") + + +def test_uplift_registry_entry(): + spec = AGENT_REGISTRY["uplift-merge-conflict-resolver"] + assert spec.build_env is None, ( + "The uplift agent's inputs need no hand-written env serializer." + ) + assert spec.input_schema is UpliftInputs, ( + "The registry should validate uplift runs against `UpliftInputs`." + ) + assert spec.job_name == "hackbot-agent-uplift-merge-conflict-resolver", ( + "The job name should match the deployed Cloud Run Job." + ) + assert spec.auto_apply_actions is False, ( + "The uplift agent records no actions: its output is a patch for review." + ) + + +def test_uplift_env_serialization(): + env = model_to_env( + UpliftInputs( + target_branch="beta", + sources=[ + {"kind": "git", "commit": "a" * 40}, + {"kind": "phabricator", "revision_id": 12345, "diff_id": 500}, + ], + bug_id=1846789, + ) + ) + + assert env["TARGET_BRANCH"] == "beta", "`target_branch` should map to its env var." + assert "TARGET_COMMIT" not in env, ( + "An unpinned target commit should not leak as an empty env var." + ) + assert env["BUG_ID"] == "1846789", "`bug_id` should map to its env var." + assert json.loads(env["SOURCES"]) == [ + {"kind": "git", "commit": "a" * 40}, + {"kind": "phabricator", "revision_id": 12345, "diff_id": 500}, + ], "`sources` should be JSON-encoded in order, since it travels as one env var." + + +def test_uplift_pinned_target_commit_reaches_the_agent(): + env = model_to_env( + UpliftInputs( + target_branch="beta", + target_commit="a" * 40, + sources=[{"kind": "git", "commit": "b" * 40}], + ) + ) + + assert env["TARGET_COMMIT"] == "a" * 40, ( + "A caller reproducing a specific uplift pins the commit, since the " + "branch name it sat on moves." + ) + + +def test_uplift_inputs_keep_a_mixed_stack_discriminated(): + inputs = UpliftInputs( + target_branch="esr128", + sources=[ + {"kind": "phabricator", "revision_id": 9}, + {"kind": "git", "commit": "abc123"}, + ], + ) + + assert [source.kind for source in inputs.sources] == ["phabricator", "git"], ( + "`kind` should discriminate the union so one run can mix both kinds." + ) + assert inputs.sources[0].diff_id is None, ( + "An unpinned `diff_id` should default to `None`, meaning the latest diff." + ) + + +def test_uplift_inputs_require_at_least_one_source(): + with pytest.raises(ValidationError, match="at least one source"): + UpliftInputs(target_branch="beta", sources=[]) + + +def test_uplift_inputs_reject_an_unknown_source_kind(): + with pytest.raises(ValidationError): + UpliftInputs(target_branch="beta", sources=[{"kind": "hg", "rev": "abc"}]) diff --git a/services/hackbot-ui/components/TriggerForm.tsx b/services/hackbot-ui/components/TriggerForm.tsx index e069c31aad..e2e1eff1d6 100644 --- a/services/hackbot-ui/components/TriggerForm.tsx +++ b/services/hackbot-ui/components/TriggerForm.tsx @@ -7,6 +7,7 @@ import { AGENTS, type AgentValue } from "@/lib/agents"; import { parseBugId } from "@/lib/bugzilla"; import { saveRun } from "@/lib/store"; import type { RunRef } from "@/lib/types"; +import { parseUpliftSources } from "@/lib/uplift"; function parseAgent(value: string | null): AgentValue { return AGENTS.some((a) => a.value === value) @@ -40,6 +41,12 @@ export function TriggerForm() { const [testScope, setTestScope] = useState( () => params.get("test_scope") ?? "" ); + const [targetBranch, setTargetBranch] = useState( + () => params.get("target_branch") ?? "" + ); + const [upliftSources, setUpliftSources] = useState( + () => params.get("sources") ?? "" + ); const [model, setModel] = useState(() => params.get("model") ?? ""); const [maxTurns, setMaxTurns] = useState(() => params.get("max_turns") ?? ""); const [effort, setEffort] = useState(() => params.get("effort") ?? ""); @@ -50,6 +57,7 @@ export function TriggerForm() { const isBuildRepairAgent = agent === "build-repair"; const isTestRepairAgent = agent === "test-repair"; const isTestPlanAgent = agent === "test-plan-generator"; + const isUpliftAgent = agent === "uplift-merge-conflict-resolver"; const needsFailureTasks = isBuildRepairAgent || isTestRepairAgent; async function onSubmit(e: React.FormEvent) { @@ -112,6 +120,19 @@ export function TriggerForm() { inputs.feature_name = featureName.trim(); inputs.feature_description = featureDescription.trim(); inputs.test_scope = testScope.trim(); + } else if (isUpliftAgent) { + if (!targetBranch.trim()) { + setError("Enter the target uplift branch."); + return; + } + const parsed = parseUpliftSources(upliftSources); + if (parsed.error) { + setError(parsed.error); + return; + } + inputs.target_branch = targetBranch.trim(); + inputs.sources = parsed.sources; + if (hasBugId) inputs.bug_id = parsedBugId; } else if (!isReproAgent) { if (!hasBugId) { setError("Enter a valid Bugzilla bug ID or bug URL."); @@ -155,9 +176,11 @@ export function TriggerForm() { "test failure" : isTestPlanAgent ? featureName.trim() - : hasBugId - ? `bug ${parsedBugId}` - : "inline report"; + : isUpliftAgent + ? `uplift to ${targetBranch.trim()}` + : hasBugId + ? `bug ${parsedBugId}` + : "inline report"; saveRun({ run_id: run.run_id, agent: run.agent, @@ -194,7 +217,7 @@ export function TriggerForm() { - {!needsFailureTasks && !isTestPlanAgent && ( + {!needsFailureTasks && !isTestPlanAgent && !isUpliftAgent && (
)} + {isUpliftAgent && ( + <> +
+ + setTargetBranch(e.target.value)} + required + /> +
+ +
+ +