feat: enhance review_changes with dynamic file requests
- Add instruction for Gemini to request files when needed - Add comprehensive tests for files parameter functionality - Test file request instruction presence/absence based on context - Run all tests, ruff, and black formatting Now review_changes can both accept context files and allow Gemini to request additional files during review for better validation. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
@@ -196,11 +196,11 @@ Author: {__author__}
|
|||||||
Configuration:
|
Configuration:
|
||||||
- Gemini Model: {GEMINI_MODEL}
|
- Gemini Model: {GEMINI_MODEL}
|
||||||
- Max Context: {MAX_CONTEXT_TOKENS:,} tokens
|
- Max Context: {MAX_CONTEXT_TOKENS:,} tokens
|
||||||
- Python: {version_info['python_version']}
|
- Python: {version_info["python_version"]}
|
||||||
- Started: {version_info['server_started']}
|
- Started: {version_info["server_started"]}
|
||||||
|
|
||||||
Available Tools:
|
Available Tools:
|
||||||
{chr(10).join(f" - {tool}" for tool in version_info['available_tools'])}
|
{chr(10).join(f" - {tool}" for tool in version_info["available_tools"])}
|
||||||
|
|
||||||
For updates, visit: https://github.com/BeehiveInnovations/gemini-mcp-server"""
|
For updates, visit: https://github.com/BeehiveInnovations/gemini-mcp-server"""
|
||||||
|
|
||||||
|
|||||||
@@ -46,6 +46,7 @@ class TestReviewChangesTool:
|
|||||||
assert request.review_type == "full"
|
assert request.review_type == "full"
|
||||||
assert request.severity_filter == "all"
|
assert request.severity_filter == "all"
|
||||||
assert request.max_depth == 5
|
assert request.max_depth == 5
|
||||||
|
assert request.files is None
|
||||||
|
|
||||||
def test_sanitize_filename(self, tool):
|
def test_sanitize_filename(self, tool):
|
||||||
"""Test filename sanitization"""
|
"""Test filename sanitization"""
|
||||||
@@ -252,3 +253,107 @@ class TestReviewChangesTool:
|
|||||||
assert "Severity Filter: high" in result
|
assert "Severity Filter: high" in result
|
||||||
assert "Focus Areas: error handling" in result
|
assert "Focus Areas: error handling" in result
|
||||||
assert "Reviewing: staged and unstaged changes" in result
|
assert "Reviewing: staged and unstaged changes" in result
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
@patch("tools.review_changes.find_git_repositories")
|
||||||
|
@patch("tools.review_changes.get_git_status")
|
||||||
|
@patch("tools.review_changes.run_git_command")
|
||||||
|
@patch("tools.review_changes.read_files")
|
||||||
|
async def test_files_parameter_with_context(
|
||||||
|
self,
|
||||||
|
mock_read_files,
|
||||||
|
mock_run_git,
|
||||||
|
mock_status,
|
||||||
|
mock_find_repos,
|
||||||
|
tool,
|
||||||
|
):
|
||||||
|
"""Test review with additional context files"""
|
||||||
|
mock_find_repos.return_value = ["/test/repo"]
|
||||||
|
mock_status.return_value = {
|
||||||
|
"branch": "main",
|
||||||
|
"ahead": 0,
|
||||||
|
"behind": 0,
|
||||||
|
"staged_files": ["file1.py"],
|
||||||
|
"unstaged_files": [],
|
||||||
|
}
|
||||||
|
|
||||||
|
# Mock git commands - need to match all calls in prepare_prompt
|
||||||
|
mock_run_git.side_effect = [
|
||||||
|
(True, "file1.py\n"), # staged files list
|
||||||
|
(True, "diff --git a/file1.py..."), # diff for file1.py
|
||||||
|
(True, ""), # unstaged files list (empty)
|
||||||
|
]
|
||||||
|
|
||||||
|
# Mock read_files
|
||||||
|
mock_read_files.return_value = (
|
||||||
|
"=== FILE: config.py ===\nCONFIG_VALUE = 42\n=== END FILE ===",
|
||||||
|
"config.py",
|
||||||
|
)
|
||||||
|
|
||||||
|
request = ReviewChangesRequest(
|
||||||
|
path="/absolute/repo/path",
|
||||||
|
files=["/absolute/repo/path/config.py"],
|
||||||
|
)
|
||||||
|
result = await tool.prepare_prompt(request)
|
||||||
|
|
||||||
|
# Verify context files are included
|
||||||
|
assert "## Context Files Summary" in result
|
||||||
|
assert "✅ Included: config.py" in result
|
||||||
|
assert "## Additional Context Files" in result
|
||||||
|
assert "=== FILE: config.py ===" in result
|
||||||
|
assert "CONFIG_VALUE = 42" in result
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
@patch("tools.review_changes.find_git_repositories")
|
||||||
|
@patch("tools.review_changes.get_git_status")
|
||||||
|
@patch("tools.review_changes.run_git_command")
|
||||||
|
async def test_files_request_instruction(
|
||||||
|
self,
|
||||||
|
mock_run_git,
|
||||||
|
mock_status,
|
||||||
|
mock_find_repos,
|
||||||
|
tool,
|
||||||
|
):
|
||||||
|
"""Test that file request instruction is added when no files provided"""
|
||||||
|
mock_find_repos.return_value = ["/test/repo"]
|
||||||
|
mock_status.return_value = {
|
||||||
|
"branch": "main",
|
||||||
|
"ahead": 0,
|
||||||
|
"behind": 0,
|
||||||
|
"staged_files": ["file1.py"],
|
||||||
|
"unstaged_files": [],
|
||||||
|
}
|
||||||
|
|
||||||
|
mock_run_git.side_effect = [
|
||||||
|
(True, "file1.py\n"), # staged files
|
||||||
|
(True, "diff --git a/file1.py..."), # diff for file1.py
|
||||||
|
(True, ""), # unstaged files (empty)
|
||||||
|
]
|
||||||
|
|
||||||
|
# Request without files
|
||||||
|
request = ReviewChangesRequest(path="/absolute/repo/path")
|
||||||
|
result = await tool.prepare_prompt(request)
|
||||||
|
|
||||||
|
# Should include instruction for requesting files
|
||||||
|
assert "If you need additional context files" in result
|
||||||
|
assert "standardized JSON response format" in result
|
||||||
|
|
||||||
|
# Request with files - should not include instruction
|
||||||
|
request_with_files = ReviewChangesRequest(
|
||||||
|
path="/absolute/repo/path", files=["/some/file.py"]
|
||||||
|
)
|
||||||
|
|
||||||
|
# Need to reset mocks for second call
|
||||||
|
mock_find_repos.return_value = ["/test/repo"]
|
||||||
|
mock_run_git.side_effect = [
|
||||||
|
(True, "file1.py\n"), # staged files
|
||||||
|
(True, "diff --git a/file1.py..."), # diff for file1.py
|
||||||
|
(True, ""), # unstaged files (empty)
|
||||||
|
]
|
||||||
|
|
||||||
|
# Mock read_files to return empty (file not found)
|
||||||
|
with patch("tools.review_changes.read_files") as mock_read:
|
||||||
|
mock_read.return_value = ("", "")
|
||||||
|
result_with_files = await tool.prepare_prompt(request_with_files)
|
||||||
|
|
||||||
|
assert "If you need additional context files" not in result_with_files
|
||||||
|
|||||||
@@ -270,35 +270,45 @@ class ReviewChanges(BaseTool):
|
|||||||
context_files_content = []
|
context_files_content = []
|
||||||
context_files_summary = []
|
context_files_summary = []
|
||||||
context_tokens = 0
|
context_tokens = 0
|
||||||
|
|
||||||
if request.files:
|
if request.files:
|
||||||
remaining_tokens = max_tokens - total_tokens
|
remaining_tokens = max_tokens - total_tokens
|
||||||
|
|
||||||
# Read context files with remaining token budget
|
# Read context files with remaining token budget
|
||||||
file_content, file_summary = read_files(request.files)
|
file_content, file_summary = read_files(request.files)
|
||||||
|
|
||||||
# Check if context files fit in remaining budget
|
# Check if context files fit in remaining budget
|
||||||
if file_content:
|
if file_content:
|
||||||
context_tokens = estimate_tokens(file_content)
|
context_tokens = estimate_tokens(file_content)
|
||||||
|
|
||||||
if context_tokens <= remaining_tokens:
|
if context_tokens <= remaining_tokens:
|
||||||
# Use the full content from read_files
|
# Use the full content from read_files
|
||||||
context_files_content = [file_content]
|
context_files_content = [file_content]
|
||||||
# Parse summary to create individual file summaries
|
# Parse summary to create individual file summaries
|
||||||
summary_lines = file_summary.split('\n')
|
summary_lines = file_summary.split("\n")
|
||||||
for line in summary_lines:
|
for line in summary_lines:
|
||||||
if line.strip() and not line.startswith('Total files:'):
|
if line.strip() and not line.startswith("Total files:"):
|
||||||
context_files_summary.append(f"✅ Included: {line.strip()}")
|
context_files_summary.append(f"✅ Included: {line.strip()}")
|
||||||
else:
|
else:
|
||||||
context_files_summary.append(f"⚠️ Context files too large (~{context_tokens:,} tokens, budget: ~{remaining_tokens:,} tokens)")
|
context_files_summary.append(
|
||||||
|
f"⚠️ Context files too large (~{context_tokens:,} tokens, budget: ~{remaining_tokens:,} tokens)"
|
||||||
|
)
|
||||||
# Include as much as fits
|
# Include as much as fits
|
||||||
if remaining_tokens > 1000: # Only if we have reasonable space
|
if remaining_tokens > 1000: # Only if we have reasonable space
|
||||||
truncated_content = file_content[:int(len(file_content) * (remaining_tokens / context_tokens) * 0.9)]
|
truncated_content = file_content[
|
||||||
context_files_content.append(f"\n--- BEGIN CONTEXT FILES (TRUNCATED) ---\n{truncated_content}\n--- END CONTEXT FILES ---\n")
|
: int(
|
||||||
|
len(file_content)
|
||||||
|
* (remaining_tokens / context_tokens)
|
||||||
|
* 0.9
|
||||||
|
)
|
||||||
|
]
|
||||||
|
context_files_content.append(
|
||||||
|
f"\n--- BEGIN CONTEXT FILES (TRUNCATED) ---\n{truncated_content}\n--- END CONTEXT FILES ---\n"
|
||||||
|
)
|
||||||
context_tokens = remaining_tokens
|
context_tokens = remaining_tokens
|
||||||
else:
|
else:
|
||||||
context_tokens = 0
|
context_tokens = 0
|
||||||
|
|
||||||
total_tokens += context_tokens
|
total_tokens += context_tokens
|
||||||
|
|
||||||
# Build the final prompt
|
# Build the final prompt
|
||||||
@@ -373,7 +383,9 @@ class ReviewChanges(BaseTool):
|
|||||||
# Add context files content if provided
|
# Add context files content if provided
|
||||||
if context_files_content:
|
if context_files_content:
|
||||||
prompt_parts.append("\n## Additional Context Files")
|
prompt_parts.append("\n## Additional Context Files")
|
||||||
prompt_parts.append("The following files are provided for additional context. They have NOT been modified.\n")
|
prompt_parts.append(
|
||||||
|
"The following files are provided for additional context. They have NOT been modified.\n"
|
||||||
|
)
|
||||||
prompt_parts.extend(context_files_content)
|
prompt_parts.extend(context_files_content)
|
||||||
|
|
||||||
# Add review instructions
|
# Add review instructions
|
||||||
@@ -384,4 +396,12 @@ class ReviewChanges(BaseTool):
|
|||||||
"potential bugs, security issues, and any edge cases not covered."
|
"potential bugs, security issues, and any edge cases not covered."
|
||||||
)
|
)
|
||||||
|
|
||||||
|
# Add instruction for requesting files if needed
|
||||||
|
if not request.files:
|
||||||
|
prompt_parts.append(
|
||||||
|
"\nIf you need additional context files to properly review these changes "
|
||||||
|
"(such as configuration files, documentation, or related code), "
|
||||||
|
"you may request them using the standardized JSON response format."
|
||||||
|
)
|
||||||
|
|
||||||
return "\n".join(prompt_parts)
|
return "\n".join(prompt_parts)
|
||||||
|
|||||||
@@ -173,7 +173,7 @@ def get_git_status(repo_path: str) -> Dict[str, any]:
|
|||||||
"rev-list",
|
"rev-list",
|
||||||
"--count",
|
"--count",
|
||||||
"--left-right",
|
"--left-right",
|
||||||
f'{status["branch"]}@{{upstream}}...HEAD',
|
f"{status['branch']}@{{upstream}}...HEAD",
|
||||||
],
|
],
|
||||||
)
|
)
|
||||||
if success:
|
if success:
|
||||||
|
|||||||
Reference in New Issue
Block a user