Skip to content

Conversation

@dtsong
Copy link
Owner

@dtsong dtsong commented Jan 23, 2026

Summary

  • Adds 5 MCP tools: review_pr, check_pr_size, check_pr_lint, get_review_history, get_cost_summary
  • Tools use lazy imports to avoid circular dependencies
  • Full test coverage with 8 tests

Test plan

  • All 8 new tests pass
  • Full suite (254 tests) passes
  • Lint clean

Closes #14

🤖 Generated with Claude Code

Implements 5 MCP tools: review_pr, check_pr_size, check_pr_lint,
get_review_history, and get_cost_summary. Uses lazy imports to
avoid circular dependencies.

Closes #14

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@dtsong dtsong merged commit 9e69ab4 into main Jan 23, 2026
2 checks passed
@dtsong dtsong deleted the feat/14-mcp-tools branch January 23, 2026 20:57
@github-actions
Copy link

AI Code Review

Summary

The PR adds 5 MCP tools with good test coverage and uses lazy imports to avoid circular dependencies. The implementation is generally solid, but there are some error handling and edge case issues that need attention.

Confidence: 0.00 (low)
This PR may require human review

Strengths

  • Good use of lazy imports to avoid circular dependencies
  • Comprehensive test coverage with 8 tests covering different scenarios
  • Clear tool descriptions and well-defined input schemas
  • Proper separation of concerns with individual functions for each tool
  • Good error messages for missing Supabase credentials

Issues Found

Major (6)

src/pr_review_agent/mcp/tools.py:134 - logic

The repo.split('/') will raise ValueError if the repo string doesn't contain exactly one slash
Suggestion: Add validation to ensure repo format is correct before splitting

src/pr_review_agent/mcp/tools.py:194 - logic

Same repo.split('/') issue exists in _check_pr_size function
Suggestion: Add validation to ensure repo format is correct before splitting

src/pr_review_agent/mcp/tools.py:218 - logic

Same repo.split('/') issue exists in _check_pr_lint function
Suggestion: Add validation to ensure repo format is correct before splitting

src/pr_review_agent/mcp/tools.py:247 - logic

Same repo.split('/') issue exists in _get_review_history function
Suggestion: Add validation to ensure repo format is correct before splitting

src/pr_review_agent/mcp/tools.py:306 - logic

Same repo.split('/') issue exists in _get_cost_summary function when repo filter is provided
Suggestion: Add validation to ensure repo format is correct before splitting

src/pr_review_agent/mcp/tools.py:125 - logic

The _review_pr function doesn't handle exceptions that could be thrown during GitHub API calls, config loading, or LLM calls
Suggestion: Wrap the function logic in try-catch blocks to handle potential API errors, network issues, or configuration problems gracefully

Minor (3)

src/pr_review_agent/mcp/tools.py:256 - logic

The Supabase query doesn't handle potential exceptions from the database call
Suggestion: Add error handling for Supabase query failures to provide meaningful error messages

src/pr_review_agent/mcp/tools.py:300 - logic

The Supabase query in _get_cost_summary doesn't handle potential exceptions from the database call
Suggestion: Add error handling for Supabase query failures to provide meaningful error messages

src/pr_review_agent/mcp/tools.py:292 - performance

Date calculation could be simplified and the import is unnecessarily nested
Suggestion: Move the timedelta import to the top and simplify the date calculation

Suggestion (1)

src/pr_review_agent/mcp/tools.py:1 - style

Consider extracting common repo validation logic into a helper function to reduce code duplication
Suggestion: Create a _parse_repo helper function to validate and parse repo strings consistently across all tools

Concerns

  • Lack of input validation for repo format could cause runtime errors
  • Missing error handling for external API calls could lead to unhandled exceptions
  • No rate limiting or timeout handling for potentially expensive operations like full PR reviews

Questions

  • Should there be any authentication/authorization checks before allowing these tools to run?
  • Are there any rate limits or cost controls that should be implemented for the expensive LLM review operations?
  • Would it be beneficial to add caching for repeated PR data fetches?

Model: claude-sonnet-4-20250514 | Cost: $0.0405 |
Tokens: 6159 in / 1465 out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Phase 6.2] MCP Tools

2 participants