Skip to content

Revert "test(mcp): verify scoped execution and OAuth credential isolation" - #42039

Merged
joshua-berri merged 1 commit into
mainfrom
revert-41731-litellm_mcp_integration_regressions_4506
Sep 19, 2026
Merged

joshua-berri merged 1 commit into
mainfrom
revert-41731-litellm_mcp_integration_regressions_4506

Conversation

@mateo-berri

Copy link
Copy Markdown
Contributor

Reverts #41731

@mateo-berri
mateo-berri requested review from a team and joshua-berri September 19, 2026 23:13
@greptile-apps

greptile-apps Bot commented Sep 19, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 4/5

The production code is unchanged, but explicit repository test-quality requirements must be satisfied before merging this revert

Findings

  1. P2 Security coverage removed ▶
  2. P2 Mocks replace production dispatch ▶
  3. P2 Hook parameters remain untyped ▶
  4. P2 Routine comments violate policy ▶

Summary

This PR reverts scoped MCP execution and OAuth-isolation regression coverage, restores two legacy mock-heavy hook suites, and switches test upstreams back to FastMCP

  • Removes gateway-level authorization, credential-isolation, and fail-closed scenarios
  • Restores hook and guardrail tests that primarily exercise test-defined implementations
  • Updates MCP test servers from MCPServer to FastMCP
  • Removes the corresponding integration contract mappings

Reviews (1) · Last reviewed commit: "Revert "test(mcp): verify scoped executi..."

@@ -1311,27 +1311,6 @@
],
"tests/integration/cost_calculation/test_cost_tracking.py::test_case_bills_expected_cost[us.anthropic.claude-opus-5-v1:0-stream_full_usage]": [
"quota_management.spend_tracking.scripted_wire.logs_cost"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Security coverage removed These deletions remove unique permission, credential-isolation, and fail-closed gateway tests. Repository rules prohibit weakening existing coverage, so restore equivalent coverage before merging

Rule Used: What: Flag any modifications to existing tests and verify they don't weaken test coverage or mask regressions. Why: Developers may alter tests to make failing code pass rather than fix the actual bug, hiding regressions. Good: ``` // Test updated t... (source)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Comment on lines +155 to +195
class MockProxyLogging:
"""Mock proxy logging object for testing MCP guardrails"""

def __init__(self, guardrails: Optional[list] = None):
self.guardrails = guardrails if guardrails is not None else []
self.call_details = {"user_api_key_cache": DualCache()}
self.dynamic_success_callbacks = []
self.call_count = 0

def get_combined_callback_list(self, dynamic_success_callbacks, global_callbacks):
"""Return the guardrails for testing"""
return self.guardrails

def _convert_mcp_to_llm_format(self, request_obj, kwargs: dict) -> dict:
"""Convert MCP tool call to LLM message format"""
tool_call_content = (
f"Tool: {request_obj.tool_name}\nArguments: {request_obj.arguments}"
)

return {
"messages": [{"role": "user", "content": tool_call_content}],
"model": kwargs.get("model", "mcp-tool-call"),
"user_api_key_user_id": kwargs.get("user_api_key_user_id"),
"user_api_key_team_id": kwargs.get("user_api_key_team_id"),
}

def _convert_llm_result_to_mcp_response(self, llm_result, request_obj):
"""Convert LLM result back to MCP response format"""
return None # For testing, we don't need to convert back

def _parse_pre_mcp_call_hook_response(self, response, original_request):
"""Parse pre MCP call hook response"""
return response

async def async_pre_mcp_tool_call_hook(
self,
kwargs: dict,
request_obj: Any,
start_time: datetime,
end_time: datetime,
) -> Optional[Any]:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Mocks replace production dispatch These suites call test-owned dispatchers directly, so production MCP dispatch can break unnoticed. Meaningful production-facing tests are required before merging

Context Used: AGENTS.md (source)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Comment on lines +32 to +34
async def async_pre_mcp_tool_call_hook(
self, kwargs, request_obj: MCPPreCallRequestObject, start_time, end_time
) -> Optional[MCPPreCallResponseObject]:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Hook parameters remain untyped New hook methods leave parameters untyped and use Any, violating the requirement for strongly typed function parameters. Fix this before merging

Context Used: AGENTS.md (source)

Comment on lines +1 to +6
"""
Test file for MCP Hook Architecture

This file demonstrates the new MCP hook system with comprehensive examples
and validation tests.
"""

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Routine comments violate policy Both new test modules add extensive narration and routine docstrings, violating the repository comment policy. Remove unnecessary commentary before merging

Context Used: AGENTS.md (source)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@codecov

codecov Bot commented Sep 19, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@joshua-berri
joshua-berri merged commit 93ebc1e into main Sep 19, 2026
89 of 90 checks passed
@joshua-berri
joshua-berri deleted the revert-41731-litellm_mcp_integration_regressions_4506 branch September 19, 2026 23:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants