feat: add MCP tools exposing review agent functionality

[dtsong]

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 (237 tests) passes
• Lint clean

Closes #14

🤖 Generated with Claude Code

[github-actions]

AI Code Review

Summary

The PR adds 5 MCP tools with good structure and test coverage. However, there are several critical issues including error handling gaps, potential security vulnerabilities, and missing edge case validation.

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 main functionality
• Clean separation of concerns with individual functions for each tool
• Proper JSON schema definitions for tool inputs
• Good error messages for missing credentials

Issues Found

Critical (5)

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

No validation that repo contains exactly one '/' character before splitting. Invalid repo formats like 'owner' or 'owner/repo/extra' will cause IndexError or incorrect parsing.
Suggestion: Add validation to ensure repo is in correct 'owner/repo' format before splitting

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

Same repo format validation issue exists in _check_pr_size function.
Suggestion: Add the same repo format validation as suggested for _review_pr

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

Same repo format validation issue exists in _check_pr_lint function.
Suggestion: Add the same repo format validation as suggested for _review_pr

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

Same repo format validation issue exists in _get_review_history function.
Suggestion: Add the same repo format validation as suggested for _review_pr

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

Same repo format validation issue exists in _get_cost_summary function when repo filter is provided.
Suggestion: Add validation for the optional repo parameter

Major (5)

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

No exception handling in _review_pr function. Network errors, API failures, or import errors could cause crashes.
Suggestion: Add try-catch block to handle exceptions and return meaningful error messages

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

No exception handling in _check_pr_size function for GitHub API or configuration loading failures.
Suggestion: Add try-catch block to handle exceptions gracefully

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

No exception handling in _check_pr_lint function for potential failures.
Suggestion: Add try-catch block to handle exceptions gracefully

src/pr_review_agent/mcp/tools.py:246 - security

Direct use of user input (repo) in Supabase query without validation. Could potentially be exploited for SQL injection if the query builder doesn't properly escape inputs.
Suggestion: Validate repo format and consider additional input sanitization before database queries

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

No exception handling for Supabase operations or datetime operations that could fail.
Suggestion: Add try-catch block to handle database and datetime exceptions

Minor (2)

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

The 'days' parameter has a default of 30 but this is only documented in the description, not enforced in the code when the parameter is missing.
Suggestion: The current implementation correctly uses args.get('days', 30), so this is actually handled properly

tests/test_mcp_tools.py:1 - testing

Missing tests for exception handling paths and edge cases like invalid repo formats, missing environment variables in actual usage scenarios.
Suggestion: Add tests for error conditions and edge cases to improve coverage

Suggestion (1)

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

Each function loads config independently. Consider caching configuration if these tools are called frequently.
Suggestion: Consider implementing configuration caching for better performance

Concerns

• Critical lack of input validation for repo format could cause runtime errors
• No exception handling means any API or system failure will crash the tools
• Potential security implications with unvalidated user input in database queries
• Missing tests for error conditions and edge cases

Questions

• Should there be rate limiting or caching for these tools since they make external API calls?
• Are there any specific security requirements for the Supabase queries that should be implemented?
• Should the tools return structured data instead of formatted text for better programmatic usage?

---

_{Model: claude-sonnet-4-20250514 | Cost: $0.0505 |
Tokens: 6159 in / 2135 out}