Skip to content

Conversation

@JoJoJoJoJoJoJo
Copy link
Contributor

@JoJoJoJoJoJoJo JoJoJoJoJoJoJo commented Jun 25, 2025

User description

Preparation for mcpm cli


PR Type

Enhancement


Description

  • Extract core router logic into reusable module

  • Move client connection handling to separate module

  • Consolidate log management utilities

  • Update import paths across codebase


Changes walkthrough 📝

Relevant files
Configuration changes
6 files
router.py
Update import path for log directory utility                         
+2/-1     
event.py
Update import path for router constants                                   
+1/-1     
app.py
Update import path for log directory utility                         
+1/-1     
sse_app.py
Update import path for log directory utility                         
+1/-1     
config.py
Remove router constants moved to core module                         
+0/-4     
platform.py
Remove log directory utility moved to core                             
+0/-32   
Enhancement
4 files
client_connection.py
Add new client connection management module                           
[link]   
router.py
Extract core router logic into standalone module                 
+377/-0 
log_manager.py
Move log directory utility and rename class                           
+34/-2   
router.py
Refactor to inherit from core router class                             
+34/-365
Tests
1 files
test_router.py
Update import paths for refactored modules                             
+6/-6     

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @JoJoJoJoJoJoJo JoJoJoJoJoJoJo marked this pull request as ready for review June 25, 2025 10:50
    Copilot AI review requested due to automatic review settings June 25, 2025 10:50
    Copy link
    Contributor

    Copilot AI left a comment

    Choose a reason for hiding this comment

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

    Pull Request Overview

    This PR refactors the router functionality by extracting core routing logic into the new mcpm/core/router module and updating associated references throughout the codebase. Key changes include updating import paths from mcpm/router to mcpm/core/router, moving log directory retrieval to the new log manager, and renaming internal handler functions (e.g. from get_active_servers to get_target_servers) for improved clarity in core logic.

    Reviewed Changes

    Copilot reviewed 10 out of 12 changed files in this pull request and generated no comments.

    Show a summary per file
    File Description
    tests/test_router.py Updated import paths and patched internal handler names.
    src/mcpm/utils/platform.py Removed get_log_directory now provided by the core log manager.
    src/mcpm/utils/config.py Removed splitor constants as part of the refactor.
    src/mcpm/router/sse_app.py Updated log manager import, reflecting new module organization.
    src/mcpm/router/router.py Extended core router functionality; replaced direct logic with calls to core methods.
    src/mcpm/monitor/event.py Updated splitor constants import to use new core router definitions.
    src/mcpm/core/utils/log_manager.py Introduced log manager with a renamed class (ServerLogManager).
    src/mcpm/core/router/router.py Extracted core router logic including server management and handler functions.
    src/mcpm/commands/router.py Updated log manager and platform imports to support changes.
    Comments suppressed due to low confidence (2)

    src/mcpm/router/router.py:121

    • The inner get_prompt function calls self.get_prompt which may cause confusion with the core method of the same name. Consider renaming the inner function or adding a clarifying comment to ensure the intended method is being invoked.
                result = await self.get_prompt(req.params, target_servers)
    

    src/mcpm/router/router.py:154

    • [nitpick] The log message prints 'target_servers', which is a list and may be ambiguous in context. It would be clearer to log the specific server or clarify that multiple target servers are being referenced.
                    logger.error(f"Error calling tool {req.params.name} on server {target_servers}: {e}")
    

    @qodo-merge-pro
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Exception Handling

    The exception handling in _server_lifespan_cycle uses exception groups syntax (except* Exception) which may not be compatible with all Python versions. Also, the fallback from streamablehttp to sse for RemoteServerConfig could mask important connection errors.

    except* HTTPStatusError as e:
        if isinstance(self.server_config, RemoteServerConfig):
            logger.warning(f"Failed to connect to server {self.server_config.name} with streamablehttp, try sse")
            await self._connect_to_server(_sse_transport_context(self.server_config))
        else:
            raise e
    except* Exception as e:
        logger.error(f"Failed to connect to server {self.server_config.name}: {e}")
        self._initialized_event.set()
        self._shutdown_event.set()
    Resource Conflict

    In the add_server method, when handling resource URI conflicts in auto mode, the code modifies the resource URI but then uses the modified URI as a key while storing the original resource object. This could lead to inconsistencies between the stored resource and its lookup key.

        )
    self.resources_mapping[str(resource_uri)] = resource
    self.capabilities_to_server_id["resources"][str(resource_uri)] = server_id
    Missing Headers

    In the StatusCodeResponse class, the raw_headers attribute is referenced but not defined in the class. This will cause an AttributeError when the response is used.

        "headers": self.raw_headers,
    }

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Jun 25, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Replace exception group syntax

    The exception group syntax except is only available in Python 3.11+. Use
    regular except clauses to maintain compatibility with older Python versions.
    *

    src/mcpm/core/router/client_connection.py [82-94]

     async def _server_lifespan_cycle(self):
         try:
             await self._connect_to_server(self._transport_context_factory(self.server_config))
    -    except* HTTPStatusError as e:
    +    except HTTPStatusError as e:
             if isinstance(self.server_config, RemoteServerConfig):
                 logger.warning(f"Failed to connect to server {self.server_config.name} with streamablehttp, try sse")
                 await self._connect_to_server(_sse_transport_context(self.server_config))
             else:
                 raise e
    -    except* Exception as e:
    +    except Exception as e:
             logger.error(f"Failed to connect to server {self.server_config.name}: {e}")
             self._initialized_event.set()
             self._shutdown_event.set()
    • Apply / Chat
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion correctly identifies that the except* syntax is only available in Python 3.11 and later. Replacing it with standard except clauses ensures broader compatibility with older Python versions, preventing potential SyntaxError exceptions in environments running Python < 3.11. This is a critical fix for code portability and robustness.

    Medium
    Organization
    best practice
    Update tool name during conflict resolution

    The conflict resolution implementation correctly follows the pattern but should
    update the tool object with the new name when auto-resolving conflicts. This
    ensures consistency between the mapping key and the tool's actual name property.

    src/mcpm/core/router/router.py [99-109]

     tool_name = tool.name
     if tool_name in self.capabilities_to_server_id["tools"]:
         if self.on_name_conflict == "strict":
             raise ValueError(
                 f"Tool {tool_name} already exists. Please use unique tool names across all servers."
             )
         else:
             # Auto resolve by adding server name prefix
             tool_name = f"{server_id}{TOOL_SPLITOR}{tool_name}"
    +        tool = tool.model_copy(update={"name": tool_name})
     self.capabilities_to_server_id["tools"][tool_name] = server_id
     self.tools_mapping[tool_name] = tool
    • Apply / Chat
    Suggestion importance[1-10]: 5

    __

    Why:
    Relevant best practice - When managing server configurations and capabilities across multiple servers, implement proper conflict resolution strategies for duplicate names by either using strict mode to raise errors or auto-resolving conflicts with server-specific prefixes.

    Low
    • Update

    @github-actions
    Copy link
    Contributor

    Summary

    Refactors the router by moving its core implementation to src/mcpm/core/router/, introduces McpRouterCore, renames errlog_managercore/utils/log_manager, and updates imports/tests and uv.lock accordingly (≈ 1 k LOC added, 0.9 k removed).

    Review

    Nice modularisation—separating protocol-agnostic logic from the HTTP layer makes the codebase cleaner and easier to reuse.

    • Public API is preserved via MCPRouter(McpRouterCore), good!
    • Consider exporting McpRouterCore in an __init__.py or docs so users can find it.
    • New core files lack module-level docstrings; adding them would aid maintainability.
    uv.lock churn is large—ensure it’s intentional and reproducible (same uv version).

    Otherwise the changes look solid and tests pass. 👍


    View workflow run

    Copy link
    Contributor

    @calmini calmini left a comment

    Choose a reason for hiding this comment

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

    LGTM

    @calmini calmini merged commit 89ab6ba into main Jun 26, 2025
    6 checks passed
    @calmini calmini deleted the jonathan/ref-move-router-core-logic-to-module branch June 26, 2025 03:07
    @mcpm-semantic-release
    Copy link

    🎉 This PR is included in version 1.14.1 🎉

    The release is available on GitHub release

    Your semantic-release bot 📦🚀

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    4 participants