diff --git a/pdd/agentic_change.py b/pdd/agentic_change.py index bc4acdce..e63e8eb7 100644 --- a/pdd/agentic_change.py +++ b/pdd/agentic_change.py @@ -4,6 +4,7 @@ import re import shutil import subprocess +import sys import tempfile from pathlib import Path from typing import List, Tuple, Optional, Any @@ -15,7 +16,6 @@ console = Console() - def _escape_format_braces(text: str) -> str: """ Escape curly braces in text to prevent Python's .format() from @@ -126,8 +126,16 @@ def _setup_repository(owner: str, repo: str, quiet: bool) -> Path: check=False ) if result.returncode != 0: + try: + shutil.rmtree(temp_dir) + except Exception as cleanup_error: + print(f"Warning: Failed to cleanup temp directory {temp_dir}: {cleanup_error}", file=sys.stderr) raise RuntimeError(f"Failed to clone repository: {result.stderr.strip()}") except Exception as e: + try: + shutil.rmtree(temp_dir) + except Exception as cleanup_error: + print(f"Warning: Failed to cleanup temp directory {temp_dir}: {cleanup_error}", file=sys.stderr) raise RuntimeError(f"Failed to execute clone command: {e}") return temp_dir diff --git a/prompts b/prompts deleted file mode 120000 index 334bf993..00000000 --- a/prompts +++ /dev/null @@ -1 +0,0 @@ -pdd/prompts \ No newline at end of file diff --git a/tests/test_agentic_change_issue_418.py b/tests/test_agentic_change_issue_418.py new file mode 100644 index 00000000..7967c55b --- /dev/null +++ b/tests/test_agentic_change_issue_418.py @@ -0,0 +1,127 @@ +"""Tests for Issue #418: Temp directory cleanup on git clone failure.""" + +import json +from pathlib import Path +from unittest.mock import MagicMock, patch + +import pytest + +from pdd.agentic_change import run_agentic_change + + +def test_temp_dir_cleaned_up_on_clone_failure(tmp_path: Path): + """Test temp directory is cleaned up when git clone fails.""" + real_temp_dir = tmp_path / "pdd_test_repo" + real_temp_dir.mkdir() + + with patch("pdd.agentic_change.shutil.which") as mock_which, \ + patch("pdd.agentic_change.subprocess.run") as mock_subprocess, \ + patch("pdd.agentic_change.tempfile.mkdtemp", return_value=str(real_temp_dir)), \ + patch("pdd.agentic_change.console"), \ + patch("pdd.agentic_change.Path.cwd") as mock_cwd: + + mock_which.return_value = "/usr/bin/gh" + mock_cwd.return_value.__truediv__.return_value.exists.return_value = False + + issue_data = { + "title": "Test", "body": "Test", "user": {"login": "test"}, + "comments_url": "https://api.github.com/repos/owner/repo/issues/1/comments" + } + + def subprocess_side_effect(args, **kwargs): + cmd = args if isinstance(args, list) else [] + if "api" in cmd and "repos/owner/repo/issues/1" in cmd[2]: + return MagicMock(returncode=0, stdout=json.dumps(issue_data)) + if "api" in cmd and "comments" in cmd[2]: + return MagicMock(returncode=0, stdout="[]") + if "git" in cmd[0] and "remote" in cmd: + return MagicMock(returncode=1) + if "repo" in cmd and "clone" in cmd: + return MagicMock(returncode=1, stderr="fatal: repository not found") + return MagicMock(returncode=0, stdout="") + + mock_subprocess.side_effect = subprocess_side_effect + + assert real_temp_dir.exists() + success, msg, _, _, _ = run_agentic_change("https://github.com/owner/repo/issues/1") + + assert success is False + assert "Failed to clone repository" in msg + assert not real_temp_dir.exists(), "Temp directory not cleaned up on failure" + + +def test_temp_dir_cleaned_up_on_subprocess_exception(tmp_path: Path): + """Test temp directory is cleaned up when subprocess raises exception.""" + real_temp_dir = tmp_path / "pdd_test_exception" + real_temp_dir.mkdir() + + with patch("pdd.agentic_change.shutil.which", return_value="/usr/bin/gh"), \ + patch("pdd.agentic_change.subprocess.run") as mock_subprocess, \ + patch("pdd.agentic_change.tempfile.mkdtemp", return_value=str(real_temp_dir)), \ + patch("pdd.agentic_change.console"), \ + patch("pdd.agentic_change.Path.cwd") as mock_cwd: + + mock_cwd.return_value.__truediv__.return_value.exists.return_value = False + issue_data = { + "title": "Test", "body": "Test", "user": {"login": "test"}, + "comments_url": "https://api.github.com/repos/owner/repo/issues/1/comments" + } + + def subprocess_side_effect(args, **kwargs): + cmd = args if isinstance(args, list) else [] + if "api" in cmd and "repos/owner/repo/issues/1" in cmd[2]: + return MagicMock(returncode=0, stdout=json.dumps(issue_data)) + if "api" in cmd and "comments" in cmd[2]: + return MagicMock(returncode=0, stdout="[]") + if "git" in cmd[0] and "remote" in cmd: + return MagicMock(returncode=1) + if "repo" in cmd and "clone" in cmd: + raise OSError("Command not found: gh") + return MagicMock(returncode=0) + + mock_subprocess.side_effect = subprocess_side_effect + + assert real_temp_dir.exists() + success, msg, _, _, _ = run_agentic_change("https://github.com/owner/repo/issues/1") + + assert success is False + assert "Failed to execute clone command" in msg + assert not real_temp_dir.exists(), "Temp directory not cleaned up on exception" + + +def test_temp_dir_not_cleaned_up_on_success(tmp_path: Path): + """Regression test: successful clone should keep temp directory.""" + real_temp_dir = tmp_path / "pdd_test_success" + real_temp_dir.mkdir() + + with patch("pdd.agentic_change.shutil.which", return_value="/usr/bin/gh"), \ + patch("pdd.agentic_change.subprocess.run") as mock_subprocess, \ + patch("pdd.agentic_change.tempfile.mkdtemp", return_value=str(real_temp_dir)), \ + patch("pdd.agentic_change.run_agentic_change_orchestrator", return_value=(True, "Success", 1.0, "gpt-4", ["file.py"])), \ + patch("pdd.agentic_change.console"), \ + patch("pdd.agentic_change.Path.cwd") as mock_cwd: + + mock_cwd.return_value.__truediv__.return_value.exists.return_value = False + issue_data = { + "title": "Test", "body": "Test", "user": {"login": "test"}, + "comments_url": "https://api.github.com/repos/owner/repo/issues/1/comments" + } + + def subprocess_side_effect(args, **kwargs): + cmd = args if isinstance(args, list) else [] + if "api" in cmd and "repos/owner/repo/issues/1" in cmd[2]: + return MagicMock(returncode=0, stdout=json.dumps(issue_data)) + if "api" in cmd and "comments" in cmd[2]: + return MagicMock(returncode=0, stdout="[]") + if "git" in cmd[0] and "remote" in cmd: + return MagicMock(returncode=1) + if "repo" in cmd and "clone" in cmd: + return MagicMock(returncode=0, stdout="Cloning...") + return MagicMock(returncode=0) + + mock_subprocess.side_effect = subprocess_side_effect + + success, msg, _, _, _ = run_agentic_change("https://github.com/owner/repo/issues/1") + + assert success is True + assert real_temp_dir.exists(), "Successful clone should not cleanup temp directory" diff --git a/tests/test_e2e_issue_418.py b/tests/test_e2e_issue_418.py new file mode 100644 index 00000000..0361b3c6 --- /dev/null +++ b/tests/test_e2e_issue_418.py @@ -0,0 +1,88 @@ +"""E2E tests for Issue #418: Temp directory cleanup on git clone failure.""" + +import glob +import json +import os +import subprocess +import sys +import tempfile +from pathlib import Path + + +def get_project_root() -> Path: + return Path(__file__).parent.parent + + +def test_temp_dir_cleaned_up_on_clone_failure(tmp_path: Path): + """E2E: Temp directory cleaned up when clone fails.""" + project_root = get_project_root() + + test_script = f''' +import sys, json, glob, tempfile, os +sys.path.insert(0, "{project_root}") +from pdd.agentic_change import _setup_repository + +temp_root = tempfile.gettempdir() +before = len(glob.glob(os.path.join(temp_root, "pdd_*"))) + +try: + _setup_repository("nonexistent-user", "nonexistent-repo", quiet=True) +except RuntimeError: + pass + +after = len(glob.glob(os.path.join(temp_root, "pdd_*"))) +print(json.dumps({{"leaked": after > before}})) +''' + + script_file = tmp_path / "test_leak.py" + script_file.write_text(test_script) + + env = os.environ.copy() + env['PYTHONPATH'] = str(project_root) + + result = subprocess.run( + [sys.executable, str(script_file)], + capture_output=True, text=True, timeout=30, env=env + ) + + assert result.returncode == 0, f"Script failed: {result.stderr}" + output = json.loads(result.stdout.strip()) + assert not output['leaked'], "Temp directory leaked after clone failure" + + +def test_temp_dir_kept_on_success(tmp_path: Path): + """E2E: Successful clone keeps temp directory.""" + project_root = get_project_root() + + test_script = f''' +import sys, json, os +from pathlib import Path +from unittest.mock import patch, MagicMock +sys.path.insert(0, "{project_root}") +from pdd.agentic_change import _setup_repository + +with patch('pdd.agentic_change.subprocess.run') as mock_run: + mock_run.return_value = MagicMock(returncode=0, stderr="") + try: + result_dir = _setup_repository("test-owner", "test-repo", quiet=True) + exists = result_dir.exists() + except: + exists = False + +print(json.dumps({{"exists": exists}})) +''' + + script_file = tmp_path / "test_success.py" + script_file.write_text(test_script) + + env = os.environ.copy() + env['PYTHONPATH'] = str(project_root) + + result = subprocess.run( + [sys.executable, str(script_file)], + capture_output=True, text=True, timeout=30, env=env + ) + + assert result.returncode == 0, f"Script failed: {result.stderr}" + output = json.loads(result.stdout.strip()) + assert output['exists'], "Successful clone should keep temp directory"