Rebase consolidation improvements and code review fixes - #726
Open
majamassarini wants to merge 15 commits into
Open
Rebase consolidation improvements and code review fixes#726majamassarini wants to merge 15 commits into
majamassarini wants to merge 15 commits into
Conversation
PR Summary by QodoConsolidate rebase MRs across sibling Jira issues + version-aware triage fixes
AI Description
Diagram
High-Level Assessment
Files changed (10)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
7 rules 1.
|
![qodo-for-packit[bot]](https://avatars.githubusercontent.com/in/4066991?s=60&v=4){kind=small}
Member
Author
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit ec90fb4 |
Implement consolidation logic for rebase merge requests to group multiple CVEs targeting the same (component, stream, version) into a single MR, matching the existing rebuild consolidation pattern. Changes: - Add rebase_consolidation.py module with find_rebase_siblings() to discover and verify sibling issues requiring the same rebase - Extend RebaseData model with consolidated_issues and consolidation_summary fields, plus all_jira_issues property - Integrate consolidate_rebase_siblings workflow step in triage agent with routing logic for REBASE resolution - Update rebase agent to handle consolidated issues: include all in MR description, comment on all issues, label all issues - Add unit tests for JQL building and RebaseData properties Impact: Reduces duplicate MRs significantly (e.g., 51 dotnet CVEs → ~10 grouped MRs by stream). Improves reviewer efficiency and maintains consistency with rebuild workflow. https://redhat.atlassian.net/browse/PACKIT-5186 Assisted-by: Claude Sonnet 4.5 <noreply@anthropic.com>
1. Fix format_jira_links_for_mr call - pass list not unpacked args - Was: format_jira_links_for_mr(*all_issues) with 13 issues - Now: format_jira_links_for_mr(all_issues) as expected - Fixes: "takes 1 positional argument but 13 were given" error 2. Add REBASE to applicability check routing - Include Resolution.REBASE in determine_target_branch routing - Ensures rebase issues go through consolidation workflow 3. Label consolidated siblings to prevent duplicate work - Set ymir_triaged_rebase on all consolidated siblings - Siblings skip re-triage (existing terminal label check) - Only primary issue queued for rebase = single build Result: 13 dotnet CVEs now trigger ONE rebase+build instead of 13 Assisted-by: Claude Sonnet 4.5 <noreply@anthropic.com>
When a rebase fails, all consolidated siblings must be updated with error labels so they can be re-triaged. Previously only the primary issue was labeled, leaving siblings stuck with ymir_triaged_rebase. Changes: - On graceful failure: label all issues with ymir_rebase_failed - On exception/retry exhaustion: label all with ymir_rebase_errored - Both remove ymir_triaged_rebase from all issues Result: Failed rebases no longer leave orphaned issues that can't retry Assisted-by: Claude Sonnet 4.5 <noreply@anthropic.com>
Prevent circular consolidation where two issues each find the other as a sibling when one has already been labeled as consolidated. Scenario: - RHEL-100 triages first, finds RHEL-102 as sibling - Labels RHEL-102 with ymir_triaged_rebase - RHEL-102 later re-triaged (via ymir_todo) - Without this fix: RHEL-102 finds RHEL-100 as sibling - Result: circular consolidation, duplicate MRs Fix: Add ymir_triaged_rebase to excluded labels in JQL query Assisted-by: Claude Sonnet 4.5 <noreply@anthropic.com>
When triage identifies rebase siblings for consolidation, each sibling now gets a comment linking to the primary issue. This provides immediate visibility into which issue will handle the actual rebase work. The comment is posted during triage (when siblings are labeled), so users know upfront which issue to follow for status updates. Rebase agent already handles comments on both success and failure: - Success: all issues get MR link - Failure: primary gets error details, siblings get link to primary Comments are skipped in dry-run mode. Assisted-by: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
Removed code duplication by extracting the pattern for posting failure comments to consolidated siblings into a reusable helper function `post_failure_comments_to_consolidated_siblings`. This function is now used in both failure paths: - Graceful workflow failure (comment_in_jira step) - Retry exhaustion (final attempt error handling) No functional changes, just DRYer code. Assisted-by: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
Prevent consolidation of issues that were explicitly postponed during their own triage. If a sibling issue was already triaged and marked as postponed, it should not be included in another issue's rebase MR. This completes the set of excluded terminal triage labels: - ymir_triaged_not_affected - ymir_triaged_backport - ymir_triaged_rebuild - ymir_triaged_rebase - ymir_triaged_postponed (now added) Assisted-by: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
Triage now directly checks the current package version in dist-git before deciding on REBASE resolution. This prevents queuing unnecessary rebase work when the package was already rebased by another issue. **Triage agent changes:** - Before deciding on REBASE, check current version in spec file using rpmspec + rpmdev-vercmp - If package already at or beyond target version: * Search for the Jira issue that performed the rebase * Return NOT_AFFECTED instead of REBASE * Provide guidance referencing existing issue and build NVR * Suggest adding current issue to that build's Errata **Rebase agent instruction changes (defensive):** - Updated to return helpful guidance instead of hard error when "already at version" is detected - This is a safeguard in case triage misses the check Example scenario this fixes: - RHEL-192466 rebases dotnet8.0 to 8.0.129 - RHEL-211859 and 12 siblings (CVEs for same package) get triaged - Old behavior: triage returns REBASE, all queued, rebase agent errors - New behavior: each independently triaged as NOT_AFFECTED with link to RHEL-192466 and guidance to add to Errata Note: NOT_AFFECTED issues are NOT consolidated - each gets triaged separately with individual comments, which provides better visibility. Assisted-by: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
String equality check would incorrectly exclude siblings when LLM extracts target version in slightly different format (e.g., "1.2.3" vs "1.2.3-rc1"). Use rpmdev-vercmp for proper RPM version comparison. Assisted-by: Claude Sonnet 4.5 (200k context) <noreply@anthropic.com>
Sequential LLM calls for sibling analysis caused significant delays, as each candidate required both eligibility check and version analysis. Now use asyncio.gather to parallelize all candidate analysis. Sequential Jira API calls (comments and labels) for consolidated issues also created unnecessary delays. All Jira updates now run in parallel using asyncio.gather across affected issues. This improves overall workflow responsiveness when processing issues with multiple consolidated siblings. Assisted-by: Claude Sonnet 4.5 (200k context) <noreply@anthropic.com>
Issue packit#8: Label update loops duplicated 3x with identical all_issues reconstruction. Extracted update_labels_for_all_issues() helper that handles primary + consolidated siblings in parallel. Used in success, failure, and error paths. Issue packit#6: JQL building logic duplicated between build_rebase_siblings_jql and build_rebuild_siblings_jql. Extracted build_siblings_jql() utility accepting excluded_labels list. Both functions now delegate to shared implementation. Uses JiraLabels enum constants instead of string literals. Assisted-by: Claude Sonnet 4.5 (200k context) <noreply@anthropic.com>
Tests incorrectly expected ymir_triaged_rebase to NOT be in the excluded labels list. However, commit 530fccb specifically added this exclusion to prevent circular consolidation: Scenario: RHEL-100 triages first and labels RHEL-102 as consolidated sibling (ymir_triaged_rebase). If RHEL-102 is later re-triaged, it would find RHEL-100 as a sibling, creating circular consolidation. Updated tests to: - Assert ymir_triaged_rebase IS in the excluded labels - Assert ymir_triaged_postponed IS in the excluded labels - Update docstring to clarify purpose (prevent circular consolidation) Assisted-by: Claude Sonnet 4.5 (200k context) <noreply@anthropic.com>
Previously, asyncio.gather() on comment_in_jira() calls would abort the entire workflow step if a single Jira comment failed. This was risky because the comment step runs late in the workflow (after rebase/build/MR creation), so a transient Jira failure could convert a successful run into an exception path and trigger retries. Changes: - Added post_comments_to_all_issues() helper that wraps each comment in try/except and logs failures without propagating - Updated success path to use the helper (replaces bare gather) - Wrapped primary issue error comments in try/except - Updated post_failure_comments_to_consolidated_siblings() to isolate per-sibling failures using gather with internal error handling Now a single Jira comment failure logs a warning but doesn't abort the workflow or prevent comments to other issues. Assisted-by: Claude Sonnet 4.5 (200k context) <noreply@anthropic.com>
Previously, post_failure_comments_to_consolidated_siblings() used is_error=True when calling comment_in_jira(). This caused the comment to be skipped on non-user-triggered (automatic) runs, because comment_in_jira() intentionally suppresses error comments unless user_triggered=True to avoid spamming maintainers. However, the sibling link comment is informational (pointing to where error details can be found), not a noisy error notification. It should always be posted so consolidated siblings have a reference to the primary issue. Changed is_error=False so these informational link comments are posted on all runs (automatic and user-triggered). Only the detailed error comment on the primary issue remains gated by user_triggered. Assisted-by: Claude Sonnet 4.5 (200k context) <noreply@anthropic.com>
Previously, parallel comment/label functions built all_issues by concatenating the primary issue with consolidated sibling keys without deduplication. If duplicates were present in consolidated_issues (or the primary issue appeared in the sibling list), the same Jira issue would receive multiple identical comments/label updates. Changes: - update_labels_for_all_issues(): Deduplicate with dict.fromkeys() - post_comments_to_all_issues(): Deduplicate with dict.fromkeys() - post_failure_comments_to_consolidated_siblings(): Deduplicate by building a dict keyed by issue_key This makes the functions robust against malformed/duplicated inputs while preserving order (primary issue is always first). Assisted-by: Claude Sonnet 4.5 (200k context) <noreply@anthropic.com>
majamassarini
force-pushed
the
group-rebases
branch
from
July 31, 2026 10:43
59f6de4 to
a2b67a7
Compare
Member
Author
|
/agentic_review |
Comment on lines
+159
to
+173
| """Update Jira labels for primary issue and all consolidated siblings in parallel.""" | ||
| # Deduplicate in case consolidated_issues contains duplicates or the primary issue | ||
| all_issues = list(dict.fromkeys([primary_issue] + [item.issue_key for item in consolidated_issues])) | ||
| await asyncio.gather( | ||
| *[ | ||
| tasks.set_jira_labels( | ||
| jira_issue=issue, | ||
| labels_to_add=labels_to_add, | ||
| labels_to_remove=labels_to_remove, | ||
| dry_run=dry_run, | ||
| user_triggered=user_triggered, | ||
| ) | ||
| for issue in all_issues | ||
| ] | ||
| ) |
There was a problem hiding this comment.
1. Label fanout aborts on error 🐞 Bug ☼ Reliability
In rebase_agent.update_labels_for_all_issues(), a single Jira label update failure will raise out of asyncio.gather() and can interrupt the overall rebase processing while leaving some issues labeled and others not. This affects success/failure/error paths that now label primary + consolidated siblings together.
Agent Prompt
### Issue description
`update_labels_for_all_issues()` uses `asyncio.gather()` without per-issue exception isolation. If `tasks.set_jira_labels()` raises for one issue, the whole await raises, which can disrupt the rebase task and leave inconsistent labels across the primary/sibling issues.
### Issue Context
This helper is used from multiple rebase paths (final retry exhaustion, success, and failure), so it needs to be resilient to partial Jira outages.
### Fix Focus Areas
- ymir/agents/rebase_agent.py[151-173]
- ymir/agents/rebase_agent.py[618-626]
- ymir/agents/rebase_agent.py[703-714]
- ymir/agents/rebase_agent.py[723-731]
### Suggested fix
- Wrap each `tasks.set_jira_labels(...)` call in a small inner coroutine with `try/except` (like the comment fanout helper), log warnings per issue, and continue.
- OR use `asyncio.gather(..., return_exceptions=True)` and iterate results to log failures.
- Ensure the helper never raises due to a single sibling label failure (unless you explicitly want to fail the whole workflow).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+183
to
+185
| if analysis.requires_same_rebase: | ||
| cmp_result = compare_versions(analysis.target_version, rebase_data.version) | ||
| if cmp_result == 0: |
There was a problem hiding this comment.
2. Missing target_version breaks compare 🐞 Bug ≡ Correctness
find_rebase_siblings() calls compare_versions(analysis.target_version, ...) when requires_same_rebase is true, but target_version is optional in the SiblingRebaseAnalysis schema. If the LLM returns null for target_version, the comparison raises and that sibling is treated as an analysis failure (excluded).
Agent Prompt
### Issue description
`SiblingRebaseAnalysis.target_version` is nullable, but the code assumes it’s always present when `requires_same_rebase` is true and passes it into `compare_versions()`. A null value triggers an exception and causes an avoidable false-negative (sibling excluded).
### Issue Context
The exception is caught and the candidate is excluded, so the workflow won’t crash, but consolidation quality suffers and debugging is harder.
### Fix Focus Areas
- ymir/agents/rebase_consolidation.py[85-95]
- ymir/agents/rebase_consolidation.py[183-207]
### Suggested fix
- Add an explicit guard:
- If `analysis.requires_same_rebase` and not `analysis.target_version`, return an exclusion summary like "missing target_version" without calling `compare_versions()`.
- Alternatively, enforce a Pydantic validator: `requires_same_rebase=True` => `target_version` must be non-null (and ideally non-empty).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+214
to
+215
| # Analyze all candidates in parallel | ||
| results = await asyncio.gather(*[analyze_candidate(c) for c in candidates]) |
There was a problem hiding this comment.
3. Unbounded sibling analysis concurrency 🐞 Bug ➹ Performance
find_rebase_siblings() gathers analysis for all candidates at once; with max_results=50 this can spawn many concurrent eligibility checks and LLM/Jira calls. This can increase latency and reduce successful consolidation under throttling or transient Jira/model issues.
Agent Prompt
### Issue description
Sibling analysis runs with `asyncio.gather()` over all candidates, with no concurrency cap. With up to 50 siblings, this can overwhelm downstream services and increase failures/timeouts.
### Issue Context
Each candidate can trigger multiple remote operations (eligibility tool + LLM agent using Jira tools), so even moderate sibling counts can be expensive.
### Fix Focus Areas
- ymir/agents/rebase_consolidation.py[125-131]
- ymir/agents/rebase_consolidation.py[214-229]
### Suggested fix
- Introduce a concurrency limit via `asyncio.Semaphore` (e.g., 5–10) around `analyze_candidate`.
- Or process candidates in batches (chunks) and `await gather()` per batch.
- Keep per-candidate exception handling as-is so one failure doesn’t abort the whole consolidation.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+35
to
+41
| try: | ||
| result = subprocess.run( # noqa: S603 | ||
| ["rpmdev-vercmp", version1, version2], # noqa: S607 | ||
| capture_output=True, | ||
| text=True, | ||
| check=False, | ||
| ) |
There was a problem hiding this comment.
4. Version compare blocks event loop 🐞 Bug ☼ Reliability
compare_versions() uses synchronous subprocess.run() without a timeout, and it is called from async sibling analysis; a slow or stuck rpmdev-vercmp call will block the event loop and stall consolidation progress. This can manifest as long pauses during triage/rebase sibling analysis.
Agent Prompt
### Issue description
`compare_versions()` runs `rpmdev-vercmp` via blocking `subprocess.run()` with no timeout. Because it’s called inside async code, it blocks the event loop while executing and has no upper bound on runtime.
### Issue Context
This function is currently used by rebase sibling consolidation during triage.
### Fix Focus Areas
- ymir/common/version_utils.py[19-55]
- ymir/agents/rebase_consolidation.py[183-186]
### Suggested fix
- Add a small timeout to `subprocess.run(..., timeout=...)` and handle `subprocess.TimeoutExpired` by raising a clear `RuntimeError`.
- To avoid blocking the event loop, either:
- run the subprocess call via `await asyncio.to_thread(compare_versions, v1, v2)` at the call site, or
- refactor to use `asyncio.create_subprocess_exec` in an async version comparator helper.
- Keep arguments as a list (no shell) to preserve safety.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit a2b67a7 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR implements rebase consolidation (grouping multiple CVE issues into a single rebase MR) and addresses code review findings for performance and maintainability.
Key Features
Rebase Consolidation
Code Review Fixes
rpmdev-vercmpinstead of string equality for sibling version matchingasyncio.gather()for sibling analysis and Jira API callsChanges
Consolidation Implementation
Performance & Maintainability
build_siblings_jql()for JQL construction,update_labels_for_all_issues()for label updatescompare_versions()inversion_utils.pywrapsrpmdev-vercmpTesting
Tested against dotnet8.0 CVE issues (RHEL-211859 and 12 siblings):
Related Issues
Addresses feedback from rebase consolidation implementation review and Slack discussion about RHEL-211859 "already at version" error.
🤖 Generated with Claude Code