Remove URL manipulation in MCPServerClient

[aponcedeleonch]

• Removed _prepare_url_for_proxy_mode method that appended /mcp or /sse paths
• Workload URLs now passed directly to underlying MCP clients without modification
• Eliminated temporary URL override logic in _execute_with_session
• Added 7 unit tests verifying URL remains unchanged during:
  • MCPServerClient initialization
  • list_tools operations (both streamable-http and SSE)
  • call_tool operations
  • Multiple consecutive operations
• All tests verify both the workload URL property and the URL passed to MCP clients

🤖 Generated with Claude Code

[claude]

PR Review: Remove URL manipulation in MCPServerClient

Summary

This PR removes the URL manipulation logic that was appending /mcp or /sse paths to workload URLs. The changes simplify the client code and add comprehensive test coverage.

Positive Aspects ✅

1. Code Simplification: Removing the _prepare_url_for_proxy_mode method and the temporary URL override logic significantly simplifies the code and reduces complexity
2. Excellent Test Coverage: 7 new tests comprehensively verify URL handling across different scenarios (initialization, operations, proxy modes, multiple operations)
3. Cleaner Architecture: URLs are now passed directly to underlying MCP clients without modification, making the behavior more transparent and predictable
4. No Side Effects: Workload URLs remain unchanged, preventing unexpected mutations

Code Quality Observations

Strengths:

• Tests follow consistent naming patterns and clear documentation
• Good use of mocking to isolate units under test
• Tests verify both workload state and client call arguments

Minor Suggestions:

1. Test Redundancy (Lines 241-273, 397-434):

   • test_workload_url_unchanged_after_init and test_workload_url_unchanged_with_proxy_mode are very similar - only difference is the proxy_mode field
   • test_workload_url_unchanged_with_proxy_mode_explicit duplicates some logic from test_workload_url_unchanged_during_list_tools_streamable
   • Consider consolidating or using pytest.mark.parametrize to reduce duplication

2. Test Organization (Lines 277-484):

   • The async tests for URL preservation could be parameterized by transport type (streamable vs SSE) to reduce code duplication
   • Example approach:

   @pytest.mark.parametrize("url,proxy_mode,client_mock", [
       ("http://localhost:8080/mcp/test-server", "streamable", "streamablehttp_client"),
       ("http://localhost:8080/sse/test-server", "sse", "sse_client"),
   ])
   async def test_workload_url_unchanged_during_list_tools(url, proxy_mode, client_mock):
       # consolidated test logic

3. Mock Setup Duplication:

   • The mock setup pattern is repeated across multiple tests
   • Consider extracting to a fixture or helper function

Potential Issues

Critical:

• ⚠️ Breaking Change Risk: If any existing deployments rely on the automatic /mcp or /sse path appending, they will break. The PR description doesn't mention migration strategy or backward compatibility
• Consider documenting what URLs should look like now (should they already include /mcp or /sse?)

Minor:

• The removed logging in _prepare_url_for_proxy_mode (lines 124, 128 in diff) provided visibility into URL modifications. Consider adding a debug log in _execute_with_session that shows the final URL being used

Security & Performance

• ✅ No security concerns identified
• ✅ Performance should improve slightly (fewer string operations, no temporary state modifications)
• ✅ Thread-safety improved by eliminating mutable state changes

Testing Recommendations

1. Run the full test suite to ensure no regressions in other components
2. Consider adding integration tests with actual MCP servers to verify end-to-end behavior
3. Test with real workload URLs to confirm they work without path manipulation

Questions

1. What should workload URLs look like now? Should they include the transport-specific paths (/mcp, /sse) in the stored URL?
2. Are there any existing workloads in production that might be affected by this change?
3. Should there be migration documentation or backwards compatibility handling?

Overall Assessment

Recommendation: Approve with minor concerns

The code changes are clean and well-tested. The main concern is ensuring this doesn't break existing deployments. If workload URLs are already stored with the correct paths, this is a great simplification. If not, you may need a migration strategy.

Score: 8/10

• Code Quality: 9/10
• Test Coverage: 10/10
• Documentation: 6/10 (missing migration notes)
• Risk Assessment: 7/10 (potential breaking change)

[aponcedeleonch]

@claude check again the newest commit. Verify if the fixes were applied. Make your comment short and precise

[claude]

Claude finished @aponcedeleonch's task —— View job

---

Review Complete ✓

• Read current test file
• Verify if test redundancy fixes were applied
• Provide assessment

Assessment

Fixes Applied Successfully ✅

The latest commit "Refactor URL preservation tests to reduce duplication" addresses the previous review concerns:

1. Test redundancy eliminated: Used @pytest.mark.parametrize to consolidate duplicated tests (lines 256-278, 281-324, 326-376)
2. Cleaner test organization: Three parameterized tests now cover what previously required 5+ separate test functions
3. Shared mock fixture: Created mock_mcp_session fixture (lines 241-253) to eliminate mock setup duplication

Result: Test coverage maintained while significantly reducing code duplication. Well done.

---

Job Run | Branch: avoid-url-mangling

[therealnb]

There were definitely cases where that URL mangling was required (that is why I put it in there, but it was questionable that was the right place for it to be sure).

I think it is fine to take it out from here and test further. We can fix the problems somewhere else.