feat: close branch-list / push-delete / default-branch gaps (#171-#176)#174
Merged
Conversation
Three missing-capability issues addressed in one change. Issue #171 — register git_branch_list as a lean MCP tool - Extend existing _branch_ops.git_branch_list with schema requested in the issue: branch_type (local|remote|all), pattern, contains, merged. Return structured records (name/sha/is_current/upstream). - Legacy remote=/all= bools kept as aliases for back-compat; explicit conflict with branch_type surfaces as error text. - Add GitBranchList pydantic model and register in lean/registry_git. - 21 tests (test_git_branch_list.py rewritten). Issue #172 — default_branch passthrough on github_update_repo_settings - Add default_branch: str | None field to GitHubUpdateRepoSettings and forward to the PATCH /repos/{owner}/{name} payload. - Unblocks workflows that need to flip the repo's default branch (e.g. main -> development for sub-package conventions). - 1 new test verifies the payload entry. Issue #173 — delete + raw refspec support on git_push - Add delete: bool and refspec: str | None fields to GitPush. - model_validator enforces five mutual-exclusion rules; same guards mirrored in the handler in case lean dispatch skips pydantic. - delete=True dispatches `git push <remote> --delete <branch>`; refspec dispatches `git push <remote> <refspec>`. - Closes the circular dead-end where shell-runner refused `git push --delete` and the MCP tool didn't support it. - 16 new tests in test_git_push_delete.py.
|
Claude finished @MementoRC's task —— View job Code Review Complete ✅Reviewing PR #174 that closes issues #171, #172, and #173. This PR adds missing capabilities for branch listing, push deletion, and default branch settings. Review Tasks
🟢 Overall Assessment: APPROVEThis is a well-designed, thoroughly tested implementation that addresses three specific gaps in the MCP Git tooling. The code demonstrates excellent engineering practices with comprehensive validation, strong type safety, and extensive test coverage. 🚀 StrengthsExcellent Design & Architecture
Robust Implementation
Outstanding Test Coverage
📝 Code Quality Analysisgit/models.py:108-155 - GitPush ValidationEXCELLENT ✅ The
git/_branch_ops.py:121-142 - Legacy ResolutionGOOD ✅ The
git/_remote_ops.py:103-118 - Validation GuardsEXCELLENT ✅ Handler-level validation mirrors Pydantic model validation:
🔒 Security AssessmentNO SECURITY CONCERNS ✅
⚡ Performance ConsiderationsOPTIMAL ✅
🧪 Test Quality AssessmentComprehensive Coverage
Test Design Excellence
💡 Minor SuggestionsCode Style
Future Enhancements
✅ Specific ValidationsIssue #171 - Branch List Enhancement
Issue #172 - GitHub Default Branch
Issue #173 - Push Delete Support
🎯 RecommendationAPPROVE AND MERGE - This PR represents high-quality engineering work that successfully addresses all three issues with excellent attention to detail, comprehensive testing, and thoughtful design decisions. |
This was referenced May 20, 2026
Bundled into PR #174 to avoid staggering MCP server restarts. Issue #175 — git_branch_list sort - Add sort: str | None field to GitBranchList (e.g. '-committerdate', 'refname', 'authordate'); mirrors `git for-each-ref --sort=<key>`. - When set, branches are collected via for_each_ref with NUL-separated format; existing pattern/contains/merged filters apply post-fetch. - sort=None preserves the legacy collection path (no perf regression). - 10 new tests in TestGitBranchListSort. Issue #176 — git_push dry_run - Add dry_run: bool = False to GitPush; orthogonal to all existing mutual-exclusion rules (no validator changes). - Appends --dry-run to all four push paths: normal, force, delete, raw refspec. Return string carries a 'dry-run' marker. - 11 new tests in TestGitPushDryRun.
MementoRC
changed the title
|
Claude finished @MementoRC's task —— View job Code Review for PR #174 ✅Reviewed PR that closes issues #171, #172, #173, #175, and #176 - adding missing capabilities for branch listing, push deletion, and default branch settings. Review Tasks
🟢 Overall Assessment: APPROVEThis is an excellent implementation that addresses six specific gaps in the MCP Git tooling. The code demonstrates strong engineering practices with comprehensive validation, excellent test coverage, and thoughtful design decisions. 🚀 StrengthsExcellent Design & Architecture
Security & Input Validation
Outstanding Test Coverage
📋 Detailed Code Analysisgit/models.py:108-161 - GitPush ValidationEXCELLENT ✅ The @model_validator(mode="after")
def _validate_delete_and_refspec(self) -> "GitPush":
if self.delete:
if not self.branch:
raise ValueError("delete=True requires branch to be set")
# ... comprehensive mutual exclusion logic
git/_branch_ops.py:121-142 - Legacy ResolutionEXCELLENT ✅ The
git/_branch_ops.py:222-264 - Sorted Branch CollectionEXCELLENT ✅ The sorted branch implementation using
git/_remote_ops.py:108-123 - Handler Validation GuardsEXCELLENT ✅ Dual validation approach:
github/repos.py:139-140 - Default Branch AdditionSIMPLE & CORRECT ✅ Clean integration of if default_branch is not None:
payload["default_branch"] = default_branch⚡ Performance AnalysisOPTIMAL PERFORMANCE ✅
🧪 Test Quality AssessmentComprehensive & Well-Designed
Test Coverage Highlights
💡 Minor Enhancement OpportunitiesCode Organization
Future Considerations
✅ Issue Resolution ValidationIssue #171 - Branch List Enhancement ✅
Issue #172 - GitHub Default Branch ✅
Issue #173 - Push Delete Support ✅
Issue #175 - Sort Support ✅
Issue #176 - Dry Run Support ✅
🎯 RecommendationAPPROVE AND MERGE ✅ This PR represents high-quality engineering work that successfully addresses all stated issues. The implementation demonstrates:
The dual validation strategy, comprehensive test coverage, and careful attention to backward compatibility make this a exemplary contribution to the codebase. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Six small missing-capability issues bundled into one change. Originally three (#171/#172/#173); #175 and #176 were rolled in during review to avoid staggering MCP server restarts.
git_branch_listas a lean MCP tool; extend schema withbranch_type(local/remote/all),pattern,contains,merged. Legacyremote=/all=bools preserved as aliases.default_branch: str | Nonepassthrough ongithub_update_repo_settingsso callers can flip the repo default branch without falling back to the GitHub UI.delete: boolandrefspec: str | Nonefields togit_pushwith a pydanticmodel_validatorenforcing five mutual-exclusion rules. Resolves the circular dead-end where the shell-runner refusedgit push --deleteand the MCP tool didn't support it.sort: str | Nonetogit_branch_list(e.g.'-committerdate','refname'); when set, branches are collected viagit for-each-refwith the sort key applied natively. Filters (pattern/contains/merged) apply post-fetch.sort=Nonepreserves the legacy collection path.dry_run: bool = Falsetogit_push, orthogonal to every existing flag. Appends--dry-runto all four push paths (normal/force/delete/refspec) and surfaces a marker in the return string.Files changed
git/models.py,github/models.pygit/_branch_ops.py,git/_remote_ops.py,github/repos.pylean/registry_git.py(registersgit_branch_list)tests/unit/git/test_git_branch_list.py(rewritten + sort suite, 31 tests),tests/unit/git/test_git_push_delete.py(new, +dry_run suite, 27 tests),tests/unit/github/test_github_repo_settings.py(+1 test)Test plan
pixi run -e quality test-unit tests/unit/git/test_git_branch_list.py— 31 pass (21 original + 10 sort)pixi run -e quality test-unit tests/unit/git/test_git_push_delete.py tests/unit/git/test_git_push_force_with_lease.py— 35 pass (8 force_with_lease + 16 delete + 11 dry_run)pixi run -e quality test-unit tests/unit/github/test_github_repo_settings.py— 43 pass (1 new)Design notes
git_branch_listalready had a partial implementation (3-field model:repo_path,remote,all,pattern) that wasn't registered. Old bools kept as aliases; conflict with explicitbranch_typesurfaces as an error string (matches the tool's existing error-return style rather than raising).for-each-refwhensortis non-None; the legacyrepo.branches/repo.remote().refsiteration path is untouched, so unsorted callers see zero perf delta.--dry-runis a generalgit pushflag that works with every other push mode, so coupling it to delete-only would be artificial.44 vs 30 tool count inconsistencymentioned as an aside in Missing tool: enumerate local branches (git branch --list / git for-each-ref) #171 is not addressed here — separate bug worth its own issue.