Skip to content

Bug 2051452: Add the uplift merge-conflict resolver agent - #6876

Open
cgsheeh wants to merge 9 commits into
mozilla:masterfrom
cgsheeh:uplift-agents
Open

cgsheeh wants to merge 9 commits into
mozilla:masterfrom
cgsheeh:uplift-agents

Conversation

@cgsheeh

@cgsheeh cgsheeh commented Sep 18, 2026

Copy link
Copy Markdown
Member

Adds the first uplift sub-agent: it reproduces a failed uplift cherry-pick on a
stable branch, resolves the merge conflicts, and returns the resulting patch
with a confidence level for review. Sources are git commits and/or Phabricator
revisions applied in order, so a stack is just several sources, and a caller can
pin both the diff to uplift and the commit to uplift onto. It never pushes or
lands anything, auto-applies no actions, and does not build Firefox — resolving
a conflict is a source-level judgment and a reviewer compiles the result.

resolved is not taken on trust. The agent grades its own work, so after the
session the run checks the checkout — no conflicted paths, no cherry-pick left
open, nothing uncommitted, and commits that actually collect to a patch — and
overrules the claim if they disagree, publishing the verified report alongside
what the agent wrote. Confidence stays the agent's own judgment. The Lando side
of this work (the uplift checkbox, waiting on a run, retrieving the result,
Phabricator submission and human approval) is not in this PR.

Issue: #6865

An uplift run is an ordered list of sources applied onto a stable branch, so a
stack is simply several sources. `kind` discriminates the two shapes a source
can take, letting one run mix them.

Each kind knows two things about itself: how to materialize whatever the agent
needs on disk (`fetch_diff`) and how to describe itself as a work item in the
prompt (`render_work_item`). Keeping both on the model means a third source kind
touches this module only.

A Phabricator source carries only `revision_id` plus an optional `diff_id`, not
the diff text. Run inputs reach the Cloud Run Job as environment variables, so
an inlined diff would be bounded by the env size limit. `diff_id` defaults to
the revision's latest, but a caller that knows which diff it landed should pin
it: otherwise a revision updated between request and run resolves to different
code. Diffs are named on disk by diff as well as revision, since sources are
all fetched up front and two pins on one revision would otherwise share a file.

A fetched diff carries its author and its base commit. The author because the
uplift commit must keep it -- Lando refuses a patch authored by hackbot -- and
the base commit because `git apply --3way` needs it fetched to find the blobs
the diff names. moz-phab abbreviates that hash for a repo the size of firefox
and `git fetch` refuses an abbreviation, so it is expanded before use.

`RequestedSource` is the record of what a run was asked to uplift and what the
fetch pinned each source down to, which for an unpinned source is not
derivable from the input. It is not evidence anything was applied.

`Report` is the schema of the `report.json` the agent writes, which is model
output and so validated rather than trusted. Every field defaults, so a partial
report still parses. What validation buys is catching a plausible but wrong
value: `"resolved": "false"` is a string `bool()` reads as `True`, and
`confidence` is what a caller gates on.

Issue: mozilla#6865
Split the way every other agent splits them: `system.md` is how to resolve an
uplift and holds no per-run detail, `task.md` is what this run is uplifting,
onto what, and where to report it.

`system.md` covers how to apply each kind of source, how to resolve a conflict
without dropping the patch's functional change, which Mozilla MCP to consult
when the intent is unclear, and the report shape. Four things it insists on:
confidence is graded conservatively, since a wrong uplift on a stable branch is
expensive and the grade is what a human triages on; every commit keeps the
original patch author, because Lando refuses a patch authored by hackbot; a
Phabricator diff's base commit is fetched before applying it, without which the
three-way merge finds no blobs and leaves nothing to resolve; and the checkout
has to agree with the report, since that is checked mechanically.

Keeping the run out of `system.md` leaves an identical prefix on every run for
prompt caching to reuse, and keeps Phabricator-supplied strings -- an author
name reaches the prompt through `--author` -- out of the most privileged part of
the context.

Issue: mozilla#6865
The agent grades its own work, and a caller gates landing on `resolved`. Taken
on trust that accepts a report claiming success while listing unresolved hunks,
a tree with conflict markers still in the index, a cherry-pick left open, work
left uncommitted, or a claimed resolution with no patch behind it at all.

None of those are judgment calls, so they are checked against the repository.
Confidence stays the agent's own grade, which only it can give.

"There is a patch" is checked the way the runtime collects one, since HEAD
merely differing from the commit the run started at does not mean there is
something to collect: HEAD can move backwards, and an empty commit moves it
while changing nothing. So the base has to remain an ancestor, the range has to
hold commits, and those commits have to change something.

Tested against real repositories, including a genuinely conflicted cherry-pick:
these are statements about git's own state, and a stub could only assert we
asked for it. The fixtures neutralise global and system git config, since a
developer's commit signing or hooks path would otherwise reach these
repositories -- and the runtime's own git calls too.

Issue: mozilla#6865
A single-stage claude-agent-sdk run: ask each source to materialize what it
needs on disk, render the task against the result, drive the session, then
check the checkout and report what happened. The runtime collects the resolved
commits separately into `changes/changes.patch`.

The per-kind differences live on the source models, so this module only zips
sources against their fetch results and lets each render itself. Diffs come
from the broker's read-only Conduit proxy rather than arriving as input.

The result records `base_commit` and `requested_sources`, so a reviewer can see
what the run was applied onto and what each source resolved to without
re-deriving it. A `resolved` the checks disagree with is overruled to `false`,
with the reasons in `verification_failures`. `report.json` is published with
that verified outcome rather than the agent's claim, which would otherwise
contradict the run summary; what the agent wrote is published verbatim beside
it as `report.unverified.json`.

No Firefox build tools: resolving a conflict is a source-level judgment, and a
build would need the `mach bootstrap` toolchain `build-repair` carries.
`setting_sources` loads `project` so the agent reads the checkout's own
`CLAUDE.md` and in-tree skills; `local` names a gitignored file a fresh clone
cannot have. `effort` is omitted unless a run supplies one, since the API
already defaults to `high`.

`run_session` is a seam, so `run_uplift` is driven end to end against a real
checkout the stand-in session commits into -- including once all the way
through, resolving a genuine conflict and reapplying the collected patch to a
clean clone, since a patch that will not land is the one way the run can look
successful and be useless.

Issue: mozilla#6865
`AgentInputs` reads the per-run inputs the platform passes as environment
variables, then `main` prepares the checkout and hands off to `run_uplift`.

The checkout is pinned before anything else reads the tree, to `target_commit`
when the caller supplied one and the branch tip otherwise: a branch name moves,
so pinning is what reproduces the checkout a failed uplift was seen on. Empty
strings count as absent, since compose passes every unset optional input as
`${VAR:-}`.

Issue: mozilla#6865
The agent container is the least-trusted component in the system and binds no
durable credential, so the Phabricator key lives in a sidecar that serves
`phabricator_proxy`'s read-only Conduit mount over loopback and substitutes the
real key. The agent sends a placeholder token it never has to hold.

Only `/phabricator/api` is mounted, unlike `bug-fix`'s broker: this agent's
model-facing Bugzilla and Phabricator tools come from the bugbug MCP, and the
broker exists solely so agent code can fetch each source's diff before the
prompt is rendered. The methods that needs are already on the proxy's allow
list.

The security docs gain that Conduit proxy mount, a third shape of broker
capability they did not describe, alongside the agents running a broker.

Issue: mozilla#6865
One image with two targets, following `bug-fix`: `agent` runs the resolver,
`broker` the Conduit proxy. `hackbot.toml` asks the platform for a Firefox
checkout, the one capability the agent needs prepared -- no `[firefox]` table,
since it never builds, and so no ffmpeg either.

Compose runs the agent against the bugbug MCP and the broker so a local run
takes the same path as a deployed one, minus the uploader. Both sidecars hold
the credentials; the agent service binds none.

Issue: mozilla#6865
`UpliftInputs` is the agent's public contract; env vars derive from it, so no
`build_env` is needed. `target_commit` is optional, so Lando can pin the
checkout it failed on once it knows to. The registry entry leaves
`auto_apply_actions` off: the agent records no actions, and its output is a
patch for a human to review.

The UI gets the agent in its shared list, which backs both the trigger form and
the run filter, plus the fields a run actually needs -- without them the form
falls through to its bug-id-only default and submits without `target_branch` or
`sources`. `parseUpliftSources` validates the stack against the same shapes
`UpliftSource` accepts, so a malformed entry is a message beside the field
rather than a 422 after submitting.

Issue: mozilla#6865
The inputs, the outputs, and how to run the agent locally. Notes that
`report.json` is the verified report and `report.unverified.json` is the
agent's own, since `resolved` differs between them exactly when the checks
rejected a claim.

`agents/uplift/` is framed as a home for several uplift sub-agents, since the
merge-conflict resolver is the first of them.

Issue: mozilla#6865

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Report validation, patch verification, commit metadata preservation, and pinned local execution have correctness gaps.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds an uplift agent that resolves Firefox backport conflicts and publishes verified patches and reports for human review.

Changes:

  • Adds uplift-agent orchestration, prompting, verification, broker, packaging, and deployment configuration.
  • Exposes uplift runs through the API and UI.
  • Adds comprehensive unit and patch round-trip tests.
File summaries
File Description
uv.lock Registers uplift workspace dependencies.
services/hackbot-ui/lib/uplift.ts Parses uplift sources.
services/hackbot-ui/lib/uplift.test.ts Tests source parsing.
services/hackbot-ui/lib/agents.ts Registers the UI agent.
services/hackbot-ui/components/TriggerForm.tsx Adds uplift inputs.
services/hackbot-api/tests/test_agents.py Tests API registration and serialization.
services/hackbot-api/app/schemas.py Defines uplift input schemas.
services/hackbot-api/app/agents.py Registers the Cloud Run job.
docs/hackbot/security.md Documents broker security.
docs/hackbot/agents.md Documents the new agent.
docker-compose.yml Includes uplift services.
agents/uplift/tests/test_uplift_verify.py Tests repository verification.
agents/uplift/tests/test_uplift_run.py Tests orchestration.
agents/uplift/tests/test_uplift_report.py Tests report handling.
agents/uplift/tests/test_uplift_prompt.py Tests prompt rendering.
agents/uplift/tests/test_uplift_patch_roundtrip.py Tests patch reapplication.
agents/uplift/tests/test_uplift_options.py Tests SDK options.
agents/uplift/tests/test_uplift_main.py Tests entrypoint handoff.
agents/uplift/tests/test_uplift_diffs.py Tests Phabricator fetching.
agents/uplift/tests/conftest.py Provides Git test fixtures.
agents/uplift/README.md Documents usage and outputs.
agents/uplift/pyproject.toml Defines the package.
agents/uplift/hackbot.toml Configures the Firefox checkout.
agents/uplift/hackbot_agents/uplift_merge_conflict_resolver/verify.py Verifies completed resolutions.
agents/uplift/hackbot_agents/uplift_merge_conflict_resolver/prompts/task.md Defines per-run tasks.
agents/uplift/hackbot_agents/uplift_merge_conflict_resolver/prompts/system.md Defines resolution workflow.
agents/uplift/hackbot_agents/uplift_merge_conflict_resolver/config.py Defines source and report models.
agents/uplift/hackbot_agents/uplift_merge_conflict_resolver/broker.py Hosts the Conduit proxy.
agents/uplift/hackbot_agents/uplift_merge_conflict_resolver/agent.py Implements orchestration and reporting.
agents/uplift/hackbot_agents/uplift_merge_conflict_resolver/__main__.py Connects runtime inputs to the agent.
agents/uplift/hackbot_agents/uplift_merge_conflict_resolver/__init__.py Initializes the package.
agents/uplift/Dockerfile Builds agent and broker images.
agents/uplift/compose.yml Configures local services.
Review details

Suppressed comments (1)

agents/uplift/hackbot_agents/uplift_merge_conflict_resolver/config.py:237

  • confidence remains optional even when resolved is true, so a report containing only {"resolved": true} can pass all checkout checks and publish a resolved patch with no confidence level. Since confidence is part of the resolver's review contract, add model validation that requires it for a claimed resolution.
    confidence: Literal["high", "medium", "low"] | None = None
  • Files reviewed: 31/33 changed files
  • Comments generated: 7
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread agents/uplift/compose.yml
Comment on lines +40 to +41
- TARGET_BRANCH=${TARGET_BRANCH:-}
- SOURCES=${SOURCES:-}

model_config = ConfigDict(extra="ignore")

resolved: bool = False
Comment on lines +58 to +62
# 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")
Comment on lines +75 to +78
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")
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.")
leaving you no conflict markers at all.
3. Apply it with a three-way merge so conflicts surface as markers:
`git apply --3way <diff-file>`
4. Resolve any conflicts and commit the fixed patch.
<textarea
id="upliftSources"
placeholder={
'[{"kind": "git", "commit": "abc123"},\n' +

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file doesn't look like a config to me :) I would expect just some constants here, maybe within a class.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need a system prompt at all?


## When you are done

Leave all resolved patches committed in the working tree (the platform collects

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per our latest discussion with @suhaibmujahid, it shouldn't commit anything (we don't even set git identity). The edits will be collected automatically in changes.patch (see build repair agent). Maybe it's different in other agents, not sure.

Comment thread agents/uplift/README.md
@@ -0,0 +1,67 @@
# Uplift Agents

Home for the backport/uplift sub-agents that help land patches on Firefox's

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, this is not a sub-agent. I'm not even sure we can change the name from hackbot_agents to uplift; I thought it was just a convention. See agents/README.md


For a **Git** source (a commit SHA):

1. Fetch the commit and its parent so cherry-pick can three-way merge:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably what you need here, but we prefetch 500 commits for the test-repair agent, for example. So if you're sure that the ones you need are easy to prefetch, it's another option.

]


class UpliftInputs(BaseModel):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you really need such a complicated input? If things can be resolved at runtime, it's better to do that, in my experience, because later you might also want to reproduce the runs locally, run evals for the agent, implement deduplication, and many of those are easier with a simple input. I ended up using just the Taskcluster task ID for CI agents, and everything else is resolved from there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants