fix(agent-cli): Windows temp-cwd cleanup must not fail a successful batch

src/skillspector/providers/_agent_cli.py

@@ -53,6 +53,7 @@
 import subprocess
 import tempfile
 import threading
+import time
 from collections.abc import Callable
 from dataclasses import dataclass
 from typing import Any
@@ -632,6 +633,37 @@ def _drain_stream(stream: Any, buf: bytearray, cap: int, on_overflow: Any) -> No
             pass
 
 
+_CLEANUP_RETRIES = 10
+_CLEANUP_RETRY_DELAY_S = 0.2
+
+
+def _cleanup_temp_dir(path: str) -> None:
+    """Best-effort removal of the per-invocation temp cwd.
+
+    On Windows a directory cannot be deleted while any process has it as its
+    working directory or holds a handle inside it. The agent CLI's process
+    tree can outlive the direct child by a beat (a ``.cmd`` shim's node child,
+    or a killed-but-not-reaped child on the timeout path), so the first
+    attempts may fail transiently with ``WinError 32``. Retry briefly, then
+    leak the directory with a warning rather than raise: by the time cleanup
+    runs the model's response is already in hand, and a temp-dir leak must
+    never fail the batch (#315).
+    """
+    for _ in range(_CLEANUP_RETRIES):
+        try:
+            shutil.rmtree(path)
+            return
+        except OSError:
+            time.sleep(_CLEANUP_RETRY_DELAY_S)
+    shutil.rmtree(path, ignore_errors=True)
+    if os.path.isdir(path):
+        logger.warning(
+            "Could not remove temp dir %s (handle still held by the CLI "
+            "process tree?); leaking it rather than failing the batch",
+            path,
+        )
+
+
 def _run_bounded(
     proc: subprocess.Popen, prompt_bytes: bytes, timeout: float
 ) -> tuple[int | None, bytes, bytes, bool]:
@@ -767,7 +799,12 @@ def run_agent_cli(
     child_env = _scrub_env()
 
     # -- Run in a temporary directory (no CWD access) -------------------------
-    with tempfile.TemporaryDirectory(prefix="skillspector_cli_") as tmp_cwd:
+    # mkdtemp + explicit best-effort cleanup instead of TemporaryDirectory:
+    # the context manager's rmtree-at-__exit__ raises on Windows while the
+    # CLI's process tree still holds the directory as its cwd, turning an
+    # already-successful call into a batch failure (#315).
+    tmp_cwd = tempfile.mkdtemp(prefix="skillspector_cli_")
+    try:
         logger.debug(
             "Running %s argv=%r cwd=%s timeout=%ss",
             binary_name,
@@ -792,6 +829,8 @@ def run_agent_cli(
         # CLI cannot exhaust memory before the cap is enforced (a chatty child
         # could otherwise buffer unbounded output until the timeout).
         returncode, stdout_raw, stderr_raw, overflow = _run_bounded(proc, prompt_bytes, timeout)
+    finally:
+        _cleanup_temp_dir(tmp_cwd)
 
     # -- Fail-closed checks ---------------------------------------------------
     if overflow:

tests/unit/test_agent_cli.py

@@ -733,3 +733,95 @@ def test_is_available_false_even_when_binary_present(
         ok, reason = _agent_cli.is_available("agy")
         assert ok is False
         assert "disabled" in (reason or "")
+
+
+# ---------------------------------------------------------------------------
+# _cleanup_temp_dir — Windows-safe temp cwd removal (#315)
+# ---------------------------------------------------------------------------
+
+
+class TestCleanupTempDir:
+    """Cleanup failure must never outrank a successful CLI response (#315).
+
+    On Windows the CLI's process tree can hold the temp cwd (a ``.cmd``
+    shim's node child, or a killed child on the timeout path), so rmtree can
+    fail transiently — or persistently — after the model already answered.
+    """
+
+    def test_removes_directory(self, tmp_path) -> None:
+        target = tmp_path / "cli_cwd"
+        target.mkdir()
+        (target / "scratch.txt").write_text("x")
+        _agent_cli._cleanup_temp_dir(str(target))
+        assert not target.exists()
+
+    def test_retries_transient_failure_then_succeeds(
+        self, tmp_path, monkeypatch: pytest.MonkeyPatch
+    ) -> None:
+        target = tmp_path / "cli_cwd"
+        target.mkdir()
+        real_rmtree = _agent_cli.shutil.rmtree
+        calls = {"n": 0}
+
+        def flaky_rmtree(path, ignore_errors=False):
+            calls["n"] += 1
+            if calls["n"] < 3:
+                raise OSError(32, "held by another process")
+            return real_rmtree(path, ignore_errors=ignore_errors)
+
+        monkeypatch.setattr(_agent_cli.shutil, "rmtree", flaky_rmtree)
+        monkeypatch.setattr(_agent_cli.time, "sleep", lambda _s: None)
+        _agent_cli._cleanup_temp_dir(str(target))
+        assert not target.exists()
+        assert calls["n"] == 3
+
+    def test_never_raises_when_removal_keeps_failing(
+        self, tmp_path, monkeypatch: pytest.MonkeyPatch
+    ) -> None:
+        target = tmp_path / "cli_cwd"
+        target.mkdir()
+
+        def stuck_rmtree(path, ignore_errors=False):
+            if not ignore_errors:
+                raise OSError(32, "held by another process")
+
+        monkeypatch.setattr(_agent_cli.shutil, "rmtree", stuck_rmtree)
+        monkeypatch.setattr(_agent_cli.time, "sleep", lambda _s: None)
+        # Must not raise; the directory is leaked deliberately.
+        _agent_cli._cleanup_temp_dir(str(target))
+        assert target.exists()
+
+    @pytest.mark.skipif(sys.platform != "win32", reason="Windows handle semantics")
+    def test_open_handle_does_not_raise_on_windows(
+        self, tmp_path, monkeypatch: pytest.MonkeyPatch
+    ) -> None:
+        """A real open handle inside the dir — the exact WinError 32 case."""
+        target = tmp_path / "cli_cwd"
+        target.mkdir()
+        monkeypatch.setattr(_agent_cli.time, "sleep", lambda _s: None)
+        held = open(target / "held.txt", "w")  # noqa: SIM115 — handle held on purpose
+        try:
+            _agent_cli._cleanup_temp_dir(str(target))  # must not raise
+        finally:
+            held.close()
+        _agent_cli._cleanup_temp_dir(str(target))
+        assert not target.exists()
+
+
+@patch("skillspector.providers._agent_cli.find_binary", return_value=CLAUDE_BINARY)
+@patch("skillspector.providers._agent_cli.subprocess.Popen")
+class TestRunAgentCLISurvivesCleanupFailure:
+    def test_response_returned_when_temp_dir_cannot_be_removed(
+        self, mock_popen: MagicMock, _mock_binary: MagicMock, monkeypatch: pytest.MonkeyPatch
+    ) -> None:
+        """Regression for #315: rmtree failure must not fail the batch."""
+        mock_popen.return_value = _make_ok_process(_GOOD_CLAUDE_OUTPUT.encode())
+
+        def stuck_rmtree(path, ignore_errors=False):
+            if not ignore_errors:
+                raise OSError(32, "held by another process")
+
+        monkeypatch.setattr(_agent_cli.shutil, "rmtree", stuck_rmtree)
+        monkeypatch.setattr(_agent_cli.time, "sleep", lambda _s: None)
+        result = run_agent_cli("claude", PROMPT, model=MODEL)
+        assert result  # the model's answer survives the cleanup failure