Add OpenHands agentic PR review - #28
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟢 Good taste — The workflow is narrowly triggered, uses least-privilege permissions, pins the OpenHands action and extension source to a verified commit older than seven days, and disables dependency caching. The repository-specific review guidance is focused and actionable.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM — This introduces a secret-backed, write-capable automation that executes a pinned third-party review action, but the maintainer-controlled trigger,pull_requestevent, immutable action pin, and disabled cache materially constrain exposure.
VERDICT:
✅ Worth merging: Core workflow and security boundaries are sound. GitHub Actions cannot submit an approving review, so this is posted as a non-blocking comment.
KEY INSIGHT:
The design keeps deeper automated review opt-in while avoiding fork-secret exposure and mutable action code.
Was this automated review useful? React with 👍 or 👎 to this review to help us measure review quality.
Workflow run: https://github.com/wende/macbeth/actions/runs/30500732836
There was a problem hiding this comment.
🟡 Needs improvement
First marked review; no prior marked findings to classify.
New findings
- The workflow does not implement the documented maintainer-controlled
openhands-reviewlabel trigger.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM — The workflow is secret-backed and write-capable; its actual broad automatic triggers make the control boundary differ from the description.
VERDICT:
❌ Needs rework: Align the event/filter with the documented explicit label trigger before merging.
KEY INSIGHT:
The trigger policy currently runs on ordinary same-repository PR activity rather than label opt-in.
Was this automated review useful? React with 👍 or 👎 to this review to help us measure review quality.
Workflow run: https://github.com/wende/macbeth/actions/runs/30501227982
There was a problem hiding this comment.
🟡 Needs improvement
Previous review follow-up
- Label-trigger contradiction — resolved. The previous finding flagged a contradiction between the documented
openhands-reviewlabel trigger and the actual broad event list. The PR description has been rewritten to describe the workflow as automatic, and the inline comment in.github/workflows/openhands-pr-review.yml(lines 4-6) now explicitly documents the same-repository-only / no-label design. The contradiction no longer exists.
New findings
- Missing
continue-on-error: trueis the main correctness gap (see inline).
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM — Secret-backed, write-capable, runs on every same-repo push including drafts; a missing advisory-mode flag means transient failures will block PRs (see inline).
VERDICT:
❌ Needs rework: Add continue-on-error: true so this advisory reviewer cannot block PRs on transient API failures.
KEY INSIGHT:
The skill file is sound, the action SHA is well-aged (8 days, past the 7-day rule), but the job must be marked advisory to match the MiniMax workflow's existing convention.
Was this automated review useful? React with 👍 or 👎 to this review to help us measure review quality.
Workflow run: https://github.com/wende/macbeth/actions/runs/30501380566
Superseded: the PR description now documents automatic review as intended, and the later OpenHands follow-up verified the finding as resolved.
There was a problem hiding this comment.
Previous review follow-up
- Label-trigger contradiction — resolved. Event list and inline comment now consistently describe the same-repository automatic trigger, with no
labeledevent or label filter. - Missing
continue-on-error— resolved. The job now carriescontinue-on-error: trueso a model or third-party failure cannot block the PR. - Missing
labeledevent / no label filter — resolved (intentional). Documented inline as same-repository automatic; no maintainer label is required.
New findings
- 🟠
issues: writeis unused —.github/workflows/openhands-pr-review.yml:13grantsissues: write, but the action only ever callsPOST/DELETE repos/.../pulls/.../reviews, which needspull-requests: write. The PR description explicitly states it grants only the permissions required to post reviews, so theissues: writeline is both unused and inconsistent with that claim. Drop it to keep the job at least privilege.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW — Pinning, fork-skip,continue-on-error, and concurrency are sound. The unusedissues: writescope widens the blast radius if the token is ever misused, but does not on its own create a new vulnerability.
VERDICT:
🟡 Acceptable — approve once the issues: write line is removed.
Was this automated review useful? React with 👍 or 👎 to this review to help us measure review quality.
Workflow run: https://github.com/wende/macbeth/actions/runs/30502554784
| pull-requests: write | ||
| issues: write |
There was a problem hiding this comment.
🟠 Important: issues: write is unused. The action only calls the pull-request reviews API (POST/DELETE repos/.../pulls/.../reviews), which is covered by pull-requests: write. The PR description claims it grants only the permissions required to post reviews, so this line is both unused and inconsistent with that claim — drop it to keep the job at least privilege.
| pull-requests: write | |
| issues: write | |
| pull-requests: write |
There was a problem hiding this comment.
Previous review follow-up
- Label-trigger contradiction — resolved. The event list does not include
labeled, and the inline comment now consistently describes the same-repository automatic trigger. - Missing
continue-on-error— resolved. The review job carriescontinue-on-error: true. - Unused
issues: write(line 13) — still present. Verified against the pinned action at1f5bd01: the only GitHub writes aregh api -X POST/DELETE repos/.../pulls/.../reviews[/...](action.yml permission probe) and the same endpoints fromagent_script.py; the GraphQL queries inagent_script.pyonly touch PR review threads, review comments, and the diff files endpoint; the agent is givenget_default_tools(no GitHub MCP server) andgrep -in 'issue' agent_script.pyreturns nothing. The PR description still claims the workflow "grants only the permissions required to post reviews", so this line is both unused and contradicts the stated least-privilege guarantee. Same finding as the existing unresolved thread, no new evidence to add.
New findings
None.
Was this automated review useful? React with 👍 or 👎 to this review to help us measure review quality.
Workflow run: https://github.com/wende/macbeth/actions/runs/30502848579
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- If the workflow only needs to comment on PRs, consider dropping
issues: writefrom the permissions block to minimize the token scope. - You can use the built-in
${{ github.token }}instead of${{ secrets.GITHUB_TOKEN }}for thegithub-tokeninput so the workflow doesn’t depend on an extra secret.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- If the workflow only needs to comment on PRs, consider dropping `issues: write` from the permissions block to minimize the token scope.
- You can use the built-in `${{ github.token }}` instead of `${{ secrets.GITHUB_TOKEN }}` for the `github-token` input so the workflow doesn’t depend on an extra secret.
## Individual Comments
### Comment 1
<location path=".github/workflows/openhands-pr-review.yml" line_range="10-13" />
<code_context>
+ pull_request:
+ types: [opened, reopened, ready_for_review, synchronize]
+
+permissions:
+ contents: read
+ pull-requests: write
+ issues: write
+
+concurrency:
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Consider narrowing workflow permissions to the minimum required (e.g., dropping `issues: write` if not needed).
This workflow requests `contents: read`, `pull-requests: write`, and `issues: write`. If the job never interacts with issues, remove `issues: write` to better enforce least privilege and limit the impact of a compromised action or dependency.
Suggested implementation:
```
permissions:
contents: read
pull-requests: write
```
Verify that no jobs or steps in this workflow (or called reusable workflows) create, modify, or comment on issues. If any issue-related operations exist elsewhere in the file, either reintroduce `issues: write` or refactor those steps to use pull request comments or other mechanisms that match the reduced permission set.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| permissions: | ||
| contents: read | ||
| pull-requests: write | ||
| issues: write |
There was a problem hiding this comment.
🚨 suggestion (security): Consider narrowing workflow permissions to the minimum required (e.g., dropping issues: write if not needed).
This workflow requests contents: read, pull-requests: write, and issues: write. If the job never interacts with issues, remove issues: write to better enforce least privilege and limit the impact of a compromised action or dependency.
Suggested implementation:
permissions:
contents: read
pull-requests: write
Verify that no jobs or steps in this workflow (or called reusable workflows) create, modify, or comment on issues. If any issue-related operations exist elsewhere in the file, either reintroduce issues: write or refactor those steps to use pull request comments or other mechanisms that match the reduced permission set.
There was a problem hiding this comment.
🤖 MiniMax review — MiniMax-M3
Verdict: ✅ Approve
Adds an AI-assisted PR review skill guide and a GitHub Actions workflow that invokes an LLM-based reviewer on same-repo PRs. The workflow follows security best practices (SHA-pinned action, scoped permissions, explicit fork-PR filter, concurrency cancel, timeout, continue-on-error for advisory behavior) and the skill file is well-targeted guidance for the project's TS/Swift/MCP stack.
There was a problem hiding this comment.
Previous review follow-up
- Label-trigger contradiction (line 8) — resolved. The event list is now
[opened, reopened, ready_for_review, synchronize]with nolabeledevent or label filter, and the inline comment consistently describes the same-repository automatic trigger. - Missing
continue-on-error(line 25) — resolved. The job now carriescontinue-on-error: true, matching the siblingminimax-pr-review.ymlconvention. - Unused
issues: write(line 13) — still present. Verified against the pinned action at1f5bd01be92de79719259e7ff96d01764ce0aa52(v1.0.0): the only GitHub writes arePOST/DELETE repos/.../pulls/.../reviewsdeclared inaction.yml, and the bundled OpenHands extensions GitHub surface is read-only (GraphQL for previous review context,GET /pulls/{n}/files). The action does not write issues, labels, or assignees. The PR description still claims the workflow grants only the permissions required to post reviews, so the line is both unused and inconsistent with that claim. The siblingminimax-pr-review.yml:9confirmspull-requests: writealone is sufficient — dropissues: writeto match the stated least-privilege intent.
New findings
None beyond the still-present issues: write line above. The pin to 1f5bd01b... is well-formed (tag 52da8beb points to the same commit, age/publisher within the same first-party organization as the reviewed repo so the 7-day waiting rule is intentionally excluded) and the pinned action's enable-uv-cache default is already 'false', so the loss of the explicit consumer override is intentional rather than a regression.
Risk assessment
- [Overall PR] 🟡 MEDIUM — unchanged from the previous review. The workflow is secret-backed, write-capable, and runs on every same-repository PR event (including drafts), but the trigger, advisory flag, and pinned action are otherwise sound. The remaining over-privilege is small but real: drop
issues: writeto bring the manifest in line with the description.
Verdict
❌ Needs rework — one line change (issues: write on line 13) to align the workflow with the PR description's stated least-privilege intent.
Was this automated review useful? React with 👍 or 👎 to this review to help us measure review quality.
Workflow run: https://github.com/wende/macbeth/actions/runs/30506104311
1 participant
What changed
wende/agentic-pr-reviewv1.0.0 action at its immutable release commitWhy
The existing prompt-only reviewer is constrained by supplied diff text. OpenHands checks out the PR repository and can inspect full files, callers, tests, and prior review threads with terminal and file tools before posting inline findings.
Reviews run automatically when a same-repository PR is opened or reopened, when a draft becomes ready, and on every new commit. Concurrent runs are grouped per PR so a new commit cancels an obsolete in-progress review.
Impact
Once this workflow is merged to the default branch, every same-repository PR receives a MiniMax M3 review automatically. Each rerun reads previous reviews and threads, verifies earlier findings against the current HEAD, summarizes what was resolved or remains, and adds only fitting new inline findings. The existing MiniMax workflow remains separate.
For security, this uses the
pull_requestevent. Repository secrets are therefore not exposed to fork PRs; reviews of fork PRs will need a separate trusted design if that becomes necessary.Validation
actionlint .github/workflows/openhands-pr-review.ymlgit diff --cached --checkMINIMAX_API_KEYexists in GitHub Actions secretswende/agentic-pr-review