Fix #418: Clean up temp directories when git clone fails

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

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"

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"