Skip to content
8 changes: 4 additions & 4 deletions .github/workflows/pr-review-by-openhands.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@ on:
# 2. A draft PR is marked as ready for review, OR
# 3. A maintainer adds the 'review-this' label, OR
# 4. A maintainer requests openhands-agent or all-hands-bot as a reviewer
# Only users with write access can add labels or request reviews, ensuring security.
# Adding labels and requesting reviewers requires write access.
# The PR code is explicitly checked out for review, but secrets are only accessible
# because the workflow runs in the base repository context
# because the workflow runs in the base repository context.
pull_request_target:
types: [opened, ready_for_review, labeled, review_requested]

Expand All @@ -26,7 +26,7 @@ jobs:
# 2. A draft PR is converted to ready for review by a non-first-time contributor, OR
# 3. 'review-this' label is added, OR
# 4. openhands-agent or all-hands-bot is requested as a reviewer
# Note: FIRST_TIME_CONTRIBUTOR PRs require manual trigger via label/reviewer request
# Note: FIRST_TIME_CONTRIBUTOR and NONE PRs require manual trigger via label/reviewer request.
if: |
(github.event.action == 'opened' && github.event.pull_request.draft == false && github.event.pull_request.author_association != 'FIRST_TIME_CONTRIBUTOR' && github.event.pull_request.author_association != 'NONE') ||
(github.event.action == 'ready_for_review' && github.event.pull_request.author_association != 'FIRST_TIME_CONTRIBUTOR' && github.event.pull_request.author_association != 'NONE') ||
Expand All @@ -39,7 +39,7 @@ jobs:
runs-on: ubuntu-24.04
steps:
- name: Run PR Review
uses: OpenHands/software-agent-sdk/.github/actions/pr-review@main
uses: OpenHands/extensions/plugins/pr-review@main
with:
llm-model: litellm_proxy/claude-sonnet-4-5-20250929
llm-base-url: https://llm-proxy.app.all-hands.dev
Expand Down
85 changes: 85 additions & 0 deletions .github/workflows/pr-review-evaluation.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
---
name: PR Review Evaluation

# This workflow evaluates how well PR review comments were addressed.
# It runs when a PR is closed to assess review effectiveness.
#
# Security note: pull_request_target is safe here because:
# 1. Only triggers on PR close (not on code changes)
# 2. Does not checkout PR code - only downloads artifacts from trusted workflow runs
# 3. Runs evaluation scripts from the extensions repo, not from the PR

on:
pull_request_target:
types: [closed]

permissions:
contents: read
pull-requests: read

jobs:
evaluate:
runs-on: ubuntu-24.04
env:
PR_NUMBER: ${{ github.event.pull_request.number }}
REPO_NAME: ${{ github.repository }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion - Unused Variable: PR_MERGED is captured but never used in the workflow. Either use it to conditionally run evaluation (line 69-76), or remove it.

If you want to evaluate all closed PRs regardless of merge status, this is fine and you can ignore this comment. But if evaluation is only meaningful for merged PRs, add a condition.

PR_MERGED: ${{ github.event.pull_request.merged }}

steps:
- name: Download review trace artifact
id: download-trace
uses: dawidd6/action-download-artifact@v6
continue-on-error: true
with:
workflow: pr-review-by-openhands.yml
name: pr-review-trace-${{ github.event.pull_request.number }}
path: trace-info
search_artifacts: true
if_no_artifact_found: warn

- name: Check if trace file exists
id: check-trace
run: |
if [ -f "trace-info/laminar_trace_info.json" ]; then
echo "trace_exists=true" >> $GITHUB_OUTPUT
echo "Found trace file for PR #$PR_NUMBER"
else
echo "trace_exists=false" >> $GITHUB_OUTPUT
echo "No trace file found for PR #$PR_NUMBER - skipping evaluation"
fi

# Always checkout main branch for security - cannot test script changes in PRs
- name: Checkout extensions repository
if: steps.check-trace.outputs.trace_exists == 'true'
uses: actions/checkout@v5
with:
repository: OpenHands/extensions
path: extensions

- name: Set up Python
if: steps.check-trace.outputs.trace_exists == 'true'
uses: actions/setup-python@v6
with:
python-version: '3.12'

- name: Install dependencies
if: steps.check-trace.outputs.trace_exists == 'true'
run: pip install lmnr

- name: Run evaluation
if: steps.check-trace.outputs.trace_exists == 'true'
env:
# Script expects LMNR_PROJECT_API_KEY; org secret is named LMNR_SKILLS_API_KEY
LMNR_PROJECT_API_KEY: ${{ secrets.LMNR_SKILLS_API_KEY }}
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
run: |
python extensions/plugins/pr-review/scripts/evaluate_review.py \
--trace-file trace-info/laminar_trace_info.json

- name: Upload evaluation logs
uses: actions/upload-artifact@v5
if: always() && steps.check-trace.outputs.trace_exists == 'true'
with:
name: pr-review-evaluation-${{ github.event.pull_request.number }}
path: '*.log'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion - Implicit Contract: The *.log pattern assumes the evaluation script creates .log files in the working directory. This is fragile - if the script changes its logging behavior or location, artifacts silently disappear.

Consider either:

  1. Document this contract in a comment
  2. Have the script output a known filename
  3. Upload the entire working directory or a specific subdirectory

retention-days: 30
Loading