Skip to content

Fix mcp-server-git unit tests, update build instructions #2470

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

evalstate
Copy link
Member

Make the unit tests detect git default branch name, update README with uv build/usage instructions for development.

Description

Server Details

  • Server: mcp-server-git
  • Changes to: tests, README

Motivation and Context

Prepare for security release

How Has This Been Tested?

Breaking Changes

No.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Protocol Documentation
  • My changes follows MCP security best practices
  • I have updated the server's README accordingly
  • I have tested this with an LLM client
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have documented all environment variables and configuration options

Additional context

I have not reviewed the preceding changes in detail.

…cy test advice.

2) Make the Unit Test insensitive to the git default branch name
@evalstate evalstate requested review from cliffhall and olaservo August 2, 2025 09:22
@olaservo olaservo added server-git Reference implementation for the Git MCP server - src/git enhancement New feature or request labels Aug 2, 2025
@olaservo
Copy link
Member

@claude could you review this pull request once the checks are all completed?

Copy link
Contributor

claude bot commented Aug 17, 2025

Claude finished @olaservo's task —— View job


PR Review Complete ✅

Reviewed PR #2470: "Fix mcp-server-git unit tests, update build instructions"

Review Summary

Overall Assessment

This is a well-executed PR that addresses the key issues mentioned in the description. All CI checks are passing, and the changes are focused and appropriate for preparing a security release.

Todo List:

  • Check CI status (All passing: Python, TypeScript, and PR checks)
  • Read repository guidelines (No CLAUDE.md found)
  • Review test changes for git default branch detection
  • Review README updates for uv build instructions
  • Analyze other file changes
  • Check for security concerns
  • Provide comprehensive review feedback

Key Changes Reviewed:

✅ Test Improvements (test_server.py)

  • Excellent fix: Lines 49-50 and 64-65 now properly detect the default branch name using test_repository.active_branch.name instead of hardcoding "main"
  • This makes tests more robust and will work regardless of git's default branch configuration
  • Clean implementation that follows the existing test patterns

✅ README Documentation (README.md)

  • Comprehensive update to development section with proper uv workflow
  • Clear instructions for virtual environment setup, testing, building, and running
  • Good addition of Python version pinning instructions
  • Maintains existing Docker and installation instructions while improving development guidance

✅ Configuration Updates

  • .python-version: Updated from 3.12 to 3.13 (appropriate upgrade)
  • pyproject.toml: Updated Python version requirements and dependencies
  • uv.lock: Updated dependency lock file (appropriate for dependency updates)

✅ Server Enhancement (server.py)

  • Added proper imports for Path and Optional types (lines 2-3)
  • Minor code quality improvements

Security Assessment

  • No security concerns identified
  • Changes are defensive and improve robustness
  • No sensitive information exposed
  • Dependency updates appear safe

Code Quality

  • All changes follow existing code patterns and conventions
  • Test changes are minimal and focused on the specific issue
  • README improvements enhance developer experience
  • Clean, well-structured changes throughout

Recommendations

Approve - This PR is ready to merge. The changes are:

  • Focused on the stated objectives
  • Well-implemented and tested
  • Improve code robustness and documentation
  • Pass all CI checks
  • Appropriate for a security release preparation

@olaservo
Copy link
Member

@claude is the .python-version: Updated from 3.12 to 3.13 (appropriate upgrade) necessary for the other changes being made here?

Copy link
Contributor

claude bot commented Aug 17, 2025

Claude finished @olaservo's task —— View job


Analyzing Python Version Upgrade Necessity

Investigating whether the Python 3.12 → 3.13 upgrade is necessary for the other changes in this PR.

Todo List:

  • Examine the changed files to understand dependencies
  • Check pyproject.toml for Python 3.13 requirements
  • Review test changes for version dependencies
  • Analyze README and other changes
  • Provide clear answer about necessity

Answer: No, the Python 3.13 upgrade is NOT necessary for the other changes.

Detailed Analysis

Key Findings:

  1. The code uses Python 3.10+ union syntax: The codebase extensively uses str | None and list[str] syntax (lines 43, 55, 117, 140, 182, 210 in server.py), but this syntax was introduced in Python 3.10, not 3.13.

  2. Original requirement was Python >=3.10: The previous pyproject.toml specified requires-python = ">=3.10", which already supported all the syntax being used.

  3. Test changes are version-agnostic: The test improvements (lines 49-50, 64-65 in test_server.py) that detect the default branch name using test_repository.active_branch.name work with any Python version that supports the existing GitPython library.

  4. No Python 3.13-specific features: None of the changes in this PR require Python 3.13-specific features. The changes are:

    • Dynamic git branch detection in tests (version-agnostic)
    • README documentation updates (no code impact)
    • Minor import additions (Path, Optional) that work in Python 3.10+
  5. Dependencies haven't changed significantly: While dependency versions were updated in pyproject.toml and uv.lock, none require Python 3.13 specifically.

Conclusion: The Python version upgrade from 3.12 to 3.13 appears to be an incidental change rather than a requirement. The PR's core objectives (fixing unit tests for git default branch detection and updating build instructions) can be accomplished with Python 3.10+.

Recommendation: The version bump could be reverted to maintain broader compatibility unless there's a specific organizational requirement to use Python 3.13.


@@ -1 +1 @@
3.10
3.13
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we revert this version bump if its not really needed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request server-git Reference implementation for the Git MCP server - src/git
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants