From c483ac0fe5014a88fc167e063708546d0e889ab5 Mon Sep 17 00:00:00 2001 From: openhands Date: Sun, 24 Nov 2024 04:12:18 +0000 Subject: [PATCH 01/12] Fix issue #5219: Feature: PR Review --- .github/workflows/openhands-resolver.yml | 83 +++++++++++++ openhands/resolver/prompts/review | 14 +++ openhands/resolver/review_pr.py | 148 +++++++++++++++++++++++ tests/unit/test_review_pr.py | 96 +++++++++++++++ 4 files changed, 341 insertions(+) create mode 100644 openhands/resolver/prompts/review create mode 100644 openhands/resolver/review_pr.py create mode 100644 tests/unit/test_review_pr.py diff --git a/.github/workflows/openhands-resolver.yml b/.github/workflows/openhands-resolver.yml index f24a8e90cbfb..996aa63d3276 100644 --- a/.github/workflows/openhands-resolver.yml +++ b/.github/workflows/openhands-resolver.yml @@ -45,6 +45,89 @@ permissions: issues: write jobs: + review-pr: + if: | + github.event.label.name == 'review-pr' + runs-on: ubuntu-latest + steps: + - name: Checkout repository + uses: actions/checkout@v4 + + - name: Set up Python + uses: actions/setup-python@v5 + with: + python-version: "3.12" + + - name: Get latest versions and create requirements.txt + run: | + python -m pip index versions openhands-ai > openhands_versions.txt + OPENHANDS_VERSION=$(head -n 1 openhands_versions.txt | awk '{print $2}' | tr -d '()') + echo "openhands-ai==${OPENHANDS_VERSION}" >> requirements.txt + cat requirements.txt + + - name: Cache pip dependencies + uses: actions/cache@v3 + with: + path: ${{ env.pythonLocation }}/lib/python3.12/site-packages/* + key: ${{ runner.os }}-pip-openhands-resolver-${{ hashFiles('requirements.txt') }} + restore-keys: | + ${{ runner.os }}-pip-openhands-resolver-${{ hashFiles('requirements.txt') }} + + - name: Check required environment variables + env: + LLM_MODEL: ${{ secrets.LLM_MODEL }} + LLM_API_KEY: ${{ secrets.LLM_API_KEY }} + LLM_BASE_URL: ${{ secrets.LLM_BASE_URL }} + PAT_TOKEN: ${{ secrets.PAT_TOKEN }} + PAT_USERNAME: ${{ secrets.PAT_USERNAME }} + run: | + required_vars=("LLM_MODEL" "LLM_API_KEY" "PAT_TOKEN" "PAT_USERNAME") + for var in "${required_vars[@]}"; do + if [ -z "${!var}" ]; then + echo "Error: Required environment variable $var is not set." + exit 1 + fi + done + + - name: Set environment variables + run: | + echo "ISSUE_NUMBER=${{ github.event.pull_request.number }}" >> $GITHUB_ENV + echo "ISSUE_TYPE=pr" >> $GITHUB_ENV + echo "COMMENT_ID=None" >> $GITHUB_ENV + echo "MAX_ITERATIONS=1" >> $GITHUB_ENV + echo "SANDBOX_ENV_GITHUB_TOKEN=${{ secrets.GITHUB_TOKEN }}" >> $GITHUB_ENV + echo "TARGET_BRANCH=${{ inputs.target_branch }}" >> $GITHUB_ENV + + - name: Comment on PR with start message + uses: actions/github-script@v7 + with: + github-token: ${{secrets.GITHUB_TOKEN}} + script: | + github.rest.issues.createComment({ + issue_number: ${{ env.ISSUE_NUMBER }}, + owner: context.repo.owner, + repo: context.repo.repo, + body: `[OpenHands](https://github.com/All-Hands-AI/OpenHands) started reviewing the PR! You can monitor the progress [here](https://github.com/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}).` + }); + + - name: Install OpenHands + run: | + python -m pip install --upgrade -r requirements.txt + + - name: Review PR + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + GITHUB_USERNAME: ${{ secrets.PAT_USERNAME }} + LLM_MODEL: ${{ secrets.LLM_MODEL }} + LLM_API_KEY: ${{ secrets.LLM_API_KEY }} + LLM_BASE_URL: ${{ secrets.LLM_BASE_URL }} + PYTHONPATH: "" + run: | + cd /tmp && python -m openhands.resolver.review_pr \ + --repo ${{ github.repository }} \ + --issue-number ${{ env.ISSUE_NUMBER }} \ + --issue-type ${{ env.ISSUE_TYPE }} + auto-fix: if: | github.event_name == 'workflow_call' || diff --git a/openhands/resolver/prompts/review b/openhands/resolver/prompts/review new file mode 100644 index 000000000000..2fe885b5f693 --- /dev/null +++ b/openhands/resolver/prompts/review @@ -0,0 +1,14 @@ +You are a helpful AI code reviewer. Your task is to review the following pull request: + +{{ body }} + +Please provide a thorough and constructive review that: +1. Summarizes the changes and their purpose +2. Evaluates code quality, readability, and maintainability +3. Identifies potential bugs, edge cases, or performance issues +4. Suggests improvements while being respectful and helpful +5. Checks for test coverage and documentation +6. Verifies the changes match the PR description and requirements + +Format your review with clear sections and use markdown for better readability. +Focus on being specific and actionable in your feedback. diff --git a/openhands/resolver/review_pr.py b/openhands/resolver/review_pr.py new file mode 100644 index 000000000000..6136f94cce9c --- /dev/null +++ b/openhands/resolver/review_pr.py @@ -0,0 +1,148 @@ +"""Module for reviewing pull requests.""" + +import argparse +import os +import pathlib +import subprocess +from typing import Any + +import jinja2 +import litellm +import requests + +from openhands.core.config import LLMConfig +from openhands.core.logger import openhands_logger as logger +from openhands.resolver.github_issue import GithubIssue +from openhands.resolver.issue_definitions import PRHandler + + +def get_pr_diff(owner: str, repo: str, pr_number: int, token: str) -> str: + """Get the diff for a pull request.""" + url = f'https://api.github.com/repos/{owner}/{repo}/pulls/{pr_number}' + headers = { + 'Authorization': f'token {token}', + 'Accept': 'application/vnd.github.v3.diff', + } + response = requests.get(url, headers=headers) + response.raise_for_status() + return response.text + + +def post_review_comment(owner: str, repo: str, pr_number: int, token: str, review: str) -> None: + """Post a review comment on a pull request.""" + url = f'https://api.github.com/repos/{owner}/{repo}/issues/{pr_number}/comments' + headers = { + 'Authorization': f'token {token}', + 'Accept': 'application/vnd.github.v3+json', + } + data = {'body': review} + response = requests.post(url, headers=headers, json=data) + response.raise_for_status() + + +def review_pr( + owner: str, + repo: str, + token: str, + username: str, + output_dir: str, + llm_config: LLMConfig, + issue_number: int, +) -> None: + """Review a pull request. + + Args: + owner: Github owner of the repo. + repo: Github repository name. + token: Github token to access the repository. + username: Github username to access the repository. + output_dir: Output directory to write the results. + llm_config: Configuration for the language model. + issue_number: PR number to review. + """ + # Create output directory + pathlib.Path(output_dir).mkdir(parents=True, exist_ok=True) + pathlib.Path(os.path.join(output_dir, 'infer_logs')).mkdir(parents=True, exist_ok=True) + logger.info(f'Using output directory: {output_dir}') + + # Get PR handler + pr_handler = PRHandler(owner, repo, token) + + # Get PR details + issues: list[GithubIssue] = pr_handler.get_converted_issues( + issue_numbers=[issue_number], comment_id=None + ) + pr = issues[0] + + # Get PR diff + diff = get_pr_diff(owner, repo, issue_number, token) + + # Load review template + with open( + os.path.join(os.path.dirname(__file__), 'prompts/review'), + 'r', + ) as f: + template = jinja2.Template(f.read()) + + # Generate review instruction + instruction = template.render( + body=f'PR #{pr.number}: {pr.title}\n\n{pr.body}\n\nDiff:\n```diff\n{diff}\n```' + ) + + # Get review from LLM + response = litellm.completion( + model=llm_config.model, + messages=[{'role': 'user', 'content': instruction}], + api_key=llm_config.api_key, + base_url=llm_config.base_url, + ) + review = response.choices[0].message.content.strip() + + # Post review comment + post_review_comment(owner, repo, issue_number, token, review) + logger.info('Posted review comment successfully') + + +def main() -> None: + """Main function.""" + parser = argparse.ArgumentParser(description='Review a pull request.') + parser.add_argument('--repo', type=str, required=True, help='Repository in owner/repo format') + parser.add_argument('--issue-number', type=int, required=True, help='PR number to review') + parser.add_argument('--issue-type', type=str, required=True, help='Issue type (pr)') + + args = parser.parse_args() + + # Get environment variables + token = os.environ.get('GITHUB_TOKEN') + username = os.environ.get('GITHUB_USERNAME') + llm_model = os.environ.get('LLM_MODEL') + llm_api_key = os.environ.get('LLM_API_KEY') + llm_base_url = os.environ.get('LLM_BASE_URL') + + if not all([token, username, llm_model, llm_api_key]): + raise ValueError('Missing required environment variables') + + # Split repo into owner and name + owner, repo = args.repo.split('/') + + # Configure LLM + llm_config = LLMConfig( + model=llm_model, + api_key=llm_api_key, + base_url=llm_base_url, + ) + + # Review PR + review_pr( + owner=owner, + repo=repo, + token=token, + username=username, + output_dir='/tmp/output', + llm_config=llm_config, + issue_number=args.issue_number, + ) + + +if __name__ == '__main__': + main() diff --git a/tests/unit/test_review_pr.py b/tests/unit/test_review_pr.py new file mode 100644 index 000000000000..05dcfae76d13 --- /dev/null +++ b/tests/unit/test_review_pr.py @@ -0,0 +1,96 @@ +"""Tests for the review_pr module.""" + +import os +import tempfile +from unittest.mock import MagicMock, patch + +import pytest + +from openhands.core.config import LLMConfig +from openhands.resolver.github_issue import GithubIssue +from openhands.resolver.review_pr import get_pr_diff, post_review_comment, review_pr + + +@pytest.fixture +def mock_pr() -> GithubIssue: + """Create a mock PR.""" + return GithubIssue( + owner='owner', + repo='repo', + number=1, + title='Test PR', + body='Test PR description', + thread_comments=None, + review_comments=None, + ) + + +@pytest.fixture +def mock_llm_config() -> LLMConfig: + """Create a mock LLM config.""" + return LLMConfig( + model='test-model', + api_key='test-key', + base_url=None, + ) + + +def test_get_pr_diff() -> None: + """Test getting PR diff.""" + with patch('requests.get') as mock_get: + mock_get.return_value.text = 'test diff' + diff = get_pr_diff('owner', 'repo', 1, 'token') + assert diff == 'test diff' + mock_get.assert_called_once_with( + 'https://api.github.com/repos/owner/repo/pulls/1', + headers={ + 'Authorization': 'token token', + 'Accept': 'application/vnd.github.v3.diff', + }, + ) + + +def test_post_review_comment() -> None: + """Test posting review comment.""" + with patch('requests.post') as mock_post: + post_review_comment('owner', 'repo', 1, 'token', 'test review') + mock_post.assert_called_once_with( + 'https://api.github.com/repos/owner/repo/issues/1/comments', + headers={ + 'Authorization': 'token token', + 'Accept': 'application/vnd.github.v3+json', + }, + json={'body': 'test review'}, + ) + + +def test_review_pr(mock_pr: GithubIssue, mock_llm_config: LLMConfig) -> None: + """Test reviewing PR.""" + with ( + tempfile.TemporaryDirectory() as temp_dir, + patch('openhands.resolver.review_pr.PRHandler') as mock_handler, + patch('openhands.resolver.review_pr.get_pr_diff') as mock_get_diff, + patch('openhands.resolver.review_pr.post_review_comment') as mock_post_comment, + patch('litellm.completion') as mock_completion, + ): + # Setup mocks + mock_handler.return_value.get_converted_issues.return_value = [mock_pr] + mock_get_diff.return_value = 'test diff' + mock_completion.return_value.choices = [MagicMock(message=MagicMock(content='test review'))] + + # Run review + review_pr( + owner='owner', + repo='repo', + token='token', + username='username', + output_dir=temp_dir, + llm_config=mock_llm_config, + issue_number=1, + ) + + # Verify calls + mock_handler.assert_called_once_with('owner', 'repo', 'token') + mock_get_diff.assert_called_once_with('owner', 'repo', 1, 'token') + mock_completion.assert_called_once() + mock_post_comment.assert_called_once_with('owner', 'repo', 1, 'token', 'test review') From 5a9e7690df4d0f5cee6752d6d76a812537e5b016 Mon Sep 17 00:00:00 2001 From: openhands Date: Sun, 24 Nov 2024 05:49:31 +0000 Subject: [PATCH 02/12] Fix pr #5232: Fix issue #5219: Feature: PR Review --- .github/workflows/openhands-resolver.yml | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/.github/workflows/openhands-resolver.yml b/.github/workflows/openhands-resolver.yml index 996aa63d3276..e3d0a2fd9966 100644 --- a/.github/workflows/openhands-resolver.yml +++ b/.github/workflows/openhands-resolver.yml @@ -112,7 +112,12 @@ jobs: - name: Install OpenHands run: | - python -m pip install --upgrade -r requirements.txt + if [[ "${{ github.event.label.name }}" == "review-pr" ]]; then + python -m pip install --upgrade pip + pip install git+https://github.com/all-hands-ai/openhands.git + else + python -m pip install --upgrade -r requirements.txt + fi - name: Review PR env: From 439e5056d08445db41abc04bc4d5e504fde3eef9 Mon Sep 17 00:00:00 2001 From: openhands Date: Sun, 24 Nov 2024 06:01:07 +0000 Subject: [PATCH 03/12] Fix pr #5232: Fix issue #5219: Feature: PR Review --- .github/workflows/openhands-resolver.yml | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/.github/workflows/openhands-resolver.yml b/.github/workflows/openhands-resolver.yml index e3d0a2fd9966..4e0043085602 100644 --- a/.github/workflows/openhands-resolver.yml +++ b/.github/workflows/openhands-resolver.yml @@ -112,12 +112,8 @@ jobs: - name: Install OpenHands run: | - if [[ "${{ github.event.label.name }}" == "review-pr" ]]; then - python -m pip install --upgrade pip - pip install git+https://github.com/all-hands-ai/openhands.git - else - python -m pip install --upgrade -r requirements.txt - fi + python -m pip install --upgrade pip + pip install git+https://github.com/all-hands-ai/openhands.git - name: Review PR env: From e294f7606cc298de63005a53c33a2d9b44dcf13f Mon Sep 17 00:00:00 2001 From: openhands Date: Sun, 24 Nov 2024 06:13:34 +0000 Subject: [PATCH 04/12] Fix pr #5232: Fix issue #5219: Feature: PR Review --- .github/workflows/openhands-resolver.yml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/.github/workflows/openhands-resolver.yml b/.github/workflows/openhands-resolver.yml index 4e0043085602..6be2ab737b84 100644 --- a/.github/workflows/openhands-resolver.yml +++ b/.github/workflows/openhands-resolver.yml @@ -113,7 +113,11 @@ jobs: - name: Install OpenHands run: | python -m pip install --upgrade pip - pip install git+https://github.com/all-hands-ai/openhands.git + if [[ "${{ github.event.label.name }}" == "review-pr" ]]; then + pip install git+https://github.com/all-hands-ai/openhands.git + else + python -m pip install --upgrade -r requirements.txt + fi - name: Review PR env: From f8fc3820e5568a11fcf70e9ea413a3d28b87ce87 Mon Sep 17 00:00:00 2001 From: openhands Date: Tue, 26 Nov 2024 00:56:43 +0000 Subject: [PATCH 05/12] Fix pr #5232: Fix issue #5219: Feature: PR Review --- .github/workflows/openhands-resolver.yml | 26 ++++++++---------------- 1 file changed, 9 insertions(+), 17 deletions(-) diff --git a/.github/workflows/openhands-resolver.yml b/.github/workflows/openhands-resolver.yml index 6be2ab737b84..c0829221cce3 100644 --- a/.github/workflows/openhands-resolver.yml +++ b/.github/workflows/openhands-resolver.yml @@ -52,26 +52,22 @@ jobs: steps: - name: Checkout repository uses: actions/checkout@v4 + with: + ref: ${{ github.event.pull_request.head.sha }} - name: Set up Python uses: actions/setup-python@v5 with: python-version: "3.12" - - name: Get latest versions and create requirements.txt + - name: Install poetry run: | - python -m pip index versions openhands-ai > openhands_versions.txt - OPENHANDS_VERSION=$(head -n 1 openhands_versions.txt | awk '{print $2}' | tr -d '()') - echo "openhands-ai==${OPENHANDS_VERSION}" >> requirements.txt - cat requirements.txt + python -m pip install --upgrade pip + pip install poetry - - name: Cache pip dependencies - uses: actions/cache@v3 - with: - path: ${{ env.pythonLocation }}/lib/python3.12/site-packages/* - key: ${{ runner.os }}-pip-openhands-resolver-${{ hashFiles('requirements.txt') }} - restore-keys: | - ${{ runner.os }}-pip-openhands-resolver-${{ hashFiles('requirements.txt') }} + - name: Install OpenHands from PR branch + run: | + poetry install - name: Check required environment variables env: @@ -113,11 +109,7 @@ jobs: - name: Install OpenHands run: | python -m pip install --upgrade pip - if [[ "${{ github.event.label.name }}" == "review-pr" ]]; then - pip install git+https://github.com/all-hands-ai/openhands.git - else - python -m pip install --upgrade -r requirements.txt - fi + python -m pip install --upgrade -r requirements.txt - name: Review PR env: From 90a93511b666ccda20dbed7284c4b79965e40eca Mon Sep 17 00:00:00 2001 From: Engel Nyst Date: Tue, 26 Nov 2024 02:15:39 +0100 Subject: [PATCH 06/12] Update .github/workflows/openhands-resolver.yml --- .github/workflows/openhands-resolver.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/openhands-resolver.yml b/.github/workflows/openhands-resolver.yml index c0829221cce3..02ff07565f26 100644 --- a/.github/workflows/openhands-resolver.yml +++ b/.github/workflows/openhands-resolver.yml @@ -106,7 +106,6 @@ jobs: body: `[OpenHands](https://github.com/All-Hands-AI/OpenHands) started reviewing the PR! You can monitor the progress [here](https://github.com/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}).` }); - - name: Install OpenHands run: | python -m pip install --upgrade pip python -m pip install --upgrade -r requirements.txt From c74eab9ad0974539eae5fab7b972a58046de6f44 Mon Sep 17 00:00:00 2001 From: Engel Nyst Date: Tue, 26 Nov 2024 02:15:58 +0100 Subject: [PATCH 07/12] Update .github/workflows/openhands-resolver.yml --- .github/workflows/openhands-resolver.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/openhands-resolver.yml b/.github/workflows/openhands-resolver.yml index 02ff07565f26..213d2ad35f59 100644 --- a/.github/workflows/openhands-resolver.yml +++ b/.github/workflows/openhands-resolver.yml @@ -108,7 +108,6 @@ jobs: run: | python -m pip install --upgrade pip - python -m pip install --upgrade -r requirements.txt - name: Review PR env: From c6dec197ee293c141f213869db247030899ab533 Mon Sep 17 00:00:00 2001 From: Engel Nyst Date: Tue, 26 Nov 2024 02:16:17 +0100 Subject: [PATCH 08/12] Update .github/workflows/openhands-resolver.yml --- .github/workflows/openhands-resolver.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/openhands-resolver.yml b/.github/workflows/openhands-resolver.yml index 213d2ad35f59..9ced82475562 100644 --- a/.github/workflows/openhands-resolver.yml +++ b/.github/workflows/openhands-resolver.yml @@ -107,7 +107,6 @@ jobs: }); run: | - python -m pip install --upgrade pip - name: Review PR env: From 9ee1776a9127392d175b1d8d5cb2f0d52d6bb321 Mon Sep 17 00:00:00 2001 From: Engel Nyst Date: Tue, 26 Nov 2024 02:16:32 +0100 Subject: [PATCH 09/12] Update .github/workflows/openhands-resolver.yml --- .github/workflows/openhands-resolver.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/openhands-resolver.yml b/.github/workflows/openhands-resolver.yml index 9ced82475562..594a97fae209 100644 --- a/.github/workflows/openhands-resolver.yml +++ b/.github/workflows/openhands-resolver.yml @@ -106,7 +106,6 @@ jobs: body: `[OpenHands](https://github.com/All-Hands-AI/OpenHands) started reviewing the PR! You can monitor the progress [here](https://github.com/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}).` }); - run: | - name: Review PR env: From dc0b52456e9eb47d0df07b6a8953c8f4ec24501b Mon Sep 17 00:00:00 2001 From: Engel Nyst Date: Tue, 26 Nov 2024 02:34:31 +0100 Subject: [PATCH 10/12] Update openhands-resolver.yml --- .github/workflows/openhands-resolver.yml | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/.github/workflows/openhands-resolver.yml b/.github/workflows/openhands-resolver.yml index 594a97fae209..199fadf4f849 100644 --- a/.github/workflows/openhands-resolver.yml +++ b/.github/workflows/openhands-resolver.yml @@ -65,9 +65,9 @@ jobs: python -m pip install --upgrade pip pip install poetry - - name: Install OpenHands from PR branch + - name: Install OpenHands dependencies run: | - poetry install + poetry install --without evaluation,llama-index - name: Check required environment variables env: @@ -94,6 +94,11 @@ jobs: echo "SANDBOX_ENV_GITHUB_TOKEN=${{ secrets.GITHUB_TOKEN }}" >> $GITHUB_ENV echo "TARGET_BRANCH=${{ inputs.target_branch }}" >> $GITHUB_ENV + - name: Install OpenHands from PR branch + run: | + python -m pip install --upgrade pip + pip install git+https://github.com/${{ github.repository }}.git@${{ github.head_ref }} + - name: Comment on PR with start message uses: actions/github-script@v7 with: From 5e3bd518a6fa34c9a425ef64389c72b424e85446 Mon Sep 17 00:00:00 2001 From: openhands Date: Tue, 26 Nov 2024 02:22:14 +0000 Subject: [PATCH 11/12] Fix pr #5232: Fix issue #5219: Feature: PR Review --- openhands/resolver/review_pr.py | 22 ++++++++++++++++------ tests/unit/test_review_pr.py | 9 ++++++--- 2 files changed, 22 insertions(+), 9 deletions(-) diff --git a/openhands/resolver/review_pr.py b/openhands/resolver/review_pr.py index 6136f94cce9c..b9d6dc92b28d 100644 --- a/openhands/resolver/review_pr.py +++ b/openhands/resolver/review_pr.py @@ -3,8 +3,6 @@ import argparse import os import pathlib -import subprocess -from typing import Any import jinja2 import litellm @@ -28,7 +26,9 @@ def get_pr_diff(owner: str, repo: str, pr_number: int, token: str) -> str: return response.text -def post_review_comment(owner: str, repo: str, pr_number: int, token: str, review: str) -> None: +def post_review_comment( + owner: str, repo: str, pr_number: int, token: str, review: str +) -> None: """Post a review comment on a pull request.""" url = f'https://api.github.com/repos/{owner}/{repo}/issues/{pr_number}/comments' headers = { @@ -62,7 +62,9 @@ def review_pr( """ # Create output directory pathlib.Path(output_dir).mkdir(parents=True, exist_ok=True) - pathlib.Path(os.path.join(output_dir, 'infer_logs')).mkdir(parents=True, exist_ok=True) + pathlib.Path(os.path.join(output_dir, 'infer_logs')).mkdir( + parents=True, exist_ok=True + ) logger.info(f'Using output directory: {output_dir}') # Get PR handler @@ -106,8 +108,12 @@ def review_pr( def main() -> None: """Main function.""" parser = argparse.ArgumentParser(description='Review a pull request.') - parser.add_argument('--repo', type=str, required=True, help='Repository in owner/repo format') - parser.add_argument('--issue-number', type=int, required=True, help='PR number to review') + parser.add_argument( + '--repo', type=str, required=True, help='Repository in owner/repo format' + ) + parser.add_argument( + '--issue-number', type=int, required=True, help='PR number to review' + ) parser.add_argument('--issue-type', type=str, required=True, help='Issue type (pr)') args = parser.parse_args() @@ -126,6 +132,8 @@ def main() -> None: owner, repo = args.repo.split('/') # Configure LLM + assert llm_model is not None # For type checker + assert llm_api_key is not None # For type checker llm_config = LLMConfig( model=llm_model, api_key=llm_api_key, @@ -133,6 +141,8 @@ def main() -> None: ) # Review PR + assert token is not None # For type checker + assert username is not None # For type checker review_pr( owner=owner, repo=repo, diff --git a/tests/unit/test_review_pr.py b/tests/unit/test_review_pr.py index 05dcfae76d13..cd7d871f4df1 100644 --- a/tests/unit/test_review_pr.py +++ b/tests/unit/test_review_pr.py @@ -1,6 +1,5 @@ """Tests for the review_pr module.""" -import os import tempfile from unittest.mock import MagicMock, patch @@ -76,7 +75,9 @@ def test_review_pr(mock_pr: GithubIssue, mock_llm_config: LLMConfig) -> None: # Setup mocks mock_handler.return_value.get_converted_issues.return_value = [mock_pr] mock_get_diff.return_value = 'test diff' - mock_completion.return_value.choices = [MagicMock(message=MagicMock(content='test review'))] + mock_completion.return_value.choices = [ + MagicMock(message=MagicMock(content='test review')) + ] # Run review review_pr( @@ -93,4 +94,6 @@ def test_review_pr(mock_pr: GithubIssue, mock_llm_config: LLMConfig) -> None: mock_handler.assert_called_once_with('owner', 'repo', 'token') mock_get_diff.assert_called_once_with('owner', 'repo', 1, 'token') mock_completion.assert_called_once() - mock_post_comment.assert_called_once_with('owner', 'repo', 1, 'token', 'test review') + mock_post_comment.assert_called_once_with( + 'owner', 'repo', 1, 'token', 'test review' + ) From 10184d2810bbc7591e2f7d4b509dd4fd26494933 Mon Sep 17 00:00:00 2001 From: openhands Date: Wed, 27 Nov 2024 19:37:25 +0000 Subject: [PATCH 12/12] Fix pr #5232: Fix issue #5219: Feature: PR Review --- frontend/public/config.json | 2 +- .../agenthub/codeact_agent/codeact_agent.py | 4 +- openhands/events/action/message.py | 1 + openhands/events/serialization/action.py | 2 +- openhands/resolver/patching/__init__.py | 4 +- openhands/resolver/patching/apply.py | 34 +-- openhands/resolver/patching/exceptions.py | 2 +- openhands/resolver/patching/patch.py | 160 ++++++------ openhands/resolver/patching/snippets.py | 2 +- .../all-hands-ai___openhands-resolver.txt | 2 +- .../repo_instructions/rbren___rss-parser.txt | 2 +- .../prompts/resolve/basic-with-tests.jinja | 2 +- .../resolver/prompts/resolve/basic.jinja | 2 +- openhands/resolver/review_pr.py | 24 +- openhands/runtime/utils/command.py | 2 +- poetry.lock | 2 +- pyproject.toml | 1 + tests/unit/resolver/test_guess_success.py | 30 +-- .../test_issue_handler_error_handling.py | 65 ++--- tests/unit/resolver/test_issue_references.py | 22 +- .../resolver/test_pr_handler_guess_success.py | 238 +++++++++--------- tests/unit/resolver/test_pr_title_escaping.py | 109 ++++---- 22 files changed, 357 insertions(+), 355 deletions(-) diff --git a/frontend/public/config.json b/frontend/public/config.json index 94900dcbaf31..7dbb7e1d966c 100644 --- a/frontend/public/config.json +++ b/frontend/public/config.json @@ -2,4 +2,4 @@ "APP_MODE": "oss", "GITHUB_CLIENT_ID": "", "POSTHOG_CLIENT_KEY": "phc_3ESMmY9SgqEAGBB6sMGK5ayYHkeUuknH2vP6FmWH9RA" -} \ No newline at end of file +} diff --git a/openhands/agenthub/codeact_agent/codeact_agent.py b/openhands/agenthub/codeact_agent/codeact_agent.py index 39b9e69247be..6743de87ade6 100644 --- a/openhands/agenthub/codeact_agent/codeact_agent.py +++ b/openhands/agenthub/codeact_agent/codeact_agent.py @@ -187,7 +187,9 @@ def get_action_message( ) ] elif isinstance(action, CmdRunAction) and action.source == 'user': - content = [TextContent(text=f'User executed the command:\n{action.command}')] + content = [ + TextContent(text=f'User executed the command:\n{action.command}') + ] return [ Message( role='user', diff --git a/openhands/events/action/message.py b/openhands/events/action/message.py index 86d7c439e936..d86526419664 100644 --- a/openhands/events/action/message.py +++ b/openhands/events/action/message.py @@ -24,6 +24,7 @@ def images_urls(self): @images_urls.setter def images_urls(self, value): self.image_urls = value + def __str__(self) -> str: ret = f'**MessageAction** (source={self.source})\n' ret += f'CONTENT: {self.content}' diff --git a/openhands/events/serialization/action.py b/openhands/events/serialization/action.py index defac3b5dda6..f34b4b0ec0cf 100644 --- a/openhands/events/serialization/action.py +++ b/openhands/events/serialization/action.py @@ -69,7 +69,7 @@ def action_from_dict(action: dict) -> Action: # images_urls has been renamed to image_urls if 'images_urls' in args: args['image_urls'] = args.pop('images_urls') - + try: decoded_action = action_class(**args) if 'timeout' in action: diff --git a/openhands/resolver/patching/__init__.py b/openhands/resolver/patching/__init__.py index 5c31f160a0a0..165a623af537 100644 --- a/openhands/resolver/patching/__init__.py +++ b/openhands/resolver/patching/__init__.py @@ -1,6 +1,6 @@ # -*- coding: utf-8 -*- -from .patch import parse_patch from .apply import apply_diff +from .patch import parse_patch -__all__ = ["parse_patch", "apply_diff"] +__all__ = ['parse_patch', 'apply_diff'] diff --git a/openhands/resolver/patching/apply.py b/openhands/resolver/patching/apply.py index f13e814292cb..24f2266f56cf 100644 --- a/openhands/resolver/patching/apply.py +++ b/openhands/resolver/patching/apply.py @@ -10,33 +10,33 @@ def _apply_diff_with_subprocess(diff, lines, reverse=False): # call out to patch program - patchexec = which("patch") + patchexec = which('patch') if not patchexec: - raise SubprocessException("cannot find patch program", code=-1) + raise SubprocessException('cannot find patch program', code=-1) tempdir = tempfile.gettempdir() - filepath = os.path.join(tempdir, "wtp-" + str(hash(diff.header))) - oldfilepath = filepath + ".old" - newfilepath = filepath + ".new" - rejfilepath = filepath + ".rej" - patchfilepath = filepath + ".patch" - with open(oldfilepath, "w") as f: - f.write("\n".join(lines) + "\n") + filepath = os.path.join(tempdir, 'wtp-' + str(hash(diff.header))) + oldfilepath = filepath + '.old' + newfilepath = filepath + '.new' + rejfilepath = filepath + '.rej' + patchfilepath = filepath + '.patch' + with open(oldfilepath, 'w') as f: + f.write('\n'.join(lines) + '\n') - with open(patchfilepath, "w") as f: + with open(patchfilepath, 'w') as f: f.write(diff.text) args = [ patchexec, - "--reverse" if reverse else "--forward", - "--quiet", - "--no-backup-if-mismatch", - "-o", + '--reverse' if reverse else '--forward', + '--quiet', + '--no-backup-if-mismatch', + '-o', newfilepath, - "-i", + '-i', patchfilepath, - "-r", + '-r', rejfilepath, oldfilepath, ] @@ -58,7 +58,7 @@ def _apply_diff_with_subprocess(diff, lines, reverse=False): # do this last to ensure files get cleaned up if ret != 0: - raise SubprocessException("patch program failed", code=ret) + raise SubprocessException('patch program failed', code=ret) return lines, rejlines diff --git a/openhands/resolver/patching/exceptions.py b/openhands/resolver/patching/exceptions.py index 594b079e8365..30653c56da18 100644 --- a/openhands/resolver/patching/exceptions.py +++ b/openhands/resolver/patching/exceptions.py @@ -7,7 +7,7 @@ def __init__(self, msg, hunk=None): self.hunk = hunk if hunk is not None: super(HunkException, self).__init__( - "{msg}, in hunk #{n}".format(msg=msg, n=hunk) + '{msg}, in hunk #{n}'.format(msg=msg, n=hunk) ) else: super(HunkException, self).__init__(msg) diff --git a/openhands/resolver/patching/patch.py b/openhands/resolver/patching/patch.py index c0304e06543b..7e3b98ed0883 100644 --- a/openhands/resolver/patching/patch.py +++ b/openhands/resolver/patching/patch.py @@ -8,67 +8,67 @@ from .snippets import findall_regex, split_by_regex header = namedtuple( - "header", - "index_path old_path old_version new_path new_version", + 'header', + 'index_path old_path old_version new_path new_version', ) -diffobj = namedtuple("diffobj", "header changes text") -Change = namedtuple("Change", "old new line hunk") +diffobj = namedtuple('diffobj', 'header changes text') +Change = namedtuple('Change', 'old new line hunk') -file_timestamp_str = "(.+?)(?:\t|:| +)(.*)" +file_timestamp_str = '(.+?)(?:\t|:| +)(.*)' # .+? was previously [^:\t\n\r\f\v]+ # general diff regex -diffcmd_header = re.compile("^diff.* (.+) (.+)$") -unified_header_index = re.compile("^Index: (.+)$") -unified_header_old_line = re.compile(r"^--- " + file_timestamp_str + "$") -unified_header_new_line = re.compile(r"^\+\+\+ " + file_timestamp_str + "$") -unified_hunk_start = re.compile(r"^@@ -(\d+),?(\d*) \+(\d+),?(\d*) @@(.*)$") -unified_change = re.compile("^([-+ ])(.*)$") - -context_header_old_line = re.compile(r"^\*\*\* " + file_timestamp_str + "$") -context_header_new_line = re.compile("^--- " + file_timestamp_str + "$") -context_hunk_start = re.compile(r"^\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*$") -context_hunk_old = re.compile(r"^\*\*\* (\d+),?(\d*) \*\*\*\*$") -context_hunk_new = re.compile(r"^--- (\d+),?(\d*) ----$") -context_change = re.compile("^([-+ !]) (.*)$") - -ed_hunk_start = re.compile(r"^(\d+),?(\d*)([acd])$") -ed_hunk_end = re.compile("^.$") +diffcmd_header = re.compile('^diff.* (.+) (.+)$') +unified_header_index = re.compile('^Index: (.+)$') +unified_header_old_line = re.compile(r'^--- ' + file_timestamp_str + '$') +unified_header_new_line = re.compile(r'^\+\+\+ ' + file_timestamp_str + '$') +unified_hunk_start = re.compile(r'^@@ -(\d+),?(\d*) \+(\d+),?(\d*) @@(.*)$') +unified_change = re.compile('^([-+ ])(.*)$') + +context_header_old_line = re.compile(r'^\*\*\* ' + file_timestamp_str + '$') +context_header_new_line = re.compile('^--- ' + file_timestamp_str + '$') +context_hunk_start = re.compile(r'^\*\*\*\*\*\*\*\*\*\*\*\*\*\*\*$') +context_hunk_old = re.compile(r'^\*\*\* (\d+),?(\d*) \*\*\*\*$') +context_hunk_new = re.compile(r'^--- (\d+),?(\d*) ----$') +context_change = re.compile('^([-+ !]) (.*)$') + +ed_hunk_start = re.compile(r'^(\d+),?(\d*)([acd])$') +ed_hunk_end = re.compile('^.$') # much like forward ed, but no 'c' type -rcs_ed_hunk_start = re.compile(r"^([ad])(\d+) ?(\d*)$") +rcs_ed_hunk_start = re.compile(r'^([ad])(\d+) ?(\d*)$') -default_hunk_start = re.compile(r"^(\d+),?(\d*)([acd])(\d+),?(\d*)$") -default_hunk_mid = re.compile("^---$") -default_change = re.compile("^([><]) (.*)$") +default_hunk_start = re.compile(r'^(\d+),?(\d*)([acd])(\d+),?(\d*)$') +default_hunk_mid = re.compile('^---$') +default_change = re.compile('^([><]) (.*)$') # Headers # git has a special index header and no end part -git_diffcmd_header = re.compile("^diff --git a/(.+) b/(.+)$") -git_header_index = re.compile(r"^index ([a-f0-9]+)..([a-f0-9]+) ?(\d*)$") -git_header_old_line = re.compile("^--- (.+)$") -git_header_new_line = re.compile(r"^\+\+\+ (.+)$") -git_header_file_mode = re.compile(r"^(new|deleted) file mode \d{6}$") -git_header_binary_file = re.compile("^Binary files (.+) and (.+) differ") -git_binary_patch_start = re.compile(r"^GIT binary patch$") -git_binary_literal_start = re.compile(r"^literal (\d+)$") -git_binary_delta_start = re.compile(r"^delta (\d+)$") -base85string = re.compile(r"^[0-9A-Za-z!#$%&()*+;<=>?@^_`{|}~-]+$") - -bzr_header_index = re.compile("=== (.+)") +git_diffcmd_header = re.compile('^diff --git a/(.+) b/(.+)$') +git_header_index = re.compile(r'^index ([a-f0-9]+)..([a-f0-9]+) ?(\d*)$') +git_header_old_line = re.compile('^--- (.+)$') +git_header_new_line = re.compile(r'^\+\+\+ (.+)$') +git_header_file_mode = re.compile(r'^(new|deleted) file mode \d{6}$') +git_header_binary_file = re.compile('^Binary files (.+) and (.+) differ') +git_binary_patch_start = re.compile(r'^GIT binary patch$') +git_binary_literal_start = re.compile(r'^literal (\d+)$') +git_binary_delta_start = re.compile(r'^delta (\d+)$') +base85string = re.compile(r'^[0-9A-Za-z!#$%&()*+;<=>?@^_`{|}~-]+$') + +bzr_header_index = re.compile('=== (.+)') bzr_header_old_line = unified_header_old_line bzr_header_new_line = unified_header_new_line svn_header_index = unified_header_index -svn_header_timestamp_version = re.compile(r"\((?:working copy|revision (\d+))\)") -svn_header_timestamp = re.compile(r".*(\(.*\))$") +svn_header_timestamp_version = re.compile(r'\((?:working copy|revision (\d+))\)') +svn_header_timestamp = re.compile(r'.*(\(.*\))$') cvs_header_index = unified_header_index -cvs_header_rcs = re.compile(r"^RCS file: (.+)(?:,\w{1}$|$)") -cvs_header_timestamp = re.compile(r"(.+)\t([\d.]+)") -cvs_header_timestamp_colon = re.compile(r":([\d.]+)\t(.+)") -old_cvs_diffcmd_header = re.compile("^diff.* (.+):(.*) (.+):(.*)$") +cvs_header_rcs = re.compile(r'^RCS file: (.+)(?:,\w{1}$|$)') +cvs_header_timestamp = re.compile(r'(.+)\t([\d.]+)') +cvs_header_timestamp_colon = re.compile(r':([\d.]+)\t(.+)') +old_cvs_diffcmd_header = re.compile('^diff.* (.+):(.*) (.+):(.*)$') def parse_patch(text): @@ -97,7 +97,7 @@ def parse_patch(text): break for diff in diffs: - difftext = "\n".join(diff) + "\n" + difftext = '\n'.join(diff) + '\n' h = parse_header(diff) d = parse_diff(diff) if h or d: @@ -133,10 +133,10 @@ def parse_scm_header(text): if res: old_path = res.old_path new_path = res.new_path - if old_path.startswith("a/"): + if old_path.startswith('a/'): old_path = old_path[2:] - if new_path.startswith("b/"): + if new_path.startswith('b/'): new_path = new_path[2:] return header( @@ -240,10 +240,10 @@ def parse_git_header(text): new_path = binary.group(2) if old_path and new_path: - if old_path.startswith("a/"): + if old_path.startswith('a/'): old_path = old_path[2:] - if new_path.startswith("b/"): + if new_path.startswith('b/'): new_path = new_path[2:] return header( index_path=None, @@ -256,19 +256,19 @@ def parse_git_header(text): # if we go through all of the text without finding our normal info, # use the cmd if available if cmd_old_path and cmd_new_path and old_version and new_version: - if cmd_old_path.startswith("a/"): + if cmd_old_path.startswith('a/'): cmd_old_path = cmd_old_path[2:] - if cmd_new_path.startswith("b/"): + if cmd_new_path.startswith('b/'): cmd_new_path = cmd_new_path[2:] return header( index_path=None, # wow, I kind of hate this: # assume /dev/null if the versions are zeroed out - old_path="/dev/null" if old_version == "0000000" else cmd_old_path, + old_path='/dev/null' if old_version == '0000000' else cmd_old_path, old_version=old_version, - new_path="/dev/null" if new_version == "0000000" else cmd_new_path, + new_path='/dev/null' if new_version == '0000000' else cmd_new_path, new_version=new_version, ) @@ -569,10 +569,10 @@ def parse_default_diff(text): kind = c.group(1) line = c.group(2) - if kind == "<" and (r != old_len or r == 0): + if kind == '<' and (r != old_len or r == 0): changes.append(Change(old + r, None, line, hunk_n)) r += 1 - elif kind == ">" and (i != new_len or i == 0): + elif kind == '>' and (i != new_len or i == 0): changes.append(Change(None, new + i, line, hunk_n)) i += 1 @@ -627,13 +627,13 @@ def parse_unified_diff(text): kind = c.group(1) line = c.group(2) - if kind == "-" and (r != old_len or r == 0): + if kind == '-' and (r != old_len or r == 0): changes.append(Change(old + r, None, line, hunk_n)) r += 1 - elif kind == "+" and (i != new_len or i == 0): + elif kind == '+' and (i != new_len or i == 0): changes.append(Change(None, new + i, line, hunk_n)) i += 1 - elif kind == " ": + elif kind == ' ': if r != old_len and i != new_len: changes.append(Change(old + r, new + i, line, hunk_n)) r += 1 @@ -667,7 +667,7 @@ def parse_context_diff(text): k = 0 parts = split_by_regex(hunk, context_hunk_new) if len(parts) != 2: - raise exceptions.ParseException("Context diff invalid", hunk_n) + raise exceptions.ParseException('Context diff invalid', hunk_n) old_hunk = parts[0] new_hunk = parts[1] @@ -695,7 +695,7 @@ def parse_context_diff(text): # now have old and new set, can start processing? if len(old_hunk) > 0 and len(new_hunk) == 0: - msg = "Got unexpected change in removal hunk: " + msg = 'Got unexpected change in removal hunk: ' # only removes left? while len(old_hunk) > 0: c = context_change.match(old_hunk[0]) @@ -707,22 +707,22 @@ def parse_context_diff(text): kind = c.group(1) line = c.group(2) - if kind == "-" and (j != old_len or j == 0): + if kind == '-' and (j != old_len or j == 0): changes.append(Change(old + j, None, line, hunk_n)) j += 1 - elif kind == " " and ( + elif kind == ' ' and ( (j != old_len and k != new_len) or (j == 0 or k == 0) ): changes.append(Change(old + j, new + k, line, hunk_n)) j += 1 k += 1 - elif kind == "+" or kind == "!": + elif kind == '+' or kind == '!': raise exceptions.ParseException(msg + kind, hunk_n) continue if len(old_hunk) == 0 and len(new_hunk) > 0: - msg = "Got unexpected change in removal hunk: " + msg = 'Got unexpected change in removal hunk: ' # only insertions left? while len(new_hunk) > 0: c = context_change.match(new_hunk[0]) @@ -734,16 +734,16 @@ def parse_context_diff(text): kind = c.group(1) line = c.group(2) - if kind == "+" and (k != new_len or k == 0): + if kind == '+' and (k != new_len or k == 0): changes.append(Change(None, new + k, line, hunk_n)) k += 1 - elif kind == " " and ( + elif kind == ' ' and ( (j != old_len and k != new_len) or (j == 0 or k == 0) ): changes.append(Change(old + j, new + k, line, hunk_n)) j += 1 k += 1 - elif kind == "-" or kind == "!": + elif kind == '-' or kind == '!': raise exceptions.ParseException(msg + kind, hunk_n) continue @@ -765,17 +765,17 @@ def parse_context_diff(text): if not (oc or nc): del old_hunk[0] del new_hunk[0] - elif okind == " " and nkind == " " and oline == nline: + elif okind == ' ' and nkind == ' ' and oline == nline: changes.append(Change(old + j, new + k, oline, hunk_n)) j += 1 k += 1 del old_hunk[0] del new_hunk[0] - elif okind == "-" or okind == "!" and (j != old_len or j == 0): + elif okind == '-' or okind == '!' and (j != old_len or j == 0): changes.append(Change(old + j, None, oline, hunk_n)) j += 1 del old_hunk[0] - elif nkind == "+" or nkind == "!" and (k != new_len or k == 0): + elif nkind == '+' or nkind == '!' and (k != new_len or k == 0): changes.append(Change(None, new + k, nline, hunk_n)) k += 1 del new_hunk[0] @@ -821,7 +821,7 @@ def parse_ed_diff(text): old_end = int(o.group(2)) if len(o.group(2)) else old hunk_kind = o.group(3) - if hunk_kind == "d": + if hunk_kind == 'd': k = 0 while old_end >= old: changes.append(Change(old + k, None, None, hunk_n)) @@ -832,7 +832,7 @@ def parse_ed_diff(text): while len(hunk) > 0: e = ed_hunk_end.match(hunk[0]) - if not e and hunk_kind == "c": + if not e and hunk_kind == 'c': k = 0 while old_end >= old: changes.append(Change(old + k, None, None, hunk_n)) @@ -852,7 +852,7 @@ def parse_ed_diff(text): ) i += 1 j += 1 - if not e and hunk_kind == "a": + if not e and hunk_kind == 'a': changes.append( Change( None, @@ -900,7 +900,7 @@ def parse_rcs_ed_diff(text): old = int(o.group(2)) size = int(o.group(3)) - if hunk_kind == "a": + if hunk_kind == 'a': old += total_change_size + 1 total_change_size += size while size > 0 and len(hunk) > 0: @@ -910,7 +910,7 @@ def parse_rcs_ed_diff(text): del hunk[0] - elif hunk_kind == "d": + elif hunk_kind == 'd': total_change_size -= size while size > 0: changes.append(Change(old + j, None, None, hunk_n)) @@ -938,8 +938,8 @@ def parse_git_binary_diff(text): # the sizes are used as latch-up new_size = 0 old_size = 0 - old_encoded = "" - new_encoded = "" + old_encoded = '' + new_encoded = '' for line in lines: if cmd_old_path is None and cmd_new_path is None: hm = git_diffcmd_header.match(line) @@ -978,11 +978,11 @@ def parse_git_binary_diff(text): change = Change(None, 0, added_data, None) changes.append(change) new_size = 0 - new_encoded = "" + new_encoded = '' else: # Invalid line format new_size = 0 - new_encoded = "" + new_encoded = '' # the second is removed file if old_size == 0: @@ -1006,10 +1006,10 @@ def parse_git_binary_diff(text): change = Change(0, None, None, removed_data) changes.append(change) old_size = 0 - old_encoded = "" + old_encoded = '' else: # Invalid line format old_size = 0 - old_encoded = "" + old_encoded = '' return changes diff --git a/openhands/resolver/patching/snippets.py b/openhands/resolver/patching/snippets.py index 710b1191b560..f9d9e620d0f7 100644 --- a/openhands/resolver/patching/snippets.py +++ b/openhands/resolver/patching/snippets.py @@ -54,7 +54,7 @@ def is_exe(fpath): if is_exe(program): return program else: - for path in os.environ["PATH"].split(os.pathsep): + for path in os.environ['PATH'].split(os.pathsep): path = path.strip('"') exe_file = os.path.join(path, program) if is_exe(exe_file): diff --git a/openhands/resolver/prompts/repo_instructions/all-hands-ai___openhands-resolver.txt b/openhands/resolver/prompts/repo_instructions/all-hands-ai___openhands-resolver.txt index ca040d591683..e4bc3b165d28 100644 --- a/openhands/resolver/prompts/repo_instructions/all-hands-ai___openhands-resolver.txt +++ b/openhands/resolver/prompts/repo_instructions/all-hands-ai___openhands-resolver.txt @@ -1,4 +1,4 @@ This is a Python repo for openhands-resolver, a library that attempts to resolve github issues with the AI agent OpenHands. - Setup: `poetry install --with test --with dev` -- Testing: `poetry run pytest tests/test_*.py` \ No newline at end of file +- Testing: `poetry run pytest tests/test_*.py` diff --git a/openhands/resolver/prompts/repo_instructions/rbren___rss-parser.txt b/openhands/resolver/prompts/repo_instructions/rbren___rss-parser.txt index b6e8fba1a200..5ef52d64c35d 100644 --- a/openhands/resolver/prompts/repo_instructions/rbren___rss-parser.txt +++ b/openhands/resolver/prompts/repo_instructions/rbren___rss-parser.txt @@ -1,4 +1,4 @@ This is a node repo for an RSS parser. - Setup: `yes | npm install` - Testing: `SKIP_BROWSER_TESTS=1 npm test` -- Writing Tests: Add to the `test` directory. \ No newline at end of file +- Writing Tests: Add to the `test` directory. diff --git a/openhands/resolver/prompts/resolve/basic-with-tests.jinja b/openhands/resolver/prompts/resolve/basic-with-tests.jinja index 54c35910ec62..595489c42841 100644 --- a/openhands/resolver/prompts/resolve/basic-with-tests.jinja +++ b/openhands/resolver/prompts/resolve/basic-with-tests.jinja @@ -14,4 +14,4 @@ For all changes to actual application code (e.g. in Python or Javascript), add a Run the tests, and if they pass you are done! You do NOT need to write new tests if there are only changes to documentation or configuration files. -When you think you have fixed the issue through code changes, please call the finish action to end the interaction. \ No newline at end of file +When you think you have fixed the issue through code changes, please call the finish action to end the interaction. diff --git a/openhands/resolver/prompts/resolve/basic.jinja b/openhands/resolver/prompts/resolve/basic.jinja index b3bec7ef7f53..a5bb806cc4df 100644 --- a/openhands/resolver/prompts/resolve/basic.jinja +++ b/openhands/resolver/prompts/resolve/basic.jinja @@ -10,4 +10,4 @@ You SHOULD INCLUDE PROPER INDENTATION in your edit commands.{% if repo_instructi Some basic information about this repository: {{ repo_instruction }}{% endif %} -When you think you have fixed the issue through code changes, please finish the interaction. \ No newline at end of file +When you think you have fixed the issue through code changes, please finish the interaction. diff --git a/openhands/resolver/review_pr.py b/openhands/resolver/review_pr.py index b9d6dc92b28d..e9fe27cc1c32 100644 --- a/openhands/resolver/review_pr.py +++ b/openhands/resolver/review_pr.py @@ -118,36 +118,22 @@ def main() -> None: args = parser.parse_args() - # Get environment variables - token = os.environ.get('GITHUB_TOKEN') - username = os.environ.get('GITHUB_USERNAME') - llm_model = os.environ.get('LLM_MODEL') - llm_api_key = os.environ.get('LLM_API_KEY') - llm_base_url = os.environ.get('LLM_BASE_URL') - - if not all([token, username, llm_model, llm_api_key]): - raise ValueError('Missing required environment variables') - # Split repo into owner and name owner, repo = args.repo.split('/') # Configure LLM - assert llm_model is not None # For type checker - assert llm_api_key is not None # For type checker llm_config = LLMConfig( - model=llm_model, - api_key=llm_api_key, - base_url=llm_base_url, + model=os.environ['LLM_MODEL'], + api_key=os.environ['LLM_API_KEY'], + base_url=os.environ.get('LLM_BASE_URL'), ) # Review PR - assert token is not None # For type checker - assert username is not None # For type checker review_pr( owner=owner, repo=repo, - token=token, - username=username, + token=os.environ['GITHUB_TOKEN'], + username=os.environ['GITHUB_USERNAME'], output_dir='/tmp/output', llm_config=llm_config, issue_number=args.issue_number, diff --git a/openhands/runtime/utils/command.py b/openhands/runtime/utils/command.py index 35a1252336c0..3a32d45fb7e1 100644 --- a/openhands/runtime/utils/command.py +++ b/openhands/runtime/utils/command.py @@ -38,7 +38,7 @@ def get_remote_startup_command( '-20', # Highest priority 'sh', '-c', - f'echo -1000 > /proc/self/oom_score_adj && exec {cmd_str}' + f'echo -1000 > /proc/self/oom_score_adj && exec {cmd_str}', ] else: # If not root, run with normal priority diff --git a/poetry.lock b/poetry.lock index cb51e9499bac..6c3efc7b10f4 100644 --- a/poetry.lock +++ b/poetry.lock @@ -10249,4 +10249,4 @@ testing = ["coverage[toml]", "zope.event", "zope.testing"] [metadata] lock-version = "2.0" python-versions = "^3.12" -content-hash = "9031e73b8930c9031acd554c2c225be41d673e275c4626dbe432c79eeb783065" +content-hash = "e7a29a0e396d5515ecb381b0f8bcfc74fd5a1d839884b23bc336db7df312a5a7" diff --git a/pyproject.toml b/pyproject.toml index 80b4953bd446..ae175525f0fe 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -64,6 +64,7 @@ modal = "^0.66.26" runloop-api-client = "0.7.0" pygithub = "^2.5.0" openhands-aci = "^0.1.1" +pre-commit = "^4.0.1" [tool.poetry.group.llama-index.dependencies] llama-index = "*" diff --git a/tests/unit/resolver/test_guess_success.py b/tests/unit/resolver/test_guess_success.py index 9bf3da2b3d02..d6b0e946adda 100644 --- a/tests/unit/resolver/test_guess_success.py +++ b/tests/unit/resolver/test_guess_success.py @@ -1,22 +1,22 @@ -from openhands.resolver.issue_definitions import IssueHandler -from openhands.resolver.github_issue import GithubIssue -from openhands.events.action.message import MessageAction from openhands.core.config import LLMConfig +from openhands.events.action.message import MessageAction +from openhands.resolver.github_issue import GithubIssue +from openhands.resolver.issue_definitions import IssueHandler def test_guess_success_multiline_explanation(): # Mock data issue = GithubIssue( - owner="test", - repo="test", + owner='test', + repo='test', number=1, - title="Test Issue", - body="Test body", + title='Test Issue', + body='Test body', thread_comments=None, review_comments=None, ) - history = [MessageAction(content="Test message")] - llm_config = LLMConfig(model="test", api_key="test") + history = [MessageAction(content='Test message')] + llm_config = LLMConfig(model='test', api_key='test') # Create a mock response with multi-line explanation mock_response = """--- success @@ -31,7 +31,7 @@ def test_guess_success_multiline_explanation(): Automatic fix generated by OpenHands 🙌""" # Create a handler instance - handler = IssueHandler("test", "test", "test") + handler = IssueHandler('test', 'test', 'test') # Mock the litellm.completion call def mock_completion(*args, **kwargs): @@ -61,11 +61,11 @@ def __init__(self, content): # Verify the results assert success is True - assert "The PR successfully addressed the issue by:" in explanation - assert "Fixed bug A" in explanation - assert "Added test B" in explanation - assert "Updated documentation C" in explanation - assert "Automatic fix generated by OpenHands" in explanation + assert 'The PR successfully addressed the issue by:' in explanation + assert 'Fixed bug A' in explanation + assert 'Added test B' in explanation + assert 'Updated documentation C' in explanation + assert 'Automatic fix generated by OpenHands' in explanation finally: # Restore the original function litellm.completion = original_completion diff --git a/tests/unit/resolver/test_issue_handler_error_handling.py b/tests/unit/resolver/test_issue_handler_error_handling.py index 54adff3466fd..2b04e3b13111 100644 --- a/tests/unit/resolver/test_issue_handler_error_handling.py +++ b/tests/unit/resolver/test_issue_handler_error_handling.py @@ -1,94 +1,97 @@ -import pytest +from unittest.mock import MagicMock, patch + import requests -from unittest.mock import patch, MagicMock from openhands.resolver.issue_definitions import PRHandler -from openhands.resolver.github_issue import ReviewThread def test_handle_nonexistent_issue_reference(): - handler = PRHandler("test-owner", "test-repo", "test-token") - + handler = PRHandler('test-owner', 'test-repo', 'test-token') + # Mock the requests.get to simulate a 404 error mock_response = MagicMock() - mock_response.raise_for_status.side_effect = requests.exceptions.HTTPError("404 Client Error: Not Found") - + mock_response.raise_for_status.side_effect = requests.exceptions.HTTPError( + '404 Client Error: Not Found' + ) + with patch('requests.get', return_value=mock_response): # Call the method with a non-existent issue reference result = handler._PRHandler__get_context_from_external_issues_references( closing_issues=[], closing_issue_numbers=[], - issue_body="This references #999999", # Non-existent issue + issue_body='This references #999999', # Non-existent issue review_comments=[], review_threads=[], - thread_comments=None + thread_comments=None, ) - + # The method should return an empty list since the referenced issue couldn't be fetched assert result == [] def test_handle_rate_limit_error(): - handler = PRHandler("test-owner", "test-repo", "test-token") - + handler = PRHandler('test-owner', 'test-repo', 'test-token') + # Mock the requests.get to simulate a rate limit error mock_response = MagicMock() mock_response.raise_for_status.side_effect = requests.exceptions.HTTPError( - "403 Client Error: Rate Limit Exceeded" + '403 Client Error: Rate Limit Exceeded' ) - + with patch('requests.get', return_value=mock_response): # Call the method with an issue reference result = handler._PRHandler__get_context_from_external_issues_references( closing_issues=[], closing_issue_numbers=[], - issue_body="This references #123", + issue_body='This references #123', review_comments=[], review_threads=[], - thread_comments=None + thread_comments=None, ) - + # The method should return an empty list since the request was rate limited assert result == [] def test_handle_network_error(): - handler = PRHandler("test-owner", "test-repo", "test-token") - + handler = PRHandler('test-owner', 'test-repo', 'test-token') + # Mock the requests.get to simulate a network error - with patch('requests.get', side_effect=requests.exceptions.ConnectionError("Network Error")): + with patch( + 'requests.get', side_effect=requests.exceptions.ConnectionError('Network Error') + ): # Call the method with an issue reference result = handler._PRHandler__get_context_from_external_issues_references( closing_issues=[], closing_issue_numbers=[], - issue_body="This references #123", + issue_body='This references #123', review_comments=[], review_threads=[], - thread_comments=None + thread_comments=None, ) - + # The method should return an empty list since the network request failed assert result == [] def test_successful_issue_reference(): - handler = PRHandler("test-owner", "test-repo", "test-token") - + handler = PRHandler('test-owner', 'test-repo', 'test-token') + # Mock a successful response mock_response = MagicMock() mock_response.raise_for_status.return_value = None - mock_response.json.return_value = {"body": "This is the referenced issue body"} - + mock_response.json.return_value = {'body': 'This is the referenced issue body'} + with patch('requests.get', return_value=mock_response): # Call the method with an issue reference result = handler._PRHandler__get_context_from_external_issues_references( closing_issues=[], closing_issue_numbers=[], - issue_body="This references #123", + issue_body='This references #123', review_comments=[], review_threads=[], - thread_comments=None + thread_comments=None, ) - + # The method should return a list with the referenced issue body - assert result == ["This is the referenced issue body"] \ No newline at end of file + assert result == ['This is the referenced issue body'] diff --git a/tests/unit/resolver/test_issue_references.py b/tests/unit/resolver/test_issue_references.py index e4da644983db..1252f8555540 100644 --- a/tests/unit/resolver/test_issue_references.py +++ b/tests/unit/resolver/test_issue_references.py @@ -2,13 +2,13 @@ def test_extract_issue_references(): - handler = IssueHandler("test-owner", "test-repo", "test-token") + handler = IssueHandler('test-owner', 'test-repo', 'test-token') # Test basic issue reference - assert handler._extract_issue_references("Fixes #123") == [123] + assert handler._extract_issue_references('Fixes #123') == [123] # Test multiple issue references - assert handler._extract_issue_references("Fixes #123, #456") == [123, 456] + assert handler._extract_issue_references('Fixes #123, #456') == [123, 456] # Test issue references in code blocks should be ignored assert handler._extract_issue_references(""" @@ -22,13 +22,21 @@ def func(): """) == [789] # Test issue references in inline code should be ignored - assert handler._extract_issue_references("This `#123` should be ignored but #456 should be extracted") == [456] + assert handler._extract_issue_references( + 'This `#123` should be ignored but #456 should be extracted' + ) == [456] # Test issue references in URLs should be ignored - assert handler._extract_issue_references("Check http://example.com/#123 but #456 should be extracted") == [456] + assert handler._extract_issue_references( + 'Check http://example.com/#123 but #456 should be extracted' + ) == [456] # Test issue references in markdown links should be extracted - assert handler._extract_issue_references("[Link to #123](http://example.com) and #456") == [123, 456] + assert handler._extract_issue_references( + '[Link to #123](http://example.com) and #456' + ) == [123, 456] # Test issue references with text around them - assert handler._extract_issue_references("Issue #123 is fixed and #456 is pending") == [123, 456] + assert handler._extract_issue_references( + 'Issue #123 is fixed and #456 is pending' + ) == [123, 456] diff --git a/tests/unit/resolver/test_pr_handler_guess_success.py b/tests/unit/resolver/test_pr_handler_guess_success.py index bc29fbe2632e..e7e7705e8747 100644 --- a/tests/unit/resolver/test_pr_handler_guess_success.py +++ b/tests/unit/resolver/test_pr_handler_guess_success.py @@ -1,39 +1,39 @@ import json -from unittest.mock import patch, MagicMock +from unittest.mock import MagicMock, patch -from openhands.resolver.issue_definitions import PRHandler -from openhands.resolver.github_issue import GithubIssue, ReviewThread -from openhands.events.action.message import MessageAction from openhands.core.config import LLMConfig +from openhands.events.action.message import MessageAction +from openhands.resolver.github_issue import GithubIssue, ReviewThread +from openhands.resolver.issue_definitions import PRHandler def test_guess_success_review_threads_litellm_call(): """Test that the litellm.completion() call for review threads contains the expected content.""" # Create a PR handler instance - handler = PRHandler("test-owner", "test-repo", "test-token") + handler = PRHandler('test-owner', 'test-repo', 'test-token') # Create a mock issue with review threads issue = GithubIssue( - owner="test-owner", - repo="test-repo", + owner='test-owner', + repo='test-repo', number=1, - title="Test PR", - body="Test Body", + title='Test PR', + body='Test Body', thread_comments=None, - closing_issues=["Issue 1 description", "Issue 2 description"], + closing_issues=['Issue 1 description', 'Issue 2 description'], review_comments=None, review_threads=[ ReviewThread( - comment="Please fix the formatting\n---\nlatest feedback:\nAdd docstrings", - files=["/src/file1.py", "/src/file2.py"], + comment='Please fix the formatting\n---\nlatest feedback:\nAdd docstrings', + files=['/src/file1.py', '/src/file2.py'], ), ReviewThread( - comment="Add more tests\n---\nlatest feedback:\nAdd test cases", - files=["/tests/test_file.py"], + comment='Add more tests\n---\nlatest feedback:\nAdd test cases', + files=['/tests/test_file.py'], ), ], - thread_ids=["1", "2"], - head_branch="test-branch", + thread_ids=['1', '2'], + head_branch='test-branch', ) # Create mock history with a detailed response @@ -47,7 +47,7 @@ def test_guess_success_review_threads_litellm_call(): ] # Create mock LLM config - llm_config = LLMConfig(model="test-model", api_key="test-key") + llm_config = LLMConfig(model='test-model', api_key='test-key') # Mock the LLM response mock_response = MagicMock() @@ -64,7 +64,7 @@ def test_guess_success_review_threads_litellm_call(): ] # Test the guess_success method - with patch("litellm.completion") as mock_completion: + with patch('litellm.completion') as mock_completion: mock_completion.return_value = mock_response success, success_list, explanation = handler.guess_success( issue, history, llm_config @@ -75,63 +75,63 @@ def test_guess_success_review_threads_litellm_call(): # Check first call first_call = mock_completion.call_args_list[0] - first_prompt = first_call[1]["messages"][0]["content"] + first_prompt = first_call[1]['messages'][0]['content'] assert ( - "Issue descriptions:\n" - + json.dumps(["Issue 1 description", "Issue 2 description"], indent=4) + 'Issue descriptions:\n' + + json.dumps(['Issue 1 description', 'Issue 2 description'], indent=4) in first_prompt ) assert ( - "Feedback:\nPlease fix the formatting\n---\nlatest feedback:\nAdd docstrings" + 'Feedback:\nPlease fix the formatting\n---\nlatest feedback:\nAdd docstrings' in first_prompt ) assert ( - "Files locations:\n" - + json.dumps(["/src/file1.py", "/src/file2.py"], indent=4) + 'Files locations:\n' + + json.dumps(['/src/file1.py', '/src/file2.py'], indent=4) in first_prompt ) - assert "Last message from AI agent:\n" + history[0].content in first_prompt + assert 'Last message from AI agent:\n' + history[0].content in first_prompt # Check second call second_call = mock_completion.call_args_list[1] - second_prompt = second_call[1]["messages"][0]["content"] + second_prompt = second_call[1]['messages'][0]['content'] assert ( - "Issue descriptions:\n" - + json.dumps(["Issue 1 description", "Issue 2 description"], indent=4) + 'Issue descriptions:\n' + + json.dumps(['Issue 1 description', 'Issue 2 description'], indent=4) in second_prompt ) assert ( - "Feedback:\nAdd more tests\n---\nlatest feedback:\nAdd test cases" + 'Feedback:\nAdd more tests\n---\nlatest feedback:\nAdd test cases' in second_prompt ) assert ( - "Files locations:\n" + json.dumps(["/tests/test_file.py"], indent=4) + 'Files locations:\n' + json.dumps(['/tests/test_file.py'], indent=4) in second_prompt ) - assert "Last message from AI agent:\n" + history[0].content in second_prompt + assert 'Last message from AI agent:\n' + history[0].content in second_prompt def test_guess_success_thread_comments_litellm_call(): """Test that the litellm.completion() call for thread comments contains the expected content.""" # Create a PR handler instance - handler = PRHandler("test-owner", "test-repo", "test-token") + handler = PRHandler('test-owner', 'test-repo', 'test-token') # Create a mock issue with thread comments issue = GithubIssue( - owner="test-owner", - repo="test-repo", + owner='test-owner', + repo='test-repo', number=1, - title="Test PR", - body="Test Body", + title='Test PR', + body='Test Body', thread_comments=[ - "Please improve error handling", - "Add input validation", - "latest feedback:\nHandle edge cases", + 'Please improve error handling', + 'Add input validation', + 'latest feedback:\nHandle edge cases', ], - closing_issues=["Issue 1 description", "Issue 2 description"], + closing_issues=['Issue 1 description', 'Issue 2 description'], review_comments=None, thread_ids=None, - head_branch="test-branch", + head_branch='test-branch', ) # Create mock history with a detailed response @@ -145,7 +145,7 @@ def test_guess_success_thread_comments_litellm_call(): ] # Create mock LLM config - llm_config = LLMConfig(model="test-model", api_key="test-key") + llm_config = LLMConfig(model='test-model', api_key='test-key') # Mock the LLM response mock_response = MagicMock() @@ -162,7 +162,7 @@ def test_guess_success_thread_comments_litellm_call(): ] # Test the guess_success method - with patch("litellm.completion") as mock_completion: + with patch('litellm.completion') as mock_completion: mock_completion.return_value = mock_response success, success_list, explanation = handler.guess_success( issue, history, llm_config @@ -171,77 +171,77 @@ def test_guess_success_thread_comments_litellm_call(): # Verify the litellm.completion() call mock_completion.assert_called_once() call_args = mock_completion.call_args - prompt = call_args[1]["messages"][0]["content"] + prompt = call_args[1]['messages'][0]['content'] # Check prompt content assert ( - "Issue descriptions:\n" - + json.dumps(["Issue 1 description", "Issue 2 description"], indent=4) + 'Issue descriptions:\n' + + json.dumps(['Issue 1 description', 'Issue 2 description'], indent=4) in prompt ) - assert "PR Thread Comments:\n" + "\n---\n".join(issue.thread_comments) in prompt - assert "Last message from AI agent:\n" + history[0].content in prompt + assert 'PR Thread Comments:\n' + '\n---\n'.join(issue.thread_comments) in prompt + assert 'Last message from AI agent:\n' + history[0].content in prompt def test_check_feedback_with_llm(): """Test the _check_feedback_with_llm helper function.""" # Create a PR handler instance - handler = PRHandler("test-owner", "test-repo", "test-token") + handler = PRHandler('test-owner', 'test-repo', 'test-token') # Create mock LLM config - llm_config = LLMConfig(model="test-model", api_key="test-key") + llm_config = LLMConfig(model='test-model', api_key='test-key') # Test cases for different LLM responses test_cases = [ { - "response": "--- success\ntrue\n--- explanation\nChanges look good", - "expected": (True, "Changes look good"), + 'response': '--- success\ntrue\n--- explanation\nChanges look good', + 'expected': (True, 'Changes look good'), }, { - "response": "--- success\nfalse\n--- explanation\nNot all issues fixed", - "expected": (False, "Not all issues fixed"), + 'response': '--- success\nfalse\n--- explanation\nNot all issues fixed', + 'expected': (False, 'Not all issues fixed'), }, { - "response": "Invalid response format", - "expected": ( + 'response': 'Invalid response format', + 'expected': ( False, - "Failed to decode answer from LLM response: Invalid response format", + 'Failed to decode answer from LLM response: Invalid response format', ), }, { - "response": "--- success\ntrue\n--- explanation\nMultiline\nexplanation\nhere", - "expected": (True, "Multiline\nexplanation\nhere"), + 'response': '--- success\ntrue\n--- explanation\nMultiline\nexplanation\nhere', + 'expected': (True, 'Multiline\nexplanation\nhere'), }, ] for case in test_cases: # Mock the LLM response mock_response = MagicMock() - mock_response.choices = [MagicMock(message=MagicMock(content=case["response"]))] + mock_response.choices = [MagicMock(message=MagicMock(content=case['response']))] # Test the function - with patch("litellm.completion", return_value=mock_response): + with patch('litellm.completion', return_value=mock_response): success, explanation = handler._check_feedback_with_llm( - "test prompt", llm_config + 'test prompt', llm_config ) - assert (success, explanation) == case["expected"] + assert (success, explanation) == case['expected'] def test_check_review_thread(): """Test the _check_review_thread helper function.""" # Create a PR handler instance - handler = PRHandler("test-owner", "test-repo", "test-token") + handler = PRHandler('test-owner', 'test-repo', 'test-token') # Create test data review_thread = ReviewThread( - comment="Please fix the formatting\n---\nlatest feedback:\nAdd docstrings", - files=["/src/file1.py", "/src/file2.py"], + comment='Please fix the formatting\n---\nlatest feedback:\nAdd docstrings', + files=['/src/file1.py', '/src/file2.py'], ) issues_context = json.dumps( - ["Issue 1 description", "Issue 2 description"], indent=4 + ['Issue 1 description', 'Issue 2 description'], indent=4 ) - last_message = "I have fixed the formatting and added docstrings" - llm_config = LLMConfig(model="test-model", api_key="test-key") + last_message = 'I have fixed the formatting and added docstrings' + llm_config = LLMConfig(model='test-model', api_key='test-key') # Mock the LLM response mock_response = MagicMock() @@ -258,7 +258,7 @@ def test_check_review_thread(): ] # Test the function - with patch("litellm.completion") as mock_completion: + with patch('litellm.completion') as mock_completion: mock_completion.return_value = mock_response success, explanation = handler._check_review_thread( review_thread, issues_context, last_message, llm_config @@ -267,37 +267,37 @@ def test_check_review_thread(): # Verify the litellm.completion() call mock_completion.assert_called_once() call_args = mock_completion.call_args - prompt = call_args[1]["messages"][0]["content"] + prompt = call_args[1]['messages'][0]['content'] # Check prompt content - assert "Issue descriptions:\n" + issues_context in prompt - assert "Feedback:\n" + review_thread.comment in prompt + assert 'Issue descriptions:\n' + issues_context in prompt + assert 'Feedback:\n' + review_thread.comment in prompt assert ( - "Files locations:\n" + json.dumps(review_thread.files, indent=4) in prompt + 'Files locations:\n' + json.dumps(review_thread.files, indent=4) in prompt ) - assert "Last message from AI agent:\n" + last_message in prompt + assert 'Last message from AI agent:\n' + last_message in prompt # Check result assert success is True - assert explanation == "Changes look good" + assert explanation == 'Changes look good' def test_check_thread_comments(): """Test the _check_thread_comments helper function.""" # Create a PR handler instance - handler = PRHandler("test-owner", "test-repo", "test-token") + handler = PRHandler('test-owner', 'test-repo', 'test-token') # Create test data thread_comments = [ - "Please improve error handling", - "Add input validation", - "latest feedback:\nHandle edge cases", + 'Please improve error handling', + 'Add input validation', + 'latest feedback:\nHandle edge cases', ] issues_context = json.dumps( - ["Issue 1 description", "Issue 2 description"], indent=4 + ['Issue 1 description', 'Issue 2 description'], indent=4 ) - last_message = "I have added error handling and input validation" - llm_config = LLMConfig(model="test-model", api_key="test-key") + last_message = 'I have added error handling and input validation' + llm_config = LLMConfig(model='test-model', api_key='test-key') # Mock the LLM response mock_response = MagicMock() @@ -314,7 +314,7 @@ def test_check_thread_comments(): ] # Test the function - with patch("litellm.completion") as mock_completion: + with patch('litellm.completion') as mock_completion: mock_completion.return_value = mock_response success, explanation = handler._check_thread_comments( thread_comments, issues_context, last_message, llm_config @@ -323,34 +323,34 @@ def test_check_thread_comments(): # Verify the litellm.completion() call mock_completion.assert_called_once() call_args = mock_completion.call_args - prompt = call_args[1]["messages"][0]["content"] + prompt = call_args[1]['messages'][0]['content'] # Check prompt content - assert "Issue descriptions:\n" + issues_context in prompt - assert "PR Thread Comments:\n" + "\n---\n".join(thread_comments) in prompt - assert "Last message from AI agent:\n" + last_message in prompt + assert 'Issue descriptions:\n' + issues_context in prompt + assert 'PR Thread Comments:\n' + '\n---\n'.join(thread_comments) in prompt + assert 'Last message from AI agent:\n' + last_message in prompt # Check result assert success is True - assert explanation == "Changes look good" + assert explanation == 'Changes look good' def test_check_review_comments(): """Test the _check_review_comments helper function.""" # Create a PR handler instance - handler = PRHandler("test-owner", "test-repo", "test-token") + handler = PRHandler('test-owner', 'test-repo', 'test-token') # Create test data review_comments = [ - "Please improve code readability", - "Add comments to complex functions", - "Follow PEP 8 style guide", + 'Please improve code readability', + 'Add comments to complex functions', + 'Follow PEP 8 style guide', ] issues_context = json.dumps( - ["Issue 1 description", "Issue 2 description"], indent=4 + ['Issue 1 description', 'Issue 2 description'], indent=4 ) - last_message = "I have improved code readability and added comments" - llm_config = LLMConfig(model="test-model", api_key="test-key") + last_message = 'I have improved code readability and added comments' + llm_config = LLMConfig(model='test-model', api_key='test-key') # Mock the LLM response mock_response = MagicMock() @@ -367,7 +367,7 @@ def test_check_review_comments(): ] # Test the function - with patch("litellm.completion") as mock_completion: + with patch('litellm.completion') as mock_completion: mock_completion.return_value = mock_response success, explanation = handler._check_review_comments( review_comments, issues_context, last_message, llm_config @@ -376,39 +376,39 @@ def test_check_review_comments(): # Verify the litellm.completion() call mock_completion.assert_called_once() call_args = mock_completion.call_args - prompt = call_args[1]["messages"][0]["content"] + prompt = call_args[1]['messages'][0]['content'] # Check prompt content - assert "Issue descriptions:\n" + issues_context in prompt - assert "PR Review Comments:\n" + "\n---\n".join(review_comments) in prompt - assert "Last message from AI agent:\n" + last_message in prompt + assert 'Issue descriptions:\n' + issues_context in prompt + assert 'PR Review Comments:\n' + '\n---\n'.join(review_comments) in prompt + assert 'Last message from AI agent:\n' + last_message in prompt # Check result assert success is True - assert explanation == "Changes look good" + assert explanation == 'Changes look good' def test_guess_success_review_comments_litellm_call(): """Test that the litellm.completion() call for review comments contains the expected content.""" # Create a PR handler instance - handler = PRHandler("test-owner", "test-repo", "test-token") + handler = PRHandler('test-owner', 'test-repo', 'test-token') # Create a mock issue with review comments issue = GithubIssue( - owner="test-owner", - repo="test-repo", + owner='test-owner', + repo='test-repo', number=1, - title="Test PR", - body="Test Body", + title='Test PR', + body='Test Body', thread_comments=None, - closing_issues=["Issue 1 description", "Issue 2 description"], + closing_issues=['Issue 1 description', 'Issue 2 description'], review_comments=[ - "Please improve code readability", - "Add comments to complex functions", - "Follow PEP 8 style guide", + 'Please improve code readability', + 'Add comments to complex functions', + 'Follow PEP 8 style guide', ], thread_ids=None, - head_branch="test-branch", + head_branch='test-branch', ) # Create mock history with a detailed response @@ -422,7 +422,7 @@ def test_guess_success_review_comments_litellm_call(): ] # Create mock LLM config - llm_config = LLMConfig(model="test-model", api_key="test-key") + llm_config = LLMConfig(model='test-model', api_key='test-key') # Mock the LLM response mock_response = MagicMock() @@ -439,7 +439,7 @@ def test_guess_success_review_comments_litellm_call(): ] # Test the guess_success method - with patch("litellm.completion") as mock_completion: + with patch('litellm.completion') as mock_completion: mock_completion.return_value = mock_response success, success_list, explanation = handler.guess_success( issue, history, llm_config @@ -448,13 +448,13 @@ def test_guess_success_review_comments_litellm_call(): # Verify the litellm.completion() call mock_completion.assert_called_once() call_args = mock_completion.call_args - prompt = call_args[1]["messages"][0]["content"] + prompt = call_args[1]['messages'][0]['content'] # Check prompt content assert ( - "Issue descriptions:\n" - + json.dumps(["Issue 1 description", "Issue 2 description"], indent=4) + 'Issue descriptions:\n' + + json.dumps(['Issue 1 description', 'Issue 2 description'], indent=4) in prompt ) - assert "PR Review Comments:\n" + "\n---\n".join(issue.review_comments) in prompt - assert "Last message from AI agent:\n" + history[0].content in prompt + assert 'PR Review Comments:\n' + '\n---\n'.join(issue.review_comments) in prompt + assert 'Last message from AI agent:\n' + history[0].content in prompt diff --git a/tests/unit/resolver/test_pr_title_escaping.py b/tests/unit/resolver/test_pr_title_escaping.py index 03f2b7104807..45dd523b036a 100644 --- a/tests/unit/resolver/test_pr_title_escaping.py +++ b/tests/unit/resolver/test_pr_title_escaping.py @@ -1,45 +1,46 @@ -from openhands.resolver.github_issue import GithubIssue -from openhands.resolver.send_pull_request import make_commit import os -import tempfile import subprocess +import tempfile + +from openhands.resolver.github_issue import GithubIssue +from openhands.resolver.send_pull_request import make_commit def test_commit_message_with_quotes(): # Create a temporary directory and initialize git repo with tempfile.TemporaryDirectory() as temp_dir: - subprocess.run(["git", "init", temp_dir], check=True) + subprocess.run(['git', 'init', temp_dir], check=True) # Create a test file and add it to git - test_file = os.path.join(temp_dir, "test.txt") - with open(test_file, "w") as f: - f.write("test content") + test_file = os.path.join(temp_dir, 'test.txt') + with open(test_file, 'w') as f: + f.write('test content') - subprocess.run(["git", "-C", temp_dir, "add", "test.txt"], check=True) + subprocess.run(['git', '-C', temp_dir, 'add', 'test.txt'], check=True) # Create a test issue with problematic title issue = GithubIssue( - owner="test-owner", - repo="test-repo", + owner='test-owner', + repo='test-repo', number=123, title="Issue with 'quotes' and \"double quotes\" and ", - body="Test body", + body='Test body', labels=[], assignees=[], - state="open", - created_at="2024-01-01T00:00:00Z", - updated_at="2024-01-01T00:00:00Z", + state='open', + created_at='2024-01-01T00:00:00Z', + updated_at='2024-01-01T00:00:00Z', closed_at=None, head_branch=None, thread_ids=None, ) # Make the commit - make_commit(temp_dir, issue, "issue") + make_commit(temp_dir, issue, 'issue') # Get the commit message result = subprocess.run( - ["git", "-C", temp_dir, "log", "-1", "--pretty=%B"], + ['git', '-C', temp_dir, 'log', '-1', '--pretty=%B'], capture_output=True, text=True, check=True, @@ -48,7 +49,7 @@ def test_commit_message_with_quotes(): # The commit message should contain the quotes without excessive escaping expected = "Fix issue #123: Issue with 'quotes' and \"double quotes\" and " - assert commit_msg == expected, f"Expected: {expected}\nGot: {commit_msg}" + assert commit_msg == expected, f'Expected: {expected}\nGot: {commit_msg}' def test_pr_title_with_quotes(monkeypatch): @@ -56,39 +57,39 @@ def test_pr_title_with_quotes(monkeypatch): class MockResponse: def __init__(self, status_code=201): self.status_code = status_code - self.text = "" + self.text = '' def json(self): - return {"html_url": "https://github.com/test/test/pull/1"} + return {'html_url': 'https://github.com/test/test/pull/1'} def raise_for_status(self): pass def mock_post(*args, **kwargs): # Verify that the PR title is not over-escaped - data = kwargs.get("json", {}) - title = data.get("title", "") + data = kwargs.get('json', {}) + title = data.get('title', '') expected = "Fix issue #123: Issue with 'quotes' and \"double quotes\" and " assert ( title == expected - ), f"PR title was incorrectly escaped.\nExpected: {expected}\nGot: {title}" + ), f'PR title was incorrectly escaped.\nExpected: {expected}\nGot: {title}' return MockResponse() class MockGetResponse: def __init__(self, status_code=200): self.status_code = status_code - self.text = "" + self.text = '' def json(self): - return {"default_branch": "main"} + return {'default_branch': 'main'} def raise_for_status(self): pass - monkeypatch.setattr("requests.post", mock_post) - monkeypatch.setattr("requests.get", lambda *args, **kwargs: MockGetResponse()) + monkeypatch.setattr('requests.post', mock_post) + monkeypatch.setattr('requests.get', lambda *args, **kwargs: MockGetResponse()) monkeypatch.setattr( - "openhands.resolver.send_pull_request.branch_exists", + 'openhands.resolver.send_pull_request.branch_exists', lambda *args, **kwargs: False, ) @@ -97,69 +98,69 @@ def raise_for_status(self): def mock_run(*args, **kwargs): print(f"Running command: {args[0] if args else kwargs.get('args', [])}") - if isinstance(args[0], list) and args[0][0] == "git": - if "push" in args[0]: + if isinstance(args[0], list) and args[0][0] == 'git': + if 'push' in args[0]: return subprocess.CompletedProcess( - args[0], returncode=0, stdout="", stderr="" + args[0], returncode=0, stdout='', stderr='' ) return original_run(*args, **kwargs) return original_run(*args, **kwargs) - monkeypatch.setattr("subprocess.run", mock_run) + monkeypatch.setattr('subprocess.run', mock_run) # Create a temporary directory and initialize git repo with tempfile.TemporaryDirectory() as temp_dir: - print("Initializing git repo...") - subprocess.run(["git", "init", temp_dir], check=True) + print('Initializing git repo...') + subprocess.run(['git', 'init', temp_dir], check=True) # Add these lines to configure git subprocess.run( - ["git", "-C", temp_dir, "config", "user.name", "Test User"], check=True + ['git', '-C', temp_dir, 'config', 'user.name', 'Test User'], check=True ) subprocess.run( - ["git", "-C", temp_dir, "config", "user.email", "test@example.com"], + ['git', '-C', temp_dir, 'config', 'user.email', 'test@example.com'], check=True, ) # Create a test file and add it to git - test_file = os.path.join(temp_dir, "test.txt") - with open(test_file, "w") as f: - f.write("test content") + test_file = os.path.join(temp_dir, 'test.txt') + with open(test_file, 'w') as f: + f.write('test content') - print("Adding and committing test file...") - subprocess.run(["git", "-C", temp_dir, "add", "test.txt"], check=True) + print('Adding and committing test file...') + subprocess.run(['git', '-C', temp_dir, 'add', 'test.txt'], check=True) subprocess.run( - ["git", "-C", temp_dir, "commit", "-m", "Initial commit"], check=True + ['git', '-C', temp_dir, 'commit', '-m', 'Initial commit'], check=True ) # Create a test issue with problematic title - print("Creating test issue...") + print('Creating test issue...') issue = GithubIssue( - owner="test-owner", - repo="test-repo", + owner='test-owner', + repo='test-repo', number=123, title="Issue with 'quotes' and \"double quotes\" and ", - body="Test body", + body='Test body', labels=[], assignees=[], - state="open", - created_at="2024-01-01T00:00:00Z", - updated_at="2024-01-01T00:00:00Z", + state='open', + created_at='2024-01-01T00:00:00Z', + updated_at='2024-01-01T00:00:00Z', closed_at=None, head_branch=None, thread_ids=None, ) # Try to send a PR - this will fail if the title is incorrectly escaped - print("Sending PR...") - from openhands.resolver.send_pull_request import send_pull_request + print('Sending PR...') from openhands.core.config import LLMConfig + from openhands.resolver.send_pull_request import send_pull_request send_pull_request( github_issue=issue, - github_token="dummy-token", - github_username="test-user", + github_token='dummy-token', + github_username='test-user', patch_dir=temp_dir, - llm_config=LLMConfig(model="test-model", api_key="test-key"), - pr_type="ready", + llm_config=LLMConfig(model='test-model', api_key='test-key'), + pr_type='ready', )