Skip to content

Add MR Consolidation Agent for combining backport MRs#633

Open
TomasKorbar wants to merge 2 commits into
packit:mainfrom
TomasKorbar:mr_consolidation_agent
Open

Add MR Consolidation Agent for combining backport MRs#633
TomasKorbar wants to merge 2 commits into
packit:mainfrom
TomasKorbar:mr_consolidation_agent

Conversation

@TomasKorbar

@TomasKorbar TomasKorbar commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator

Introduces a new agent that automatically consolidates multiple backport
merge requests for the same package-branch into a single comprehensive MR.

Key components:

  • Redis Hash-based queue with atomic Lua script for concurrent worker
    safety (at-most-one-active, one-pending invariant per package-branch)
  • Backport agent integration: submits consolidation jobs after filing MRs
    when enabled via per-package consolidation.json config
  • Stale MR filtering: rejects MRs not based on the current target branch
    HEAD, logging skipped MRs as warnings
  • LLM-driven patch merging with size-based ordering heuristic (larger
    patch applied first) and automatic context-line adaptation for
    overlapping patches
  • Build verification with retry loop (same pattern as backport agent)
  • Automatic re-queue when more than 2 MRs need consolidation, chaining
    runs until all are folded into one
  • Label-triggered consolidation: maintainers can add ymir_consolidate_base
    and ymir_consolidate_next Jira labels to request consolidation of two
    specific MRs; the fetcher pairs them and submits a targeted job
  • Post-consolidation Jira updates: comments the consolidated MR URL on
    all involved Jira issues
  • Original MRs are labeled ymir_consolidated (not closed) and excluded
    from future consolidation searches
  • Standalone mode via Makefile for manual testing
  • E2E tests with cached backport replay and LLM-as-a-judge evaluation
  • Architecture documentation for developer reference

@gemini-code-assist gemini-code-assist Bot left a comment

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.

Code Review

This pull request introduces the MR Consolidation Agent, which automates combining multiple backport merge requests for a package and branch into a single, verified merge request. The changes include the core workflow orchestration, Redis queue management, OpenShift deployment manifests, documentation, and extensive test suites. Review feedback suggests removing a risky and redundant git add -A command in the staging step, correcting a minor markdown table formatting issue in the documentation, and using the full path /usr/bin/python in the c9s deployment configuration for consistency.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread ymir/agents/mr_consolidation_agent.py Outdated
Comment thread docs/mr_consolidation_architecture.md
Comment thread openshift/deployment-mr-consolidation-agent-c9s.yml Outdated
@TomasKorbar TomasKorbar force-pushed the mr_consolidation_agent branch 4 times, most recently from 1d862fb to 544617f Compare July 1, 2026 09:21

@nforro nforro left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm pasting Claude's review without doing a human review of it, since when I do I usually end up being too late to the party and the PR is already merged 😅 But it's probably faster to just let your Claude deal with it anyway.

Bug: MERGE_CONSOLIDATION_QUEUE in RedisQueues.all_queues() causes type error

RedisQueues.all_queues() returns {queue.value for queue in cls} — it includes every enum member. The consolidation queue is a Redis Hash, not a List. But _get_existing_issue_keys_from_redis in the fetcher calls lrange on every queue from all_queues(). Calling lrange on a Hash key triggers a Redis WRONGTYPE error on every fetcher cycle. The per-queue except Exception catch prevents a crash, but it produces a spurious warning log on every run.

Fix options: Either exclude the consolidation queue from all_queues() (rename to exclude it, or filter it out), or move it out of the RedisQueues enum since it's semantically different (a hash, not a queue).

Bug: JiraLabels.MR_CONSOLIDATED value doesn't match the GitLab label used in code

JiraLabels.MR_CONSOLIDATED = "ymir_mr_consolidated" but the actual label applied in mark_original_mrs and filtered in list_open_mrs is the string literal "ymir_consolidated" (without _mr_). The constant is defined but never referenced by the code that applies the label. If someone later refactors to use the constant, the labels won't match. Either the constant value should be "ymir_consolidated" or the code should reference the constant. Same issue: MR_CONSOLIDATION_ERRORED is defined but unused anywhere in this PR.

Issue: Undeclared dynamic attributes on Pydantic model

state._all_open_mrs (line 968 in mr_consolidation_agent.py) and state._current_mrs_count (line 1098) are set on ConsolidationState but never declared as fields. Pydantic v2 allows underscore-prefixed attributes as private attrs (bypassing validation), so this works at runtime, but it's fragile — these are load-bearing state used across workflow steps. They should be declared as proper fields on ConsolidationState (e.g., all_open_mrs: list[dict] = Field(default_factory=list) and current_mrs_count: int = Field(default=0)).

Issue: release_strategy from PackageConsolidationConfig is never propagated

The PackageConsolidationConfig model defines a release_strategy field, and the architecture doc describes two strategies ("merged" vs "per_commit"). But fetch_consolidation_config (called in the backport agent) only checks config.merge_mrs — it never reads or forwards release_strategy. The MergeConsolidationJob model also lacks a release_strategy field. The queue-mode worker always uses the default "merged". The config field and the architecture doc section describe a feature that isn't wired up.

Issue: test_consolidated_patches_consistent contradicts the agent's purpose

This test (line 2508) asserts byte-equality between backport patches and consolidated patches: assert consolidated_patches[patch_name] == patch_content. But the agent's core job is to adapt overlapping patches — the instructions explicitly say to modify context lines when patches touch the same files. The LLM judge evaluator correctly says "NOT by whether it is byte-identical," but this deterministic test would fail for any test case where adaptation was needed. It currently skips for cached backports, so it may not fire yet, but it will produce false failures once live backport test cases exercise overlapping patches.

Nit: Misleading noqa comment in tasks.py

from ymir.common.merge_queue import (  # noqa: F401 — re-exported for backward compatibility

This is brand-new code — there is no backward compatibility concern. The comment should say something like "re-exported for use by unit tests and consolidation agent" or just drop the explanation.

Nit: MergeConsolidationJob docstring has wrong Redis key name

The docstring says merge_consolidation_jobs but the actual key is merge_consolidation_queue (from RedisQueues.MERGE_CONSOLIDATION_QUEUE.value).

Nit: CloseMergeRequestTool is registered but unused

CloseMergeRequestTool is added to the gateway (6 lines in gateway.py, 55 lines in gitlab.py), but the architecture doc explicitly states "Original MRs are labeled ymir_consolidated (not closed)" and the workflow only calls add_merge_request_labels. This is dead code in this PR. If it's intended for future use, it could be added in a separate PR when needed.

Observation: submit_merge_job check-then-set is not atomic

The pending-exists check and subsequent hset in submit_merge_job are two separate Redis calls. A race between two concurrent backport agents could result in both seeing no pending key and both creating one (second overwrites first). Since auto-mode jobs are equivalent (the consolidation agent queries GitLab regardless), this is harmless for auto-mode. But for label-triggered mode with different source_issues, a race could lose the first job's issue keys. The risk is low given the fetcher runs as a single instance, but worth noting.

Observation: ListProjectMergeRequestsTool has no pagination

The tool fetches at most 100 MRs (per_page: "100") with no pagination loop. Unlikely to be a problem for a single package/branch, but worth documenting the limit.

@TomasKorbar TomasKorbar force-pushed the mr_consolidation_agent branch 2 times, most recently from e5e4917 to df045e8 Compare July 1, 2026 16:03
@TomasKorbar

TomasKorbar commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator Author

@nforro I addressed your Claudes suggestions :D all should be good now

Introduces a new agent that automatically consolidates multiple backport
merge requests for the same package-branch into a single comprehensive MR.

Key components:
- Redis Hash-based queue with atomic Lua script for concurrent worker
  safety (at-most-one-active, one-pending invariant per package-branch)
- Backport agent integration: submits consolidation jobs after filing MRs
  when enabled via per-package consolidation.json config
- Stale MR filtering: rejects MRs not based on the current target branch
  HEAD, logging skipped MRs as warnings
- LLM-driven patch merging with size-based ordering heuristic (larger
  patch applied first) and automatic context-line adaptation for
  overlapping patches
- Build verification with retry loop (same pattern as backport agent)
- Automatic re-queue when more than 2 MRs need consolidation, chaining
  runs until all are folded into one
- Label-triggered consolidation: maintainers can add ymir_consolidate_base
  and ymir_consolidate_next Jira labels to request consolidation of two
  specific MRs; the fetcher pairs them and submits a targeted job
- Post-consolidation Jira updates: comments the consolidated MR URL on
  all involved Jira issues
- Original MRs are labeled ymir_consolidated (not closed) and excluded
  from future consolidation searches
- Standalone mode via Makefile for manual testing
- E2E tests with cached backport replay and LLM-as-a-judge evaluation
- Architecture documentation for developer reference

Co-authored-by: Cursor <cursoragent@cursor.com>
@TomasKorbar TomasKorbar force-pushed the mr_consolidation_agent branch from 39370d8 to ca97097 Compare July 2, 2026 07:23
lbarcziova
lbarcziova previously approved these changes Jul 2, 2026

@lbarcziova lbarcziova left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

amazing work, thank you so much! 🚀

few remarks, but I don't want to block this so that we can check it out on prod ASAP. The most important is the one about config file name.

Comment thread ymir/agents/prompts/mr_consolidation/instructions.j2
Comment thread ymir/common/constants.py

RETRY_NEEDED = "ymir_retry_needed"

MR_CONSOLIDATED = "ymir_consolidated"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

iiuc this is always just backports, right? Then I would maybe rename this to ymir_consolidated_backports or similar.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't suppose there would be two different rebases for a single release, but a rebase and a backport (or more) are IMHO quite possible (even if the backport is effectively obsoleted by the rebase).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

And there even could be pure spec change that would have to be consolidated with backport, so perhaps ymir_consolidated is more appropriate 🤷‍♂️

Comment thread ymir/agents/tasks.py Outdated
Comment thread ymir/common/models.py Outdated
TomasTomecek
TomasTomecek previously approved these changes Jul 2, 2026

@TomasTomecek TomasTomecek left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's go!!! this is amazing


verdict = asyncio.run(evaluator.evaluate(artifacts, context))

assert verdict.passed, f"LLM judge FAILED for {test_case.name}:\n{verdict.reasoning}"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

you're the best, even adding an e2e test case

and checks out the target branch. Then fetches all candidate MR branches into the local
clone.

#### Stale MR Filtering

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I understand this requirement but am also concerned that people would prefer to also include stale MRs, let's see in the next few months in the feedback how this works

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Perhaps we will extend this if neccessary then.

Comment thread docs/mr_consolidation_architecture.md Outdated
@TomasKorbar TomasKorbar dismissed stale reviews from TomasTomecek and lbarcziova via 5bd455c July 2, 2026 09:41
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.

4 participants