diff --git a/ymir/agents/backport_agent.py b/ymir/agents/backport_agent.py index 04fa058ce..af48e0271 100644 --- a/ymir/agents/backport_agent.py +++ b/ymir/agents/backport_agent.py @@ -3,6 +3,7 @@ import logging import os import re +import shutil import sys import traceback from dataclasses import dataclass @@ -963,7 +964,10 @@ async def prepare_normal_backport(state): package=state.package, dist_git_branch=state.dist_git_branch, ) - state.unpacked_sources = tasks.get_unpacked_sources(state.local_clone, state.package) + builddir = local_tool_options.get("builddir") + state.unpacked_sources = tasks.get_unpacked_sources( + state.local_clone, state.package, builddir=Path(builddir) if builddir else None + ) for idx, upstream_patch in enumerate(state.upstream_patches): patch_name = f"{state.jira_issue}-{idx}.patch" content = await run_tool( @@ -1901,25 +1905,29 @@ async def comment_in_jira(state): workflow.add_step("submit_consolidation_job", submit_consolidation_job) workflow.add_step("comment_in_jira", comment_in_jira) - response = await workflow.run( - BackportState( - package=package, - dist_git_branch=dist_git_branch, - dist_git_namespace=dist_git_namespace, - upstream_patches=upstream_patches, - jira_issue=jira_issue, - workspace_id=workspace_id, - cve_id=cve_id, - justification=justification, - triage_summary=triage_summary, - fix_version=fix_version, - attempts_remaining=max_build_attempts, - shipped_zstream_candidates=shipped_zstream_candidates or [], - inherited_publication_checkpoint=inherited_publication_checkpoint, - inheritance_disabled=inheritance_disabled, - ), - ) - return response.state + try: + response = await workflow.run( + BackportState( + package=package, + dist_git_branch=dist_git_branch, + dist_git_namespace=dist_git_namespace, + upstream_patches=upstream_patches, + jira_issue=jira_issue, + workspace_id=workspace_id, + cve_id=cve_id, + justification=justification, + triage_summary=triage_summary, + fix_version=fix_version, + attempts_remaining=max_build_attempts, + shipped_zstream_candidates=shipped_zstream_candidates or [], + inherited_publication_checkpoint=inherited_publication_checkpoint, + inheritance_disabled=inheritance_disabled, + ), + ) + return response.state + finally: + if builddir := local_tool_options.get("builddir"): + shutil.rmtree(builddir, ignore_errors=True) def _parse_upstream_patches(upstream_patches_raw: str) -> list[str]: diff --git a/ymir/agents/cve_applicability_agent.py b/ymir/agents/cve_applicability_agent.py index 60f726931..220a38a1c 100644 --- a/ymir/agents/cve_applicability_agent.py +++ b/ymir/agents/cve_applicability_agent.py @@ -62,7 +62,6 @@ def build_applicability_prompt( dep_issue_key: str | None, patch_files: list[str], unpacked_sources: Path, - local_clone: Path, prep_ok: bool = True, ) -> str: cve_label = cve_id or "the CVE" @@ -88,7 +87,6 @@ def build_applicability_prompt( f"cannot verify the full dependency chain — classify as 'Inconclusive'.\n" ) - sources_rel = unpacked_sources.relative_to(local_clone) if patch_files: patch_info = "Upstream fix patches are available at: " + ", ".join(patch_files) else: @@ -112,10 +110,10 @@ def build_applicability_prompt( {rebuild_context} {patch_info} {fallback_warning} - The unpacked package source is at: {sources_rel} + The unpacked package source is at: {unpacked_sources} CRITICAL: Your analysis MUST be based on the package source at - {sources_rel} — this is the actual version shipped in RHEL. + {unpacked_sources} — this is the actual version shipped in RHEL. Do NOT clone or check the latest upstream repository — it may already contain the fix, which is irrelevant to whether the shipped RHEL version is affected. If the fix patch applies @@ -127,7 +125,7 @@ def build_applicability_prompt( (package.json, requirements.txt, go.mod, pom.xml, etc.) does NOT mean the component is shipped. What matters is whether the component's actual source or compiled files exist on disk in - {sources_rel}. If the vulnerable library's files are absent + {unpacked_sources}. If the vulnerable library's files are absent (e.g. no node_modules//, no vendored source, `find` returns empty), classify as "Component not Present" regardless of what manifests or import statements declare. Manifests can @@ -156,12 +154,12 @@ def build_applicability_prompt( 2. If upstream fix patches are available, read them to identify the specific files and functions modified by the fix. Then verify whether those files physically exist in - {sources_rel} (use `find` to locate them). If the + {unpacked_sources} (use `find` to locate them). If the vulnerable library's files are completely absent from the source tree, that is definitive evidence of Component not Present — stop and classify accordingly. 3. Search for those files/functions in the package source at - {sources_rel}. Do NOT look at any other copy of the source. + {unpacked_sources}. Do NOT look at any other copy of the source. If the files do not exist on disk, that is decisive — do not override this with manifest declarations or import statements. 4. If the vulnerable code is not present, determine why — older diff --git a/ymir/agents/merge_request_agent.py b/ymir/agents/merge_request_agent.py index 6a7012aa8..1481d20b5 100644 --- a/ymir/agents/merge_request_agent.py +++ b/ymir/agents/merge_request_agent.py @@ -2,6 +2,7 @@ import logging import os import re +import shutil import sys import traceback from pathlib import Path @@ -325,8 +326,12 @@ async def comment_in_mr(state): workflow.add_step("commit_and_push", commit_and_push) workflow.add_step("comment_in_mr", comment_in_mr) - response = await workflow.run(State(merge_request_url=merge_request_url)) - return response.state + try: + response = await workflow.run(State(merge_request_url=merge_request_url)) + return response.state + finally: + if builddir := local_tool_options.get("builddir"): + shutil.rmtree(builddir, ignore_errors=True) if merge_request_url := os.getenv("MERGE_REQUEST_URL", None): logger.info("Running in direct mode with environment variables") diff --git a/ymir/agents/mr_consolidation_agent.py b/ymir/agents/mr_consolidation_agent.py index ea89955b9..6dc6e17b1 100644 --- a/ymir/agents/mr_consolidation_agent.py +++ b/ymir/agents/mr_consolidation_agent.py @@ -4,6 +4,7 @@ import logging import os import re +import shutil import time import traceback from datetime import timedelta @@ -1671,8 +1672,12 @@ async def handle_failure(state): # CVE / Jira lists are collected from commit footers after branches # are fetched — do not seed from branch metadata. - response = await workflow.run(initial_state) - return response.state + try: + response = await workflow.run(initial_state) + return response.state + finally: + if builddir := local_tool_options.get("builddir"): + shutil.rmtree(builddir, ignore_errors=True) _CONSOLIDATED_MARKER = "## Consolidated Backport MR" diff --git a/ymir/agents/rebase_agent.py b/ymir/agents/rebase_agent.py index e421928c7..27b6af025 100644 --- a/ymir/agents/rebase_agent.py +++ b/ymir/agents/rebase_agent.py @@ -1,6 +1,7 @@ import asyncio import logging import os +import shutil import sys import traceback from pathlib import Path @@ -671,23 +672,27 @@ async def comment_in_jira(state): workflow.add_step("commit_push_and_open_mr", commit_push_and_open_mr) workflow.add_step("comment_in_jira", comment_in_jira) - response = await workflow.run( - State( - package=package, - dist_git_branch=dist_git_branch, - dist_git_namespace=dist_git_namespace, - version=version, - jira_issue=jira_issue, - workspace_id=workspace_id, - cve_id=cve_id, - fix_version=fix_version, - justification=justification, - triage_summary=triage_summary, - consolidated_issues=consolidated_issues or [], - consolidation_summary=consolidation_summary, - ), - ) - return response.state + try: + response = await workflow.run( + State( + package=package, + dist_git_branch=dist_git_branch, + dist_git_namespace=dist_git_namespace, + version=version, + jira_issue=jira_issue, + workspace_id=workspace_id, + cve_id=cve_id, + fix_version=fix_version, + justification=justification, + triage_summary=triage_summary, + consolidated_issues=consolidated_issues or [], + consolidation_summary=consolidation_summary, + ), + ) + return response.state + finally: + if builddir := local_tool_options.get("builddir"): + shutil.rmtree(builddir, ignore_errors=True) if ( (package := os.getenv("PACKAGE", None)) diff --git a/ymir/agents/rebuild_consolidation.py b/ymir/agents/rebuild_consolidation.py index a10fe74a6..32ca4187a 100644 --- a/ymir/agents/rebuild_consolidation.py +++ b/ymir/agents/rebuild_consolidation.py @@ -338,7 +338,6 @@ async def _check_sibling_applicability( dep_issue_key=dep_issue_key, patch_files=[], unpacked_sources=unpacked_sources, - local_clone=local_clone, ) response = await agent.run( prompt, diff --git a/ymir/agents/tasks.py b/ymir/agents/tasks.py index 02ba03bfb..13a507188 100644 --- a/ymir/agents/tasks.py +++ b/ymir/agents/tasks.py @@ -5,6 +5,7 @@ import re import shutil import subprocess +import tempfile import unicodedata from collections.abc import Awaitable, Callable from datetime import UTC, datetime @@ -1298,11 +1299,13 @@ async def resolve_current_canonical_mr_title( return None -def get_unpacked_sources(local_clone: Path, package: str) -> Path: +def get_unpacked_sources(local_clone: Path, package: str, builddir: Path | None = None) -> Path: """ Get a path to the root of extracted archive directory tree (referenced as TLD - in RPM documentation) for a given package. + in RPM documentation) for a given package. When *builddir* is given, looks + there instead of under *local_clone*. """ + base = builddir or local_clone with Specfile(local_clone / f"{package}.spec") as spec: name = spec.expand("%{name}") version = spec.expand("%{version}") @@ -1314,23 +1317,25 @@ def get_unpacked_sources(local_clone: Path, package: str) -> Path: buildsubdir = buildsubdir.split("/")[0] # RPM 4.20+ uses a per-build directory named %{NAME}-%{VERSION}-build - per_build_dir = local_clone / f"{name}-{version}-build" + per_build_dir = base / f"{name}-{version}-build" sources_dir = per_build_dir / buildsubdir if sources_dir.is_dir(): return sources_dir # Older RPM versions unpack directly under _builddir - sources_dir = local_clone / buildsubdir + sources_dir = base / buildsubdir if sources_dir.is_dir(): return sources_dir raise ValueError(f"Unpacked source directory does not exist: {sources_dir}") -async def _fallback_extract_sources(local_clone: Path, package: str) -> Path: +async def _fallback_extract_sources(local_clone: Path, package: str) -> tuple[Path, str]: """ Fallback when centpkg/rhpkg prep fails: extract the primary source archive using Source0 from the spec file. + Returns (unpacked_sources, extract_dir) where extract_dir is a /tmp + path the caller must clean up. """ try: with Specfile(local_clone / f"{package}.spec") as spec: @@ -1343,20 +1348,22 @@ async def _fallback_extract_sources(local_clone: Path, package: str) -> Path: raise ValueError(f"Could not determine source archive for {package}: {e}") from e logger.info(f"Using Source0 from spec: {archive.name}") - extract_dir = local_clone / "_extracted" - extract_dir.mkdir(exist_ok=True) + extract_dir = Path(tempfile.mkdtemp(prefix="rpmbuild-fallback-")) cmd = ["/usr/lib/rpm/rpmuncompress", "-x", str(archive)] logger.info(f"Extracting {archive.name} to {extract_dir}") - exit_code, _, stderr = await run_subprocess(cmd, cwd=extract_dir) - if exit_code != 0: - raise ValueError(f"Failed to extract {archive.name}: {stderr}") - - subdirs = [d for d in extract_dir.iterdir() if d.is_dir()] + try: + exit_code, _, stderr = await run_subprocess(cmd, cwd=extract_dir) + if exit_code != 0: + raise ValueError(f"Failed to extract {archive.name}: {stderr}") + subdirs = [d for d in extract_dir.iterdir() if d.is_dir()] + except BaseException: + shutil.rmtree(extract_dir, ignore_errors=True) + raise if len(subdirs) == 1: - return subdirs[0] - return extract_dir + return subdirs[0], str(extract_dir) + return extract_dir, str(extract_dir) async def clone_and_prep_sources( @@ -1366,10 +1373,11 @@ async def clone_and_prep_sources( jira_issue: str, ref: str | None = None, dist_git_namespace: str | None = None, -) -> tuple[Path, Path, bool]: +) -> tuple[Path, Path, bool, str | None]: """ Clone dist-git repo and run centpkg/rhpkg sources + prep. - Returns (local_clone, unpacked_sources, prep_succeeded). + Returns (local_clone, unpacked_sources, prep_succeeded, builddir). + The caller must clean up *builddir* (a /tmp path) when done. Read-only: no fork, no push — just for source analysis. Falls back to manual archive extraction if prep fails (e.g. missing @@ -1420,20 +1428,34 @@ async def clone_and_prep_sources( # Run prep locally rather than via MCP gateway: the agent container is # RHEL-based so rpmbuild evaluates %prep macros correctly, whereas the # MCP gateway runs Fedora and would expand them differently. - result = await run_tool( - RunPackagePrepTool(), - dist_git_path=str(local_clone), - package=package, - dist_git_branch=dist_git_branch, - ) + prep_tool = RunPackagePrepTool() + try: + result = await run_tool( + prep_tool, + dist_git_path=str(local_clone), + package=package, + dist_git_branch=dist_git_branch, + ) + except BaseException: + if builddir := (prep_tool.options or {}).get("builddir"): + shutil.rmtree(builddir, ignore_errors=True) + raise if "Prep FAILED" not in result: - unpacked = get_unpacked_sources(local_clone, package) - return local_clone, unpacked, True + builddir = prep_tool.options.get("builddir") + try: + unpacked = get_unpacked_sources( + local_clone, package, builddir=Path(builddir) if builddir else None + ) + except BaseException: + if builddir: + shutil.rmtree(builddir, ignore_errors=True) + raise + return local_clone, unpacked, True, builddir logger.warning(f"prep failed for {package}, falling back to manual extraction: {result}") - unpacked = await _fallback_extract_sources(local_clone, package) - return local_clone, unpacked, False + unpacked, fallback_builddir = await _fallback_extract_sources(local_clone, package) + return local_clone, unpacked, False, fallback_builddir class InvalidConsolidationConfigError(Exception): diff --git a/ymir/agents/triage_agent.py b/ymir/agents/triage_agent.py index cd035c971..6ed999125 100644 --- a/ymir/agents/triage_agent.py +++ b/ymir/agents/triage_agent.py @@ -555,6 +555,8 @@ async def run_workflow( if mock_env := get_mock_local_tool_env(jira_issue): local_tool_options = {"env": mock_env} + cleanup = {"builddir": None} + async with mcp_tools(os.getenv("MCP_GATEWAY_URL"), call_meta={"jira_issue": jira_issue}) as gateway_tools: triage_agent = triage_agent_factory(gateway_tools, local_tool_options) @@ -961,7 +963,7 @@ async def check_cve_applicability(state): logger.warning(f"Failed to check branches for {package}: {e}") try: - local_clone, unpacked_sources, prep_ok = await tasks.clone_and_prep_sources( + local_clone, unpacked_sources, prep_ok, builddir = await tasks.clone_and_prep_sources( package=package, dist_git_branch=clone_branch, available_tools=gateway_tools, @@ -981,6 +983,7 @@ async def check_cve_applicability(state): if not prep_ok: logger.warning(f"Source prep failed for {package} — analyzing unpatched upstream source") + cleanup["builddir"] = builddir state.applicability_local_clone = local_clone state.applicability_unpacked_sources = unpacked_sources state.applicability_used_fallback = not prep_ok @@ -996,7 +999,7 @@ async def check_cve_applicability(state): ) patch_name = f"{state.jira_issue}-{idx}.patch" (local_clone / patch_name).write_text(content) - patch_files.append(patch_name) + patch_files.append(str(local_clone / patch_name)) except Exception: logger.warning(f"Could not fetch patch from {url}") @@ -1014,7 +1017,6 @@ async def check_cve_applicability(state): dep_issue_key=dep_issue_key, patch_files=patch_files, unpacked_sources=unpacked_sources, - local_clone=local_clone, prep_ok=prep_ok, ) @@ -1211,12 +1213,6 @@ async def consolidate_rebase_siblings(state): return "comment_in_jira" async def comment_in_jira(state): - applicability_dir = Path(os.environ["GIT_REPO_BASEPATH"]) / APPLICABILITY_DIR / state.jira_issue - if applicability_dir.exists(): - shutil.rmtree(applicability_dir, ignore_errors=True) - state.applicability_local_clone = None - state.applicability_unpacked_sources = None - comment_text = state.triage_result.format_for_comment(auto_chain=auto_chain) if state.applicability_check_skipped: comment_text += ( @@ -1267,8 +1263,21 @@ async def comment_in_jira(state): workflow.add_step("consolidate_rebase_siblings", consolidate_rebase_siblings) workflow.add_step("comment_in_jira", comment_in_jira) - response = await workflow.run(TriageState(jira_issue=jira_issue)) - return response.state + response = None + try: + response = await workflow.run(TriageState(jira_issue=jira_issue)) + return response.state + finally: + applicability_dir = ( + Path(os.environ.get("GIT_REPO_BASEPATH", "/git-repos")) / APPLICABILITY_DIR / jira_issue + ) + if applicability_dir.exists(): + shutil.rmtree(applicability_dir, ignore_errors=True) + if cleanup["builddir"]: + shutil.rmtree(cleanup["builddir"], ignore_errors=True) + if response is not None: + response.state.applicability_local_clone = None + response.state.applicability_unpacked_sources = None async def label_postponed_issues(jira_issue: str, output: OutputSchema, dry_run: bool, user_triggered: bool): diff --git a/ymir/tools/unprivileged/tests/unit/test_wicked_git.py b/ymir/tools/unprivileged/tests/unit/test_wicked_git.py index ebfac8662..daaa55370 100644 --- a/ymir/tools/unprivileged/tests/unit/test_wicked_git.py +++ b/ymir/tools/unprivileged/tests/unit/test_wicked_git.py @@ -1,4 +1,6 @@ +import shutil import subprocess +from pathlib import Path import pytest from beeai_framework.middleware.trajectory import GlobalTrajectoryMiddleware @@ -310,15 +312,14 @@ async def mock_run_subprocess(cmd, **kwargs): ) assert "Prep succeeded" in output.result assert "prep done successfully" in output.result + builddir = tool.options.get("builddir") + assert builddir is not None, "builddir should be stored in options on success" + assert Path(builddir).is_dir(), "builddir should exist on disk after success" + shutil.rmtree(builddir, ignore_errors=True) @pytest.mark.asyncio -async def test_run_package_prep_failure_cleans_build_dir(dist_git_dir): - # Simulate a partially-patched build subdirectory left behind by rpmbuild - build_dir = dist_git_dir / "ruby-3.3.10" - build_dir.mkdir() - (build_dir / "patched_file.rb").write_text("partially applied content") - +async def test_run_package_prep_failure_cleans_builddir(dist_git_dir): async def mock_run_subprocess(cmd, **kwargs): return (1, "", "patch failed to apply") @@ -334,21 +335,19 @@ async def mock_run_subprocess(cmd, **kwargs): ) assert "Prep FAILED" in output.result assert "cleaned up" in output.result - assert not build_dir.exists(), "Build directory should have been removed on failure" + assert tool.options.get("builddir") is None, "builddir option should be cleared on failure" @pytest.mark.asyncio -async def test_run_package_prep_failure_preserves_non_matching_dirs(dist_git_dir): - # Create directories: one matching the package name, one not - build_dir = dist_git_dir / "ruby-3.3.10" - build_dir.mkdir() - other_dir = dist_git_dir / "some-other-dir" - other_dir.mkdir() +async def test_run_package_prep_reuses_builddir(dist_git_dir): + call_count = 0 async def mock_run_subprocess(cmd, **kwargs): - return (1, "", "") + nonlocal call_count + call_count += 1 + return (0, f"prep done {call_count}", "") - flexmock(wicked_git_mod).should_receive("run_subprocess").replace_with(mock_run_subprocess).once() + flexmock(wicked_git_mod).should_receive("run_subprocess").replace_with(mock_run_subprocess).twice() tool = RunPackagePrepTool() await tool.run( @@ -358,8 +357,19 @@ async def mock_run_subprocess(cmd, **kwargs): dist_git_branch="c10s", ), ) - assert not build_dir.exists(), "Build directory should have been removed" - assert other_dir.exists(), "Non-matching directory should be preserved" + first_builddir = tool.options.get("builddir") + + await tool.run( + input=RunPackagePrepInput( + dist_git_path=str(dist_git_dir), + package="ruby", + dist_git_branch="c10s", + ), + ) + second_builddir = tool.options.get("builddir") + + assert first_builddir == second_builddir, "builddir should be reused across calls" + shutil.rmtree(first_builddir, ignore_errors=True) @pytest.mark.asyncio diff --git a/ymir/tools/unprivileged/wicked_git.py b/ymir/tools/unprivileged/wicked_git.py index c7c1e6713..78a893d7a 100644 --- a/ymir/tools/unprivileged/wicked_git.py +++ b/ymir/tools/unprivileged/wicked_git.py @@ -1,5 +1,6 @@ import re import shutil +import tempfile from pathlib import Path from beeai_framework.context import RunContext @@ -93,7 +94,9 @@ async def _run( raise ToolError(f"ERROR: {e}") from e -def build_rpmdefines(dist_git_path: Path, branch: str, spec_path: Path) -> list[str]: +def build_rpmdefines( + dist_git_path: Path, branch: str, spec_path: Path, *, builddir: Path | None = None +) -> list[str]: if not (parsed := parse_branch_name(branch)): raise ToolError(f"Cannot parse branch name: {branch}") major, minor = parsed @@ -107,7 +110,7 @@ def build_rpmdefines(dist_git_path: Path, branch: str, spec_path: Path) -> list[ "--define", f"_specdir {root}", "--define", - f"_builddir {root}", + f"_builddir {builddir or root}", "--define", f"_srcrpmdir {root}", "--define", @@ -157,8 +160,14 @@ class RunPackagePrepTool(Tool[RunPackagePrepInput, ToolRunOptions, StringToolOut timeout = 600 description = """ Runs the package prep step (source extraction + patch application) to verify - that all patches apply cleanly. If prep fails, the build subdirectory is - removed to prevent inspection of a partially-patched source tree. + that all patches apply cleanly. Build output goes to a /tmp directory stored + in options["builddir"]; the caller must clean it up after use. If prep fails, + the build directory is removed to prevent inspection of a partially-patched + source tree. + + Callers that read back "builddir" from a shared options dict must pass a + non-empty dict (e.g. containing "working_directory"); the framework replaces + an empty dict with None, so mutations would go to an internal copy instead. """ input_schema = RunPackagePrepInput @@ -178,20 +187,31 @@ async def _run( if not dist_git.exists(): raise ToolError(f"Dist-git path does not exist: {dist_git}") - spec_path = dist_git / f"{tool_input.package}.spec" - defines = build_rpmdefines(dist_git, tool_input.dist_git_branch, spec_path) - cmd = ["rpmbuild", *defines, "--nodeps", "-bp", str(spec_path)] + if self.options is None: + self._options = {} + builddir = self.options.get("builddir") + if not builddir: + builddir = tempfile.mkdtemp(prefix="rpmbuild-") + self.options["builddir"] = builddir - exit_code, stdout, stderr = await run_subprocess(cmd, cwd=dist_git) + try: + spec_path = dist_git / f"{tool_input.package}.spec" + defines = build_rpmdefines( + dist_git, tool_input.dist_git_branch, spec_path, builddir=Path(builddir) + ) + cmd = ["rpmbuild", *defines, "--nodeps", "-bp", str(spec_path)] + + exit_code, stdout, stderr = await run_subprocess(cmd, cwd=dist_git) + except BaseException: + shutil.rmtree(builddir, ignore_errors=True) + self.options.pop("builddir", None) + raise if exit_code == 0: return StringToolOutput(result=f"Prep succeeded.\n{stdout}") - # Prep failed — remove build subdirectories to prevent stale state. - # rpmbuild creates directories like -/ under the dist-git root. - for child in dist_git.iterdir(): - if child.is_dir() and child.name.startswith(tool_input.package + "-"): - shutil.rmtree(child, ignore_errors=True) + shutil.rmtree(builddir, ignore_errors=True) + self.options.pop("builddir", None) return StringToolOutput( result=f"Prep FAILED (exit code {exit_code}). "