Conversation
* Add GitHub App auth option for PR review bot * docs: mention required GitHub permissions for auth --------- Co-authored-by: openhands <openhands@all-hands.dev>
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🔴 Needs rework - Critical issue found: production workflow references personal fork instead of official repo. Authentication logic looks solid but lacks error handling and tests.
| steps: | ||
| - name: Run PR Review | ||
| uses: OpenHands/software-agent-sdk/.github/actions/pr-review@main | ||
| uses: enyst/agent-sdk/.github/actions/pr-review@main |
There was a problem hiding this comment.
🔴 Critical: Production workflow is pointing to a personal fork (enyst/agent-sdk) instead of the official repo. This needs to reference OpenHands/software-agent-sdk/.github/actions/pr-review@main or this is a major security/reliability issue.
| uses: enyst/agent-sdk/.github/actions/pr-review@main | |
| uses: OpenHands/software-agent-sdk/.github/actions/pr-review@main |
| LLM_API_KEY: ${{ inputs.llm-api-key }} | ||
| GITHUB_TOKEN: ${{ inputs.github-token }} | ||
| GITHUB_TOKEN: ${{ steps.github-app-token.outputs.token || inputs.github-token }} | ||
| run: | |
There was a problem hiding this comment.
🟠 Important: Silent fallback could hide configuration errors. If someone provides GitHub App credentials but token generation fails, they won't know. Consider adding explicit validation:
if [ -n "${{ inputs.github-app-id }}" ] && [ -z "$GITHUB_TOKEN" ]; then
echo "Error: GitHub App credentials provided but token generation failed."
exit 1
fi| github.event.label.name == 'review-this' || | ||
| github.event.requested_reviewer.login == 'openhands-agent' || | ||
| github.event.requested_reviewer.login == 'openhands-reviewer' || | ||
| github.event.requested_reviewer.login == 'openhands-reviewer[bot]' || |
There was a problem hiding this comment.
🟡 Suggestion: Adding multiple reviewer triggers is good, but this conditional is getting complex. Consider documenting why both openhands-reviewer and openhands-reviewer[bot] are needed (presumably one for manual requests, one for automatic assignments?).
| llm-api-key: ${{ secrets.LLM_API_KEY }} | ||
| # Authentication options: | ||
| # - Preferred: GitHub App installation token (created inside the composite action) | ||
| # - Fallback: all-hands-bot PAT (existing behavior) |
There was a problem hiding this comment.
🟡 Suggestion: The comment says "Preferred: GitHub App" but you still provide both. If App auth is truly preferred and available, you could make the PAT conditional. As-is, you're always passing both and relying on the action's internal fallback logic. This works but might not be what you intended.
2 participants
Summary
[fill in a summary of this PR]
Checklist