Merge branch 'main' into openhands-fix-issue-8199

openhands/resolver/interfaces/issue_definitions.py

@@ -18,7 +18,7 @@ from openhands.resolver.utils import extract_image_urls
 
 class ServiceContext:
     issue_type: ClassVar[str]
-    default_git_patch: ClassVar[str] = 'No changes made yet'
+    default_git_patch: ClassVar[str] = 'No code changes were made or needed'
 
     def __init__(self, strategy: IssueHandlerInterface, llm_config: LLMConfig | None):
         self._strategy = strategy
@@ -362,7 +362,7 @@ class ServiceContextIssue(ServiceContext):
         Args:
             issue: The issue to check
             history: The agent's history
-            git_patch: Optional git patch showing the changes made
+            git_patch: Optional git patch showing the changes made. If None, indicates no code changes were needed.
         """
         last_message = history[-1].message
         # Include thread comments in the prompt if they exist

openhands/resolver/send_pull_request.py

@@ -158,67 +158,79 @@ def initialize_repo(
     return dest_dir
 
 
-def make_commit(repo_dir: str, issue: Issue, issue_type: str) -> None:
+def make_commit(repo_dir: str, issue: Issue, issue_type: str) -> bool:
     """Make a commit with the changes to the repository.
 
     Args:
         repo_dir: The directory containing the repository
         issue: The issue to fix
         issue_type: The type of the issue
+
+    Returns:
+        bool: True if changes were committed, False if there were no changes to commit
     """
-    # Check if git username is set
-    result = subprocess.run(
-        f'git -C {repo_dir} config user.name',
-        shell=True,
-        capture_output=True,
-        text=True,
-    )
-
-    if not result.stdout.strip():
-        # If username is not set, configure git
-        subprocess.run(
-            f'git -C {repo_dir} config user.name "openhands" && '
-            f'git -C {repo_dir} config user.email "openhands@all-hands.dev" && '
-            f'git -C {repo_dir} config alias.git "git --no-pager"',
+    try:
+        # Check if git username is set
+        result = subprocess.run(
+            f'git -C {repo_dir} config user.name',
             shell=True,
-            check=True,
+            capture_output=True,
+            text=True,
         )
-        logger.info('Git user configured as openhands')
 
-    # Add all changes to the git index
-    result = subprocess.run(
-        f'git -C {repo_dir} add .', shell=True, capture_output=True, text=True
-    )
-    if result.returncode != 0:
-        logger.error(f'Error adding files: {result.stderr}')
-        raise RuntimeError('Failed to add files to git')
+        if not result.stdout.strip():
+            # If username is not set, configure git
+            subprocess.run(
+                f'git -C {repo_dir} config user.name "openhands" && '
+                f'git -C {repo_dir} config user.email "openhands@all-hands.dev" && '
+                f'git -C {repo_dir} config alias.git "git --no-pager"',
+                shell=True,
+            )
+            logger.info('Git user configured as openhands')
 
-    # Check the status of the git index
-    status_result = subprocess.run(
-        f'git -C {repo_dir} status --porcelain',
-        shell=True,
-        capture_output=True,
-        text=True,
-    )
-
-    # If there are no changes, raise an error
-    if not status_result.stdout.strip():
-        logger.error(
-            f'No changes to commit for issue #{issue.number}. Skipping commit.'
+        # Add all changes to the git index
+        result = subprocess.run(
+            f'git -C {repo_dir} add .', shell=True, capture_output=True, text=True
         )
-        raise RuntimeError('ERROR: Openhands failed to make code changes.')
+        if result.returncode != 0:
+            logger.error(f'Error adding files: {result.stderr}')
+            return False
 
-    # Prepare the commit message
-    commit_message = f'Fix {issue_type} #{issue.number}: {issue.title}'
+        # Check the status of the git index
+        status_result = subprocess.run(
+            f'git -C {repo_dir} status --porcelain',
+            shell=True,
+            capture_output=True,
+            text=True,
+        )
+        if status_result.returncode != 0:
+            logger.error(f'Error checking git status: {status_result.stderr}')
+            return False
 
-    # Commit the changes
-    result = subprocess.run(
-        ['git', '-C', repo_dir, 'commit', '-m', commit_message],
-        capture_output=True,
-        text=True,
-    )
-    if result.returncode != 0:
-        raise RuntimeError(f'Failed to commit changes: {result}')
+        # If there are no changes, log it and return False
+        if not status_result.stdout.strip():
+            logger.info(
+                f'No changes to commit for issue #{issue.number}. Skipping commit.'
+            )
+            return False
+
+        # Prepare the commit message
+        commit_message = f'Fix {issue_type} #{issue.number}: {issue.title}'
+
+        # Commit the changes
+        result = subprocess.run(
+            ['git', '-C', repo_dir, 'commit', '-m', commit_message],
+            capture_output=True,
+            text=True,
+        )
+        if result.returncode != 0:
+            logger.error(f'Failed to commit changes: {result}')
+            return False
+
+        return True
+    except Exception as e:
+        logger.error(f'Error during git operations: {e}')
+        return False
 
 
 def send_pull_request(
@@ -518,7 +530,39 @@ def process_single_issue(
 
     apply_patch(patched_repo_dir, resolver_output.git_patch)
 
-    make_commit(patched_repo_dir, resolver_output.issue, issue_type)
+    has_changes = make_commit(patched_repo_dir, resolver_output.issue, issue_type)
+
+    # If there are no changes, we still want to post a comment with the result explanation
+    if not has_changes:
+        # Create a handler to post comments for both issues and PRs
+        handler = (
+            ServiceContextIssue(
+                GithubIssueHandler(
+                    resolver_output.issue.owner,
+                    resolver_output.issue.repo,
+                    token,
+                    username,
+                    base_domain,
+                ),
+                llm_config,
+            )
+            if platform == ProviderType.GITHUB
+            else ServiceContextIssue(
+                GitlabIssueHandler(
+                    resolver_output.issue.owner,
+                    resolver_output.issue.repo,
+                    token,
+                    username,
+                    base_domain,
+                ),
+                llm_config,
+            )
+        )
+        if resolver_output.result_explanation:
+            handler.send_comment_msg(
+                resolver_output.issue.number, resolver_output.result_explanation
+            )
+        return
 
     if issue_type == 'pr':
         update_existing_pull_request(

tests/unit/resolver/github/test_send_pull_request.py

@@ -1,4 +1,5 @@
 import os
+import shutil
 import tempfile
 from unittest.mock import ANY, MagicMock, call, patch
 
@@ -440,6 +441,46 @@ def test_send_pull_request(
         assert post_data['draft'] == (pr_type == 'draft')
 
 
+def test_make_commit_failed_add(mock_output_dir, mock_issue):
+    """Test that make_commit returns False when git add fails."""
+    # Create a test file
+    test_file = os.path.join(mock_output_dir, 'test.txt')
+    with open(test_file, 'w') as f:
+        f.write('test content')
+
+    # Initialize git repo
+    os.system(f'git init {mock_output_dir}')
+
+    # Mock a failed add by making the directory not a git repo
+    shutil.rmtree(os.path.join(mock_output_dir, '.git'))
+
+    # Try to make a commit
+    result = make_commit(mock_output_dir, mock_issue, 'issue')
+
+    # Assert that the function returned False
+    assert result is False
+
+
+def test_make_commit_failed_commit(mock_output_dir, mock_issue):
+    """Test that make_commit returns False when git commit fails."""
+    # Create a test file
+    test_file = os.path.join(mock_output_dir, 'test.txt')
+    with open(test_file, 'w') as f:
+        f.write('test content')
+
+    # Initialize git repo
+    os.system(f'git init {mock_output_dir}')
+
+    # Mock a failed commit by making the directory not a git repo
+    shutil.rmtree(os.path.join(mock_output_dir, '.git'))
+
+    # Try to make a commit
+    result = make_commit(mock_output_dir, mock_issue, 'issue')
+
+    # Assert that the function returned False
+    assert result is False
+
+
 @patch('subprocess.run')
 @patch('httpx.post')
 @patch('httpx.get')
@@ -1025,6 +1066,137 @@ def test_process_single_issue_unsuccessful(
     mock_initialize_repo.assert_not_called()
     mock_apply_patch.assert_not_called()
     mock_make_commit.assert_not_called()
+
+
+@patch('openhands.resolver.send_pull_request.initialize_repo')
+@patch('openhands.resolver.send_pull_request.apply_patch')
+@patch('openhands.resolver.send_pull_request.update_existing_pull_request')
+@patch('openhands.resolver.send_pull_request.make_commit')
+@patch('openhands.resolver.interfaces.github.GithubIssueHandler.send_comment_msg')
+def test_process_single_pr_no_changes(
+    mock_send_comment_msg,
+    mock_make_commit,
+    mock_update_existing_pull_request,
+    mock_apply_patch,
+    mock_initialize_repo,
+    mock_output_dir,
+    mock_llm_config,
+):
+    """Test that process_single_issue handles PR with no changes correctly."""
+    # Setup
+    resolver_output = ResolverOutput(
+        issue=Issue(
+            owner='test-owner',
+            repo='test-repo',
+            number=1,
+            title='Test PR',
+            body='Test body',
+            head_branch='branch 1',
+        ),
+        success=True,
+        git_patch='',
+        issue_type='pr',
+        base_commit='def456',
+        result_explanation='No changes needed',
+        history=[],
+        metrics={},
+        instruction='Test instruction',
+        comment_success=None,
+        error=None,
+    )
+    token = 'test-token'
+    username = 'test-user'
+    platform = ProviderType.GITHUB
+
+    # Mock make_commit to return False (no changes)
+    mock_make_commit.return_value = False
+
+    # Mock initialize_repo to return the expected path
+    expected_path = f'{mock_output_dir}/patches/pr_1'
+    mock_initialize_repo.return_value = expected_path
+
+    # Call the function
+    process_single_issue(
+        mock_output_dir,
+        resolver_output,
+        token,
+        username,
+        platform,
+        'ready',
+        mock_llm_config,
+        None,
+        False,
+    )
+
+    # Assert that the mocked functions were called correctly
+    mock_initialize_repo.assert_called_once_with(mock_output_dir, 1, 'pr', 'branch 1')
+    mock_apply_patch.assert_called_once_with(expected_path, resolver_output.git_patch)
+    mock_make_commit.assert_called_once_with(expected_path, resolver_output.issue, 'pr')
+    mock_update_existing_pull_request.assert_not_called()
+    mock_send_comment_msg.assert_called_once_with(1, 'No changes needed')
+
+
+@patch('openhands.resolver.send_pull_request.initialize_repo')
+@patch('openhands.resolver.send_pull_request.apply_patch')
+@patch('openhands.resolver.send_pull_request.send_pull_request')
+@patch('openhands.resolver.send_pull_request.make_commit')
+def test_process_single_issue_no_changes(
+    mock_make_commit,
+    mock_send_pull_request,
+    mock_apply_patch,
+    mock_initialize_repo,
+    mock_output_dir,
+    mock_llm_config,
+):
+    """Test that process_single_issue handles issue with no changes correctly."""
+    # Setup
+    resolver_output = ResolverOutput(
+        issue=Issue(
+            owner='test-owner',
+            repo='test-repo',
+            number=1,
+            title='Test Issue',
+            body='Test body',
+        ),
+        success=True,
+        git_patch='',
+        issue_type='issue',
+        base_commit='def456',
+        result_explanation='No changes needed',
+        history=[],
+        metrics={},
+        instruction='Test instruction',
+        comment_success=None,
+        error=None,
+    )
+    token = 'test-token'
+    username = 'test-user'
+    platform = ProviderType.GITHUB
+
+    # Mock make_commit to return False (no changes)
+    mock_make_commit.return_value = False
+
+    # Mock initialize_repo to return the expected path
+    expected_path = f'{mock_output_dir}/patches/issue_1'
+    mock_initialize_repo.return_value = expected_path
+
+    # Call the function
+    process_single_issue(
+        mock_output_dir,
+        resolver_output,
+        token,
+        username,
+        platform,
+        'ready',
+        mock_llm_config,
+        None,
+        False,
+    )
+
+    # Assert that the mocked functions were called correctly
+    mock_initialize_repo.assert_called_once_with(mock_output_dir, 1, 'issue', 'def456')
+    mock_apply_patch.assert_called_once_with(expected_path, resolver_output.git_patch)
+    mock_make_commit.assert_called_once_with(expected_path, resolver_output.issue, 'issue')
     mock_send_pull_request.assert_not_called()
 
 
@@ -1197,18 +1369,22 @@ def test_make_commit_escapes_issue_title(mock_subprocess_run):
         body='Test body',
     )
 
-    # Mock subprocess.run to return success for all calls
-    mock_subprocess_run.return_value = MagicMock(
-        returncode=0, stdout='sample output', stderr=''
-    )
+    # Mock subprocess.run to simulate changes in the repo
+    mock_subprocess_run.side_effect = [
+        MagicMock(returncode=0, stdout='openhands'),  # git config check
+        MagicMock(returncode=0),  # git add
+        MagicMock(returncode=0, stdout='M file1.txt\nA file2.txt'),  # git status --porcelain (has changes)
+        MagicMock(returncode=0),  # git commit
+    ]
 
     # Call the function
     issue_type = 'issue'
-    make_commit(repo_dir, issue, issue_type)
+    result = make_commit(repo_dir, issue, issue_type)
+    assert result is True
 
     # Assert that subprocess.run was called with the correct arguments
     calls = mock_subprocess_run.call_args_list
-    assert len(calls) == 4  # git config check, git add, git commit
+    assert len(calls) == 4  # git config check, git add, git status, git commit
 
     # Check the git commit call
     git_commit_call = calls[3][0][0]
@@ -1239,15 +1415,23 @@ def test_make_commit_no_changes(mock_subprocess_run):
 
     # Mock subprocess.run to simulate no changes in the repo
     mock_subprocess_run.side_effect = [
-        MagicMock(returncode=0),
-        MagicMock(returncode=0),
-        MagicMock(returncode=1, stdout=''),  # git status --porcelain (no changes)
+        MagicMock(returncode=0, stdout='openhands'),  # git config check
+        MagicMock(returncode=0),  # git add
+        MagicMock(returncode=0, stdout=''),  # git status --porcelain (no changes)
     ]
 
-    with pytest.raises(
-        RuntimeError, match='ERROR: Openhands failed to make code changes.'
-    ):
-        make_commit(repo_dir, issue, 'issue')
+    # Should not raise an error and return False
+    result = make_commit(repo_dir, issue, 'issue')
+    assert result is False
+
+    # Verify that git commit was not called
+    assert len(mock_subprocess_run.call_args_list) == 3  # git config check, git add, git status
+
+    # Check the specific calls
+    calls = mock_subprocess_run.call_args_list
+    assert calls[0][0][0] == f'git -C {repo_dir} config user.name'  # git config check
+    assert calls[1][0][0] == f'git -C {repo_dir} add .'  # git add
+    assert calls[2][0][0] == f'git -C {repo_dir} status --porcelain'  # git status
 
     # Check that subprocess.run was called for checking git status and add, but not commit
     assert mock_subprocess_run.call_count == 3

tests/unit/resolver/gitlab/test_gitlab_send_pull_request.py

@@ -1141,22 +1141,89 @@ def test_make_commit_no_changes(mock_subprocess_run):
 
     # Mock subprocess.run to simulate no changes in the repo
     mock_subprocess_run.side_effect = [
-        MagicMock(returncode=0),
-        MagicMock(returncode=0),
-        MagicMock(returncode=1, stdout=''),  # git status --porcelain (no changes)
+        MagicMock(returncode=0),  # git config user.name
+        MagicMock(returncode=0),  # git add .
+        MagicMock(returncode=0, stdout=''),  # git status --porcelain (no changes)
     ]
 
-    with pytest.raises(
-        RuntimeError, match='ERROR: Openhands failed to make code changes.'
-    ):
-        make_commit(repo_dir, issue, 'issue')
-
-    # Check that subprocess.run was called for checking git status and add, but not commit
+    # Call make_commit and verify it returns False when there are no changes
+    result = make_commit(repo_dir, issue, 'issue')
+    assert result is False
     assert mock_subprocess_run.call_count == 3
     git_status_call = mock_subprocess_run.call_args_list[2][0][0]
     assert f'git -C {repo_dir} status --porcelain' in git_status_call
 
 
+@patch('openhands.resolver.send_pull_request.initialize_repo')
+@patch('openhands.resolver.send_pull_request.apply_patch')
+@patch('openhands.resolver.send_pull_request.send_pull_request')
+@patch('openhands.resolver.send_pull_request.make_commit')
+@patch('openhands.resolver.interfaces.gitlab.GitlabIssueHandler.send_comment_msg')
+def test_process_single_issue_no_changes(
+    mock_send_comment_msg,
+    mock_make_commit,
+    mock_send_pull_request,
+    mock_apply_patch,
+    mock_initialize_repo,
+    mock_output_dir,
+    mock_llm_config,
+):
+    """Test that process_single_issue handles issue with no changes correctly."""
+    # Setup
+    resolver_output = ResolverOutput(
+        issue=Issue(
+            owner='test-owner',
+            repo='test-repo',
+            number=1,
+            title='Test Issue',
+            body='Test body',
+        ),
+        success=True,
+        git_patch='',
+        issue_type='issue',
+        base_commit='def456',
+        result_explanation='No changes needed',
+        history=[],
+        metrics={},
+        instruction='Test instruction',
+        comment_success=None,
+        error=None,
+    )
+    token = 'test-token'
+    username = 'test-user'
+    platform = ProviderType.GITLAB
+
+    # Mock make_commit to return False (no changes)
+    mock_make_commit.return_value = False
+
+    # Mock initialize_repo to return a path
+    mock_initialize_repo.return_value = f'{mock_output_dir}/patches/issue_1'
+
+    # Call the function
+    process_single_issue(
+        mock_output_dir,
+        resolver_output,
+        token,
+        username,
+        platform,
+        'ready',
+        mock_llm_config,
+        None,
+        False,
+    )
+
+    # Assert that the mocked functions were called correctly
+    mock_initialize_repo.assert_called_once_with(mock_output_dir, 1, 'issue', 'def456')
+    mock_apply_patch.assert_called_once_with(
+        f'{mock_output_dir}/patches/issue_1', resolver_output.git_patch
+    )
+    mock_make_commit.assert_called_once_with(
+        f'{mock_output_dir}/patches/issue_1', resolver_output.issue, 'issue'
+    )
+    mock_send_pull_request.assert_not_called()
+    mock_send_comment_msg.assert_called_once_with(1, 'No changes needed')
+
+
 def test_apply_patch_rename_directory(mock_output_dir):
     # Create a sample directory structure
     old_dir = os.path.join(mock_output_dir, 'prompts', 'resolve')

tests/unit/resolver/test_no_code_changes.py

@@ -0,0 +1,113 @@
+import json
+from unittest.mock import MagicMock, patch
+
+from openhands.core.config import LLMConfig
+from openhands.events.action.message import MessageAction
+from openhands.llm import LLM
+from openhands.resolver.interfaces.github import GithubIssueHandler, GithubPRHandler
+from openhands.resolver.interfaces.issue import Issue
+from openhands.resolver.interfaces.issue_definitions import (
+    ServiceContextIssue,
+    ServiceContextPR,
+)
+
+
+def test_issue_success_without_code_changes():
+    """Test that an issue can be marked as successful without code changes."""
+    # Mock data
+    issue = Issue(
+        owner='test',
+        repo='test',
+        number=1,
+        title='Test Issue',
+        body='Test body',
+        thread_comments=['Please review this solution'],
+        review_comments=None,
+    )
+    history = [MessageAction(content='After reviewing the solution, it looks good and no changes are needed.')]
+    llm_config = LLMConfig(model='test', api_key='test')
+
+    # Create a mock response indicating success without code changes
+    mock_response = MagicMock()
+    mock_response.choices = [
+        MagicMock(
+            message=MagicMock(
+                content="""--- success
+true
+
+--- explanation
+The solution has been reviewed and no code changes are needed because:
+- The current implementation already handles the case correctly
+- The reported issue was a misunderstanding
+- No actual bug was found in the code"""
+            )
+        )
+    ]
+
+    # Use patch to mock the LLM completion call
+    with patch.object(LLM, 'completion', return_value=mock_response) as mock_completion:
+        # Create a handler instance
+        handler = ServiceContextIssue(
+            GithubIssueHandler('test', 'test', 'test'), llm_config
+        )
+
+        # Call guess_success with no git patch
+        success, _, explanation = handler.guess_success(issue, history, git_patch=None)
+
+        # Verify the results
+        assert success is True
+        assert 'The solution has been reviewed' in explanation
+        assert 'no code changes are needed' in explanation
+
+        # Verify that LLM completion was called exactly once
+        mock_completion.assert_called_once()
+
+
+def test_pr_success_without_code_changes():
+    """Test that a PR can be marked as successful without code changes."""
+    # Create a PR handler instance
+    llm_config = LLMConfig(model='test', api_key='test')
+    handler = ServiceContextPR(GithubPRHandler('test', 'test', 'test'), llm_config)
+
+    # Create a mock issue with thread comments
+    issue = Issue(
+        owner='test-owner',
+        repo='test-repo',
+        number=1,
+        title='Test PR',
+        body='Test Body',
+        thread_comments=['Please review this approach'],
+        closing_issues=['Issue description'],
+        review_comments=None,
+        thread_ids=None,
+        head_branch='test-branch',
+    )
+
+    # Create mock history with a message indicating no changes needed
+    history = [MessageAction(content='After reviewing the approach, no code changes are needed because the current implementation is correct.')]
+
+    # Mock the LLM response
+    mock_response = MagicMock()
+    mock_response.choices = [
+        MagicMock(
+            message=MagicMock(
+                content="""--- success
+true
+
+--- explanation
+The review has been completed and no code changes are needed because:
+- The current implementation is correct
+- The proposed approach would not improve the solution
+- The existing code already handles all edge cases"""
+            )
+        )
+    ]
+
+    # Test the guess_success method
+    with patch.object(LLM, 'completion', return_value=mock_response):
+        success, success_list, explanation = handler.guess_success(issue, history, git_patch=None)
+
+        # Verify the results
+        assert success is True
+        assert success_list == [True]
+        assert 'no code changes are needed' in json.loads(explanation)[0]