diff --git a/jira_label_workflow_routing.md b/jira_label_workflow_routing.md index c87139134..9058eb7be 100644 --- a/jira_label_workflow_routing.md +++ b/jira_label_workflow_routing.md @@ -102,7 +102,7 @@ flowchart TD |-------|---------|--------| | `ymir_retry_needed` | Trigger retry | Forces reprocessing | | `ymir_triaged` | Triage completed, no automated follow-up | Terminal state | -| `ymir_todo` | Maintainer-facing trigger for an e2e run | Fetcher swaps it for `ymir_triage_in_progress` on enqueue; only honored when the changelog shows the label was added by a member of the `Red Hat Employee` Jira group (verified per-issue, not via JQL). The triage run posts an ack comment and a result comment so the requester gets feedback. Default is silent — without `ymir_todo`, no comments are posted. | +| `ymir_todo` | Maintainer-facing trigger for an e2e run | Fetcher swaps it for `ymir_triage_in_progress` on enqueue; only honored when the changelog shows the label was added by a member of the `Red Hat Employee` Jira group (verified per-issue, not via JQL). The triage run posts an ack comment and a result comment so the requester gets feedback. Automatic runs remain quiet for normal results, but error comments are posted once after the final retry regardless of trigger. | | `ymir_consolidate_base` | Mark a backport MR for consolidation (base) | Paired with `ymir_consolidate_next` on another issue for the same package/branch. The fetcher matches the pair, submits a targeted consolidation job, removes both labels, and posts comments. | | `ymir_consolidate_next` | Mark a backport MR for consolidation (next) | Must be on a different issue than `ymir_consolidate_base`, for the same package/branch. | | `ymir_cli_triage` | Usage-tracking marker for CLI runs | Set at the start of a user-triggered CLI triage run. Prevents the fetcher from picking up the same issue in parallel. Persists on success as a usage-tracking marker; removed on failure so the fetcher can re-process the issue. Covered by the fetcher's stale-label recovery (IN_FLIGHT_LABELS) to handle SIGKILL/OOM. | @@ -177,7 +177,7 @@ flowchart TD ## Run Behaviour by Trigger and Flag -Two env-var flags affect pipeline behaviour: `DRY_RUN` and `JIRA_ALLOW_STATUS_CHANGES`. Verbosity is no longer controlled by an env var — the system is silent by default. The only way to opt into comments is per-issue, by adding `ymir_todo` (which flows through the task as `user_triggered=True`). +Two env-var flags affect pipeline behaviour: `DRY_RUN` and `JIRA_ALLOW_STATUS_CHANGES`. Verbosity is no longer controlled by an env var — the system is quiet by default. Adding `ymir_todo` (which flows through the task as `user_triggered=True`) opts into acknowledgement and normal result comments. Error comments are always posted regardless of trigger: workflow error resolutions are treated as no-MR results (posted immediately), and crash-based errors are posted once after the final retry. Important/Critical Y-stream CVEs may take the deterministic inheritance fast path before the normal backport agent. A restricted LLM adapts only the target @@ -194,8 +194,8 @@ so clone or queue retries continue only through normal backporting. Ground rules: -- **Default is silent.** No result or error comments are posted on the Jira issue, and intermediate `_failed` labels are not written. Only `not-affected`, `postponed`, `open-ended-analysis`, and `clarification-needed` triage resolutions still post a comment unbidden (those have no MR to look at, so the comment is the only visible explanation). -- **`user_triggered=True`** (set on the task when the issue carried `ymir_todo`) **bypasses every silence filter.** The triage agent posts an immediate private ack comment, posts the result comment, and writes `_failed` labels normally. +- **Default is quiet.** Normal result comments are suppressed on automatic runs, and intermediate `_failed` labels are not written. Only `not-affected`, `postponed`, `open-ended-analysis`, and `clarification-needed` triage resolutions still post a comment unbidden (those have no MR to look at, so the comment is the only visible explanation). Error comments are posted once after the final retry via `post_terminal_error_comment()`, regardless of trigger. +- **`user_triggered=True`** (set on the task when the issue carried `ymir_todo`) enables an immediate private ack comment, normal result comments, and `_failed` labels. Error comments do not depend on this flag — they are posted once after retries are exhausted. - **Labels that are state, not notification, are always written.** `ymir_triage_in_progress` at the start of triage, terminal `ymir_*_errored` / `ymir_triaged_*` at the end. Suppressing them would break dedup against the next fetcher sweep. - **Jira workflow status changes are opt-in via `JIRA_ALLOW_STATUS_CHANGES`.** When the env var is unset or `false` (the default), the rebase/backport agents do NOT move the issue to "In Progress" on task pop, and the issue-verification agent does NOT transition issues to "Release Pending" / "Closed". When set to `true`, all of those transitions happen. The same flag also gates the preliminary-testing agent setting **`Preliminary Testing = Pass`** — that field admits the build into the next compose, triggers erratum creation, and moves the issue to Integration. Triage and the fetcher never touch the workflow status, regardless of the flag. - **`DRY_RUN` is read by both fetcher and agent.** On the fetcher, `DRY_RUN=true` skips the atomic Jira label flip (`ymir_todo` / `ymir_retry_needed` are NOT consumed; `ymir_triage_in_progress` is NOT stamped) but the task is still pushed to Redis with the correct `user_triggered` value, so the agent — also presumably in `DRY_RUN` — can exercise its full dry-mode flow. Implication: the trigger label stays on the issue, so every subsequent fetcher sweep re-picks the same issue. That is fine in a test environment; never run a production cron with `DRY_RUN=true`. `DRY_RUN=true` also implies status changes are skipped, independent of `JIRA_ALLOW_STATUS_CHANGES`. @@ -204,9 +204,9 @@ What happens for each trigger state: | Trigger state at sweep time | Default behaviour | `DRY_RUN=true` | |---|---|---| -| **No `ymir_*` labels** (fresh issue) | Fetcher pushes to `triage_queue`. Agent stamps `ymir_triage_in_progress`, runs triage, writes a terminal `ymir_*` label. Result comment is suppressed unless the resolution is `not-affected`, `postponed`, `open-ended-analysis`, or `clarification-needed`. If the run auto-chains to rebase or backport, the downstream agent moves the Jira workflow status to "In Progress" when it pops the task — **only if `JIRA_ALLOW_STATUS_CHANGES=true`**; otherwise the status is left untouched. | Agent runs triage but `set_jira_labels` / `add_jira_comment` short-circuit on `DRY_RUN`. No labels, no comment, no MR, no workflow status change. Issue untouched in Jira. | +| **No `ymir_*` labels** (fresh issue) | Fetcher pushes to `triage_queue`. Agent stamps `ymir_triage_in_progress`, runs triage, writes a terminal `ymir_*` label. Result comment is suppressed unless the resolution is `not-affected`, `postponed`, `open-ended-analysis`, or `clarification-needed`; error comments are posted once after the final retry. If the run auto-chains to rebase or backport, the downstream agent moves the Jira workflow status to "In Progress" when it pops the task — **only if `JIRA_ALLOW_STATUS_CHANGES=true`**; otherwise the status is left untouched. | Agent runs triage but `set_jira_labels` / `add_jira_comment` short-circuit on `DRY_RUN`. No labels, no comment, no MR, no workflow status change. Issue untouched in Jira. | | **`ymir_todo`** (added by a member of `Red Hat Employee`, no `_in_progress`) | Fetcher verifies the latest `ymir_todo` add in the issue's changelog was performed by a Red Hat Employee; if so, atomically flips `ymir_todo` → `ymir_triage_in_progress` and pushes with `user_triggered=True`. Agent posts a private ack comment and a result comment on completion. `_failed` labels are written normally. Workflow status change is the same as the fresh-issue path (set by rebase/backport on auto-chain, gated on `JIRA_ALLOW_STATUS_CHANGES`). | Fetcher still verifies the author and skips the atomic flip (`ymir_todo` stays on the issue), but still pushes to Redis with `user_triggered=True`. Agent runs in dry mode and writes nothing; workflow status not changed. **Subsequent fetcher sweeps will re-push the same issue** because the trigger label was never consumed. | -| **`ymir_retry_needed`** (no `_in_progress`) | Fetcher atomically flips `ymir_retry_needed` → `ymir_triage_in_progress`, pushes with `user_triggered=False`. Agent runs full triage; behaves exactly like a fresh-issue run (no ack comment, result comment only for the four "no-MR" resolutions). Workflow status change is the same as the fresh-issue path (gated on `JIRA_ALLOW_STATUS_CHANGES`). | Fetcher skips the atomic flip (`ymir_retry_needed` stays on the issue) but still pushes to Redis with `user_triggered=False`. Agent runs in dry mode and writes nothing; workflow status not changed. Subsequent fetcher sweeps will re-push the same issue. | +| **`ymir_retry_needed`** (no `_in_progress`) | Fetcher atomically flips `ymir_retry_needed` → `ymir_triage_in_progress`, pushes with `user_triggered=False`. Agent runs full triage; behaves exactly like a fresh-issue run (no ack comment, result comment only for the four no-MR resolutions; error comments posted once after the final retry). Workflow status change is the same as the fresh-issue path (gated on `JIRA_ALLOW_STATUS_CHANGES`). | Fetcher skips the atomic flip (`ymir_retry_needed` stays on the issue) but still pushes to Redis with `user_triggered=False`. Agent runs in dry mode and writes nothing; workflow status not changed. Subsequent fetcher sweeps will re-push the same issue. | | **`ymir_todo`** or **`ymir_retry_needed`** **+** any `ymir_*_in_progress` label | Fetcher skips. Not enqueued. Workflow status not affected. | Fetcher skips. Not enqueued. Workflow status not affected. | | **Any other terminal `ymir_*` label** (e.g. `ymir_triaged_rebase`, `ymir_rebased`, `ymir_triage_errored`) | Fetcher skips. Re-run by adding `ymir_todo` (recommended — produces an ack + result comment) or `ymir_retry_needed`. Workflow status not affected. | Fetcher skips. Workflow status not affected. | diff --git a/ymir/agents/backport_agent.py b/ymir/agents/backport_agent.py index af48e0271..4e7bd917b 100644 --- a/ymir/agents/backport_agent.py +++ b/ymir/agents/backport_agent.py @@ -2040,19 +2040,17 @@ async def finalize_failure(error: ErrorData, retry_queue: str, task, comment_tex user_triggered=user_triggered, ) # Crash paths have not reached the workflow's Jira-comment step. - if user_triggered and comment_text and not dry_run: + if comment_text and not dry_run: try: async with mcp_tools( os.environ["MCP_GATEWAY_URL"], call_meta={"jira_issue": backport_data.jira_issue}, ) as gateway_tools: - await tasks.comment_in_jira( + await tasks.post_terminal_error_comment( jira_issue=backport_data.jira_issue, agent_type="Backport", comment_text=comment_text, available_tools=gateway_tools, - is_error=True, - user_triggered=user_triggered, ) except Exception as comment_error: logger.warning( diff --git a/ymir/agents/rebase_agent.py b/ymir/agents/rebase_agent.py index 27b6af025..7ab15ff5c 100644 --- a/ymir/agents/rebase_agent.py +++ b/ymir/agents/rebase_agent.py @@ -244,7 +244,7 @@ async def post_failure_comments_to_consolidated_siblings( Post link comments to consolidated siblings pointing to primary issue with error details. Uses is_error=False so these informational links are always posted, even on automatic runs. - Only the detailed error on the primary issue is gated by user_triggered. + The primary terminal error is posted separately by the final retry path. Isolates errors per-sibling so a single Jira failure doesn't abort posting to other siblings. Deduplicates issue keys to prevent multiple identical comments. @@ -790,32 +790,26 @@ async def retry( user_triggered=user_triggered, ) # Post failure feedback to Jira once, here on the final attempt - # only — never for intermediate retries. Restricted to - # user-triggered (ymir_todo) runs: a maintainer who didn't ask - # for processing shouldn't be notified, so skip the gateway - # connection entirely otherwise. - if user_triggered and not dry_run: + # only — never for intermediate retries. + if comment_text and not dry_run: try: async with mcp_tools( os.environ["MCP_GATEWAY_URL"], call_meta={"jira_issue": rebase_data.jira_issue}, ) as gateway_tools: # Post detailed error to primary issue (with error handling) - if comment_text: - try: - await tasks.comment_in_jira( - jira_issue=rebase_data.jira_issue, - agent_type="Rebase", - comment_text=comment_text, - available_tools=gateway_tools, - is_error=True, - user_triggered=user_triggered, - ) - except Exception as e: - logger.warning( - f"Failed to post error comment to primary issue " - f"{rebase_data.jira_issue}: {e}" - ) + try: + await tasks.post_terminal_error_comment( + jira_issue=rebase_data.jira_issue, + agent_type="Rebase", + comment_text=comment_text, + available_tools=gateway_tools, + ) + except Exception as e: + logger.warning( + f"Failed to post error comment to primary issue " + f"{rebase_data.jira_issue}: {e}" + ) # Link consolidated siblings to primary issue # (with per-sibling error handling) await post_failure_comments_to_consolidated_siblings( diff --git a/ymir/agents/rebuild_agent.py b/ymir/agents/rebuild_agent.py index 72bdb2658..9b4332ef2 100644 --- a/ymir/agents/rebuild_agent.py +++ b/ymir/agents/rebuild_agent.py @@ -565,11 +565,8 @@ async def retry( except Exception as e: logger.warning(f"Failed to set labels on {issue_key}: {e}") # Post failure feedback to Jira once, here on the final attempt - # only — never for intermediate retries. Restricted to - # user-triggered (ymir_todo) runs: a maintainer who didn't ask - # for processing shouldn't be notified, so skip the gateway - # connection entirely otherwise. - if user_triggered and comment_text and not dry_run: + # only — never for intermediate retries. + if comment_text and not dry_run: try: async with mcp_tools( os.environ["MCP_GATEWAY_URL"], @@ -577,13 +574,11 @@ async def retry( ) as gateway_tools: for issue_key in dict.fromkeys(rebuild_data.all_jira_issues): try: - await tasks.comment_in_jira( + await tasks.post_terminal_error_comment( jira_issue=issue_key, agent_type="Rebuild", comment_text=comment_text, available_tools=gateway_tools, - is_error=True, - user_triggered=user_triggered, ) except Exception as comment_error: logger.warning( diff --git a/ymir/agents/tasks.py b/ymir/agents/tasks.py index 9d51f3814..1609869ac 100644 --- a/ymir/agents/tasks.py +++ b/ymir/agents/tasks.py @@ -159,13 +159,11 @@ async def handle_zstream_branch_stale_error( ) as gateway_tools: for issue_key in issues: try: - await comment_in_jira( + await post_terminal_error_comment( jira_issue=issue_key, agent_type=agent_type, comment_text=str(exc), available_tools=gateway_tools, - is_error=True, - user_triggered=True, # force-post regardless of actual trigger ) except Exception as comment_error: logger.warning( @@ -669,13 +667,45 @@ async def comment_in_jira( is_error: bool = False, user_triggered: bool = False, ) -> None: - # Default is silent: error comments are only posted on user-triggered runs. - # A maintainer who didn't ask for processing should not be spammed with - # error notifications; if they want to see them, they add ymir_todo. + # Mid-workflow errors (e.g. consolidation failures in backport/rebase) are + # trigger-gated here; crash-based and resolution-based terminal errors + # bypass this via post_terminal_error_comment() after retries are exhausted. if is_error and not user_triggered: logger.info(f"Skipping Jira error comment for {jira_issue} (not user-triggered)") return + await _post_jira_comment( + jira_issue=jira_issue, + agent_type=agent_type, + comment_text=comment_text, + available_tools=available_tools, + is_error=is_error, + ) + + +async def post_terminal_error_comment( + jira_issue: str, + agent_type: str, + comment_text: str, + available_tools: list[Tool], +) -> None: + """Post an error comment for a terminal failure, regardless of trigger.""" + await _post_jira_comment( + jira_issue=jira_issue, + agent_type=agent_type, + comment_text=comment_text, + available_tools=available_tools, + is_error=True, + ) + + +async def _post_jira_comment( + jira_issue: str, + agent_type: str, + comment_text: str, + available_tools: list[Tool], + is_error: bool, +) -> None: trace_server_url = trace_viewer_issue_url(jira_issue) if is_error and trace_server_url: comment_text = ( diff --git a/ymir/agents/tests/unit/test_tasks.py b/ymir/agents/tests/unit/test_tasks.py index f4cfd50c3..3b3a0af48 100644 --- a/ymir/agents/tests/unit/test_tasks.py +++ b/ymir/agents/tests/unit/test_tasks.py @@ -970,6 +970,34 @@ async def _mock_run_tool(*args, **kwargs): ) +@pytest.mark.asyncio +async def test_post_terminal_error_comment_adds_trace_link_without_user_trigger(monkeypatch): + monkeypatch.setenv("TRACE_VIEWER_URL", "https://trace.example/") + calls = [] + + async def _mock_run_tool(*args, **kwargs): + calls.append((args, kwargs)) + + flexmock(agent_tasks).should_receive("run_tool").once().replace_with(_mock_run_tool) + + await agent_tasks.post_terminal_error_comment( + jira_issue="RHEL-1", + agent_type="Backport", + comment_text="The backport failed.", + available_tools=[], + ) + + comment = calls[0][1]["comment"] + assert comment == JIRA_COMMENT_TEMPLATE.substitute( + AGENT_TYPE="Backport", + JIRA_COMMENT=( + "The backport failed.\n\n" + "See the [Ymir execution trace|https://trace.example/#/issues/RHEL-1] " + "for additional details." + ), + ) + + @pytest.mark.asyncio async def test_comment_in_jira_does_not_add_trace_link_to_non_error_comments(monkeypatch): monkeypatch.setenv("TRACE_VIEWER_URL", "https://trace.example/") @@ -1570,14 +1598,12 @@ async def _mock_lpush(queue, payload): task = _make_task(attempts=2) flexmock(agent_tasks).should_receive("set_jira_labels").twice().replace_with(_async_noop) - flexmock(agent_tasks).should_receive("comment_in_jira").once().replace_with(_async_noop) - flexmock(agent_tasks).should_receive("comment_in_jira").with_args( + flexmock(agent_tasks).should_receive("post_terminal_error_comment").once().replace_with(_async_noop) + flexmock(agent_tasks).should_receive("post_terminal_error_comment").with_args( jira_issue="RHEL-1", agent_type="Rebuild", comment_text=str(exc), available_tools=[], - is_error=True, - user_triggered=True, ).once().replace_with(_async_noop) flexmock(agent_tasks).should_receive("mcp_tools").replace_with(_mock_mcp_tools) @@ -1618,7 +1644,7 @@ async def _mock_incr(*_args, **_kwargs): redis.should_receive("incr").replace_with(_mock_incr) flexmock(agent_tasks).should_receive("set_jira_labels").once().replace_with(_async_noop) - flexmock(agent_tasks).should_receive("comment_in_jira").never() + flexmock(agent_tasks).should_receive("post_terminal_error_comment").never() flexmock(agent_tasks).should_receive("mcp_tools").replace_with(_mock_mcp_tools) await handle_zstream_branch_stale_error( diff --git a/ymir/agents/tests/unit/test_triage_agent.py b/ymir/agents/tests/unit/test_triage_agent.py index 27e8da1f2..7ca33cc1b 100644 --- a/ymir/agents/tests/unit/test_triage_agent.py +++ b/ymir/agents/tests/unit/test_triage_agent.py @@ -49,11 +49,10 @@ Resolution.POSTPONED_PR_PENDING, Resolution.OPEN_ENDED_ANALYSIS, Resolution.CLARIFICATION_NEEDED, - Resolution.ERROR, ], ) -def test_user_triggered_always_posts(resolution): - """A maintainer-triggered run always gets a comment, regardless of resolution.""" +def test_user_triggered_run_posts_normal_results(resolution): + """A maintainer-triggered run gets a normal result comment.""" assert _should_update_jira(resolution=resolution, user_triggered=True) is True @@ -118,9 +117,10 @@ def test_non_user_triggered_still_posts_when_no_mr_will_open(resolution): assert _should_update_jira(resolution=resolution, user_triggered=False) is True -def test_non_user_triggered_error_does_not_post(): - """ERROR is handled by separate error-path machinery, not this helper.""" +def test_error_deferred_to_terminal_retry_path(): + """ERROR is dispatched to retry() and commented once after retries are exhausted.""" assert _should_update_jira(resolution=Resolution.ERROR, user_triggered=False) is False + assert _should_update_jira(resolution=Resolution.ERROR, user_triggered=True) is False def _make_payload(issue: str = "RHEL-99999", user_triggered: bool = False) -> bytes: diff --git a/ymir/agents/triage_agent.py b/ymir/agents/triage_agent.py index 6ed999125..e96973534 100644 --- a/ymir/agents/triage_agent.py +++ b/ymir/agents/triage_agent.py @@ -105,7 +105,11 @@ def _should_update_jira(resolution: Resolution = None, user_triggered: bool = Fa The unbidden cases are the resolutions that do NOT produce an MR — without a comment the result would be invisible to the requester: not-affected, postponed, open-ended-analysis, clarification-needed. + ERROR resolutions are dispatched to retry() and commented once via + post_terminal_error_comment() after retries are exhausted. """ + if resolution == Resolution.ERROR: + return False if user_triggered: return True return resolution in ( @@ -1511,7 +1515,13 @@ async def _process_triage_locked(task, input): ) return - async def retry(task, error: ErrorData, input=input, user_triggered=user_triggered): + async def retry( + task, + error: ErrorData, + comment_text=None, + input=input, + user_triggered=user_triggered, + ): task.attempts += 1 # Preserve priority on retries: ymir_todo tasks go back to # the priority queue, normal tasks to the standard one. @@ -1543,6 +1553,24 @@ async def retry(task, error: ErrorData, input=input, user_triggered=user_trigger ) except Exception as label_error: logger.warning(f"Failed to set error labels on {input.issue}: {label_error}") + if comment_text and not dry_run: + try: + async with mcp_tools( + os.environ["MCP_GATEWAY_URL"], + call_meta={"jira_issue": input.issue}, + ) as gateway_tools: + await tasks.post_terminal_error_comment( + jira_issue=input.issue, + agent_type="Triage", + comment_text=comment_text, + available_tools=gateway_tools, + ) + except Exception as comment_error: + logger.warning( + "Failed to post final triage failure comment for %s: %s", + input.issue, + comment_error, + ) error_id = await fix_await(redis.incr(RedisQueues.ERROR_ID_COUNTER.value)) entry = ErrorListEntry(error_id=error_id, queue=retry_queue, task=task, error=error) await fix_await(redis.lpush(RedisQueues.ERROR_LIST.value, entry.model_dump_json())) @@ -1629,9 +1657,11 @@ async def retry(task, error: ErrorData, input=input, user_triggered=user_trigger except Exception as e: error = "".join(traceback.format_exception(e)) logger.error(f"Exception during triage processing for {input.issue}: {error}") + reason = e.explain() if isinstance(e, FrameworkError) else e await retry( task, ErrorData(details=error, jira_issue=input.issue), + comment_text=f"Agent failed to perform triage: {reason}", ) else: logger.info(f"Triage resolved as {output.resolution.value} for {input.issue}") @@ -1798,7 +1828,11 @@ async def retry(task, error: ErrorData, input=input, user_triggered=user_trigger jira_issue=input.issue, ) ) - await retry(task, error_data) + await retry( + task, + error_data, + comment_text=f"Agent failed to perform triage: {error_data.details}", + ) elif output.resolution in POSTPONED_RESOLUTIONS: await fix_await( redis.lpush( diff --git a/ymir/common/models.py b/ymir/common/models.py index b581db3a3..3bf76ab4b 100644 --- a/ymir/common/models.py +++ b/ymir/common/models.py @@ -104,12 +104,16 @@ class Task(BaseModel): default_factory=uuid4, description="Stable identifier for this queued task across retries and requeues", ) - attempts: int = Field(default=0, description="Number of processing attempts") + attempts: int = Field( + default=0, + description="Number of completed failed attempts before the next processing run", + ) user_triggered: bool = Field( default=False, description="True when a maintainer triggered this run via the ymir_todo label — " - "causes agents to post comments and intermediate failure labels that are " - "otherwise suppressed (default is silent).", + "causes agents to post acknowledgement/result comments and intermediate " + "failure labels that are otherwise suppressed. Error comments are " + "posted regardless of this flag.", ) def to_json(self) -> str: