torbjorn/my-pal-mcp-server
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

feat: add review_pending_changes tool and enforce absolute path security

Browse Source 
- Add new review_pending_changes tool for comprehensive pre-commit reviews
- Implement filesystem sandboxing with MCP_PROJECT_ROOT
- Enforce absolute paths for all file/directory operations
- Add comprehensive git utilities for repository management
- Update all tools to use centralized path validation
- Add extensive test coverage for new features and security model
- Update documentation with new tool and path requirements
- Remove obsolete demo and guide files

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
Fahad
2025-06-09 12:42:18 +04:00
parent 00b365f108
commit 7ee610938b
23 changed files with 1369 additions and 486 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
tools/__init__.py
View File

@@ -6,6 +6,7 @@ from .analyze import AnalyzeTool
from .chat import ChatTool
from .debug_issue import DebugIssueTool
from .review_code import ReviewCodeTool
from .review_pending_changes import ReviewPendingChanges
from .think_deeper import ThinkDeeperTool
__all__ = [
@@ -14,4 +15,5 @@ __all__ = [
"DebugIssueTool",
"AnalyzeTool",
"ChatTool",
"ReviewPendingChanges",
]

6
tools/analyze.py
View File

@@ -16,7 +16,9 @@ from .base import BaseTool, ToolRequest
class AnalyzeRequest(ToolRequest):
"""Request model for analyze tool"""
files: List[str] = Field(..., description="Files or directories to analyze")
files: List[str] = Field(
..., description="Files or directories to analyze (must be absolute paths)"
)
question: str = Field(..., description="What to analyze or look for")
analysis_type: Optional[str] = Field(
None,
@@ -50,7 +52,7 @@ class AnalyzeTool(BaseTool):
"files": {
"type": "array",
"items": {"type": "string"},
"description": "Files or directories to analyze",
"description": "Files or directories to analyze (must be absolute paths)",
},
"question": {
"type": "string",

36
tools/base.py
View File

@@ -71,6 +71,32 @@ class BaseTool(ABC):
"""Return the Pydantic model for request validation"""
pass
def validate_file_paths(self, request) -> Optional[str]:
"""
Validate that all file paths in the request are absolute.
Returns error message if validation fails, None if all paths are valid.
"""
# Check if request has 'files' attribute
if hasattr(request, "files") and request.files:
for file_path in request.files:
if not os.path.isabs(file_path):
return (
f"Error: All file paths must be absolute. "
f"Received relative path: {file_path}\n"
f"Please provide the full absolute path starting with '/'"
)
# Check if request has 'path' attribute (for review_pending_changes)
if hasattr(request, "path") and request.path:
if not os.path.isabs(request.path):
return (
f"Error: Path must be absolute. "
f"Received relative path: {request.path}\n"
f"Please provide the full absolute path starting with '/'"
)
return None
async def execute(self, arguments: Dict[str, Any]) -> List[TextContent]:
"""Execute the tool with given arguments"""
try:
@@ -78,6 +104,16 @@ class BaseTool(ABC):
request_model = self.get_request_model()
request = request_model(**arguments)
# Validate file paths
path_error = self.validate_file_paths(request)
if path_error:
error_output = ToolOutput(
status="error",
content=path_error,
content_type="text",
)
return [TextContent(type="text", text=error_output.model_dump_json())]
# Prepare the prompt
prompt = await self.prepare_prompt(request)

13
tools/chat.py
View File

@@ -20,8 +20,9 @@ class ChatRequest(ToolRequest):
...,
description="Your question, topic, or current thinking to discuss with Gemini",
)
context_files: Optional[List[str]] = Field(
default_factory=list, description="Optional files for context"
files: Optional[List[str]] = Field(
default_factory=list,
description="Optional files for context (must be absolute paths)",
)
@@ -49,10 +50,10 @@ class ChatTool(BaseTool):
"type": "string",
"description": "Your question, topic, or current thinking to discuss with Gemini",
},
"context_files": {
"files": {
"type": "array",
"items": {"type": "string"},
"description": "Optional files for context",
"description": "Optional files for context (must be absolute paths)",
},
"temperature": {
"type": "number",
@@ -83,8 +84,8 @@ class ChatTool(BaseTool):
user_content = request.prompt
# Add context files if provided
if request.context_files:
file_content, _ = read_files(request.context_files)
if request.files:
file_content, _ = read_files(request.files)
user_content = f"{request.prompt}\n\n=== CONTEXT FILES ===\n{file_content}\n=== END CONTEXT ==="
# Check token limits

5
tools/debug_issue.py
View File

@@ -23,7 +23,8 @@ class DebugIssueRequest(ToolRequest):
None, description="Stack trace, logs, or additional error context"
)
files: Optional[List[str]] = Field(
None, description="Files or directories that might be related to the issue"
None,
description="Files or directories that might be related to the issue (must be absolute paths)",
)
runtime_info: Optional[str] = Field(
None, description="Environment, versions, or runtime information"
@@ -63,7 +64,7 @@ class DebugIssueTool(BaseTool):
"files": {
"type": "array",
"items": {"type": "string"},
"description": "Files or directories that might be related to the issue",
"description": "Files or directories that might be related to the issue (must be absolute paths)",
},
"runtime_info": {
"type": "string",

7
tools/review_code.py
View File

@@ -16,7 +16,10 @@ from .base import BaseTool, ToolRequest
class ReviewCodeRequest(ToolRequest):
"""Request model for review_code tool"""
files: List[str] = Field(..., description="Code files or directories to review")
files: List[str] = Field(
...,
description="Code files or directories to review (must be absolute paths)",
)
review_type: str = Field(
"full", description="Type of review: full|security|performance|quick"
)
@@ -55,7 +58,7 @@ class ReviewCodeTool(BaseTool):
"files": {
"type": "array",
"items": {"type": "string"},
"description": "Code files or directories to review",
"description": "Code files or directories to review (must be absolute paths)",
},
"review_type": {
"type": "string",

334
tools/review_pending_changes.py Normal file
View File

@@ -0,0 +1,334 @@
"""
Tool for reviewing pending git changes across multiple repositories.
"""
import json
import os
import re
from typing import Any, Dict, List, Literal, Optional
from pydantic import Field
from config import MAX_CONTEXT_TOKENS
from prompts.tool_prompts import REVIEW_PENDING_CHANGES_PROMPT
from utils.git_utils import find_git_repositories, get_git_status, run_git_command
from utils.token_utils import estimate_tokens
from .base import BaseTool, ToolRequest
class ReviewPendingChangesRequest(ToolRequest):
"""Request model for review_pending_changes tool"""
path: str = Field(
...,
description="Starting directory to search for git repositories (must be absolute path).",
)
original_request: Optional[str] = Field(
None,
description="The original user request or ticket description for the changes. Provides critical context for the review.",
)
compare_to: Optional[str] = Field(
None,
description="Optional: A git ref (branch, tag, commit hash) to compare against. If not provided, reviews local staged and unstaged changes.",
)
include_staged: bool = Field(
True,
description="Include staged changes in the review. Only applies if 'compare_to' is not set.",
)
include_unstaged: bool = Field(
True,
description="Include uncommitted (unstaged) changes in the review. Only applies if 'compare_to' is not set.",
)
focus_on: Optional[str] = Field(
None,
description="Specific aspects to focus on (e.g., 'logic for user authentication', 'database query efficiency').",
)
review_type: Literal["full", "security", "performance", "quick"] = Field(
"full", description="Type of review to perform on the changes."
)
severity_filter: Literal["critical", "high", "medium", "all"] = Field(
"all",
description="Minimum severity level to report on the changes.",
)
max_depth: int = Field(
5,
description="Maximum depth to search for nested git repositories to prevent excessive recursion.",
)
temperature: Optional[float] = Field(
None,
description="Temperature for the response (0.0 to 1.0). Lower values are more focused and deterministic.",
ge=0.0,
le=1.0,
)
thinking_mode: Optional[Literal["minimal", "low", "medium", "high", "max"]] = Field(
None, description="Thinking depth mode for the assistant."
)
class ReviewPendingChanges(BaseTool):
"""Tool for reviewing pending git changes across multiple repositories."""
def get_name(self) -> str:
return "review_pending_changes"
def get_description(self) -> str:
return (
"REVIEW PENDING GIT CHANGES - Comprehensive pre-commit review of staged/unstaged changes "
"or branch comparisons across multiple repositories. Searches recursively for git repos "
"and analyzes diffs for bugs, security issues, incomplete implementations, and alignment "
"with original requirements. Perfect for final review before committing. "
"Triggers: 'review pending changes', 'check my changes', 'validate changes', 'pre-commit review'. "
"Use this when you want to ensure changes are complete, correct, and ready to commit."
)
def get_input_schema(self) -> Dict[str, Any]:
return self.get_request_model().model_json_schema()
def get_system_prompt(self) -> str:
return REVIEW_PENDING_CHANGES_PROMPT
def get_request_model(self):
return ReviewPendingChangesRequest
def get_default_temperature(self) -> float:
"""Use analytical temperature for code review."""
from config import TEMPERATURE_ANALYTICAL
return TEMPERATURE_ANALYTICAL
def _sanitize_filename(self, name: str) -> str:
"""Sanitize a string to be a valid filename."""
# Replace path separators and other problematic characters
name = name.replace("/", "_").replace("\\", "_").replace(" ", "_")
# Remove any remaining non-alphanumeric characters except dots, dashes, underscores
name = re.sub(r"[^a-zA-Z0-9._-]", "", name)
# Limit length to avoid filesystem issues
return name[:100]
async def prepare_prompt(self, request: ReviewPendingChangesRequest) -> str:
"""Prepare the prompt with git diff information."""
# Find all git repositories
repositories = find_git_repositories(request.path, request.max_depth)
if not repositories:
return "No git repositories found in the specified path."
# Collect all diffs directly
all_diffs = []
repo_summaries = []
total_tokens = 0
max_tokens = MAX_CONTEXT_TOKENS - 50000 # Reserve tokens for prompt and response
for repo_path in repositories:
repo_name = os.path.basename(repo_path) or "root"
repo_name = self._sanitize_filename(repo_name)
# Get status information
status = get_git_status(repo_path)
changed_files = []
# Process based on mode
if request.compare_to:
# Validate the ref
is_valid_ref, err_msg = run_git_command(
repo_path,
["rev-parse", "--verify", "--quiet", request.compare_to],
)
if not is_valid_ref:
repo_summaries.append(
{
"path": repo_path,
"error": f"Invalid or unknown git ref '{request.compare_to}': {err_msg}",
"changed_files": 0,
}
)
continue
# Get list of changed files
success, files_output = run_git_command(
repo_path,
["diff", "--name-only", f"{request.compare_to}...HEAD"],
)
if success and files_output.strip():
changed_files = [
f for f in files_output.strip().split("\n") if f
]
# Generate per-file diffs
for file_path in changed_files:
success, diff = run_git_command(
repo_path,
[
"diff",
f"{request.compare_to}...HEAD",
"--",
file_path,
],
)
if success and diff.strip():
# Format diff with file header
safe_file_name = self._sanitize_filename(file_path)
diff_header = f"\n--- BEGIN DIFF: {repo_name} / {file_path} (compare to {request.compare_to}) ---\n"
diff_footer = f"\n--- END DIFF: {repo_name} / {file_path} ---\n"
formatted_diff = diff_header + diff + diff_footer
# Check token limit
diff_tokens = estimate_tokens(formatted_diff)
if total_tokens + diff_tokens <= max_tokens:
all_diffs.append(formatted_diff)
total_tokens += diff_tokens
else:
# Handle staged/unstaged changes
staged_files = []
unstaged_files = []
if request.include_staged:
success, files_output = run_git_command(
repo_path, ["diff", "--name-only", "--cached"]
)
if success and files_output.strip():
staged_files = [
f for f in files_output.strip().split("\n") if f
]
# Generate per-file diffs for staged changes
for file_path in staged_files:
success, diff = run_git_command(
repo_path, ["diff", "--cached", "--", file_path]
)
if success and diff.strip():
safe_file_name = self._sanitize_filename(file_path)
diff_header = f"\n--- BEGIN DIFF: {repo_name} / {file_path} (staged) ---\n"
diff_footer = f"\n--- END DIFF: {repo_name} / {file_path} ---\n"
formatted_diff = diff_header + diff + diff_footer
# Check token limit
from utils import estimate_tokens
diff_tokens = estimate_tokens(formatted_diff)
if total_tokens + diff_tokens <= max_tokens:
all_diffs.append(formatted_diff)
total_tokens += diff_tokens
if request.include_unstaged:
success, files_output = run_git_command(
repo_path, ["diff", "--name-only"]
)
if success and files_output.strip():
unstaged_files = [
f for f in files_output.strip().split("\n") if f
]
# Generate per-file diffs for unstaged changes
for file_path in unstaged_files:
success, diff = run_git_command(
repo_path, ["diff", "--", file_path]
)
if success and diff.strip():
safe_file_name = self._sanitize_filename(file_path)
diff_header = f"\n--- BEGIN DIFF: {repo_name} / {file_path} (unstaged) ---\n"
diff_footer = f"\n--- END DIFF: {repo_name} / {file_path} ---\n"
formatted_diff = diff_header + diff + diff_footer
# Check token limit
from utils import estimate_tokens
diff_tokens = estimate_tokens(formatted_diff)
if total_tokens + diff_tokens <= max_tokens:
all_diffs.append(formatted_diff)
total_tokens += diff_tokens
# Combine unique files
changed_files = list(set(staged_files + unstaged_files))
# Add repository summary
if changed_files:
repo_summaries.append(
{
"path": repo_path,
"branch": status["branch"],
"ahead": status["ahead"],
"behind": status["behind"],
"changed_files": len(changed_files),
"files": changed_files[:20], # First 20 for summary
}
)
if not all_diffs:
return "No pending changes found in any of the git repositories."
# Build the final prompt
prompt_parts = []
# Add original request context if provided
if request.original_request:
prompt_parts.append(
f"## Original Request/Ticket\n\n{request.original_request}\n"
)
# Add review parameters
prompt_parts.append(f"## Review Parameters\n")
prompt_parts.append(f"- Review Type: {request.review_type}")
prompt_parts.append(f"- Severity Filter: {request.severity_filter}")
if request.focus_on:
prompt_parts.append(f"- Focus Areas: {request.focus_on}")
if request.compare_to:
prompt_parts.append(f"- Comparing Against: {request.compare_to}")
else:
review_scope = []
if request.include_staged:
review_scope.append("staged")
if request.include_unstaged:
review_scope.append("unstaged")
prompt_parts.append(
f"- Reviewing: {' and '.join(review_scope)} changes"
)
# Add repository summary
prompt_parts.append(f"\n## Repository Changes Summary\n")
prompt_parts.append(
f"Found {len(repo_summaries)} repositories with changes:\n"
)
for idx, summary in enumerate(repo_summaries, 1):
prompt_parts.append(f"\n### Repository {idx}: {summary['path']}")
if "error" in summary:
prompt_parts.append(f"⚠️ Error: {summary['error']}")
else:
prompt_parts.append(f"- Branch: {summary['branch']}")
if summary["ahead"] or summary["behind"]:
prompt_parts.append(
f"- Ahead: {summary['ahead']}, Behind: {summary['behind']}"
)
prompt_parts.append(f"- Changed Files: {summary['changed_files']}")
if summary["files"]:
prompt_parts.append("\nChanged files:")
for file in summary["files"]:
prompt_parts.append(f" - {file}")
if summary["changed_files"] > len(summary["files"]):
prompt_parts.append(
f" ... and {summary['changed_files'] - len(summary['files'])} more files"
)
# Add token usage summary
if total_tokens > 0:
prompt_parts.append(f"\nTotal diff tokens: ~{total_tokens:,}")
# Add the diff contents
prompt_parts.append("\n## Git Diffs\n")
if all_diffs:
prompt_parts.extend(all_diffs)
else:
prompt_parts.append("--- NO DIFFS FOUND ---")
# Add review instructions
prompt_parts.append("\n## Review Instructions\n")
prompt_parts.append(
"Please review these changes according to the system prompt guidelines. "
"Pay special attention to alignment with the original request, completeness of implementation, "
"potential bugs, security issues, and any edge cases not covered."
)
return "\n".join(prompt_parts)

5
tools/think_deeper.py
View File

@@ -27,7 +27,8 @@ class ThinkDeeperRequest(ToolRequest):
description="Specific aspects to focus on (architecture, performance, security, etc.)",
)
files: Optional[List[str]] = Field(
None, description="Optional file paths or directories for additional context"
None,
description="Optional file paths or directories for additional context (must be absolute paths)",
)
@@ -66,7 +67,7 @@ class ThinkDeeperTool(BaseTool):
"files": {
"type": "array",
"items": {"type": "string"},
"description": "Optional file paths or directories for additional context",
"description": "Optional file paths or directories for additional context (must be absolute paths)",
},
"temperature": {
"type": "number",