Conversation
This commit addresses issue #93 by improving the PR review plugin documentation to achieve feature parity with the software-agent-sdk version. Key improvements: Documentation Enhancements: - Add comprehensive 'How It Works' section explaining the workflow - Document the composite action approach and its benefits - Add detailed explanation of skill overriding mechanism - Include reference example link to SDK's code-review skill Security Documentation: - Explain why pull_request_target is used - Document SDK secrets mitigation strategy with link to docs - Add security safeguards section with detailed explanations - Document caching strategy and its security implications - Add comment in action.yml explaining caching decision Example Reviews: - Add section showcasing 4 real PR reviews from SDK repo - Include table with PR links, descriptions, and highlights - Demonstrate inline comments, priority labeling, and feedback quality Troubleshooting: - Expand with detailed symptoms and solutions format - Add 'Review Taking Too Long' section - Add 'Agent Not Using Custom Skill' section - Improve existing sections with specific steps and code examples Related Resources: - Add comprehensive links section to extensions repo, SDK docs, and skills - Link to SDK secrets guide - Include OpenHands Cloud alternative The documentation now provides clearer guidance for: - Users migrating from software-agent-sdk - Teams customizing review guidelines - Troubleshooting common issues - Understanding security implications Closes #93 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.
Taste Rating: 🟡 Acceptable
This documentation work solves a real problem (issue #93) and takes a pragmatic approach. Good transparency on security, solid real-world examples, and helpful troubleshooting. However, missing concrete evidence of verification and one clarity gap around trigger behavior.
Verdict: ✅ Worth merging with the suggested improvements addressed.
Key Insight: The security transparency (acknowledging risks + explaining mitigations) is exactly right—documentation should be honest about tradeoffs, not pretend they don't exist.
| @@ -205,19 +237,32 @@ triggers: | |||
|
|
|||
| You are a code reviewer for this project. Follow these guidelines: | |||
There was a problem hiding this comment.
🟠 Important: You say the trigger "must use /codereview" but don't explain what happens if someone uses a different trigger. Does it silently fail? Use the default skill? Break the workflow?
Add one sentence: "If you use a different trigger name, the agent will not recognize your custom skill and will fall back to the default code-review behavior from the extensions repository."
|
|
||
| This workflow uses `pull_request_target` instead of `pull_request` so the code review agent can access secrets and post comments on PRs from forks. The workflow runs in the context of the base repository (not the fork), which provides access to repository secrets. | ||
|
|
||
| ### Security Safeguards | ||
|
|
||
| 1. **Trusted Contributors Only**: The workflow automatically runs for non-first-time contributors. First-time contributors require a maintainer to manually add the `review-this` label or request a review. | ||
|
|
||
| 2. **Explicit Checkout**: The PR code is explicitly checked out in a separate directory (`pr-repo/`) and is not automatically trusted. | ||
|
|
||
| 3. **No Credential Persistence**: The checkout uses `persist-credentials: false` to prevent git credentials from being stored. | ||
|
|
||
| 4. **SDK Secret Masking**: API keys are passed as SDK secrets rather than plain environment variables, which provides automatic masking in logs and prevents the agent from directly accessing these credentials during code execution. | ||
|
|
||
| ### Potential Risk | ||
|
|
||
| ⚠️ **Important**: A malicious contributor could submit a PR containing code designed to exfiltrate your `LLM_API_KEY` when the review agent analyzes their code. | ||
|
|
||
| **Mitigation**: The PR review workflow passes API keys as SDK secrets, which are: | ||
| - Automatically masked in all agent output and logs | ||
| - Not directly accessible as environment variables during agent execution | ||
| - Only used internally by the SDK for LLM API calls | ||
|
|
||
| For more details on how SDK secrets work, see the [SDK Secrets documentation](https://docs.openhands.dev/sdk/guides/secrets). |
There was a problem hiding this comment.
🟢 Acceptable: This security section is admirably transparent. You're acknowledging real risks (API key exfiltration) and explaining actual mitigations (SDK secrets), not hand-waving. This is exactly the right approach for security documentation—pragmatic honesty over theoretical perfection.
| ## Example Reviews | ||
|
|
||
| See real automated reviews in action from the OpenHands Software Agent SDK repository: | ||
|
|
||
| | PR | Description | Review Highlights | | ||
| |---|---|---| | ||
| | [#1927](https://github.com/OpenHands/software-agent-sdk/pull/1927#pullrequestreview-3767493657) | Composite GitHub Action refactor | Comprehensive review with 🔴 Critical, 🟠 Important, and 🟡 Suggestion labels | | ||
| | [#1916](https://github.com/OpenHands/software-agent-sdk/pull/1916#pullrequestreview-3758297071) | Add example for reconstructing messages | Critical issues flagged with clear explanations | | ||
| | [#1904](https://github.com/OpenHands/software-agent-sdk/pull/1904#pullrequestreview-3751821740) | Update code-review skill guidelines | APPROVED review highlighting key strengths | | ||
| | [#1889](https://github.com/OpenHands/software-agent-sdk/pull/1889#pullrequestreview-3747576245) | Fix tmux race condition | Technical review of concurrency fix with dual-lock strategy analysis | | ||
|
|
||
| These examples demonstrate: | ||
| - **Inline comments** on specific lines of code | ||
| - **Priority labeling** (🔴 Critical, 🟠 Important, 🟡 Suggestion, 🟢 Nit) | ||
| - **Actionable feedback** with code examples and suggestions | ||
| - **Context-aware analysis** considering the broader codebase | ||
|
|
||
| ## Troubleshooting |
There was a problem hiding this comment.
🟢 Acceptable: Real examples with real PRs. This is exactly what users need—concrete proof of what the tool actually produces, not theoretical marketing claims. Well done.
| **Solutions**: | ||
| 1. **Check workflow file location**: Ensure the workflow file is in `.github/workflows/` directory | ||
| 2. **Verify PR author association**: First-time contributors (`FIRST_TIME_CONTRIBUTOR` or `NONE` association) require manual trigger via label or reviewer request for security | ||
| 3. **Check secrets**: Verify `LLM_API_KEY` is set in repository settings under **Settings → Secrets and variables → Actions** | ||
| 4. **Review trigger conditions**: Check the `if:` condition in your workflow matches your intended trigger events | ||
| 5. **Check Actions tab**: Look for workflow run errors in the **Actions** tab of your repository | ||
|
|
||
| ### Review Comments Not Appearing | ||
|
|
||
| 1. Check the `GITHUB_TOKEN` has write permissions for pull requests | ||
| 2. Review the workflow logs for API errors | ||
| 3. Verify the LLM API key is valid | ||
| **Symptoms**: Workflow runs successfully but no comments appear on the PR | ||
|
|
||
| **Solutions**: | ||
| 1. **Check permissions**: Ensure your workflow has these permissions: | ||
| ```yaml | ||
| permissions: | ||
| contents: read | ||
| pull-requests: write | ||
| issues: write | ||
| ``` | ||
| 2. **Verify GITHUB_TOKEN**: For fork PRs, ensure you're using `pull_request_target` (not `pull_request`) | ||
| 3. **Check workflow logs**: Look for GitHub API errors in the workflow execution logs | ||
| 4. **Validate LLM API key**: Test that your `LLM_API_KEY` is valid and has available quota | ||
| 5. **Review rate limits**: Check if you've hit GitHub API rate limits (rare with default tokens) | ||
|
|
||
| ### Review Taking Too Long | ||
|
|
||
| **Symptoms**: Review runs for more than 5-10 minutes | ||
|
|
||
| **Solutions**: | ||
| 1. **Large PRs**: PRs with many files or large diffs take longer to analyze | ||
| - Consider splitting large PRs into smaller, focused changes | ||
| - The agent truncates very large diffs (>100,000 characters) automatically | ||
| 2. **LLM API delays**: Check if your LLM provider is experiencing slowdowns | ||
| 3. **Check concurrency**: The workflow uses concurrency control to cancel previous runs when new commits are pushed | ||
|
|
||
| ### Rate Limiting | ||
|
|
There was a problem hiding this comment.
🟡 Suggestion: This troubleshooting expansion is very detailed. Question: Are these all problems users have actually reported, or are some theoretical? If they're real support tickets, great. If you're solving imaginary problems, trim the imaginary ones.
Specifically, "Review Taking Too Long" feels like you're solving a problem that might not exist yet. Keep it if users have complained; cut it if they haven't.
3 participants
This PR addresses #93 by improving the PR review plugin documentation to close the implementation gap with software-agent-sdk.
Summary
The extensions repo already has the core PR review functionality migrated from software-agent-sdk. This PR focuses on closing the documentation gap to achieve feature parity with the SDK's comprehensive docs.
Changes
Documentation Enhancements
Security Documentation
Example Reviews
Troubleshooting
Related Resources
What This Achieves
After this PR, the extensions repo will have:
Testing
Related Issues
Closes #93
Note: This is primarily a documentation improvement. The core functionality is already fully migrated and working. The code changes are minimal (just a comment in action.yml explaining caching).