feat: add safe git_rm tool with single-file constraint

[MementoRC]

Summary

• Adds git_rm tool with safety-first design for LLM context
• Single file only (string param, not list) — prevents mass deletion
• Rejects wildcards, glob chars, ., .., trailing /, and shell injection characters
• Always inserts -- separator before file arg to prevent flag injection
• Supports --cached (remove from index only) and --dry-run (preview)

Files Changed

• src/mcp_server_git/git/models.py — GitRm model
• src/mcp_server_git/git/operations_extended.py — git_rm() implementation
• src/mcp_server_git/lean/registry_git.py — tool registration
• tests/unit/git/test_git_rm.py — 27 unit tests (7 success, 13 validation, 3 error handling + parametrized)

Test plan

• 27/27 unit tests passing locally
• CI passes on all platforms
• Live test via MCP discover + execute

🤖 Generated with Claude Code

[github-actions]

Claude finished @MementoRC's task —— View job

---

Code Review Complete ✅

I've thoroughly reviewed the git_rm tool implementation and it demonstrates excellent security-first design principles. Here's my comprehensive feedback:

Review Checklist:

• Analyze implementation in models.py
• Review operations_extended.py implementation
• Check registry integration
• Examine test coverage and quality
• Assess security considerations
• Evaluate performance implications
• Provide comprehensive feedback

🔒 Security (Excellent)

The implementation prioritizes security with multiple layers of validation:

Strengths:

• Single file constraint: file: str parameter prevents mass deletion (vs. list[str])
• Wildcard blocking: Rejects *, ?, [, ], {, } using _UNSAFE_PATH regex
• Directory safety: Blocks ., .., and trailing / paths
• Shell injection protection: DANGEROUS_CHARS regex blocks ;|&$()`
• Flag injection prevention: Always uses -- separator in src/mcp_server_git/git/operations_extended.py:249
• Input sanitization: Strips whitespace and validates non-empty paths

Minor suggestion: Consider adding validation for absolute paths (paths starting with /) to prevent accidental system file removal.

📋 Code Quality (Very Good)

Model Design (src/mcp_server_git/git/models.py:255-259):

• Clean Pydantic model with descriptive field documentation
• Proper defaults and type hints
• Follows existing patterns in the codebase

Implementation (src/mcp_server_git/git/operations_extended.py:215-264):

• Well-structured validation pipeline
• Clear error messages with emoji indicators
• Proper exception handling for both GitCommandError and generic exceptions
• Good separation of concerns with helper regex patterns

Registration (src/mcp_server_git/lean/registry_git.py:476-483):

• Consistent integration following existing tool patterns
• Proper complexity classification as "focused"

🧪 Test Coverage (Excellent)

The test suite in tests/unit/git/test_git_rm.py is comprehensive with 27 tests:

Coverage highlights:

• Success scenarios (7 tests): Basic removal, --cached, --dry-run, combinations, nested paths
• Validation (13 tests): Empty strings, whitespace, dangerous chars, wildcards using @pytest.mark.parametrize
• Error handling (3 tests): Git errors with bytes/string stderr, general exceptions

Test quality:

• Good use of mocking to isolate functionality
• Parametrized tests for comprehensive wildcard/character validation
• Clear assertions on both success indicators and git.rm call verification

⚡ Performance (Good)

• Lightweight regex validation with pre-compiled patterns
• Single file operation prevents performance issues from bulk operations
• Minimal overhead from validation checks

🔍 Potential Issues

Minor concerns:

1. Unicode handling: Consider testing with non-ASCII filenames
2. Path normalization: Paths like ./file.py or dir/../file.py might bypass some checks
3. Long path support: No explicit validation for filesystem path length limits

📝 Suggestions for Enhancement

1. Path normalization: Add os.path.normpath() before validation to handle relative path components
2. Unicode testing: Add test cases for files with non-ASCII characters
3. Documentation: Consider adding usage examples in docstring showing --cached and --dry-run use cases

🎯 Overall Assessment

This is a high-quality, security-focused implementation that successfully addresses the challenge of providing safe git rm functionality for LLM contexts. The single-file constraint and comprehensive validation make mass deletion nearly impossible while still providing necessary functionality.

Recommendation: ✅ Approve

The implementation demonstrates excellent defensive programming practices and thorough testing. The minor suggestions above are enhancements rather than blocking issues.

---

[github-actions]

Claude finished @MementoRC's task —— View job

---

PR Code Review

I'll review this git_rm tool implementation focusing on code quality, security, performance, and test coverage.

Review Checklist:

• Analyze git_rm model implementation
• Review operations_extended.py implementation
• Check registry integration
• Examine test coverage and quality
• Assess security considerations
• Evaluate performance implications
• Provide comprehensive feedback

🔒 Security Analysis: Excellent

The implementation demonstrates exceptional security-first design:

Comprehensive Input Validation:

• Single file constraint: file: str parameter prevents mass deletion scenarios (src/mcp_server_git/git/models.py:257)
• Wildcard blocking: _UNSAFE_PATH regex rejects *?[]{} characters (src/mcp_server_git/git/operations_extended.py:213)
• Directory traversal protection: Blocks ., .., trailing / (operations_extended.py:232-240)
• Shell injection prevention: DANGEROUS_CHARS regex blocks ;|&$() (operations_extended.py:14,245`)
• Flag injection protection: Always uses -- separator before filename (operations_extended.py:254-255)
• Path normalization: Uses os.path.normpath() to handle ./, ../, etc. (operations_extended.py:236)

Security Strengths:

• Whitespace stripping prevents bypass attempts
• Unicode filenames properly supported (test coverage includes résumé.txt, 日本語.csv)
• Proper error handling prevents information leakage

📋 Code Quality: Very Good

Model Design (src/mcp_server_git/git/models.py:255-259):

• Clean Pydantic model following established patterns
• Descriptive field documentation explaining constraints
• Sensible defaults for cached and dry_run flags

Implementation Structure (src/mcp_server_git/git/operations_extended.py:216-269):

• Clear validation pipeline with early returns
• Excellent error messages with emoji indicators for UX
• Proper separation of concerns with dedicated regex patterns
• Consistent exception handling for both GitCommandError and generic exceptions

Registry Integration (src/mcp_server_git/lean/registry_git.py:476-483):

• Follows existing tool registration patterns perfectly
• Appropriate complexity classification as "focused"
• Clear, concise description highlighting safety features

🧪 Test Coverage: Outstanding

The test suite (tests/unit/git/test_git_rm.py) provides comprehensive coverage with 27 well-structured tests:

Test Categories:

• Success scenarios (7 tests): Basic removal, --cached, --dry-run, flag combinations, nested paths, whitespace handling
• Input validation (13 tests): Empty inputs, wildcards, dangerous characters, directory patterns
• Error handling (3 tests): Git command errors, exception handling
• Unicode support (4 parametrized tests): Non-ASCII filenames

Test Quality Highlights:

• Excellent use of @pytest.mark.parametrize for comprehensive character validation
• Proper mocking isolates functionality from actual git operations
• Clear assertions verify both success indicators AND git command invocation
• Real-world Unicode test cases (Japanese, German umlauts, accents)

⚡ Performance: Efficient

• Lightweight regex validation with pre-compiled patterns
• Single file constraint prevents performance issues from bulk operations
• Minimal computational overhead from validation checks
• Early validation returns avoid unnecessary processing

🎯 Minor Enhancement Opportunities

1. Absolute path handling: Consider validating against absolute paths starting with / to prevent accidental system file removal
2. Path length limits: Could add filesystem path length validation for edge cases
3. Symlink considerations: No explicit handling of symbolic links (may be acceptable by design)

🔍 Potential Issues Found: None Critical

Architecture Decision Questions:

• The choice to reject all absolute paths vs. just / prefix could be documented
• Symlink handling policy could be clarified (current behavior likely correct)

📝 Code Style & Best Practices

Excellent adherence to:

• Consistent error message formatting with emoji prefixes
• Proper docstring documentation
• Clean separation of validation logic
• Following established codebase patterns
• PEP-8 compliant code structure

🏆 Overall Assessment

This is an exceptionally well-implemented security-focused tool that successfully addresses the challenge of providing safe git rm functionality in LLM contexts. The implementation demonstrates:

• Defense in depth: Multiple validation layers prevent various attack vectors
• Comprehensive testing: 27 tests covering success, validation, and error scenarios
• Clear documentation: Self-documenting code with excellent error messages
• Performance consciousness: Efficient validation without unnecessary overhead

Recommendation: ✅ Strong Approve

The implementation sets a high standard for security-conscious tool design. The single-file constraint combined with comprehensive input validation makes mass deletion virtually impossible while maintaining necessary functionality.

---

[github-actions]

Claude finished @MementoRC's task —— View job

---

PR Code Review

I've thoroughly reviewed this git_rm tool implementation and it demonstrates excellent security-first design principles.

Review Checklist:

• Analyze git_rm model implementation
• Review operations_extended.py implementation
• Check registry integration
• Examine test coverage and quality
• Assess security considerations
• Evaluate performance implications
• Provide comprehensive feedback

🔒 Security Analysis: Excellent

The implementation demonstrates exceptional security-first design with multiple validation layers:

Input Validation Strengths:

• Single file constraint: file: str parameter prevents mass deletion scenarios (src/mcp_server_git/git/models.py:257)
• Wildcard blocking: _UNSAFE_PATH regex rejects *?[]{} characters (src/mcp_server_git/git/operations_extended.py:213,246)
• Directory traversal protection: Blocks ., .., trailing / (operations_extended.py:232-240)
• Shell injection prevention: DANGEROUS_CHARS regex blocks ;|&$() (operations_extended.py:14,249`)
• Flag injection protection: Always uses -- separator before filename (operations_extended.py:258)
• Absolute path blocking: Rejects paths starting with / (operations_extended.py:242-244)
• Path normalization: Uses os.path.normpath() to handle ./, ../, etc. (operations_extended.py:236)

Security Architecture:

• Whitespace stripping prevents bypass attempts
• Proper Unicode support with comprehensive test coverage
• Clear validation order prevents edge case bypasses

📋 Code Quality: Very Good

Model Design (src/mcp_server_git/git/models.py:255-259):

• Clean Pydantic model following established patterns
• Descriptive field documentation explaining constraints
• Sensible defaults for cached and dry_run flags

Implementation Structure (src/mcp_server_git/git/operations_extended.py:216-273):

• Clear validation pipeline with descriptive error messages
• Excellent UX with emoji indicators (✅, ❌, 🔍)
• Proper separation of concerns with dedicated regex patterns
• Robust exception handling for both GitCommandError and general exceptions
• Good docstring with safety explanation

Registry Integration (src/mcp_server_git/lean/registry_git.py:476-483):

• Follows existing tool registration patterns perfectly
• Clear description highlighting safety features
• Appropriate complexity classification as "focused"

🧪 Test Coverage: Outstanding

The test suite (tests/unit/git/test_git_rm.py) provides comprehensive coverage with 27 well-structured tests:

Test Organization:

• Success scenarios (7 tests): Basic removal, --cached, --dry-run, flag combinations, nested paths, whitespace handling
• Input validation (13 tests): Empty inputs, wildcards, dangerous characters, directory patterns, absolute paths
• Error handling (3 tests): Git command errors with different stderr types, general exceptions
• Unicode support (4 parametrized tests): International filenames

Test Quality Highlights:

• Excellent use of @pytest.mark.parametrize for comprehensive validation coverage
• Proper mocking isolates functionality from actual git operations
• Clear assertions verify both success indicators AND git command invocation
• Real-world Unicode test cases (Japanese, German, French)

⚡ Performance: Efficient

• Lightweight regex validation with pre-compiled patterns
• Early validation returns avoid unnecessary processing
• Single file constraint prevents performance issues from bulk operations
• Minimal computational overhead from validation pipeline

🔍 Potential Issues: Minor

Architecture Considerations:

1. Symlink handling: No explicit validation for symbolic links (may be acceptable by design)
2. Case sensitivity: File system case-sensitivity differences not addressed (likely not needed)
3. Path length limits: No validation for filesystem path length constraints (edge case)

Code Style:

• All validation follows consistent error message patterns
• Good separation between validation and execution logic
• Clear variable naming and structure

📝 Suggestions for Enhancement

1. Documentation: Consider adding usage examples in the docstring
2. Error granularity: Could distinguish between "file not found" vs "permission denied" git errors
3. Validation order: Current order is logical and efficient

🏆 Overall Assessment

This is an exceptionally well-implemented security-focused tool that successfully addresses the challenge of providing safe git rm functionality in LLM contexts. The implementation demonstrates:

• Defense in depth: Multiple validation layers prevent various attack vectors
• Comprehensive testing: 27 tests covering success, validation, and error scenarios
• Clear UX: Excellent error messages with visual indicators
• Performance consciousness: Efficient validation without unnecessary overhead

Strengths Summary:

• Single-file constraint eliminates mass deletion risk
• Comprehensive input validation blocks all major attack vectors
• Excellent test coverage with real-world scenarios
• Clean, maintainable code following project patterns
• Outstanding security design suitable for LLM contexts

Recommendation: ✅ Strong Approve

The implementation sets a high standard for security-conscious tool design. The single-file constraint combined with comprehensive input validation makes accidental or malicious mass deletion virtually impossible while maintaining necessary functionality.

---