fix(sdk): pass workspace git patch to critics

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]:

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."""