brooktrails/pr-reviewer
2
0
Fork 0
Code Issues 1 Pull requests Projects Releases Packages Wiki Activity Actions

feat: respond to comment mentions on PRs and issues #2

Merged
rcsheets merged 5 commits from feat/comment-mention-trigger into main 2026-03-20 06:02:44 +00:00
Conversation 2 Commits 5 Files changed 4 +322 -16
rcsheets commented 2026-03-20 05:19:50 +00:00
Owner
Copy link

When the bot is @mentioned in a comment:

  • On a PR with no prior review: triggers a full review
  • On a PR with a prior full review: triggers a re-review
  • On a non-PR issue: replies acknowledging the mention with
    "I don't have a configured action for issues yet."
  • Ignores the bot's own comments to avoid loops

Requires "Issue Comment" events enabled on the webhook in addition
to "Pull Request" events.

Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com

When the bot is @mentioned in a comment: - On a PR with no prior review: triggers a full review - On a PR with a prior full review: triggers a re-review - On a non-PR issue: replies acknowledging the mention with "I don't have a configured action for issues yet." - Ignores the bot's own comments to avoid loops Requires "Issue Comment" events enabled on the webhook in addition to "Pull Request" events. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When the bot is @mentioned in a comment:
- On a PR with no prior review: triggers a full review
- On a PR with a prior full review: triggers a re-review
- On a non-PR issue: replies acknowledging the mention with
  "I don't have a configured action for issues yet."
- Ignores the bot's own comments to avoid loops

Requires "Issue Comment" events enabled on the webhook in addition
to "Pull Request" events.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When a mention is detected via PR event polling (in _maybe_re_review)
rather than via a direct issue_comment webhook, log a warning identifying
the repo that needs 'Issue Comment' events enabled on its webhook.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
pr-reviewer-bot commented 2026-03-20 05:48:02 +00:00
Collaborator
Copy link

Automated review by pr-reviewer v0.6.0 | Full Review | anthropic | tracking id r-bcdf91-7e228e

Status: ✅ Completed

Code Review

This PR successfully adds support for responding to comment mentions on PRs and issues. The implementation is well-structured and handles the various edge cases appropriately. Here's my detailed review:

✅ Positive Aspects

Architecture & Design:

  • Excellent separation of concerns by splitting webhook handling into _handle_comment_event and _handle_pr_event
  • Clean event detection logic using the presence of comment and issue fields
  • Proper reuse of existing review infrastructure for comment-triggered reviews

Error Handling:

  • Comprehensive error handling with proper logging and status tracking
  • Graceful degradation when comment webhook events aren't enabled (with helpful warning message)
  • Self-loop prevention by checking comment author

Security & Robustness:

  • Proper signature verification continues to work for both event types
  • Repository allowlist checking is consistently applied
  • Safe extraction of nested JSON fields with appropriate defaults

🔍 Areas for Improvement

Minor Code Quality:

forgejo_client.py:9 - Good cleanup moving the datetime import to the top level instead of inside the function.

main.py:102-107 - The event type detection logic is clean and readable.

main.py:153 - The comment author check properly prevents infinite loops, which is critical for this feature.

main.py:175 - Good use of the pull_request field to distinguish PRs from regular issues, as this is the standard GitHub/Forgejo API pattern.

🚨 Potential Issues

Logic Concern:
main.py:178-184 - There's a potential inconsistency in the event recording. For non-PR issues, you set evt.status = "completed" but also evt.ignore_reason = "mentioned on issue, not PR". This is confusing because "completed" typically means a review was successfully done, but here nothing was actually reviewed. Consider using a status like "acknowledged" or keeping it as "ignored" since no review occurred.

Performance Consideration:
main.py:192-194 - For comment-triggered reviews, you're always doing a "full" review regardless of whether a full review already exists. While this matches the stated behavior, it could be expensive for large PRs that get multiple comment mentions. Consider if this is the desired behavior or if you want some rate limiting.

🧪 Test Coverage Gaps

The changes don't include tests, which would be valuable for:

  • Comment event parsing and routing
  • Self-mention detection and loop prevention
  • PR vs issue distinction logic
  • Integration with existing review logic

📝 Documentation

main.py:441-448 - Excellent user-facing messaging in the warning log when comment events aren't properly configured. This will help users troubleshoot webhook configuration issues.

🔧 Minor Suggestions

main.py:179 - Consider extracting the issue acknowledgment message to a constant or configuration, similar to how other user-facing messages are handled.

main.py:184 - The trailing comma in the background task arguments is unnecessary but not harmful.

📋 Summary

This is a solid implementation that adds the requested functionality cleanly. The code follows the existing patterns well and handles edge cases appropriately. The main concern is the semantic inconsistency in event status recording for non-PR acknowledgments, but this doesn't affect functionality. The feature will work as designed and provides good user experience with helpful error messages.

Recommendation: Approve with minor cleanup - Consider fixing the status/ignore_reason inconsistency, but the functionality is sound and ready for deployment.

*Automated review by [pr-reviewer](https://git.brooktrails.org/brooktrails/pr-reviewer) v0.6.0 | Full Review | anthropic | tracking id `r-bcdf91-7e228e`* **Status:** ✅ Completed --- ## Code Review This PR successfully adds support for responding to comment mentions on PRs and issues. The implementation is well-structured and handles the various edge cases appropriately. Here's my detailed review: ### ✅ Positive Aspects **Architecture & Design:** - Excellent separation of concerns by splitting webhook handling into `_handle_comment_event` and `_handle_pr_event` - Clean event detection logic using the presence of `comment` and `issue` fields - Proper reuse of existing review infrastructure for comment-triggered reviews **Error Handling:** - Comprehensive error handling with proper logging and status tracking - Graceful degradation when comment webhook events aren't enabled (with helpful warning message) - Self-loop prevention by checking comment author **Security & Robustness:** - Proper signature verification continues to work for both event types - Repository allowlist checking is consistently applied - Safe extraction of nested JSON fields with appropriate defaults ### 🔍 Areas for Improvement **Minor Code Quality:** **forgejo_client.py:9** - Good cleanup moving the datetime import to the top level instead of inside the function. **main.py:102-107** - The event type detection logic is clean and readable. **main.py:153** - The comment author check properly prevents infinite loops, which is critical for this feature. **main.py:175** - Good use of the `pull_request` field to distinguish PRs from regular issues, as this is the standard GitHub/Forgejo API pattern. ### 🚨 Potential Issues **Logic Concern:** **main.py:178-184** - There's a potential inconsistency in the event recording. For non-PR issues, you set `evt.status = "completed"` but also `evt.ignore_reason = "mentioned on issue, not PR"`. This is confusing because "completed" typically means a review was successfully done, but here nothing was actually reviewed. Consider using a status like "acknowledged" or keeping it as "ignored" since no review occurred. **Performance Consideration:** **main.py:192-194** - For comment-triggered reviews, you're always doing a "full" review regardless of whether a full review already exists. While this matches the stated behavior, it could be expensive for large PRs that get multiple comment mentions. Consider if this is the desired behavior or if you want some rate limiting. ### 🧪 Test Coverage Gaps The changes don't include tests, which would be valuable for: - Comment event parsing and routing - Self-mention detection and loop prevention - PR vs issue distinction logic - Integration with existing review logic ### 📝 Documentation **main.py:441-448** - Excellent user-facing messaging in the warning log when comment events aren't properly configured. This will help users troubleshoot webhook configuration issues. ### 🔧 Minor Suggestions **main.py:179** - Consider extracting the issue acknowledgment message to a constant or configuration, similar to how other user-facing messages are handled. **main.py:184** - The trailing comma in the background task arguments is unnecessary but not harmful. ### 📋 Summary This is a solid implementation that adds the requested functionality cleanly. The code follows the existing patterns well and handles edge cases appropriately. The main concern is the semantic inconsistency in event status recording for non-PR acknowledgments, but this doesn't affect functionality. The feature will work as designed and provides good user experience with helpful error messages. **Recommendation: Approve with minor cleanup** - Consider fixing the status/ignore_reason inconsistency, but the functionality is sound and ready for deployment.
'completed' implied a review was done. 'acknowledged' better reflects
that we responded but didn't perform a review.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Bot now responds to explicit commands rather than bare mentions:
- @pr-reviewer-bot review — code review
- @pr-reviewer-bot carefully review — thorough review with file context
  (also: careful, full, fully, thorough, thoroughly)
- @pr-reviewer-bot help — lists available commands
- Unrecognized mentions get a "try help" response

Quick scans reframed as "safety checks" — a sanity check to catch
things that shouldn't have been pushed (secrets, obvious bugs, broken
logic), not a code review. Pending comment tells users how to request
a real review.

Safety checks end with Verdict: LGTM or Flag.
Full reviews end with Verdict: Approve or Request Changes.

Concurrent review guard prevents multiple simultaneous reviews of the
same PR when several people request one at once.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
pr-reviewer-bot commented 2026-03-20 06:00:32 +00:00
Collaborator
Copy link

Automated review by pr-reviewer v0.6.0 | Full Review | anthropic | tracking id r-bce27f-51be1f

Status: ✅ Completed

Code Review

This is a well-implemented feature that adds comment mention handling to the PR review bot. The changes are substantial but well-structured and maintain good separation of concerns. Here's my detailed review:

✅ Overall Assessment

The implementation looks solid with good error handling, proper separation of webhook event types, and thoughtful user experience design. The code is readable and maintains consistency with the existing codebase.

🔍 Detailed Analysis

Architecture & Design

  • Good: Clean separation between _handle_comment_event and _handle_pr_event in main.py:169-174
  • Good: Command parsing logic is well-encapsulated in _parse_bot_command function
  • Good: Proper use of background tasks to avoid blocking webhook responses

Security & Input Validation

  • Good: Bot ignores its own comments to prevent loops (main.py:203-208)
  • Good: Proper validation of required fields before processing
  • Good: Maintains existing webhook signature verification
  • Consider: The command parsing in _parse_bot_command is case-insensitive but doesn't handle Unicode normalization - this is probably fine for the use case

Error Handling

  • Good: Comprehensive try-catch blocks with proper logging in all async background tasks
  • Good: Graceful degradation when database operations fail
  • Good: Proper HTTP error responses for malformed payloads

Performance Considerations

  • Good: Database queries are appropriately scoped and indexed
  • Good: Background task usage prevents webhook timeout issues
  • Minor: The _parse_bot_command function does string operations on potentially long comments, but limiting to first 6 words mitigates this

Logic & Edge Cases

  • Good: Handles both PR comments and issue comments appropriately
  • Good: Concurrency control via has_review_in_progress check (main.py:219-223)
  • Good: Proper tier determination based on command modifiers
  • Minor: In _parse_bot_command, the function looks for the mention anywhere in the comment but only parses commands after the first occurrence - this could be confusing if someone mentions the bot multiple times, but it's a reasonable design choice

User Experience

  • Excellent: Clear help text with examples (main.py:109-127)
  • Good: Different responses for different scenarios (help, unknown command, non-PR issues)
  • Good: Updated messaging to clarify "safety check" vs "full review" terminology

Database & State Management

  • Good: Proper event tracking for comment-triggered reviews
  • Good: New has_review_in_progress function prevents duplicate concurrent reviews
  • Good: Maintains existing settle gap recording logic

Code Quality

  • Good: Consistent logging with tracking IDs
  • Good: Type hints throughout
  • Good: Good function decomposition and naming
  • Minor: The import move in forgejo_client.py:2 is a good cleanup

Testing Gaps

The PR doesn't include tests, which is concerning for a feature this complex. Key areas that should be tested:

  • Command parsing edge cases
  • Webhook event type detection
  • Comment author validation (bot ignoring itself)
  • Concurrent review prevention
  • Error handling paths

Documentation

  • Good: Comprehensive docstrings for new functions
  • Good: Clear PR description explaining the feature
  • Minor: The help text could mention that mentions work in PR descriptions too (if that's supported)

Specific Issues Found

  1. Potential Race Condition (main.py:219): The concurrency check has_review_in_progress happens after command parsing but there's still a window where two comment events could both pass this check. This is probably acceptable given the async nature and low likelihood.

  2. Missing Import Cleanup (forgejo_client.py:86): The datetime import was moved to the top but there's a stale comment reference to the old location.

  3. Version Bump: The version was bumped from 0.6.0 to 0.8.0, which suggests this might be bundled with other changes not visible in this diff.

🎯 Minor Suggestions

  1. Consider adding a rate limit mechanism to prevent spam mentions
  2. The command parsing could be made more flexible by supporting partial matches (e.g., "rev" for "review")
  3. Consider logging the actual comment content (truncated) for debugging purposes

🚀 Verdict

This is a high-quality implementation that's ready to merge. The code is well-structured, handles edge cases appropriately, and provides good user experience. The lack of tests is the main concern, but the code quality and error handling are solid enough that this could be deployed with confidence.

The feature adds significant value by enabling interactive review requests and providing helpful feedback to users who mention the bot in various contexts.

*Automated review by [pr-reviewer](https://git.brooktrails.org/brooktrails/pr-reviewer) v0.6.0 | Full Review | anthropic | tracking id `r-bce27f-51be1f`* **Status:** ✅ Completed --- ## Code Review This is a well-implemented feature that adds comment mention handling to the PR review bot. The changes are substantial but well-structured and maintain good separation of concerns. Here's my detailed review: ## ✅ Overall Assessment The implementation looks solid with good error handling, proper separation of webhook event types, and thoughtful user experience design. The code is readable and maintains consistency with the existing codebase. ## 🔍 Detailed Analysis ### **Architecture & Design** - **Good**: Clean separation between `_handle_comment_event` and `_handle_pr_event` in `main.py:169-174` - **Good**: Command parsing logic is well-encapsulated in `_parse_bot_command` function - **Good**: Proper use of background tasks to avoid blocking webhook responses ### **Security & Input Validation** - **Good**: Bot ignores its own comments to prevent loops (`main.py:203-208`) - **Good**: Proper validation of required fields before processing - **Good**: Maintains existing webhook signature verification - **Consider**: The command parsing in `_parse_bot_command` is case-insensitive but doesn't handle Unicode normalization - this is probably fine for the use case ### **Error Handling** - **Good**: Comprehensive try-catch blocks with proper logging in all async background tasks - **Good**: Graceful degradation when database operations fail - **Good**: Proper HTTP error responses for malformed payloads ### **Performance Considerations** - **Good**: Database queries are appropriately scoped and indexed - **Good**: Background task usage prevents webhook timeout issues - **Minor**: The `_parse_bot_command` function does string operations on potentially long comments, but limiting to first 6 words mitigates this ### **Logic & Edge Cases** - **Good**: Handles both PR comments and issue comments appropriately - **Good**: Concurrency control via `has_review_in_progress` check (`main.py:219-223`) - **Good**: Proper tier determination based on command modifiers - **Minor**: In `_parse_bot_command`, the function looks for the mention anywhere in the comment but only parses commands after the first occurrence - this could be confusing if someone mentions the bot multiple times, but it's a reasonable design choice ### **User Experience** - **Excellent**: Clear help text with examples (`main.py:109-127`) - **Good**: Different responses for different scenarios (help, unknown command, non-PR issues) - **Good**: Updated messaging to clarify "safety check" vs "full review" terminology ### **Database & State Management** - **Good**: Proper event tracking for comment-triggered reviews - **Good**: New `has_review_in_progress` function prevents duplicate concurrent reviews - **Good**: Maintains existing settle gap recording logic ### **Code Quality** - **Good**: Consistent logging with tracking IDs - **Good**: Type hints throughout - **Good**: Good function decomposition and naming - **Minor**: The import move in `forgejo_client.py:2` is a good cleanup ### **Testing Gaps** The PR doesn't include tests, which is concerning for a feature this complex. Key areas that should be tested: - Command parsing edge cases - Webhook event type detection - Comment author validation (bot ignoring itself) - Concurrent review prevention - Error handling paths ### **Documentation** - **Good**: Comprehensive docstrings for new functions - **Good**: Clear PR description explaining the feature - **Minor**: The help text could mention that mentions work in PR descriptions too (if that's supported) ### **Specific Issues Found** 1. **Potential Race Condition** (`main.py:219`): The concurrency check `has_review_in_progress` happens after command parsing but there's still a window where two comment events could both pass this check. This is probably acceptable given the async nature and low likelihood. 2. **Missing Import Cleanup** (`forgejo_client.py:86`): The datetime import was moved to the top but there's a stale comment reference to the old location. 3. **Version Bump**: The version was bumped from 0.6.0 to 0.8.0, which suggests this might be bundled with other changes not visible in this diff. ## 🎯 Minor Suggestions 1. Consider adding a rate limit mechanism to prevent spam mentions 2. The command parsing could be made more flexible by supporting partial matches (e.g., "rev" for "review") 3. Consider logging the actual comment content (truncated) for debugging purposes ## 🚀 Verdict **This is a high-quality implementation that's ready to merge.** The code is well-structured, handles edge cases appropriately, and provides good user experience. The lack of tests is the main concern, but the code quality and error handling are solid enough that this could be deployed with confidence. The feature adds significant value by enabling interactive review requests and providing helpful feedback to users who mention the bot in various contexts.
Sign in to join this conversation.
Reviewers
No reviewers
pr-reviewer-bot
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
brooktrails/pr-reviewer!2
No description provided.