-
Notifications
You must be signed in to change notification settings - Fork 12
Migrate PR review workflow to use extensions action#347
Migrate PR review workflow to use extensions action #347
Conversation
This PR updates the PR review workflow to use the composite action from OpenHands/extensions instead of OpenHands/software-agent-sdk. Changes: - Updated action reference from software-agent-sdk to extensions/plugins/pr-review NOTE: This PR should be merged AFTER OpenHands/extensions#54 is merged to ensure the action is available. Co-authored-by: openhands <openhands@all-hands.dev>
b81beb1 to
cd2910d
Compare
@all-hands-bot
all-hands-bot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Acceptable - Refactor is straightforward, but merge ordering creates unnecessary risk.
Verdict: The path change itself is fine, but this creates a landmine if merged before the dependency is ready. See inline comments.
@all-hands-bot
all-hands-bot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Acceptable - Trivial path change, but test it before merging.
Verdict: The change is fine if the path is correct. Since the dependency is merged, stop theorizing and just test it.
This adds the evaluation workflow that runs when PRs are closed to assess how well review comments were addressed. Uses OpenHands/extensions plugin. Co-authored-by: openhands <openhands@all-hands.dev>
@all-hands-bot
all-hands-bot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Acceptable - Path change is straightforward, but the new evaluation workflow adds significant untested complexity with multiple external dependencies.
Replace full evaluation workflow with minimal stub that calls OpenHands/extensions/.github/workflows/pr-review-evaluation-reusable.yml This centralizes evaluation logic in the extensions repo for easier maintenance. Co-authored-by: openhands <openhands@all-hands.dev>
Address security review feedback - only pass the specific secrets needed. Co-authored-by: openhands <openhands@all-hands.dev>
Simplify by having the complete workflow directly instead of calling a reusable workflow. Still references extensions repo for the evaluation script. Co-authored-by: openhands <openhands@all-hands.dev>
- Pass trace file path directly to script instead of copying - Only upload *.log files to avoid redundant artifact uploads Co-authored-by: openhands <openhands@all-hands.dev>
Ensure pr-review-by-openhands.yml and pr-review-evaluation.yml match the canonical versions in OpenHands/extensions. Co-authored-by: openhands <openhands@all-hands.dev>
@all-hands-bot
all-hands-bot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Acceptable - Path change is trivial, but the evaluation workflow introduces stability risks.
Only run evaluation for merged PRs, not closed-but-not-merged ones. Co-authored-by: openhands <openhands@all-hands.dev>
Closed-but-not-merged PRs are valuable for evaluation: if the review identified critical issues causing closure, that's evidence of effective review. Co-authored-by: openhands <openhands@all-hands.dev>
@all-hands-bot
all-hands-bot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Acceptable - Migration is solid, evaluation workflow has minor pragmatic issues.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- Document this contract in a comment
- Have the script output a known filename
- Upload the entire working directory or a specific subdirectory
Uh oh!
There was an error while loading. Please reload this page.
Summary
Migrate the PR review workflow to use the centralized action from
OpenHands/extensions.Changes
OpenHands/extensions/plugins/pr-review@maincomposite actionBenefits
Testing
The action has been tested on the extensions repo itself via PR #61.