Add Phase 2 to MR cleanup: reset Jira labels for closed bot MRs

[lbarcziova]

No description provided.

[gemini-code-assist]

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

[qodo-for-packit]

PR Summary by Qodo

Add phase 2 MR cleanup to reset Jira labels for closed bot MRs

✨ Enhancement 📝 Documentation ⚙️ Configuration changes 🕐 40+ Minutes

AI Description

• Add phase 2 to reset Jira ymir_* outcome labels for closed bot-authored MRs.
• Add per-phase toggles and configurable bot authors; remove single-MR TARGET_MR mode.
• Document new Jira/MR labels and workflow transitions for MR-closed issues.

Diagram

graph TD
  Cron["OpenShift CronJob"] --> Cleanup["mr_cleanup.py"]
  Cleanup --> GitLab["GitLab API"] --> OpenMRs["Open bot MRs"] --> Phase1["Phase 1: close stale"] --> GitLab
  GitLab --> ClosedMRs["Closed bot MRs"] --> Phase2["Phase 2: reset Jira labels"] --> Jira["Jira API"]
  Phase1 --> Jira

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Reset Jira labels immediately when phase 1 closes an MR

• ➕ Fewer total GitLab scans (no separate closed-MR sweep for phase-1-closed MRs)
• ➕ Tighter coupling between “MR closed” event and Jira label state
• ➖ Doesn’t cover MRs closed outside phase 1 (manual rejects, other automation) without additional logic
• ➖ Harder to preserve the current separation between “MR cleanup” and “Jira relabeling” concerns

2. Event-driven approach (GitLab webhook → Jira relabel)

• ➕ Near-real-time Jira label correctness
• ➕ Avoids periodic scanning of closed MRs and repeated commit parsing
• ➖ Requires new infrastructure (webhook receiver, auth, retries, observability)
• ➖ More failure modes (dropped events) and operational overhead than a cron scan

3. Persist processed MR state in a datastore instead of MR labels

• ➕ Avoids writing “processing” labels back to GitLab MRs
• ➕ More flexible tracking (timestamps, partial progress, retries)
• ➖ Introduces stateful dependency (DB/Redis) and migrations/backups
• ➖ Loses the convenience of GitLab-native idempotency markers visible to operators

Recommendation: The current two-phase, cron-driven scan is a good fit for reliability and operational simplicity: it is idempotent via the GitLab marker label (ymir_jiras_cleaned_up), prevents incorrect Jira relabeling via the active-key guard (skip issues still referenced by open MRs), and deduplicates resets within a run. Consider the event-driven alternative only if scan volume becomes a bottleneck.

Files changed (6) +276 / -56

Enhancement (2) +233 / -29

constants.pyAdd JiraLabels.MR_CLOSED constant +2/-0

Add JiraLabels.MR_CLOSED constant

• Introduces the ymir_mr_closed label in the shared JiraLabels enum so it can be referenced consistently across the codebase.

ymir/common/constants.py

mr_cleanup.pyImplement phase 2 closed-MR Jira label reset with guards and idempotency +231/-29

Implement phase 2 closed-MR Jira label reset with guards and idempotency

• Adds phase toggles (CLOSE_STALE_MRS, RESET_CLOSED_MR_JIRAS) and configurable bot authors (GITLAB_BOT_AUTHORS). Implements a closed-MR scan that skips already-processed MRs, batch-fetches Jira labels, removes ymir_* outcome labels while preserving trigger/control labels, adds ymir_mr_closed, and stamps the MR with ymir_jiras_cleaned_up; also guards against Jiras still referenced by open MRs and deduplicates Jira resets within a run.

ymir/mr_cleanup/mr_cleanup.py

Documentation (2) +33 / -21

jira_label_workflow_routing.mdDocument ymir_mr_closed state and MR-level ymir_jiras_cleaned_up label +7/-3

Document ymir_mr_closed state and MR-level ymir_jiras_cleaned_up label

• Extends the Jira label state machine and label reference tables to include ymir_mr_closed as a non-merged terminal path. Documents the GitLab MR label ymir_jiras_cleaned_up and updates the document timestamp.

jira_label_workflow_routing.md

README.mdRewrite MR cleanup docs around two-phase behavior and new config +26/-18

Rewrite MR cleanup docs around two-phase behavior and new config

• Reframes the cronjob as two phases (close stale MRs, reset Jira labels for closed MRs). Adds a configuration table and updates usage examples, including phase-2-only runs with alternate bot accounts.

ymir/mr_cleanup/README.md

Other (2) +10 / -6

MakefileRemove TARGET_MR support from mr-cleanup run targets +2/-6

Remove TARGET_MR support from mr-cleanup run targets

• Drops the TARGET_MR env passthrough from dry-run and live run targets. Updates usage comments to reflect that runs now report general changes (not single-MR targeting).

Makefile

mr-cleanup.envAdd phase toggles and configurable bot authors to env template +8/-0

Add phase toggles and configurable bot authors to env template

• Adds optional environment variables to customize bot usernames scanned and to enable/disable each cleanup phase. Keeps defaults as comments for multi-deployment use.

templates/mr-cleanup.env

[qodo-for-packit]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 7 rules

1. MR marked reset on failure ✓ Resolved 🐞 Bug ☼ Reliability

Description

process_rejected_mr() always labels the MR with ymir_jiras_cleaned_up even when Jira label
fetch/reset was skipped or failed for one or more referenced issues. Because fetch_closed_mrs()
excludes MRs with that label, those skipped/failed Jira resets won’t be retried automatically.

Code

ymir/mr_cleanup/mr_cleanup.py[R433-447]

+        for key in sorted(jira_keys):
+            if key in skip_jira_keys:
+                logger.info("Skipping %s — still referenced by an open MR or already reset", key)
+                continue
+            current_labels = jira_labels.get(key)
+            if current_labels is None:
+                logger.warning("Could not fetch labels for %s (MR %s) — skipping issue", key, mr_url)
+                continue
+            try:
+                if self._reset_jira_labels(key, current_labels):
+                    reset_keys.add(key)
+            except Exception:
+                logger.exception("Failed to reset labels for %s (MR %s)", key, mr_url)
+
+        self._add_label(mr, JIRA_RESET_MR_LABEL)

Relevance

●●● Strong

Team has accepted fixes preventing “processed”/dedup labels from blocking retries (PR #540) and
improving failure handling (PR #675).

PR-#540
PR-#675

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Closed MRs are selected specifically to exclude those already labeled as processed, but the code
marks them as processed even when some Jira work was not done (missing label data or exceptions),
which prevents the cronjob from retrying those failures.

ymir/mr_cleanup/mr_cleanup.py[325-350]
ymir/mr_cleanup/mr_cleanup.py[352-377]
ymir/mr_cleanup/mr_cleanup.py[441-452]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`process_rejected_mr()` unconditionally calls `_add_label(mr, JIRA_RESET_MR_LABEL)` at the end, even when:
- Jira labels could not be fetched for an issue (`current_labels is None`), or
- `_reset_jira_labels()` raises an exception.

Since `fetch_closed_mrs()` filters out MRs with `not[labels]=ymir_jiras_cleaned_up`, a transient Jira failure can permanently prevent automatic reprocessing and leave Jira labels stale.

## Issue Context
The script is intended to be a daily cronjob; it should be able to recover from partial/transient failures by retrying later.

## Fix Focus Areas
- ymir/mr_cleanup/mr_cleanup.py[325-350]
- ymir/mr_cleanup/mr_cleanup.py[352-377]
- ymir/mr_cleanup/mr_cleanup.py[418-452]

## Suggested fix
Track whether all non-skipped Jira keys were successfully handled before applying `ymir_jiras_cleaned_up` to the MR:
- Maintain flags/counters like `had_failures` / `unprocessed_keys`.
- Only add `JIRA_RESET_MR_LABEL` if every key is either (a) intentionally skipped due to `skip_jira_keys`, or (b) successfully reset (or confirmed already in desired state).
- If any key couldn’t be fetched/reset due to errors, do **not** mark the MR as processed so it is retried on the next run (optionally log a summary of remaining keys).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Active keys miscomputed ✓ Resolved 🐞 Bug ≡ Correctness

Description

_run_stale_mr_cleanup() computes active_jira_keys as all_keys - closed_keys, which can drop a Jira
key that is still referenced by another MR that remained open. Phase 2 then may reset Jira labels
for an issue that still has an open MR referencing it.

Code

ymir/mr_cleanup/mr_cleanup.py[R505-529]

+    def _run_stale_mr_cleanup(self, mrs: list[dict]) -> set[str]:
+        """Returns the set of Jira keys still referenced by open (not-closed) MRs."""
+        mr_jira_keys, all_keys = self._extract_all_jira_keys(mrs)

        # Batch-query Jira statuses
        jira_statuses = self.fetch_jira_statuses(all_keys)
        logger.info("Fetched statuses for %d Jira issues", len(jira_statuses))

        # Process each MR
+        closed_keys: set[str] = set()
        results: dict[Action, list[str]] = {}
        for mr in mrs:
            try:
                keys = mr_jira_keys.get(mr["id"])
                action = Action.ERRORED if keys is None else self.process_mr(mr, keys, jira_statuses)
+                if action == Action.CLOSED and keys:
+                    closed_keys.update(keys)
            except Exception:
                logger.exception("Failed to process MR %s", mr.get("web_url", mr.get("id")))
                action = Action.ERRORED
            results.setdefault(action, []).append(mr["web_url"])

        summary = {action.value: len(urls) for action, urls in results.items()}
-        logger.info("MR cleanup complete: %s", summary)
-        if self.dry_run and (urls := results.get(Action.CLOSED)):
-            logger.info("DRY_RUN — would close %d MRs:", len(urls))
-            for url in urls:
-                logger.info("  %s", url)
+        logger.info("Stale MR cleanup complete: %s", summary)
+        return all_keys - closed_keys

Relevance

●●● Strong

Repo reviewers favor correctness around “active vs closed” tracking to avoid acting on still-active
items (retry/anchor bug fixes in PR #540).

PR-#540

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Phase 2’s skip set is derived from _run_stale_mr_cleanup(); since that function subtracts a union
of keys from closed MRs, any Jira key referenced by both a closed MR and a still-open MR can be
erroneously omitted from the skip set, contradicting the documented intent to skip issues referenced
by open MRs.

ymir/mr_cleanup/mr_cleanup.py[454-473]
ymir/mr_cleanup/mr_cleanup.py[505-530]
ymir/mr_cleanup/README.md[11-14]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Phase 1 returns `active_jira_keys` using `return all_keys - closed_keys`, where `closed_keys` is the union of Jira keys from any MR that was closed by the script. If a Jira key appears in multiple open MRs and only some of them are closed (e.g., another MR is skipped and remains open), subtracting `closed_keys` incorrectly removes that key from the “active” set, allowing phase 2 to reset Jira labels even though an open MR still references the issue.

## Issue Context
Phase 2 explicitly intends to skip Jira issues still referenced by an open MR.

## Fix Focus Areas
- ymir/mr_cleanup/mr_cleanup.py[505-529]
- ymir/mr_cleanup/mr_cleanup.py[454-473]

## Suggested fix
In `_run_stale_mr_cleanup`, build the returned set from the keys belonging to MRs that remain open after processing (i.e., those whose `action != Action.CLOSED`), instead of using `all_keys - closed_keys`. For example:
- initialize `active_keys=set()`
- for each MR: after computing `action`, if `action != Action.CLOSED` then `active_keys.update(keys)`
- return `active_keys`
This guarantees phase 2 skips any Jira key still referenced by an open MR.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

[lbarcziova]

@antbob I extended our auto-closing script to do the cleanup on Jiras as well which your team was interested in (see referenced Jira), I made it possible to skip the auto-closing and configure the Gitlab bot name(s) for which to run the clean-up. Let me know if this works for you!

[antbob]

@lbarcziova thats great, thanks!

[opohorel]

wouldn't it make sense to add "ymir_merged" here, so we can be sure it never gets removed?

[lbarcziova]

makes sense, even though iirc ymir_merged is set from the QE agents which are not yet deployed.

But this made me realise a bigger problem: scenario where RHEL-100 has two bot MRs - MR-A merged, MR-B closed. Phase 2 picks up MR-B and wants to reset RHEL-100's labels. That doesn't look correct. I will think about what would be the best way to avoid this (first, but expensive solution would be to parse all the resolved Jiras from the merged MRs).

[opohorel]

LGTM, thanks!

[lbarcziova]

@opohorel PTAL, the last commit changes it a bit, the ymir_mr_closed label is now added to Jiras referenced by closed bot MRs but preserves existing automation labels (e.g. ymir_backported); I realised stripping them would break coverage metrics

[opohorel]

LGTM, thanks for looking into this!