diff --git a/tests/test_loop.py b/tests/test_loop.py index 6ad25ecf..bb879cbf 100644 --- a/tests/test_loop.py +++ b/tests/test_loop.py @@ -182,9 +182,11 @@ def create_message(self, payload, stream): class FakeClientTwoEmptyThenDone: def __init__(self): self.calls = 0 + self.payloads = [] def create_message(self, payload, stream): self.calls += 1 + self.payloads.append(payload) if self.calls <= 2: return { "id": str(self.calls), @@ -203,9 +205,11 @@ def create_message(self, payload, stream): class FakeClientThreeEmpty: def __init__(self): self.calls = 0 + self.payloads = [] def create_message(self, payload, stream): self.calls += 1 + self.payloads.append(payload) return { "id": str(self.calls), "role": "assistant", @@ -214,7 +218,7 @@ def create_message(self, payload, stream): } -def test_loop_retries_on_empty_assistant_turn(tmp_path: Path): +def test_single_empty_turn_does_not_stall_run(tmp_path: Path): client = FakeClientEmptyThenDone() runner = Runner(client=client, repo=tmp_path, model="m", stream=False) @@ -222,15 +226,46 @@ def test_loop_retries_on_empty_assistant_turn(tmp_path: Path): assert client.calls == 2 assert result["response"]["content"] == [{"type": "text", "text": "ok continuing"}] - assert client.second_payload is not None - continuation_messages = [ - m - for m in client.second_payload["messages"] - if m["role"] == "user" - and m["content"] - and "Continue. You ended your previous turn with no output." in m["content"][0].get("text", "") + assert result["response"]["content"][0]["text"] == "ok continuing" + + +class FakeClientEmptyToolEmptyThenDone: + def __init__(self): + self.calls = 0 + self.payloads = [] + + def create_message(self, payload, stream): + self.calls += 1 + self.payloads.append(payload) + if self.calls == 1: + return {"id": "1", "role": "assistant", "content": []} + if self.calls == 2: + return { + "id": "2", + "role": "assistant", + "content": [{"type": "tool_use", "id": "tool-1", "name": "Ls", "input": {"path": "."}}], + } + if self.calls == 3: + return {"id": "3", "role": "assistant", "content": [{"type": "text", "text": " "}]} + return {"id": "4", "role": "assistant", "content": [{"type": "text", "text": "done"}]} + + +def test_empty_turn_counter_resets_after_tool_turn(tmp_path: Path): + client = FakeClientEmptyToolEmptyThenDone() + runner = Runner(client=client, repo=tmp_path, model="m", stream=False) + + result = runner.run("list and continue") + + assert client.calls == 4 + assert result["response"]["content"] == [{"type": "text", "text": "done"}] + all_texts = [ + b.get("text", "") + for payload in client.payloads + for message in payload.get("messages", []) + for b in message.get("content", []) + if isinstance(b, dict) and b.get("type") == "text" ] - assert continuation_messages + assert not any("RECOVERY: blank turns detected." in text for text in all_texts) def test_tool_result_followup_is_pure_tool_result_message(tmp_path: Path): @@ -336,6 +371,14 @@ def test_loop_retries_twice_then_succeeds(tmp_path: Path): assert client.calls == 3 assert result["response"]["content"] == [{"type": "text", "text": "ok after two empties"}] + recovery_messages = [ + b.get("text", "") + for m in client.payloads[2]["messages"] + if m.get("role") == "user" + for b in m.get("content", []) + if isinstance(b, dict) and b.get("type") == "text" + ] + assert any("RECOVERY: blank turns detected." in text for text in recovery_messages) def test_loop_stops_after_retry_limit_on_empty_turns(tmp_path: Path): @@ -345,6 +388,8 @@ def test_loop_stops_after_retry_limit_on_empty_turns(tmp_path: Path): result = runner.run("whatever") assert client.calls == 3 + assert result["execution"]["terminated_reason"] == "stalled_empty_responses" + assert result["execution"]["completed"] is False assert result["response"]["content"] == [] diff --git a/tests/test_state_runtime.py b/tests/test_state_runtime.py index 16e604a7..8902cdd0 100644 --- a/tests/test_state_runtime.py +++ b/tests/test_state_runtime.py @@ -238,6 +238,92 @@ def test_verification_detail_event_keeps_raw_trace(tmp_path: Path, monkeypatch: assert "last_validation_summary:" in out +def test_git_changed_files_keeps_git_status_behavior(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + class Proc: + returncode = 0 + stdout = " M src/a.py\n?? tests/test_a.py\n" + + monkeypatch.setattr(state_runtime.subprocess, "run", lambda *args, **kwargs: Proc()) + assert state_runtime.git_changed_files(tmp_path) == ["src/a.py", "tests/test_a.py"] + + +def test_git_changed_files_non_git_first_call_returns_empty(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + class Proc: + returncode = 128 + stdout = "" + + monkeypatch.setattr(state_runtime.subprocess, "run", lambda *args, **kwargs: Proc()) + monkeypatch.setattr(state_runtime, "_NON_GIT_SNAPSHOT_BY_REPO", {}) + assert state_runtime.git_changed_files(tmp_path) == [] + + +def test_git_changed_files_non_git_detects_new_file(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + class Proc: + returncode = 128 + stdout = "" + + monkeypatch.setattr(state_runtime.subprocess, "run", lambda *args, **kwargs: Proc()) + monkeypatch.setattr(state_runtime, "_NON_GIT_SNAPSHOT_BY_REPO", {}) + assert state_runtime.git_changed_files(tmp_path) == [] + (tmp_path / "created.txt").write_text("hello", encoding="utf-8") + assert state_runtime.git_changed_files(tmp_path) == ["created.txt"] + + +def test_git_changed_files_non_git_detects_modified_file(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + class Proc: + returncode = 128 + stdout = "" + + file_path = tmp_path / "file.txt" + file_path.write_text("one", encoding="utf-8") + monkeypatch.setattr(state_runtime.subprocess, "run", lambda *args, **kwargs: Proc()) + monkeypatch.setattr(state_runtime, "_NON_GIT_SNAPSHOT_BY_REPO", {}) + assert state_runtime.git_changed_files(tmp_path) == [] + file_path.write_text("three", encoding="utf-8") + assert state_runtime.git_changed_files(tmp_path) == ["file.txt"] + + +def test_git_changed_files_non_git_detects_deleted_file(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + class Proc: + returncode = 128 + stdout = "" + + file_path = tmp_path / "deleted.txt" + file_path.write_text("x", encoding="utf-8") + monkeypatch.setattr(state_runtime.subprocess, "run", lambda *args, **kwargs: Proc()) + monkeypatch.setattr(state_runtime, "_NON_GIT_SNAPSHOT_BY_REPO", {}) + assert state_runtime.git_changed_files(tmp_path) == [] + file_path.unlink() + assert state_runtime.git_changed_files(tmp_path) == ["deleted.txt"] + + +def test_git_changed_files_non_git_ignores_repo_ignored_paths(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + class Proc: + returncode = 128 + stdout = "" + + monkeypatch.setattr(state_runtime.subprocess, "run", lambda *args, **kwargs: Proc()) + monkeypatch.setattr(state_runtime, "_NON_GIT_SNAPSHOT_BY_REPO", {}) + assert state_runtime.git_changed_files(tmp_path) == [] + (tmp_path / ".git").mkdir() + (tmp_path / ".git" / "config").write_text("x", encoding="utf-8") + assert state_runtime.git_changed_files(tmp_path) == [] + + +def test_git_changed_files_non_git_returns_posix_relative_paths(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + class Proc: + returncode = 128 + stdout = "" + + monkeypatch.setattr(state_runtime.subprocess, "run", lambda *args, **kwargs: Proc()) + monkeypatch.setattr(state_runtime, "_NON_GIT_SNAPSHOT_BY_REPO", {}) + assert state_runtime.git_changed_files(tmp_path) == [] + nested = tmp_path / "a" / "b" + nested.mkdir(parents=True) + (nested / "c.txt").write_text("x", encoding="utf-8") + assert state_runtime.git_changed_files(tmp_path) == ["a/b/c.txt"] + + def test_repeated_validation_updates_compact_state_without_repeated_prose( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: diff --git a/tests/test_state_tooling_mutation_policy.py b/tests/test_state_tooling_mutation_policy.py index ad18ea3e..9dfc60c2 100644 --- a/tests/test_state_tooling_mutation_policy.py +++ b/tests/test_state_tooling_mutation_policy.py @@ -1,5 +1,6 @@ from __future__ import annotations +import json from pathlib import Path from villani_code.state import Runner @@ -126,3 +127,61 @@ def test_patch_existing_file_rewrite_heavy_is_rejected(tmp_path: Path) -> None: result = execute_tool_with_policy(runner, "Patch", {"unified_diff": "\n".join(diff_lines)}, "1", 0) assert result["is_error"] is True assert "Rewrite-heavy mutation rejected" in str(result["content"]) + + +def test_failed_bash_command_is_blocked_on_immediate_retry(tmp_path: Path) -> None: + runner = _runner(tmp_path) + command = "python -c \"import sys; sys.exit(1)\"" + first = execute_tool_with_policy(runner, "Bash", {"command": command}, "1", 0) + assert first["is_error"] is False + assert json.loads(str(first["content"]))["exit_code"] != 0 + second = execute_tool_with_policy(runner, "Bash", {"command": command}, "2", 0) + assert second["is_error"] is True + assert "already failed" in str(second["content"]) + + +def test_failed_bash_retry_block_normalizes_whitespace(tmp_path: Path) -> None: + runner = _runner(tmp_path) + first = execute_tool_with_policy(runner, "Bash", {"command": " python -c \"import sys; sys.exit(1)\" "}, "1", 0) + assert json.loads(str(first["content"]))["exit_code"] != 0 + second = execute_tool_with_policy(runner, "Bash", {"command": "python -c \"import sys; sys.exit(1)\""}, "2", 0) + assert second["is_error"] is True + assert "already failed" in str(second["content"]) + + +def test_failed_bash_retry_block_is_case_insensitive_on_windows(tmp_path: Path) -> None: + runner = _runner(tmp_path) + runner._shell_retry_case_insensitive = True + first = execute_tool_with_policy(runner, "Bash", {"command": "PyThOn -c \"import sys; sys.exit(1)\""}, "1", 0) + assert json.loads(str(first["content"]))["exit_code"] != 0 + second = execute_tool_with_policy(runner, "Bash", {"command": "python -C \"import sys; sys.exit(1)\""}, "2", 0) + assert second["is_error"] is True + + +def test_materially_different_bash_command_is_allowed_after_failure(tmp_path: Path) -> None: + runner = _runner(tmp_path) + failed = execute_tool_with_policy(runner, "Bash", {"command": "python -c \"import sys; sys.exit(1)\""}, "1", 0) + assert json.loads(str(failed["content"]))["exit_code"] != 0 + allowed = execute_tool_with_policy(runner, "Bash", {"command": "python -c \"print('ok')\""}, "2", 0) + assert allowed["is_error"] is False + assert json.loads(str(allowed["content"]))["exit_code"] == 0 + + +def test_successful_bash_command_clears_retry_block(tmp_path: Path) -> None: + runner = _runner(tmp_path) + command = "python -c \"import sys; sys.exit(1)\"" + failed = execute_tool_with_policy(runner, "Bash", {"command": command}, "1", 0) + assert json.loads(str(failed["content"]))["exit_code"] != 0 + success = execute_tool_with_policy(runner, "Bash", {"command": "python -c \"print('reset')\""}, "2", 0) + assert json.loads(str(success["content"]))["exit_code"] == 0 + retry = execute_tool_with_policy(runner, "Bash", {"command": command}, "3", 0) + assert retry["is_error"] is False + assert json.loads(str(retry["content"]))["exit_code"] != 0 + + +def test_non_shell_tools_are_unaffected_by_failed_shell_retry_memory(tmp_path: Path) -> None: + runner = _runner(tmp_path) + _ = execute_tool_with_policy(runner, "Bash", {"command": "python -c \"import sys; sys.exit(1)\""}, "1", 0) + read = execute_tool_with_policy(runner, "Read", {"file_path": "missing.txt"}, "2", 0) + assert read["is_error"] is True + assert "No such file" in str(read["content"]) diff --git a/villani_code/state.py b/villani_code/state.py index d9f5a5bc..e56da96f 100644 --- a/villani_code/state.py +++ b/villani_code/state.py @@ -496,6 +496,7 @@ def __init__( self._no_progress_cycles = 0 self._recovery_count = 0 self._last_failed_tool_sig = "" + self._last_failed_shell_command: dict[str, Any] | None = None self._repo_map = "" self._retriever: Retriever | None = None self._context_budget = ( @@ -870,7 +871,9 @@ def run( ) initial_read_enforced = True self._save_session_snapshot(messages) - empty_turn_retries = 0 + consecutive_empty_assistant_turns = 0 + empty_recovery_injected = False + last_successful_concrete_action = "none yet" start = time.monotonic() turns_used = 0 tool_calls_used = 1 if initial_read_enforced else 0 @@ -986,6 +989,30 @@ def _has_meaningful_benchmark_edit() -> bool: ] return bool(meaningful) + def _assistant_text(blocks: list[dict[str, Any]]) -> str: + return "\n".join( + str(block.get("text", "")) + for block in blocks + if isinstance(block, dict) and block.get("type") == "text" + ) + + def _has_meaningful_assistant_text(blocks: list[dict[str, Any]]) -> bool: + return bool(_assistant_text(blocks).strip()) + + def _build_empty_turn_recovery_message() -> str: + files_changed = _attributed_changed_files() + files_fragment = ", ".join(files_changed[:4]) if files_changed else "none yet" + if len(files_changed) > 4: + files_fragment = f"{files_fragment}, +{len(files_changed) - 4} more" + return ( + "RECOVERY: blank turns detected. " + f"Objective: {instruction.strip() or 'complete the assigned mission'}. " + f"Last concrete action: {last_successful_concrete_action}. " + f"Changed files: {files_fragment}. " + "Missing milestone: next verifiable step toward completion. " + "Take exactly one concrete next action now and continue." + ) + def _finish_bounded( response: dict[str, Any], reason: str, completed: bool ) -> dict[str, Any]: @@ -1117,33 +1144,45 @@ def _budget_reason( ) turns_used += 1 - tool_uses = [ - b for b in response.get("content", []) if b.get("type") == "tool_use" - ] - empty = is_effectively_empty_content(response.get("content", [])) - if not tool_uses and empty and empty_turn_retries < 2: - empty_turn_retries += 1 - messages.append( - { - "role": "user", - "content": [ - { - "type": "text", - "text": "Continue. You ended your previous turn with no output. Resume the task from where you left off and either call the next tool or provide the next part of the answer.", - } - ], - } - ) + content_blocks = response.get("content", []) + tool_uses = [b for b in content_blocks if b.get("type") == "tool_use"] + has_meaningful_text = _has_meaningful_assistant_text(content_blocks) + empty = is_effectively_empty_content(content_blocks) + empty_assistant_turn = not tool_uses and not has_meaningful_text + if empty_assistant_turn: + consecutive_empty_assistant_turns += 1 + if empty_recovery_injected: + return _finish_bounded(response, "stalled_empty_responses", False) + if consecutive_empty_assistant_turns >= 2: + messages.append( + { + "role": "user", + "content": [ + {"type": "text", "text": _build_empty_turn_recovery_message()} + ], + } + ) + empty_recovery_injected = True + reason = _budget_reason() + if reason: + return _finish_bounded(response, reason, reason == "completed") + continue reason = _budget_reason() if reason: return _finish_bounded(response, reason, reason == "completed") continue - if tool_uses or not empty: - empty_turn_retries = 0 + else: + consecutive_empty_assistant_turns = 0 + empty_recovery_injected = False + if tool_uses: + last_successful_concrete_action = ( + f"assistant emitted {len(tool_uses)} tool call(s)" + ) + elif has_meaningful_text: + last_successful_concrete_action = "assistant produced substantive text output" if benchmark_forced_read_no_progress_guard_active and tool_uses: benchmark_forced_read_no_progress_guard_active = False if not tool_uses: - content_blocks = response.get("content", []) only_textual_response = bool(content_blocks) and all( isinstance(block, dict) and block.get("type") == "text" for block in content_blocks @@ -1487,6 +1526,11 @@ def _budget_reason( self._no_progress_cycles = 0 self._recovery_count = 0 self._last_failed_tool_sig = "" + consecutive_empty_assistant_turns = 0 + empty_recovery_injected = False + last_successful_concrete_action = ( + f"executed tool(s): {', '.join(str(b.get('name', '')) for b in tool_uses)}" + ) else: sig = "|".join(f"{b.get('name')}:{b.get('input')}" for b in tool_uses) if sig and sig == self._last_failed_tool_sig: diff --git a/villani_code/state_runtime.py b/villani_code/state_runtime.py index 9e03824d..463f1a82 100644 --- a/villani_code/state_runtime.py +++ b/villani_code/state_runtime.py @@ -29,6 +29,7 @@ _DIAGNOSIS_KEYS = ("target_file", "bug_class", "fix_intent") +_NON_GIT_SNAPSHOT_BY_REPO: dict[str, dict[str, tuple[int, int]]] = {} def _user_message_is_safe_for_text_injection(message: dict[str, Any]) -> bool: @@ -606,9 +607,48 @@ def truncate_tool_result(tool_name: str, result: dict[str, Any]) -> dict[str, An return result +def _snapshot_workspace_files(repo: Path) -> dict[str, tuple[int, int]]: + snapshot: dict[str, tuple[int, int]] = {} + for current_root, dirs, files in repo.walk(): + rel_root = current_root.relative_to(repo).as_posix() + if rel_root != "." and is_ignored_repo_path(rel_root): + dirs[:] = [] + continue + dirs[:] = [ + name + for name in dirs + if not is_ignored_repo_path(f"{rel_root}/{name}" if rel_root != "." else name) + ] + for name in files: + rel = f"{rel_root}/{name}" if rel_root != "." else name + rel = rel.replace("\\", "/") + if is_ignored_repo_path(rel): + continue + file_path = current_root / name + try: + stat = file_path.stat() + except FileNotFoundError: + continue + snapshot[rel] = (stat.st_size, stat.st_mtime_ns) + return snapshot + + def git_changed_files(repo: Any) -> list[str]: proc = subprocess.run(["git", "status", "--short"], cwd=repo, capture_output=True, text=True) - return [line[3:].strip() for line in proc.stdout.splitlines() if line.strip()] + if proc.returncode == 0: + return [line[3:].strip() for line in proc.stdout.splitlines() if line.strip()] + + repo_path = Path(repo).resolve() + repo_key = str(repo_path) + current = _snapshot_workspace_files(repo_path) + previous = _NON_GIT_SNAPSHOT_BY_REPO.get(repo_key) + _NON_GIT_SNAPSHOT_BY_REPO[repo_key] = current + if previous is None: + return [] + added = set(current) - set(previous) + deleted = set(previous) - set(current) + modified = {path for path in set(current) & set(previous) if current[path] != previous[path]} + return sorted(added | deleted | modified) diff --git a/villani_code/state_tooling.py b/villani_code/state_tooling.py index c84c64ca..4afef8a9 100644 --- a/villani_code/state_tooling.py +++ b/villani_code/state_tooling.py @@ -1,6 +1,8 @@ from __future__ import annotations import difflib +import json +import os import py_compile import re from dataclasses import dataclass @@ -85,6 +87,62 @@ def _extract_literal_content_from_payload(content: str) -> str: return content +def _normalize_shell_command_for_retry(command: str, *, windows_case_insensitive: bool) -> str: + normalized = str(command or "").replace("\r\n", "\n").replace("\r", "\n") + normalized = " ".join(normalized.split()) + if windows_case_insensitive: + normalized = normalized.casefold() + return normalized + + +def _should_block_failed_shell_retry(runner: Any, tool_name: str, tool_input: dict[str, Any]) -> bool: + if tool_name != "Bash": + return False + last_failed = getattr(runner, "_last_failed_shell_command", None) + if not isinstance(last_failed, dict) or not last_failed.get("failed"): + return False + windows_case_insensitive = bool( + getattr(runner, "_shell_retry_case_insensitive", os.name == "nt") + ) + new_normalized = _normalize_shell_command_for_retry( + str(tool_input.get("command", "")), + windows_case_insensitive=windows_case_insensitive, + ) + return new_normalized != "" and new_normalized == str(last_failed.get("normalized", "")) + + +def _record_shell_retry_state(runner: Any, tool_name: str, tool_input: dict[str, Any], result: dict[str, Any]) -> None: + if tool_name != "Bash": + return + command = str(tool_input.get("command", "")) + windows_case_insensitive = bool( + getattr(runner, "_shell_retry_case_insensitive", os.name == "nt") + ) + normalized = _normalize_shell_command_for_retry( + command, + windows_case_insensitive=windows_case_insensitive, + ) + failed = bool(result.get("is_error", False)) + stderr_summary = "" + if not failed: + try: + payload = json.loads(str(result.get("content", ""))) + except Exception: + payload = {} + if isinstance(payload, dict): + failed = int(payload.get("exit_code", 0) or 0) != 0 + stderr_summary = str(payload.get("stderr", "")).strip().splitlines()[0][:160] if payload.get("stderr") else "" + if failed: + runner._last_failed_shell_command = { + "command": command, + "normalized": normalized, + "failed": True, + "stderr_summary": stderr_summary, + } + else: + runner._last_failed_shell_command = None + + def _analyze_text_rewrite( path: str, existed: bool, before_text: str, after_text: str, thresholds: MutationGuardThresholds ) -> MutationEditAnalysis: @@ -493,6 +551,11 @@ def execute_tool_with_policy( runner._tighten_tool_input(tool_name, tool_input) if tool_name in {"Write", "Patch"}: _normalize_mutation_payload(tool_name, tool_input) + if _should_block_failed_shell_retry(runner, tool_name, tool_input): + return { + "content": "This shell command already failed in the previous attempt. Choose a materially different next action instead of retrying verbatim.", + "is_error": True, + } policy = runner.permissions.evaluate_with_reason( tool_name, @@ -666,6 +729,7 @@ def _debug_callback_with_turn(event_type: str, payload: dict[str, Any]) -> None: debug_callback=_debug_callback_with_turn, tool_call_id=stable_tool_use_id, ) + _record_shell_retry_state(runner, tool_name, tool_input, result) runner.event_callback( { "type": "tool_result",