From 53303f86bec63cbe1f52c5fe5735fa73c42857a7 Mon Sep 17 00:00:00 2001 From: Fahad Date: Mon, 9 Jun 2025 21:43:45 +0400 Subject: [PATCH] feat: enhance review_changes with dynamic file requests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 --- server.py | 6 +- tests/test_review_changes.py | 105 +++++++++++++++++++++++++++++++++++ tools/review_changes.py | 42 ++++++++++---- utils/git_utils.py | 2 +- 4 files changed, 140 insertions(+), 15 deletions(-) diff --git a/server.py b/server.py index 5c464bb..1a28635 100644 --- a/server.py +++ b/server.py @@ -196,11 +196,11 @@ Author: {__author__} Configuration: - Gemini Model: {GEMINI_MODEL} - Max Context: {MAX_CONTEXT_TOKENS:,} tokens -- Python: {version_info['python_version']} -- Started: {version_info['server_started']} +- Python: {version_info["python_version"]} +- Started: {version_info["server_started"]} 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""" diff --git a/tests/test_review_changes.py b/tests/test_review_changes.py index 4185e16..a2b1220 100644 --- a/tests/test_review_changes.py +++ b/tests/test_review_changes.py @@ -46,6 +46,7 @@ class TestReviewChangesTool: assert request.review_type == "full" assert request.severity_filter == "all" assert request.max_depth == 5 + assert request.files is None def test_sanitize_filename(self, tool): """Test filename sanitization""" @@ -252,3 +253,107 @@ class TestReviewChangesTool: assert "Severity Filter: high" in result assert "Focus Areas: error handling" 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 diff --git a/tools/review_changes.py b/tools/review_changes.py index 762b691..2b47f69 100644 --- a/tools/review_changes.py +++ b/tools/review_changes.py @@ -270,35 +270,45 @@ class ReviewChanges(BaseTool): context_files_content = [] context_files_summary = [] context_tokens = 0 - + if request.files: remaining_tokens = max_tokens - total_tokens - + # Read context files with remaining token budget file_content, file_summary = read_files(request.files) - + # Check if context files fit in remaining budget if file_content: context_tokens = estimate_tokens(file_content) - + if context_tokens <= remaining_tokens: # Use the full content from read_files context_files_content = [file_content] # Parse summary to create individual file summaries - summary_lines = file_summary.split('\n') + summary_lines = file_summary.split("\n") 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()}") 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 if remaining_tokens > 1000: # Only if we have reasonable space - truncated_content = file_content[: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") + truncated_content = file_content[ + : 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 else: context_tokens = 0 - + total_tokens += context_tokens # Build the final prompt @@ -373,7 +383,9 @@ class ReviewChanges(BaseTool): # Add context files content if provided if context_files_content: 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) # Add review instructions @@ -384,4 +396,12 @@ class ReviewChanges(BaseTool): "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) diff --git a/utils/git_utils.py b/utils/git_utils.py index 87fead0..3c085d5 100644 --- a/utils/git_utils.py +++ b/utils/git_utils.py @@ -173,7 +173,7 @@ def get_git_status(repo_path: str) -> Dict[str, any]: "rev-list", "--count", "--left-right", - f'{status["branch"]}@{{upstream}}...HEAD', + f"{status['branch']}@{{upstream}}...HEAD", ], ) if success: