feat(code-reviewer): Implement async calls and tests #13
Conversation
This reverts commit dad2b2f.
hamiltop
force-pushed
the
feat/code-reviewer-impl
branch
from
215a05e to
dc48f12
Compare
hamiltop
force-pushed
the
feat/code-reviewer-impl
branch
from
72f0416 to
225c940
Compare
This reverts commit 4a4e1e8.
…e review"" This reverts commit e5b2236.
Modify RetryMixin to set temperature to 2.0 (instead of 1.0) when retrying an LLM call after an LLMNoResponseError if the original temperature was 0.
Update the message sent by `handle_awaiting_input` to explicitly instruct the agent to use the `finish` tool with the JSON review comments, rather than just sending "FINISH". This aims to prevent the agent from misinterpreting the message as user input and looping.
Based on agent feedback that calculating line numbers from the diff alone is unreliable, remove instructions telling the agent to do so. Reinforce the requirement to determine and verify line numbers using `cat` and `grep -n` on the checked-out files in /workspace.
Strengthen the prompt instructions to state that line numbers MUST be determined *exclusively* by reading the file content in /workspace using `cat` and `grep -n`. Explicitly forbid using the `git diff` output for determining line numbers. Update the example `line_number_justification` to remove reference to the diff hunk.
1 similar comment
![phineas-remind101-sandbox[bot]](https://avatars.githubusercontent.com/in/1216457?s=60&v=4){kind=small}
There was a problem hiding this comment.
OpenHands AI Code Review:
General Feedback:
- The test suite has been updated effectively to cover the new asynchronous methods (
test_get_pr_diff_success_async,test_post_review_async, etc.) usingpytest.mark.asyncio,unittest.mock.AsyncMock, and patchinghttpx.AsyncClient. Tests cover success cases, edge cases (like no comments), and error handling scenarios. - New tests added for the GitLab interface's async functions (
test_gitlab_get_pr_diff_success,test_gitlab_post_review_success). These tests correctly usepytest.mark.asyncioand mockhttpx.AsyncClientto simulate API interactions, including fetching MR details and posting discussions (comments). The tests accurately reflect the GitLab API specifics, like expecting a 201 status for comment creation and handling positional comments. - This new workflow automates PR reviews using OpenHands. It's well-structured with clear inputs, secrets, triggers, and permissions. The logic for determining the authentication token (App/PAT/Default) and selecting the OpenHands installation source (release branch vs experimental) based on triggers is robust. Environment variables are correctly set up for the
review_prscript. Adding steps to dump Docker logs on failure and check the review result enhances debuggability. Consider explicitly defining the permissions needed for thedockercommands if they require more than the default.
Line-Specific Feedback: (see comments below)
| logger.error(f'Error fetching diff for PR #{pr_number}: {e}') | ||
| raise # Re-raise other exceptions | ||
|
|
||
| async def post_review(self, pr_number: int, comments: list[ReviewComment]) -> None: |
There was a problem hiding this comment.
The post_review function is now asynchronous, using httpx.AsyncClient for making API calls. It intelligently bundles line-specific comments and general feedback into a single review submission, which is efficient. Error handling with try...except blocks and logging for HTTPStatusError and other exceptions are good additions.
| thread_ids, | ||
| ) | ||
|
|
||
| async def get_pr_diff(self, pr_number: int) -> str: |
There was a problem hiding this comment.
Successfully converted get_pr_diff to an async function using httpx.AsyncClient. The specific 'Accept': 'application/vnd.github.v3.diff' header is correctly used. Error handling and logging for potential HTTPStatusError have been added, ensuring robustness.
| ) | ||
| return stdout.decode(), stderr.decode() | ||
|
|
||
| async def checkout_pr(self, pr_number: int, repo_dir: str): |
There was a problem hiding this comment.
The checkout_pr function is now asynchronous. It correctly fetches PR details using httpx.AsyncClient and uses a new async helper _run_git_command to execute git commands (clone, fetch, checkout) asynchronously. This improves performance by not blocking the event loop during I/O operations.
|
|
||
| return converted_issues | ||
|
|
||
| async def post_review(self, pr_number: int, comments: list[ReviewComment]) -> None: |
There was a problem hiding this comment.
This function is now async. It first fetches MR details asynchronously to get diff SHAs required for positional comments. Then, it iterates through comments and posts each one as a separate discussion using httpx.AsyncClient. While async, posting comments individually might be less efficient than batching if GitLab supported it. Good error handling is included for both fetching details and posting comments.
| ) | ||
| # Consider raising | ||
|
|
||
| async def get_pr_diff(self, pr_number: int) -> str: |
There was a problem hiding this comment.
Converted get_pr_diff to async using httpx.AsyncClient. It correctly targets the GitLab diffs endpoint and extracts the diff content from the response list. Includes appropriate error handling and logging, raising exceptions after logging them for upstream handling.
- Make line, code_snippet, line_number_justification mandatory unless review_level=pr - Add instruction to only use finish tool for output
- Fetch PR diff and head commit SHA before posting review. - Map head commit line numbers to unified diff positions. - Use position and commit_id instead of line in GitHub API call. - Handle cases where line mapping fails by adding to general comments. - Adds get_pr_head_commit method to GithubPRHandler.
There was a problem hiding this comment.
OpenHands AI Code Review:\n\nGeneral Feedback / Unpositioned Comments:\n- (Unpositioned) /workspace/.github/workflows/openhands-code-reviewer.yml:150: The workflow installs openhands-ai directly from a specific feature branch (feat/code-reviewer-impl) on a fork (remind101/OpenHands). While suitable for testing this PR, this should be updated to point to a stable release tag or the main repository's development/stable branch before this workflow is widely adopted or merged into a stable branch.\n- (Unpositioned) /workspace/.github/workflows/openhands-code-reviewer.yml:316: The filename review_output_${{ env.PR_NUMBER }}.jsonl is specified here for the --output-file argument, implying JSON Lines format. However, the invoked script review_pr.py writes standard JSON. Consider changing the filename extension to .json in this workflow step and subsequent steps (like artifact upload and the post_review_comments script call) for clarity and consistency with the actual file content.\n- (Unpositioned) /workspace/openhands/code_reviewer/post_review_comments.py:123: The help text for the --output-file argument claims the expected file is .jsonl. However, the script correctly uses json.loads which parses standard JSON, consistent with the output of review_pr.py. Update the help text to refer to .json instead of .jsonl.\n\n
2 participants
End-user friendly description of the problem this fixes or functionality that this introduces.
Give a summary of what the PR does, explaining any non-trivial design decisions.
Link of any specific issues this addresses.