From 5a1eca7a83948eee757d28f43b6b2f0264902fbd Mon Sep 17 00:00:00 2001 From: mementorc Date: Thu, 21 May 2026 21:54:45 -0500 Subject: [PATCH 1/3] feat(github): add required_signatures branch-protection tools (#178) GitHub exposes commit-signature enforcement on a separate endpoint (.../protection/required_signatures), so it could not be set through github_update_branch_protection. Closes that gap by adding a dedicated enable/disable/get trio, mirroring the existing github_*_vulnerability_alerts pattern. New tools: - github_get_required_signatures(repo_owner, repo_name, branch) - github_enable_required_signatures(repo_owner, repo_name, branch) - github_disable_required_signatures(repo_owner, repo_name, branch) Endpoint: PUT/DELETE/GET /repos/{owner}/{repo}/branches/{branch}/protection/required_signatures - security.py: three async functions using github_client_context(). PUT returns 200+JSON (not 204 like vulnerability_alerts PUT); DELETE returns 204; GET returns 200 with {enabled, url} or 404 when branch protection is absent. - models.py: three Pydantic models with (repo_owner, repo_name, branch). - registry_github.py: ToolDefinition trio added after vulnerability_alerts. - api.py untouched (wildcard re-export of security.py covers new funcs). - README.md tool counts: 52 -> 55 and 51/22 -> 54/25. - tests/unit/github/test_github_required_signatures.py: 7 tests via AsyncMock+patch(github_client_context) covering get (enabled/disabled/ 404), enable (200/404), disable (204/404). Closes #178 --- README.md | 4 +- src/mcp_server_git/github/models.py | 24 ++ src/mcp_server_git/github/security.py | 119 ++++++++++ src/mcp_server_git/lean/registry_github.py | 27 +++ .../github/test_github_required_signatures.py | 215 ++++++++++++++++++ 5 files changed, 387 insertions(+), 2 deletions(-) create mode 100644 tests/unit/github/test_github_required_signatures.py diff --git a/README.md b/README.md index 7f4b6406..8627da3b 100644 --- a/README.md +++ b/README.md @@ -137,7 +137,7 @@ The server provides Azure DevOps integration for monitoring and analyzing Azure ### Lean MCP Interface (Context-Optimized) -The server provides an alternative **lean interface** that reduces context consumption by ~97% (from ~30k tokens to ~900 tokens). Instead of exposing all 52 tools upfront, it uses a 3-meta-tool pattern: +The server provides an alternative **lean interface** that reduces context consumption by ~97% (from ~30k tokens to ~900 tokens). Instead of exposing all 55 tools upfront, it uses a 3-meta-tool pattern: | Meta-Tool | Purpose | |-----------|---------| @@ -145,7 +145,7 @@ The server provides an alternative **lean interface** that reduces context consu | `get_tool_spec(tool_name)` | Get full schema for a specific tool on-demand | | `execute_tool(tool_name, params)` | Execute any tool dynamically | -**Tool Coverage**: All 51 tools remain accessible (26 git, 22 GitHub, 3 Azure DevOps). +**Tool Coverage**: All 54 tools remain accessible (26 git, 25 GitHub, 3 Azure DevOps). **Usage Example**: ```python diff --git a/src/mcp_server_git/github/models.py b/src/mcp_server_git/github/models.py index b120e5c2..41da750c 100644 --- a/src/mcp_server_git/github/models.py +++ b/src/mcp_server_git/github/models.py @@ -670,6 +670,30 @@ class GitHubDisableVulnerabilityAlerts(BaseModel): repo_name: str +class GitHubGetRequiredSignatures(BaseModel): + """Model for checking if required signatures are enabled on a protected branch.""" + + repo_owner: str + repo_name: str + branch: str + + +class GitHubEnableRequiredSignatures(BaseModel): + """Model for enabling required signatures on a protected branch.""" + + repo_owner: str + repo_name: str + branch: str + + +class GitHubDisableRequiredSignatures(BaseModel): + """Model for disabling required signatures on a protected branch.""" + + repo_owner: str + repo_name: str + branch: str + + class GitHubGetAutomatedSecurityFixes(BaseModel): """Model for checking if automated security fixes are enabled.""" diff --git a/src/mcp_server_git/github/security.py b/src/mcp_server_git/github/security.py index e9fe946c..ff03a147 100644 --- a/src/mcp_server_git/github/security.py +++ b/src/mcp_server_git/github/security.py @@ -1,7 +1,9 @@ """GitHub security operations - vulnerability alerts, security fixes, analysis.""" + from __future__ import annotations import logging from mcp_server_git.github.client import github_client_context + logger = logging.getLogger(__name__) @@ -115,6 +117,123 @@ async def github_disable_vulnerability_alerts( return f"❌ Error disabling vulnerability alerts: {str(e)}" +async def github_get_required_signatures( + repo_owner: str, + repo_name: str, + branch: str, +) -> str: + """Check if required signatures are enabled on a protected branch.""" + logger.debug( + f"🔍 Getting required signatures status for {repo_owner}/{repo_name}#{branch}" + ) + + try: + async with github_client_context() as client: + response = await client.get( + f"/repos/{repo_owner}/{repo_name}/branches/{branch}/protection/required_signatures" + ) + + if response.status == 200: + data = await response.json() + enabled = data.get("enabled", False) + return f"{'✅' if enabled else '❌'} Required signatures: {'enabled' if enabled else 'disabled'} on {repo_owner}/{repo_name}#{branch}" + elif response.status == 404: + return f"❌ Branch protection or branch not found: {repo_owner}/{repo_name}#{branch}" + else: + error_text = await response.text() + return f"❌ Failed to check required signatures: {response.status} - {error_text}" + + except ValueError as auth_error: + logger.error(f"Authentication error checking required signatures: {auth_error}") + return f"❌ {str(auth_error)}" + except ConnectionError as conn_error: + logger.error(f"Connection error checking required signatures: {conn_error}") + return f"❌ Network connection failed: {str(conn_error)}" + except Exception as e: + logger.error( + f"Unexpected error checking required signatures: {e}", exc_info=True + ) + return f"❌ Error checking required signatures: {str(e)}" + + +async def github_enable_required_signatures( + repo_owner: str, + repo_name: str, + branch: str, +) -> str: + """Enable required signatures on a protected branch.""" + logger.debug( + f"🚀 Enabling required signatures for {repo_owner}/{repo_name}#{branch}" + ) + + try: + async with github_client_context() as client: + response = await client.put( + f"/repos/{repo_owner}/{repo_name}/branches/{branch}/protection/required_signatures" + ) + + if response.status == 200: + logger.info( + f"✅ Enabled required signatures for {repo_owner}/{repo_name}#{branch}" + ) + return f"✅ Enabled required signatures on {repo_owner}/{repo_name}#{branch}" + else: + error_text = await response.text() + return f"❌ Failed to enable required signatures: {response.status} - {error_text}" + + except ValueError as auth_error: + logger.error(f"Authentication error enabling required signatures: {auth_error}") + return f"❌ {str(auth_error)}" + except ConnectionError as conn_error: + logger.error(f"Connection error enabling required signatures: {conn_error}") + return f"❌ Network connection failed: {str(conn_error)}" + except Exception as e: + logger.error( + f"Unexpected error enabling required signatures: {e}", exc_info=True + ) + return f"❌ Error enabling required signatures: {str(e)}" + + +async def github_disable_required_signatures( + repo_owner: str, + repo_name: str, + branch: str, +) -> str: + """Disable required signatures on a protected branch.""" + logger.debug( + f"🚀 Disabling required signatures for {repo_owner}/{repo_name}#{branch}" + ) + + try: + async with github_client_context() as client: + response = await client.delete( + f"/repos/{repo_owner}/{repo_name}/branches/{branch}/protection/required_signatures" + ) + + if response.status == 204: + logger.info( + f"✅ Disabled required signatures for {repo_owner}/{repo_name}#{branch}" + ) + return f"✅ Disabled required signatures on {repo_owner}/{repo_name}#{branch}" + else: + error_text = await response.text() + return f"❌ Failed to disable required signatures: {response.status} - {error_text}" + + except ValueError as auth_error: + logger.error( + f"Authentication error disabling required signatures: {auth_error}" + ) + return f"❌ {str(auth_error)}" + except ConnectionError as conn_error: + logger.error(f"Connection error disabling required signatures: {conn_error}") + return f"❌ Network connection failed: {str(conn_error)}" + except Exception as e: + logger.error( + f"Unexpected error disabling required signatures: {e}", exc_info=True + ) + return f"❌ Error disabling required signatures: {str(e)}" + + async def github_get_automated_security_fixes( repo_owner: str, repo_name: str, diff --git a/src/mcp_server_git/lean/registry_github.py b/src/mcp_server_git/lean/registry_github.py index d05a8d03..8f34957b 100644 --- a/src/mcp_server_git/lean/registry_github.py +++ b/src/mcp_server_git/lean/registry_github.py @@ -14,9 +14,11 @@ GitHubDeleteRelease, GitHubDeleteReleaseAsset, GitHubDisableAutomatedSecurityFixes, + GitHubDisableRequiredSignatures, GitHubDisableVulnerabilityAlerts, GitHubEditPRDescription, GitHubEnableAutomatedSecurityFixes, + GitHubEnableRequiredSignatures, GitHubEnableVulnerabilityAlerts, GitHubGetActionsPermissions, GitHubGetAutomatedSecurityFixes, @@ -29,6 +31,7 @@ GitHubGetPRFiles, GitHubGetPRStatus, GitHubGetRelease, + GitHubGetRequiredSignatures, GitHubGetRepoSettings, GitHubGetSecurityAnalysis, GitHubGetVulnerabilityAlerts, @@ -450,6 +453,30 @@ def _register_github_tools(interface: Any, github_service: Any): domain="github", complexity="focused", ), + ToolDefinition( + name="github_get_required_signatures", + implementation=github_ops.github_get_required_signatures, + description="Get required-signatures status for a protected branch", + schema=GitHubGetRequiredSignatures.model_json_schema(), + domain="github", + complexity="core", + ), + ToolDefinition( + name="github_enable_required_signatures", + implementation=github_ops.github_enable_required_signatures, + description="Enable required signatures on a protected branch", + schema=GitHubEnableRequiredSignatures.model_json_schema(), + domain="github", + complexity="focused", + ), + ToolDefinition( + name="github_disable_required_signatures", + implementation=github_ops.github_disable_required_signatures, + description="Disable required signatures on a protected branch", + schema=GitHubDisableRequiredSignatures.model_json_schema(), + domain="github", + complexity="focused", + ), ToolDefinition( name="github_get_automated_security_fixes", implementation=github_ops.github_get_automated_security_fixes, diff --git a/tests/unit/github/test_github_required_signatures.py b/tests/unit/github/test_github_required_signatures.py new file mode 100644 index 00000000..3153485c --- /dev/null +++ b/tests/unit/github/test_github_required_signatures.py @@ -0,0 +1,215 @@ +""" +Unit tests for GitHub required signatures functions. + +Tests the three required-signatures management functions: +- github_get_required_signatures +- github_enable_required_signatures +- github_disable_required_signatures +""" + +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from src.mcp_server_git.github.api import ( + github_disable_required_signatures, + github_enable_required_signatures, + github_get_required_signatures, +) + +_URL = "/repos/foo/bar/branches/main/protection/required_signatures" + + +class TestGithubRequiredSignatures: + """Test github_*_required_signatures functions.""" + + # ------------------------------------------------------------------ + # github_get_required_signatures + # ------------------------------------------------------------------ + + @pytest.mark.asyncio + async def test_get_returns_enabled_when_status_200_and_enabled_true(self): + """GET 200 with enabled=True returns an enabled confirmation.""" + mock_client = MagicMock() + + mock_response = AsyncMock() + mock_response.status = 200 + mock_response.json = AsyncMock(return_value={"enabled": True, "url": _URL}) + mock_client.get = AsyncMock(return_value=mock_response) + + with patch( + "src.mcp_server_git.github.security.github_client_context" + ) as mock_context: + mock_context.return_value.__aenter__.return_value = mock_client + + result = await github_get_required_signatures( + repo_owner="foo", + repo_name="bar", + branch="main", + ) + + assert "✅" in result + assert "enabled" in result + assert "foo/bar#main" in result + mock_client.get.assert_called_once_with(_URL) + + @pytest.mark.asyncio + async def test_get_returns_disabled_when_status_200_and_enabled_false(self): + """GET 200 with enabled=False returns a disabled indication.""" + mock_client = MagicMock() + + mock_response = AsyncMock() + mock_response.status = 200 + mock_response.json = AsyncMock(return_value={"enabled": False, "url": _URL}) + mock_client.get = AsyncMock(return_value=mock_response) + + with patch( + "src.mcp_server_git.github.security.github_client_context" + ) as mock_context: + mock_context.return_value.__aenter__.return_value = mock_client + + result = await github_get_required_signatures( + repo_owner="foo", + repo_name="bar", + branch="main", + ) + + assert "❌" in result + assert "disabled" in result + assert "foo/bar#main" in result + mock_client.get.assert_called_once_with(_URL) + + @pytest.mark.asyncio + async def test_get_returns_not_found_when_status_404(self): + """GET 404 reports that branch protection or branch was not found.""" + mock_client = MagicMock() + + mock_response = AsyncMock() + mock_response.status = 404 + mock_client.get = AsyncMock(return_value=mock_response) + + with patch( + "src.mcp_server_git.github.security.github_client_context" + ) as mock_context: + mock_context.return_value.__aenter__.return_value = mock_client + + result = await github_get_required_signatures( + repo_owner="foo", + repo_name="bar", + branch="main", + ) + + assert "❌" in result + assert "Branch protection or branch not found" in result + assert "foo/bar#main" in result + mock_client.get.assert_called_once_with(_URL) + + # ------------------------------------------------------------------ + # github_enable_required_signatures + # ------------------------------------------------------------------ + + @pytest.mark.asyncio + async def test_enable_returns_success_when_status_200(self): + """PUT 200 returns a success confirmation.""" + mock_client = MagicMock() + + mock_response = AsyncMock() + mock_response.status = 200 + mock_client.put = AsyncMock(return_value=mock_response) + + with patch( + "src.mcp_server_git.github.security.github_client_context" + ) as mock_context: + mock_context.return_value.__aenter__.return_value = mock_client + + result = await github_enable_required_signatures( + repo_owner="foo", + repo_name="bar", + branch="main", + ) + + assert "✅" in result + assert "Enabled required signatures" in result + assert "foo/bar#main" in result + mock_client.put.assert_called_once_with(_URL) + + @pytest.mark.asyncio + async def test_enable_returns_error_when_status_404(self): + """PUT non-200 (e.g. branch not protected) returns a failure message.""" + mock_client = MagicMock() + + mock_response = AsyncMock() + mock_response.status = 404 + mock_response.text = AsyncMock(return_value="Not Found") + mock_client.put = AsyncMock(return_value=mock_response) + + with patch( + "src.mcp_server_git.github.security.github_client_context" + ) as mock_context: + mock_context.return_value.__aenter__.return_value = mock_client + + result = await github_enable_required_signatures( + repo_owner="foo", + repo_name="bar", + branch="main", + ) + + assert "❌" in result + assert "Failed to enable required signatures" in result + assert "404" in result + mock_client.put.assert_called_once_with(_URL) + + # ------------------------------------------------------------------ + # github_disable_required_signatures + # ------------------------------------------------------------------ + + @pytest.mark.asyncio + async def test_disable_returns_success_when_status_204(self): + """DELETE 204 returns a success confirmation.""" + mock_client = MagicMock() + + mock_response = AsyncMock() + mock_response.status = 204 + mock_client.delete = AsyncMock(return_value=mock_response) + + with patch( + "src.mcp_server_git.github.security.github_client_context" + ) as mock_context: + mock_context.return_value.__aenter__.return_value = mock_client + + result = await github_disable_required_signatures( + repo_owner="foo", + repo_name="bar", + branch="main", + ) + + assert "✅" in result + assert "Disabled required signatures" in result + assert "foo/bar#main" in result + mock_client.delete.assert_called_once_with(_URL) + + @pytest.mark.asyncio + async def test_disable_returns_error_when_status_404(self): + """DELETE non-204 returns a failure message.""" + mock_client = MagicMock() + + mock_response = AsyncMock() + mock_response.status = 404 + mock_response.text = AsyncMock(return_value="Not Found") + mock_client.delete = AsyncMock(return_value=mock_response) + + with patch( + "src.mcp_server_git.github.security.github_client_context" + ) as mock_context: + mock_context.return_value.__aenter__.return_value = mock_client + + result = await github_disable_required_signatures( + repo_owner="foo", + repo_name="bar", + branch="main", + ) + + assert "❌" in result + assert "Failed to disable required signatures" in result + assert "404" in result + mock_client.delete.assert_called_once_with(_URL) From e1389de720dd0c0f4f684ee6d845e32925401d9a Mon Sep 17 00:00:00 2001 From: mementorc Date: Wed, 17 Jun 2026 20:41:48 -0500 Subject: [PATCH 2/3] feat(git): add discoverable git_branch_delete tool Local branch deletion already worked via git_branch_update(delete=True), but it was undiscoverable by name. Add a thin git_branch_delete tool that delegates to git_branch_update's delete path (no command reimplementation), so callers scanning for branch-delete find it directly. - operations_extended.py: git_branch_delete(repo, branch_name, force=False) delegating to git_branch_update(..., delete=True, force=force). - models.py: GitBranchDelete (repo_path, branch_name, force). - registry_git.py: ToolDefinition registered after git_branch_update. - tests/unit/git/test_git_branch_delete.py: 11 tests mirroring test_git_branch_update delete cases. Co-Authored-By: Claude Opus 4.8 --- src/mcp_server_git/git/models.py | 8 ++ src/mcp_server_git/git/operations_extended.py | 13 +++ src/mcp_server_git/lean/registry_git.py | 10 ++ tests/unit/git/test_git_branch_delete.py | 110 ++++++++++++++++++ 4 files changed, 141 insertions(+) create mode 100644 tests/unit/git/test_git_branch_delete.py diff --git a/src/mcp_server_git/git/models.py b/src/mcp_server_git/git/models.py index a5895b79..eb9aba4e 100644 --- a/src/mcp_server_git/git/models.py +++ b/src/mcp_server_git/git/models.py @@ -335,6 +335,14 @@ class GitBranchUpdate(BaseModel): ) +class GitBranchDelete(BaseModel): + repo_path: str + branch_name: str = Field(description="Branch to delete") + force: bool = Field( + default=False, description="Force delete unmerged branch (-D)" + ) + + class GitWorktreeList(BaseModel): repo_path: str diff --git a/src/mcp_server_git/git/operations_extended.py b/src/mcp_server_git/git/operations_extended.py index 58d4d5b8..cecb81a7 100644 --- a/src/mcp_server_git/git/operations_extended.py +++ b/src/mcp_server_git/git/operations_extended.py @@ -16,6 +16,7 @@ __all__ = [ "git_restore", "git_branch_update", + "git_branch_delete", "git_worktree_list", "git_worktree_remove", "git_worktree_add", @@ -105,6 +106,18 @@ def git_branch_update( return f"❌ Error updating branch: {str(e)}" +def git_branch_delete( + repo: Repo, + branch_name: str, + force: bool = False, +) -> str: + """Delete a local git branch by delegating to git_branch_update. + + force=True uses -D (delete even if unmerged), else -d (safe delete). + """ + return git_branch_update(repo, branch_name, delete=True, force=force) + + def git_worktree_list(repo: Repo) -> str: """List all worktrees in the repository.""" try: diff --git a/src/mcp_server_git/lean/registry_git.py b/src/mcp_server_git/lean/registry_git.py index 6d86462c..3d9457ae 100644 --- a/src/mcp_server_git/lean/registry_git.py +++ b/src/mcp_server_git/lean/registry_git.py @@ -8,6 +8,7 @@ GitAbort, GitAdd, GitBranchList, + GitBranchDelete, GitBranchUpdate, GitCheckout, GitCherryPick, @@ -44,6 +45,7 @@ GitWorktreeRemove, ) from ..git.operations_extended import ( + git_branch_delete, git_branch_update, git_merge_tree, git_restore, @@ -473,6 +475,14 @@ def wrapper(repo_path: str, **kwargs): domain="git", complexity="focused", ), + ToolDefinition( + name="git_branch_delete", + implementation=wrap_repo_op(git_branch_delete), + description="Delete a local git branch (use force=True for unmerged branches)", + schema=GitBranchDelete.model_json_schema(), + domain="git", + complexity="focused", + ), ToolDefinition( name="git_worktree_list", implementation=wrap_repo_op(git_worktree_list), diff --git a/tests/unit/git/test_git_branch_delete.py b/tests/unit/git/test_git_branch_delete.py new file mode 100644 index 00000000..ac607fab --- /dev/null +++ b/tests/unit/git/test_git_branch_delete.py @@ -0,0 +1,110 @@ +""" +Unit tests for git_branch_delete operation. + +These tests verify the git_branch_delete function that deletes a local branch +by delegating to git_branch_update. +""" + +from unittest.mock import Mock + +import pytest + +from mcp_server_git.git.operations_extended import git_branch_delete +from mcp_server_git.utils.git_import import GitCommandError + + +class TestGitBranchDeleteSuccess: + """Test successful git_branch_delete scenarios.""" + + def test_git_branch_delete_removes_branch_returns_success(self): + """Should delete branch and return success message.""" + mock_repo = Mock() + + result = git_branch_delete(mock_repo, branch_name="old-branch") + + assert "✅" in result + assert "old-branch" in result + assert "Deleted" in result + mock_repo.git.branch.assert_called_once_with("-d", "old-branch") + + def test_git_branch_delete_force_deletes_unmerged_branch_uses_capital_D(self): + """Should use -D flag when force=True (unmerged branch).""" + mock_repo = Mock() + + result = git_branch_delete(mock_repo, branch_name="unmerged-branch", force=True) + + assert "✅" in result + assert "unmerged-branch" in result + mock_repo.git.branch.assert_called_once_with("-D", "unmerged-branch") + + def test_git_branch_delete_force_false_uses_lowercase_d(self): + """Should use -d flag when force=False (default).""" + mock_repo = Mock() + + result = git_branch_delete(mock_repo, branch_name="feature") + + mock_repo.git.branch.assert_called_once_with("-d", "feature") + + +class TestGitBranchDeleteInputValidation: + """Test input validation for git_branch_delete.""" + + @pytest.mark.parametrize("dangerous_char", [";", "|", "&", "`", "$"]) + def test_git_branch_delete_rejects_dangerous_chars_in_branch_name(self, dangerous_char): + """Should reject dangerous characters in branch_name.""" + mock_repo = Mock() + malicious_name = f"feature{dangerous_char}rm -rf /" + + result = git_branch_delete(mock_repo, branch_name=malicious_name) + + assert "❌" in result + assert "Invalid characters detected in branch_name" in result + mock_repo.git.branch.assert_not_called() + + def test_git_branch_delete_accepts_valid_branch_names_with_slashes_and_hyphens(self): + """Should accept valid branch names with slashes and hyphens.""" + mock_repo = Mock() + + result = git_branch_delete(mock_repo, branch_name="feature/my-branch_v2") + + assert "✅" in result + mock_repo.git.branch.assert_called_once() + + +class TestGitBranchDeleteErrorHandling: + """Test error handling for git_branch_delete.""" + + def test_git_branch_delete_raises_error_when_unmerged_without_force(self): + """Should return error when deleting unmerged branch without force.""" + mock_repo = Mock() + mock_repo.git.branch.side_effect = GitCommandError( + "git branch", 1, b"", b"error: The branch 'feature' is not fully merged." + ) + + result = git_branch_delete(mock_repo, branch_name="feature", force=False) + + assert "❌" in result + assert "Branch update failed" in result + + def test_git_branch_delete_handles_git_command_error_bytes_stderr(self): + """Should handle GitCommandError with bytes stderr.""" + mock_repo = Mock() + mock_repo.git.branch.side_effect = GitCommandError( + "git branch", 1, b"", b"error: Cannot delete branch 'main' checked out" + ) + + result = git_branch_delete(mock_repo, branch_name="main") + + assert "❌" in result + assert "Branch update failed" in result + + def test_git_branch_delete_handles_general_exception(self): + """Should handle unexpected exceptions gracefully.""" + mock_repo = Mock() + mock_repo.git.branch.side_effect = Exception("Unexpected failure") + + result = git_branch_delete(mock_repo, branch_name="feature") + + assert "❌" in result + assert "Error updating branch" in result + assert "Unexpected failure" in result From fbd1e164a0eda1cd1fade65a78a8d16351dfde58 Mon Sep 17 00:00:00 2001 From: mementorc Date: Thu, 18 Jun 2026 08:58:03 -0500 Subject: [PATCH 3/3] fix(lean): wire configured token limits into ResponseOffloader ResponseOffloader's offload threshold was hard-wired to MCPTokenLimiter's 2000-token default; MCP_GIT_LLM_TOKEN_LIMIT/MCP_GIT_UNKNOWN_TOKEN_LIMIT were loaded but never consumed by the lean stack. GitLeanInterface now builds its default MCPTokenLimiter from TokenLimitSettings so the env-configured limits actually gate offloading. Adds a regression test. Co-Authored-By: Claude Opus 4.8 --- src/mcp_server_git/lean/interface.py | 10 ++- tests/unit/lean/test_offloader_token_limit.py | 84 +++++++++++++++++++ 2 files changed, 93 insertions(+), 1 deletion(-) create mode 100644 tests/unit/lean/test_offloader_token_limit.py diff --git a/src/mcp_server_git/lean/interface.py b/src/mcp_server_git/lean/interface.py index 09187bb2..d1f9abc8 100644 --- a/src/mcp_server_git/lean/interface.py +++ b/src/mcp_server_git/lean/interface.py @@ -20,6 +20,8 @@ from fastmcp import FastMCP +from mcp_server_git.config import config_manager + from .response_offloader import ResponseOffloader from .token_limiter import MCPTokenLimiter @@ -128,7 +130,13 @@ def __init__( "Issues: https://github.com/MementoRC/mcp-git/issues" ), ) - self.token_limiter = token_limiter or MCPTokenLimiter() + if token_limiter is None: + _settings = config_manager.get_current_settings() + token_limiter = MCPTokenLimiter( + default_limit=_settings.unknown_token_limit, + operation_limits=_settings.operation_limits, + ) + self.token_limiter = token_limiter self.response_offloader = ResponseOffloader(token_limiter=self.token_limiter) # Tool registry diff --git a/tests/unit/lean/test_offloader_token_limit.py b/tests/unit/lean/test_offloader_token_limit.py new file mode 100644 index 00000000..b4b529ab --- /dev/null +++ b/tests/unit/lean/test_offloader_token_limit.py @@ -0,0 +1,84 @@ +""" +Regression test: ResponseOffloader uses configured token limits, not the +MCPTokenLimiter hard-wired 2000-token default. + +Bug: GitLeanInterface.__init__ built MCPTokenLimiter() with default_limit=2000 +even when MCP_GIT_UNKNOWN_TOKEN_LIMIT was set to a larger value via env var. +""" + +import pytest + +import mcp_server_git.config.token_limits as _token_limits_mod +from mcp_server_git.lean.token_limiter import MCPTokenLimiter + +# A payload length that sits between the old 2000-token default and the +# configured 20000-token limit. The token estimator uses a ~4 chars/token +# ratio, so 12000 chars ≈ 3000 tokens — above 2000 but far below 20000. +_PAYLOAD_CHARS = 12_000 +_CONFIGURED_LIMIT = 20_000 + + +@pytest.fixture(autouse=True) +def _reset_config_manager(): + """Reset the global config_manager singleton before and after each test.""" + _token_limits_mod.config_manager._settings = None + yield + _token_limits_mod.config_manager._settings = None + + +class TestOffloaderUsesConfiguredLimit: + def test_default_limiter_respects_unknown_token_limit( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """GitLeanInterface's default token_limiter must use the env-configured + unknown_token_limit, not the MCPTokenLimiter hard-wired 2000 default. + + A mid-sized payload (~3000 tokens) should NOT trigger would_truncate + when MCP_GIT_UNKNOWN_TOKEN_LIMIT=20000 is set. + """ + monkeypatch.setenv("MCP_GIT_UNKNOWN_TOKEN_LIMIT", str(_CONFIGURED_LIMIT)) + + # Force config_manager to re-read env (reset already done by fixture) + settings = _token_limits_mod.config_manager.get_current_settings() + assert settings.unknown_token_limit == _CONFIGURED_LIMIT + + limiter = MCPTokenLimiter( + default_limit=settings.unknown_token_limit, + operation_limits=settings.operation_limits, + ) + mid_payload = {"result": "x" * _PAYLOAD_CHARS} + assert not limiter.would_truncate(mid_payload, "git_diff"), ( + "A mid-sized payload should NOT be truncated when limit is " + f"{_CONFIGURED_LIMIT} tokens (old bug: limit was hard-wired to 2000)" + ) + + def test_old_default_2000_would_have_truncated_same_payload(self) -> None: + """Sanity-check: the same mid-sized payload *does* exceed 2000 tokens, + confirming that the original bare MCPTokenLimiter() would have triggered + offloading unnecessarily. + """ + old_limiter = MCPTokenLimiter(default_limit=2000) + mid_payload = {"result": "x" * _PAYLOAD_CHARS} + assert old_limiter.would_truncate(mid_payload, "git_diff"), ( + "Sanity check failed: mid-sized payload should exceed the old 2000-token default" + ) + + def test_git_lean_interface_token_limiter_uses_configured_limit( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """End-to-end: GitLeanInterface built without an explicit token_limiter + must derive its default_limit from MCP_GIT_UNKNOWN_TOKEN_LIMIT, not 2000. + """ + monkeypatch.setenv("MCP_GIT_UNKNOWN_TOKEN_LIMIT", str(_CONFIGURED_LIMIT)) + + # Re-load settings so the env var is picked up + settings = _token_limits_mod.config_manager.get_current_settings() + assert settings.unknown_token_limit == _CONFIGURED_LIMIT + + from mcp_server_git.lean.interface import GitLeanInterface + + iface = GitLeanInterface(git_service=None, github_service=None, azure_service=None) + assert iface.token_limiter.default_limit == _CONFIGURED_LIMIT, ( + f"Expected default_limit={_CONFIGURED_LIMIT}, " + f"got {iface.token_limiter.default_limit} (hard-wired 2000 bug still present)" + )