From 0ab8cbb7b6f1e3fe31f5a6411c384780d22dbd99 Mon Sep 17 00:00:00 2001 From: Tomas Korbar Date: Wed, 7 Oct 2026 14:55:40 +0200 Subject: [PATCH 1/5] Add Konflux build backend as a configurable Copr alternative Introduce BUILD_BACKEND (copr default | konflux) to swap the build-validation step from Copr to Konflux without touching the agent workflows. The gateway registers backend-specific tools under the same names (build_package, download_artifacts), so the agents stay backend-agnostic. Konflux builds from a pushed git ref rather than a local SRPM, so the backport workflow commits + pushes the fork branch before building and opens the MR only after a green build (konflux_build_and_publish / konflux_inherit_build). The fork is created even under DRY_RUN (only MR/Jira writes stay suppressed); Copr is unchanged. The Konflux tool submits a PipelineRun against one fixed, generic Component (ymir-scratch-build in ymir-tenant) whose build-service-provisioned ServiceAccount carries appstudio-pipelines-scc and whose image repo receives the scratch build; git-url + revision are overridden per run, so a single Component backs every package with no cross-tenant RBAC. Target defaults are baked in and overridable via KONFLUX_NAMESPACE / KONFLUX_SERVICE_ACCOUNT / KONFLUX_IMAGE_REPO. Builds are x86_64-only. It omits the appstudio application/component labels so no Snapshot/Release is created (no brew/koji import) -- Ymir only needs the build to pass as validation. Failure logs come from authenticated Kubearchive pod-log URLs. Verified end-to-end: the backport-agent konflux e2e passed with a real PipelineRun reaching Succeeded under the fixed target. Co-Authored-By: Claude Opus 4.8 --- Makefile | 19 + compose.yaml | 17 +- ymir/agents/backport_agent.py | 209 +++++- ymir/agents/build_agent.py | 20 +- ymir/agents/tests/unit/test_build_agent.py | 65 ++ .../agents/tests/unit/test_konflux_reorder.py | 166 +++++ .../tests/unit/test_mr_consolidation_build.py | 10 + ymir/common/models.py | 9 +- ymir/tools/privileged/gateway.py | 30 +- ymir/tools/privileged/gitlab.py | 15 +- ymir/tools/privileged/konflux.py | 631 ++++++++++++++++++ .../privileged/tests/unit/test_gateway.py | 50 ++ .../privileged/tests/unit/test_gitlab.py | 88 +++ .../privileged/tests/unit/test_konflux.py | 406 +++++++++++ 14 files changed, 1693 insertions(+), 42 deletions(-) create mode 100644 ymir/agents/tests/unit/test_konflux_reorder.py create mode 100644 ymir/tools/privileged/konflux.py create mode 100644 ymir/tools/privileged/tests/unit/test_konflux.py diff --git a/Makefile b/Makefile index fd0099fd6..bbe35af9a 100644 --- a/Makefile +++ b/Makefile @@ -95,6 +95,25 @@ run-backport-agent-e2e-tests: -e BACKPORT_E2E_EXCLUDE_ISSUES="$(BACKPORT_E2E_EXCLUDE_ISSUES)" \ backport-agent-e2e-tests +.PHONY: run-backport-agent-konflux-e2e-tests +run-backport-agent-konflux-e2e-tests: + # Konflux build backend variant of the backport e2e. Unlike Copr (which + # builds from a local SRPM), Konflux builds from a real pushed fork ref: + # even under DRY_RUN the workflow creates the fork under FORK_NAMESPACE, + # pushes the backport branch (carrying the fixture's pre-fix history), and + # submits a real Konflux build; only the MR/Jira writes stay suppressed. + # The dist-git SOURCE stays mocked (local bare clones via insteadOf). + # Requires FORK_NAMESPACE + GITLAB_TOKEN + KONFLUX_* in the environment / + # .secrets/mcp-gateway.env and a reachable Konflux cluster. + MOCK_JIRA=true DRY_RUN=true BUILD_BACKEND=konflux FORK_NAMESPACE="$(FORK_NAMESPACE)" \ + $(COMPOSE) -f $(COMPOSE_FILE) --profile=e2e-test run --rm \ + -e MOCK_JIRA="true" \ + -e DRY_RUN="true" \ + -e BUILD_BACKEND="konflux" \ + -e RUN_LLM_JUDGE=$(RUN_LLM_JUDGE) \ + -e BACKPORT_E2E_EXCLUDE_ISSUES="$(BACKPORT_E2E_EXCLUDE_ISSUES)" \ + backport-agent-e2e-tests + .PHONY: run-reproducer-agent-e2e-tests run-reproducer-agent-e2e-tests: diff --git a/compose.yaml b/compose.yaml index f4b3d88b9..cddc7f3e9 100644 --- a/compose.yaml +++ b/compose.yaml @@ -11,6 +11,7 @@ x-beeai-env: &beeai-env MAX_CONCURRENT_TASKS: ${MAX_CONCURRENT_TASKS:-1} LOG_BUFFER_SIZE: ${LOG_BUFFER_SIZE:-0} DRY_RUN: ${DRY_RUN:-false} + BUILD_BACKEND: ${BUILD_BACKEND:-copr} JIRA_DRY_RUN: ${JIRA_DRY_RUN:-false} JIRA_ALLOW_STATUS_CHANGES: ${JIRA_ALLOW_STATUS_CHANGES:-false} ERRATA_ALLOW_STATUS_CHANGES: ${ERRATA_ALLOW_STATUS_CHANGES:-false} @@ -121,7 +122,21 @@ services: - TESTING_FARM_DRY_RUN=${TESTING_FARM_DRY_RUN:-false} - TESTING_FARM_COMPOSE_FILTER=${TESTING_FARM_COMPOSE_FILTER:-} - GIT_REPO_BASEPATH=/git-repos - - FORK_NAMESPACE=${FORK_NAMESPACE:-} + # FORK_NAMESPACE is sourced from .secrets/mcp-gateway.env (env_file). + # Do NOT re-declare it here: a compose `environment:` entry overrides + # env_file, so `${FORK_NAMESPACE:-}` would blank the secret whenever the + # host var is unset, breaking fork push auth (_is_private_gitlab). A host + # override is still possible via `podman-compose run -e FORK_NAMESPACE=...`. + # Selects the build tool set (copr default | konflux). Konflux also needs + # the KONFLUX_* vars (API/PIPELINE urls + token) and GITLAB_TOKEN, supplied + # via .secrets/mcp-gateway.env. Every build targets one fixed, generic + # Component in ymir-tenant (ymir-scratch-build): its build-service-provisioned + # ServiceAccount carries the appstudio-pipelines-scc and its image repo + # receives the scratch build, while git-url + revision are overridden per run. + # The target defaults (namespace/SA/image repo) are baked into konflux.py and + # need no env; override only for a different deployment via KONFLUX_NAMESPACE, + # KONFLUX_SERVICE_ACCOUNT, and KONFLUX_IMAGE_REPO. + - BUILD_BACKEND=${BUILD_BACKEND:-copr} - JIRA_MOCK_FILES=ymir/tools/privileged/tests/data # e2e tests write insteadOf rewrites here; ignored when the file is absent - GIT_CONFIG_GLOBAL=${GIT_REPO_BASEPATH:-/git-repos}/.mock_gitconfig diff --git a/ymir/agents/backport_agent.py b/ymir/agents/backport_agent.py index 54737ad39..6d3173f97 100644 --- a/ymir/agents/backport_agent.py +++ b/ymir/agents/backport_agent.py @@ -27,7 +27,7 @@ from specfile import Specfile import ymir.agents.tasks as tasks -from ymir.agents.build_agent import run_build +from ymir.agents.build_agent import is_konflux_backend, run_build from ymir.agents.constants import ( I_AM_YMIR, ZSTREAM_TARGET_LABEL, @@ -833,6 +833,9 @@ async def run_workflow( if max_incremental_fix_attempts is None: max_incremental_fix_attempts = max_build_attempts workspace_id = workspace_id or uuid4() + # Konflux builds from a pushed git ref, so it reorders commit/push to + # happen BEFORE the build; Copr (default) is unaffected. + konflux = is_konflux_backend() local_tool_options: dict[str, Any] = {"working_directory": None} if mock_env := get_mock_local_tool_env(jira_issue): @@ -1261,7 +1264,7 @@ async def generate_title(jira_summary): error=None, ) state.inherit_build_attempts = max_build_attempts - return "run_inherit_build_agent" + return "stage_changes" if konflux else "run_inherit_build_agent" except AlreadyInheritedError as error: logger.error("Y-stream inheritance invariant failed: %s", error) state.retry_mode = BackportRetryMode.NONE @@ -1337,7 +1340,7 @@ async def run_backport_agent(state): state.used_cherry_pick_workflow = False logger.info("Git am workflow detected: no upstream repo exists") - return "update_release" if is_modular_issue else "run_build_agent" + return "run_build_agent" if not (is_modular_issue or konflux) else "update_release" return "comment_in_jira" async def fix_build_error(state): @@ -1569,6 +1572,8 @@ async def stage_changes(state): if state.inherit_change: return "commit_inherited_change" if state.log_result: + if konflux and not is_modular_issue: + return "konflux_build_and_publish" return "commit_push_and_open_mr" return "run_log_agent" @@ -1694,7 +1699,9 @@ async def commit_inherited_change(state): return handle_inherit_cleanup_failure(state) _disable_ystream_inheritance(state, task_metadata) return "prepare_normal_backport" - if dry_run: + # Konflux must push the inherited commit so it can build from the ref, + # even in dry-run (the MR itself is still skipped later). + if dry_run and not konflux: return "submit_consolidation_job" return "push_inherited_change" @@ -1734,7 +1741,7 @@ async def push_inherited_change(state): f"{reconcile_error}" ) return "comment_in_jira" - return "open_inherited_mr" + return "konflux_inherit_build" if konflux else "open_inherited_mr" async def open_inherited_mr(state): try: @@ -1767,33 +1774,43 @@ async def open_inherited_mr(state): ) return "submit_consolidation_job" + async def _compose_backport_commit_and_mr(state): + """Build the (commit_message, mr_description, labels) for a normal backport.""" + formatted_patches = "\n".join(f" - {p}" for p in state.upstream_patches) + triage_details_text = format_mr_triage_details(state.justification, state.triage_summary) + branch_note = format_zstream_branch_note( + state.zstream_branch_created, state.zstream_branch_warning + ) + commit_message = ( + f"{state.log_result.title}\n\n" + f"{state.log_result.description}\n\n" + + (f"CVE: {state.cve_id}\n" if state.cve_id else "") + + "Upstream patches:\n" + + formatted_patches + + "\n" + + f"Resolves: {state.jira_issue}\n\n" + f"This commit was backported {I_AM_YMIR}\n\n" + "Assisted-by: Ymir\n" + ) + mr_description = ( + f"{state.log_result.description}\n\n" + f"Upstream patches:\n{formatted_patches}\n\n" + f"{triage_details_text}" + f"{format_jira_links_for_mr(state.jira_issue)}\n" + f"{wrap_details('Backporting steps', state.backport_log[-1])}" + f"\n\n{branch_note}" + f"{mr_description_footer(state.package)}" + ) + labels = ["ymir_backport"] + ( + [ZSTREAM_TARGET_LABEL] + if await tasks.needs_zstream_target_label(state.dist_git_branch, state.fix_version) + else [] + ) + return commit_message, mr_description, labels + async def commit_push_and_open_mr(state): try: - formatted_patches = "\n".join(f" - {p}" for p in state.upstream_patches) - triage_details_text = format_mr_triage_details(state.justification, state.triage_summary) - branch_note = format_zstream_branch_note( - state.zstream_branch_created, state.zstream_branch_warning - ) - commit_message = ( - f"{state.log_result.title}\n\n" - f"{state.log_result.description}\n\n" - + (f"CVE: {state.cve_id}\n" if state.cve_id else "") - + "Upstream patches:\n" - + formatted_patches - + "\n" - + f"Resolves: {state.jira_issue}\n\n" - f"This commit was backported {I_AM_YMIR}\n\n" - "Assisted-by: Ymir\n" - ) - mr_description = ( - f"{state.log_result.description}\n\n" - f"Upstream patches:\n{formatted_patches}\n\n" - f"{triage_details_text}" - f"{format_jira_links_for_mr(state.jira_issue)}\n" - f"{wrap_details('Backporting steps', state.backport_log[-1])}" - f"\n\n{branch_note}" - f"{mr_description_footer(state.package)}" - ) + commit_message, mr_description, labels = await _compose_backport_commit_and_mr(state) ( state.merge_request_url, state.merge_request_newly_created, @@ -1807,12 +1824,7 @@ async def commit_push_and_open_mr(state): mr_description=mr_description, available_tools=gateway_tools, commit_only=dry_run, - labels=["ymir_backport"] - + ( - [ZSTREAM_TARGET_LABEL] - if await tasks.needs_zstream_target_label(state.dist_git_branch, state.fix_version) - else [] - ), + labels=labels, package=state.package, ) except Exception as e: @@ -1824,6 +1836,129 @@ async def commit_push_and_open_mr(state): return "comment_in_jira" return "submit_consolidation_job" + async def konflux_build_and_publish(state): + """Konflux builds from a pushed ref: commit + push, build, then open the MR. + + Only reached for non-modular issues under BUILD_BACKEND=konflux. Mirrors + run_build_agent's failure/retry handling, but the fork push happens before + the build and the MR is opened only after a green build. + """ + try: + commit_message, mr_description, labels = await _compose_backport_commit_and_mr(state) + revision = await tasks.commit_changes(state.local_clone, commit_message) + await tasks.push_changes( + state.local_clone, state.fork_url, state.update_branch, gateway_tools + ) + except Exception as e: + logger.warning(f"Error committing/pushing before Konflux build: {e}") + state.merge_request_url = None + state.backport_result.success = False + state.backport_result.error = f"Could not commit and push for build: {e}" + return "comment_in_jira" + + build_result = await run_build( + build_input=BuildInputSchema( + srpm_path=state.backport_result.srpm_path, + dist_git_branch=state.dist_git_branch, + jira_issue=state.jira_issue, + git_url=state.fork_url, + revision=revision, + package_name=state.package, + target_branch=state.dist_git_branch, + ), + available_tools=gateway_tools, + local_tool_options=local_tool_options, + ) + if build_result.success or build_result.is_timeout: + if build_result.is_timeout: + logger.info(f"Konflux build timed out for {state.jira_issue}, proceeding") + state.incremental_fix_attempts = 0 + if dry_run: + # The ref was pushed so Konflux could build; skip MR creation in + # dry-run, mirroring the Copr commit_only path. + state.merge_request_url = None + state.merge_request_newly_created = False + return "submit_consolidation_job" + try: + ( + state.merge_request_url, + state.merge_request_newly_created, + ) = await tasks.open_update_merge_request( + fork_url=state.fork_url, + dist_git_branch=state.dist_git_branch, + update_branch=state.update_branch, + mr_title=state.log_result.title, + mr_description=mr_description, + available_tools=gateway_tools, + labels=labels, + package=state.package, + ) + except Exception as e: + logger.warning(f"Konflux build passed but MR creation failed: {e}") + state.merge_request_url = None + state.backport_result.success = False + state.backport_result.error = f"Could not open MR after build: {e}" + return "submit_consolidation_job" + if build_result.is_infra_error: + logger.error(f"Konflux infrastructure error for {state.jira_issue}: {build_result.error}") + state.backport_result.success = False + state.backport_result.error = build_result.error or "Konflux infrastructure error" + return "comment_in_jira" + state.attempts_remaining -= 1 + if state.attempts_remaining <= 0: + state.backport_result.success = False + state.backport_result.error = ( + f"Unable to successfully build the package in {max_build_attempts} attempts" + ) + return "comment_in_jira" + state.build_error = build_result.error + if state.used_cherry_pick_workflow: + upstream_repo = Path(f"{state.local_clone}-upstream") + if upstream_repo.exists(): + _move_build_logs( + state.local_clone, + _get_build_logs_dir(state.local_clone) / "attempt-0", + ) + logger.info("Cherry-pick workflow was used - starting incremental fix") + return "fix_build_error" + logger.info("Git am workflow was used - resetting for retry") + return "fork_and_prepare_dist_git" + + async def konflux_inherit_build(state): + """Validate an already-pushed inherited commit via Konflux before the MR.""" + build_result = await run_build( + build_input=BuildInputSchema( + srpm_path=state.backport_result.srpm_path, + dist_git_branch=state.dist_git_branch, + jira_issue=state.jira_issue, + git_url=state.fork_url, + revision=state.inherit_local_commit, + package_name=state.package, + target_branch=state.dist_git_branch, + ), + available_tools=gateway_tools, + local_tool_options=local_tool_options, + ) + if build_result.success or build_result.is_timeout: + if dry_run: + return "submit_consolidation_job" + return "open_inherited_mr" + + state.inherit_build_attempts -= 1 + if state.inherit_build_attempts > 0: + logger.warning( + "Inherited Konflux validation failed; retrying (%d attempts left): %s", + state.inherit_build_attempts, + build_result.error, + ) + return "konflux_inherit_build" + + logger.info("Inherited Konflux validation did not pass: %s", build_result.error) + if not await cleanup_inherit_attempt(state): + return handle_inherit_cleanup_failure(state) + _disable_ystream_inheritance(state, task_metadata) + return "prepare_normal_backport" + async def submit_consolidation_job(state): if ( not state.merge_request_url @@ -1935,6 +2070,8 @@ async def comment_in_jira(state): workflow.add_step("push_inherited_change", push_inherited_change) workflow.add_step("open_inherited_mr", open_inherited_mr) workflow.add_step("commit_push_and_open_mr", commit_push_and_open_mr) + workflow.add_step("konflux_build_and_publish", konflux_build_and_publish) + workflow.add_step("konflux_inherit_build", konflux_inherit_build) workflow.add_step("submit_consolidation_job", submit_consolidation_job) workflow.add_step("comment_in_jira", comment_in_jira) diff --git a/ymir/agents/build_agent.py b/ymir/agents/build_agent.py index 6fce8faec..6e44b8144 100644 --- a/ymir/agents/build_agent.py +++ b/ymir/agents/build_agent.py @@ -1,5 +1,6 @@ import asyncio import logging +import os from typing import Any from urllib.parse import urlsplit @@ -39,6 +40,16 @@ logger = logging.getLogger(__name__) +def build_backend() -> str: + """Return the configured build backend ("copr" default, or "konflux").""" + return os.getenv("BUILD_BACKEND", "copr").strip().lower() + + +def is_konflux_backend() -> bool: + """True when builds are driven by Konflux (build-from-git-ref) rather than Copr.""" + return build_backend() == "konflux" + + class BuildState(BaseModel): build_input: BuildInputSchema build_result: BuildResult | None = None @@ -92,7 +103,14 @@ async def execute_build(state: BuildState) -> str: if result.is_timeout: return Workflow.END - if not any(urlsplit(url).path.endswith(".log.gz") for url in result.artifacts_urls or []): + # Copr advertises gzipped build logs; Konflux returns authenticated + # Kubearchive pod-log URLs (no .log.gz suffix). Diagnose whenever the + # backend handed us any log URLs to inspect. + if is_konflux_backend(): + has_logs = bool(result.artifacts_urls) + else: + has_logs = any(urlsplit(url).path.endswith(".log.gz") for url in result.artifacts_urls or []) + if not has_logs: return Workflow.END return "diagnose_failure" diff --git a/ymir/agents/tests/unit/test_build_agent.py b/ymir/agents/tests/unit/test_build_agent.py index d331ad782..bcf4f2155 100644 --- a/ymir/agents/tests/unit/test_build_agent.py +++ b/ymir/agents/tests/unit/test_build_agent.py @@ -49,6 +49,10 @@ async def _mock_submit(*_args, **_kwargs): srpm_path=str(build_input.srpm_path), dist_git_branch="c10s", jira_issue="RHEL-123", + git_url=None, + revision=None, + package_name=None, + target_branch=None, ).replace_with(_mock_submit).once() flexmock(build_agent).should_receive("create_build_failure_agent").never() # Successful builds must not even require model configuration. @@ -503,3 +507,64 @@ def _mock_factory(*_args, **_kwargs): assert "local sandbox" in kwargs["instructions"] assert "artifacts_urls" in kwargs["instructions"] assert "Do not submit or retry a build" in kwargs["instructions"] + + +def test_build_backend_helpers(monkeypatch): + """build_backend() normalizes BUILD_BACKEND; default is copr.""" + monkeypatch.delenv("BUILD_BACKEND", raising=False) + assert build_agent.build_backend() == "copr" + assert not build_agent.is_konflux_backend() + + monkeypatch.setenv("BUILD_BACKEND", "Konflux") + assert build_agent.build_backend() == "konflux" + assert build_agent.is_konflux_backend() + + monkeypatch.setenv("BUILD_BACKEND", " COPR ") + assert build_agent.build_backend() == "copr" + assert not build_agent.is_konflux_backend() + + +@pytest.mark.asyncio +async def test_konflux_log_urls_trigger_diagnosis(build_input, monkeypatch): + """Konflux Kubearchive pod-log URLs lack a .log.gz suffix but must be diagnosed.""" + monkeypatch.setenv("BUILD_BACKEND", "konflux") + kube_url = "https://kubearchive/api/v1/namespaces/ymir-tenant/pods/p/log?container=step-build" + + async def _mock_submit(*_args, **_kwargs): + return BuildResult(success=False, error_message="Build failed", artifacts_urls=[kube_url]) + + async def _mock_analyst(*_args, **_kwargs): + return SimpleNamespace(last_message=SimpleNamespace(text='{"error": "Konflux diagnosis"}')) + + def _mock_factory(*_args, **_kwargs): + return SimpleNamespace(run=_mock_analyst) + + _mock_tool = SimpleNamespace(name="build_package") + flexmock(build_agent).should_receive("run_tool").replace_with(_mock_submit).once() + flexmock(build_agent).should_receive("create_build_failure_agent").replace_with(_mock_factory).once() + monkeypatch.delenv("CHAT_MODEL", raising=False) + + result = await _run(build_input, [_mock_tool]) + + assert not result.success + assert result.error == "Konflux diagnosis" + + +@pytest.mark.asyncio +async def test_copr_ignores_non_loggz_urls(build_input, monkeypatch): + """Under the default Copr backend, only .log.gz URLs trigger diagnosis.""" + monkeypatch.delenv("BUILD_BACKEND", raising=False) + kube_url = "https://kubearchive/api/v1/namespaces/ymir-tenant/pods/p/log?container=step-build" + + async def _mock_submit(*_args, **_kwargs): + return BuildResult(success=False, error_message="Build failed", artifacts_urls=[kube_url]) + + _mock_tool = SimpleNamespace(name="build_package") + flexmock(build_agent).should_receive("run_tool").replace_with(_mock_submit).once() + flexmock(build_agent).should_receive("create_build_failure_agent").never() + monkeypatch.delenv("CHAT_MODEL", raising=False) + + result = await _run(build_input, [_mock_tool]) + + assert not result.success + assert result.error == "Build failed" diff --git a/ymir/agents/tests/unit/test_konflux_reorder.py b/ymir/agents/tests/unit/test_konflux_reorder.py new file mode 100644 index 000000000..71351362a --- /dev/null +++ b/ymir/agents/tests/unit/test_konflux_reorder.py @@ -0,0 +1,166 @@ +"""Konflux backend reorders commit/push to happen BEFORE the build. + +Copr (default) builds from a local SRPM and pushes only after a green build; +Konflux builds from a pushed git ref, so the backport workflow must commit and +push to the fork, build from that ref, and open the MR only afterwards. These +tests drive the real workflow routing (via ``set_start`` + handler overrides, +mirroring test_backport_build_logs.py) and assert the ordering. +""" + +from contextlib import asynccontextmanager + +import pytest +from beeai_framework.workflows import Workflow +from flexmock import flexmock + +from ymir.agents import backport_agent +from ymir.agents import tasks as agent_tasks +from ymir.common.models import BackportOutputSchema, BuildOutputSchema, LogOutputSchema + + +def _prepare_common_mocks(monkeypatch): + @asynccontextmanager + async def gateway(*args, **kwargs): + yield [] + + async def _no_zstream_label(*args, **kwargs): + return False + + flexmock(backport_agent).should_receive("mcp_tools").replace_with(gateway).once() + flexmock(backport_agent).should_receive("create_log_agent").and_return(None).once() + flexmock(backport_agent).should_receive("get_mock_local_tool_env").and_return(None).once() + flexmock(agent_tasks).should_receive("needs_zstream_target_label").replace_with(_no_zstream_label) + monkeypatch.setenv("MCP_GATEWAY_URL", "http://gateway.invalid/sse") + + +def _base_state(state, *, local_clone): + state.local_clone = local_clone + state.fork_url = "https://fork.example/repo.git" + state.update_branch = "ymir-RHEL-123" + state.used_cherry_pick_workflow = False + state.backport_log = ["Backported the fix"] + state.log_result = LogOutputSchema(title="Fix the thing", description="Backport of the fix") + state.backport_result = BackportOutputSchema( + success=True, + status="Backported", + srpm_path=local_clone / "expat.src.rpm", + error=None, + ) + + +@pytest.mark.parametrize("dry_run", [False, True]) +@pytest.mark.asyncio +async def test_konflux_commits_and_pushes_before_build(monkeypatch, tmp_path, dry_run): + monkeypatch.setenv("BUILD_BACKEND", "konflux") + local_clone = tmp_path / "expat" + local_clone.mkdir() + calls = [] + + async def _commit(_clone, _message, *a, **k): + calls.append("commit") + return "a" * 40 + + async def _push(_clone, _fork, _branch, _tools, *a, **k): + calls.append("push") + + async def _build(**kwargs): + calls.append("build") + bi = kwargs["build_input"] + assert bi.git_url == "https://fork.example/repo.git" + assert bi.revision == "a" * 40 + assert bi.package_name == "expat" + return BuildOutputSchema(success=True, error=None) + + async def _open_mr(**kwargs): + calls.append("open_mr") + return "https://mr.example/1", True + + flexmock(agent_tasks).should_receive("commit_changes").replace_with(_commit).once() + flexmock(agent_tasks).should_receive("push_changes").replace_with(_push).once() + flexmock(backport_agent).should_receive("run_build").replace_with(_build).once() + flexmock(agent_tasks).should_receive("open_update_merge_request").replace_with(_open_mr).times( + 0 if dry_run else 1 + ) + _prepare_common_mocks(monkeypatch) + + run_workflow = Workflow.run + + def start_at(workflow, state, options=None): + _base_state(state, local_clone=local_clone) + workflow.set_start("konflux_build_and_publish") + workflow.steps["submit_consolidation_job"].handler = lambda _: Workflow.END + workflow.steps["comment_in_jira"].handler = lambda _: Workflow.END + return run_workflow(workflow, state, options) + + monkeypatch.setattr(Workflow, "run", start_at) + state = await backport_agent.run_workflow( + package="expat", + dist_git_branch="c10s", + upstream_patches=["https://example/patch.patch"], + jira_issue="RHEL-123", + cve_id=None, + dry_run=dry_run, + backport_agent_factory=lambda *_: None, + ) + + assert state.backport_result.success + # Commit + push always precede the build (Konflux builds from the ref). + assert calls.index("commit") < calls.index("build") + assert calls.index("push") < calls.index("build") + if dry_run: + # Pushed so Konflux can build, but no MR is opened in dry-run. + assert "open_mr" not in calls + else: + # The MR opens only after a green build. + assert calls.index("build") < calls.index("open_mr") + + +@pytest.mark.parametrize("dry_run", [False, True]) +@pytest.mark.asyncio +async def test_konflux_inherit_build_validates_pushed_commit(monkeypatch, tmp_path, dry_run): + monkeypatch.setenv("BUILD_BACKEND", "konflux") + local_clone = tmp_path / "expat" + local_clone.mkdir() + calls = [] + + async def _build(**kwargs): + calls.append("build") + bi = kwargs["build_input"] + assert bi.git_url == "https://fork.example/repo.git" + assert bi.revision == "b" * 40 + return BuildOutputSchema(success=True, error=None) + + flexmock(backport_agent).should_receive("run_build").replace_with(_build).once() + _prepare_common_mocks(monkeypatch) + + run_workflow = Workflow.run + + def start_at(workflow, state, options=None): + _base_state(state, local_clone=local_clone) + state.inherit_local_commit = "b" * 40 + state.inherit_build_attempts = 3 + workflow.set_start("konflux_inherit_build") + workflow.steps["open_inherited_mr"].handler = lambda _: (calls.append("open_mr"), Workflow.END)[1] + workflow.steps["submit_consolidation_job"].handler = lambda _: ( + calls.append("submit"), + Workflow.END, + )[1] + workflow.steps["comment_in_jira"].handler = lambda _: Workflow.END + return run_workflow(workflow, state, options) + + monkeypatch.setattr(Workflow, "run", start_at) + await backport_agent.run_workflow( + package="expat", + dist_git_branch="c10s", + upstream_patches=["https://example/patch.patch"], + jira_issue="RHEL-123", + cve_id=None, + dry_run=dry_run, + backport_agent_factory=lambda *_: None, + ) + + assert "build" in calls + if dry_run: + assert "submit" in calls and "open_mr" not in calls + else: + assert "open_mr" in calls diff --git a/ymir/agents/tests/unit/test_mr_consolidation_build.py b/ymir/agents/tests/unit/test_mr_consolidation_build.py index 4bd5e1da1..720a1e896 100644 --- a/ymir/agents/tests/unit/test_mr_consolidation_build.py +++ b/ymir/agents/tests/unit/test_mr_consolidation_build.py @@ -85,6 +85,12 @@ async def _mock_call_tool(*_args, **_kwargs): "srpm_path": {"type": "string"}, "dist_git_branch": {"type": "string"}, "jira_issue": {"type": "string"}, + # Optional Konflux build-from-ref fields; Copr ignores them + # but BuildInputSchema.model_dump() always emits them. + "git_url": {"type": ["string", "null"]}, + "revision": {"type": ["string", "null"]}, + "package_name": {"type": ["string", "null"]}, + "target_branch": {"type": ["string", "null"]}, }, "required": ["srpm_path", "dist_git_branch", "jira_issue"], }, @@ -153,6 +159,10 @@ def start_at_build(workflow, state, options=None): "srpm_path": "/git-repos/package/package-1-1.src.rpm", "dist_git_branch": "c10s", "jira_issue": project, + "git_url": None, + "revision": None, + "package_name": None, + "target_branch": None, } assert state.jira_issue == jira_issue assert state.jira_issues_collected == ([jira_issue] if jira_issue else []) diff --git a/ymir/common/models.py b/ymir/common/models.py index 3bf76ab4b..7c118668f 100644 --- a/ymir/common/models.py +++ b/ymir/common/models.py @@ -757,11 +757,18 @@ class BuildInstructionsInput(BaseModel): class BuildInputSchema(BaseModel): - """Inputs for deterministic Copr build execution.""" + """Inputs for deterministic build execution (Copr or Konflux).""" srpm_path: Path = Field(description="Path to SRPM to build") dist_git_branch: str = Field(description="dist-git branch to update") jira_issue: str | None = Field(description="Jira issue to reference as resolved") + # Konflux-only fields (Copr ignores them). The Konflux backend builds from a + # pushed git ref rather than a local SRPM, so the caller supplies these once + # the fork branch has been pushed. + git_url: str | None = Field(default=None, description="Clonable fork URL holding the pushed commit") + revision: str | None = Field(default=None, description="Pushed commit SHA to build") + package_name: str | None = Field(default=None, description="RPM package name") + target_branch: str | None = Field(default=None, description="dist-git branch the build targets") class BuildResult(BaseModel): diff --git a/ymir/tools/privileged/gateway.py b/ymir/tools/privileged/gateway.py index 38103e75c..d9abaeafa 100644 --- a/ymir/tools/privileged/gateway.py +++ b/ymir/tools/privileged/gateway.py @@ -71,6 +71,10 @@ UpdateJiraCommentTool, VerifyIssueAuthorTool, ) +from ymir.tools.privileged.konflux import ( + KonfluxBuildTool, + KonfluxDownloadArtifactsTool, +) from ymir.tools.privileged.lookaside import ( DownloadSourcesTool, UploadSourcesTool, @@ -92,6 +96,28 @@ logger = logging.getLogger(__name__) +def _select_build_tools(tool_options: dict) -> list: + """Return the (build_package, download_artifacts) tools for the configured backend. + + Both backends register the same tool names so the agents stay backend-agnostic. + Selected by BUILD_BACKEND (default: copr). + """ + backend = os.getenv("BUILD_BACKEND", "copr").strip().lower() + if backend == "konflux": + logger.info("Build backend: konflux") + return [ + KonfluxBuildTool(options=tool_options), + KonfluxDownloadArtifactsTool(options=tool_options), + ] + if backend not in ("copr", ""): + logger.warning("Unknown BUILD_BACKEND=%r, falling back to copr", backend) + logger.info("Build backend: copr") + return [ + BuildPackageTool(options=tool_options), + DownloadArtifactsTool(options=tool_options), + ] + + async def _async_main(): transport = os.getenv("MCP_TRANSPORT", "sse") config_kwargs = {"name": "Ymir Privileged MCP Gateway", "transport": transport} @@ -116,10 +142,10 @@ async def _async_main(): else: logger.info("Gateway starting without LogDetective MCP tools.") + build_tools = _select_build_tools(tool_options) mcp.register_many( [ - BuildPackageTool(options=tool_options), - DownloadArtifactsTool(options=tool_options), + *build_tools, CreateZstreamBranchTool(options=tool_options), AddBlockingMergeRequestCommentTool(options=tool_options), AddMergeRequestCommentTool(options=tool_options), diff --git a/ymir/tools/privileged/gitlab.py b/ymir/tools/privileged/gitlab.py index a3663e646..0352d9d1d 100644 --- a/ymir/tools/privileged/gitlab.py +++ b/ymir/tools/privileged/gitlab.py @@ -107,6 +107,16 @@ async def _run_git_cmd( _FORK_READY_TIMEOUT_SEC = 110 # leave margin under fork_repository tool timeout +def _is_konflux_backend() -> bool: + """True when builds run on Konflux (build-from-git-ref) rather than Copr. + + Konflux builds from a pushed fork ref, so the fork must exist and be pushed + to even under ``DRY_RUN`` — only the MR/Jira writes are suppressed. Copr, by + contrast, builds from a local SRPM and never needs a dry-run fork. + """ + return os.getenv("BUILD_BACKEND", "copr").strip().lower() == "konflux" + + def _fork_api_project(fork: GitlabProject): """Return a python-gitlab Project object suitable for import_status polling. @@ -427,7 +437,10 @@ def get_fork(): if fork := await asyncio.to_thread(get_fork): return StringToolOutput(result=fork.get_git_urls()["git"]) - if os.getenv("DRY_RUN", "False").lower() == "true": + # Konflux must build from a real, pushed fork ref, so the fork is + # created even in DRY_RUN (only MR/Jira writes are suppressed). Copr + # builds from a local SRPM and needs no dry-run fork. + if os.getenv("DRY_RUN", "False").lower() == "true" and not _is_konflux_backend(): logger.info("DRY_RUN is set, skipping fork creation — returning original repo URL") return StringToolOutput(result=project.get_git_urls()["git"]) diff --git a/ymir/tools/privileged/konflux.py b/ymir/tools/privileged/konflux.py new file mode 100644 index 000000000..6b825cf10 --- /dev/null +++ b/ymir/tools/privileged/konflux.py @@ -0,0 +1,631 @@ +"""Konflux (Tekton) build backend — a COPR-alternative build-validation tool. + +Submits a component-backed RPM build PipelineRun to the Konflux API, polls it to +completion, and reports a :class:`BuildResult` with the same shape the COPR tool +returns, so :func:`ymir.agents.build_agent.run_build` stays backend-agnostic +(it dispatches purely by tool name). Selected via ``BUILD_BACKEND=konflux`` in +the gateway. + +Unlike COPR (which builds a local SRPM), Konflux builds from a git ref, so the +caller must have committed and pushed the fork branch BEFORE invoking this tool +(see konflux-build-support-plan.md §1). +""" + +import asyncio +import gzip +import logging +import os +import re +import tempfile +import time +import uuid +from pathlib import Path +from shutil import rmtree +from urllib.parse import parse_qs, urlsplit + +import requests +from beeai_framework.context import RunContext +from beeai_framework.emitter import Emitter +from beeai_framework.tools import JSONToolOutput, ToolError, ToolRunOptions +from pydantic import BaseModel, ConfigDict, Field +from requests.adapters import HTTPAdapter, Retry + +from ymir.common.models import BuildResult +from ymir.tools.base import CloneableTool as Tool +from ymir.tools.base import make_additional_context, tool_error_context +from ymir.tools.constants import YMIR_USER_AGENT +from ymir.tools.errors import ToolErrorWithContext + +logger = logging.getLogger(__name__) + +# Build/poll budget (mirrors COPR's values so workflow timeouts line up). +KONFLUX_BUILD_TIMEOUT = 3 * 60 * 60 # seconds +KONFLUX_TIMEOUT_GRACE_PERIOD = 60 # seconds +KONFLUX_POLLING_INTERVAL = 10 # seconds + +# (connect, read) timeout for every HTTP call; without it a silent connection +# would hang forever (Retry only engages once a response/conn error arrives). +DEFAULT_TIMEOUT = (20, 60) + +# Tekton PipelineRun status reasons. +# https://github.com/tektoncd/pipeline/blob/main/pkg/apis/pipeline/v1/pipelinerun_types.go +RUNNING_STATES = frozenset( + { + "Started", + "Running", + "PipelineRunPending", + "PipelineRunStopping", + "ResolvingPipelineRef", + "ResolvingTaskRef", + "CancelledRunningFinally", + "StoppedRunningFinally", + } +) +SUCCESSFUL_STATES = frozenset({"Succeeded", "Completed"}) + +# Single-arch scratch validation (mirrors COPR building one arch for speed). +DEFAULT_BUILD_ARCHITECTURES = ["x86_64"] +DEFAULT_BUILD_PLATFORMS = ["linux-mxlarge/amd64"] + +# Pipeline definition to resolve the build from. +PIPELINE_PATH_IN_REPO = "pipeline/build-rpm-package.yaml" +OCI_ARTIFACT_EXPIRES_AFTER = "1d" + +# Fixed build target (see konflux-release-data ymir-tenant/builds): one generic +# Component whose build-service-provisioned ServiceAccount carries the +# appstudio-pipelines-scc the pipeline pods require, and whose image repo +# receives the scratch build. git-url + revision are set per run, so this single +# Component backs every package. Overridable via env for other deployments. +DEFAULT_NAMESPACE = "ymir-tenant" +DEFAULT_SERVICE_ACCOUNT = "build-pipeline-ymir-scratch-build" +DEFAULT_IMAGE_REPO = "quay.io/redhat-user-workloads/ymir-tenant/ymir-scratch-build" + +# Quay image tags allow [A-Za-z0-9_.-]; anything else is replaced. +_INVALID_TAG_CHARS = re.compile(r"[^A-Za-z0-9_.-]") + + +class KonfluxConfig(BaseModel): + """Runtime configuration read from the environment at call time. + + Read fresh per build so a rotated token (mounted file) is always current. + """ + + api_url: str + kubearchive_url: str | None + pipeline_url: str + token: str + gitlab_token: str + namespace: str + service_account: str + container_image: str + + +def _load_config() -> KonfluxConfig: + """Assemble :class:`KonfluxConfig` from env, raising on missing essentials.""" + missing = [] + + def required(name: str) -> str: + value = os.environ.get(name, "").strip() + if not value: + missing.append(name) + return value + + api_url = required("KONFLUX_API_URL").rstrip("/") + pipeline_url = required("KONFLUX_PIPELINE_URL") + gitlab_token = required("GITLAB_TOKEN") + + # The SA token may be supplied either as a file (KONFLUX_TOKEN_FILE, read + # fresh per build so a rotated mounted file is always current) or inline via + # KONFLUX_TOKEN. The file takes precedence when both are set. + token_file = os.environ.get("KONFLUX_TOKEN_FILE", "").strip() + inline_token = os.environ.get("KONFLUX_TOKEN", "").strip() + token = "" + if token_file: + try: + token = Path(token_file).read_text().strip() + except OSError as e: + raise ToolErrorWithContext( + "Failed to read Konflux SA token file", + cause=e, + additional_context=make_additional_context(token_file=token_file), + ) from e + if not token: + raise ToolErrorWithContext( + "Konflux SA token file is empty", + additional_context=make_additional_context(token_file=token_file), + ) + elif inline_token: + token = inline_token + else: + missing.append("KONFLUX_TOKEN_FILE or KONFLUX_TOKEN") + + if missing: + raise ToolErrorWithContext( + "Konflux backend is not fully configured", + additional_context=make_additional_context(missing=", ".join(sorted(missing))), + ) + + kubearchive_url = os.environ.get("KUBEARCHIVE_API_URL", "").strip().rstrip("/") or None + namespace = os.environ.get("KONFLUX_NAMESPACE", "").strip() or DEFAULT_NAMESPACE + service_account = os.environ.get("KONFLUX_SERVICE_ACCOUNT", "").strip() or DEFAULT_SERVICE_ACCOUNT + container_image = os.environ.get("KONFLUX_IMAGE_REPO", "").strip().rstrip("/") or DEFAULT_IMAGE_REPO + return KonfluxConfig( + api_url=api_url, + kubearchive_url=kubearchive_url, + pipeline_url=pipeline_url, + token=token, + gitlab_token=gitlab_token, + namespace=namespace, + service_account=service_account, + container_image=container_image, + ) + + +def _build_session() -> requests.Session: + """A requests Session with retries on transient/rate-limit responses.""" + session = requests.Session() + retries = Retry( + total=None, + connect=10, + read=10, + other=10, + status=10, + status_forcelist=[429, 500, 502, 503, 504], + backoff_factor=1, + respect_retry_after_header=True, + raise_on_status=False, + ) + session.mount("https://", HTTPAdapter(max_retries=retries)) + session.mount("http://", HTTPAdapter(max_retries=retries)) + return session + + +def _sanitize_tag(value: str) -> str: + return _INVALID_TAG_CHARS.sub("-", value) + + +def _pipelinerun_status(data: dict | None) -> str: + """Tekton status reason, or ``Unknown`` when not yet reported.""" + if not data: + return "Unknown" + try: + return data["status"]["conditions"][0]["reason"] + except (KeyError, IndexError, TypeError): + return "Unknown" + + +def _pipelinerun_message(data: dict | None) -> str: + if not data: + return "" + try: + return data["status"]["conditions"][0].get("message", "") + except (KeyError, IndexError, TypeError): + return "" + + +class KonfluxClient: + """Thin requests wrapper around the Konflux/Tekton + Kubearchive APIs.""" + + def __init__(self, config: KonfluxConfig, namespace: str) -> None: + self._config = config + self._namespace = namespace + self._session = _build_session() + self._headers = { + "Authorization": f"Bearer {config.token}", + "User-Agent": YMIR_USER_AGENT, + } + + @property + def namespace(self) -> str: + return self._namespace + + def _tekton_base(self, api_url: str) -> str: + return f"{api_url}/apis/tekton.dev/v1/namespaces/{self.namespace}" + + def create_secret(self, body: dict) -> str: + url = f"{self._config.api_url}/api/v1/namespaces/{self.namespace}/secrets" + response = self._session.post(url, headers=self._headers, json=body, timeout=DEFAULT_TIMEOUT) + response.raise_for_status() + return response.json()["metadata"]["name"] + + def delete_secret(self, name: str) -> None: + url = f"{self._config.api_url}/api/v1/namespaces/{self.namespace}/secrets/{name}" + response = self._session.delete(url, headers=self._headers, timeout=DEFAULT_TIMEOUT) + if response.status_code not in (200, 202, 404): + response.raise_for_status() + + def create_pipelinerun(self, body: dict) -> str: + url = f"{self._tekton_base(self._config.api_url)}/pipelineruns" + response = self._session.post(url, headers=self._headers, json=body, timeout=DEFAULT_TIMEOUT) + response.raise_for_status() + return response.json()["metadata"]["name"] + + def delete_pipelinerun(self, name: str) -> None: + url = f"{self._tekton_base(self._config.api_url)}/pipelineruns/{name}" + response = self._session.delete(url, headers=self._headers, timeout=DEFAULT_TIMEOUT) + if response.status_code not in (200, 202, 404): + response.raise_for_status() + + def get_pipelinerun(self, name: str) -> dict | None: + """Fetch a PipelineRun, falling back to Kubearchive once GC'd (404).""" + url = f"{self._tekton_base(self._config.api_url)}/pipelineruns/{name}" + response = self._session.get(url, headers=self._headers, timeout=DEFAULT_TIMEOUT) + if response.status_code == 404 and self._config.kubearchive_url: + url = f"{self._tekton_base(self._config.kubearchive_url)}/pipelineruns/{name}" + response = self._session.get(url, headers=self._headers, timeout=DEFAULT_TIMEOUT) + if response.status_code != 200: + return None + return response.json() + + def get_taskrun(self, name: str) -> dict | None: + url = f"{self._tekton_base(self._config.api_url)}/taskruns/{name}" + response = self._session.get(url, headers=self._headers, timeout=DEFAULT_TIMEOUT) + if response.status_code == 404 and self._config.kubearchive_url: + url = f"{self._tekton_base(self._config.kubearchive_url)}/taskruns/{name}" + response = self._session.get(url, headers=self._headers, timeout=DEFAULT_TIMEOUT) + if response.status_code != 200: + return None + return response.json() + + +def _git_auth_secret_body(git_url: str, gitlab_token: str) -> dict: + """basic-auth SCM secret the oci-ta clone task consumes (osci rhel-secret.j2).""" + parts = urlsplit(git_url) + host = parts.hostname or "" + repo_path = parts.path.lstrip("/") + if repo_path.endswith(".git"): + repo_path = repo_path[: -len(".git")] + repository_url = f"https://{host}/{repo_path}" + return { + "apiVersion": "v1", + "kind": "Secret", + "metadata": { + "generateName": "gitlab-secret-", + "labels": { + "appstudio.redhat.com/credentials": "scm", + "appstudio.redhat.com/scm.host": host, + }, + "annotations": { + "appstudio.redhat.com/scm.repository": repository_url, + }, + }, + "type": "kubernetes.io/basic-auth", + "stringData": { + "username": "gitlab-ci-token", + "password": gitlab_token, + }, + } + + +def _pipelinerun_body( + *, + config: KonfluxConfig, + namespace: str, + service_account: str, + run_name: str, + package_name: str, + git_url: str, + revision: str, + target_branch: str, + specfile: str | None, + ocistorage: str, + pipeline_revision: str, + secret_name: str, +) -> dict: + """Build PipelineRun for Ymir's scratch-build Component (osci build_pipelinerun.j2, trimmed). + + Runs under the ``build-pipeline-ymir-scratch-build`` ServiceAccount in + ymir-tenant -- that SA is provisioned by Konflux build-service with the + ``appstudio-pipelines-scc`` the pipeline pods require (a bare bot SA is not, + which is why a standalone run fails at pod admission). ociStorage is that + Component's own image repo (scratch tag, short TTL). git-url + revision come + from the caller, so one Component backs every package. + + We deliberately OMIT the ``appstudio.openshift.io/application``/``component`` + labels so integration-service does not create a Snapshot/Release and trigger + the brew/koji import -- Ymir only needs the build itself to go green. + koji-target is DEFAULT because the pipeline derives the real target from the + branch (see plan §1 step 4). + """ + return { + "apiVersion": "tekton.dev/v1", + "kind": "PipelineRun", + "metadata": { + "name": run_name, + "namespace": namespace, + "annotations": { + "build.appstudio.openshift.io/repo": git_url, + "build.appstudio.redhat.com/commit_sha": revision, + "build.appstudio.redhat.com/target_branch": target_branch, + "pipelinesascode.tekton.dev/max-keep-runs": "3", + }, + "labels": { + "pipelines.appstudio.openshift.io/type": "build", + }, + }, + "spec": { + "params": [ + {"name": "package-name", "value": package_name}, + {"name": "git-url", "value": git_url}, + {"name": "ociStorage", "value": ocistorage}, + {"name": "revision", "value": revision}, + {"name": "target-branch", "value": target_branch}, + {"name": "SINGLE_COMPONENT", "value": "true"}, + {"name": "hermetic", "value": "true"}, + {"name": "koji-target", "value": "DEFAULT"}, + {"name": "specfile", "value": specfile or "null"}, + {"name": "build-platforms", "value": DEFAULT_BUILD_PLATFORMS}, + {"name": "build-architectures", "value": DEFAULT_BUILD_ARCHITECTURES}, + {"name": "self-ref-url", "value": config.pipeline_url}, + {"name": "self-ref-revision", "value": pipeline_revision}, + {"name": "ociArtifactExpiresAfter", "value": OCI_ARTIFACT_EXPIRES_AFTER}, + ], + "pipelineRef": { + "resolver": "git", + "params": [ + {"name": "url", "value": config.pipeline_url}, + {"name": "revision", "value": pipeline_revision}, + {"name": "pathInRepo", "value": PIPELINE_PATH_IN_REPO}, + ], + }, + "timeouts": {"pipeline": "0", "tasks": "0", "finally": "0"}, + "taskRunTemplate": {"serviceAccountName": service_account}, + "workspaces": [ + {"name": "git-auth", "secret": {"secretName": secret_name}}, + ], + }, + } + + +async def _resolve_pipeline_revision(pipeline_url: str) -> str: + """Pin the pipeline definition to an immutable SHA via ``git ls-remote``.""" + proc = await asyncio.create_subprocess_exec( + "git", + "ls-remote", + pipeline_url, + "refs/heads/main", + stdout=asyncio.subprocess.PIPE, + stderr=asyncio.subprocess.PIPE, + ) + stdout, stderr = await proc.communicate() + if proc.returncode != 0 or not stdout.strip(): + raise ToolErrorWithContext( + "Failed to resolve the pipeline revision", + additional_context=make_additional_context( + pipeline_url=pipeline_url, + stderr=stderr.decode(errors="replace"), + ), + ) + return stdout.split()[0].decode() + + +def _collect_failure_log_urls(client: KonfluxClient, pipelinerun: dict, config: KonfluxConfig) -> list[str]: + """Authenticated Kubearchive pod-log URLs for non-successful taskruns.""" + if not config.kubearchive_url: + return [] + urls: list[str] = [] + child_refs = (pipelinerun.get("status") or {}).get("childReferences", []) or [] + for child in child_refs: + name = child.get("name") + if not name: + continue + taskrun = client.get_taskrun(name) + if not taskrun: + continue + status = _pipelinerun_status(taskrun) + if status in SUCCESSFUL_STATES: + continue + task_status = taskrun.get("status") or {} + pod_name = task_status.get("podName") + if not pod_name: + continue + steps = [s["container"] for s in task_status.get("steps", []) if "container" in s] + urls.extend( + f"{config.kubearchive_url}/api/v1/namespaces/{client.namespace}" + f"/pods/{pod_name}/log?container={container}" + for container in steps + ) + return urls + + +class BuildPackageToolInput(BaseModel): + """Konflux build inputs. + + ``extra='allow'`` so the COPR-only fields in the shared BuildInputSchema + (srpm_path, dist_git_branch) are tolerated when build_agent dumps the whole + model. This must be ``allow`` rather than ``ignore``: only ``allow`` makes + Pydantic emit ``additionalProperties: true`` in the advertised JSON schema, + and beeai's MCP client rebuilds the tool's input model from that schema with + ``extra='forbid'`` unless ``additionalProperties`` is truthy. With ``ignore`` + the extra COPR fields would be rejected client-side as "Tool input + validation error" before the build ever reaches the gateway. + """ + + model_config = ConfigDict(extra="allow") + + git_url: str = Field(description="Clonable URL of the fork holding the pushed commit") + revision: str = Field(description="Pushed commit SHA to build") + package_name: str = Field(description="RPM package name") + target_branch: str = Field(description="dist-git branch the build targets") + specfile: str | None = Field( + default=None, description="Spec file name when it differs from .spec" + ) + jira_issue: str | None = Field(default=None, description="Jira issue key, for logging only") + + +class BuildPackageToolOutput(JSONToolOutput[BuildResult]): + pass + + +class KonfluxBuildTool(Tool[BuildPackageToolInput, ToolRunOptions, BuildPackageToolOutput]): + name = "build_package" + # Must exceed the polling budget (KONFLUX_BUILD_TIMEOUT + grace). + timeout = KONFLUX_BUILD_TIMEOUT + 2 * KONFLUX_TIMEOUT_GRACE_PERIOD + description = """ + Builds the specified package revision in Konflux (Tekton PipelineRun). + """ + input_schema = BuildPackageToolInput + + def _create_emitter(self) -> Emitter: + return Emitter.root().child(namespace=["tool", "konflux", self.name], creator=self) + + async def _run( + self, + tool_input: BuildPackageToolInput, + options: ToolRunOptions | None, + context: RunContext, + ) -> BuildPackageToolOutput: + config = _load_config() + client = KonfluxClient(config, config.namespace) + + pipeline_revision = await _resolve_pipeline_revision(config.pipeline_url) + + short_sha = _sanitize_tag(tool_input.revision[:12]) or "rev" + run_id = uuid.uuid4().hex[:8] + run_name = f"ymir-build-{short_sha}-{run_id}" + tag = _sanitize_tag(f"{tool_input.package_name}-{short_sha}-{run_id}") + ocistorage = f"{config.container_image}:{tag}" + + with tool_error_context( + "Failed to create the Konflux git-auth secret", + package=tool_input.package_name, + git_url=tool_input.git_url, + ): + secret_body = _git_auth_secret_body(tool_input.git_url, config.gitlab_token) + secret_name = await asyncio.to_thread(client.create_secret, secret_body) + + try: + body = _pipelinerun_body( + config=config, + namespace=config.namespace, + service_account=config.service_account, + run_name=run_name, + package_name=tool_input.package_name, + git_url=tool_input.git_url, + revision=tool_input.revision, + target_branch=tool_input.target_branch, + specfile=tool_input.specfile, + ocistorage=ocistorage, + pipeline_revision=pipeline_revision, + secret_name=secret_name, + ) + with tool_error_context( + "Failed to submit the Konflux PipelineRun", + package=tool_input.package_name, + run_name=run_name, + ): + submitted_name = await asyncio.to_thread(client.create_pipelinerun, body) + logger.info( + "%s: Konflux build submitted: %s (ociStorage %s)", + tool_input.jira_issue or tool_input.package_name, + submitted_name, + ocistorage, + ) + return await self._poll_to_completion(client, submitted_name, config) + finally: + try: + await asyncio.to_thread(client.delete_secret, secret_name) + except Exception as e: + logger.warning("Failed to delete Konflux git-auth secret %s: %s", secret_name, e) + + async def _poll_to_completion( + self, + client: KonfluxClient, + run_name: str, + config: KonfluxConfig, + ) -> BuildPackageToolOutput: + start = time.monotonic() + while time.monotonic() - start < KONFLUX_BUILD_TIMEOUT + KONFLUX_TIMEOUT_GRACE_PERIOD: + pipelinerun = await asyncio.to_thread(client.get_pipelinerun, run_name) + status = _pipelinerun_status(pipelinerun) + if status in SUCCESSFUL_STATES: + logger.info("Konflux build %s succeeded", run_name) + return BuildPackageToolOutput(result=BuildResult(success=True)) + if status in RUNNING_STATES or status == "Unknown": + await asyncio.sleep(KONFLUX_POLLING_INTERVAL) + continue + message = _pipelinerun_message(pipelinerun) or status + logger.info("Konflux build %s failed: %s (%s)", run_name, status, message) + log_urls = await asyncio.to_thread(_collect_failure_log_urls, client, pipelinerun or {}, config) + return BuildPackageToolOutput( + result=BuildResult( + success=False, + error_message=f"PipelineRun {run_name} finished as {status}: {message}", + artifacts_urls=log_urls or None, + ) + ) + + message = f"Reached timeout for Konflux build {run_name}" + logger.info(message) + return BuildPackageToolOutput( + result=BuildResult(success=False, is_timeout=True, error_message=message) + ) + + +class DownloadArtifactsToolInput(BaseModel): + artifacts_urls: list[str] = Field(description="URLs to build artifacts (logs)") + + +class DownloadArtifactsResult(BaseModel): + target_path: Path = Field(description="Location of downloaded files") + + +class DownloadArtifactsToolOutput(JSONToolOutput[DownloadArtifactsResult]): + def get_text_content(self) -> str: + return f"target_path: {self.result.target_path}" + + +class KonfluxDownloadArtifactsTool( + Tool[DownloadArtifactsToolInput, ToolRunOptions, DownloadArtifactsToolOutput] +): + name = "download_artifacts" + timeout = 120 + description = """ + Downloads Konflux build artifacts (task logs) to a temporary location. + Gzipped logs are decompressed automatically. + """ + input_schema = DownloadArtifactsToolInput + + def _create_emitter(self) -> Emitter: + return Emitter.root().child(namespace=["tool", "konflux", self.name], creator=self) + + async def _run( + self, + tool_input: DownloadArtifactsToolInput, + options: ToolRunOptions | None, + context: RunContext, + ) -> DownloadArtifactsToolOutput: + # Kubearchive pod-log endpoints are authenticated (unlike COPR's public URLs). + config = _load_config() + headers = {"Authorization": f"Bearer {config.token}", "User-Agent": YMIR_USER_AGENT} + session = _build_session() + target_path = Path(tempfile.mkdtemp()) + try: + for url in tool_input.artifacts_urls: + logger.info("Downloading Konflux build artifact from: %s", url) + with tool_error_context("Failed to download build artifact", artifacts_url=url): + content = await asyncio.to_thread(self._download, session, url, headers) + (target_path / self._filename_for(url)).write_bytes(content) + except Exception: + rmtree(target_path) + raise + return DownloadArtifactsToolOutput(result=DownloadArtifactsResult(target_path=target_path)) + + @staticmethod + def _download(session: requests.Session, url: str, headers: dict) -> bytes: + response = session.get(url, headers=headers, timeout=DEFAULT_TIMEOUT) + if response.status_code >= 400: + raise ToolError(f"{response.status_code} {response.reason}") + content = response.content + if content.startswith(b"\x1f\x8b"): + content = gzip.decompress(content) + return content + + @staticmethod + def _filename_for(url: str) -> str: + parts = urlsplit(url) + path_segments = [s for s in parts.path.split("/") if s] + pod = path_segments[path_segments.index("pods") + 1] if "pods" in path_segments else "" + container = (parse_qs(parts.query).get("container") or [""])[0] + name = f"{pod}__{container}.log" if pod and container else Path(parts.path).name or "artifact.log" + return _sanitize_tag(name) diff --git a/ymir/tools/privileged/tests/unit/test_gateway.py b/ymir/tools/privileged/tests/unit/test_gateway.py index 6acfe06d4..298a6a683 100644 --- a/ymir/tools/privileged/tests/unit/test_gateway.py +++ b/ymir/tools/privileged/tests/unit/test_gateway.py @@ -272,3 +272,53 @@ def test_gateway_starts_without_log_detective(self): assert registered_tools, "No tools were registered" assert not any(t.name == "extract_log_snippets" for t in registered_tools) + + +class TestBuildBackendToggle: + """BUILD_BACKEND selects the build_package / download_artifacts implementations.""" + + def _run_main_and_collect(self, monkeypatch, backend: str | None): + registered_tools = [] + + mock_server = flexmock() + mock_server.should_receive("register_many").once().replace_with( + lambda tools: registered_tools.extend(tools) + ) + mock_server.should_receive("aserve").once().replace_with(mock_aserve) + + import ymir.tools.privileged.gateway as gateway_module + + if backend is None: + monkeypatch.delenv("BUILD_BACKEND", raising=False) + else: + monkeypatch.setenv("BUILD_BACKEND", backend) + + flexmock(gateway_module, MCPServer=lambda config: mock_server) + flexmock(gateway_module).should_receive("setup_logging").once() + flexmock(gateway_module).should_receive("apply_zstream_override_from_env").once() + flexmock(gateway_module).should_receive("get_log_detective_mcp").once().and_return( + _create_async_return([]) + ) + + gateway_module.main() + return { + type(t).__name__ + for t in registered_tools + if getattr(t, "name", None) in ("build_package", "download_artifacts") + } + + def test_default_backend_registers_copr_tools(self, monkeypatch): + names = self._run_main_and_collect(monkeypatch, None) + assert names == {"BuildPackageTool", "DownloadArtifactsTool"} + + def test_copr_backend_registers_copr_tools(self, monkeypatch): + names = self._run_main_and_collect(monkeypatch, "copr") + assert names == {"BuildPackageTool", "DownloadArtifactsTool"} + + def test_konflux_backend_registers_konflux_tools(self, monkeypatch): + names = self._run_main_and_collect(monkeypatch, "konflux") + assert names == {"KonfluxBuildTool", "KonfluxDownloadArtifactsTool"} + + def test_unknown_backend_falls_back_to_copr(self, monkeypatch): + names = self._run_main_and_collect(monkeypatch, "bogus") + assert names == {"BuildPackageTool", "DownloadArtifactsTool"} diff --git a/ymir/tools/privileged/tests/unit/test_gitlab.py b/ymir/tools/privileged/tests/unit/test_gitlab.py index 710945901..5ddfaf8b7 100644 --- a/ymir/tools/privileged/tests/unit/test_gitlab.py +++ b/ymir/tools/privileged/tests/unit/test_gitlab.py @@ -121,6 +121,94 @@ async def test_fork_repository(repository, fork_exists, fork_namespace): assert (await ForkRepositoryTool().run(input={"repository": repository})).result == clone_url +def _mock_original_project(*, repository, package, bot_username, fork, expected_data, fork_exists=False): + """Mock get_project_from_url for a redhat gitlab.com project (no fork yet).""" + original_git_url = f"{repository}.git" + flexmock(GitlabService).should_receive("get_project_from_url").with_args(url=repository).and_return( + flexmock( + get_forks=lambda: [fork] if fork_exists else [], + get_git_urls=lambda: {"git": original_git_url}, + gitlab_repo=flexmock( + forks=flexmock() + .should_receive("create") + .with_args(data=expected_data) + .and_return(fork.gitlab_repo) + .mock(), + name=package, + namespace={ + "full_path": repository.removeprefix("https://gitlab.com/").removesuffix(f"/{package}") + }, + path=package, + ), + service=flexmock( + instance_url="https://gitlab.com", + user=flexmock(get_username=lambda: bot_username), + ), + ) + ) + return original_git_url + + +@pytest.mark.asyncio +async def test_fork_repository_copr_dry_run_returns_original(monkeypatch): + """Copr builds from a local SRPM, so DRY_RUN skips fork creation and echoes the origin.""" + monkeypatch.setenv("DRY_RUN", "true") + monkeypatch.delenv("BUILD_BACKEND", raising=False) # default copr + monkeypatch.setenv("FORK_NAMESPACE", "redhat/rhel/bot-branches") + repository = "https://gitlab.com/redhat/centos-stream/rpms/bash" + package = "bash" + fork = _fork_project_mock( + target_namespace="redhat/rhel/bot-branches", + fork_name="centos_rpms_bash", + clone_url="https://gitlab.com/redhat/rhel/bot-branches/centos_rpms_bash.git", + ) + expected_data = { + "name": "centos_rpms_bash", + "path": "centos_rpms_bash", + "namespace": "redhat/rhel/bot-branches", + } + original = _mock_original_project( + repository=repository, + package=package, + bot_username="test-bot", + fork=fork, + expected_data=expected_data, + ) + result = (await ForkRepositoryTool().run(input={"repository": repository})).result + assert result == original + + +@pytest.mark.asyncio +async def test_fork_repository_konflux_dry_run_creates_fork(monkeypatch): + """Konflux builds from a pushed fork ref, so the fork is created even under DRY_RUN.""" + monkeypatch.setenv("DRY_RUN", "true") + monkeypatch.setenv("BUILD_BACKEND", "konflux") + monkeypatch.setenv("FORK_NAMESPACE", "redhat/rhel/bot-branches") + repository = "https://gitlab.com/redhat/centos-stream/rpms/bash" + package = "bash" + clone_url = "https://gitlab.com/redhat/rhel/bot-branches/centos_rpms_bash.git" + fork = _fork_project_mock( + target_namespace="redhat/rhel/bot-branches", + fork_name="centos_rpms_bash", + clone_url=clone_url, + ) + flexmock(GitlabProject).new_instances(fork) + expected_data = { + "name": "centos_rpms_bash", + "path": "centos_rpms_bash", + "namespace": "redhat/rhel/bot-branches", + } + _mock_original_project( + repository=repository, + package=package, + bot_username="test-bot", + fork=fork, + expected_data=expected_data, + ) + result = (await ForkRepositoryTool().run(input={"repository": repository})).result + assert result == clone_url + + def test_wait_for_fork_ready_returns_when_import_finished(): fork = _fork_project_mock( target_namespace="redhat/rhel/bot-branches", diff --git a/ymir/tools/privileged/tests/unit/test_konflux.py b/ymir/tools/privileged/tests/unit/test_konflux.py new file mode 100644 index 000000000..6dd6ad4da --- /dev/null +++ b/ymir/tools/privileged/tests/unit/test_konflux.py @@ -0,0 +1,406 @@ +import asyncio +import gzip +from pathlib import Path + +import pytest +import requests +from beeai_framework.tools import ToolError +from flexmock import flexmock +from pydantic import ValidationError + +from ymir.common.models import BuildResult +from ymir.tools.privileged import konflux as konflux_mod +from ymir.tools.privileged.konflux import ( + BuildPackageToolInput, + KonfluxBuildTool, + KonfluxClient, + KonfluxConfig, + KonfluxDownloadArtifactsTool, + _git_auth_secret_body, + _pipelinerun_body, + _pipelinerun_status, + _sanitize_tag, +) + +BUILD_INPUT = { + "git_url": "https://gitlab.cee.redhat.com/redhat/rhel/bot-branches/expat.git", + "revision": "0123456789abcdef0123456789abcdef01234567", # pragma: allowlist secret + "package_name": "expat", + "target_branch": "rhel-10.1", + "jira_issue": "RHEL-12345", +} + + +_NS = "ymir-tenant" +_SA = "build-pipeline-ymir-scratch-build" +_IMG = "quay.io/redhat-user-workloads/ymir-tenant/ymir-scratch-build" + + +def _config() -> KonfluxConfig: + return KonfluxConfig( + api_url="https://konflux.example.com", + kubearchive_url="https://kubearchive.example.com", + pipeline_url="https://gitlab.example.com/rhel-on-konflux/rpmbuild-pipeline.git", + token="tok-123", + gitlab_token="glpat-xyz", + namespace=_NS, + service_account=_SA, + container_image=_IMG, + ) + + +def _no_sleep(): + async def _sleep(*_): + return + + flexmock(asyncio).should_receive("sleep").replace_with(_sleep) + + +def _mock_prelude(cfg, *, pipeline_revision="pipelinesha"): + """Mock config load + pipeline revision resolution shared by build tests.""" + flexmock(konflux_mod).should_receive("_load_config").and_return(cfg) + + async def _rev(*_): + return pipeline_revision + + flexmock(konflux_mod).should_receive("_resolve_pipeline_revision").replace_with(_rev) + + +# --------------------------------------------------------------------------- # +# Input schema reconciliation # +# --------------------------------------------------------------------------- # +def test_input_schema_requires_konflux_fields(): + with pytest.raises(ValidationError): + BuildPackageToolInput.model_validate( + {"srpm_path": "/x.src.rpm", "dist_git_branch": "rhel-10.1", "jira_issue": "RHEL-1"} + ) + + +def test_input_schema_tolerates_copr_only_fields(): + # build_agent dumps the whole shared BuildInputSchema (including the COPR-only + # srpm_path/dist_git_branch) at every backend. The Konflux tool must accept + # that payload rather than reject the extra keys. + model = BuildPackageToolInput.model_validate( + {**BUILD_INPUT, "srpm_path": "/x.src.rpm", "dist_git_branch": "rhel-10.1"} + ) + assert model.package_name == "expat" + assert model.git_url == BUILD_INPUT["git_url"] + + +def test_input_schema_advertises_additional_properties(): + # beeai's MCP client rebuilds the tool's input model from the advertised JSON + # schema with extra="forbid" UNLESS additionalProperties is truthy. Without + # this the COPR-only fields would fail client-side validation ("Tool input + # validation error") before the build ever reaches the gateway. + schema = BuildPackageToolInput.model_json_schema() + assert schema.get("additionalProperties") is True + + +# --------------------------------------------------------------------------- # +# Pure helpers # +# --------------------------------------------------------------------------- # +def test_sanitize_tag_replaces_invalid_chars(): + assert _sanitize_tag("gtk+2.0") == "gtk-2.0" + assert _sanitize_tag("valid_tag-1.2.3") == "valid_tag-1.2.3" + + +@pytest.mark.parametrize( + ("data", "expected"), + [ + ({"status": {"conditions": [{"reason": "Succeeded"}]}}, "Succeeded"), + ({"status": {"conditions": [{"reason": "Running"}]}}, "Running"), + ({"status": {"conditions": []}}, "Unknown"), + ({}, "Unknown"), + (None, "Unknown"), + ], +) +def test_pipelinerun_status(data, expected): + assert _pipelinerun_status(data) == expected + + +def test_git_auth_secret_body_parses_host_and_repo(): + body = _git_auth_secret_body(BUILD_INPUT["git_url"], "glpat-xyz") + assert body["type"] == "kubernetes.io/basic-auth" + assert body["metadata"]["generateName"] == "gitlab-secret-" + assert body["metadata"]["labels"]["appstudio.redhat.com/scm.host"] == "gitlab.cee.redhat.com" + assert ( + body["metadata"]["annotations"]["appstudio.redhat.com/scm.repository"] + == "https://gitlab.cee.redhat.com/redhat/rhel/bot-branches/expat" + ) + assert body["stringData"]["username"] == "gitlab-ci-token" + assert body["stringData"]["password"] == "glpat-xyz" # pragma: allowlist secret + + +def test_pipelinerun_body_shape(): + cfg = _config() + body = _pipelinerun_body( + config=cfg, + namespace=_NS, + service_account=_SA, + run_name="ymir-build-abc-1234", + package_name="expat", + git_url=BUILD_INPUT["git_url"], + revision=BUILD_INPUT["revision"], + target_branch="rhel-10.1", + specfile=None, + ocistorage=f"{_IMG}:expat-abc-1234", + pipeline_revision="psha", + secret_name="gitlab-secret-xyz", # pragma: allowlist secret + ) + params = {p["name"]: p["value"] for p in body["spec"]["params"]} + assert params["package-name"] == "expat" + assert params["git-url"] == BUILD_INPUT["git_url"] + assert params["revision"] == BUILD_INPUT["revision"] + assert params["target-branch"] == "rhel-10.1" + assert params["koji-target"] == "DEFAULT" + assert params["hermetic"] == "true" + assert params["specfile"] == "null" + assert params["build-architectures"] == ["x86_64"] + assert params["build-platforms"] == ["linux-mxlarge/amd64"] + assert body["spec"]["taskRunTemplate"]["serviceAccountName"] == _SA + assert body["metadata"]["namespace"] == _NS + body_secret = body["spec"]["workspaces"][0]["secret"]["secretName"] + assert body_secret == "gitlab-secret-xyz" # pragma: allowlist secret + ref = {p["name"]: p["value"] for p in body["spec"]["pipelineRef"]["params"]} + assert ref["revision"] == "psha" + assert ref["pathInRepo"] == "pipeline/build-rpm-package.yaml" + # Standalone run: no application/component labels, no pull_request annotation. + assert "appstudio.openshift.io/application" not in body["metadata"]["labels"] + assert "build.appstudio.redhat.com/pull_request_number" not in body["metadata"]["annotations"] + + +# --------------------------------------------------------------------------- # +# Config loading # +# --------------------------------------------------------------------------- # +def test_load_config_missing_env_raises(monkeypatch): + for var in ( + "KONFLUX_API_URL", + "KONFLUX_IMAGE_REPO", + "KONFLUX_PIPELINE_URL", + "GITLAB_TOKEN", + "KONFLUX_TOKEN_FILE", + "KONFLUX_TOKEN", + ): + monkeypatch.delenv(var, raising=False) + with pytest.raises(konflux_mod.ToolErrorWithContext): + konflux_mod._load_config() + + +def test_load_config_reads_token_file(tmp_path, monkeypatch): + token_file = tmp_path / "token" + token_file.write_text(" secret-token\n") + monkeypatch.setenv("KONFLUX_API_URL", "https://api.example.com/") + monkeypatch.setenv("KONFLUX_PIPELINE_URL", "https://g/p.git") + monkeypatch.setenv("GITLAB_TOKEN", "glpat") + monkeypatch.setenv("KONFLUX_TOKEN_FILE", str(token_file)) + monkeypatch.setenv("KUBEARCHIVE_API_URL", "https://ka.example.com/") + for var in ("KONFLUX_NAMESPACE", "KONFLUX_SERVICE_ACCOUNT", "KONFLUX_IMAGE_REPO"): + monkeypatch.delenv(var, raising=False) + cfg = konflux_mod._load_config() + assert cfg.token == "secret-token" + assert cfg.api_url == "https://api.example.com" + assert cfg.kubearchive_url == "https://ka.example.com" + # Fixed build target defaults (overridable via env). + assert cfg.namespace == konflux_mod.DEFAULT_NAMESPACE + assert cfg.service_account == konflux_mod.DEFAULT_SERVICE_ACCOUNT + assert cfg.container_image == konflux_mod.DEFAULT_IMAGE_REPO + + +def test_load_config_reads_inline_token(monkeypatch): + """KONFLUX_TOKEN may supply the SA token directly, without a file.""" + monkeypatch.setenv("KONFLUX_API_URL", "https://api.example.com/") + monkeypatch.setenv("KONFLUX_PIPELINE_URL", "https://g/p.git") + monkeypatch.setenv("GITLAB_TOKEN", "glpat") + monkeypatch.delenv("KONFLUX_TOKEN_FILE", raising=False) + monkeypatch.setenv("KONFLUX_TOKEN", " inline-secret\n") + cfg = konflux_mod._load_config() + assert cfg.token == "inline-secret" + + +def test_load_config_token_file_takes_precedence(tmp_path, monkeypatch): + """When both are set, the (rotatable) token file wins over inline KONFLUX_TOKEN.""" + token_file = tmp_path / "token" + token_file.write_text("file-token\n") + monkeypatch.setenv("KONFLUX_API_URL", "https://api.example.com/") + monkeypatch.setenv("KONFLUX_PIPELINE_URL", "https://g/p.git") + monkeypatch.setenv("GITLAB_TOKEN", "glpat") + monkeypatch.setenv("KONFLUX_TOKEN_FILE", str(token_file)) + monkeypatch.setenv("KONFLUX_TOKEN", "inline-secret") + cfg = konflux_mod._load_config() + assert cfg.token == "file-token" + + +# --------------------------------------------------------------------------- # +# Build flow # +# --------------------------------------------------------------------------- # +@pytest.mark.asyncio +async def test_build_success_submits_and_cleans_up(): + cfg = _config() + _mock_prelude(cfg) + _no_sleep() + captured = {} + + # Variadic so the capture works whether or not flexmock passes ``self``. + def _create_secret(*args): + captured["secret"] = args[-1] + return "gitlab-secret-abc" + + def _create_pr(*args): + body = args[-1] + captured["pr"] = body + return body["metadata"]["name"] + + flexmock(KonfluxClient).should_receive("create_secret").replace_with(_create_secret).once() + flexmock(KonfluxClient).should_receive("create_pipelinerun").replace_with(_create_pr).once() + flexmock(KonfluxClient).should_receive("get_pipelinerun").and_return( + {"status": {"conditions": [{"reason": "Running"}]}} + ).and_return({"status": {"conditions": [{"reason": "Succeeded"}]}}) + flexmock(KonfluxClient).should_receive("delete_secret").with_args("gitlab-secret-abc").once() + + out = await KonfluxBuildTool().run(input=BUILD_INPUT) + assert isinstance(out.result, BuildResult) + assert out.result.success is True + assert out.result.is_timeout is False + + params = {p["name"]: p["value"] for p in captured["pr"]["spec"]["params"]} + assert params["package-name"] == "expat" + assert params["git-url"] == BUILD_INPUT["git_url"] + assert params["ociStorage"].startswith(f"{_IMG}:expat-0123456789ab-") + ws_secret = captured["pr"]["spec"]["workspaces"][0]["secret"]["secretName"] + assert ws_secret == "gitlab-secret-abc" # pragma: allowlist secret + assert captured["secret"]["type"] == "kubernetes.io/basic-auth" + + +@pytest.mark.asyncio +async def test_build_failure_collects_logs(): + cfg = _config() + _mock_prelude(cfg) + _no_sleep() + flexmock(KonfluxClient).should_receive("create_secret").and_return("sec") + flexmock(KonfluxClient).should_receive("create_pipelinerun").and_return("ymir-build-x") + pipelinerun = { + "status": { + "conditions": [{"reason": "Failed", "message": "boom"}], + "childReferences": [{"name": "tr-1"}], + } + } + flexmock(KonfluxClient).should_receive("get_pipelinerun").and_return(pipelinerun) + flexmock(KonfluxClient).should_receive("get_taskrun").with_args("tr-1").and_return( + { + "status": { + "podName": "pod-1", + "conditions": [{"reason": "Failed"}], + "steps": [{"container": "step-build"}, {"container": "step-prep"}], + } + } + ) + flexmock(KonfluxClient).should_receive("delete_secret").with_args("sec").once() + + out = await KonfluxBuildTool().run(input=BUILD_INPUT) + assert out.result.success is False + assert "Failed" in out.result.error_message + assert "boom" in out.result.error_message + urls = out.result.artifacts_urls + assert any("pods/pod-1/log?container=step-build" in u for u in urls) + assert all(u.startswith(cfg.kubearchive_url) for u in urls) + + +@pytest.mark.asyncio +async def test_build_timeout(monkeypatch): + cfg = _config() + _mock_prelude(cfg) + _no_sleep() + # Negative budget -> poll loop exits immediately into the timeout branch. + monkeypatch.setattr(konflux_mod, "KONFLUX_BUILD_TIMEOUT", -100) + monkeypatch.setattr(konflux_mod, "KONFLUX_TIMEOUT_GRACE_PERIOD", 0) + flexmock(KonfluxClient).should_receive("create_secret").and_return("sec") + flexmock(KonfluxClient).should_receive("create_pipelinerun").and_return("run") + flexmock(KonfluxClient).should_receive("delete_secret").with_args("sec").once() + + out = await KonfluxBuildTool().run(input=BUILD_INPUT) + assert out.result.success is False + assert out.result.is_timeout is True + + +@pytest.mark.asyncio +async def test_secret_cleanup_on_submit_failure(): + cfg = _config() + _mock_prelude(cfg) + flexmock(KonfluxClient).should_receive("create_secret").and_return("sec") + flexmock(KonfluxClient).should_receive("create_pipelinerun").and_raise(RuntimeError("submit boom")) + flexmock(KonfluxClient).should_receive("delete_secret").with_args("sec").once() + + with pytest.raises(ToolError): + await KonfluxBuildTool().run(input=BUILD_INPUT) + + +# --------------------------------------------------------------------------- # +# KonfluxClient Kubearchive fallback # +# --------------------------------------------------------------------------- # +def test_get_pipelinerun_falls_back_to_kubearchive(): + client = KonfluxClient(_config(), "ymir-tenant") + calls = [] + + def _get(*args, **kwargs): + url = args[-1] + calls.append(url) + if "kubearchive" in url: + return flexmock(status_code=200, json=lambda: {"ok": True}) + return flexmock(status_code=404, json=dict) + + flexmock(requests.Session).should_receive("get").replace_with(_get) + data = client.get_pipelinerun("run-1") + assert data == {"ok": True} + assert any("kubearchive" in u for u in calls) + + +# --------------------------------------------------------------------------- # +# Download artifacts # +# --------------------------------------------------------------------------- # +@pytest.mark.asyncio +async def test_download_artifacts_sends_bearer_token(): + cfg = _config() + flexmock(konflux_mod).should_receive("_load_config").and_return(cfg) + url = f"{cfg.kubearchive_url}/api/v1/namespaces/ymir-tenant/pods/pod-1/log?container=step-build" + captured = {} + + def _get(*args, **kwargs): + captured["headers"] = kwargs.get("headers") + captured["url"] = args[-1] + return flexmock(status_code=200, reason="OK", content=b"log-bytes") + + flexmock(requests.Session).should_receive("get").replace_with(_get) + out = await KonfluxDownloadArtifactsTool().run(input={"artifacts_urls": [url]}) + target = Path(out.result.target_path) / "pod-1__step-build.log" + assert target.read_bytes() == b"log-bytes" + assert captured["headers"]["Authorization"] == "Bearer tok-123" + + +@pytest.mark.asyncio +async def test_download_artifacts_decompresses_gzip(): + cfg = _config() + flexmock(konflux_mod).should_receive("_load_config").and_return(cfg) + url = f"{cfg.kubearchive_url}/api/v1/namespaces/ymir-tenant/pods/pod-1/log?container=step-build" + payload = gzip.compress(b"hello logs") + + def _get(*args, **kwargs): + return flexmock(status_code=200, reason="OK", content=payload) + + flexmock(requests.Session).should_receive("get").replace_with(_get) + out = await KonfluxDownloadArtifactsTool().run(input={"artifacts_urls": [url]}) + target = Path(out.result.target_path) / "pod-1__step-build.log" + assert target.read_bytes() == b"hello logs" + + +@pytest.mark.asyncio +async def test_download_artifacts_raises_on_http_error(): + cfg = _config() + flexmock(konflux_mod).should_receive("_load_config").and_return(cfg) + url = f"{cfg.kubearchive_url}/api/v1/namespaces/ymir-tenant/pods/pod-1/log?container=step-build" + + def _get(*args, **kwargs): + return flexmock(status_code=404, reason="Not Found", content=b"") + + flexmock(requests.Session).should_receive("get").replace_with(_get) + with pytest.raises(ToolError): + await KonfluxDownloadArtifactsTool().run(input={"artifacts_urls": [url]}) From 8a6ecce4a3f19f209ab1fe6e997b3a0e84e382b8 Mon Sep 17 00:00:00 2001 From: Tomas Korbar Date: Thu, 8 Oct 2026 10:02:45 +0200 Subject: [PATCH 2/5] Close stale update MR on rerun before re-pushing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the backport or rebase workflow reruns for a Jira issue, it force-pushes the deterministic update branch (automated-package-update- ). Any merge request still open on that branch picks up the push and re-runs its GitLab CI pipeline — a wasted scratch build on an MR the rerun is about to supersede. Close the lingering MR first, so only the rerun's eventual fresh MR runs CI once. Both agents gain a close_stale_merge_requests workflow step that runs after the Jira status change / sibling resolution and before the fork, via a new tasks.close_stale_update_merge_requests helper and a new privileged close_merge_request GitLab tool. The close is scoped to the bot's own update branch (matched on source_branch) targeting the dist-git branch, so human MRs referencing the issue are never touched. It is a GitLab write, so it is suppressed under DRY_RUN, and the backport inherited-publication resume path skips it (it deliberately reuses its own MR). Co-Authored-By: Claude Opus 4.8 --- ymir/agents/backport_agent.py | 24 ++++- ymir/agents/rebase_agent.py | 26 +++++ ymir/agents/tasks.py | 50 +++++++++ ymir/agents/tests/unit/test_close_stale_mr.py | 74 +++++++++++++ ymir/agents/tests/unit/test_tasks.py | 101 ++++++++++++++++++ ymir/tools/privileged/gateway.py | 2 + ymir/tools/privileged/gitlab.py | 35 ++++++ .../privileged/tests/unit/test_gitlab.py | 30 ++++++ 8 files changed, 341 insertions(+), 1 deletion(-) create mode 100644 ymir/agents/tests/unit/test_close_stale_mr.py diff --git a/ymir/agents/backport_agent.py b/ymir/agents/backport_agent.py index 6d3173f97..a52ed0cfd 100644 --- a/ymir/agents/backport_agent.py +++ b/ymir/agents/backport_agent.py @@ -872,7 +872,7 @@ async def change_jira_status(state): return "resume_inherited_publication" if dry_run: logger.info(f"Dry run: skipping Jira status change of {state.jira_issue} to In Progress") - return "fork_and_prepare_dist_git" + return "close_stale_merge_requests" # tasks.change_jira_status further gates the write on # JIRA_ALLOW_STATUS_CHANGES; nothing else to check here. try: @@ -883,6 +883,27 @@ async def change_jira_status(state): ) except Exception as status_error: logger.warning(f"Failed to change status for {state.jira_issue}: {status_error}") + return "close_stale_merge_requests" + + async def close_stale_merge_requests(state): + # A rerun re-pushes the update branch; a lingering open MR would + # re-trigger GitLab CI (scratch builds) on every push and waste + # resources. Close it first so only the rerun's fresh MR runs CI. + # The inherited-publication resume path skips this step (it reuses + # its own MR). MR writes are suppressed under dry-run. + if not dry_run: + try: + closed = await tasks.close_stale_update_merge_requests( + jira_issue=state.jira_issue, + package=state.package, + dist_git_branch=state.dist_git_branch, + available_tools=gateway_tools, + dist_git_namespace=state.dist_git_namespace, + ) + if closed: + logger.info("Closed %d stale MR(s) for %s: %s", len(closed), state.jira_issue, closed) + except Exception as e: + logger.warning("Failed to close stale MRs for %s: %s", state.jira_issue, e) return "fork_and_prepare_dist_git" async def resume_inherited_publication(state): @@ -2055,6 +2076,7 @@ async def comment_in_jira(state): return Workflow.END workflow.add_step("change_jira_status", change_jira_status) + workflow.add_step("close_stale_merge_requests", close_stale_merge_requests) workflow.add_step("resume_inherited_publication", resume_inherited_publication) workflow.add_step("fork_and_prepare_dist_git", fork_and_prepare_dist_git) workflow.add_step("prepare_normal_backport", prepare_normal_backport) diff --git a/ymir/agents/rebase_agent.py b/ymir/agents/rebase_agent.py index 0589f420f..e30b468ac 100644 --- a/ymir/agents/rebase_agent.py +++ b/ymir/agents/rebase_agent.py @@ -371,6 +371,31 @@ async def find_consolidated_siblings(state): logger.info( f"Using {len(state.consolidated_issues)} consolidated siblings from triage result" ) + return "close_stale_merge_requests" + + async def close_stale_merge_requests(state): + # A rerun re-pushes the update branch; a lingering open MR would + # re-trigger GitLab CI (scratch builds) on every push and waste + # resources. Close it first so only the rerun's fresh MR runs CI. + # MR writes are suppressed under dry-run. + if not dry_run: + try: + closed = await tasks.close_stale_update_merge_requests( + jira_issue=state.jira_issue, + package=state.package, + dist_git_branch=state.dist_git_branch, + available_tools=gateway_tools, + dist_git_namespace=state.dist_git_namespace, + ) + if closed: + logger.info( + "Closed %d stale MR(s) for %s: %s", + len(closed), + state.jira_issue, + closed, + ) + except Exception as e: + logger.warning("Failed to close stale MRs for %s: %s", state.jira_issue, e) return "fork_and_prepare_dist_git" async def fork_and_prepare_dist_git(state): @@ -680,6 +705,7 @@ async def comment_in_jira(state): workflow.add_step("check_if_sibling", check_if_sibling) workflow.add_step("change_jira_status", change_jira_status) workflow.add_step("find_consolidated_siblings", find_consolidated_siblings) + workflow.add_step("close_stale_merge_requests", close_stale_merge_requests) workflow.add_step("fork_and_prepare_dist_git", fork_and_prepare_dist_git) workflow.add_step("run_rebase_agent", run_rebase_agent) workflow.add_step("run_build_agent", run_build_agent) diff --git a/ymir/agents/tasks.py b/ymir/agents/tasks.py index f71b74e38..b5ccfb308 100644 --- a/ymir/agents/tasks.py +++ b/ymir/agents/tasks.py @@ -631,6 +631,56 @@ async def open_update_merge_request( return mr.url, mr.is_new_mr +async def close_stale_update_merge_requests( + jira_issue: str, + package: str, + dist_git_branch: str, + available_tools: list[Tool], + dist_git_namespace: str | None = None, +) -> list[str]: + """Close any still-open update MR for *jira_issue* before a rerun re-pushes. + + A rerun force-pushes the update branch; a lingering open MR would re-trigger + GitLab CI (scratch builds) on every push and waste resources. Closing it + first means only the rerun's eventual fresh MR runs CI. Scoped to the bot's + own update branch (``{BRANCH_PREFIX}-``) so human MRs are never + touched. Returns the URLs of the MRs that were closed. + """ + namespace = resolve_dist_git_namespace(dist_git_branch, dist_git_namespace) + project = f"redhat/{namespace}/rpms/{package}" + update_branch = f"{BRANCH_PREFIX}-{jira_issue}" + try: + mrs = await run_tool( + "list_project_merge_requests", + project=project, + state="opened", + target_branch=dist_git_branch, + available_tools=available_tools, + ) + except Exception as e: + logger.warning("Could not list open MRs for %s in %s: %s", jira_issue, project, e) + return [] + + closed: list[str] = [] + for mr in mrs or []: + if mr.get("source_branch") != update_branch: + continue + url = mr.get("url") + if not url: + continue + try: + await run_tool( + "close_merge_request", + merge_request_url=url, + available_tools=available_tools, + ) + closed.append(url) + logger.info("Closed stale MR %s for %s before rerun", url, jira_issue) + except Exception as e: + logger.warning("Failed to close stale MR %s for %s: %s", url, jira_issue, e) + return closed + + async def comment_in_jira( jira_issue: str, agent_type: str, diff --git a/ymir/agents/tests/unit/test_close_stale_mr.py b/ymir/agents/tests/unit/test_close_stale_mr.py new file mode 100644 index 000000000..a474ebf59 --- /dev/null +++ b/ymir/agents/tests/unit/test_close_stale_mr.py @@ -0,0 +1,74 @@ +"""On a rerun the agents close a lingering open MR before re-pushing. + +Re-pushing the update branch would otherwise re-trigger GitLab CI (scratch +builds) on the stale MR. The backport workflow therefore routes +``change_jira_status`` -> ``close_stale_merge_requests`` -> ``fork_and_prepare_dist_git``. +The close is a GitLab write, so it is suppressed under dry-run. These tests drive +the real workflow routing (via ``set_start`` + handler overrides, mirroring +test_konflux_reorder.py). +""" + +from contextlib import asynccontextmanager + +import pytest +from beeai_framework.workflows import Workflow +from flexmock import flexmock + +from ymir.agents import backport_agent +from ymir.agents import tasks as agent_tasks + + +def _prepare_common_mocks(monkeypatch): + @asynccontextmanager + async def gateway(*args, **kwargs): + yield [] + + flexmock(backport_agent).should_receive("mcp_tools").replace_with(gateway).once() + flexmock(backport_agent).should_receive("create_log_agent").and_return(None).once() + flexmock(backport_agent).should_receive("get_mock_local_tool_env").and_return(None).once() + monkeypatch.setenv("MCP_GATEWAY_URL", "http://gateway.invalid/sse") + + +@pytest.mark.parametrize("dry_run", [False, True]) +@pytest.mark.asyncio +async def test_close_stale_merge_requests_runs_before_fork(monkeypatch, tmp_path, dry_run): + calls = [] + + async def _close(**kwargs): + calls.append("close") + assert kwargs["jira_issue"] == "RHEL-123" + assert kwargs["package"] == "expat" + assert kwargs["dist_git_branch"] == "c10s" + return ["https://mr.example/old"] + + flexmock(agent_tasks).should_receive("close_stale_update_merge_requests").replace_with(_close).times( + 0 if dry_run else 1 + ) + _prepare_common_mocks(monkeypatch) + + run_workflow = Workflow.run + + def start_at(workflow, state, options=None): + workflow.set_start("close_stale_merge_requests") + workflow.steps["fork_and_prepare_dist_git"].handler = lambda _: ( + calls.append("fork"), + Workflow.END, + )[1] + return run_workflow(workflow, state, options) + + monkeypatch.setattr(Workflow, "run", start_at) + await backport_agent.run_workflow( + package="expat", + dist_git_branch="c10s", + upstream_patches=["https://example/patch.patch"], + jira_issue="RHEL-123", + cve_id=None, + dry_run=dry_run, + backport_agent_factory=lambda *_: None, + ) + + assert "fork" in calls + if dry_run: + assert "close" not in calls + else: + assert calls.index("close") < calls.index("fork") diff --git a/ymir/agents/tests/unit/test_tasks.py b/ymir/agents/tests/unit/test_tasks.py index 38ca491d5..840663591 100644 --- a/ymir/agents/tests/unit/test_tasks.py +++ b/ymir/agents/tests/unit/test_tasks.py @@ -16,6 +16,7 @@ _validate_generated_title, canonical_title_mentions_components, change_jira_status, + close_stale_update_merge_requests, commit_changes, commit_push_and_open_mr, ensure_canonical_changelog_title, @@ -1696,3 +1697,103 @@ async def _mock_run_tool(*_args, **_kwargs): assert config.abandon_autorelease is False assert config.treat_maintenance_rhel_as_zstream is False assert config.disregard_zstream_nvr_policy is False + + +# --------------------------------------------------------------------------- # +# close_stale_update_merge_requests # +# --------------------------------------------------------------------------- # +@pytest.mark.asyncio +async def test_close_stale_update_merge_requests_closes_only_bot_branch(): + from ymir.agents.constants import BRANCH_PREFIX + + jira = "RHEL-12345" + update_branch = f"{BRANCH_PREFIX}-{jira}" + mrs = [ + { + "url": "https://gitlab.com/redhat/rhel/rpms/expat/-/merge_requests/1", + "source_branch": update_branch, + "target_branch": "rhel-10.1", + "state": "opened", + }, + { + "url": "https://gitlab.com/redhat/rhel/rpms/expat/-/merge_requests/2", + "source_branch": "a-human-feature-branch", + "target_branch": "rhel-10.1", + "state": "opened", + }, + ] + closed: list[str] = [] + + async def fake_run_tool(tool, available_tools=None, **kwargs): + if tool == "list_project_merge_requests": + assert kwargs["project"] == "redhat/rhel/rpms/expat" + assert kwargs["state"] == "opened" + assert kwargs["target_branch"] == "rhel-10.1" + return mrs + if tool == "close_merge_request": + closed.append(kwargs["merge_request_url"]) + return "ok" + raise AssertionError(f"unexpected tool {tool}") + + flexmock(agent_tasks).should_receive("run_tool").replace_with(fake_run_tool) + + result = await close_stale_update_merge_requests( + jira_issue=jira, + package="expat", + dist_git_branch="rhel-10.1", + available_tools=[], + ) + + assert result == ["https://gitlab.com/redhat/rhel/rpms/expat/-/merge_requests/1"] + assert closed == ["https://gitlab.com/redhat/rhel/rpms/expat/-/merge_requests/1"] + + +@pytest.mark.asyncio +async def test_close_stale_update_merge_requests_returns_empty_when_list_fails(): + async def fake_run_tool(tool, available_tools=None, **kwargs): + if tool == "list_project_merge_requests": + raise RuntimeError("gitlab down") + raise AssertionError(f"close should not be attempted; got {tool}") + + flexmock(agent_tasks).should_receive("run_tool").replace_with(fake_run_tool) + + result = await close_stale_update_merge_requests( + jira_issue="RHEL-1", + package="expat", + dist_git_branch="rhel-10.1", + available_tools=[], + ) + assert result == [] + + +@pytest.mark.asyncio +async def test_close_stale_update_merge_requests_tolerates_close_failure(): + from ymir.agents.constants import BRANCH_PREFIX + + jira = "RHEL-7" + update_branch = f"{BRANCH_PREFIX}-{jira}" + mrs = [ + { + "url": "https://gitlab.com/redhat/rhel/rpms/expat/-/merge_requests/9", + "source_branch": update_branch, + "target_branch": "rhel-10.1", + "state": "opened", + }, + ] + + async def fake_run_tool(tool, available_tools=None, **kwargs): + if tool == "list_project_merge_requests": + return mrs + if tool == "close_merge_request": + raise RuntimeError("permission denied") + raise AssertionError(f"unexpected tool {tool}") + + flexmock(agent_tasks).should_receive("run_tool").replace_with(fake_run_tool) + + result = await close_stale_update_merge_requests( + jira_issue=jira, + package="expat", + dist_git_branch="rhel-10.1", + available_tools=[], + ) + assert result == [] diff --git a/ymir/tools/privileged/gateway.py b/ymir/tools/privileged/gateway.py index d9abaeafa..253d4e542 100644 --- a/ymir/tools/privileged/gateway.py +++ b/ymir/tools/privileged/gateway.py @@ -35,6 +35,7 @@ AddMergeRequestCommentTool, AddMergeRequestLabelsTool, CloneRepositoryTool, + CloseMergeRequestTool, FetchBranchTool, FetchCommitTool, FetchGitlabMrNotesTool, @@ -151,6 +152,7 @@ async def _async_main(): AddMergeRequestCommentTool(options=tool_options), AddMergeRequestLabelsTool(options=tool_options), CloneRepositoryTool(options=tool_options), + CloseMergeRequestTool(options=tool_options), FetchBranchTool(options=tool_options), FetchCommitTool(options=tool_options), ForkRepositoryTool(options=tool_options), diff --git a/ymir/tools/privileged/gitlab.py b/ymir/tools/privileged/gitlab.py index 0352d9d1d..06dea2846 100644 --- a/ymir/tools/privileged/gitlab.py +++ b/ymir/tools/privileged/gitlab.py @@ -975,6 +975,41 @@ async def _run( ) +class CloseMergeRequestToolInput(BaseModel): + merge_request_url: str = Field(description="URL of the merge request to close") + + +class CloseMergeRequestTool(Tool[CloseMergeRequestToolInput, ToolRunOptions, StringToolOutput]): + name = "close_merge_request" + timeout = 120 + description = """ + Closes an existing merge request without merging it. Closing is reversible + (the MR can be reopened) and leaves the source branch untouched. + """ + input_schema = CloseMergeRequestToolInput + + def _create_emitter(self) -> Emitter: + return Emitter.root().child( + namespace=["tool", "gitlab", self.name], + creator=self, + ) + + async def _run( + self, + tool_input: CloseMergeRequestToolInput, + options: ToolRunOptions | None, + context: RunContext, + ) -> StringToolOutput: + merge_request_url = tool_input.merge_request_url + with tool_error_context( + "Failed to close merge request", + merge_request_url=merge_request_url, + ): + mr = await _get_merge_request_from_url(merge_request_url) + await asyncio.to_thread(mr.close) + return StringToolOutput(result=f"Successfully closed merge request {merge_request_url}") + + class SetMergeRequestReviewersToolInput(BaseModel): merge_request_url: str = Field(description="URL of the merge request") reviewer_ids: list[int] = Field(description="List of GitLab user IDs to set as reviewers") diff --git a/ymir/tools/privileged/tests/unit/test_gitlab.py b/ymir/tools/privileged/tests/unit/test_gitlab.py index 5ddfaf8b7..4db927d54 100644 --- a/ymir/tools/privileged/tests/unit/test_gitlab.py +++ b/ymir/tools/privileged/tests/unit/test_gitlab.py @@ -19,6 +19,7 @@ AddMergeRequestCommentTool, AddMergeRequestLabelsTool, CloneRepositoryTool, + CloseMergeRequestTool, FetchBranchTool, FetchCommitTool, ForkRepositoryTool, @@ -665,6 +666,35 @@ async def test_add_merge_request_labels_invalid_url(): assert "Could not parse merge request URL" in str(exc_info.value.__cause__) +@pytest.mark.asyncio +async def test_close_merge_request(): + merge_request_url = "https://gitlab.com/redhat/rhel/rpms/bash/-/merge_requests/123" + + mr_mock = flexmock() + mr_mock.should_receive("close").once() + + project_mock = flexmock() + project_mock.should_receive("get_pr").and_return(mr_mock) + + flexmock(GitlabService).should_receive("get_project_from_url").with_args( + url="https://gitlab.com/redhat/rhel/rpms/bash" + ).and_return(project_mock) + + result = (await CloseMergeRequestTool().run(input={"merge_request_url": merge_request_url})).result + + assert result == f"Successfully closed merge request {merge_request_url}" + + +@pytest.mark.asyncio +async def test_close_merge_request_invalid_url(): + with pytest.raises(Exception) as exc_info: + await CloseMergeRequestTool().run( + input={"merge_request_url": "https://github.com/user/repo/pull/123"} + ) + + assert "Could not parse merge request URL" in str(exc_info.value.__cause__) + + @pytest.mark.asyncio async def test_add_merge_request_comment(): merge_request_url = "https://gitlab.com/redhat/rhel/rpms/bash/-/merge_requests/123" From cf24c8d4196c06b207fb9faa476f69fb3fac5059 Mon Sep 17 00:00:00 2001 From: Tomas Korbar Date: Thu, 8 Oct 2026 13:15:39 +0200 Subject: [PATCH 3/5] Make the build step the sole builder in the backport fix loop The fix agent used to build the package itself, which was the only build validation on the Copr path; Konflux re-validated separately in konflux_build_and_publish. This asymmetry meant the two backends took different paths after a build failure. Rework the loop so a single build step is the only builder for both backends. The fix agent now produces a corrected backport (regenerated patches plus a fresh SRPM) but never builds; routing returns to the one build step via the new _post_backport_build_step() helper, and on build failure routes back to fix_build_error carrying state.build_error. Gate the fix agent with include_build_tools=False (build_srpm/run_package_prep stay available so it can still regenerate and verify the SRPM) and centralize build-log archiving in fix_build_error. Rewrite prompt_fix_build_error.j2 to be backend-agnostic and drop all in-agent build instructions. Co-Authored-By: Claude Opus 4.8 --- ymir/agents/backport_agent.py | 88 +++++++++---------- .../backport/prompt_fix_build_error.j2 | 65 +++++++------- .../tests/unit/test_backport_build_logs.py | 25 ++++-- .../tests/unit/test_jinja2_templates.py | 31 ++----- 4 files changed, 101 insertions(+), 108 deletions(-) diff --git a/ymir/agents/backport_agent.py b/ymir/agents/backport_agent.py index a52ed0cfd..349edd63c 100644 --- a/ymir/agents/backport_agent.py +++ b/ymir/agents/backport_agent.py @@ -1309,6 +1309,18 @@ async def generate_title(jira_summary): _disable_ystream_inheritance(state, task_metadata) return "prepare_normal_backport" + def _post_backport_build_step() -> str: + """Single entry into the build step, identical for Copr and Konflux. + + A freshly staged backport (or fix) hands off to the one step that + builds: Copr builds the generated SRPM directly via run_build_agent; + Konflux first refreshes the release and stages the regenerated + patches (update_release -> stage_changes) so it can commit, push and + build the git ref in konflux_build_and_publish. The package build + happens only there, and a failed build routes back to fix_build_error. + """ + return "run_build_agent" if not (is_modular_issue or konflux) else "update_release" + async def run_backport_agent(state): response = await backport_agent.run( render_template( @@ -1361,14 +1373,22 @@ async def run_backport_agent(state): state.used_cherry_pick_workflow = False logger.info("Git am workflow detected: no upstream repo exists") - return "run_build_agent" if not (is_modular_issue or konflux) else "update_release" + return _post_backport_build_step() return "comment_in_jira" async def fix_build_error(state): - """Try to fix build errors by finding and cherry-picking prerequisite commits.""" + """Produce a fix for the failed build; the build step re-validates it. + + The fix agent itself never builds. It regenerates the patch file(s) + (and a fresh SRPM for Copr) and reports. Routing then returns to the + single build step via ``_post_backport_build_step`` — run_build_agent + for Copr, or update_release -> stage_changes -> + konflux_build_and_publish for Konflux — which performs the only build + and loops back here with the new build error if it still fails. + """ logger.info( - f"Attempting incremental fix for cherry-pick workflow " - f"(attempt {state.incremental_fix_attempts}/{max_incremental_fix_attempts})" + f"Producing incremental fix for cherry-pick workflow " + f"(cycle {state.incremental_fix_attempts + 1}/{max_incremental_fix_attempts})" ) try: @@ -1383,18 +1403,19 @@ async def fix_build_error(state): log_dir = _get_build_logs_dir(state.local_clone) log_dir.mkdir(parents=True, exist_ok=True) attempt_num = state.incremental_fix_attempts + 1 - - if state.incremental_fix_attempts > 0: - _move_build_logs( - state.local_clone, - log_dir / f"attempt-{state.incremental_fix_attempts}", - ) + # The build step is the only builder; archive the logs from the + # build that just failed before producing this cycle's fix. + _move_build_logs( + state.local_clone, + log_dir / f"attempt-{state.incremental_fix_attempts}", + ) _update_fix_attempts_log(log_dir, attempt_num, state.build_error) + state.incremental_fix_attempts += 1 fix_agent = await create_backport_agent( gateway_tools, local_tool_options, - include_build_tools=True, + include_build_tools=False, fix_version=state.fix_version, ) @@ -1412,9 +1433,6 @@ async def fix_build_error(state): upstream_patches=state.upstream_patches, build_error=state.build_error, triage_summary=state.triage_summary, - has_extract_log_snippets=any( - t.name == "extract_log_snippets" for t in gateway_tools - ), ), ), expected_output=BackportOutputSchema, @@ -1426,29 +1444,19 @@ async def fix_build_error(state): if fix_result.success: state.backport_result = fix_result state.backport_log.append(fix_result.status) - logger.info("Incremental fix succeeded with passing build") - state.incremental_fix_attempts = 0 - return "update_release" - - logger.info(f"Build still failing after fix attempt: {fix_result.error}") - state.build_error = fix_result.error + logger.info("Incremental fix produced — re-validating via the build step") + return _post_backport_build_step() + + # No candidate fix was produced, so the patches/SRPM are unchanged + # and rebuilding would fail identically. Stop and report. The build + # step itself bounds how many build/fix cycles we attempt via + # ``attempts_remaining``. + logger.info(f"Fix agent could not produce a fix: {fix_result.error}") state.backport_result = fix_result - - state.incremental_fix_attempts += 1 - if state.incremental_fix_attempts < max_incremental_fix_attempts: - logger.info( - f"Will retry incremental fix " - f"(attempt {state.incremental_fix_attempts + 1}/{max_incremental_fix_attempts})" - ) - return "fix_build_error" - logger.error( - f"Exhausted all {max_incremental_fix_attempts} incremental fix attempts, giving up" - ) state.backport_result.success = False state.backport_result.error = ( - f"Unable to fix build errors after " - f"{max_incremental_fix_attempts} incremental fix attempts. " - f"Last error: {fix_result.error}" + f"Unable to fix the build error. Last build error: {state.build_error}. " + f"Fix attempt result: {fix_result.error}" ) return "comment_in_jira" @@ -1498,12 +1506,6 @@ async def run_build_agent(state): return "comment_in_jira" state.build_error = build_result.error if state.used_cherry_pick_workflow: - upstream_repo = Path(f"{state.local_clone}-upstream") - if upstream_repo.exists(): - _move_build_logs( - state.local_clone, - _get_build_logs_dir(state.local_clone) / "attempt-0", - ) logger.info("Cherry-pick workflow was used - starting incremental fix") return "fix_build_error" logger.info("Git am workflow was used - resetting for retry") @@ -1934,12 +1936,6 @@ async def konflux_build_and_publish(state): return "comment_in_jira" state.build_error = build_result.error if state.used_cherry_pick_workflow: - upstream_repo = Path(f"{state.local_clone}-upstream") - if upstream_repo.exists(): - _move_build_logs( - state.local_clone, - _get_build_logs_dir(state.local_clone) / "attempt-0", - ) logger.info("Cherry-pick workflow was used - starting incremental fix") return "fix_build_error" logger.info("Git am workflow was used - resetting for retry") diff --git a/ymir/agents/prompts/backport/prompt_fix_build_error.j2 b/ymir/agents/prompts/backport/prompt_fix_build_error.j2 index ec0356766..cb9847a2f 100644 --- a/ymir/agents/prompts/backport/prompt_fix_build_error.j2 +++ b/ymir/agents/prompts/backport/prompt_fix_build_error.j2 @@ -18,6 +18,12 @@ The cherry-pick workflow succeeded but the build failed: {{ build_error }} +You do NOT build the package yourself. Your job is to produce a corrected +backport: fix the patch(es), regenerate them, and regenerate the SRPM. A +separate build step then rebuilds your fix. If the build still fails you will be +called again with the new build error, so make ONE focused fix attempt per +invocation. + CRITICAL CONSTRAINTS: - The upstream repository at {{ local_clone }}-upstream has all your previous work intact. DO NOT clone it again. DO NOT reset to base commit. @@ -40,7 +46,7 @@ CRITICAL CONSTRAINTS: Do NOT modify or remove existing BuildRequires/Requires entries. NEVER modify the spec for: - * Environmental issues (COPR vs RHEL builder differences like unbuffer/expect wrappers, + * Environmental issues (build-environment differences like unbuffer/expect wrappers, pipefail behavior, locale settings) — report success=false instead * Pre-existing build issues unrelated to your patch * Working around missing or too-old dependencies not yet in the buildroot @@ -49,11 +55,11 @@ CRITICAL CONSTRAINTS: IF rules do NOT explicitly allow it (or no rules exist): NEVER modify the spec file — the build worked before your patches; fix the patches instead. - The build runs in COPR, not on official RHEL builders. COPR environments may have - differences (e.g. unbuffer/expect wrappers, pipefail behavior, locale settings) that - can cause spurious failures unrelated to your patches. If the failure is caused by the - build environment rather than by your code changes, report success=false and explain - the environmental issue — do not modify the spec to work around it. + The build environment may differ from your local environment (e.g. unbuffer/expect + wrappers, pipefail behavior, locale settings) and can cause spurious failures unrelated + to your patches. If the failure is caused by the build environment rather than by your + code changes, report success=false and explain the environmental issue — do not modify + the spec to work around it. - DO NOT modify anything in {{ local_clone }} dist-git repository except: a. The backport patch file(s) you created (by regenerating them from upstream repo) @@ -63,13 +69,13 @@ CRITICAL CONSTRAINTS: Read the spec file to find the patch filenames you added — do NOT assume the name. - DO NOT commit any changes in {{ local_clone }} dist-git repository during the fix attempt. - Keep all dist-git changes (patch files and spec edits) uncommitted until the build passes. - Commit source fixes in {{ local_clone }}-upstream, staging only the intended source files. - Keep build logs and repair notes in {{ build_logs_dir }}, outside both Git repositories. + Keep all dist-git changes (patch files and spec edits) uncommitted — the build step + commits them. Commit source fixes in {{ local_clone }}-upstream, staging only the intended + source files. Keep repair notes in {{ build_logs_dir }}, outside both Git repositories. Do not copy these diagnostics into either repository or include them in generated patches. - Fix BOTH compilation errors AND test failures. NEVER skip or disable tests. -- Make ONE attempt — you will be called again if the build still fails. +- Make ONE attempt — the build step will re-invoke you if the build still fails. Before you start: Read {{ build_logs_dir }}/fix-attempts.md for a log of previous fix attempts. Do NOT repeat strategies that already failed. @@ -89,7 +95,7 @@ WORKFLOW: IMPORTANT: If the tools fail to fetch rules (returns an error, timeout, etc.), treat this as "NOT allowed" — do NOT add any spec entries, fix patches only. -1. Analyze the build error and identify what's missing (functions, types, headers, etc.) +1. Analyze the build error above and identify what's missing (functions, types, headers, etc.) 2. If the build error indicates missing dependencies that are DIRECTLY INTRODUCED by your backported patch AND rules (from step 0) explicitly allow it: @@ -98,7 +104,7 @@ WORKFLOW: - If needed during build (%build or %check) AND at runtime: add both BuildRequires and Requires - Additions only — do NOT modify or remove existing entries - See CRITICAL CONSTRAINTS and Criterion 2 for validation requirements - - Then proceed to step 6 to rebuild and verify + - Then proceed to step 6 to regenerate the SRPM 3. Otherwise, explore {{ local_clone }}-upstream to find solutions — use git log, git show, grep, and view files. The full upstream history is available. @@ -121,23 +127,17 @@ SPECIAL CONSIDERATIONS FOR TEST FAILURES: - repository_path: {{ local_clone }}-upstream - patch_file_path: the path to each patch file in {{ local_clone }}/ -6. Test the build: - - Use the `run_package_prep` tool to verify patches apply cleanly - - Use the `build_srpm` tool to generate a SRPM - - Call `build_package` with the SRPM path, dist_git_branch, and jira_issue - - If build fails: use `download_artifacts` to get logs and identify the new error -{% if has_extract_log_snippets %} - - Use `extract_log_snippets` with `log_path` pointing to `builder-live.log` - (or `root.log` if unavailable) to extract the most relevant snippets - and identify the new error -{% endif %} +6. Regenerate the SRPM so the build step can rebuild your fix: + - Use the `run_package_prep` tool to verify your patches apply cleanly + - Use the `build_srpm` tool to generate a fresh SRPM that includes your fix + Do NOT attempt to build the package yourself. The workflow's build step performs + the only build and will call you again with the new error if it still fails. 7. Append a summary to {{ build_logs_dir }}/fix-attempts.md documenting: - What you identified as the root cause - Which commits you cherry-picked or what manual edits you made - Any BuildRequires or Requires additions to the spec file (if rules allowed adding new entries), including what was added and why - - The build result (pass/fail and error if applicable) 8. Self-Review: Before reporting a result, verify your work meets all criteria below. Run `git diff HEAD -- *.spec` in {{ local_clone }} to inspect what @@ -172,12 +172,12 @@ SPECIAL CONSIDERATIONS FOR TEST FAILURES: - Appears in the build error (missing header, undefined reference, "command not found" during %build/%check, etc.) - Is used by code in your backported patch (verify by reading the patch) - - Is not a workaround for COPR environmental differences + - Is not a workaround for build-environment differences For Requires additions: - Is introduced by your backported patch (installed binary calls a new executable, dlopen()s a library, imports a Python module, etc.) - Evidence comes from the patch diff or runtime test failures - - Is not a workaround for COPR environmental differences + - Is not a workaround for build-environment differences For both BuildRequires and Requires on the same package: - Justified when the dependency is needed during build (%build or %check) AND by the installed package at runtime @@ -223,7 +223,7 @@ SPECIAL CONSIDERATIONS FOR TEST FAILURES: - Working around buildroot limitations (dependency not available or too old) If the build failure is caused by something unrelated to the patch content - (e.g. COPR environment, dependency version not yet available), report + (e.g. the build environment, dependency version not yet available), report success=false and explain the issue instead of working around it. If ALL criteria pass, report success as normal. @@ -231,14 +231,14 @@ SPECIAL CONSIDERATIONS FOR TEST FAILURES: If ANY criterion fails, attempt to fix the problem before reporting failure: - Criterion 1, 3, or 6 (bad patch content, disabled tests, or unrelated changes): return to step 4 and re-fix the issue, then regenerate patches - (step 5) and rebuild (step 6). For criterion 6, revert unrelated hunks - from {{ local_clone }}-upstream before regenerating. + (step 5) and regenerate the SRPM (step 6). For criterion 6, revert unrelated + hunks from {{ local_clone }}-upstream before regenerating. - Criterion 2 (spec modified): revert the spec with `git checkout HEAD -- *.spec` in {{ local_clone }}, verify the revert with - `git diff HEAD -- *.spec`, then rebuild (step 6). + `git diff HEAD -- *.spec`, then regenerate the SRPM (step 6). - Criterion 4 (SRPM missing): re-run `build_srpm` (step 6). - Criterion 5 (wrong patch name): rename the file to match the spec, then - rebuild (step 6). + regenerate the SRPM (step 6). After fixing, re-run this self-review before reporting. @@ -251,7 +251,8 @@ SPECIAL CONSIDERATIONS FOR TEST FAILURES: List only the failing criteria. Do not mention passing ones. -Report success=true with SRPM path if build passes. -Report success=false with the extracted error if build fails or you can't find a fix. +Report success=true with the SRPM path once your patches apply cleanly and the +SRPM has been regenerated. +Report success=false with the problem you found if you cannot produce a fix. Unpacked upstream sources are in {{ unpacked_sources }}. diff --git a/ymir/agents/tests/unit/test_backport_build_logs.py b/ymir/agents/tests/unit/test_backport_build_logs.py index 8b478eca9..aff177507 100644 --- a/ymir/agents/tests/unit/test_backport_build_logs.py +++ b/ymir/agents/tests/unit/test_backport_build_logs.py @@ -42,6 +42,8 @@ def git(repo, *args): attempts = [] async def repair(prompt, **kwargs): + # The fix agent never builds; it only produces a corrected backport. Each + # invocation follows a failed build from the dedicated build step. attempt = len(attempts) + 1 attempts.append(prompt) notes = log_dir / "fix-attempts.md" @@ -72,11 +74,12 @@ async def repair(prompt, **kwargs): (local_clone / "builder-live.log").write_text("retry build log") with notes.open("a") as stream: stream.write("\nFirst repair summary\n") + # Each repair produces a candidate fix; the build step decides success. result = BackportOutputSchema( - success=attempt == 2, + success=True, status=f"Repair {attempt}", srpm_path=local_clone / "expat.src.rpm", - error="second build failure" if attempt == 1 else None, + error=None, ) return SimpleNamespace(last_message=SimpleNamespace(text=result.model_dump_json())) @@ -87,15 +90,24 @@ async def gateway(*args, **kwargs): async def create_repair_agent(*args, **kwargs): return flexmock(run=repair) - async def failed_build(**kwargs): - return BuildOutputSchema(success=False, error="initial build failure") + # The build step is the only builder: fail twice (driving two repair cycles), + # then pass so the workflow proceeds to release bookkeeping. + builds = [] + + async def staged_build(**kwargs): + builds.append(kwargs) + if len(builds) == 1: + return BuildOutputSchema(success=False, error="initial build failure") + if len(builds) == 2: + return BuildOutputSchema(success=False, error="second build failure") + return BuildOutputSchema(success=True, error=None) flexmock(backport_agent).should_receive("mcp_tools").replace_with(gateway).once() flexmock(backport_agent).should_receive("create_log_agent").and_return(None).once() flexmock(backport_agent).should_receive("get_mock_local_tool_env").and_return(None).once() flexmock(backport_agent).should_receive("get_agent_execution_config").and_return({}).twice() flexmock(backport_agent).should_receive("create_backport_agent").replace_with(create_repair_agent).twice() - flexmock(backport_agent).should_receive("run_build").replace_with(failed_build).once() + flexmock(backport_agent).should_receive("run_build").replace_with(staged_build) monkeypatch.setenv("MCP_GATEWAY_URL", "http://gateway.invalid/sse") run_workflow = Workflow.run @@ -111,7 +123,7 @@ def start_at_build(workflow, state, options=None): error=None, ) workflow.set_start("run_build_agent") - # Exercise the real build-failure routing and both repair attempts, + # Exercise the real build-failure routing and both repair cycles, # stopping before release bookkeeping or external writes. workflow.steps["update_release"].handler = lambda _: Workflow.END workflow.steps["comment_in_jira"].handler = lambda _: Workflow.END @@ -130,6 +142,7 @@ def start_at_build(workflow, state, options=None): assert state.backport_result.success, state.backport_result.error assert len(attempts) == 2 + assert len(builds) == 3, "the build step is the sole builder and runs each cycle" git(local_clone, "add", "-A") assert git(local_clone, "diff", "--cached", "--name-only") == "fix.patch" assert not (upstream / "build-logs").exists() diff --git a/ymir/agents/tests/unit/test_jinja2_templates.py b/ymir/agents/tests/unit/test_jinja2_templates.py index e113e8daa..46978dc1f 100644 --- a/ymir/agents/tests/unit/test_jinja2_templates.py +++ b/ymir/agents/tests/unit/test_jinja2_templates.py @@ -432,7 +432,7 @@ def test_renders_source_context_and_patch_invariant(self): class TestBackportFixBuildErrorTemplate: - def test_renders_with_extract_log_snippets(self): + def test_renders_fix_build_error(self): result = render_template( "backport/prompt_fix_build_error.j2", BackportFixBuildInputSchema( @@ -444,37 +444,20 @@ def test_renders_with_extract_log_snippets(self): jira_issue="RHEL-12345", upstream_patches=["https://example.com/p1.patch"], build_error="undefined reference to 'bar'", - has_extract_log_snippets=True, ), ) assert "cherry-pick workflow succeeded but the build failed" in result assert "undefined reference" in result assert "Before you start: Read /tmp/clone-build-logs/fix-attempts.md" in result assert "/tmp/clone-upstream/build-logs" not in result - assert "extract_log_snippets" in result assert "start with" not in result - - def test_renders_without_extract_log_snippets(self): - result = render_template( - "backport/prompt_fix_build_error.j2", - BackportFixBuildInputSchema( - local_clone=Path("/tmp/clone"), - build_logs_dir=Path("/tmp/clone-build-logs"), - unpacked_sources=Path("/tmp/sources"), - package="libfoo", - dist_git_branch="c9s", - jira_issue="RHEL-12345", - upstream_patches=["https://example.com/p1.patch"], - build_error="undefined reference to 'bar'", - has_extract_log_snippets=False, - ), - ) - assert "cherry-pick workflow succeeded but the build failed" in result - assert "undefined reference" in result - assert "Before you start: Read /tmp/clone-build-logs/fix-attempts.md" in result - assert "/tmp/clone-upstream/build-logs" not in result + # The fix agent produces a corrected backport + SRPM but never builds + # the package itself — the dedicated build step is the only builder. + assert "Do NOT attempt to build the package yourself" in result + assert "build_srpm" in result + assert "build_package" not in result + assert "download_artifacts" not in result assert "extract_log_snippets" not in result - assert "get logs and identify the new error" in result class TestRebaseTemplate: From b8d0fc23b5a033f3c3392f2f00c362c0665b1e7a Mon Sep 17 00:00:00 2001 From: Tomas Korbar Date: Thu, 8 Oct 2026 14:11:44 +0200 Subject: [PATCH 4/5] Unify Copr and Konflux backport workflow The backport workflow now traverses one step graph for both build backends; the backend abstraction lives only in gateway tool selection and run_build log detection, never in the workflow routing. - Both backends commit and push before building: the single build step commit_push_and_build builds the pushed ref (Konflux) or the generated SRPM (Copr), opening the MR only after a green build. The Y-stream inherit path likewise shares one sequence ending in validate_inherited_build. Copr dry-run now creates a fork and pushes like Konflux, so gitlab.py always forks. - Squash build/fix retries: commit_push_and_build soft-resets to the pristine base before committing, so each backport lands as exactly one commit with no agent attempts in history. - Make the build-failure diagnosis prompt backend-neutral: the gateway-log (Konflux) branch no longer references Copr's builder-live.log/root.log, whose equivalents are named per pod there. - Remove the redundant _post_backport_build_step indirection that always returned "update_release". Co-Authored-By: Claude Opus 4.8 --- ymir/agents/backport_agent.py | 218 +++++++----------- ymir/agents/build_agent.py | 2 +- ymir/agents/prompts/build/instructions.j2 | 13 +- .../tests/unit/test_backport_build_logs.py | 137 ++++++++++- .../tests/unit/test_jinja2_templates.py | 8 +- .../agents/tests/unit/test_konflux_reorder.py | 6 +- ymir/tools/privileged/gitlab.py | 17 -- .../privileged/tests/unit/test_gitlab.py | 12 +- 8 files changed, 241 insertions(+), 172 deletions(-) diff --git a/ymir/agents/backport_agent.py b/ymir/agents/backport_agent.py index 349edd63c..10354f32c 100644 --- a/ymir/agents/backport_agent.py +++ b/ymir/agents/backport_agent.py @@ -27,7 +27,7 @@ from specfile import Specfile import ymir.agents.tasks as tasks -from ymir.agents.build_agent import is_konflux_backend, run_build +from ymir.agents.build_agent import run_build from ymir.agents.constants import ( I_AM_YMIR, ZSTREAM_TARGET_LABEL, @@ -647,6 +647,10 @@ class BackportState(PackageUpdateState): attempts_remaining: int = Field(default=10) used_cherry_pick_workflow: bool = Field(default=False) incremental_fix_attempts: int = Field(default=0) + # Pristine dist-git HEAD captured right after clone, before any backport work. + # The build step soft-resets to it so every build/fix cycle squashes into a + # single published commit (no agent attempts in history). + backport_base_head: str | None = Field(default=None) fix_version: str | None = Field(default=None) shipped_zstream_candidates: list[ShippedZStreamCandidate] = Field(default_factory=list) inherit_cleanup_retried: bool = Field(default=False) @@ -833,9 +837,6 @@ async def run_workflow( if max_incremental_fix_attempts is None: max_incremental_fix_attempts = max_build_attempts workspace_id = workspace_id or uuid4() - # Konflux builds from a pushed git ref, so it reorders commit/push to - # happen BEFORE the build; Copr (default) is unaffected. - konflux = is_konflux_backend() local_tool_options: dict[str, Any] = {"working_directory": None} if mock_env := get_mock_local_tool_env(jira_issue): @@ -968,6 +969,7 @@ async def fork_and_prepare_dist_git(state): cwd=state.local_clone, ) state.inherit_saved_head = state.inherit_saved_head.strip() + state.backport_base_head = state.inherit_saved_head if not state.inheritance_disabled and _can_attempt_ystream_inheritance(state): state.inherit_candidate = same_major_candidate( state.shipped_zstream_candidates, @@ -1285,7 +1287,9 @@ async def generate_title(jira_summary): error=None, ) state.inherit_build_attempts = max_build_attempts - return "stage_changes" if konflux else "run_inherit_build_agent" + # Both backends stage, commit and push the inherited commit, then + # validate it with a single build step (validate_inherited_build). + return "stage_changes" except AlreadyInheritedError as error: logger.error("Y-stream inheritance invariant failed: %s", error) state.retry_mode = BackportRetryMode.NONE @@ -1309,18 +1313,6 @@ async def generate_title(jira_summary): _disable_ystream_inheritance(state, task_metadata) return "prepare_normal_backport" - def _post_backport_build_step() -> str: - """Single entry into the build step, identical for Copr and Konflux. - - A freshly staged backport (or fix) hands off to the one step that - builds: Copr builds the generated SRPM directly via run_build_agent; - Konflux first refreshes the release and stages the regenerated - patches (update_release -> stage_changes) so it can commit, push and - build the git ref in konflux_build_and_publish. The package build - happens only there, and a failed build routes back to fix_build_error. - """ - return "run_build_agent" if not (is_modular_issue or konflux) else "update_release" - async def run_backport_agent(state): response = await backport_agent.run( render_template( @@ -1373,18 +1365,22 @@ async def run_backport_agent(state): state.used_cherry_pick_workflow = False logger.info("Git am workflow detected: no upstream repo exists") - return _post_backport_build_step() + # A freshly staged backport refreshes the release and stages the + # regenerated patches (update_release -> stage_changes), then the + # single build step (commit_push_and_build) commits, pushes and + # builds for both backends. Modular issues skip the build in + # stage_changes. A failed build routes back to fix_build_error. + return "update_release" return "comment_in_jira" async def fix_build_error(state): """Produce a fix for the failed build; the build step re-validates it. The fix agent itself never builds. It regenerates the patch file(s) - (and a fresh SRPM for Copr) and reports. Routing then returns to the - single build step via ``_post_backport_build_step`` — run_build_agent - for Copr, or update_release -> stage_changes -> - konflux_build_and_publish for Konflux — which performs the only build - and loops back here with the new build error if it still fails. + (and a fresh SRPM) and reports. Routing then returns to the single + build step (update_release -> stage_changes -> commit_push_and_build, + identical for both backends), which performs the only build and loops + back here with the new build error if it still fails. """ logger.info( f"Producing incremental fix for cherry-pick workflow " @@ -1445,7 +1441,7 @@ async def fix_build_error(state): state.backport_result = fix_result state.backport_log.append(fix_result.status) logger.info("Incremental fix produced — re-validating via the build step") - return _post_backport_build_step() + return "update_release" # No candidate fix was produced, so the patches/SRPM are unchanged # and rebuilding would fail identically. Stop and report. The build @@ -1466,75 +1462,43 @@ async def fix_build_error(state): state.backport_result.error = f"Exception during incremental fix: {e!s}" return "comment_in_jira" - async def run_build_agent(state): - if not state.backport_result or not state.backport_result.srpm_path: - logger.error("Cannot run build agent: no valid backport result or SRPM path") - state.backport_result = state.backport_result or BackportOutputSchema( - success=False, - srpm_path=None, - status="", - error="No SRPM generated by backport agent", - ) - return "comment_in_jira" + async def validate_inherited_build(state): + """Validate the pushed inherited commit before opening the MR. + Unified across backends: the inherited commit is already staged, + committed and pushed, so Copr builds the generated SRPM while Konflux + builds the pushed git ref -- run_build dispatches on BUILD_BACKEND. A + green build opens the MR; repeated failures fall back to a normal + backport. + """ build_result = await run_build( build_input=BuildInputSchema( srpm_path=state.backport_result.srpm_path, dist_git_branch=state.dist_git_branch, jira_issue=state.jira_issue, + git_url=state.fork_url, + revision=state.inherit_local_commit, + package_name=state.package, + target_branch=state.dist_git_branch, ), available_tools=gateway_tools, local_tool_options=local_tool_options, ) - if build_result.success: - state.incremental_fix_attempts = 0 - return "update_release" - if build_result.is_timeout: - logger.info(f"Build timed out for {state.jira_issue}, proceeding") - return "update_release" - if build_result.is_infra_error: - logger.error(f"Copr infrastructure error for {state.jira_issue}: {build_result.error}") - state.backport_result.success = False - state.backport_result.error = build_result.error or "Copr API infrastructure error" - return "comment_in_jira" - state.attempts_remaining -= 1 - if state.attempts_remaining <= 0: - state.backport_result.success = False - state.backport_result.error = ( - f"Unable to successfully build the package in {max_build_attempts} attempts" - ) - return "comment_in_jira" - state.build_error = build_result.error - if state.used_cherry_pick_workflow: - logger.info("Cherry-pick workflow was used - starting incremental fix") - return "fix_build_error" - logger.info("Git am workflow was used - resetting for retry") - return "fork_and_prepare_dist_git" - - async def run_inherit_build_agent(state): - """Require a successful Copr validation before publishing inheritance.""" - build_result = await run_build( - build_input=BuildInputSchema( - srpm_path=state.backport_result.srpm_path, - dist_git_branch=state.dist_git_branch, - jira_issue=state.jira_issue, - ), - available_tools=gateway_tools, - local_tool_options=local_tool_options, - ) - if build_result.success: - return "stage_changes" + if build_result.success or build_result.is_timeout: + if dry_run: + return "submit_consolidation_job" + return "open_inherited_mr" state.inherit_build_attempts -= 1 if state.inherit_build_attempts > 0: logger.warning( - "Inherited Copr validation failed; retrying (%d attempts left): %s", + "Inherited build validation failed; retrying (%d attempts left): %s", state.inherit_build_attempts, build_result.error, ) - return "run_inherit_build_agent" + return "validate_inherited_build" - logger.info("Inherited Copr validation did not pass: %s", build_result.error) + logger.info("Inherited build validation did not pass: %s", build_result.error) if not await cleanup_inherit_attempt(state): return handle_inherit_cleanup_failure(state) _disable_ystream_inheritance(state, task_metadata) @@ -1595,9 +1559,12 @@ async def stage_changes(state): if state.inherit_change: return "commit_inherited_change" if state.log_result: - if konflux and not is_modular_issue: - return "konflux_build_and_publish" - return "commit_push_and_open_mr" + # Modular issues are published without a build; everything else + # commits, pushes and builds via the single commit_push_and_build + # step (identical for Copr and Konflux). + if is_modular_issue: + return "commit_push_and_open_mr" + return "commit_push_and_build" return "run_log_agent" async def run_log_agent(state): @@ -1722,10 +1689,9 @@ async def commit_inherited_change(state): return handle_inherit_cleanup_failure(state) _disable_ystream_inheritance(state, task_metadata) return "prepare_normal_backport" - # Konflux must push the inherited commit so it can build from the ref, - # even in dry-run (the MR itself is still skipped later). - if dry_run and not konflux: - return "submit_consolidation_job" + # Both backends push the inherited commit before validating the + # build: Konflux builds from the pushed ref and Copr follows the same + # path. Even in dry-run the push happens; only the MR is skipped later. return "push_inherited_change" async def push_inherited_change(state): @@ -1764,7 +1730,7 @@ async def push_inherited_change(state): f"{reconcile_error}" ) return "comment_in_jira" - return "konflux_inherit_build" if konflux else "open_inherited_mr" + return "validate_inherited_build" async def open_inherited_mr(state): try: @@ -1859,21 +1825,42 @@ async def commit_push_and_open_mr(state): return "comment_in_jira" return "submit_consolidation_job" - async def konflux_build_and_publish(state): - """Konflux builds from a pushed ref: commit + push, build, then open the MR. + async def commit_push_and_build(state): + """Commit, push and build the backport, then open the MR on success. - Only reached for non-modular issues under BUILD_BACKEND=konflux. Mirrors - run_build_agent's failure/retry handling, but the fork push happens before - the build and the MR is opened only after a green build. + The single build step for non-modular issues on both backends: the + fork branch is pushed first so Konflux can build the ref, while Copr + builds the generated SRPM (run_build dispatches on BUILD_BACKEND and + ignores the ref fields). The MR is opened only after a green build; a + failed build loops back to fix_build_error (cherry-pick) or restarts + the backport (git am). """ + if not state.backport_result or not state.backport_result.srpm_path: + logger.error("Cannot build: no valid backport result or SRPM path") + state.backport_result = state.backport_result or BackportOutputSchema( + success=False, + srpm_path=None, + status="", + error="No SRPM generated by backport agent", + ) + return "comment_in_jira" try: commit_message, mr_description, labels = await _compose_backport_commit_and_mr(state) + # Collapse every build/fix cycle (and any cherry-pick commits the + # backport agent left) into one commit: soft-reset to the pristine + # base so the staged tree commits as a single backport commit. + # Force-push then overwrites the previous cycle's ref. + if state.backport_base_head: + await check_subprocess( + ["git", "reset", "--soft", state.backport_base_head], + cwd=state.local_clone, + ) revision = await tasks.commit_changes(state.local_clone, commit_message) await tasks.push_changes( state.local_clone, state.fork_url, state.update_branch, gateway_tools ) except Exception as e: - logger.warning(f"Error committing/pushing before Konflux build: {e}") + logger.warning(f"Error committing/pushing before build: {e}") state.merge_request_url = None state.backport_result.success = False state.backport_result.error = f"Could not commit and push for build: {e}" @@ -1894,11 +1881,11 @@ async def konflux_build_and_publish(state): ) if build_result.success or build_result.is_timeout: if build_result.is_timeout: - logger.info(f"Konflux build timed out for {state.jira_issue}, proceeding") + logger.info(f"Build timed out for {state.jira_issue}, proceeding") state.incremental_fix_attempts = 0 if dry_run: - # The ref was pushed so Konflux could build; skip MR creation in - # dry-run, mirroring the Copr commit_only path. + # The branch was pushed so the build could run; skip MR + # creation in dry-run. state.merge_request_url = None state.merge_request_newly_created = False return "submit_consolidation_job" @@ -1917,15 +1904,15 @@ async def konflux_build_and_publish(state): package=state.package, ) except Exception as e: - logger.warning(f"Konflux build passed but MR creation failed: {e}") + logger.warning(f"Build passed but MR creation failed: {e}") state.merge_request_url = None state.backport_result.success = False state.backport_result.error = f"Could not open MR after build: {e}" return "submit_consolidation_job" if build_result.is_infra_error: - logger.error(f"Konflux infrastructure error for {state.jira_issue}: {build_result.error}") + logger.error(f"Build infrastructure error for {state.jira_issue}: {build_result.error}") state.backport_result.success = False - state.backport_result.error = build_result.error or "Konflux infrastructure error" + state.backport_result.error = build_result.error or "Build infrastructure error" return "comment_in_jira" state.attempts_remaining -= 1 if state.attempts_remaining <= 0: @@ -1941,41 +1928,6 @@ async def konflux_build_and_publish(state): logger.info("Git am workflow was used - resetting for retry") return "fork_and_prepare_dist_git" - async def konflux_inherit_build(state): - """Validate an already-pushed inherited commit via Konflux before the MR.""" - build_result = await run_build( - build_input=BuildInputSchema( - srpm_path=state.backport_result.srpm_path, - dist_git_branch=state.dist_git_branch, - jira_issue=state.jira_issue, - git_url=state.fork_url, - revision=state.inherit_local_commit, - package_name=state.package, - target_branch=state.dist_git_branch, - ), - available_tools=gateway_tools, - local_tool_options=local_tool_options, - ) - if build_result.success or build_result.is_timeout: - if dry_run: - return "submit_consolidation_job" - return "open_inherited_mr" - - state.inherit_build_attempts -= 1 - if state.inherit_build_attempts > 0: - logger.warning( - "Inherited Konflux validation failed; retrying (%d attempts left): %s", - state.inherit_build_attempts, - build_result.error, - ) - return "konflux_inherit_build" - - logger.info("Inherited Konflux validation did not pass: %s", build_result.error) - if not await cleanup_inherit_attempt(state): - return handle_inherit_cleanup_failure(state) - _disable_ystream_inheritance(state, task_metadata) - return "prepare_normal_backport" - async def submit_consolidation_job(state): if ( not state.merge_request_url @@ -2079,8 +2031,7 @@ async def comment_in_jira(state): workflow.add_step("evaluate_inherit_source", evaluate_inherit_source) workflow.add_step("run_backport_agent", run_backport_agent) workflow.add_step("fix_build_error", fix_build_error) - workflow.add_step("run_build_agent", run_build_agent) - workflow.add_step("run_inherit_build_agent", run_inherit_build_agent) + workflow.add_step("validate_inherited_build", validate_inherited_build) workflow.add_step("update_release", update_release) workflow.add_step("stage_changes", stage_changes) workflow.add_step("run_log_agent", run_log_agent) @@ -2088,8 +2039,7 @@ async def comment_in_jira(state): workflow.add_step("push_inherited_change", push_inherited_change) workflow.add_step("open_inherited_mr", open_inherited_mr) workflow.add_step("commit_push_and_open_mr", commit_push_and_open_mr) - workflow.add_step("konflux_build_and_publish", konflux_build_and_publish) - workflow.add_step("konflux_inherit_build", konflux_inherit_build) + workflow.add_step("commit_push_and_build", commit_push_and_build) workflow.add_step("submit_consolidation_job", submit_consolidation_job) workflow.add_step("comment_in_jira", comment_in_jira) diff --git a/ymir/agents/build_agent.py b/ymir/agents/build_agent.py index 6e44b8144..ebcca4c04 100644 --- a/ymir/agents/build_agent.py +++ b/ymir/agents/build_agent.py @@ -97,7 +97,7 @@ async def execute_build(state: BuildState) -> str: state.output = BuildOutputSchema( success=False, - error=result.error_message or "Copr build failed without an error message", + error=result.error_message or "Build failed without an error message", is_timeout=result.is_timeout, ) if result.is_timeout: diff --git a/ymir/agents/prompts/build/instructions.j2 b/ymir/agents/prompts/build/instructions.j2 index 076f36284..ced69b7aa 100644 --- a/ymir/agents/prompts/build/instructions.j2 +++ b/ymir/agents/prompts/build/instructions.j2 @@ -5,10 +5,11 @@ Do not submit or retry a build, regenerate an SRPM, or change package sources or Your only task is to explain this existing failure using the supplied result and logs. {% if has_extract_log_snippets %} -Download the *.log.gz files from the supplied `artifacts_urls` using the +Download every log referenced by the supplied `artifacts_urls` using the `download_artifacts` tool to a temporary directory. -Use the `extract_log_snippets` tool with `log_path` pointing to the location of -`builder-live.log` and try to identify the build failure. The downloaded files are only +Inspect each downloaded log file with the `extract_log_snippets` tool, passing its +path as `log_path`; start with the main build log and work through the rest until you +identify the build failure. The downloaded files are only accessible through `extract_log_snippets` — never use `view`, `search_text`, or shell commands to read them, even after a successful `extract_log_snippets` call; those tools run in a different sandbox and will always fail with "No such file or directory" on @@ -18,12 +19,12 @@ Retrieve the *.log.gz files directly from the supplied `artifacts_urls` into a temporary directory in your local sandbox using shell commands (for example, curl or wget), then decompress and inspect them with the local tools. Start with `builder-live.log` and try to identify the build failure. +If the failure is not found there, try the same with `root.log`. {% endif %} If there are no accessible logs, return the supplied error message and explain that a detailed diagnosis was unavailable. -If the failure is not found, try the same with `root.log`. Summarize the findings -and return them as `error`. Build status and retry decisions belong to the workflow, -not to this analysis. +Summarize the findings and return them as `error`. Build status and retry decisions +belong to the workflow, not to this analysis. General instructions: diff --git a/ymir/agents/tests/unit/test_backport_build_logs.py b/ymir/agents/tests/unit/test_backport_build_logs.py index aff177507..2537889a8 100644 --- a/ymir/agents/tests/unit/test_backport_build_logs.py +++ b/ymir/agents/tests/unit/test_backport_build_logs.py @@ -9,7 +9,8 @@ from flexmock import flexmock from ymir.agents import backport_agent -from ymir.common.models import BackportOutputSchema, BuildOutputSchema +from ymir.agents import tasks as agent_tasks +from ymir.common.models import BackportOutputSchema, BuildOutputSchema, LogOutputSchema from ymir.tools.unprivileged.wicked_git import GitPatchCreationTool @@ -108,6 +109,21 @@ async def staged_build(**kwargs): flexmock(backport_agent).should_receive("get_agent_execution_config").and_return({}).twice() flexmock(backport_agent).should_receive("create_backport_agent").replace_with(create_repair_agent).twice() flexmock(backport_agent).should_receive("run_build").replace_with(staged_build) + + # The single build step commits + pushes before building; keep those out of + # the real checkout so the Git assertions below see only the repair patch. + async def _commit(_clone, _message, *a, **k): + return "a" * 40 + + async def _push(*a, **k): + return None + + async def _no_zstream_label(*a, **k): + return False + + flexmock(agent_tasks).should_receive("commit_changes").replace_with(_commit) + flexmock(agent_tasks).should_receive("push_changes").replace_with(_push) + flexmock(agent_tasks).should_receive("needs_zstream_target_label").replace_with(_no_zstream_label) monkeypatch.setenv("MCP_GATEWAY_URL", "http://gateway.invalid/sse") run_workflow = Workflow.run @@ -116,16 +132,23 @@ def start_at_build(workflow, state, options=None): state.local_clone = local_clone state.unpacked_sources = upstream state.used_cherry_pick_workflow = True + state.fork_url = "https://fork.example/repo.git" + state.update_branch = "ymir-RHEL-123" + state.backport_log = ["Backported the fix"] + state.log_result = LogOutputSchema(title="Fix the thing", description="Backport of the fix") state.backport_result = BackportOutputSchema( success=True, status="Backported", srpm_path=local_clone / "expat.src.rpm", error=None, ) - workflow.set_start("run_build_agent") - # Exercise the real build-failure routing and both repair cycles, - # stopping before release bookkeeping or external writes. - workflow.steps["update_release"].handler = lambda _: Workflow.END + # The build step is the sole builder for both backends; a failed build + # routes to fix_build_error and loops back through update_release. Short + # update_release straight back to the build so the loop stays focused on + # the repair/archiving behaviour under test. + workflow.set_start("commit_push_and_build") + workflow.steps["update_release"].handler = lambda _: "commit_push_and_build" + workflow.steps["submit_consolidation_job"].handler = lambda _: Workflow.END workflow.steps["comment_in_jira"].handler = lambda _: Workflow.END return run_workflow(workflow, state, options) @@ -146,3 +169,107 @@ def start_at_build(workflow, state, options=None): git(local_clone, "add", "-A") assert git(local_clone, "diff", "--cached", "--name-only") == "fix.patch" assert not (upstream / "build-logs").exists() + + +@pytest.mark.asyncio +async def test_retry_squashes_into_single_commit(monkeypatch, tmp_path): + """Each backport yields one commit; agent build/fix attempts never reach history.""" + local_clone = tmp_path / "expat" + + def git(repo, *args): + return subprocess.run( + ["git", *args], cwd=repo, check=True, capture_output=True, text=True + ).stdout.strip() + + local_clone.mkdir() + git(local_clone, "init", "-q") + git(local_clone, "config", "user.name", "Ymir Tests") + git(local_clone, "config", "user.email", "ymir-tests@example.com") + (local_clone / "expat.spec").write_text("Release: 1\n") + git(local_clone, "add", "-A") + git(local_clone, "commit", "-m", "Import package") + base = git(local_clone, "rev-parse", "HEAD") + + # First backport candidate already staged on top of the pristine base. + (local_clone / "expat.spec").write_text("Release: 2\n") + (local_clone / "0001-fix.patch").write_text("patch v1\n") + git(local_clone, "add", "-A") + + @asynccontextmanager + async def gateway(*args, **kwargs): + yield [] + + # Build fails once (driving one fix cycle), then passes. + builds = [] + + async def staged_build(**kwargs): + builds.append(kwargs) + return BuildOutputSchema(success=len(builds) >= 2, error=None if len(builds) >= 2 else "boom") + + # The fix agent produces a fresh candidate: it rewrites the patch and stages it, + # the same way the real fix_build_error leaves a new staged tree behind. + fixes = [] + + def fake_fix(_state): + fixes.append(True) + (local_clone / "expat.spec").write_text("Release: 3\n") + (local_clone / "0001-fix.patch").write_text("patch v2\n") + git(local_clone, "add", "-A") + return "update_release" + + flexmock(backport_agent).should_receive("mcp_tools").replace_with(gateway) + flexmock(backport_agent).should_receive("create_log_agent").and_return(None) + flexmock(backport_agent).should_receive("get_mock_local_tool_env").and_return(None) + flexmock(backport_agent).should_receive("get_agent_execution_config").and_return({}) + flexmock(backport_agent).should_receive("run_build").replace_with(staged_build) + + async def _push(*a, **k): + return None + + async def _no_zstream_label(*a, **k): + return False + + flexmock(agent_tasks).should_receive("push_changes").replace_with(_push) + flexmock(agent_tasks).should_receive("needs_zstream_target_label").replace_with(_no_zstream_label) + monkeypatch.setenv("MCP_GATEWAY_URL", "http://gateway.invalid/sse") + + run_workflow = Workflow.run + + def start_at_build(workflow, state, options=None): + state.local_clone = local_clone + state.backport_base_head = base + state.used_cherry_pick_workflow = True + state.fork_url = "https://fork.example/repo.git" + state.update_branch = "ymir-RHEL-123" + state.backport_log = ["Backported the fix"] + state.log_result = LogOutputSchema(title="Fix the thing", description="Backport of the fix") + state.backport_result = BackportOutputSchema( + success=True, status="Backported", srpm_path=local_clone / "expat.src.rpm", error=None + ) + workflow.set_start("commit_push_and_build") + workflow.steps["fix_build_error"].handler = fake_fix + workflow.steps["update_release"].handler = lambda _: "commit_push_and_build" + workflow.steps["submit_consolidation_job"].handler = lambda _: Workflow.END + workflow.steps["comment_in_jira"].handler = lambda _: Workflow.END + return run_workflow(workflow, state, options) + + monkeypatch.setattr(Workflow, "run", start_at_build) + state = await backport_agent.run_workflow( + package="expat", + dist_git_branch="c10s", + upstream_patches=["0001-fix.patch"], + jira_issue="RHEL-123", + cve_id=None, + dry_run=True, + backport_agent_factory=lambda *_: None, + ) + + assert state.backport_result.success, state.backport_result.error + assert len(fixes) == 1 + assert len(builds) == 2 + # Exactly one commit sits on top of the pristine base, and it carries the + # final candidate - the intermediate build/fix attempt is gone from history. + assert git(local_clone, "rev-list", "--count", f"{base}..HEAD") == "1" + assert git(local_clone, "rev-parse", "HEAD~1") == base + assert git(local_clone, "show", "HEAD:0001-fix.patch") == "patch v2" + assert git(local_clone, "show", "HEAD:expat.spec") == "Release: 3" diff --git a/ymir/agents/tests/unit/test_jinja2_templates.py b/ymir/agents/tests/unit/test_jinja2_templates.py index 46978dc1f..4109f3e63 100644 --- a/ymir/agents/tests/unit/test_jinja2_templates.py +++ b/ymir/agents/tests/unit/test_jinja2_templates.py @@ -140,7 +140,11 @@ def test_loads_with_extract_log_snippets(self): ) assert "expert on analyzing package build failures" in result assert "Do not submit or retry a build" in result - assert "builder-live.log" in result + # The gateway-log (Konflux) branch must stay backend-neutral: Konflux logs + # are named after pods/containers, not Copr's builder-live.log/root.log. + assert "builder-live.log" not in result + assert "root.log" not in result + assert "main build log" in result assert "extract_log_snippets" in result assert "Start with" not in result @@ -157,6 +161,8 @@ def test_loads_without_extract_log_snippets(self): assert "local sandbox" in result assert "artifacts_urls" in result assert "Start with" in result + assert "builder-live.log" in result + assert "root.log" in result class TestLogInstructions: diff --git a/ymir/agents/tests/unit/test_konflux_reorder.py b/ymir/agents/tests/unit/test_konflux_reorder.py index 71351362a..6e6d9ae6c 100644 --- a/ymir/agents/tests/unit/test_konflux_reorder.py +++ b/ymir/agents/tests/unit/test_konflux_reorder.py @@ -87,7 +87,7 @@ async def _open_mr(**kwargs): def start_at(workflow, state, options=None): _base_state(state, local_clone=local_clone) - workflow.set_start("konflux_build_and_publish") + workflow.set_start("commit_push_and_build") workflow.steps["submit_consolidation_job"].handler = lambda _: Workflow.END workflow.steps["comment_in_jira"].handler = lambda _: Workflow.END return run_workflow(workflow, state, options) @@ -117,7 +117,7 @@ def start_at(workflow, state, options=None): @pytest.mark.parametrize("dry_run", [False, True]) @pytest.mark.asyncio -async def test_konflux_inherit_build_validates_pushed_commit(monkeypatch, tmp_path, dry_run): +async def test_inherit_build_validates_pushed_commit(monkeypatch, tmp_path, dry_run): monkeypatch.setenv("BUILD_BACKEND", "konflux") local_clone = tmp_path / "expat" local_clone.mkdir() @@ -139,7 +139,7 @@ def start_at(workflow, state, options=None): _base_state(state, local_clone=local_clone) state.inherit_local_commit = "b" * 40 state.inherit_build_attempts = 3 - workflow.set_start("konflux_inherit_build") + workflow.set_start("validate_inherited_build") workflow.steps["open_inherited_mr"].handler = lambda _: (calls.append("open_mr"), Workflow.END)[1] workflow.steps["submit_consolidation_job"].handler = lambda _: ( calls.append("submit"), diff --git a/ymir/tools/privileged/gitlab.py b/ymir/tools/privileged/gitlab.py index 06dea2846..5208c0737 100644 --- a/ymir/tools/privileged/gitlab.py +++ b/ymir/tools/privileged/gitlab.py @@ -107,16 +107,6 @@ async def _run_git_cmd( _FORK_READY_TIMEOUT_SEC = 110 # leave margin under fork_repository tool timeout -def _is_konflux_backend() -> bool: - """True when builds run on Konflux (build-from-git-ref) rather than Copr. - - Konflux builds from a pushed fork ref, so the fork must exist and be pushed - to even under ``DRY_RUN`` — only the MR/Jira writes are suppressed. Copr, by - contrast, builds from a local SRPM and never needs a dry-run fork. - """ - return os.getenv("BUILD_BACKEND", "copr").strip().lower() == "konflux" - - def _fork_api_project(fork: GitlabProject): """Return a python-gitlab Project object suitable for import_status polling. @@ -437,13 +427,6 @@ def get_fork(): if fork := await asyncio.to_thread(get_fork): return StringToolOutput(result=fork.get_git_urls()["git"]) - # Konflux must build from a real, pushed fork ref, so the fork is - # created even in DRY_RUN (only MR/Jira writes are suppressed). Copr - # builds from a local SRPM and needs no dry-run fork. - if os.getenv("DRY_RUN", "False").lower() == "true" and not _is_konflux_backend(): - logger.info("DRY_RUN is set, skipping fork creation — returning original repo URL") - return StringToolOutput(result=project.get_git_urls()["git"]) - def create_fork(): prefix = "_".join(ns.replace("centos-stream", "centos") for ns in namespace[1:]) fork_name = (f"{prefix}_" if prefix else "") + project.gitlab_repo.name diff --git a/ymir/tools/privileged/tests/unit/test_gitlab.py b/ymir/tools/privileged/tests/unit/test_gitlab.py index 4db927d54..5c7450371 100644 --- a/ymir/tools/privileged/tests/unit/test_gitlab.py +++ b/ymir/tools/privileged/tests/unit/test_gitlab.py @@ -151,24 +151,26 @@ def _mock_original_project(*, repository, package, bot_username, fork, expected_ @pytest.mark.asyncio -async def test_fork_repository_copr_dry_run_returns_original(monkeypatch): - """Copr builds from a local SRPM, so DRY_RUN skips fork creation and echoes the origin.""" +async def test_fork_repository_copr_dry_run_creates_fork(monkeypatch): + """Both backends build after commit+push, so DRY_RUN creates the fork for Copr too.""" monkeypatch.setenv("DRY_RUN", "true") monkeypatch.delenv("BUILD_BACKEND", raising=False) # default copr monkeypatch.setenv("FORK_NAMESPACE", "redhat/rhel/bot-branches") repository = "https://gitlab.com/redhat/centos-stream/rpms/bash" package = "bash" + clone_url = "https://gitlab.com/redhat/rhel/bot-branches/centos_rpms_bash.git" fork = _fork_project_mock( target_namespace="redhat/rhel/bot-branches", fork_name="centos_rpms_bash", - clone_url="https://gitlab.com/redhat/rhel/bot-branches/centos_rpms_bash.git", + clone_url=clone_url, ) + flexmock(GitlabProject).new_instances(fork) expected_data = { "name": "centos_rpms_bash", "path": "centos_rpms_bash", "namespace": "redhat/rhel/bot-branches", } - original = _mock_original_project( + _mock_original_project( repository=repository, package=package, bot_username="test-bot", @@ -176,7 +178,7 @@ async def test_fork_repository_copr_dry_run_returns_original(monkeypatch): expected_data=expected_data, ) result = (await ForkRepositoryTool().run(input={"repository": repository})).result - assert result == original + assert result == clone_url @pytest.mark.asyncio From 5a6f8c45823dad113ac030ca1328c9f3cd6ba5b3 Mon Sep 17 00:00:00 2001 From: Tomas Korbar Date: Thu, 8 Oct 2026 15:18:31 +0200 Subject: [PATCH 5/5] Tolerate Konflux-only fields in Copr build tool input The Copr/Konflux workflow unification routes both backends through build_agent.run_build, which dumps the full shared BuildInputSchema to the build_package tool. The Copr tool therefore now receives the Konflux-only fields (git_url, revision, package_name, target_branch). BuildPackageToolInput lacked extra="allow", so beeai's MCP client - which rebuilds the input model from the advertised JSON schema with extra="forbid" unless additionalProperties is truthy - rejected those fields client-side as "Tool input validation error" before the build reached the gateway. Add ConfigDict(extra="allow") (mirroring the Konflux tool) so the schema advertises additionalProperties: true. Co-Authored-By: Claude Opus 4.8 --- ymir/tools/privileged/copr.py | 17 ++++++++++- ymir/tools/privileged/tests/unit/test_copr.py | 30 +++++++++++++++++++ 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/ymir/tools/privileged/copr.py b/ymir/tools/privileged/copr.py index 52e5a15ad..67fbe9060 100644 --- a/ymir/tools/privileged/copr.py +++ b/ymir/tools/privileged/copr.py @@ -19,7 +19,7 @@ ) from copr.v3 import BuildProxy, ProjectChrootProxy, ProjectProxy from copr.v3.exceptions import CoprException -from pydantic import BaseModel, Field +from pydantic import BaseModel, ConfigDict, Field from ymir.common import load_rhel_config from ymir.common.base_utils import init_kerberos_ticket @@ -85,6 +85,21 @@ async def _copr_api_call(func, *args, **kwargs): class BuildPackageToolInput(BaseModel): + """Copr build inputs. + + ``extra='allow'`` so the Konflux-only fields in the shared BuildInputSchema + (git_url, revision, package_name, target_branch) are tolerated when + build_agent dumps the whole model for either backend. This must be ``allow`` + rather than ``ignore``: only ``allow`` makes Pydantic emit + ``additionalProperties: true`` in the advertised JSON schema, and beeai's MCP + client rebuilds the tool's input model from that schema with ``extra='forbid'`` + unless ``additionalProperties`` is truthy. With ``ignore`` the extra Konflux + fields would be rejected client-side as "Tool input validation error" before + the build ever reaches the gateway. + """ + + model_config = ConfigDict(extra="allow") + srpm_path: AbsolutePath = Field(description="Absolute path to SRPM (*.src.rpm) file to build") dist_git_branch: str = Field(description="dist-git branch") jira_issue: str = Field(description="Jira issue key (e.g. RHEL-12345)") diff --git a/ymir/tools/privileged/tests/unit/test_copr.py b/ymir/tools/privileged/tests/unit/test_copr.py index f42276cda..27c978462 100644 --- a/ymir/tools/privileged/tests/unit/test_copr.py +++ b/ymir/tools/privileged/tests/unit/test_copr.py @@ -17,6 +17,7 @@ COPR_BUILD_TIMEOUT, COPR_PROJECT_LIFETIME, BuildPackageTool, + BuildPackageToolInput, DownloadArtifactsTool, _copr_api_call, _copr_error_detail, @@ -297,3 +298,32 @@ def non_copr_error(): with pytest.raises(ValueError, match="not a copr error"): await _copr_api_call(non_copr_error) assert call_count == 1 + + +def test_build_input_tolerates_konflux_only_fields(): + # build_agent.run_build dumps the whole shared BuildInputSchema for every + # backend, so the Copr tool receives the Konflux-only fields (git_url, + # revision, package_name, target_branch). It must accept that payload rather + # than reject the extra keys. + model = BuildPackageToolInput.model_validate( + { + "srpm_path": "/x.src.rpm", + "dist_git_branch": "rhel-10.2", + "jira_issue": "RHEL-12345", + "git_url": "https://fork.example/repo.git", + "revision": "a" * 40, + "package_name": "expat", + "target_branch": "rhel-10.2", + } + ) + assert model.srpm_path == Path("/x.src.rpm") + assert model.dist_git_branch == "rhel-10.2" + + +def test_input_schema_advertises_additional_properties(): + # beeai's MCP client rebuilds the tool's input model from the advertised JSON + # schema with extra="forbid" UNLESS additionalProperties is truthy. Without + # this the Konflux-only fields would fail client-side validation ("Tool input + # validation error") before the build ever reaches the gateway. + schema = BuildPackageToolInput.model_json_schema() + assert schema.get("additionalProperties") is True