Fix: Include all terminal labels when checking sibling completion status

ymir/agents/rebase_consolidation.py

@@ -78,17 +78,41 @@ def build_rebase_siblings_jql(
         issue_key: Primary issue to exclude
         component: Package component
         fix_version: Target fix version
-        exclude_triaged: If True, exclude already-triaged issues (for queueing new siblings).
+        exclude_triaged: If True, exclude all terminal states (for queueing new siblings).
                         If False, include all siblings (for consolidating in rebase MR).
     """
     excluded = []
     if exclude_triaged:
+        # Exclude non-retriable terminal states to ensure JQL filtering before the 50-result limit.
+        # Post-query filtering is not equivalent because we might miss real pending siblings
+        # if there are >50 total candidates including many already-processed ones.
+        #
+        # Per jira_label_workflow_routing.md:
+        # - ERRORED labels (triage/backport/rebase_errored) block retry → exclude (terminal)
+        # - FAILED labels (backport/rebase_failed) may auto-retry → DO NOT exclude
+        #
+        # NOTE: Do NOT exclude ymir_rebase_sibling here - it's not a terminal triage state,
+        # it's a queueing marker. Excluding it here would break check_and_queue_primary_if_ready()
+        # which needs to find queued-but-not-started siblings to know if primary should wait.
+        # queue_siblings_for_triage() handles the re-queueing check in its defensive filter.
         excluded = [
+            # Triage decisions (non-retriable - sibling has been triaged and decided)
             JiraLabels.TRIAGED_NOT_AFFECTED.value,
             JiraLabels.TRIAGED_BACKPORT.value,
             JiraLabels.TRIAGED_REBUILD.value,
             JiraLabels.TRIAGED_REBASE.value,
             JiraLabels.TRIAGED_POSTPONED.value,
+            JiraLabels.TRIAGED.value,  # Open-ended-analysis, no automated follow-up
+            # Completion labels (non-retriable - work successfully finished)
+            JiraLabels.BACKPORTED.value,
+            JiraLabels.REBASED.value,
+            JiraLabels.REBUILT.value,
+            # ERRORED labels (block retry, need human attention)
+            JiraLabels.TRIAGE_ERRORED.value,
+            JiraLabels.BACKPORT_ERRORED.value,
+            JiraLabels.REBASE_ERRORED.value,
+            JiraLabels.REBUILD_ERRORED.value,
+            JiraLabels.NEEDS_ATTENTION.value,  # Clarification-needed, blocked
         ]
     return build_siblings_jql(
         issue_key=issue_key,
@@ -192,25 +216,32 @@ async def queue_siblings_for_triage(
                 logger.info(f"Sibling {candidate_key} not eligible: {eligibility_result.reason}")
                 continue
 
-            # Check if already queued as sibling or already triaged (any resolution)
-            # Skip if already processed to avoid re-triaging completed issues
+            # Defensive check for non-retriable terminal labels (should already be filtered by JQL,
+            # but check again in case of Jira indexing delays or race conditions).
+            # Per jira_label_workflow_routing.md: FAILED labels are retriable, ERRORED block retry.
             candidate_labels, _ = await tasks.get_jira_issue_metadata(candidate_key)
-            terminal_labels = [
+            terminal_labels = {
                 JiraLabels.REBASE_SIBLING.value,
                 JiraLabels.TRIAGED_REBASE.value,
                 JiraLabels.TRIAGED_BACKPORT.value,
                 JiraLabels.TRIAGED_REBUILD.value,
                 JiraLabels.TRIAGED_NOT_AFFECTED.value,
                 JiraLabels.TRIAGED_POSTPONED.value,
+                JiraLabels.TRIAGED.value,
                 JiraLabels.BACKPORTED.value,
                 JiraLabels.REBASED.value,
                 JiraLabels.REBUILT.value,
-            ]
-            if any(label in candidate_labels for label in terminal_labels):
-                found_labels = [label for label in terminal_labels if label in candidate_labels]
+                JiraLabels.TRIAGE_ERRORED.value,
+                JiraLabels.BACKPORT_ERRORED.value,
+                JiraLabels.REBASE_ERRORED.value,
+                JiraLabels.REBUILD_ERRORED.value,
+                JiraLabels.NEEDS_ATTENTION.value,
+            }
+            found_terminal = terminal_labels.intersection(candidate_labels)
+            if found_terminal:
                 logger.info(
                     f"Sibling {candidate_key} already processed "
-                    f"(has terminal label: {found_labels}), skipping"
+                    f"(has non-retriable terminal label: {found_terminal}), skipping"
                 )
                 continue
 
@@ -392,27 +423,49 @@ async def check_and_queue_primary_if_ready(
             )
             return
 
-        # Use the same JQL builder to ensure we only get siblings of THIS primary
+        # Find siblings that are still pending (not finished processing).
+        # build_rebase_siblings_jql() excludes terminal triage states but NOT ymir_rebase_sibling,
+        # so queued-but-not-started siblings will be found (critical for correct readiness check).
         jql = build_rebase_siblings_jql(
             issue_key=primary_issue,
             component=component,
             fix_version=fix_version,
         )
-        # Check for siblings that are still processing: either still labeled as sibling
-        # (not started triage yet) OR in-progress (triage removes ymir_rebase_sibling
-        # at start, so we need to check both).
-        # Exclude siblings with terminal triage labels - they're done, even if they
-        # triaged to a different resolution (e.g. BACKPORT instead of REBASE).
+
+        # A sibling is "pending" (blocks the primary) if it has NOT finished processing.
+        # Pending states:
+        #   - ymir_rebase_sibling (queued but not started)
+        #   - ymir_triage_in_progress (currently being triaged)
+        #
+        # Terminal states (sibling is done, won't block primary):
+        #   - Any ymir_triaged_* label (triage complete, decision made)
+        #   - Any completion label (backported/rebased/rebuilt)
+        #   - Any error/failed label (won't proceed, even if retriable later)
         sibling_label = JiraLabels.REBASE_SIBLING.value
         in_progress_label = JiraLabels.TRIAGE_IN_PROGRESS.value
         terminal_labels = [
+            # Triage decisions
             JiraLabels.TRIAGED_REBASE.value,
             JiraLabels.TRIAGED_BACKPORT.value,
             JiraLabels.TRIAGED_REBUILD.value,
             JiraLabels.TRIAGED_NOT_AFFECTED.value,
             JiraLabels.TRIAGED_POSTPONED.value,
+            # Completions
+            JiraLabels.BACKPORTED.value,
+            JiraLabels.REBASED.value,
+            JiraLabels.REBUILT.value,
+            # Errors (won't proceed even if retriable)
+            JiraLabels.TRIAGE_ERRORED.value,
+            JiraLabels.BACKPORT_ERRORED.value,
+            JiraLabels.REBASE_ERRORED.value,
+            JiraLabels.REBUILD_ERRORED.value,
+            # Failures (won't proceed)
+            JiraLabels.BACKPORT_FAILED.value,
+            JiraLabels.REBASE_FAILED.value,
+            JiraLabels.REBUILD_FAILED.value,
         ]
         excluded = ", ".join(f'"{label}"' for label in terminal_labels)
+        # Find siblings that are pending (have sibling/in-progress label) AND not terminal
         jql_pending = (
             f'{jql} AND (labels = "{sibling_label}" OR labels = "{in_progress_label}") '
             f"AND labels not in ({excluded})"

ymir/agents/tests/unit/test_rebase_consolidation.py

@@ -24,15 +24,44 @@ def test_build_rebase_siblings_jql_escapes_component_quotes():
 
 
 def test_build_rebase_siblings_jql_excludes_correct_labels():
-    """Verify that rebase consolidation excludes terminal triage labels to prevent circular consolidation."""
+    """Verify that JQL excludes non-retriable states but includes retriable FAILED labels.
+
+    Per jira_label_workflow_routing.md:
+    - ERRORED labels (triage/backport/rebase_errored) block retry → exclude
+    - FAILED labels (backport/rebase_failed) may auto-retry → include (don't exclude)
+
+    This prevents missing pending siblings when there are >50 total candidates.
+    """
     jql = build_rebase_siblings_jql("RHEL-100", "python3.12", "rhel-9.8")
-    # Should exclude issues already triaged (prevents circular consolidation)
+
+    # Triage decisions (non-retriable)
     assert '"ymir_triaged_not_affected"' in jql
     assert '"ymir_triaged_backport"' in jql
     assert '"ymir_triaged_rebuild"' in jql
-    assert '"ymir_triaged_rebase"' in jql  # Prevents circular consolidation
+    assert '"ymir_triaged_rebase"' in jql
     assert '"ymir_triaged_postponed"' in jql
 
+    # Completion labels (non-retriable)
+    assert '"ymir_backported"' in jql
+    assert '"ymir_rebased"' in jql
+    assert '"ymir_rebuilt"' in jql
+
+    # ERRORED labels (block retry, must exclude)
+    assert '"ymir_triage_errored"' in jql
+    assert '"ymir_backport_errored"' in jql
+    assert '"ymir_rebase_errored"' in jql
+    assert '"ymir_rebuild_errored"' in jql
+
+    # FAILED labels (may auto-retry, must NOT exclude)
+    assert '"ymir_backport_failed"' not in jql
+    assert '"ymir_rebase_failed"' not in jql
+    assert '"ymir_rebuild_failed"' not in jql
+
+    # ymir_rebase_sibling must NOT be excluded - it's a queueing state, not a terminal state
+    # Excluding it would break check_and_queue_primary_if_ready() which needs to find
+    # queued-but-not-started siblings
+    assert '"ymir_rebase_sibling"' not in jql
+
 
 class TestSiblingCommentExtraction:
     """Tests for extracting and matching sibling references from Jira comments."""
@@ -180,3 +209,166 @@ def test_extract_multiple_inline_cards(self):
         assert "RHEL-200" in result
         assert "See" in result
         assert "and" in result
+
+
+class TestTerminalLabels:
+    """Behavioral tests for terminal label handling in sibling consolidation.
+
+    These tests verify that the production code actually excludes all terminal states,
+    preventing bugs like RHEL-248139 where primaries got stuck waiting for siblings
+    that had already finished with ymir_backported or ymir_backport_errored.
+    """
+
+    def test_jql_excludes_all_triage_decision_labels(self):
+        """JQL must exclude all triage decision labels to avoid re-queueing decided siblings."""
+        from ymir.common.constants import JiraLabels
+
+        jql = build_rebase_siblings_jql("RHEL-100", "postgresql", "rhel-9.8")
+
+        # Verify each triage decision label appears in the JQL exclusion
+        for label in [
+            JiraLabels.TRIAGED_REBASE.value,
+            JiraLabels.TRIAGED_BACKPORT.value,
+            JiraLabels.TRIAGED_REBUILD.value,
+            JiraLabels.TRIAGED_NOT_AFFECTED.value,
+            JiraLabels.TRIAGED_POSTPONED.value,
+            JiraLabels.TRIAGED.value,  # Open-ended-analysis
+            JiraLabels.NEEDS_ATTENTION.value,  # Clarification-needed
+        ]:
+            assert f'"{label}"' in jql, f"JQL must exclude {label} but it's missing from: {jql}"
+
+    def test_jql_excludes_all_completion_labels(self):
+        """JQL must exclude completion labels or primaries wait forever for completed siblings.
+
+        Regression test for RHEL-248139 where ymir_backported was not excluded.
+        """
+        from ymir.common.constants import JiraLabels
+
+        jql = build_rebase_siblings_jql("RHEL-100", "postgresql", "rhel-9.8")
+
+        # These were the missing labels that caused RHEL-248139
+        for label in [
+            JiraLabels.BACKPORTED.value,
+            JiraLabels.REBASED.value,
+            JiraLabels.REBUILT.value,
+        ]:
+            assert f'"{label}"' in jql, f"JQL must exclude {label} but it's missing from: {jql}"
+
+    def test_jql_excludes_errored_labels(self):
+        """JQL must exclude ERRORED labels which block retry.
+
+        Per jira_label_workflow_routing.md: ERRORED labels (triage/backport/rebase_errored)
+        block retry and need human attention, so they're terminal for sibling queueing.
+        """
+        from ymir.common.constants import JiraLabels
+
+        jql = build_rebase_siblings_jql("RHEL-100", "postgresql", "rhel-9.8")
+
+        # ERRORED labels block retry → must exclude
+        for label in [
+            JiraLabels.TRIAGE_ERRORED.value,
+            JiraLabels.BACKPORT_ERRORED.value,
+            JiraLabels.REBASE_ERRORED.value,
+            JiraLabels.REBUILD_ERRORED.value,
+        ]:
+            assert f'"{label}"' in jql, (
+                f"JQL must exclude {label} (blocks retry) but it's missing from: {jql}"
+            )
+
+    def test_jql_includes_failed_labels(self):
+        """JQL must NOT exclude FAILED labels which may auto-retry.
+
+        Per jira_label_workflow_routing.md: FAILED labels (backport/rebase_failed)
+        "May auto-retry", so excluding them breaks the retry mechanism where a new
+        sibling triggers re-queueing of failed issues.
+        """
+        from ymir.common.constants import JiraLabels
+
+        jql = build_rebase_siblings_jql("RHEL-100", "postgresql", "rhel-9.8")
+
+        # FAILED labels may auto-retry → must NOT exclude
+        for label in [
+            JiraLabels.BACKPORT_FAILED.value,
+            JiraLabels.REBASE_FAILED.value,
+            JiraLabels.REBUILD_FAILED.value,
+        ]:
+            assert f'"{label}"' not in jql, (
+                f"JQL must NOT exclude {label} (may auto-retry) but it's excluded in: {jql}"
+            )
+
+    def test_jql_does_not_exclude_sibling_marker(self):
+        """JQL must NOT exclude ymir_rebase_sibling - it's a queueing state, not terminal.
+
+        Regression test: check_and_queue_primary_if_ready() needs to find queued siblings
+        that haven't started triage yet (have ymir_rebase_sibling label). If we excluded
+        this label, the primary would be released early while siblings are still pending.
+
+        queue_siblings_for_triage() handles the re-queueing check in its defensive filter.
+        """
+        from ymir.common.constants import JiraLabels
+
+        jql = build_rebase_siblings_jql("RHEL-100", "postgresql", "rhel-9.8")
+
+        assert f'"{JiraLabels.REBASE_SIBLING.value}"' not in jql, (
+            f"JQL must NOT exclude {JiraLabels.REBASE_SIBLING.value} (queueing state, not terminal)"
+        )
+
+    def test_jql_exclusion_applies_before_50_result_limit(self):
+        """Terminal labels must be excluded in JQL, not post-query, to avoid missing pending siblings.
+
+        If there are 60 siblings where 40 have terminal labels and 20 are pending:
+        - Correct: JQL excludes 40 terminal, returns 20 pending
+        - Bug: JQL returns first 50 (35 terminal + 15 pending), post-filter → miss 5 pending
+
+        This test verifies the exclusion is in the JQL string (server-side filtering).
+        """
+        from ymir.common.constants import JiraLabels
+
+        jql = build_rebase_siblings_jql("RHEL-100", "postgresql", "rhel-9.8")
+
+        # Critical: the exclusion MUST be in the JQL query string itself
+        assert "labels not in" in jql, "JQL must have 'labels not in' clause for server-side filtering"
+
+        # Spot-check a few terminal labels to ensure they're in the JQL, not filtered post-query
+        critical_labels = [
+            JiraLabels.BACKPORTED.value,  # Caused RHEL-248139
+            JiraLabels.BACKPORT_ERRORED.value,  # ERRORED blocks retry, must exclude
+            JiraLabels.TRIAGE_ERRORED.value,  # ERRORED blocks retry, must exclude
+        ]
+        for label in critical_labels:
+            assert f'"{label}"' in jql, (
+                f"Critical terminal label {label} must be in JQL for server-side filtering"
+            )
+
+        # FAILED labels must NOT be in JQL (they're retriable)
+        retriable_labels = [
+            JiraLabels.BACKPORT_FAILED.value,
+            JiraLabels.REBASE_FAILED.value,
+        ]
+        for label in retriable_labels:
+            assert f'"{label}"' not in jql, f"Retriable label {label} must NOT be excluded in JQL"
+
+    def test_queued_sibling_blocks_primary(self):
+        """Regression: Queued siblings with ymir_rebase_sibling must be found as pending.
+
+        Before fix: build_rebase_siblings_jql() excluded ymir_rebase_sibling, then
+        check_and_queue_primary_if_ready() added AND labels = "ymir_rebase_sibling",
+        resulting in zero matches. Primary was released while queued siblings were pending.
+
+        After fix: ymir_rebase_sibling is NOT excluded in JQL, so the pending query
+        correctly finds queued-but-not-started siblings.
+        """
+
+        # Simulate the pending-sibling query in check_and_queue_primary_if_ready()
+        jql = build_rebase_siblings_jql("RHEL-100", "postgresql", "rhel-9.8")
+
+        # The query should be able to find siblings with ymir_rebase_sibling
+        # This is the key fix: if ymir_rebase_sibling were excluded from JQL,
+        # then check_and_queue_primary_if_ready() adding:
+        #   AND (labels = "ymir_rebase_sibling" OR labels = "ymir_triage_in_progress")
+        # would return zero results (contradictory query: exclude X AND require X)
+
+        # The key assertion: ymir_rebase_sibling must NOT appear in the exclusion list
+        assert '"ymir_rebase_sibling"' not in jql, (
+            "ymir_rebase_sibling in exclusion list would make pending query contradictory"
+        )