Add MR Consolidation Agent for combining backport MRs#633
Conversation
There was a problem hiding this comment.
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.
1d862fb to
544617f
Compare
nforro
left a comment
There was a problem hiding this comment.
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 compatibilityThis 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.
e5e4917 to
df045e8
Compare
|
@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>
39370d8 to
ca97097
Compare
lbarcziova
left a comment
There was a problem hiding this comment.
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.
|
|
||
| RETRY_NEEDED = "ymir_retry_needed" | ||
|
|
||
| MR_CONSOLIDATED = "ymir_consolidated" |
There was a problem hiding this comment.
iiuc this is always just backports, right? Then I would maybe rename this to ymir_consolidated_backports or similar.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
And there even could be pure spec change that would have to be consolidated with backport, so perhaps ymir_consolidated is more appropriate 🤷♂️
TomasTomecek
left a comment
There was a problem hiding this comment.
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}" |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Perhaps we will extend this if neccessary then.
Introduces a new agent that automatically consolidates multiple backport
merge requests for the same package-branch into a single comprehensive MR.
Key components:
safety (at-most-one-active, one-pending invariant per package-branch)
when enabled via per-package consolidation.json config
HEAD, logging skipped MRs as warnings
patch applied first) and automatic context-line adaptation for
overlapping patches
runs until all are folded into one
and ymir_consolidate_next Jira labels to request consolidation of two
specific MRs; the fetcher pairs them and submits a targeted job
all involved Jira issues
from future consolidation searches