fedora-csb-system-manager

ReviewPR Workflow

Review a GitHub pull request with comprehensive checks, diff analysis, and approval/feedback workflow.

When to Use

  • “review pr”
  • “review pull request”
  • “check pr changes”
  • “approve pr”
  • “request changes on pr”

Quick Commands

View PR

# View PR summary
gh pr view <number>

# View in browser
gh pr view <number> --web

# View specific fields
gh pr view <number> --json title,body,state,author,reviews

Review PR

# Approve
gh pr review <number> --approve

# Request changes
gh pr review <number> --request-changes --body "Feedback here"

# Comment only
gh pr review <number> --comment --body "Looks good, minor suggestions"

Checkout and Test

# Checkout PR branch locally
gh pr checkout <number>

# Run tests
npm test  # or your test command

# Return to previous branch
git checkout -

Workflow Steps

1. Get PR Information

# Fetch comprehensive PR data
gh pr view <number> --json \
  number,title,body,state,author,headRefName,baseRefName,\
  mergeable,statusCheckRollup,reviews,commits

# Store for analysis

Present summary:

PR #123: Add user authentication
Author: @username
Branch: feature/auth → main
Status: OPEN
Mergeable: TRUE

Commits: 5
Files changed: 12
Reviews: 1 approval, 0 changes requested

2. Check CI/CD Status

# Get check status
gh pr checks <number>

# Detailed status
gh pr view <number> --json statusCheckRollup --jq '
  .statusCheckRollup |
  group_by(.conclusion) |
  map({status: .[0].conclusion, count: length})
'

Present status:

Checks: ✓ 8/8 passing
- ✓ tests (3m 24s)
- ✓ lint (1m 12s)
- ✓ build (2m 45s)
- ✓ security scan (2m 01s)

3. Review Changes

# View diff
gh pr diff <number>

# View diff stats
gh pr diff <number> --name-status

# Save diff for analysis
gh pr diff <number> > /tmp/pr_${number}.diff

Analyze changes:

  • Identify file types (code, tests, docs, config)
  • Look for security concerns
  • Check test coverage
  • Review code quality

4. Check for Common Issues

Security concerns:

# Look for potential security issues in diff
gh pr diff <number> | grep -E "(password|secret|api_key|token|private_key)"

Test coverage:

# Check if tests were added
gh pr diff <number> --name-status | grep -c "_test\|spec\|test_"

Large changes:

# Count lines changed
gh pr diff <number> --stat | tail -1

5. Request Feedback or Approve

If issues found:

gh pr review <number> --request-changes --body "$(cat <<'EOF'
Thanks for the PR! I have a few suggestions:

1. **Security**: Line 45 has a hardcoded API key
   - Move to environment variable or config

2. **Testing**: Missing tests for the new auth middleware
   - Add tests in `tests/auth.test.ts`

3. **Documentation**: Update README with auth setup instructions

Let me know if you have questions!
EOF
)"

If approved:

gh pr review <number> --approve --body "$(cat <<'EOF'
LGTM!

Changes look good:
✓ Tests added and passing
✓ Security best practices followed
✓ Code is well-documented

Approved to merge.
EOF
)"

6. Suggest Next Steps

Based on review outcome:

If approved and checks passing:

✓ PR approved
✓ All checks passing
✓ No merge conflicts

Ready to merge:
gh pr merge <number> --squash  # or --merge, --rebase

If changes requested:

Requested changes on PR #123

Waiting for author to address feedback.
Track updates: gh pr view <number>

If checks failing:

Cannot approve: checks are failing

Fix issues first:
gh pr checks <number>

Advanced Usage

Review Checklist

Create a review checklist template:

cat > review_checklist.md <<'EOF'
## Code Review Checklist

### Functionality
- [ ] Changes match the PR description
- [ ] No obvious bugs or logical errors
- [ ] Edge cases are handled

### Code Quality
- [ ] Code follows project style guidelines
- [ ] No code duplication
- [ ] Functions are small and focused
- [ ] Variable names are clear

### Testing
- [ ] Tests added for new functionality
- [ ] Tests cover edge cases
- [ ] All tests passing

### Security
- [ ] No hardcoded secrets or credentials
- [ ] Input validation present
- [ ] No obvious security vulnerabilities

### Documentation
- [ ] Code is commented where necessary
- [ ] README updated if needed
- [ ] API docs updated if applicable

### Performance
- [ ] No obvious performance issues
- [ ] Database queries are optimized
- [ ] No memory leaks
EOF

Inline Comments with GitHub API

For thorough code reviews, you can add inline comments on specific file paths and line numbers using the GitHub API. This provides context-aware feedback directly on the code.

Basic Syntax

# Single inline comment
gh api repos/OWNER/REPO/pulls/PR_NUMBER/reviews \
  -f event="COMMENT" \
  -f body="Overall review summary" \
  -F comments[][path]="path/to/file.go" \
  -F comments[][line]=42 \
  -F comments[][side]="RIGHT" \
  -F comments[][body]="Your feedback here (supports Markdown)"

Required Parameters

  • event: Review event type

    • "COMMENT" - Comment without approval/rejection
    • "APPROVE" - Approve the PR
    • "REQUEST_CHANGES" - Request changes (blocks merge)
  • body: Overall review summary (appears at top of review)

  • For each inline comment:

    • comments[][path] - File path relative to repo root
    • comments[][line] - Line number in the diff
    • comments[][side] - "RIGHT" (new code) or "LEFT" (old code)
    • comments[][body] - Comment text (Markdown supported)

Complete Example: Request Changes with Multiple Inline Comments

# Get repository info first
REPO=$(gh repo view --json nameWithOwner -q .nameWithOwner)
PR_NUMBER=81

# Post comprehensive review with inline comments
gh api repos/${REPO}/pulls/${PR_NUMBER}/reviews \
  -f event="REQUEST_CHANGES" \
  -f body="$(cat <<'EOF'
Thanks for the PR! This adds valuable functionality, but I found several issues that need addressing before merge.

## Summary
- ❌ **Critical**: No tests included (blocker)
- ⚠️ **High**: Inconsistent error handling
- 💡 **Medium**: Some code duplication

Please address the critical and high-priority issues. The medium items are optional improvements.
EOF
)" \
  -F comments[][path]="internal/artifacthub/client.go" \
  -F comments[][line]=1 \
  -F comments[][side]="RIGHT" \
  -F comments[][body]="$(cat <<'EOF'
**Missing Package Documentation**

Add package-level documentation:
\`\`\`go
// Package artifacthub provides a client for interacting with the Artifact Hub API
// to discover, retrieve, and install Tekton tasks and pipelines.
package artifacthub
\`\`\`
EOF
)" \
  -F comments[][path]="internal/artifacthub/client.go" \
  -F comments[][line]=221 \
  -F comments[][side]="RIGHT" \
  -F comments[][body]="$(cat <<'EOF'
**Missing Error Wrapping**

Use \`%w\` to wrap errors for better error inspection:
\`\`\`go
return nil, fmt.Errorf("API request failed with status %d: %w", resp.StatusCode, err)
\`\`\`

This enables \`errors.Is()\` and \`errors.As()\` usage.
EOF
)" \
  -F comments[][path]="internal/tools/artifacthub.go" \
  -F comments[][line]=47 \
  -F comments[][side]="RIGHT" \
  -F comments[][body]="$(cat <<'EOF'
**Inconsistent Return Signature**

This function returns an error that's always \`nil\`. Make it consistent with similar functions:

\`\`\`go
// Remove error return since it's never used
func installArtifactHubTask() *mcp.ServerTool {
    return mcp.NewServerTool(...)
}
\`\`\`
EOF
)" \
  -F comments[][path]="internal/tools/artifacthub.go" \
  -F comments[][line]=312 \
  -F comments[][side]="RIGHT" \
  -F comments[][body]="$(cat <<'EOF'
**Code Duplication (DRY Violation)**

This parameter conversion code is duplicated at lines 312-329 and 375-392. Extract to a helper function:

\`\`\`go
func convertParamValue(value interface{}) string {
    switch v := value.(type) {
    case string:
        return v
    case int:
        return strconv.Itoa(v)
    case float64:
        return strconv.FormatFloat(v, 'f', -1, 64)
    case bool:
        return strconv.FormatBool(v)
    default:
        return fmt.Sprintf("%v", v)
    }
}
\`\`\`
EOF
)"

Example: Approve with Inline Suggestions

# Approve but add non-blocking suggestions
gh api repos/${REPO}/pulls/${PR_NUMBER}/reviews \
  -f event="APPROVE" \
  -f body="$(cat <<'EOF'
LGTM! Great work on this feature.

I've left a few inline suggestions for future improvements, but none are blocking.

✅ Functionality is solid
✅ Code follows Go best practices
✅ Error handling is good

Approved to merge.
EOF
)" \
  -F comments[][path]="internal/artifacthub/client.go" \
  -F comments[][line]=128 \
  -F comments[][side]="RIGHT" \
  -F comments[][body]="💡 **Optional**: Consider making the HTTP timeout configurable via a \`ClientOption\` function for better testability." \
  -F comments[][path]="internal/tools/artifacthub.go" \
  -F comments[][line]=100 \
  -F comments[][side]="RIGHT" \
  -F comments[][body]="💡 **Future enhancement**: Could add caching here to avoid repeated Artifact Hub lookups for the same package."

Example: Comment on Old Code (LEFT side)

# Comment on code being removed/changed (LEFT side of diff)
gh api repos/${REPO}/pulls/${PR_NUMBER}/reviews \
  -f event="COMMENT" \
  -f body="Question about the changes" \
  -F comments[][path]="pkg/handler.go" \
  -F comments[][line]=15 \
  -F comments[][side]="LEFT" \
  -F comments[][body]="Why was this error handling removed? Was it redundant?"

Finding Line Numbers

To determine the correct line numbers for comments:

# View the diff with line numbers
gh pr diff ${PR_NUMBER} > /tmp/pr.diff

# Or view specific file
gh pr diff ${PR_NUMBER} | grep -A5 -B5 "filename"

# The line number refers to the line in the DIFF, not the final file

Important: Line numbers refer to the position in the diff, not the absolute line number in the file.

Best Practices for Inline Comments

⚠️ CRITICAL: ALWAYS ASK USER BEFORE POSTING REVIEWS

When reviewing PRs, NEVER post the review directly without explicit user approval:

  1. Draft the review with all inline comments
  2. Show the user what you plan to post (full command with all comments)
  3. Ask for confirmation: “Should I post this review?”
  4. Wait for approval before executing the command

This prevents accidental reviews and allows the user to review/modify the feedback.

Other Best Practices:

  1. Use for specific issues: Inline comments are best for pointing out specific problems
  2. General feedback in body: Put high-level summary in the review body
  3. Include suggestions: Don’t just point out problems, suggest fixes
  4. Use code blocks: Format code suggestions with triple backticks
  5. Be constructive: Frame feedback positively
  6. Prioritize issues: Use tags like ❌ (critical), ⚠️ (important), 💡 (suggestion)
  7. Explain reasoning: Help the author understand WHY something should change
  8. Limit quantity: Too many inline comments can be overwhelming (aim for <15)

Troubleshooting

Line number errors:

# Error: "line N does not exist in diff"
# Solution: The line number must be a changed line (added/removed/modified)
# Use only lines visible in: gh pr diff <number>

Side confusion:

  • "RIGHT" = New code (green in GitHub UI, lines with +)
  • "LEFT" = Old code (red in GitHub UI, lines with -)

Testing your review:

# Before posting real review, test with event="COMMENT"
# This won't block the PR or send notifications
gh api repos/${REPO}/pulls/${PR_NUMBER}/reviews \
  -f event="COMMENT" \
  -f body="Test review" \
  # ... test your inline comments

Batch Review

Review multiple PRs:

# Get all open PRs
gh pr list --json number,title,author

# Review each
for pr in $(gh pr list --json number --jq '.[].number'); do
  echo "Reviewing PR #$pr"
  gh pr view "$pr"
  # ... review process
done

Common Scenarios

Scenario 1: Quick Approval

# View PR
gh pr view 123

# Check status
gh pr checks 123

# If all good, approve
gh pr checks 123 && \
  gh pr review 123 --approve --body "LGTM! Approving." && \
  gh pr merge 123 --squash

Scenario 2: Detailed Review

# Checkout and test locally
gh pr checkout 123
npm test
npm run lint

# If passing, approve
if [ $? -eq 0 ]; then
  gh pr review 123 --approve --body "Tested locally. All passing."
else
  gh pr review 123 --request-changes --body "Tests failing locally. Please investigate."
fi

# Return to main
git checkout main

Scenario 3: Collaborative Review

# Check existing reviews
gh pr view 123 --json reviews

# Add your review
gh pr review 123 --comment --body "Adding review comments. Will approve after @otherreviewer weighs in."

# Later, after discussion
gh pr review 123 --approve

Scenario 4: Thorough Review with Inline Comments

# 1. Fetch PR information
REPO=$(gh repo view --json nameWithOwner -q .nameWithOwner)
PR_NUMBER=123

gh pr view ${PR_NUMBER} --json title,body,author,files
gh pr checks ${PR_NUMBER}

# 2. Analyze the diff
gh pr diff ${PR_NUMBER} > /tmp/pr_${PR_NUMBER}.diff

# 3. Check for issues
## No tests?
gh pr diff ${PR_NUMBER} --name-only | grep -q "_test" || echo "⚠️ No tests found"

## Security concerns?
gh pr diff ${PR_NUMBER} | grep -iE "(password|secret|api_key)" && echo "⚠️ Potential secrets"

# 4. Post comprehensive review with inline comments
gh api repos/${REPO}/pulls/${PR_NUMBER}/reviews \
  -f event="REQUEST_CHANGES" \
  -f body="$(cat <<'EOF'
Thanks for the PR! I've reviewed the changes and found several issues.

## Summary
- ❌ **Critical**: Missing test coverage (blocker)
- ⚠️ **High**: Error handling needs improvement
- 💡 **Medium**: Consider refactoring duplicated code

I've left inline comments on specific lines. Please address the critical issues before merge.
EOF
)" \
  -F comments[][path]="internal/client.go" \
  -F comments[][line]=1 \
  -F comments[][side]="RIGHT" \
  -F comments[][body]="**Missing package documentation** - Add a package comment explaining the purpose" \
  -F comments[][path]="internal/client.go" \
  -F comments[][line]=150 \
  -F comments[][side]="RIGHT" \
  -F comments[][body]="**Error wrapping**: Use \`%w\` here to preserve the error chain" \
  -F comments[][path]="internal/handler.go" \
  -F comments[][line]=45 \
  -F comments[][side]="RIGHT" \
  -F comments[][body]="**Missing tests**: Add tests for this new handler in \`handler_test.go\`"

# 5. Track the review
echo "✅ Review posted. Track updates:"
echo "gh pr view ${PR_NUMBER}"

Custom Tool: Enhanced Review

For complex review workflows:

# In tools/ directory
./ReviewPR.ts --pr 123 --checklist --auto-test --report

Tool capabilities:

  • Automatic checklist generation
  • Clone and test PR in isolated environment
  • Generate review report with metrics
  • Compare against style guide automatically
  • Integration with code quality tools (SonarQube, etc.)

Review Templates

Approval Template

gh pr review <number> --approve --body "$(cat <<'EOF'
✓ Code review complete

**What I reviewed:**
- Code changes and logic
- Test coverage
- Security considerations
- Documentation

**Summary:**
All looks good! Changes are well-implemented and tested.

Approved to merge.
EOF
)"

Request Changes Template

gh pr review <number> --request-changes --body "$(cat <<'EOF'
Thanks for the PR! I have some feedback:

## Required Changes

1. **Issue 1**
   - Description
   - Suggested fix

2. **Issue 2**
   - Description
   - Suggested fix

## Optional Suggestions

- Suggestion 1
- Suggestion 2

Let me know if you have questions!
EOF
)"

Comment Template

gh pr review <number> --comment --body "$(cat <<'EOF'
A few suggestions (non-blocking):

💡 **Performance**: Consider caching the result of `expensiveFunction()`
💡 **Testing**: Might be worth adding a test for the error case
💡 **Documentation**: Could add a code comment explaining why we use this approach

Feel free to address in this PR or a follow-up.
EOF
)"

Best Practices

⚠️ #1 RULE: ALWAYS ASK USER BEFORE POSTING REVIEWS

Never post PR reviews, approvals, or request changes without explicit user confirmation. Always:

  1. Draft the review and show it to the user
  2. Ask: “Should I post this review?”
  3. Wait for approval before executing

Other Best Practices:

  1. Review promptly: Don’t let PRs sit unreviewed
  2. Be constructive: Frame feedback positively
  3. Explain reasoning: Don’t just say “change this”
  4. Test locally when needed: For complex changes, test yourself
  5. Check the context: Read linked issues/discussions
  6. Review tests first: Ensure tests validate the changes
  7. Look for security issues: Always consider security implications
  8. Don’t nitpick: Focus on important issues
  9. Approve fast, merge slow: Approve quickly but merge after all reviewers approve
  10. Follow up: Check if your feedback was addressed

Integration with Other Workflows

  • CheckStatus: Check CI before reviewing
  • CreatePR: Review immediately after creating your own PRs
  • ManageIssues: Link issues in review comments

Resources