Skip to content

Conversation

@dtsong
Copy link
Owner

@dtsong dtsong commented Jan 23, 2026

Summary

  • Exposes 3 MCP resources: config://, review://, metrics://
  • Config resource reads .ai-review.yaml from disk
  • Review/metrics resources query Supabase with proper error handling
  • 17 tests covering all resource handlers

Test plan

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

Closes #16

🤖 Generated with Claude Code

Exposes 3 resources via MCP: config://ai-review.yaml,
review://{owner}/{repo}/{pr}, and metrics://{owner}/{repo}/summary.
All backed by Supabase queries with proper error handling.

Closes #16

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@dtsong dtsong merged commit a2beb58 into main Jan 23, 2026
3 checks passed
@dtsong dtsong deleted the feat/16-mcp-resources branch January 23, 2026 21:00
@github-actions
Copy link

AI Code Review

Summary

The PR implements MCP resources for exposing configuration, reviews, and metrics data. The implementation is generally well-structured with good test coverage, but has several critical security and error handling issues that need to be addressed.

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

Strengths

  • Comprehensive test coverage with 17 tests covering all major code paths
  • Clean separation of concerns with dedicated functions for each resource type
  • Good error handling for missing environment variables
  • Proper JSON serialization with default=str for datetime objects
  • Well-structured URI parsing and routing logic

Issues Found

Critical (2)

src/pr_review_agent/mcp/resources.py:94 - security

SQL injection vulnerability when parsing pr_number from URI path without validation
Suggestion: Validate that pr_number is a valid integer before passing to int() and the database query

src/pr_review_agent/mcp/resources.py:103 - logic

IndexError will occur when result.data is empty, causing the function to crash instead of returning the intended error message
Suggestion: Check if result.data exists and has elements before accessing index 0

Major (3)

src/pr_review_agent/mcp/resources.py:73 - logic

Missing exception handling for Supabase client operations which could fail due to network issues, authentication problems, or database errors
Suggestion: Wrap Supabase operations in try-except blocks to handle potential exceptions gracefully

src/pr_review_agent/mcp/resources.py:122 - logic

Missing exception handling for Supabase client operations in metrics function, same issue as review function
Suggestion: Wrap Supabase operations in try-except blocks to handle potential exceptions gracefully

src/pr_review_agent/mcp/resources.py:89 - security

No validation or sanitization of owner and repo parameters that are used in database queries, potential for injection attacks
Suggestion: Validate that owner and repo contain only safe characters (alphanumeric, hyphens, underscores) before using in queries

Minor (2)

src/pr_review_agent/mcp/resources.py:64 - performance

Supabase client is created on every request instead of being reused
Suggestion: Consider creating a shared client instance or connection pool for better performance

src/pr_review_agent/mcp/resources.py:146 - logic

Division by zero protection exists but the confidence calculation could still fail with malformed data
Suggestion: Add additional validation to ensure confidence_score values are numeric before calculation

Concerns

  • Critical security vulnerabilities around input validation that could lead to SQL injection
  • Missing exception handling for database operations could cause crashes
  • No rate limiting or authentication for resource access
  • Direct database queries without any caching could impact performance

Questions

  • Should there be any authentication/authorization checks before allowing access to review and metrics data?
  • Are there any rate limiting considerations for the Supabase queries?
  • Should the config resource support reading from different locations or just the current directory?
  • Would it be beneficial to add caching for frequently accessed data?

Model: claude-sonnet-4-20250514 | Cost: $0.0346 |
Tokens: 5355 in / 1237 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.3] MCP Resources

2 participants