diff --git a/.github/workflows/openhands-code-reviewer.yml b/.github/workflows/openhands-code-reviewer.yml new file mode 100644 index 000000000000..fbf68a77d476 --- /dev/null +++ b/.github/workflows/openhands-code-reviewer.yml @@ -0,0 +1,369 @@ +name: Auto-Review PR with OpenHands + +on: + workflow_call: + inputs: + max_iterations: + required: false + type: number + default: 50 + review_macro: + required: false + type: string + default: "@openhands-reviewer" + review_level: + required: false + type: string + default: "line" # Default changed to line + description: "Level of review (e.g., 'line', 'file', 'pr')" + review_depth: + required: false + type: string + default: "deep" # Default changed to deep + description: "Depth of review (e.g., 'quick', 'deep')" + LLM_MODEL: + required: false + type: string + default: "anthropic/claude-3-5-sonnet-20241022" + LLM_API_VERSION: + required: false + type: string + default: "" + base_container_image: + required: false + type: string + default: "" + description: "Custom sandbox env" + secrets: + LLM_MODEL: + required: false + LLM_API_KEY: + required: true + APP_ID: + required: false + APP_PRIVATE_KEY: + required: false + LLM_BASE_URL: + required: false + PAT_TOKEN: + required: false + PAT_USERNAME: + required: false + + pull_request: + types: [opened, synchronize, reopened, labeled] + issue_comment: # Triggered when a comment is made on a PR (issues are treated as PRs in GitHub API for comments) + types: [created] + pull_request_review_comment: + types: [created] + pull_request_review: + types: [submitted] + +permissions: + contents: read # Read repo contents + pull-requests: write # Write comments/reviews + issues: write # Write comments (needed for issue_comment trigger on PRs) + +jobs: + auto-review: + permissions: + contents: read + pull-requests: write # Change to 'write' if the bot needs to post comments + if: | + github.event_name == 'workflow_call' || + ( + github.event_name == 'pull_request' && + ( + github.event.action == 'opened' || + github.event.action == 'reopened' || + github.event.action == 'synchronize' + ) + ) || + (github.event_name == 'pull_request' && github.event.action == 'labeled' && github.event.label.name == 'review-me') || + ( + (github.event_name == 'issue_comment' || github.event_name == 'pull_request_review_comment') && + contains(github.event.comment.body, inputs.review_macro || '@openhands-reviewer') && + (github.event.comment.author_association == 'OWNER' || github.event.comment.author_association == 'COLLABORATOR' || github.event.comment.author_association == 'MEMBER') && + github.event.issue.pull_request + ) || + ( + github.event_name == 'pull_request_review' && + contains(github.event.review.body, inputs.review_macro || '@openhands-reviewer') && + (github.event.review.author_association == 'OWNER' || github.event.review.author_association == 'COLLABORATOR' || github.event.review.author_association == 'MEMBER') + ) + runs-on: ubuntu-latest + env: + JOB_APP_ID: ${{ secrets.APP_ID }} + JOB_APP_PRIVATE_KEY: ${{ secrets.APP_PRIVATE_KEY }} + steps: + - name: Checkout repository + uses: actions/checkout@v4 + + - name: Set up Python + uses: actions/setup-python@v5 + with: + python-version: "3.12" + + - name: Generate GitHub App Token + id: generate-token + # Attempting cache bust + # Only run if App ID and Key are provided via secrets + if: ${{ env.JOB_APP_ID != '' && env.JOB_APP_PRIVATE_KEY != '' }} + uses: actions/create-github-app-token@v1 + with: + permission-contents: read + permission-issues: write + permission-pull-requests: write + app-id: ${{ secrets.APP_ID }} + private-key: ${{ secrets.APP_PRIVATE_KEY }} + + - name: Determine Auth Token + id: determine-auth-token + run: | + if [ -n "${{ steps.generate-token.outputs.token }}" ]; then + echo "Using GitHub App Token" + echo "AUTH_TOKEN=${{ steps.generate-token.outputs.token }}" >> $GITHUB_ENV + elif [ -n "${{ secrets.PAT_TOKEN }}" ]; then + echo "Using PAT Token" + echo "AUTH_TOKEN=${{ secrets.PAT_TOKEN }}" >> $GITHUB_ENV + else + echo "Using default GITHUB_TOKEN" + echo "AUTH_TOKEN=${{ github.token }}" >> $GITHUB_ENV + fi + - name: Log Auth Token Source + run: | + if [ -n "${{ steps.generate-token.outputs.token }}" ]; then + echo "Auth Token Source: GitHub App Token" + elif [ -n "${{ secrets.PAT_TOKEN }}" ]; then + echo "Auth Token Source: PAT Token" + else + echo "Auth Token Source: Default GITHUB_TOKEN" + fi + - name: Create requirements.txt and get branch SHA + id: setup_reqs_and_sha + env: + # Use the determined auth token for git clone and ls-remote + GIT_TOKEN: ${{ env.AUTH_TOKEN }} + run: | + echo "Using openhands-ai from remind101/OpenHands@feat/code-reviewer-impl" + # Create a new requirements.txt locally within the workflow + echo "git+https://${GIT_TOKEN}@github.com/remind101/OpenHands.git@feat/code-reviewer-impl#egg=openhands-ai" > /tmp/requirements.txt + cat /tmp/requirements.txt + + echo "Fetching latest commit SHA for feat/code-reviewer-impl..." + SHA=$(git ls-remote https://${GIT_TOKEN}@github.com/remind101/OpenHands.git refs/heads/feat/code-reviewer-impl | awk '{print $1}') + echo "Latest SHA: $SHA" + if [ -z "$SHA" ]; then + echo "Error: Could not retrieve SHA for feat/code-reviewer-impl branch." + exit 1 + fi + echo "OPENHANDS_BRANCH_SHA=$SHA" >> $GITHUB_ENV + + - name: Cache pip dependencies + if: | + !( + github.event.label.name == 'fix-me-experimental' || + ( + (github.event_name == 'issue_comment' || github.event_name == 'pull_request_review_comment') && + contains(github.event.comment.body, '@openhands-agent-exp') + ) || + ( + github.event_name == 'pull_request_review' && + contains(github.event.review.body, '@openhands-agent-exp') + ) + ) + uses: actions/cache@v4 + with: + path: ${{ env.pythonLocation }}/lib/python3.12/site-packages/* + key: ${{ runner.os }}-pip-openhands-resolver-${{ env.OPENHANDS_BRANCH_SHA }} + restore-keys: | + ${{ runner.os }}-pip-openhands-resolver-${{ env.OPENHANDS_BRANCH_SHA }} + ${{ runner.os }}-pip-openhands-resolver- + + - name: Check required environment variables + env: + LLM_MODEL: ${{ secrets.LLM_MODEL || inputs.LLM_MODEL }} + LLM_API_KEY: ${{ secrets.LLM_API_KEY }} + LLM_BASE_URL: ${{ secrets.LLM_BASE_URL }} + LLM_API_VERSION: ${{ inputs.LLM_API_VERSION }} + PAT_TOKEN: ${{ secrets.PAT_TOKEN }} + PAT_USERNAME: ${{ secrets.PAT_USERNAME }} + APP_TOKEN_GENERATED: ${{ steps.generate-token.outputs.token && 'true' || 'false' }} + run: | + required_vars=("LLM_API_KEY") + for var in "${required_vars[@]}"; do + if [ -z "${!var}" ]; then + echo "Error: Required environment variable $var is not set." + exit 1 + fi + done + + # Check optional variables and warn about fallbacks + if [ -z "$LLM_BASE_URL" ]; then + echo "Warning: LLM_BASE_URL is not set, will use default API endpoint" + fi + + # Check auth token source + if [ "$APP_TOKEN_GENERATED" == "true" ]; then + echo "Info: Using GitHub App Token for authentication." + elif [ -n "$PAT_TOKEN" ]; then + echo "Info: Using PAT_TOKEN for authentication." + else + echo "Warning: Neither App Token nor PAT_TOKEN is set, falling back to default GITHUB_TOKEN. This may have insufficient permissions." + fi + + if [ -z "$PAT_USERNAME" ]; then + echo "Warning: PAT_USERNAME is not set, will use openhands-agent" + fi + + - name: Set environment variables and outputs + id: set_vars # Add ID here + env: + REVIEW_BODY: ${{ github.event.review.body || '' }} + run: | + # All triggers for this workflow relate to a Pull Request + echo "PR_NUMBER=${{ github.event.pull_request.number || github.event.issue.number }}" >> $GITHUB_ENV + + if [ -n "$REVIEW_BODY" ]; then + echo "COMMENT_ID=${{ github.event.review.id || 'None' }}" >> $GITHUB_ENV + else + echo "COMMENT_ID=${{ github.event.comment.id || 'None' }}" >> $GITHUB_ENV + fi + + echo "MAX_ITERATIONS=${{ inputs.max_iterations || 50 }}" >> $GITHUB_ENV + # REVIEW_LEVEL and REVIEW_DEPTH are now passed directly to the script + echo "SANDBOX_ENV_GITHUB_TOKEN=${{ env.AUTH_TOKEN }}" >> $GITHUB_ENV + # Set SANDBOX_BASE_CONTAINER_IMAGE: Priority: inputs -> repo/org var -> empty string + if [ -n "${{ inputs.base_container_image }}" ]; then + echo "Using base_container_image from input: ${{ inputs.base_container_image }}" + FINAL_BASE_IMAGE="${{ inputs.base_container_image }}" + elif [ -n "${{ vars.SANDBOX_BASE_CONTAINER_IMAGE }}" ]; then + echo "Using SANDBOX_BASE_CONTAINER_IMAGE from repo/org vars." + FINAL_BASE_IMAGE="${{ vars.SANDBOX_BASE_CONTAINER_IMAGE }}" + else + echo "No base_container_image input or repo/org variable found. Defaulting to empty." + FINAL_BASE_IMAGE="" + fi + echo "determined_base_image=$FINAL_BASE_IMAGE" >> $GITHUB_OUTPUT + + - name: Comment on PR with start message + uses: actions/github-script@v7 + with: + github-token: ${{ env.AUTH_TOKEN }} + script: | + github.rest.issues.createComment({ + issue_number: ${{ env.PR_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 + id: install_openhands + uses: actions/github-script@v7 + env: + COMMENT_BODY: ${{ github.event.comment.body || '' }} + REVIEW_BODY: ${{ github.event.review.body || '' }} + LABEL_NAME: ${{ github.event.label.name || '' }} + EVENT_NAME: ${{ github.event_name }} + with: + script: | + const commentBody = process.env.COMMENT_BODY.trim(); + const reviewBody = process.env.REVIEW_BODY.trim(); + const labelName = process.env.LABEL_NAME.trim(); + const eventName = process.env.EVENT_NAME.trim(); + // Check conditions for experimental reviewer + const isExperimentalLabel = labelName === "review-me-experimental"; // Example experimental label + const isIssueCommentExperimental = + (eventName === "issue_comment" || eventName === "pull_request_review_comment") && + commentBody.includes("@openhands-reviewer-exp"); // Example experimental macro + const isReviewCommentExperimental = + eventName === "pull_request_review" && reviewBody.includes("@openhands-reviewer-exp"); // Example experimental macro + + // Set output variable + core.setOutput('isExperimental', isExperimentalLabel || isIssueCommentExperimental || isReviewCommentExperimental); + + // Perform package installation + if (isExperimentalLabel || isIssueCommentExperimental || isReviewCommentExperimental) { + console.log("Installing experimental OpenHands..."); + await exec.exec("python -m pip install --upgrade pip"); + await exec.exec("pip install git+https://github.com/all-hands-ai/openhands.git"); + } else { + console.log("Installing from requirements.txt..."); + await exec.exec("python -m pip install --upgrade pip"); + await exec.exec("pip install -r /tmp/requirements.txt"); + } + + - name: Attempt to review PR + env: + GITHUB_TOKEN: ${{ env.AUTH_TOKEN }} + GITHUB_USERNAME: ${{ secrets.PAT_USERNAME || 'openhands-agent' }} + GIT_USERNAME: ${{ secrets.PAT_USERNAME || 'openhands-agent' }} + LLM_MODEL: ${{ secrets.LLM_MODEL || inputs.LLM_MODEL }} + LLM_API_KEY: ${{ secrets.LLM_API_KEY }} + LLM_BASE_URL: ${{ secrets.LLM_BASE_URL }} + LLM_API_VERSION: ${{ inputs.LLM_API_VERSION }} + PYTHONPATH: "" + SANDBOX_BASE_CONTAINER_IMAGE: ${{ steps.set_vars.outputs.determined_base_image }} + run: | + echo "Using AUTH_TOKEN: $(echo $AUTH_TOKEN | cut -c 1-4)...$(echo $AUTH_TOKEN | rev | cut -c 1-4 | rev)" + # Run from the workspace directory where the repo is checked out + python -m openhands.code_reviewer.review_pr \ + --selected-repo ${{ github.repository }} \ + --pr-number ${{ env.PR_NUMBER }} \ + --max-iterations ${{ env.MAX_ITERATIONS }} \ + --comment-id ${{ env.COMMENT_ID }} \ + --output-file ./output/review_output_${{ env.PR_NUMBER }}.jsonl \ + --is-experimental ${{ steps.install_openhands.outputs.isExperimental }} \ + --review-level ${{ inputs.review_level || 'line' }} \ + --review-depth ${{ inputs.review_depth || 'deep' }} \ + --llm-num-retries 5 + + + + - name: Dump Docker Logs + if: always() # Run even if the previous step failed + run: | + echo "Attempting to dump logs from runtime container..." + CONTAINER_ID=$(docker ps -a --filter "name=openhands-runtime-" --format "{{.ID}}" | head -n 1) + if [ -n "$CONTAINER_ID" ]; then + echo "Found container ID: $CONTAINER_ID" + docker logs "$CONTAINER_ID" + else + echo "No container found matching 'openhands-runtime-*'." + fi + + - name: Check review result + id: check_result + run: | + if grep -q '"success":true' ./output/review_output_${{ env.PR_NUMBER }}.jsonl; then + echo "REVIEW_SUCCESS=true" >> $GITHUB_OUTPUT + else + echo "REVIEW_SUCCESS=false" >> $GITHUB_OUTPUT + fi + + - name: Upload review_output.jsonl as artifact + uses: actions/upload-artifact@v4 + if: always() # Upload even if the previous steps fail + with: + name: reviewer-output + path: ./output/review_output_${{ env.PR_NUMBER }}.jsonl + retention-days: 30 # Keep the artifact for 30 days + - name: Post Review Comments + if: always() # Post comments even if the review script failed (to report failure) + env: + GITHUB_TOKEN: ${{ env.AUTH_TOKEN }} + GITHUB_USERNAME: ${{ secrets.PAT_USERNAME || 'openhands-agent' }} + LLM_MODEL: ${{ secrets.LLM_MODEL || inputs.LLM_MODEL }} + LLM_API_KEY: ${{ secrets.LLM_API_KEY }} + LLM_BASE_URL: ${{ secrets.LLM_BASE_URL }} + LLM_API_VERSION: ${{ inputs.LLM_API_VERSION }} + PYTHONPATH: "" + REVIEW_SUCCESS: ${{ steps.check_result.outputs.REVIEW_SUCCESS }} + run: | + python -m openhands.code_reviewer.post_review_comments \ + --output-file ./output/review_output_${{ env.PR_NUMBER }}.jsonl \ + --selected-repo ${{ github.repository }} \ + --pr-number ${{ env.PR_NUMBER }} + + # The post_review_comments script handles success/failure reporting. diff --git a/.github/workflows/openhands-resolver.yml b/.github/workflows/openhands-resolver.yml index f2e63e9f995d..3aacda4afbd1 100644 --- a/.github/workflows/openhands-resolver.yml +++ b/.github/workflows/openhands-resolver.yml @@ -118,6 +118,7 @@ jobs: else echo "Using default GITHUB_TOKEN" echo "AUTH_TOKEN=${{ github.token }}" >> $GITHUB_ENV + fi - name: Log Auth Token Source run: | if [ -n "${{ steps.generate-token.outputs.token }}" ]; then @@ -234,7 +235,7 @@ jobs: echo "MAX_ITERATIONS=${{ inputs.max_iterations || 50 }}" >> $GITHUB_ENV echo "SANDBOX_ENV_GITHUB_TOKEN=${{ env.AUTH_TOKEN }}" >> $GITHUB_ENV - echo "SANDBOX_ENV_BASE_CONTAINER_IMAGE=${{ inputs.base_container_image }}" >> $GITHUB_ENV + echo "SANDBOX_BASE_CONTAINER_IMAGE=${{ inputs.base_container_image }}" >> $GITHUB_ENV # Set branch variables echo "TARGET_BRANCH=${{ inputs.target_branch || 'main' }}" >> $GITHUB_ENV diff --git a/dev_config/python/.pre-commit-config.yaml b/dev_config/python/.pre-commit-config.yaml index 0ae868fe70fd..d2c288b7b320 100644 --- a/dev_config/python/.pre-commit-config.yaml +++ b/dev_config/python/.pre-commit-config.yaml @@ -37,7 +37,7 @@ repos: hooks: - id: mypy additional_dependencies: - [types-requests, types-setuptools, types-pyyaml, types-toml] + [types-requests, types-setuptools, types-pyyaml, types-toml, types-aiofiles] entry: mypy --config-file dev_config/python/mypy.ini openhands/ always_run: true pass_filenames: false diff --git a/openhands/code_reviewer/__init__.py b/openhands/code_reviewer/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/openhands/code_reviewer/post_review_comments.py b/openhands/code_reviewer/post_review_comments.py new file mode 100644 index 000000000000..5708d56b3a6e --- /dev/null +++ b/openhands/code_reviewer/post_review_comments.py @@ -0,0 +1,155 @@ +import argparse +import asyncio +import json +import os + +from openhands.code_reviewer.reviewer_output import ReviewerOutput +from openhands.core.logger import openhands_logger as logger +from openhands.integrations.service_types import ProviderType +from openhands.resolver.interfaces.github import GithubPRHandler + + +def get_pr_handler( + owner: str, + repo: str, + token: str | None, + platform: ProviderType, +) -> GithubPRHandler: # Return specific type now + """Get the GitHub PR handler. Raises error for other platforms.""" + if platform != ProviderType.GITHUB: + raise ValueError( + f'Unsupported platform for code review comments: {platform}. Only GitHub is supported.' + ) + + gh_token = token or os.environ.get('GITHUB_TOKEN') + if not gh_token: + raise ValueError('GitHub token is required for GitHub PR handler') + + return GithubPRHandler(token=gh_token, owner=owner, repo=repo) + + +def post_comments( + output_file: str, + token: str | None, + selected_repo: str, + pr_number: int, +): + from openhands.code_reviewer.reviewer_output import ReviewComment + + """Reads review output and posts comments to the PR.""" + logger.info(f'Reading review output from: {output_file}') + try: + with open(output_file, 'r') as f: + # Read the entire file content + file_content = f.read() + if not file_content: + logger.error(f'Output file is empty: {output_file}') + return + output_data = json.loads(file_content) + # Manually construct ReviewComment objects + comments_data = output_data.pop( + 'comments', [] + ) # Get comments list, remove from dict + comments_objects = [ReviewComment(**c) for c in comments_data] + # Construct ReviewerOutput, passing the objects list + review_output = ReviewerOutput(**output_data, comments=comments_objects) + except FileNotFoundError: + logger.error(f'Output file not found: {output_file}') + return + except json.JSONDecodeError: + logger.error(f'Failed to decode JSON from output file: {output_file}') + return + except Exception as e: + logger.error(f'Error reading or parsing output file {output_file}: {e}') + return + + if not review_output.success: + logger.error(f'Review generation failed. Error: {review_output.error}') + # Optionally post a general failure comment? For now, just log. + return + + if not review_output.comments: + logger.warning('Review was successful, but no comments were generated.') + # Optionally post a comment indicating review completed with no findings? + return + + logger.info(f'Successfully parsed {len(review_output.comments)} comments.') + + try: + owner, repo = selected_repo.split('/') + except ValueError: + logger.error(f'Invalid repository format: {selected_repo}. Use owner/repo.') + return + + # Assume GitHub platform + platform = ProviderType.GITHUB + try: + pr_handler = get_pr_handler(owner, repo, token, platform) + except ValueError as e: # Catch specific error from get_pr_handler + logger.error(f'Configuration error getting PR handler: {e}') + return + + logger.info( + f'Posting {len(review_output.comments)} comments to PR #{pr_number} on {platform.value}...' + ) + + # Post comments using the handler + # The handler interface might need adjustment if post_review doesn't exist + # or takes different arguments. Assuming a method like post_review(pr_number, comments) + # Check if the handler has the post_review method + if not hasattr(pr_handler, 'post_review'): + logger.error(f'{type(pr_handler).__name__} does not have a post_review method.') + return + comments_to_post = review_output.comments + try: + asyncio.run( + pr_handler.post_review(pr_number=pr_number, comments=comments_to_post) + ) + except Exception: # Catch errors during comment posting + logger.exception( + f'Failed to post comments to PR #{pr_number}' + ) # Use logger.exception for stack trace + return # Exit if posting fails + + logger.info(f'Successfully posted comments to PR #{pr_number}.') + + +def main(): + parser = argparse.ArgumentParser(description='Post review comments to a PR.') + parser.add_argument( + '--output-file', + type=str, + required=True, + help='Path to the review_output.jsonl file.', + ) + parser.add_argument( + '--selected-repo', + type=str, + required=True, + help='Repository where the PR exists in the format `owner/repo`.', + ) + parser.add_argument( + '--pr-number', + type=int, + required=True, + help='Pull Request number to post comments to.', + ) + parser.add_argument( + '--token', + type=str, + default=None, + help='Platform token (GitHub PAT or GitLab access token). Reads from env vars (GITHUB_TOKEN/GITLAB_TOKEN) if not provided.', + ) + + args = parser.parse_args() + + post_comments( + output_file=args.output_file, + token=args.token, + selected_repo=args.selected_repo, + pr_number=args.pr_number, + ) + + +if __name__ == '__main__': + main() diff --git a/openhands/code_reviewer/prompts/review/basic-review.jinja b/openhands/code_reviewer/prompts/review/basic-review.jinja new file mode 100644 index 000000000000..a914c93f5f82 --- /dev/null +++ b/openhands/code_reviewer/prompts/review/basic-review.jinja @@ -0,0 +1,56 @@ +You are an AI code reviewer. Your task is to review the following pull request for the repository located in /workspace. +An environment with the repository checked out at the PR's head commit is available for you to analyze the code. + +# Pull Request Details +Title: {{ pr_data.title }} +{% if pr_data.body %} +Body: +{{ pr_data.body }} +{% endif %} + +# Review Task +First, ensure the latest changes are fetched using `git fetch origin`. Then, analyze the code changes between the base branch (`{{ pr_data['base']['ref'] }}`) and the head branch (`{{ pr_data['head']['ref'] }}`) using git commands (e.g., `git diff origin/{{ pr_data['base']['ref'] }}...origin/{{ pr_data['head']['ref'] }}`). Base your review on the following parameters: +- Review Level: `{{ review_level }}` (Specifies the granularity: 'line' for specific lines, 'file' for overall file changes, 'pr' for a high-level summary) +- Review Depth: `{{ review_depth }}` (Specifies the thoroughness: 'quick' for obvious issues, 'medium' for standard checks, 'deep' for in-depth analysis including potential bugs and security concerns) +**It is crucial that you strictly adhere to the specified Review Level and Review Depth.** + +When reviewing changes: +- Identify the file path from the `git diff` output (usually the line starting with `+++ b/`). This is the path in the head commit. +- Comments should only be placed on lines that exist in the head commit (lines starting with `+` or ` ` within the diff hunk). Do not comment on removed lines (starting with `-`). + - **Check Existing Comments:** Before adding a comment, consider if similar feedback has already been provided by other reviewers on this PR. Avoid adding duplicate comments. + - **Write Actionable Feedback:** Ensure your comments are constructive and actionable. Suggest specific improvements or changes rather than just explaining what the code does. +- **Line Number Determination and Verification:** Line numbers for comments MUST be determined **exclusively** by reading the file content from the `/workspace` directory (which is checked out to the head commit). Use `cat ` to get the content and `grep -n` (or manual counting in the `cat` output) to find the line number of the *exact* code you want to comment on. **Do NOT use the `git diff` output to determine or calculate line numbers.** This is unreliable. The line number reported by `grep -n` on the `/workspace` file is the **only** valid source for the `line` field in your comment. If the exact line text appears multiple times, use the surrounding code context (from the `cat` output) to identify the correct instance. The line numbers you provide MUST correspond to the line numbers in the head commit version. + + +{% if repo_instruction %} +# Repository Guidelines/Instructions +Please also consider the following repository-specific guidelines during your review: +{{ repo_instruction }} +{% endif %} + +# Output Format +Your final action **MUST** be the `finish` tool call. +- The `message` argument of this tool call **MUST** contain **ONLY** a single, raw JSON list containing review comment objects. It must NOT contain any other text, explanations, or markdown formatting. +- You can include any summary or explanation of your review process in the `thought` that accompanies the `finish` tool call. + +Each comment object in the JSON list should have the following structure: +- `path`: (string) The full path to the file being commented on, relative to the repository root (e.g., "openhands/core/config.py"). +- `comment`: (string) The text of your review comment. + - `line`: (integer) The line number in the *head commit version* of the file the comment refers to, determined and verified as described above. Required for all comments unless `review_level` is 'pr'. + - `code_snippet`: (string) The exact line(s) of code from the head commit version that the comment refers to. Include 1-3 lines for context if helpful. Required for all comments unless `review_level` is 'pr'. + - `line_number_justification`: (string) A brief explanation of how the `line` number was determined using file content verification (e.g., "Verified line 42 with `grep -n 'exact code line' /workspace/src/utils/parser.py`"). Required for all comments unless `review_level` is 'pr'. + +Example structure of the JSON list (this exact string goes into the `message` argument of the `finish` tool call): +`[{"path": "src/utils/parser.py", "line": 42, "code_snippet": "... code line(s) ...", "line_number_justification": "...", "comment": "..."}, {"path": "src/main.py", "comment": "..."}]` + +IMPORTANT: +- Focus your review on the changes between the base branch (`{{ pr_data['base']['ref'] }}`) and the head branch (`{{ pr_data['head']['ref'] }}`). +- Adhere strictly to the specified JSON output format for your final response. +- The `message` argument of your `finish` tool call **MUST** contain **ONLY** the raw JSON list string. No extra text, no markdown. +- Any explanatory text belongs in the accompanying `thought`, NOT the `message` argument. +- Do NOT attempt to modify any files. Your role is only to review. +- Do NOT ask for human help or clarification. Provide the review based on the information given. +- Use the `finish` tool call with the JSON review in the `message` argument as your **very last step** to signal completion. +- If no issues are found, the `message` argument should contain the exact string `[]`. + - You are running in a non-interactive environment. Do NOT ask questions or wait for user input. Proceed directly to the `finish` action with the JSON review when your analysis is complete. + - Do not use any other action other than `finish` to output the review. diff --git a/openhands/code_reviewer/review_pr.py b/openhands/code_reviewer/review_pr.py new file mode 100644 index 000000000000..d56604f6bc98 --- /dev/null +++ b/openhands/code_reviewer/review_pr.py @@ -0,0 +1,962 @@ +import argparse +import asyncio +import dataclasses # Added for serialization +import json +import os +import shutil +from typing import Any, Callable, Dict, List, cast # Add Callable, cast +from uuid import uuid4 + +import aiofiles # type: ignore[import-untyped] +from jinja2 import Template +from pydantic import BaseModel, SecretStr + +import openhands + +# from openhands.resolver.interfaces.issue_definitions import ServiceContextPR # Removed, not used +from openhands.code_reviewer.reviewer_output import ReviewComment, ReviewerOutput +from openhands.controller.state.state import State # Added Metrics +from openhands.core.config import AgentConfig, AppConfig, LLMConfig, SandboxConfig +from openhands.core.logger import openhands_logger as logger +from openhands.core.main import FakeUserResponseFunc, create_runtime, run_controller +from openhands.core.schema.agent import ( + AgentState, # Correct import +) +from openhands.events.action import ( + Action, # Import Action + AgentFinishAction, + CmdRunAction, + MessageAction, +) +from openhands.events.event import ( + Event, # Added for history typing +) +from openhands.events.observation import ( + CmdOutputObservation, + ErrorObservation, # Added for error checking + Observation, +) +from openhands.events.stream import EventStreamSubscriber +from openhands.integrations.service_types import ProviderType +from openhands.resolver.interfaces.github import ( + GithubPRHandler, # Removed GithubIssueHandler +) +from openhands.resolver.interfaces.gitlab import ( + GitlabPRHandler, # Removed GitlabIssueHandler +) +from openhands.resolver.interfaces.issue import ( + Issue, + IssueHandlerInterface, +) +from openhands.resolver.utils import ( + get_unique_uid, + identify_token, + reset_logger_for_multiprocessing, +) +from openhands.runtime.base import Runtime +from openhands.utils.async_utils import GENERAL_TIMEOUT, call_async_from_sync + + +def handle_awaiting_input( + current_state: State, # Change AgentState to State + encapsulate_solution: bool = False, # Add optional args + try_parse: Callable[[Action | None], str] | None = None, # Add optional args +) -> str: # Change return type to str + """Handles the AWAITING_USER_INPUT state by returning a message to finish.""" + logger.info('Agent entered AWAITING_USER_INPUT state. Returning FINISH message.') + # We instruct the agent to finish, as it should not be waiting for input. + return 'You should not be waiting for input. Please finalize your review and call the `finish` tool with the JSON list of comments as the `message` argument, as per the instructions.' + + +# Helper for JSON serialization +def default_serializer(obj): + if hasattr(obj, 'to_dict'): + return obj.to_dict() + if dataclasses.is_dataclass(obj): + return dataclasses.asdict(obj) + try: + if isinstance(obj, (str, int, float, bool, list, dict, type(None))): + return obj + return str(obj) + except TypeError: + return str(obj) + + +# Don't make this confgurable for now, unless we have other competitive agents +AGENT_CLASS = 'CodeActAgent' + + +def initialize_runtime( + runtime: Runtime, + platform: ProviderType, +) -> None: + """Initialize the runtime for the agent. + + This function is called before the runtime is used to run the agent. + Currently it does nothing. + """ + logger.info('-' * 30) + logger.info('BEGIN Runtime Completion Fn') + logger.info('-' * 30) + obs: Observation + + action = CmdRunAction(command='cd /workspace') + logger.info(action, extra={'msg_type': 'ACTION'}) + obs = runtime.run_action(action) + logger.info(obs, extra={'msg_type': 'OBSERVATION'}) + if not isinstance(obs, CmdOutputObservation) or obs.exit_code != 0: + raise RuntimeError(f'Failed to change directory to /workspace.\n{obs}') + + if platform == ProviderType.GITLAB and os.getenv('GITLAB_CI') == 'true': + action = CmdRunAction(command='sudo chown -R 1001:0 /workspace/*') + logger.info(action, extra={'msg_type': 'ACTION'}) + obs = runtime.run_action(action) + logger.info(obs, extra={'msg_type': 'OBSERVATION'}) + + action = CmdRunAction(command='git config --global core.pager ""') + logger.info(action, extra={'msg_type': 'ACTION'}) + obs = runtime.run_action(action) + logger.info(obs, extra={'msg_type': 'OBSERVATION'}) + if not isinstance(obs, CmdOutputObservation) or obs.exit_code != 0: + raise RuntimeError(f'Failed to set git config.\n{obs}') + + +async def process_review( + pr_data: dict[str, Any], # Changed from issue: Issue + platform: ProviderType, + # base_commit: str, # Removed, not used here + max_iterations: int, + llm_config: LLMConfig, + output_dir: str, + base_container_image: str | None, + runtime_container_image: str | None, + prompt_template: str, + repo_dir: str, + repo_instruction: str | None = None, + reset_logger: bool = False, + review_level: str = 'file', # Default reverted to file + review_depth: str = 'quick', # Default reverted to quick +) -> ReviewerOutput: + # Setup the logger properly, so you can run multi-processing to parallelize processing + if reset_logger: + log_dir = os.path.join(output_dir, 'infer_logs') + reset_logger_for_multiprocessing(logger, str(pr_data['number']), log_dir) + else: + logger.info(f"Starting review process for PR {pr_data['number']}.") + + # Define workspace relative to the current directory (GITHUB_WORKSPACE) + workspace_base = os.path.join( + '.', # Current directory + 'workspace', + f"pr_{pr_data['number']}", + ) + # Get the absolute path of the workspace base + workspace_base = os.path.abspath(workspace_base) + # write the repo to the workspace (assuming repo is already cloned to output_dir/repo) + if os.path.exists(workspace_base): + shutil.rmtree(workspace_base) + # Copy the checked-out repo (from repo_dir) to the workspace + shutil.copytree(repo_dir, workspace_base) + + sandbox_config = SandboxConfig( + base_container_image=base_container_image, + runtime_container_image=runtime_container_image, + enable_auto_lint=False, + use_host_network=False, + timeout=300, + ) + + if os.getenv('GITLAB_CI') == 'true': + sandbox_config.local_runtime_url = os.getenv( + 'LOCAL_RUNTIME_URL', 'http://localhost' + ) + user_id = os.getuid() if hasattr(os, 'getuid') else 1000 + if user_id == 0: + sandbox_config.user_id = get_unique_uid() + + config = AppConfig( + default_agent='CodeActAgent', + runtime='docker', + max_budget_per_task=4, + max_iterations=max_iterations, + sandbox=sandbox_config, + workspace_base=workspace_base, + workspace_mount_path=workspace_base, + agents={'CodeActAgent': AgentConfig(disabled_microagents=['github'])}, + ) + + config.set_llm_config(llm_config) + + # Prepare the initial prompt/instruction for code review + template = Template(prompt_template) + prompt_vars = { + 'pr_data': pr_data, # Pass the dictionary + 'repo_instruction': repo_instruction, + 'review_level': review_level, + 'review_depth': review_depth, + } + instruction = template.render(prompt_vars) + logger.info(f'Generated Instruction (first 200 chars): {instruction[:200]}...') + + images_urls: List[str] = [] # Type hint added + + # Initialize variables needed for results + runtime = None # Define runtime here to ensure it's available in finally + event_stream = None + state: State | None = None + comments: List[ReviewComment] = [] + success = False + error_message: str | None = None + final_agent_state: AgentState | None = None + agent_history: List[Event] = [] + agent_metrics: Dict[str, Any] | None = None # Added from resolve_issue + + def on_event(evt: Event) -> None: + if isinstance(evt, CmdOutputObservation): + # Log command output observations with truncated content + MAX_LEN = 200 + content_preview = evt.content[:MAX_LEN] + if len(evt.content) > MAX_LEN: + content_preview += '... [truncated]' + logger.info( + f"CmdOutputObservation(command={evt.command}, exit_code={evt.exit_code}, content='{content_preview}')" + ) + else: + # Log other events normally (might still truncate based on default logger settings) + logger.info(evt) + + # 1. Create and connect runtime + logger.info('Creating and connecting runtime...') + runtime = create_runtime(config) + await runtime.connect() + logger.info('Runtime connected.') + event_stream = runtime.event_stream + if event_stream: + event_stream.subscribe(EventStreamSubscriber.MAIN, on_event, str(uuid4())) + else: + logger.warning( + 'Runtime does not have an event_stream attribute, cannot subscribe.' + ) + + # 3. Initialize runtime (e.g., git config) + logger.info('Initializing runtime...') + initialize_runtime(runtime, platform) + logger.info('Runtime initialized.') + # 4. Create initial action and run the agent controller + action = MessageAction(content=instruction, image_urls=images_urls) + logger.info(f'Starting agent loop with initial action: {action}') + + try: + state = await run_controller( + config=config, + initial_user_action=action, + runtime=runtime, + fake_user_response_fn=cast(FakeUserResponseFunc, handle_awaiting_input), + ) + if state is None: + error_message = 'Agent controller did not return a final state.' + logger.error(error_message) + final_agent_state = AgentState.ERROR # Treat as error + else: + final_agent_state = state.agent_state + agent_history = state.history # Store history + agent_metrics = ( + state.metrics.get() if state.metrics else None + ) # Store metrics + logger.info(f'Final agent state: {final_agent_state}') + + # Check for errors first + if final_agent_state == AgentState.ERROR: + error_message = 'Agent finished in ERROR state.' + # Try to find a more specific error in history + if agent_history: + for event in reversed(agent_history): + if isinstance(event, ErrorObservation): + error_message = f'Agent error: {event.content}' + break + logger.error(error_message) + elif final_agent_state != AgentState.FINISHED: + error_message = ( + f'Agent finished in unexpected state: {final_agent_state}' + ) + logger.warning( + error_message + ) # Log as warning, maybe comments were still generated + + # Attempt to extract comments by searching backwards through history + # NEW LOGIC: Attempt to extract comments from the AgentFinishAction message + parse_error: str | None = None + found_review_in_finish = False + + if agent_history: + last_event = agent_history[-1] + if isinstance(last_event, AgentFinishAction): + logger.info( + f'Agent finished. Attempting to parse review from final_thought: {last_event.final_thought[:200]}...' + ) + logger.debug( + f'Full final_thought content: >>>{last_event.final_thought}<<<' + ) # DEBUG + try: + # Attempt to parse the final_thought directly as JSON + parsed_content = json.loads(last_event.final_thought.strip()) + if isinstance(parsed_content, list): + # Found a list, try to validate it + validated_comments = [] + for c_dict in parsed_content: + # === Start: Reused Validation Logic === + if isinstance(c_dict, dict) and 'comment' in c_dict: + path = c_dict.get('path') + line = c_dict.get('line') + comment_text = c_dict['comment'] + valid_comment = True + + if path is not None and not isinstance(path, str): + logger.warning( + f'Skipping comment with invalid path type: {c_dict}' + ) + valid_comment = False + if line is not None and not isinstance(line, int): + if isinstance(line, str) and line.isdigit(): + line = int(line) + else: + logger.warning( + f'Skipping comment with invalid line type: {c_dict}' + ) + valid_comment = False + if not isinstance(comment_text, str): + logger.warning( + f'Skipping comment with invalid comment text type: {c_dict}' + ) + valid_comment = False + + if valid_comment: + validated_comments.append( + ReviewComment( + path=path, + comment=comment_text, + line=line, + ) + ) + else: + logger.warning( + f'Skipping invalid comment structure: {c_dict}' + ) + # === End: Reused Validation Logic === + + if validated_comments: + comments = validated_comments + found_review_in_finish = True + logger.info( + f'Extracted {len(comments)} review comments from AgentFinishAction final_thought.' + ) + else: + # It was a list, but contained no valid comments + parse_error = 'Agent finish message was a list but contained no valid comment objects.' + logger.warning( + f'{parse_error} Final thought snippet: {last_event.final_thought[:200]}' + ) + + else: + # Content was valid JSON, but not a list + parse_error = ( + 'Agent finish message content was not a JSON list.' + ) + logger.warning( + f'{parse_error} Final thought snippet: {last_event.final_thought[:200]}' + ) + + except json.JSONDecodeError as e: + parse_error = ( + f'Failed to parse agent finish message as JSON: {e}' + ) + logger.warning( + f'{parse_error} Final thought snippet: {last_event.final_thought[:200]}' + ) + except Exception as e: + parse_error = f'Error processing agent finish message: {e}' + logger.warning( + f'{parse_error} Final thought snippet: {last_event.final_thought[:200]}' + ) + else: + # Last event was not AgentFinishAction + error_message = f'Agent did not end with AgentFinishAction. Last event: {type(last_event).__name__}' + logger.error(error_message) + else: + # No history + error_message = 'Agent produced no history.' + logger.error(error_message) + + # Determine final success/error state + if found_review_in_finish and final_agent_state == AgentState.FINISHED: + success = True + error_message = None # Clear any previous agent loop error + logger.info('Review successfully extracted from AgentFinishAction.') + elif final_agent_state == AgentState.ERROR: + success = False + # Keep the original error_message from the agent loop if it exists + if not error_message: + error_message = 'Agent finished in ERROR state.' + logger.error(f'Agent finished in ERROR state: {error_message}') + else: + # Covers cases: No history, last event not Finish, Finish message invalid/empty, agent finished unexpectedly + success = False + if ( + not error_message + ): # Only set if no specific error was already logged + if parse_error: + error_message = f'Failed to extract review from finish message: {parse_error}' + elif final_agent_state != AgentState.FINISHED: + error_message = f'Agent finished in unexpected state ({final_agent_state}) and no valid review found.' + else: # Should imply finish state but parsing failed or last event wasn't finish + error_message = ( + 'Agent finished but review could not be extracted.' + ) + logger.error(f'Review processing failed: {error_message}') + + except Exception: + # Catch any other unexpected errors during processing + logger.exception('An unexpected exception occurred during agent execution:') + success = False + final_agent_state = AgentState.ERROR # Assume error state + + finally: + # Ensure runtime is closed if it was created + if runtime: + runtime.close() # Sync close + + # Construct the final output + output = ReviewerOutput( + pr_info=pr_data, + review_level=review_level, + review_depth=review_depth, + instruction=instruction, + history=[ + evt.to_dict() if hasattr(evt, 'to_dict') else dataclasses.asdict(evt) + for evt in agent_history + ], # Serialize history + comments=comments, + metrics=agent_metrics, # Pass metrics + success=success, + error=error_message, + final_agent_state=final_agent_state, + ) + + return output + + +# Helper function for JSON serialization +def json_default(obj): + if isinstance(obj, BaseModel): # Handle Pydantic models (including Issue) + return obj.model_dump() + if dataclasses.is_dataclass(obj): + # Handle other dataclasses + return dataclasses.asdict(obj) + if isinstance(obj, SecretStr): + return obj.get_secret_value() # Convert SecretStr to str + # For other types, try converting to string as a fallback + try: + return str(obj) + except Exception: + raise TypeError( + f'Object of type {obj.__class__.__name__} is not JSON serializable' + ) + + +def write_output_to_file(output_file: str, output_data: ReviewerOutput): + """Writes the ReviewerOutput data to the specified JSONL file.""" + try: + # Ensure the directory exists + os.makedirs(os.path.dirname(output_file), exist_ok=True) + with open(output_file, 'w') as f: + json.dump( + dataclasses.asdict(output_data), f, indent=2, default=json_default + ) + logger.info(f'Successfully wrote output to {output_file}') + except Exception as e: + logger.error(f'Failed to write output to {output_file}: {e}') + # Fallback: print to stdout if writing fails + print( + json.dumps(dataclasses.asdict(output_data), indent=2, default=json_default) + ) + + +async def run_review_task( + pr_url: str, + review_level: str, + review_depth: str, + token: str, + username: str, + max_iterations: int, + output_dir: str, # Keep output_dir for potential future use, though not used for printing + output_file: str, + llm_config: LLMConfig, + base_container_image: str | None, + runtime_container_image: str | None, + prompt_file: str | None, + repo_instruction_file: str | None, + base_domain: str | None, +) -> None: + """Orchestrates the code review process for a given PR URL.""" + logger.info(f'Starting review task for PR: {pr_url}') + + # 1. Identify platform and parse URL + platform = await identify_token(token, base_domain) + logger.info(f'Identified platform: {platform.value}') + handler_class: type[IssueHandlerInterface] + if platform == ProviderType.GITHUB: + handler_class = GithubPRHandler + elif platform == ProviderType.GITLAB: + handler_class = GitlabPRHandler + else: + raise ValueError(f'Unsupported platform: {platform}') + + assert hasattr( + handler_class, 'parse_pr_url' + ), f'{handler_class.__name__} lacks parse_pr_url' + owner, repo, issue_number = handler_class.parse_pr_url(pr_url) + logger.info(f'Parsed PR URL: owner={owner}, repo={repo}, number={issue_number}') + + # 2. Create Issue Handler + # Set default base_domain if None + if base_domain is None: + base_domain = 'github.com' if platform == ProviderType.GITHUB else 'gitlab.com' + issue_handler: GithubPRHandler = handler_class( # type: ignore[call-arg, assignment] + owner=owner, + repo=repo, + token=token, + username=username, + base_domain=base_domain, # Now guaranteed to be str + ) + logger.info(f'Created issue handler: {type(issue_handler).__name__}') + + # 3. Fetch PR Info (Issue object) + assert hasattr( + issue_handler, 'get_converted_issues' + ), f'{type(issue_handler).__name__} lacks get_converted_issues' + + try: + # Fetch full PR details as a dictionary + pr_data = await issue_handler.get_pr_details(issue_number) + logger.info(f'Fetched PR data for #{pr_data["number"]}') + logger.info(f'Type of pr_data: {type(pr_data)}') + logger.info( + f'Content of pr_data (keys): {list(pr_data.keys())}' + ) # Log keys for brevity + except Exception as e: + logger.error(f'Failed to fetch PR info: {e}') + # Print error output similar to main's exception handling + error_output = ReviewerOutput( + pr_info=Issue(number=issue_number, url=pr_url), # Basic info + review_level=review_level, + review_depth=review_depth, + instruction='', + history=[], + success=False, + error=f'Failed to fetch PR info: {e}', + final_agent_state=AgentState.ERROR, + ) + write_output_to_file(output_file, error_output) + return # Exit early + + # 4. Setup repository directory + repo_dir = os.path.join(output_dir, 'repo') # Use output_dir for repo checkout + os.makedirs(repo_dir, exist_ok=True) + logger.info(f'Repository directory set to: {repo_dir}') + + # 5. Checkout PR branch + try: + assert hasattr( + issue_handler, 'checkout_pr' + ), f'{type(issue_handler).__name__} lacks checkout_pr' + await issue_handler.checkout_pr(pr_data['number'], repo_dir) + logger.info(f"Checked out PR branch for #{pr_data['number']} into {repo_dir}") + # base_commit = await issue_handler.get_head_commit(repo_dir) # Not needed by process_review + # logger.info(f'Base commit set to: {base_commit}') + except Exception as e: + logger.error(f'Failed to checkout PR branch: {e}') + error_output = ReviewerOutput( + pr_info=pr_data, + review_level=review_level, + review_depth=review_depth, + instruction='', + history=[], + success=False, + error=f'Failed to checkout PR branch: {e}', + final_agent_state=AgentState.ERROR, + ) + print(json.dumps(error_output, indent=2, default=json_default)) + return # Exit early + + # 6. Read repository instructions if provided + repo_instruction: str | None = None + if repo_instruction_file: + try: + async with aiofiles.open(repo_instruction_file, mode='r') as f: + repo_instruction = await f.read() + logger.info(f'Read repository instructions from: {repo_instruction_file}') + except Exception as e: + logger.warning( + f'Could not read repository instruction file {repo_instruction_file}: {e}' + ) + # Continue without repo instructions if file reading fails + + # 7. Read prompt template + if prompt_file is None: + # Use default prompt if none provided + prompt_file = os.path.join( + os.path.dirname(__file__), 'prompts/review/basic-review.jinja' + ) + logger.info(f'Using default prompt template: {prompt_file}') + + try: + async with aiofiles.open(prompt_file, mode='r') as f: + prompt_template = await f.read() + logger.info(f'Read prompt template from: {prompt_file}') + except Exception as e: + logger.error(f'Failed to read prompt template file {prompt_file}: {e}') + error_output = ReviewerOutput( + pr_info=pr_data, + review_level=review_level, + review_depth=review_depth, + instruction='', + history=[], + success=False, + error=f'Failed to read prompt template: {e}', + final_agent_state=AgentState.ERROR, + ) + write_output_to_file(output_file, error_output) + return # Exit early + + # 9. Process the PR using the core logic function + try: + logger.info(f'Passing to process_review - Type of pr_data: {type(pr_data)}') + logger.info( + f'Passing to process_review - Content of pr_data (keys): {list(pr_data.keys())}' + ) + output = await process_review( + pr_data=pr_data, # Pass the dictionary + platform=platform, + max_iterations=max_iterations, + llm_config=llm_config, + output_dir=output_dir, # Pass output_dir for workspace creation inside process_review + base_container_image=base_container_image, + runtime_container_image=runtime_container_image, + prompt_template=prompt_template, + repo_dir=repo_dir, # Pass the checkout location + repo_instruction=repo_instruction, + reset_logger=False, # Assuming single process, no need to reset logger + review_level=review_level, + review_depth=review_depth, + ) + + # Write the final output to file + write_output_to_file(output_file, output) + if output.success: + logger.info('Review task completed successfully.') + else: + logger.warning( + f'Review task finished with success=False. Final agent state: {output.final_agent_state}. Error: {output.error}' + ) + + except Exception as e: + logger.error(f'An unexpected error occurred during review processing: {e}') + # Create a generic error output if processing fails unexpectedly + error_output = ReviewerOutput( + pr_info=pr_data, + review_level=review_level, + review_depth=review_depth, + instruction='', # May not have been generated + history=[], + success=False, + error=f'Review processing failed: {e}', + final_agent_state=AgentState.ERROR, + ) + write_output_to_file(output_file, error_output) + + +def main() -> None: + def int_or_none(value: str) -> int | None: + if value.lower() == 'none': + return None + else: + return int(value) + + parser = argparse.ArgumentParser(description='Review a single pull request.') + parser.add_argument( + '--selected-repo', + type=str, + required=True, + help='repository to review PRs in form of `owner/repo`.', + ) + parser.add_argument( + '--output-file', + type=str, + required=True, + help='Path to the output JSONL file.', + ) + parser.add_argument( + '--token', + type=str, + default=None, + help='token to access the repository.', + ) + parser.add_argument( + '--username', + type=str, + default=None, + help='username to access the repository.', + ) + parser.add_argument( + '--llm-temperature', + type=float, + default=1.0, # Default to 1.0 as before + help='Temperature for the LLM', + ) + parser.add_argument( + '--base-container-image', + type=str, + default=None, + help='base container image to use.', + ) + parser.add_argument( + '--runtime-container-image', + type=str, + default=None, + help='Container image to use.', + ) + parser.add_argument( + '--max-iterations', + type=int, + default=10, # Reduced default iterations for review? + help='Maximum number of iterations to run.', + ) + parser.add_argument( + '--pr-number', # Renamed from --issue-number + type=int, + required=True, + help='Pull Request number to review.', + ) + parser.add_argument( + '--comment-id', + type=int_or_none, + required=False, + default=None, + help='Review a specific comment thread within the PR', + ) + parser.add_argument( + '--output-dir', + type=str, + default='output', + help='Output directory to write the results.', + ) + parser.add_argument( + '--llm-model', + type=str, + default=None, + help='LLM model to use.', + ) + parser.add_argument( + '--llm-api-key', + type=str, + default=None, + help='LLM API key to use.', + ) + parser.add_argument( + '--llm-base-url', + type=str, + default=None, + help='LLM base URL to use.', + ) + parser.add_argument( + '--llm-num-retries', + type=int, + default=3, # Default number of retries + help='Number of retries for LLM API calls.', + ) + parser.add_argument( + '--prompt-file', + type=str, + default=None, + help='Path to the prompt template file in Jinja format.', + ) + parser.add_argument( + '--repo-instruction-file', + type=str, + default=None, + help='Path to the repository instruction/guideline file in text format.', + ) + parser.add_argument( + '--review-level', # Added + type=str, + default='file', + choices=['line', 'file', 'pr'], + help='Level of detail for the review (line, file, or overall PR).', + ) + parser.add_argument( + '--review-depth', # Added + type=str, + default='quick', + choices=['quick', 'medium', 'deep'], + help='Depth/thoroughness of the review (quick, medium, or deep).', + ) + parser.add_argument( + '--is-experimental', + type=lambda x: x.lower() == 'true', + default=False, + help='Whether to run in experimental mode.', + ) + parser.add_argument( + '--base-domain', + type=str, + default=None, + help='Base domain for the git server (defaults to "github.com" for GitHub and "gitlab.com" for GitLab)', + ) + + my_args = parser.parse_args() + + review_level = my_args.review_level # noqa: F841 + review_depth = my_args.review_depth # noqa: F841 + output_dir = my_args.output_dir # noqa: F841 + # Initialize container image variables + base_container_image: str | None = None + runtime_container_image: str | None = None + # Get container image from environment variable first + env_base_image_as_runtime = os.getenv( + 'SANDBOX_BASE_CONTAINER_IMAGE' + ) # Check for base image env var to use as runtime + + if env_base_image_as_runtime: + logger.info( + f'Using SANDBOX_BASE_CONTAINER_IMAGE as runtime image: {env_base_image_as_runtime}' + ) + runtime_container_image = env_base_image_as_runtime + base_container_image = ( + None # Ensure base_container_image is None if env var is used + ) + else: + # Fallback to command-line arguments if env var is not set + logger.info( + 'SANDBOX_BASE_CONTAINER_IMAGE not set, checking command-line arguments for runtime/base images.' + ) + arg_base_image = my_args.base_container_image + arg_runtime_image = my_args.runtime_container_image + + if arg_runtime_image is not None and arg_base_image is not None: + raise ValueError( + 'Cannot provide both --runtime-container-image and --base-container-image via arguments when SANDBOX_BASE_CONTAINER_IMAGE is not set.' + ) + + # Determine the final image configuration based on args + if arg_base_image is not None: + logger.info(f'Using base container image from args: {arg_base_image}') + base_container_image = arg_base_image + # runtime_container_image remains None + elif arg_runtime_image is not None: + logger.info(f'Using runtime container image from args: {arg_runtime_image}') + runtime_container_image = arg_runtime_image + # base_container_image remains None + elif not my_args.is_experimental: + # Neither arg provided, not experimental: use default runtime image + runtime_container_image = ( + f'ghcr.io/all-hands-ai/runtime:{openhands.__version__}-nikolaik' + ) + logger.info( + f'Defaulting runtime container image to: {runtime_container_image}' + ) + # base_container_image remains None + else: + # Neither arg provided, IS experimental: leave both as None + logger.info( + 'No container image specified via args or env, and is_experimental=True. Both images remain None.' + ) + # Both base_container_image and runtime_container_image remain None + + parts = my_args.selected_repo.rsplit('/', 1) + if len(parts) < 2: + raise ValueError('Invalid repository format. Expected owner/repo') + owner, repo = parts + + token_str = my_args.token or os.getenv('GITHUB_TOKEN') or os.getenv('GITLAB_TOKEN') + username = my_args.username if my_args.username else os.getenv('GIT_USERNAME') + if not username: + raise ValueError('Username is required.') + + if not token_str: + raise ValueError('Token is required.') + + token = token_str + + platform = call_async_from_sync( + identify_token, + GENERAL_TIMEOUT, + token, + my_args.base_domain, + ) + + api_key = my_args.llm_api_key or os.environ['LLM_API_KEY'] + model = my_args.llm_model or os.environ['LLM_MODEL'] + base_url = my_args.llm_base_url or os.environ.get('LLM_BASE_URL', None) + api_version = os.environ.get('LLM_API_VERSION', None) + + # Create LLMConfig instance + llm_config = LLMConfig( + model=model, + api_key=SecretStr(api_key) if api_key else None, + base_url=base_url, + num_retries=my_args.llm_num_retries, # Use the argument here + temperature=my_args.llm_temperature, # Use the argument here + ) + + # Only set api_version if it was explicitly provided, otherwise let LLMConfig handle it + if api_version is not None: + llm_config.api_version = api_version + + # Set default prompt file if not provided + prompt_file = my_args.prompt_file + if prompt_file is None: + # Use a default review prompt (adjust path as needed) + prompt_file = os.path.join( + os.path.dirname(__file__), 'prompts/review/basic-review.jinja' + ) + logger.info(f'Prompt file not specified, using default: {prompt_file}') + + # Construct pr_url + base_domain_val = my_args.base_domain + if base_domain_val is None: + base_domain_val = ( + 'github.com' if platform == ProviderType.GITHUB else 'gitlab.com' + ) + # Adjust URL format based on platform + pr_number = my_args.pr_number # Need pr_number here + if platform == ProviderType.GITLAB: + pr_url = ( + f'https://{base_domain_val}/{owner}/{repo}/-/merge_requests/{pr_number}' + ) + else: # Default to GitHub format + pr_url = f'https://{base_domain_val}/{owner}/{repo}/pull/{pr_number}' + logger.info(f'Constructed PR URL: {pr_url}') + + repo_instruction_file = my_args.repo_instruction_file # Define file path variable + asyncio.run( + run_review_task( + pr_url=pr_url, + review_level=my_args.review_level, + review_depth=my_args.review_depth, + token=token, + username=username, + max_iterations=my_args.max_iterations, + output_dir=my_args.output_dir, + output_file=my_args.output_file, + llm_config=llm_config, + base_container_image=base_container_image, + runtime_container_image=runtime_container_image, + prompt_file=prompt_file, # Pass file path + repo_instruction_file=repo_instruction_file, # Pass file path + base_domain=my_args.base_domain, # Pass original arg + ) + ) + + +if __name__ == '__main__': + main() diff --git a/openhands/code_reviewer/reviewer_output.py b/openhands/code_reviewer/reviewer_output.py new file mode 100644 index 000000000000..49c1c6e9e0f1 --- /dev/null +++ b/openhands/code_reviewer/reviewer_output.py @@ -0,0 +1,29 @@ +import dataclasses +from typing import Any, Dict, List, Optional + + +@dataclasses.dataclass +class ReviewComment: + path: Optional[str] = None # File path relative to repo root + line: Optional[int] = None # Line number for line-specific comments + comment: str = '' # The review comment text + + +@dataclasses.dataclass +class ReviewerOutput: + pr_info: Dict[ + str, Any + ] # Using dict to store PR info (number, title, owner, repo, etc.) + review_level: str # e.g., 'line', 'file', 'pr' + review_depth: str # e.g., 'quick', 'deep' + instruction: str # The instruction given to the agent + history: List[dict[str, Any]] # Agent history (actions/observations) + comments: List[ReviewComment] = dataclasses.field( + default_factory=list + ) # List of review comments + metrics: Optional[dict[str, Any]] = None # Agent metrics + final_agent_state: Optional[str] = ( + None # Final state of the agent (e.g., FINISHED, ERROR) + ) + success: bool = False # Whether the review process completed successfully + error: Optional[str] = None # Error message if success is False diff --git a/openhands/integrations/utils.py b/openhands/integrations/utils.py index 28df830a973d..26a862340b53 100644 --- a/openhands/integrations/utils.py +++ b/openhands/integrations/utils.py @@ -1,7 +1,5 @@ from pydantic import SecretStr -from openhands.integrations.github.github_service import GitHubService -from openhands.integrations.gitlab.gitlab_service import GitLabService from openhands.integrations.provider import ProviderType @@ -20,20 +18,5 @@ async def validate_provider_token( 'gitlab' if it's a GitLab token None if the token is invalid for both services """ - # Try GitHub first - try: - github_service = GitHubService(token=token, base_domain=base_domain) - # Validation deferred to actual usage - return ProviderType.GITHUB - except Exception: - pass - - # Try GitLab next - try: - gitlab_service = GitLabService(token=token, base_domain=base_domain) - # Validation deferred to actual usage - return ProviderType.GITLAB - except Exception: - pass - - return None + # Skip validation and assume GitHub + return ProviderType.GITHUB diff --git a/openhands/llm/retry_mixin.py b/openhands/llm/retry_mixin.py index 367bcbd97d2f..ca8a521cf5a6 100644 --- a/openhands/llm/retry_mixin.py +++ b/openhands/llm/retry_mixin.py @@ -45,9 +45,9 @@ def before_sleep(retry_state: Any) -> None: # Only change temperature if it's zero or not set current_temp = retry_state.kwargs.get('temperature', 0) if current_temp == 0: - retry_state.kwargs['temperature'] = 1.0 + retry_state.kwargs['temperature'] = 2.0 logger.warning( - 'LLMNoResponseError detected with temperature=0, setting temperature to 1.0 for next attempt.' + 'LLMNoResponseError detected with temperature=0, setting temperature to 2.0 for next attempt.' ) else: logger.warning( diff --git a/openhands/resolver/interfaces/github.py b/openhands/resolver/interfaces/github.py index c21364802963..f5ed1b346a11 100644 --- a/openhands/resolver/interfaces/github.py +++ b/openhands/resolver/interfaces/github.py @@ -1,8 +1,16 @@ +import asyncio +import logging +import os +import shutil from typing import Any +from urllib.parse import urlparse import httpx +from pydantic import SecretStr -from openhands.core.logger import openhands_logger as logger +from openhands.code_reviewer.reviewer_output import ( + ReviewComment, # Added for type hinting in post_review +) from openhands.resolver.interfaces.issue import ( Issue, IssueHandlerInterface, @@ -10,8 +18,12 @@ ) from openhands.resolver.utils import extract_issue_references +logger = logging.getLogger(__name__) + class GithubIssueHandler(IssueHandlerInterface): + token: SecretStr + def __init__( self, owner: str, @@ -32,6 +44,7 @@ def __init__( self.owner = owner self.repo = repo self.token = token + self.username = username self.base_domain = base_domain self.base_url = self.get_base_url() @@ -44,8 +57,9 @@ def set_owner(self, owner: str) -> None: def get_headers(self) -> dict[str, str]: return { - 'Authorization': f'token {self.token}', + 'Authorization': f'token {self.token}', # Use self.token directly 'Accept': 'application/vnd.github.v3+json', + 'X-GitHub-Api-Version': '2022-11-28', } def get_base_url(self) -> str: @@ -313,7 +327,7 @@ def __init__( self, owner: str, repo: str, - token: str, + token: str, # Expect str here username: str | None = None, base_domain: str = 'github.com', ): @@ -322,11 +336,14 @@ def __init__( Args: owner: The owner of the repository repo: The name of the repository - token: The GitHub personal access token + token: The GitHub personal access token (as str) username: Optional GitHub username base_domain: The domain for GitHub Enterprise (default: "github.com") """ + # Pass the token (str) directly to the superclass __init__ super().__init__(owner, repo, token, username, base_domain) + + # Update download_url based on potentially shadowed attributes if self.base_domain == 'github.com': self.download_url = ( f'https://api.github.com/repos/{self.owner}/{self.repo}/pulls' @@ -334,6 +351,90 @@ def __init__( else: self.download_url = f'https://{self.base_domain}/api/v3/repos/{self.owner}/{self.repo}/pulls' + @staticmethod + def parse_pr_url(pr_url: str) -> tuple[str, str, int]: + """Parse a GitHub PR URL to extract owner, repo, and PR number.""" + parsed_url = urlparse(pr_url) + path_parts = parsed_url.path.strip('/').split('/') + if len(path_parts) < 4 or path_parts[2] != 'pull': + raise ValueError(f'Invalid GitHub PR URL format: {pr_url}') + owner = path_parts[0] + repo = path_parts[1] + try: + pr_number = int(path_parts[3]) + except ValueError: + raise ValueError(f'Invalid PR number in URL: {pr_url}') + return owner, repo, pr_number + + async def _run_git_command(self, command: list[str], cwd: str) -> tuple[str, str]: + """Run a git command asynchronously and return stdout and stderr.""" + process = await asyncio.create_subprocess_exec( + *command, + stdout=asyncio.subprocess.PIPE, + stderr=asyncio.subprocess.PIPE, + cwd=cwd, + ) + stdout, stderr = await process.communicate() + if process.returncode != 0: + raise RuntimeError( + f'Git command failed: {" ".join(command)}\nStderr: {stderr.decode()}' + ) + return stdout.decode(), stderr.decode() + + async def checkout_pr(self, pr_number: int, repo_dir: str): + """Checkout the specific PR branch into the specified directory.""" + logger.info(f'Checking out PR #{pr_number} to {repo_dir}') + + # Ensure repo_dir exists and is empty or remove it + if os.path.exists(repo_dir): + if os.listdir(repo_dir): + logger.warning(f'Directory {repo_dir} is not empty. Removing it.') + shutil.rmtree(repo_dir) + os.makedirs(repo_dir) + else: + os.makedirs(repo_dir) + + # Fetch PR details from GitHub API + pr_api_url = f'{self.base_url}/pulls/{pr_number}' + async with httpx.AsyncClient() as client: + response = await client.get(pr_api_url, headers=self.headers) + response.raise_for_status() + pr_data = response.json() + + head_sha = pr_data['head']['sha'] + clone_url = self.get_clone_url() + pr_ref = f'pull/{pr_number}/head' + + logger.info(f'Cloning {self.owner}/{self.repo} into {repo_dir}') + await self._run_git_command(['git', 'clone', clone_url, '.'], cwd=repo_dir) + + logger.info(f'Fetching PR ref: {pr_ref}') + await self._run_git_command( + ['git', 'fetch', 'origin', f'{pr_ref}:{pr_ref}'], cwd=repo_dir + ) + + logger.info(f'Checking out PR ref: {pr_ref}') + await self._run_git_command(['git', 'checkout', pr_ref], cwd=repo_dir) + + logger.info(f'Resetting to head SHA: {head_sha}') + await self._run_git_command(['git', 'reset', '--hard', head_sha], cwd=repo_dir) + + logger.info(f'Successfully checked out PR #{pr_number} at commit {head_sha}') + + async def get_pr_details(self, pr_number: int) -> dict[str, Any]: + """Fetch full details for a specific Pull Request using the REST API.""" + pr_api_url = f'{self.base_url}/pulls/{pr_number}' + logger.info(f'Fetching PR details from: {pr_api_url}') + async with httpx.AsyncClient() as client: + response = await client.get(pr_api_url, headers=self.headers) + response.raise_for_status() # Raise an exception for bad status codes + pr_data = response.json() + logger.info(f'Successfully fetched details for PR #{pr_number}') + # Add owner and repo explicitly, as they might not be in the direct response + pr_data['owner'] = self.owner + pr_data['repo'] = self.repo + return pr_data + def download_pr_metadata( self, pull_number: int, comment_id: int | None = None ) -> tuple[list[str], list[int], list[str], list[ReviewThread], list[str]]: @@ -481,6 +582,217 @@ def download_pr_metadata( thread_ids, ) + async def get_pr_diff(self, pr_number: int) -> str: + """Get the diff content for a GitHub pull request.""" + url = f'{self.base_url}/pulls/{pr_number}' + headers = self.get_headers() + # Use the specific Accept header for diff + headers['Accept'] = 'application/vnd.github.v3.diff' + + async with httpx.AsyncClient() as client: + try: + response = await client.get(url, headers=headers) + response.raise_for_status() # Raise exception for 4xx/5xx status codes + logger.info(f'Successfully fetched diff for GitHub PR #{pr_number}') + return response.text + except httpx.HTTPStatusError as e: + logger.error( + f'HTTP error fetching diff for PR #{pr_number}: {e.response.status_code} - {e.response.text}' + ) + raise # Re-raise the exception after logging + except Exception as e: + logger.error(f'Error fetching diff for PR #{pr_number}: {e}') + raise # Re-raise other exceptions + + async def get_pr_head_commit(self, pr_number: int) -> str: + """Get the SHA of the head commit of a GitHub pull request.""" + pr_url = f'{self.base_url}/pulls/{pr_number}' + headers = self.get_headers() + async with httpx.AsyncClient() as client: + try: + response = await client.get(pr_url, headers=headers) + response.raise_for_status() + pr_data = response.json() + head_commit_sha = pr_data.get('head', {}).get('sha') + if not head_commit_sha: + raise ValueError( + f'Could not extract head commit SHA from PR data for PR #{pr_number}' + ) + return head_commit_sha + except httpx.HTTPStatusError as e: + logger.error( + f'HTTP error fetching PR details for PR #{pr_number}: {e.response.status_code} - {e.response.text}' + ) + raise + except Exception as e: + logger.error(f'Error fetching PR details for PR #{pr_number}: {e}') + raise + + def _map_line_to_position( + self, diff: str, file_path: str, head_line: int + ) -> int | None: + """Maps a line number in the head commit file to its position in the unified diff.""" + logger.debug(f'Attempting to map {file_path}:{head_line} to diff position.') + position = 0 + current_file_path = None + head_line_counter = 0 + in_target_file_hunk = False + found_target_file = False + + lines = diff.splitlines() + for line_content in lines: + position += 1 + if line_content.startswith('diff --git'): + # Reset for new file + current_file_path = None + in_target_file_hunk = False + found_target_file = False # Reset flag for next file + elif line_content.startswith('+++ b/'): + current_file_path = line_content[6:] + if current_file_path == file_path: + logger.debug(f'Found target file header: {line_content}') + # Reset head line counter for the start of the file's diff + head_line_counter = 0 + in_target_file_hunk = False # Wait for the first hunk header + found_target_file = True + else: + # Not the target file, clear current_file_path to avoid processing its hunks + current_file_path = None + found_target_file = False + + elif ( + found_target_file + ): # Process only if we are inside the target file's diff section + if line_content.startswith('@@'): + # Parse hunk header like @@ -l,s +l,s @@ + parts = line_content.split(' ') + logger.debug(f'Processing hunk header: {line_content}') + if len(parts) > 2 and parts[2].startswith('+'): + try: + new_start_line = int(parts[2].split(',')[0][1:]) + # Set the counter to the line number *before* the hunk starts + head_line_counter = new_start_line - 1 + in_target_file_hunk = True + logger.debug( + f'Parsed new_start_line={new_start_line}, head_line_counter reset to {head_line_counter}' + ) + except (ValueError, IndexError): + logger.warning( + f'Could not parse start line from hunk header: {line_content}' + ) + in_target_file_hunk = ( + False # Stop processing until next valid header + ) + else: + logger.warning( + f'Could not parse hunk header format: {line_content}' + ) + in_target_file_hunk = ( + False # Stop processing until next valid header + ) + elif in_target_file_hunk: + # Process lines within a hunk of the target file + if line_content.startswith('+') or line_content.startswith(' '): + head_line_counter += 1 + # logger.debug(f" Line: {line_content[:30]}... | head_line_counter = {head_line_counter}") # Optional: very verbose + if head_line_counter == head_line: + logger.debug( + f'Found match for {file_path}:{head_line} at position {position}' + ) + return position + # Ignore '-' lines for head_line_counter + + logger.warning( + f'Could not find position for {file_path}:{head_line} in diff. Reached end of diff.' + ) + return None + + async def post_review(self, pr_number: int, comments: list[ReviewComment]) -> None: + """Post review comments to a GitHub pull request. + + Args: + pr_number: The number of the pull request. + comments: A list of ReviewComment objects. + """ + review_url = f'{self.base_url}/pulls/{pr_number}/reviews' + headers = self.get_headers() # Use standard headers + # Fetch the diff first + try: + diff = await self.get_pr_diff(pr_number) + head_commit_sha = await self.get_pr_head_commit( + pr_number + ) # Also get head commit SHA + except Exception as e: + logger.error( + f'Failed to fetch diff or head commit for PR #{pr_number} before posting review: {e}' + ) + # Decide how to handle: raise, post general comment, or try posting without positions? + # For now, let's raise to make the failure explicit. + raise RuntimeError( + f'Could not fetch diff or head commit for PR #{pr_number}' + ) from e + + api_comments = [] + general_comments = [] + for comment in comments: + if comment.path and comment.line: + # Map head commit line number to diff position + position = self._map_line_to_position(diff, comment.path, comment.line) + if position: + api_comments.append( + { + 'path': comment.path, + 'position': position, # Use position instead of line + 'body': comment.comment, + } + ) + else: + # Could not map line to position, post as general comment + logger.warning( + f'Could not map {comment.path}:{comment.line} to diff position. Adding as general comment.' + ) + general_comments.append( + f'(Unpositioned) {comment.path}:{comment.line}: {comment.comment}' + ) + else: + # Collect comments without path/line for the main review body + general_comments.append(comment.comment) + + # Construct the main review body + review_body = 'OpenHands AI Code Review:\\n\\n' + if general_comments: + review_body += ( + '**General Feedback / Unpositioned Comments:**\\n' # Updated title + + '\\n'.join([f'- {gc}' for gc in general_comments]) + + '\\n\\n' + ) + if api_comments: # Check if there are any positioned comments left + review_body += '**Line-Specific Feedback:** (see comments below)' + # If only general comments exist, the line-specific part is omitted. + + review_data = { + 'body': review_body.strip(), + 'event': 'COMMENT', # Post comments without changing PR state + 'comments': api_comments, # This now contains comments with 'position' + 'commit_id': head_commit_sha, # commit_id is recommended when using position + } + + async with httpx.AsyncClient() as client: + try: + response = await client.post( + review_url, headers=headers, json=review_data + ) + response.raise_for_status() + logger.info(f'Successfully posted review to PR #{pr_number}.') + except httpx.HTTPStatusError as e: + logger.error( + f'Failed to post review to PR #{pr_number}: {e.response.status_code} {e.response.text}' + ) + raise # Re-raise after logging + except Exception as e: + logger.error(f'Unexpected error posting review to PR #{pr_number}: {e}') + raise # Re-raise after logging + # Override processing of downloaded issues def get_pr_comments( self, pr_number: int, comment_id: int | None = None diff --git a/openhands/resolver/interfaces/gitlab.py b/openhands/resolver/interfaces/gitlab.py index 22ed3c3e063a..1999d9778be9 100644 --- a/openhands/resolver/interfaces/gitlab.py +++ b/openhands/resolver/interfaces/gitlab.py @@ -3,6 +3,7 @@ import httpx +from openhands.code_reviewer.reviewer_output import ReviewComment from openhands.core.logger import openhands_logger as logger from openhands.resolver.interfaces.issue import ( Issue, @@ -19,7 +20,7 @@ def __init__( repo: str, token: str, username: str | None = None, - base_domain: str = 'gitlab.com', + base_domain: str | None = 'gitlab.com', ): """Initialize a GitLab issue handler. @@ -319,7 +320,7 @@ def __init__( repo: str, token: str, username: str | None = None, - base_domain: str = 'gitlab.com', + base_domain: str | None = 'gitlab.com', ): """Initialize a GitLab PR handler. @@ -609,3 +610,113 @@ def get_converted_issues( converted_issues.append(issue_details) return converted_issues + + async def post_review(self, pr_number: int, comments: list[ReviewComment]) -> None: + """Post review comments to a GitLab merge request.""" + if not comments: + logger.info(f'No comments to post for MR #{pr_number}.') + return + + mr_details_url = f'{self.base_url}/merge_requests/{pr_number}' + discussions_url = f'{self.base_url}/merge_requests/{pr_number}/discussions' + mr_details = None + + async with httpx.AsyncClient() as client: + # Fetch MR details asynchronously + try: + response = await client.get(mr_details_url, headers=self.headers) + response.raise_for_status() + mr_details = response.json() + # Basic validation (remains the same) + if ( + not isinstance(mr_details, dict) + or not all( + k in mr_details + for k in ['diff_refs', 'target_project_id', 'iid'] + ) + or not isinstance(mr_details.get('diff_refs'), dict) + or not all( + k in mr_details['diff_refs'] + for k in ['base_sha', 'start_sha', 'head_sha'] + ) + ): + logger.error( + f'Missing or invalid required fields in MR details response for MR #{pr_number}. Cannot post positional comments.' + ) + mr_details = None + except httpx.HTTPStatusError as e: + logger.error( + f'HTTP error fetching MR details for MR #{pr_number}: {e.response.status_code} - {e.response.text}' + ) + # Decide if we should proceed without details or raise + except Exception as e: + logger.error(f'Error fetching MR details for MR #{pr_number}: {e}') + # Decide if we should proceed without details or raise + + # Post comments asynchronously + for comment in comments: + payload: dict[str, Any] = {'body': comment.comment} + if comment.path and comment.line and mr_details: + payload['position'] = { + 'position_type': 'text', + 'base_sha': mr_details['diff_refs']['base_sha'], + 'start_sha': mr_details['diff_refs']['start_sha'], + 'head_sha': mr_details['diff_refs']['head_sha'], + 'new_path': comment.path, + 'new_line': comment.line, + } + elif comment.path or comment.line: + logger.warning( + f'Cannot add position for comment on MR #{pr_number} due to missing MR details or path/line: {comment}' + ) + + try: + response = await client.post( + discussions_url, headers=self.headers, json=payload + ) + # Check status code (201 Created) + if response.status_code == 201: + logger.info( + f'Successfully posted comment to MR #{pr_number}: {comment.comment[:50]}...' + ) + else: + logger.error( + f'Failed to post comment to MR #{pr_number}. Status: {response.status_code}, Response: {response.text}, Payload: {payload}' + ) + # Consider raising based on status code + # response.raise_for_status() # Optionally raise for non-201? + except httpx.RequestError as e: + logger.error( + f'Network error posting comment to MR #{pr_number}: {e}, Payload: {payload}' + ) + # Consider raising + except Exception as e: + logger.error( + f'Unexpected error posting comment to MR #{pr_number}: {e}, Payload: {payload}' + ) + # Consider raising + + async def get_pr_diff(self, pr_number: int) -> str: + """Get the diff content for a GitLab merge request.""" + url = f'{self.base_url}/merge_requests/{pr_number}/diffs' + async with httpx.AsyncClient() as client: + try: + response = await client.get(url, headers=self.headers) + response.raise_for_status() + diffs = response.json() + if isinstance(diffs, list) and len(diffs) > 0 and 'diff' in diffs[0]: + logger.info(f'Successfully fetched diff for GitLab MR #{pr_number}') + return diffs[0]['diff'] + else: + logger.warning( + f'Could not extract diff from response for MR #{pr_number}. Response: {diffs}' + ) + return '' + except httpx.HTTPStatusError as e: + logger.error( + f'HTTP error fetching diff for MR #{pr_number}: {e.response.status_code} - {e.response.text}' + ) + raise # Re-raise after logging + except Exception as e: + logger.error(f'Error fetching diff for MR #{pr_number}: {e}') + raise # Re-raise after logging diff --git a/openhands/resolver/resolve_issue.py b/openhands/resolver/resolve_issue.py index 03e66accb3ab..64de38c9a6c0 100644 --- a/openhands/resolver/resolve_issue.py +++ b/openhands/resolver/resolve_issue.py @@ -161,7 +161,7 @@ async def complete_runtime( return {'git_patch': git_patch} -async def process_issue( +async def process_resolve( issue: Issue, platform: ProviderType, base_commit: str, @@ -354,7 +354,7 @@ def issue_handler_factory( raise ValueError(f'Invalid issue type: {issue_type}') -async def resolve_issue( +async def run_resolve_task( owner: str, repo: str, token: str, @@ -522,7 +522,7 @@ async def resolve_issue( .strip() ) - output = await process_issue( + output = await process_resolve( issue, platform, base_commit, @@ -660,21 +660,59 @@ def int_or_none(value: str) -> int | None: my_args = parser.parse_args() - base_container_image = my_args.base_container_image - - runtime_container_image = my_args.runtime_container_image + # Initialize container image variables + base_container_image: str | None = None + runtime_container_image: str | None = None + # Get container image from environment variable first + env_base_image_as_runtime = os.getenv( + 'SANDBOX_BASE_CONTAINER_IMAGE' + ) # Check for base image env var to use as runtime + + if env_base_image_as_runtime: + logger.info( + f'Using SANDBOX_BASE_CONTAINER_IMAGE as runtime image: {env_base_image_as_runtime}' + ) + runtime_container_image = env_base_image_as_runtime + base_container_image = ( + None # Ensure base_container_image is None if env var is used + ) + else: + # Fallback to command-line arguments if env var is not set + logger.info( + 'SANDBOX_BASE_CONTAINER_IMAGE not set, checking command-line arguments for runtime/base images.' + ) + arg_base_image = my_args.base_container_image + arg_runtime_image = my_args.runtime_container_image - if runtime_container_image is not None and base_container_image is not None: - raise ValueError('Cannot provide both runtime and base container images.') + if arg_runtime_image is not None and arg_base_image is not None: + raise ValueError( + 'Cannot provide both --runtime-container-image and --base-container-image via arguments when SANDBOX_BASE_CONTAINER_IMAGE is not set.' + ) - if ( - runtime_container_image is None - and base_container_image is None - and not my_args.is_experimental - ): - runtime_container_image = ( - f'ghcr.io/all-hands-ai/runtime:{openhands.__version__}-nikolaik' - ) + # Determine the final image configuration based on args + if arg_base_image is not None: + logger.info(f'Using base container image from args: {arg_base_image}') + base_container_image = arg_base_image + # runtime_container_image remains None + elif arg_runtime_image is not None: + logger.info(f'Using runtime container image from args: {arg_runtime_image}') + runtime_container_image = arg_runtime_image + # base_container_image remains None + elif not my_args.is_experimental: + # Neither arg provided, not experimental: use default runtime image + runtime_container_image = ( + f'ghcr.io/all-hands-ai/runtime:{openhands.__version__}-nikolaik' + ) + logger.info( + f'Defaulting runtime container image to: {runtime_container_image}' + ) + # base_container_image remains None + else: + # Neither arg provided, IS experimental: leave both as None + logger.info( + 'No container image specified via args or env, and is_experimental=True. Both images remain None.' + ) + # Both base_container_image and runtime_container_image remain None parts = my_args.selected_repo.rsplit('/', 1) if len(parts) < 2: @@ -734,7 +772,7 @@ def int_or_none(value: str) -> int | None: prompt_template = f.read() asyncio.run( - resolve_issue( + run_resolve_task( owner=owner, repo=repo, token=token, diff --git a/poetry.lock b/poetry.lock index 0327733b7b5d..811336510e8f 100644 --- a/poetry.lock +++ b/poetry.lock @@ -1,4 +1,16 @@ -# This file is automatically @generated by Poetry 2.1.1 and should not be changed by hand. +# This file is automatically @generated by Poetry 2.1.2 and should not be changed by hand. + +[[package]] +name = "aiofiles" +version = "24.1.0" +description = "File support for asyncio." +optional = false +python-versions = ">=3.8" +groups = ["main"] +files = [ + {file = "aiofiles-24.1.0-py3-none-any.whl", hash = "sha256:b4ec55f4195e3eb5d7abd1bf7e061763e864dd4954231fb8539a0ef8bb8260e5"}, + {file = "aiofiles-24.1.0.tar.gz", hash = "sha256:22a075c9e5a3810f0c2e48f3008c94d68c65d763b9b03857924c99e57355166c"}, +] [[package]] name = "aiohappyeyeballs" @@ -2663,7 +2675,7 @@ grpcio = {version = ">=1.49.1,<2.0dev", optional = true, markers = "python_versi grpcio-status = {version = ">=1.49.1,<2.0.dev0", optional = true, markers = "python_version >= \"3.11\" and extra == \"grpc\""} proto-plus = [ {version = ">=1.25.0,<2.0.0dev", markers = "python_version >= \"3.13\""}, - {version = ">=1.22.3,<2.0.0dev", markers = "python_version < \"3.13\""}, + {version = ">=1.22.3,<2.0.0dev"}, ] protobuf = ">=3.19.5,<3.20.0 || >3.20.0,<3.20.1 || >3.20.1,<4.21.0 || >4.21.0,<4.21.1 || >4.21.1,<4.21.2 || >4.21.2,<4.21.3 || >4.21.3,<4.21.4 || >4.21.4,<4.21.5 || >4.21.5,<6.0.0.dev0" requests = ">=2.18.0,<3.0.0.dev0" @@ -4412,14 +4424,14 @@ types-tqdm = "*" [[package]] name = "litellm" -version = "1.67.0" +version = "1.67.2" description = "Library to easily interface with LLM API providers" optional = false python-versions = "!=2.7.*,!=3.0.*,!=3.1.*,!=3.2.*,!=3.3.*,!=3.4.*,!=3.5.*,!=3.6.*,!=3.7.*,>=3.8" groups = ["main"] files = [ - {file = "litellm-1.67.0-py3-none-any.whl", hash = "sha256:d297126f45eea8d8a3df9c0de1d9491ff20e78dab5d1aa3820602082501ba89e"}, - {file = "litellm-1.67.0.tar.gz", hash = "sha256:18439db292d85b1d886bfa35de9d999600ecc6b4fc1137f12e6810d2133c8cec"}, + {file = "litellm-1.67.2-py3-none-any.whl", hash = "sha256:32df4d17b3ead17d04793311858965e41e83a7bdf9bd661895c0e6bc9c78dc8b"}, + {file = "litellm-1.67.2.tar.gz", hash = "sha256:9e108827bff16af04fd4c35b0c1a1d6c7746c96db3870189a60141d449797487"}, ] [package.dependencies] @@ -7686,6 +7698,7 @@ python-versions = "<4,>=3.7" groups = ["test"] files = [ {file = "reportlab-4.4.0-py3-none-any.whl", hash = "sha256:0a993f1d4a765fcbdf4e26adc96b3351004ebf4d27583340595ba7edafebec32"}, + {file = "reportlab-4.4.0.tar.gz", hash = "sha256:a64d85513910e246c21dc97ccc3c9054a1d44370bf8fc1fab80af937814354d5"}, ] [package.dependencies] @@ -9259,7 +9272,7 @@ description = "A language and compiler for custom Deep Learning operations" optional = false python-versions = "*" groups = ["evaluation"] -markers = "platform_system == \"Linux\" and platform_machine == \"x86_64\" and python_version < \"3.13\"" +markers = "platform_system == \"Linux\" and platform_machine == \"x86_64\" and python_version == \"3.12\"" files = [ {file = "triton-3.0.0-1-cp310-cp310-manylinux2014_x86_64.manylinux_2_17_x86_64.whl", hash = "sha256:e1efef76935b2febc365bfadf74bcb65a6f959a9872e5bddf44cc9e0adce1e1a"}, {file = "triton-3.0.0-1-cp311-cp311-manylinux2014_x86_64.manylinux_2_17_x86_64.whl", hash = "sha256:5ce8520437c602fb633f1324cc3871c47bee3b67acf9756c1a66309b60e3216c"}, @@ -9294,6 +9307,18 @@ rich = ">=10.11.0" shellingham = ">=1.3.0" typing-extensions = ">=3.7.4.3" +[[package]] +name = "types-aiofiles" +version = "24.1.0.20250326" +description = "Typing stubs for aiofiles" +optional = false +python-versions = ">=3.9" +groups = ["dev"] +files = [ + {file = "types_aiofiles-24.1.0.20250326-py3-none-any.whl", hash = "sha256:dfb58c9aa18bd449e80fb5d7f49dc3dd20d31de920a46223a61798ee4a521a70"}, + {file = "types_aiofiles-24.1.0.20250326.tar.gz", hash = "sha256:c4bbe432fd043911ba83fb635456f5cc54f6d05fda2aadf6bef12a84f07a6efe"}, +] + [[package]] name = "types-awscrt" version = "0.24.2" @@ -10255,4 +10280,4 @@ testing = ["coverage[toml]", "zope.event", "zope.testing"] [metadata] lock-version = "2.1" python-versions = "^3.12" -content-hash = "82763fb3ce12aba7fbf76651fa3ea72be700feaabb4d944540fc1156745bb6c1" +content-hash = "afb15a619f22b041dcb26aee883b00fdd7fc080e9cc1737f7ff70c7cdb74bcc8" diff --git a/pyproject.toml b/pyproject.toml index 18d7aac846ed..330214be8ecb 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -14,7 +14,7 @@ packages = [ [tool.poetry.dependencies] python = "^3.12" -litellm = "^1.60.0" +litellm = "^1.67.2" aiohttp = ">=3.9.0,!=3.11.13" # Pin to avoid yanked version 3.11.13 google-generativeai = "*" # To use litellm with Gemini Pro API google-api-python-client = "^2.164.0" # For Google Sheets API @@ -76,8 +76,10 @@ mcp = "1.6.0" python-json-logger = "^3.2.1" playwright = "^1.51.0" prompt-toolkit = "^3.0.50" +aiofiles = "^24.1.0" [tool.poetry.group.dev.dependencies] +types-aiofiles = "*" ruff = "0.11.6" mypy = "1.15.0" pre-commit = "4.2.0" diff --git a/tests/unit/code_reviewer/__init__.py b/tests/unit/code_reviewer/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/tests/unit/code_reviewer/test_post_review_comments.py b/tests/unit/code_reviewer/test_post_review_comments.py new file mode 100644 index 000000000000..5bbd466e4451 --- /dev/null +++ b/tests/unit/code_reviewer/test_post_review_comments.py @@ -0,0 +1,230 @@ +import dataclasses +import json +from unittest.mock import AsyncMock, patch + +import pytest + +from openhands.code_reviewer.post_review_comments import post_comments +from openhands.code_reviewer.reviewer_output import ReviewComment, ReviewerOutput +from openhands.integrations.service_types import ProviderType +from openhands.resolver.interfaces.issue import Issue, IssueHandlerInterface + + +@pytest.fixture +def sample_review_output(): + return ReviewerOutput( + pr_info=Issue( + number=123, + repo='test/repo', + owner='test', + title='Test PR', + description='A test PR', + body='Body of test PR', + ), + success=True, + error=None, + review_level='line', + review_depth='quick', + instruction='Review this PR', + history=[], + metrics={}, + comments=[ + ReviewComment(path='file1.py', line=10, comment='Comment 1'), + ReviewComment(path='file2.py', line=20, comment='Comment 2'), + ], + ) + + +@patch('openhands.code_reviewer.post_review_comments.get_pr_handler') +def test_post_comments_success(mock_get_handler, tmp_path, sample_review_output): + """Tests successful posting of comments from a valid JSONL file.""" + mock_handler = AsyncMock(spec=IssueHandlerInterface, post_review=AsyncMock()) + mock_get_handler.return_value = mock_handler + + output_file = tmp_path / 'review_output_123.jsonl' + # Need to fix the ReviewerOutput structure before dumping + output_dict = dataclasses.asdict(sample_review_output) + output_dict['pr_info'] = ( + sample_review_output.pr_info.model_dump() + ) # Serialize Issue + output_data = json.dumps(output_dict) + + # Use standard open for writing the test file + with open(output_file, mode='w') as f: + f.write(output_data + '\n') # Write as JSONL + + post_comments( + str(output_file), token=None, selected_repo='test/repo', pr_number=123 + ) + + # Extract owner/repo from selected_repo + owner, repo_name = 'test', 'repo' + mock_get_handler.assert_called_once_with( + owner, repo_name, None, ProviderType.GITHUB, None + ) # Added base_domain=None + mock_handler.post_review.assert_called_once_with( + pr_number=123, comments=sample_review_output.comments + ) + + +@patch('openhands.code_reviewer.post_review_comments.get_pr_handler') +def test_post_comments_file_not_found(mock_get_handler): + """Tests behavior when the JSONL file does not exist.""" + mock_handler = AsyncMock(spec=IssueHandlerInterface, post_review=AsyncMock()) + mock_get_handler.return_value = mock_handler + + # post_comments now handles FileNotFoundError internally and logs an error + post_comments( + 'non_existent_file.jsonl', token=None, selected_repo='test/repo', pr_number=123 + ) + mock_get_handler.assert_not_called() # Handler shouldn't be created if file not found + mock_handler.post_review.assert_not_called() + + +@patch('openhands.code_reviewer.post_review_comments.get_pr_handler') +def test_post_comments_empty_file(mock_get_handler, tmp_path): + """Tests behavior when the JSONL file is empty.""" + mock_handler = AsyncMock(spec=IssueHandlerInterface, post_review=AsyncMock()) + mock_get_handler.return_value = mock_handler + + output_file = tmp_path / 'empty_review_output.jsonl' + output_file.touch() # Create empty file + + # post_comments should handle empty file gracefully (log error) + post_comments( + str(output_file), token=None, selected_repo='test/repo', pr_number=123 + ) + mock_get_handler.assert_not_called() # Handler shouldn't be created if file is empty/invalid + mock_handler.post_review.assert_not_called() + # TODO: Add assertion for logging if logging is implemented + + +@patch('openhands.code_reviewer.post_review_comments.get_pr_handler') +def test_post_comments_invalid_json(mock_get_handler, tmp_path): + """Tests behavior when the JSONL file contains invalid JSON.""" + mock_handler = AsyncMock(spec=IssueHandlerInterface, post_review=AsyncMock()) + mock_get_handler.return_value = mock_handler + + output_file = tmp_path / 'invalid_json.jsonl' + with open(output_file, mode='w') as f: + f.write('this is not valid json\\n') # Write invalid JSON + + # post_comments should handle JSONDecodeError gracefully (log error) + post_comments( + str(output_file), token=None, selected_repo='test/repo', pr_number=123 + ) + + mock_get_handler.assert_not_called() # Handler shouldn't be created if JSON is invalid + mock_handler.post_review.assert_not_called() + # TODO: Add assertion for logging if logging is implemented + + +@patch('openhands.code_reviewer.post_review_comments.get_pr_handler') +def test_post_comments_missing_comments_field( + mock_get_handler, tmp_path, sample_review_output +): + """Tests behavior when 'comments' field is null in the JSONL.""" + mock_handler = AsyncMock(spec=IssueHandlerInterface, post_review=AsyncMock()) + mock_get_handler.return_value = mock_handler + + output_file = tmp_path / 'missing_comments.jsonl' + # Write *something* to the file, the content doesn't matter much as parsing is mocked + output_data = json.dumps( + { + 'pr_info': sample_review_output.pr_info.model_dump(), + 'review_level': 'line', + 'review_depth': 'quick', + 'instruction': 'Review this PR', + 'error': None, + 'history': [], + 'metrics': {}, + 'success': True, + 'comments': None, # Explicitly null + } + ) + with open(output_file, mode='w') as f: + f.write(output_data + '\n') + + post_comments( + str(output_file), token=None, selected_repo='test/repo', pr_number=123 + ) + + # Handler should NOT be created if comments are null/missing + mock_get_handler.assert_not_called() + mock_handler.post_review.assert_not_called() + # TODO: Add assertion for logging if logging is implemented + + +@patch('openhands.code_reviewer.post_review_comments.get_pr_handler') +def test_post_comments_empty_comments_list( + mock_get_handler, tmp_path, sample_review_output +): + """Tests behavior when 'comments' list is empty.""" + mock_handler = AsyncMock(spec=IssueHandlerInterface, post_review=AsyncMock()) + mock_get_handler.return_value = mock_handler + + output_file = tmp_path / 'empty_comments_list.jsonl' + sample_review_output.comments = [] # Set comments to empty list + # Need to fix the ReviewerOutput structure before dumping + output_dict = dataclasses.asdict(sample_review_output) + output_dict['pr_info'] = ( + sample_review_output.pr_info.model_dump() + ) # Serialize Issue + # Remove fields not present in the actual JSONL output from review_pr.py + + output_data = json.dumps(output_dict) + + with open(output_file, mode='w') as f: + f.write(output_data + '\n') + + post_comments( + str(output_file), token=None, selected_repo='test/repo', pr_number=123 + ) + + # Handler should NOT be created if comments are empty + mock_get_handler.assert_not_called() + mock_handler.post_review.assert_not_called() + # TODO: Add assertion for logging if logging is implemented + + +@patch('openhands.code_reviewer.post_review_comments.get_pr_handler') +def test_post_comments_multiple_lines(mock_get_handler, tmp_path, sample_review_output): + """Tests posting comments when the JSONL file has multiple lines (should only process first).""" + mock_handler = AsyncMock(spec=IssueHandlerInterface, post_review=AsyncMock()) + mock_get_handler.return_value = mock_handler + + output_file = tmp_path / 'multiple_lines.jsonl' + + # Prepare first line data + output_dict1 = dataclasses.asdict(sample_review_output) + output_dict1['pr_info'] = sample_review_output.pr_info.model_dump() + output_data1 = json.dumps(output_dict1) + + # Prepare second line data (different comments) + sample_review_output.comments = [ + ReviewComment(path='file2.py', line=5, comment='Second comment') + ] + output_dict2 = dataclasses.asdict(sample_review_output) + output_dict2['pr_info'] = sample_review_output.pr_info.model_dump() + output_data2 = json.dumps(output_dict2) + + with open(output_file, mode='w') as f: + f.write(output_data1 + '\n') + f.write(output_data2 + '\n') + + post_comments( + str(output_file), token=None, selected_repo='test/repo', pr_number=123 + ) + + # Should only post comments from the first line + owner, repo_name = 'test', 'repo' + mock_get_handler.assert_called_once_with( + owner, repo_name, None, ProviderType.GITHUB, None + ) # Added base_domain=None + mock_handler.post_review.assert_called_once_with( + pr_number=123, + comments=[ + ReviewComment(path='file1.py', line=10, comment='Comment 1'), + ReviewComment(path='file2.py', line=20, comment='Comment 2'), + ], # Comments from the first line + ) diff --git a/tests/unit/code_reviewer/test_review_pr.py b/tests/unit/code_reviewer/test_review_pr.py new file mode 100644 index 000000000000..76783ee8604e --- /dev/null +++ b/tests/unit/code_reviewer/test_review_pr.py @@ -0,0 +1,444 @@ +import dataclasses # Added import +import json +import os +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from openhands.code_reviewer.review_pr import process_pr_for_review +from openhands.code_reviewer.reviewer_output import ReviewComment +from openhands.controller.state.state import State +from openhands.core.config import LLMConfig +from openhands.core.schema import AgentState +from openhands.events.action import MessageAction +from openhands.integrations.service_types import ProviderType +from openhands.resolver.interfaces.issue import Issue, IssueHandlerInterface + +# Sample data +SAMPLE_ISSUE = Issue( + number=101, + repo='test/repo', + owner='test', + title='Test PR for Review', + body='PR body content.', +) +SAMPLE_LLM_CONFIG = LLMConfig(model='gpt-4o') +SAMPLE_PROMPT_TEMPLATE = 'Review this diff:\n{{ pr_diff }}' +SAMPLE_DIFF = 'diff --git a/file.py b/file.py\nindex 123..456 100644\n--- a/file.py\n+++ b/file.py\n@@ -1,1 +1,1 @@\n-old line\n+new line\n' + + +@pytest.fixture +def mock_issue_handler(): + handler = AsyncMock(spec=IssueHandlerInterface) + handler.get_pr_diff = AsyncMock(return_value=SAMPLE_DIFF) + # Add other methods if needed by the function under test + return handler + + +@pytest.fixture +def setup_workspace(tmp_path): + output_dir = tmp_path / 'review_output' + output_dir.mkdir() + repo_dir = output_dir / 'repo' + repo_dir.mkdir() + # Create a dummy file in the repo to be copied + (repo_dir / 'dummy_file.txt').touch() + return str(output_dir) + + +# Mock runtime and controller +@patch('openhands.code_reviewer.review_pr.create_runtime') +@patch('openhands.code_reviewer.review_pr.run_controller') +@pytest.mark.asyncio +async def test_process_pr_success( + mock_run_controller, mock_create_runtime, mock_issue_handler, setup_workspace +): + """Tests the success case where the agent finishes and returns valid comments.""" + output_dir = setup_workspace + + # Mock Runtime + mock_runtime_instance = AsyncMock() + mock_runtime_instance.run_action = MagicMock() # Initialize run_action mock + mock_runtime_instance.close = AsyncMock() + mock_create_runtime.return_value = mock_runtime_instance + + # Mock Controller State (Success) + final_state = State() + final_state.agent_state = AgentState.FINISHED + final_comments = [ + ReviewComment(path='file.py', line=1, comment='Looks good!'), + ReviewComment(path='other.py', comment='General comment'), # No line + ] + final_message_content = json.dumps([dataclasses.asdict(c) for c in final_comments]) + final_state.history.append(MessageAction(content=final_message_content)) + final_state.history[-1]._source = 'agent' # Set internal _source attribute + mock_run_controller.return_value = final_state + + # Call the function + result = await process_pr_for_review( + issue=SAMPLE_ISSUE, + platform=ProviderType.GITHUB, + max_iterations=5, + llm_config=SAMPLE_LLM_CONFIG, + output_dir=output_dir, + base_container_image=None, + runtime_container_image=None, + prompt_template=SAMPLE_PROMPT_TEMPLATE, + issue_handler=mock_issue_handler, + review_level='line', + review_depth='full', + ) + + # Assertions + assert result.success is True + assert result.error is None + assert result.pr_info == SAMPLE_ISSUE + assert result.review_level == 'line' + assert result.review_depth == 'full' + assert len(result.comments) == 2 + assert result.comments[0].path == 'file.py' + assert result.comments[0].line == 1 + assert result.comments[0].comment == 'Looks good!' + assert result.comments[1].path == 'other.py' + assert result.comments[1].line is None + assert result.comments[1].comment == 'General comment' + assert result.history is not None # Check history exists + assert result.metrics is not None # Check metrics exist + + # Verify mocks + mock_issue_handler.get_pr_diff.assert_awaited_once_with(SAMPLE_ISSUE.number) + mock_create_runtime.assert_called_once() + mock_run_controller.assert_awaited_once() + mock_runtime_instance.close.assert_awaited_once() + + # Check workspace was created + workspace_path = os.path.join(output_dir, 'workspace', f'pr_{SAMPLE_ISSUE.number}') + assert os.path.exists(workspace_path) + assert os.path.exists(os.path.join(workspace_path, 'dummy_file.txt')) + + +# TODO: Add more test cases: success no comments, agent error, json error, diff error etc. + + +# Mock runtime and controller +@patch('openhands.code_reviewer.review_pr.create_runtime') +@patch('openhands.code_reviewer.review_pr.run_controller') +@pytest.mark.asyncio +async def test_process_pr_success_no_comments( + mock_run_controller, mock_create_runtime, mock_issue_handler, setup_workspace +): + """Tests the success case where the agent finishes but returns no valid comments.""" + output_dir = setup_workspace + + # Mock Runtime + mock_runtime_instance = AsyncMock() + mock_runtime_instance.run_action = MagicMock() + mock_runtime_instance.close = AsyncMock() + mock_create_runtime.return_value = mock_runtime_instance + + # Mock Controller State (Success, but no valid comments in last message) + final_state = State() + final_state.agent_state = AgentState.FINISHED + # Case 1: Last message is not from agent + # final_state.history.append(MessageAction(content='User message', source='user')) + # Case 2: Last message is from agent, but empty content + # final_state.history.append(MessageAction(content='', source='agent')) + # Case 3: Last message is from agent, but not valid JSON + # final_state.history.append(MessageAction(content='Not JSON', source='agent')) + # Case 4: Last message is from agent, valid JSON, but empty list + final_state.history.append(MessageAction(content='[]')) + final_state.history[-1]._source = 'agent' # Set internal _source attribute + # Case 5: Last message is from agent, valid JSON, but wrong structure + # final_state.history.append(MessageAction(content='{"comment": "hello"}', source='agent')) + + mock_run_controller.return_value = final_state + + # Call the function + result = await process_pr_for_review( + issue=SAMPLE_ISSUE, + platform=ProviderType.GITHUB, + max_iterations=5, + llm_config=SAMPLE_LLM_CONFIG, + output_dir=output_dir, + base_container_image=None, + runtime_container_image=None, + prompt_template=SAMPLE_PROMPT_TEMPLATE, + issue_handler=mock_issue_handler, + review_level='line', + review_depth='full', + ) + + # Assertions + assert result.success is True # Still successful as agent finished + assert result.error is None + assert result.pr_info == SAMPLE_ISSUE + assert len(result.comments) == 0 # No comments extracted + assert result.history is not None + assert result.metrics is not None + + # Verify mocks + mock_issue_handler.get_pr_diff.assert_awaited_once_with(SAMPLE_ISSUE.number) + mock_create_runtime.assert_called_once() + mock_run_controller.assert_awaited_once() + mock_runtime_instance.close.assert_awaited_once() + + +# Mock runtime and controller +@patch('openhands.code_reviewer.review_pr.create_runtime') +@patch('openhands.code_reviewer.review_pr.run_controller') +@pytest.mark.asyncio +async def test_process_pr_agent_error( + mock_run_controller, mock_create_runtime, mock_issue_handler, setup_workspace +): + """Tests the case where the agent finishes in an error state.""" + output_dir = setup_workspace + + # Mock Runtime + mock_runtime_instance = AsyncMock() + mock_runtime_instance.run_action = MagicMock() + mock_runtime_instance.close = AsyncMock() + mock_create_runtime.return_value = mock_runtime_instance + + # Mock Controller State (Agent Failed) + final_state = State() + final_state.agent_state = AgentState.ERROR + final_state.history.append(MessageAction(content='Error occurred')) + final_state.history[-1]._source = 'agent' + mock_run_controller.return_value = final_state + + # Call the function + result = await process_pr_for_review( + issue=SAMPLE_ISSUE, + platform=ProviderType.GITHUB, + max_iterations=5, + llm_config=SAMPLE_LLM_CONFIG, + output_dir=output_dir, + base_container_image=None, + runtime_container_image=None, + prompt_template=SAMPLE_PROMPT_TEMPLATE, + issue_handler=mock_issue_handler, + review_level='line', + review_depth='full', + ) + + # Assertions + assert result.success is False + assert result.error is not None + assert 'Agent finished in ERROR state.' in result.error + assert result.pr_info == SAMPLE_ISSUE + assert len(result.comments) == 0 + assert result.history is not None + assert result.metrics is not None + + # Verify mocks + mock_issue_handler.get_pr_diff.assert_awaited_once_with(SAMPLE_ISSUE.number) + mock_create_runtime.assert_called_once() + mock_run_controller.assert_awaited_once() + mock_runtime_instance.close.assert_awaited_once() + + +# Mock runtime and controller +@patch('openhands.code_reviewer.review_pr.create_runtime') +@patch('openhands.code_reviewer.review_pr.run_controller') +@pytest.mark.asyncio +async def test_process_pr_json_error( + mock_run_controller, mock_create_runtime, mock_issue_handler, setup_workspace +): + """Tests the case where the agent finishes but the last message is not valid JSON.""" + output_dir = setup_workspace + + # Mock Runtime + mock_runtime_instance = AsyncMock() + mock_runtime_instance.run_action = MagicMock() + mock_runtime_instance.close = AsyncMock() + mock_create_runtime.return_value = mock_runtime_instance + + # Mock Controller State (Success, but invalid JSON) + final_state = State() + final_state.agent_state = AgentState.FINISHED + final_state.history.append(MessageAction(content='This is not JSON')) + final_state.history[-1]._source = 'agent' + mock_run_controller.return_value = final_state + + # Call the function + result = await process_pr_for_review( + issue=SAMPLE_ISSUE, + platform=ProviderType.GITHUB, + max_iterations=5, + llm_config=SAMPLE_LLM_CONFIG, + output_dir=output_dir, + base_container_image=None, + runtime_container_image=None, + prompt_template=SAMPLE_PROMPT_TEMPLATE, + issue_handler=mock_issue_handler, + review_level='line', + review_depth='full', + ) + + # Assertions + assert result.success is False # Agent finished, but comment parsing failed + assert result.error is not None # Error should indicate JSON parsing failure + assert "Failed to parse agent's final message as JSON" in result.error + assert result.pr_info == SAMPLE_ISSUE + assert len(result.comments) == 0 # No comments extracted due to JSON error + assert result.history is not None + assert result.metrics is not None + + # Verify mocks + mock_issue_handler.get_pr_diff.assert_awaited_once_with(SAMPLE_ISSUE.number) + mock_create_runtime.assert_called_once() + mock_run_controller.assert_awaited_once() + mock_runtime_instance.close.assert_awaited_once() + + +# Mock runtime and controller +@patch('openhands.code_reviewer.review_pr.create_runtime') +@patch('openhands.code_reviewer.review_pr.run_controller') +@pytest.mark.asyncio +async def test_process_pr_diff_error( + mock_run_controller, mock_create_runtime, mock_issue_handler, setup_workspace +): + """Tests the case where fetching the PR diff fails.""" + output_dir = setup_workspace + + # Mock Issue Handler (Error) + mock_issue_handler.get_pr_diff.side_effect = Exception('Failed to fetch diff') + + # Mock Runtime (Should not be created) + mock_runtime_instance = AsyncMock() + mock_create_runtime.return_value = mock_runtime_instance + + # Mock Controller (Should not be run) + mock_run_controller.return_value = State() # Dummy state + + # Call the function + result = await process_pr_for_review( + issue=SAMPLE_ISSUE, + platform=ProviderType.GITHUB, + max_iterations=5, + llm_config=SAMPLE_LLM_CONFIG, + output_dir=output_dir, + base_container_image=None, + runtime_container_image=None, + prompt_template=SAMPLE_PROMPT_TEMPLATE, + issue_handler=mock_issue_handler, + review_level='line', + review_depth='full', + ) + + # Assertions + assert result.success is False + assert result.error is not None + assert 'Failed to fetch diff' in result.error + assert result.pr_info == SAMPLE_ISSUE + assert len(result.comments) == 0 + assert result.history == [] # History is initialized but empty + assert result.metrics is None # Metrics are part of state + + # Verify mocks + mock_issue_handler.get_pr_diff.assert_awaited_once_with(SAMPLE_ISSUE.number) + mock_create_runtime.assert_called_once() # Runtime is created before diff is fetched + mock_run_controller.assert_not_awaited() + mock_runtime_instance.close.assert_awaited_once() # Runtime should be closed in finally block + + +# Mock runtime and controller +@patch('openhands.code_reviewer.review_pr.create_runtime') +@patch('openhands.code_reviewer.review_pr.run_controller') +@pytest.mark.asyncio +async def test_process_pr_runtime_error( + mock_run_controller, mock_create_runtime, mock_issue_handler, setup_workspace +): + """Tests the case where creating the runtime fails.""" + output_dir = setup_workspace + + # Mock Issue Handler (Success) + mock_issue_handler.get_pr_diff.return_value = SAMPLE_DIFF + + # Mock Runtime Creation (Error) + mock_create_runtime.side_effect = Exception('Runtime creation failed') + + # Mock Controller (Should not be run) + mock_run_controller.return_value = State() # Dummy state + + # Call the function + result = await process_pr_for_review( + issue=SAMPLE_ISSUE, + platform=ProviderType.GITHUB, + max_iterations=5, + llm_config=SAMPLE_LLM_CONFIG, + output_dir=output_dir, + base_container_image=None, + runtime_container_image=None, + prompt_template=SAMPLE_PROMPT_TEMPLATE, + issue_handler=mock_issue_handler, + review_level='line', + review_depth='full', + ) + + # Assertions + assert result.success is False + assert result.error is not None + assert 'Runtime creation failed' in result.error + assert result.pr_info == SAMPLE_ISSUE + assert len(result.comments) == 0 + assert result.history == [] + assert result.metrics is None + + # Verify mocks + mock_issue_handler.get_pr_diff.assert_not_awaited() # Should not be called if runtime fails + mock_create_runtime.assert_called_once() + mock_run_controller.assert_not_awaited() + + +# Mock runtime and controller +@patch('openhands.code_reviewer.review_pr.create_runtime') +@patch('openhands.code_reviewer.review_pr.run_controller') +@pytest.mark.asyncio +async def test_process_pr_controller_error( + mock_run_controller, mock_create_runtime, mock_issue_handler, setup_workspace +): + """Tests the case where the controller fails during execution.""" + output_dir = setup_workspace + + # Mock Issue Handler (Success) + mock_issue_handler.get_pr_diff.return_value = SAMPLE_DIFF + + # Mock Runtime (Success) + mock_runtime_instance = AsyncMock() + mock_runtime_instance.run_action = MagicMock() + mock_runtime_instance.close = AsyncMock() + mock_create_runtime.return_value = mock_runtime_instance + + # Mock Controller (Error) + mock_run_controller.side_effect = Exception('Controller failed') + + # Call the function + result = await process_pr_for_review( + issue=SAMPLE_ISSUE, + platform=ProviderType.GITHUB, + max_iterations=5, + llm_config=SAMPLE_LLM_CONFIG, + output_dir=output_dir, + base_container_image=None, + runtime_container_image=None, + prompt_template=SAMPLE_PROMPT_TEMPLATE, + issue_handler=mock_issue_handler, + review_level='line', + review_depth='full', + ) + + # Assertions + assert result.success is False + assert result.error is not None + assert 'Controller failed' in result.error + assert result.pr_info == SAMPLE_ISSUE + assert len(result.comments) == 0 + assert result.history == [] # Controller failed before returning state + assert result.metrics is None + + # Verify mocks + mock_issue_handler.get_pr_diff.assert_awaited_once_with(SAMPLE_ISSUE.number) + mock_create_runtime.assert_called_once() + mock_run_controller.assert_awaited_once() + mock_runtime_instance.close.assert_awaited_once() # Should still be closed in finally block diff --git a/tests/unit/resolver/interfaces/__init__.py b/tests/unit/resolver/interfaces/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/tests/unit/resolver/interfaces/test_github.py b/tests/unit/resolver/interfaces/test_github.py new file mode 100644 index 000000000000..6901cbd653a8 --- /dev/null +++ b/tests/unit/resolver/interfaces/test_github.py @@ -0,0 +1,226 @@ +from unittest.mock import AsyncMock, MagicMock, patch + +import httpx +import pytest +from pydantic import SecretStr + +from openhands.code_reviewer.reviewer_output import ReviewComment +from openhands.resolver.interfaces.github import GithubPRHandler + +OWNER = 'test-owner' +REPO = 'test-repo' +PR_NUM = 123 +TOKEN = SecretStr('test-token') + + +@pytest.fixture +def github_handler(): + return GithubPRHandler(token=TOKEN, owner=OWNER, repo=REPO) + + +@pytest.mark.asyncio +@patch('httpx.AsyncClient') +async def test_get_pr_diff_success(MockAsyncClient, github_handler): + """Tests successful retrieval of PR diff using AsyncClient.""" + mock_response = AsyncMock(spec=httpx.Response) + mock_response.status_code = 200 + mock_response.text = 'sample diff content' + mock_response.headers = {'content-type': 'application/vnd.github.v3.diff'} + # mock_response.raise_for_status = MagicMock() # Removed - rely on status_code + + # Configure the mock client instance returned by the context manager + mock_client_instance = MockAsyncClient.return_value.__aenter__.return_value + mock_client_instance.get.return_value = mock_response + + # Call the async method + diff = await github_handler.get_pr_diff(PR_NUM) + + assert diff == 'sample diff content' + expected_headers = { + 'Accept': 'application/vnd.github.v3.diff', + 'Authorization': 'token test-token', # Use real token for assertion + 'X-GitHub-Api-Version': '2022-11-28', + } + expected_url = f'https://api.github.com/repos/{OWNER}/{REPO}/pulls/{PR_NUM}' + + # Check that the get method was called correctly on the client instance + mock_response.raise_for_status.assert_called_once() # Verify raise_for_status was called + mock_client_instance.get.assert_called_once_with( + expected_url, headers=expected_headers + ) + mock_response.raise_for_status.assert_called_once() # Verify raise_for_status was called + + +@pytest.mark.asyncio +@patch('httpx.AsyncClient') +async def test_get_pr_diff_error(MockAsyncClient, github_handler): + """Tests error handling when fetching PR diff fails using AsyncClient.""" + mock_response = AsyncMock(spec=httpx.Response) + mock_response.status_code = 404 + mock_response.text = 'Not Found' + mock_http_error = httpx.HTTPStatusError( + 'Not Found', + request=MagicMock(), + response=mock_response, + ) + # raise_for_status is synchronous + mock_response.raise_for_status = MagicMock(side_effect=mock_http_error) + + # Configure the mock client instance + mock_client_instance = MockAsyncClient.return_value.__aenter__.return_value + mock_client_instance.get.return_value = mock_response + + expected_url = f'https://api.github.com/repos/{OWNER}/{REPO}/pulls/{PR_NUM}' + expected_headers = { + 'Accept': 'application/vnd.github.v3.diff', + 'Authorization': 'token test-token', # Use real token for assertion + 'X-GitHub-Api-Version': '2022-11-28', + } + + with pytest.raises(httpx.HTTPStatusError): + await github_handler.get_pr_diff(PR_NUM) + + # Assertions *after* the expected exception + mock_client_instance.get.assert_called_once_with( + expected_url, headers=expected_headers + ) + mock_response.raise_for_status.assert_called_once() # Verify raise_for_status was called + + +@pytest.mark.asyncio +@patch('httpx.AsyncClient') +async def test_post_review_single_comment(MockAsyncClient, github_handler): + """Tests posting a review with a single comment using AsyncClient.""" + mock_post_response = AsyncMock(spec=httpx.Response) + mock_post_response.status_code = 200 + mock_post_response.json.return_value = {'id': 1} + mock_post_response.raise_for_status = MagicMock() + + # Configure the mock client instance + mock_client_instance = MockAsyncClient.return_value.__aenter__.return_value + mock_client_instance.post.return_value = mock_post_response + + comments = [ReviewComment(path='file1.py', line=10, comment='First comment')] + await github_handler.post_review(pr_number=PR_NUM, comments=comments) + + expected_url = f'https://api.github.com/repos/{OWNER}/{REPO}/pulls/{PR_NUM}/reviews' + expected_headers = { + 'Accept': 'application/vnd.github.v3+json', + 'Authorization': 'token **********', + 'X-GitHub-Api-Version': '2022-11-28', + } + # Updated expected body based on refactored post_review logic + expected_payload = { + 'body': 'OpenHands AI Code Review:\n\n**Line-Specific Feedback:** (see comments below)', + 'event': 'COMMENT', + 'comments': [{'path': 'file1.py', 'line': 10, 'body': 'First comment'}], + } + + mock_client_instance.post.assert_called_once() + args, kwargs = mock_client_instance.post.call_args + assert args[0] == expected_url + assert kwargs['headers'] == expected_headers + assert kwargs['json'] == expected_payload + mock_post_response.raise_for_status.assert_called_once() + + +@pytest.mark.asyncio +@patch('httpx.AsyncClient') +async def test_post_review_multiple_comments(MockAsyncClient, github_handler): + """Tests posting a review with multiple comments using AsyncClient.""" + mock_post_response = AsyncMock(spec=httpx.Response) + mock_post_response.status_code = 200 + mock_post_response.json.return_value = {'id': 2} + mock_post_response.raise_for_status = MagicMock() + + mock_client_instance = MockAsyncClient.return_value.__aenter__.return_value + mock_client_instance.post.return_value = mock_post_response + + comments = [ + ReviewComment(path='file1.py', line=10, comment='First comment'), + ReviewComment(path='file2.py', line=25, comment='Second comment'), + ] + await github_handler.post_review(pr_number=PR_NUM, comments=comments) + + expected_url = f'https://api.github.com/repos/{OWNER}/{REPO}/pulls/{PR_NUM}/reviews' + expected_headers = { + 'Accept': 'application/vnd.github.v3+json', + 'Authorization': 'token **********', + 'X-GitHub-Api-Version': '2022-11-28', + } + # Updated expected body based on refactored post_review logic + expected_payload = { + 'body': 'OpenHands AI Code Review:\n\n**Line-Specific Feedback:** (see comments below)', + 'event': 'COMMENT', + 'comments': [ + {'path': 'file1.py', 'line': 10, 'body': 'First comment'}, + {'path': 'file2.py', 'line': 25, 'body': 'Second comment'}, + ], + } + + mock_client_instance.post.assert_called_once() + args, kwargs = mock_client_instance.post.call_args + assert args[0] == expected_url + assert kwargs['headers'] == expected_headers + assert kwargs['json'] == expected_payload + mock_post_response.raise_for_status.assert_called_once() + + +@pytest.mark.asyncio +@patch('httpx.AsyncClient') +async def test_post_review_no_comments(MockAsyncClient, github_handler): + """Tests posting a review with no comments using AsyncClient.""" + mock_post_response = AsyncMock(spec=httpx.Response) + mock_post_response.status_code = 200 + mock_post_response.raise_for_status = MagicMock() + + mock_client_instance = MockAsyncClient.return_value.__aenter__.return_value + mock_client_instance.post.return_value = mock_post_response + + await github_handler.post_review(pr_number=PR_NUM, comments=[]) + + expected_url = f'https://api.github.com/repos/{OWNER}/{REPO}/pulls/{PR_NUM}/reviews' + expected_headers = { + 'Accept': 'application/vnd.github.v3+json', + 'Authorization': 'token **********', + 'X-GitHub-Api-Version': '2022-11-28', + } + # Updated expected body based on refactored post_review logic + expected_payload = { + 'body': 'OpenHands AI Code Review:', + 'event': 'COMMENT', + 'comments': [], + } + + mock_client_instance.post.assert_called_once() + args, kwargs = mock_client_instance.post.call_args + assert args[0] == expected_url + assert kwargs['headers'] == expected_headers + assert kwargs['json'] == expected_payload + mock_post_response.raise_for_status.assert_called_once() + + +@pytest.mark.asyncio +@patch('httpx.AsyncClient') +async def test_post_review_api_error(MockAsyncClient, github_handler): + """Tests error handling when posting review fails using AsyncClient.""" + mock_post_response = AsyncMock(spec=httpx.Response) + mock_post_response.status_code = 400 + mock_post_response.request = MagicMock(url='dummy_url') + mock_post_response.json.return_value = {'message': 'Validation Failed'} + mock_http_error = httpx.HTTPStatusError( + 'API Error', request=mock_post_response.request, response=mock_post_response + ) + mock_post_response.raise_for_status = MagicMock(side_effect=mock_http_error) + + mock_client_instance = MockAsyncClient.return_value.__aenter__.return_value + mock_client_instance.post.return_value = mock_post_response + + comments = [ReviewComment(path='file1.py', line=10, comment='Error comment')] + with pytest.raises(httpx.HTTPStatusError): + await github_handler.post_review(pr_number=PR_NUM, comments=comments) + + # Verify post was called + mock_client_instance.post.assert_called_once() + # Verify raise_for_status was called on the response mock + mock_post_response.raise_for_status.assert_called_once() diff --git a/tests/unit/resolver/interfaces/test_gitlab.py b/tests/unit/resolver/interfaces/test_gitlab.py new file mode 100644 index 000000000000..c128126716bb --- /dev/null +++ b/tests/unit/resolver/interfaces/test_gitlab.py @@ -0,0 +1,256 @@ +from unittest.mock import AsyncMock, MagicMock, patch + +import httpx +import pytest +from pydantic import SecretStr + +from openhands.code_reviewer.reviewer_output import ReviewComment +from openhands.resolver.interfaces.gitlab import GitlabPRHandler + +OWNER = 'test-group/test-subgroup' # GitLab uses group/subgroup/project +REPO = 'test-repo' +PR_NUM = 456 # Use a different number for clarity +TOKEN = SecretStr('test-gitlab-token') +BASE_DOMAIN = 'gitlab.example.com' +BASE_URL = f'https://{BASE_DOMAIN}/api/v4/projects/{OWNER.replace("/", "%2F")}%2F{REPO}' + + +@pytest.fixture +def gitlab_handler(): + # Note: GitLab owner/repo structure might differ, adjust fixture if needed + return GitlabPRHandler(token=TOKEN, owner=OWNER, repo=REPO, base_domain=BASE_DOMAIN) + + +# ================================== get_pr_diff ================================== + + +@pytest.mark.asyncio +@patch('httpx.AsyncClient') +async def test_gitlab_get_pr_diff_success(MockAsyncClient, gitlab_handler): + """Tests successful retrieval of GitLab MR diff.""" + mock_response = AsyncMock(spec=httpx.Response) + mock_response.status_code = 200 + # GitLab diff endpoint returns a list of diff versions + mock_response.json.return_value = [{'diff': 'sample gitlab diff'}] + mock_response.raise_for_status = MagicMock() + + mock_client_instance = MockAsyncClient.return_value.__aenter__.return_value + mock_client_instance.get.return_value = mock_response + + diff = await gitlab_handler.get_pr_diff(PR_NUM) + + assert diff == 'sample gitlab diff' + expected_url = f'{BASE_URL}/merge_requests/{PR_NUM}/diffs' + expected_headers = { + 'Authorization': 'Bearer **********', # Expect masked token + 'Accept': 'application/json', + } + mock_client_instance.get.assert_called_once_with( + expected_url, headers=expected_headers + ) + mock_response.raise_for_status.assert_called_once() + + +@pytest.mark.asyncio +@patch('httpx.AsyncClient') +async def test_gitlab_get_pr_diff_no_diff_found(MockAsyncClient, gitlab_handler): + """Tests GitLab MR diff retrieval when the response is empty or lacks 'diff'.""" + mock_response = AsyncMock(spec=httpx.Response) + mock_response.status_code = 200 + mock_response.json.return_value = [] # Empty list + mock_response.raise_for_status = MagicMock() + + mock_client_instance = MockAsyncClient.return_value.__aenter__.return_value + mock_client_instance.get.return_value = mock_response + + diff = await gitlab_handler.get_pr_diff(PR_NUM) + assert diff == '' # Expect empty string + + # Test with missing 'diff' key + mock_response.json.return_value = [{'no_diff_key': 'something'}] + diff = await gitlab_handler.get_pr_diff(PR_NUM) + assert diff == '' + + +@pytest.mark.asyncio +@patch('httpx.AsyncClient') +async def test_gitlab_get_pr_diff_error(MockAsyncClient, gitlab_handler): + """Tests error handling when fetching GitLab MR diff fails.""" + mock_response = AsyncMock(spec=httpx.Response) + mock_response.status_code = 404 + mock_response.text = 'Not Found' + mock_http_error = httpx.HTTPStatusError( + 'Not Found', request=MagicMock(), response=mock_response + ) + mock_response.raise_for_status = MagicMock(side_effect=mock_http_error) + + mock_client_instance = MockAsyncClient.return_value.__aenter__.return_value + mock_client_instance.get.return_value = mock_response + + with pytest.raises(httpx.HTTPStatusError): + await gitlab_handler.get_pr_diff(PR_NUM) + + expected_url = f'{BASE_URL}/merge_requests/{PR_NUM}/diffs' + mock_client_instance.get.assert_called_once_with( + expected_url, headers=gitlab_handler.headers + ) + mock_response.raise_for_status.assert_called_once() + + +# ================================== post_review ================================== + + +@pytest.mark.asyncio +@patch('httpx.AsyncClient') +async def test_gitlab_post_review_success(MockAsyncClient, gitlab_handler): + """Tests successful posting of a review comment to GitLab MR.""" + # Mock response for fetching MR details (needed for position) + mock_get_response = AsyncMock(spec=httpx.Response) + mock_get_response.status_code = 200 + mock_get_response.json.return_value = { + 'iid': PR_NUM, + 'target_project_id': 12345, + 'diff_refs': { + 'base_sha': 'abc', + 'start_sha': 'def', + 'head_sha': 'ghi', + }, + } + mock_get_response.raise_for_status = MagicMock() + + # Mock response for posting the discussion + mock_post_response = AsyncMock(spec=httpx.Response) + mock_post_response.status_code = 201 # GitLab returns 201 Created + mock_post_response.json.return_value = {'id': 'discussion_id'} + mock_post_response.raise_for_status = MagicMock() + + mock_client_instance = MockAsyncClient.return_value.__aenter__.return_value + # Set up side effects for get (details) and post (comment) + mock_client_instance.get.return_value = mock_get_response + mock_client_instance.post.return_value = mock_post_response + + comments = [ReviewComment(path='src/main.py', line=50, comment='GitLab comment')] + await gitlab_handler.post_review(pr_number=PR_NUM, comments=comments) + + # Verify MR details were fetched + details_url = f'{BASE_URL}/merge_requests/{PR_NUM}' + mock_client_instance.get.assert_called_once_with( + details_url, headers=gitlab_handler.headers + ) + mock_get_response.raise_for_status.assert_called_once() + + # Verify discussion was posted + discussions_url = f'{BASE_URL}/merge_requests/{PR_NUM}/discussions' + expected_payload = { + 'body': 'GitLab comment', + 'position': { + 'position_type': 'text', + 'base_sha': 'abc', + 'start_sha': 'def', + 'head_sha': 'ghi', + 'new_path': 'src/main.py', + 'new_line': 50, + }, + } + mock_client_instance.post.assert_called_once_with( + discussions_url, headers=gitlab_handler.headers, json=expected_payload + ) + # Note: We don't check raise_for_status on post because the code only logs non-201 + + +@pytest.mark.asyncio +@patch('httpx.AsyncClient') +async def test_gitlab_post_review_no_comments(MockAsyncClient, gitlab_handler): + """Tests posting a review with no comments to GitLab MR (should not call API).""" + mock_client_instance = MockAsyncClient.return_value.__aenter__.return_value + + await gitlab_handler.post_review(pr_number=PR_NUM, comments=[]) + + # Assert that neither get (for details) nor post (for comment) was called + mock_client_instance.get.assert_not_called() + mock_client_instance.post.assert_not_called() + + +@pytest.mark.asyncio +@patch('httpx.AsyncClient') +async def test_gitlab_post_review_fetch_details_error(MockAsyncClient, gitlab_handler): + """Tests posting review when fetching MR details fails.""" + # Mock error response for fetching MR details + mock_get_response = AsyncMock(spec=httpx.Response) + mock_get_response.status_code = 404 + mock_get_error = httpx.HTTPStatusError( + 'Not Found', request=MagicMock(), response=mock_get_response + ) + mock_get_response.raise_for_status = MagicMock(side_effect=mock_get_error) + + # Mock success response for posting the discussion (will still be attempted) + mock_post_response = AsyncMock(spec=httpx.Response) + mock_post_response.status_code = 201 + mock_post_response.raise_for_status = MagicMock() + + mock_client_instance = MockAsyncClient.return_value.__aenter__.return_value + mock_client_instance.get.return_value = mock_get_response + mock_client_instance.post.return_value = mock_post_response + + comments = [ + ReviewComment( + path='src/main.py', line=50, comment='GitLab comment without position' + ) + ] + # The function currently logs the error and proceeds without position + await gitlab_handler.post_review(pr_number=PR_NUM, comments=comments) + + # Verify MR details fetch was attempted + details_url = f'{BASE_URL}/merge_requests/{PR_NUM}' + mock_client_instance.get.assert_called_once_with( + details_url, headers=gitlab_handler.headers + ) + mock_get_response.raise_for_status.assert_called_once() + + # Verify discussion was posted without position + discussions_url = f'{BASE_URL}/merge_requests/{PR_NUM}/discussions' + expected_payload = { + 'body': 'GitLab comment without position', + # No 'position' key + } + mock_client_instance.post.assert_called_once_with( + discussions_url, headers=gitlab_handler.headers, json=expected_payload + ) + + +@pytest.mark.asyncio +@patch('httpx.AsyncClient') +async def test_gitlab_post_review_post_comment_error(MockAsyncClient, gitlab_handler): + """Tests posting review when posting the comment itself fails.""" + # Mock success for fetching MR details + mock_get_response = AsyncMock(spec=httpx.Response) + mock_get_response.status_code = 200 + mock_get_response.json.return_value = { + 'iid': PR_NUM, + 'target_project_id': 12345, + 'diff_refs': {'base_sha': 'abc', 'start_sha': 'def', 'head_sha': 'ghi'}, + } + mock_get_response.raise_for_status = MagicMock() + + # Mock error for posting the discussion + mock_post_response = AsyncMock(spec=httpx.Response) + mock_post_response.status_code = 400 # Bad Request + mock_post_response.text = 'Invalid comment format' + # The code doesn't raise_for_status on post, it just logs non-201 + # So we don't need to mock raise_for_status here for the error case. + + mock_client_instance = MockAsyncClient.return_value.__aenter__.return_value + mock_client_instance.get.return_value = mock_get_response + mock_client_instance.post.return_value = mock_post_response + + comments = [ReviewComment(path='src/main.py', line=50, comment='Failed comment')] + # The function currently logs the error and continues + await gitlab_handler.post_review(pr_number=PR_NUM, comments=comments) + + # Verify MR details fetch + mock_client_instance.get.assert_called_once() + mock_get_response.raise_for_status.assert_called_once() + + # Verify discussion post attempt + mock_client_instance.post.assert_called_once() + # No raise_for_status check needed here based on current implementation