diff --git a/openhands-sdk/openhands/sdk/agent/critic_mixin.py b/openhands-sdk/openhands/sdk/agent/critic_mixin.py index 6473c1284b..cc4c787811 100644 --- a/openhands-sdk/openhands/sdk/agent/critic_mixin.py +++ b/openhands-sdk/openhands/sdk/agent/critic_mixin.py @@ -2,6 +2,7 @@ from __future__ import annotations +import difflib from typing import TYPE_CHECKING from openhands.sdk.critic.base import CriticResult @@ -59,10 +60,10 @@ def _evaluate_with_critic( llm_convertible_events = [ e for e in events if isinstance(e, LLMConvertibleEvent) ] + git_patch = self._build_git_patch_for_critic(conversation) - # Evaluate without git_patch for now critic_result = self.critic.evaluate( - events=llm_convertible_events, git_patch=None + events=llm_convertible_events, git_patch=git_patch ) logger.info( f"✓ Critic evaluation: score={critic_result.score:.3f}, " @@ -73,6 +74,48 @@ def _evaluate_with_critic( logger.error(f"✗ Critic evaluation failed: {e}", exc_info=True) return None + def _build_git_patch_for_critic( + self, conversation: LocalConversation + ) -> str | None: + """Build a best-effort git patch for critic evaluation.""" + workspace = conversation.state.workspace + try: + changes = workspace.git_changes(".") + except Exception as e: + logger.debug(f"Unable to collect git changes for critic: {e}") + return None + + if not changes: + return None + + patch_parts: list[str] = [] + for change in changes: + try: + diff = workspace.git_diff(change.path) + except Exception as e: + logger.debug( + f"Unable to collect git diff for critic path {change.path}: {e}" + ) + continue + + path = str(change.path).replace("\\", "/") + original = diff.original or "" + modified = diff.modified or "" + file_patch = list( + difflib.unified_diff( + original.splitlines(keepends=True), + modified.splitlines(keepends=True), + fromfile=f"a/{path}", + tofile=f"b/{path}", + ) + ) + if file_patch and not file_patch[-1].endswith(("\n", "\r")): + file_patch[-1] += "\n" + patch_parts.extend(file_patch) + + git_patch = "".join(patch_parts) + return git_patch or None + def _check_iterative_refinement( self, conversation: LocalConversation, action_event: ActionEvent ) -> tuple[bool, str | None]: diff --git a/tests/sdk/agent/test_iterative_refinement.py b/tests/sdk/agent/test_iterative_refinement.py index a10faf4544..83fbab2c64 100644 --- a/tests/sdk/agent/test_iterative_refinement.py +++ b/tests/sdk/agent/test_iterative_refinement.py @@ -1,6 +1,7 @@ """Tests for iterative refinement functionality in CriticMixin.""" import json +from pathlib import Path from unittest.mock import MagicMock import pytest @@ -16,6 +17,7 @@ ) from openhands.sdk.critic.impl.api import APIBasedCritic from openhands.sdk.event import ActionEvent +from openhands.sdk.git.models import GitChange, GitChangeStatus, GitDiff from openhands.sdk.llm import MessageToolCall, TextContent from openhands.sdk.tool.builtins.finish import FinishAction @@ -27,6 +29,72 @@ def evaluate(self, events, git_patch=None): return CriticResult(score=0.5, message="Mock evaluation") +class PatchAwareCritic(MockCritic): + """Mock critic that scores based on the git patch it receives.""" + + def evaluate(self, events, git_patch=None): + expected_lines = [ + "--- a/foo.py", + "+++ b/foo.py", + "-print('old')", + "+print('new')", + ] + if git_patch and all(line in git_patch for line in expected_lines): + return CriticResult(score=1.0, message="Patch received") + return CriticResult(score=0.0, message="Patch missing") + + +class WorkspaceWithChange: + """Workspace fake with one changed file.""" + + def git_changes(self, path): + return [GitChange(status=GitChangeStatus.UPDATED, path=Path("foo.py"))] + + def git_diff(self, path): + return GitDiff( + original="print('old')\n", + modified="print('new')\n", + ) + + +class WorkspaceWithoutChanges: + """Workspace fake with no git changes.""" + + def git_changes(self, path): + return [] + + def git_diff(self, path): + raise AssertionError("git_diff should not be called without changes") + + +class WorkspaceFailingChanges: + """Workspace fake that fails while collecting git changes.""" + + def git_changes(self, path): + raise RuntimeError("not a git repository") + + def git_diff(self, path): + raise AssertionError("git_diff should not be called after git_changes fails") + + +class WorkspaceWithPartialDiffFailure: + """Workspace fake where one changed file cannot be diffed.""" + + def git_changes(self, path): + return [ + GitChange(status=GitChangeStatus.UPDATED, path=Path("foo.py")), + GitChange(status=GitChangeStatus.UPDATED, path=Path("bar.py")), + ] + + def git_diff(self, path): + if Path(path) == Path("bar.py"): + raise RuntimeError("cannot diff bar.py") + return GitDiff( + original="print('old')\n", + modified="print('new')\n", + ) + + class MockCriticMixin(CriticMixin): """Concrete implementation of CriticMixin for testing.""" @@ -326,6 +394,62 @@ def test_multiple_iterations(self): assert should_continue is False +def test_evaluate_with_critic_passes_workspace_git_patch(): + """Critic evaluation should receive a patch when workspace has changes.""" + critic = PatchAwareCritic() + mixin = MockCriticMixin(critic=critic) + conversation = create_mock_conversation() + conversation.state.events = [] + + conversation.state.workspace = WorkspaceWithChange() + + result = mixin._evaluate_with_critic(conversation, create_finish_action_event()) + + assert result == CriticResult(score=1.0, message="Patch received") + + +def test_evaluate_with_critic_runs_without_git_changes(): + """Critic evaluation should still run when no patch is available.""" + critic = PatchAwareCritic() + mixin = MockCriticMixin(critic=critic) + conversation = create_mock_conversation() + conversation.state.events = [] + + conversation.state.workspace = WorkspaceWithoutChanges() + + result = mixin._evaluate_with_critic(conversation, create_finish_action_event()) + + assert result == CriticResult(score=0.0, message="Patch missing") + + +def test_evaluate_with_critic_runs_when_git_changes_fails(): + """Critic evaluation should still run if patch collection fails.""" + critic = PatchAwareCritic() + mixin = MockCriticMixin(critic=critic) + conversation = create_mock_conversation() + conversation.state.events = [] + + conversation.state.workspace = WorkspaceFailingChanges() + + result = mixin._evaluate_with_critic(conversation, create_finish_action_event()) + + assert result == CriticResult(score=0.0, message="Patch missing") + + +def test_evaluate_with_critic_uses_available_diffs_when_one_file_fails(): + """Critic evaluation should receive available diffs if one file fails.""" + critic = PatchAwareCritic() + mixin = MockCriticMixin(critic=critic) + conversation = create_mock_conversation() + conversation.state.events = [] + + conversation.state.workspace = WorkspaceWithPartialDiffFailure() + + result = mixin._evaluate_with_critic(conversation, create_finish_action_event()) + + assert result == CriticResult(score=1.0, message="Patch received") + + class TestShouldEvaluateWithCritic: """Tests for _should_evaluate_with_critic method."""