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/README.md b/agents/uplift/README.md new file mode 100644 index 0000000000..33ee4469ee --- /dev/null +++ b/agents/uplift/README.md @@ -0,0 +1,67 @@ +# Uplift Agents + +Home for the backport/uplift sub-agents that help land patches on Firefox's +stable branches (release/beta/esr). One Cloud Run image +(`hackbot-agent-uplift`); each sub-agent is a module under `hackbot_agents/` +with its own entrypoint, job and registry entry. Module names are prefixed +`uplift_` because the runtime traces under the directory holding `__main__.py`, +which has to match the registry name. + +## Merge-conflict resolver + +`hackbot_agents/uplift_merge_conflict_resolver/` — the first sub-agent. It +reproduces a failed uplift cherry-pick on the target branch, resolves the +conflicts, and returns the patch with a confidence level for review. It never +pushes or lands anything, and it does not build Firefox. + +### Input + +Set per run (see `UpliftInputs` in hackbot-api for the full schema): + +- `TARGET_BRANCH` — branch to uplift onto, e.g. `beta`, `esr128`. +- `TARGET_COMMIT` — optional commit on that branch, for reproducing a specific + failed uplift. Branch names move; without one the tip is used. +- `SOURCES` — JSON list, applied in order; a stack is several entries. Each is + `{"kind": "git", "commit": ""}` or + `{"kind": "phabricator", "revision_id": 12345, "diff_id": 67890}`, where + `diff_id` is optional and defaults to the revision's latest. +- `BUG_ID` — optional Bugzilla bug for context. + +`BUGBUG_MCP_URL` and `BROKER_URL` are deploy-time constants: the bugbug MCP +server, and the sidecar holding the Conduit key the agent fetches diffs through. + +### Output + +- `changes/changes.patch` — the resolved uplift, as an mbox preserving each + commit's message and author. This is what a caller consumes. +- `report.json` — `resolved`, `confidence`, `conflicts`, `unresolved`, + `verification_failures`. `resolved` is not taken on trust: the run checks the + checkout for conflicted paths, an unfinished cherry-pick, uncommitted work + and whether the commits would collect to a patch at all, and overrules the + claim if they disagree. Confidence stays the agent's own judgment. +- `report.unverified.json` — the same report as the agent wrote it, before + those checks. For debugging; read `report.json`. +- `summary.md` — what conflicted and how it was resolved, for a reviewer. + +The run summary also records `base_commit` (what the patches were applied onto) +and `requested_sources` (what was asked for and what each source resolved to). + +### Run locally + +With `ANTHROPIC_API_KEY`, `BUGZILLA_API_KEY` and `PHABRICATOR_API_KEY` in a +repo-root `.env`, from the repo root: + +```sh +TARGET_BRANCH=beta \ +SOURCES='[{"kind":"git","commit":""}]' \ + docker compose up uplift-merge-conflict-resolver --build +``` + +Artifacts land in `~/hackbot/artifacts/`; apply the patch with +`git am changes/changes.patch`. + +### Tests + +```sh +uv run --package hackbot-agent-uplift --extra test pytest agents/uplift/tests +``` 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/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/__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/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/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/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] = [] 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. 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/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/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_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_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." 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=[]) 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." + ) 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/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/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.** 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 + /> +
+ +
+ +