Skip to content

Conversation

@JoJoJoJoJoJoJo
Copy link
Contributor

@JoJoJoJoJoJoJo JoJoJoJoJoJoJo commented Jun 24, 2025

User description

  • Support custom node runtime config
  • Remove duplicated test fixture

Resolves #138


PR Type

Enhancement


Description

  • Add custom node runtime configuration support

  • Implement config set/get commands for node executables

  • Auto-replace node executables based on user config

  • Clean up test fixtures and improve test structure


Changes walkthrough 📝

Relevant files
Enhancement
3 files
config.py
Add config set/get commands for node executables                 
+29/-0   
common.py
Implement node executable replacement logic                           
+20/-2   
config.py
Define node executables and fix config initialization       
+3/-1     
Miscellaneous
1 files
scope.py
Remove Python version compatibility code                                 
+1/-13   
Tests
5 files
conftest.py
Refactor test fixtures and add client registry mocks         
+20/-5   
test_add.py
Remove duplicate mocks and add node config test                   
+42/-16 
test_windsurf.py
Remove duplicate config manager fixture                                   
+0/-16   
test_remove.py
Remove redundant client registry mocks                                     
+13/-21 
test_stash_pop.py
Clean up duplicate monkeypatch setup                                         
+1/-35   
Documentation
2 files
README.md
Document new config set/get commands                                         
+2/-0     
README.zh-CN.md
Add Chinese documentation for config commands                       
+2/-0     

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Jun 24, 2025

    CI Feedback 🧐

    (Feedback updated until commit f4b774a)

    A test triggered by this PR failed. Here is an AI-generated analysis of the failure:

    Action: codex

    Failed stage: Run Codex [❌]

    Failure summary:

    The action failed because the qodo-merge-pro[bot] user does not have sufficient permissions on the
    repository. The script checks if the bot has "admin" or "write" permissions, but it appears to have
    a different permission level (likely "read" or no permission), causing the script to exit with code
    1 at line 128.

    Relevant error logs:
    1:  ##[group]Runner Image Provisioner
    2:  Hosted Compute Agent
    ...
    
    119:  ##[endgroup]
    120:  ##[group]Run set -euo pipefail
    121:  �[36;1mset -euo pipefail�[0m
    122:  �[36;1m�[0m
    123:  �[36;1mPERMISSION=$(gh api \�[0m
    124:  �[36;1m  "/repos/${GITHUB_REPOSITORY}/collaborators/qodo-merge-pro[bot]/permission" \�[0m
    125:  �[36;1m  | jq -r '.permission')�[0m
    126:  �[36;1m�[0m
    127:  �[36;1mif [[ "$PERMISSION" != "admin" && "$PERMISSION" != "write" ]]; then�[0m
    128:  �[36;1m  exit 1�[0m
    129:  �[36;1mfi�[0m
    130:  shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
    131:  env:
    132:  GH_TOKEN: ***
    133:  ##[endgroup]
    134:  ##[error]Process completed with exit code 1.
    135:  Post job cleanup.
    

    @JoJoJoJoJoJoJo JoJoJoJoJoJoJo marked this pull request as ready for review June 24, 2025 06:21
    Copilot AI review requested due to automatic review settings June 24, 2025 06:21
    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 introduces support for custom node runtime configuration and cleans up duplicated test fixtures. Key changes include updating configuration commands and tests to reflect a user-specified node executable, as well as removing redundant monkeypatch setups in tests.

    Reviewed Changes

    Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

    Show a summary per file
    File Description
    tests/test_stash_pop.py Removed redundant monkeypatch setups and updated expected output.
    tests/test_remove.py Removed duplicate test fixture for mocking server not found.
    tests/test_clients/test_windsurf.py Removed unused config_manager fixture.
    tests/test_add.py Added tests verifying custom node executable configuration.
    tests/conftest.py Updated fixture setup to leverage monkeypatch for config manager.
    src/mcpm/utils/config.py Added NODE_EXECUTABLES configuration.
    src/mcpm/commands/target_operations/common.py Introduced node executable replacement logic in server config.
    src/mcpm/commands/config.py Added set and get commands for global configuration.
    README.zh-CN.md & README.md Updated documentation to include new configuration commands.
    Comments suppressed due to low confidence (1)

    tests/test_remove.py:42

    • The mocking of windsurf_manager.get_server appears redundant since it is already set earlier in the test. Consider removing the duplicate setup.
        windsurf_manager.get_server = Mock(return_value=None)
    

    @qodo-merge-pro
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Typo

    There's a typo in the console print message - "excutable" should be "executable"

    console.print(f"[bold cyan]Replace node excutable {command} with {config_node_executable}[/]")
    server_config.command = config_node_executable
    Input Validation

    The get command doesn't validate if the provided name parameter is in the list of supported configuration keys, which could lead to confusing error messages

    def get(name):
        """Get MCPM configuration."""
        config_manager = ConfigManager()
        current_config = config_manager.get_config()
        if name not in current_config:
            console.print(f"[red]Configuration '{name}' not set or not supported.[/]")
            return
        console.print(f"[green]{name}:[/] {current_config[name]}")
    Compatibility Issue

    Removing the Python 3.10 compatibility code for StrEnum without checking if this breaks support for older Python versions

    from enum import StrEnum

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Jun 24, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Organization
    best practice
    Add structured help with examples
    Suggestion Impact:The suggestion was directly implemented - the commit added structured help text with an example showing "mcpm config get node_executable" using the backslash-escaped block format as suggested

    code diff:

    -    """Get MCPM configuration."""
    +    """Get MCPM configuration.
    +    
    +    Example:
    +    
    +    \b
    +        mcpm config get node_executable
    +    """

    The set command lacks structured help text with examples. Add clear
    documentation with usage examples using backslash-escaped blocks for proper
    display. This helps users understand how to use the command effectively.

    src/mcpm/commands/config.py [26-29]

     @config.command()
     @click.help_option("-h", "--help")
     def set():
    -    """Set MCPM configuration."""
    +    """Set MCPM configuration.
    +    
    +    Example:
    +    
    +    \b
    +        mcpm config set
    +    """

    [Suggestion processed]

    Suggestion importance[1-10]: 6

    __

    Why:
    Relevant best practice - When implementing command-line interfaces with Click, use consistent help option patterns and provide clear, structured help text with examples. Include both short (-h) and long (--help) options, and format examples using backslash-escaped blocks for proper display.

    Low
    Add structured help with examples
    Suggestion Impact:The suggestion was directly implemented - the commit added structured help text with an example showing "mcpm config get node_executable" using the backslash-escaped block format as suggested

    code diff:

    -    """Get MCPM configuration."""
    +    """Get MCPM configuration.
    +    
    +    Example:
    +    
    +    \b
    +        mcpm config get node_executable
    +    """

    The get command lacks structured help text with examples. Add clear
    documentation with usage examples using backslash-escaped blocks for proper
    display. This helps users understand the expected argument format.

    src/mcpm/commands/config.py [40-44]

     @config.command()
     @click.argument("name", required=True)
     @click.help_option("-h", "--help")
     def get(name):
    -    """Get MCPM configuration."""
    +    """Get MCPM configuration.
    +    
    +    Example:
    +    
    +    \b
    +        mcpm config get node_executable
    +    """

    [Suggestion processed]

    Suggestion importance[1-10]: 5

    __

    Why:
    Relevant best practice - When implementing command-line interfaces with Click, use consistent help option patterns and provide clear, structured help text with examples. Include both short (-h) and long (--help) options, and format examples using backslash-escaped blocks for proper display.

    Low
    General
    Fix typo in console message
    Suggestion Impact:The suggestion was directly implemented - the typo "excutable" was corrected to "executable" in the console print statement

    code diff:

    -        console.print(f"[bold cyan]Replace node excutable {command} with {config_node_executable}[/]")
    +        console.print(f"[bold cyan]Replace node executable {command} with {config_node_executable}[/]")

    Fix the typo in the console message where "excutable" should be "executable".
    This improves the user experience by providing correct spelling in the output
    message.

    src/mcpm/commands/target_operations/common.py [35-48]

     def _replace_node_executable(server_config: ServerConfig) -> ServerConfig:
         if not isinstance(server_config, STDIOServerConfig):
             return server_config
         command = server_config.command.strip()
         if command not in NODE_EXECUTABLES:
             return server_config
         config = ConfigManager().get_config()
         config_node_executable = config.get("node_executable")
         if not config_node_executable:
             return server_config
         if config_node_executable != command:
    -        console.print(f"[bold cyan]Replace node excutable {command} with {config_node_executable}[/]")
    +        console.print(f"[bold cyan]Replace node executable {command} with {config_node_executable}[/]")
             server_config.command = config_node_executable
         return server_config

    [Suggestion processed]

    Suggestion importance[1-10]: 3

    __

    Why: The suggestion correctly identifies and fixes a typo ("excutable" to "executable") in a user-facing console message. This is a valid but low-impact cosmetic improvement.

    Low
    • Update

    JoJoJoJoJoJoJo and others added 4 commits June 24, 2025 14:25
    Co-authored-by: qodo-merge-pro[bot] <151058649+qodo-merge-pro[bot]@users.noreply.github.com>
    Co-authored-by: qodo-merge-pro[bot] <151058649+qodo-merge-pro[bot]@users.noreply.github.com>
    Co-authored-by: qodo-merge-pro[bot] <151058649+qodo-merge-pro[bot]@users.noreply.github.com>
    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

    @JoJoJoJoJoJoJo JoJoJoJoJoJoJo merged commit 3c77974 into main Jun 24, 2025
    6 checks passed
    @JoJoJoJoJoJoJo JoJoJoJoJoJoJo deleted the jonathan/feat-support-config-npx-runtime branch June 24, 2025 07:19
    mcpm-semantic-release bot pushed a commit that referenced this pull request Jun 24, 2025
    # [1.14.0](v1.13.6...v1.14.0) (2025-06-24)
    
    ### Features
    
    * support custom node runtime config ([#186](#186)) ([3c77974](3c77974))
    @mcpm-semantic-release
    Copy link

    🎉 This PR is included in version 1.14.0 🎉

    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.

    [General]: Configuring support for “bun x” instead of “npx”

    3 participants