Review a contribution PR for safety, quality, and readiness. Checks for security concerns, test coverage, size appropriateness, and intent alignment. Use when reviewing external contributions.
66
81%
Does it follow best practices?
Run evals on this skill
Adds up to 20 points to the overall score
View guide
Low
Low-risk findings worth noting
Review PR #$ARGUMENTS from external contributor for safety, quality, and readiness.
PR Metadata:
!`gh pr view $ARGUMENTS --repo homeassistant-ai/ha-mcp --json author,additions,deletions,files,commits,closingIssuesReferences,isDraft,reviews,url,title,body`Contributor Stats:
!`gh api /repos/homeassistant-ai/ha-mcp/pulls/$ARGUMENTS --jq '{author: .user.login, user_id: .user.id}' | jq -r '.author' | xargs -I {} gh api /repos/homeassistant-ai/ha-mcp/contributors --jq '.[] | select(.login == "{}") | {login: .login, contributions: .contributions}'`Files Changed:
!`gh api /repos/homeassistant-ai/ha-mcp/pulls/$ARGUMENTS/files --jq '.[] | {filename: .filename, status: .status, additions: .additions, deletions: .deletions, changes: .changes, patch: .patch}' | head -50`Note: Codex reviews PRs automatically (posts as chatgpt-codex-connector[bot]). Check if Codex flagged any security concerns.
# Check Codex's reviews and any security-related comments.
# Codex findings can be inline-only, so also fetch the pull review comments
# endpoint — review bodies and conversation comments alone can miss them.
gh api /repos/homeassistant-ai/ha-mcp/pulls/$ARGUMENTS/reviews --jq '.[] | select(.user.login == "chatgpt-codex-connector[bot]") | {state: .state, body: .body}'
gh api /repos/homeassistant-ai/ha-mcp/pulls/$ARGUMENTS/comments --jq '.[] | select(.user.login == "chatgpt-codex-connector[bot]") | {path: .path, line: .line, body: .body}'
gh pr view $ARGUMENTS --repo homeassistant-ai/ha-mcp --json comments --jq '.comments[] | select(.body | contains("security") or contains("Security")) | {author: .author.login, body: .body}'If Codex flagged security issues:
If NO Codex security flags but you notice concerning patterns:
.github/ workflow modifications with pull_request_target.claude/ agent/skill changes that could affect behaviorIf security assessment passes and PR has workflow changes or new workflows:
# Check current workflow status
gh api /repos/homeassistant-ai/ha-mcp/pulls/$ARGUMENTS/requested_reviewers
# Enable workflows if not enabled (requires WRITE permission)
# This command may fail if already enabled - that's OK
gh api -X PUT /repos/homeassistant-ai/ha-mcp/actions/workflows/pr.yml/enable 2>/dev/null || echo "Workflows already enabled or no permission"Pre-existing tests (easier review if modified code is already tested):
# For each modified source file, check if tests exist
gh api /repos/homeassistant-ai/ha-mcp/pulls/$ARGUMENTS/files --jq '.[] | select(.filename | startswith("src/")) | .filename' | while read file; do
basename=$(basename "$file" .py)
echo "Checking tests for: $file"
# Method 1: Look for test files by naming convention
find tests/ -name "test_${basename}.py" -o -name "test_*${basename}*.py" 2>/dev/null | head -3
# Method 2: Grep for function/class names from the modified file
# Extract function/class names and search for them in tests
grep -E '^(def|class|async def) [a-zA-Z_]' "$file" 2>/dev/null | head -5 | while read line; do
name=$(echo "$line" | sed -E 's/.*(def|class) ([a-zA-Z_][a-zA-Z0-9_]*).*/\2/')
if [ -n "$name" ]; then
grep -r "$name" tests/ 2>/dev/null | head -1
fi
done
doneNew tests added:
# Check if PR adds or modifies tests
gh api /repos/homeassistant-ai/ha-mcp/pulls/$ARGUMENTS/files --jq '.[] | select(.filename | startswith("tests/")) | {filename: .filename, status: .status, additions: .additions}'Output Test Summary:
🧪 Test Coverage:
- Pre-existing tests: ✅ Modified code has tests / ⚠️ No tests for modified code
- New tests: ✅ PR adds X test files / ⚠️ No new tests
- Assessment: [Easy/Medium/Hard to review based on test coverage]Calculate PR size and assess appropriateness:
# From metadata: additions + deletions
total_lines=$(gh pr view $ARGUMENTS --repo homeassistant-ai/ha-mcp --json additions,deletions --jq '.additions + .deletions')
echo "Total lines changed: $total_lines"
# Get contributor experience
author=$(gh pr view $ARGUMENTS --repo homeassistant-ai/ha-mcp --json author --jq -r '.author.login')
# Check 1: Contributions to this project
project_contributions=$(gh api /repos/homeassistant-ai/ha-mcp/contributors --jq ".[] | select(.login == \"$author\") | .contributions" || echo "0")
# Check 2: Total GitHub commits (overall experience)
total_commits=$(gh api /users/$author --jq '.public_repos + .total_private_repos' 2>/dev/null || echo "unknown")
echo "Contributor: $author"
echo "Project contributions: $project_contributions"
echo "GitHub experience: $total_commits repos"Assess:
First-time to project (0-2 project contributions):
500 lines: 🔴 Too large - suggest splitting
Regular contributor (3+ project contributions):
1000 lines: 🔴 Very large - suggest splitting
Experienced GitHub user (many repos/commits overall):
Output Size Summary:
📏 PR Size:
- Lines changed: [total]
- Contributor: [first-time / regular] ([X] contributions)
- Assessment: [size appropriateness]Check linked issues:
# From metadata: closingIssuesReferences
gh pr view $ARGUMENTS --repo homeassistant-ai/ha-mcp --json closingIssuesReferences --jq '.closingIssuesReferences[] | {number: .number, title: .title}'If issue linked:
If no issue linked:
Output Intent Summary:
🎯 Intent & Linkage:
- Linked issue: #X "title" / ⚠️ No issue linked
- Solves issue: ✅ Fully addresses requirements / ⚠️ Partial / ❌ Doesn't match
- Scope: ✅ Focused / ⚠️ Scope creep detectedNote: Codex provides automated code review on all PRs. This step focuses on what Codex cannot assess:
Breaking change assessment:
Quick checks:
# Check if ruff/mypy would complain (from workflow logs if available)
gh pr checks $ARGUMENTS --repo homeassistant-ai/ha-mcp | grep -E "(ruff|mypy|lint)"
# Check for common issues in diff
grep -E "(TODO|FIXME|XXX|HACK)" /tmp/pr_$ARGUMENTS.diffOutput Quality Summary:
✨ Code Quality:
- Architecture fit: [assessment - service layer, context engineering]
- Breaking changes: ✅ None / ⚠️ Detected - [describe what's genuinely lost]
- Codex reviews: [check if Codex flagged anything critical]After completing all steps, present a short summary of what the PR does and the review findings, then ask: "Should I post this comment to the PR?"
After completing the analysis, draft a comment for the PR following these guidelines:
Comment Length:
Style:
Structure for "Good to Merge" (10-15 lines):
[Positive opening line about the contribution]
[1-2 sentences on what works well - focus on functionality, tests, architecture]
[Any minor suggestions or notes - optional, technical only]
[Closing line about readiness to merge]Note: Do NOT mention security assessment in comment unless issues were found. Security checks are internal.
Structure for "Changes Needed" (max 25 lines):
[Positive opening line acknowledging the work]
[Brief summary of the issue being solved]
**[Concern 1]:**
[1-2 lines explanation + suggestion - focus on: tests, functionality, architecture, breaking changes]
**[Concern 2]:** (if applicable)
[1-2 lines explanation + suggestion]
**[Concern 3]:** (if applicable)
[1-2 lines explanation + suggestion]
[Closing line about next steps]Note: Security concerns should be raised immediately when found, not in final structured comment.
Example - Good to Merge:
Great work on [feature/fix]. [Performance/quality metric] is impressive.
The implementation follows existing patterns and the [specific aspect] is well-designed. [Optional: Minor note about something noticed].
Ready to merge once CI passes.Example - Changes Needed:
Thanks for tackling [problem]. [Metric/impact] shows this addresses a real need.
**Test coverage:**
Missing tests for the new [feature]. Please add at least one E2E test validating [behavior]. Performance tests not required.
**[Second concern if applicable]:**
[Brief explanation and request]
Once [change 1] and [change 2] are addressed, this should be good to merge.01964cc
If you maintain this skill, you can claim it as your own. Once claimed, you can manage eval scenarios, bundle related skills, attach documentation or rules, and ensure cross-agent compatibility.