Skip to content

Conversation

@JoJoJoJoJoJoJo
Copy link
Contributor

@JoJoJoJoJoJoJo JoJoJoJoJoJoJo commented Jun 23, 2025

User description

Resolves #181


PR Type

Bug fix


Description

  • Add support for Goose builtin server configurations

  • Introduce CustomServerConfig for non-standard server types

  • Fix Pydantic validation errors in mcpm ls command

  • Update display and profile handling for custom configs


Changes walkthrough 📝

Relevant files
Bug fix
2 files
goose.py
Add builtin server type handling                                                 
+6/-1     
profile.py
Support CustomServerConfig in profile display                       
+3/-1     
Enhancement
2 files
schema.py
Add CustomServerConfig class definition                                   
+5/-1     
display.py
Add custom server config display                                                 
+10/-1   
Error handling
1 files
client_connection.py
Add custom config validation error                                             
+2/-0     
Tests
2 files
test_goose.py
Add comprehensive Goose client tests                                         
+87/-0   
test_windsurf.py
Add complete Windsurf client test suite                                   
[link]   

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 23, 2025 06:53
    Copilot AI review requested due to automatic review settings June 23, 2025 06:53
    @qodo-merge-pro
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    181 - PR Code Verified

    Compliant requirements:

    • Fix Pydantic validation error when running mcpm ls with Goose CLI provider
    • Handle Goose's default built-in extensions configuration that doesn't have command or url fields
    • Support configurations with type: builtin that only contain metadata fields

    Requires further human verification:

    • Verify that mcpm ls command works correctly with actual Goose CLI installation and builtin extensions
    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Runtime Error

    CustomServerConfig will cause runtime errors in router proxy functionality since it raises ValueError for custom configs, potentially breaking existing functionality that relies on router connections.

    else:
        raise ValueError(f"Custom server config {server_config.name} is not supported for router proxy")
    Data Loss Risk

    The normalization logic for builtin type only preserves the config in a nested structure, potentially losing important fields during round-trip conversions between client and server formats.

    if normalized.get("type") == "builtin":
        return {"config": normalized}

    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 resolves issue #181 by making the application compatible with goose builtin configurations. Key changes include updating tests to cover builtin and stdio server scenarios, introducing a CustomServerConfig into the schema and client conversions, and adding explicit error handling for unsupported custom configurations in the router proxy.

    Reviewed Changes

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

    Show a summary per file
    File Description
    tests/test_clients/test_goose.py Added tests for builtin and stdio server operations and config cleanup.
    src/mcpm/utils/display.py Extended display functionality to support CustomServerConfig.
    src/mcpm/router/client_connection.py Updated transport context to raise an error for unsupported custom configs.
    src/mcpm/core/schema.py Introduced CustomServerConfig and updated the ServerConfig union.
    src/mcpm/commands/profile.py Updated profile listing to handle CustomServerConfig instances.
    src/mcpm/clients/managers/goose.py Adjusted normalization and client conversion logic for builtin and custom configs.
    Comments suppressed due to low confidence (1)

    src/mcpm/clients/managers/goose.py:177

    • Overriding the "type" to "builtin" for CustomServerConfig in the client format conversion may cause confusion. Consider documenting this decision or using a distinct type to differentiate custom configs from built-in ones.
            elif isinstance(server_config, CustomServerConfig):
    

    @qodo-merge-pro
    Copy link
    Contributor

    CI Feedback 🧐

    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
    neither, 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.
    

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Jun 23, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Avoid mutating original configuration
    Suggestion Impact:The suggestion was directly implemented - the code was changed from `result = server_config.config` to `result = dict(server_config.config)` to create a copy and avoid mutating the original configuration

    code diff:

    -            result = server_config.config
    +            result = dict(server_config.config)

    Modifying the original config dictionary directly can cause side effects. Create
    a copy before adding the type field to avoid mutating the original
    configuration.

    src/mcpm/clients/managers/goose.py [177-179]

     elif isinstance(server_config, CustomServerConfig):
    -    result = server_config.config
    +    result = dict(server_config.config)
         result["type"] = "builtin"

    [Suggestion processed]

    Suggestion importance[1-10]: 7

    __

    Why: The suggestion correctly identifies a potential side effect. The original code mutates the server_config.config dictionary. Creating a copy with dict(server_config.config) prevents this mutation, improving the code's robustness and preventing unexpected behavior elsewhere.

    Medium
    • Update

    Co-authored-by: qodo-merge-pro[bot] <151058649+qodo-merge-pro[bot]@users.noreply.github.com>
    @github-actions
    Copy link
    Contributor

    Summary

    Adds CustomServerConfig for Goose “builtin” extensions and wires it through manager, CLI, router, and display utilities while expanding the test-suite (new Goose tests, Windsurf tests moved).

    Notable changes

    • core/schema.py: introduces CustomServerConfig and extends ServerConfig union.
    • clients/managers/goose.py: handles builtin→standard conversions and vice-versa.
    • CLI/display tweaks to recognise custom configs.
    • Router now blocks custom configs with a clear error.
    • 2 new test modules (test_goose.py, relocated test_windsurf.py).

    Review

    Nice, focused enhancement with solid test coverage. A couple of tiny nits to consider:

    • _normalize_server_config returns {"config": …} for builtin; clarify with a brief comment or docstring as the shape differs from the stdio/remote branches.
    • client_connection.py raises a generic ValueError; a custom exception (or NotImplementedError) might give clearer intent.

    Otherwise LGTM – great work!


    View workflow run

    @JoJoJoJoJoJoJo JoJoJoJoJoJoJo enabled auto-merge (squash) June 24, 2025 02:35
    @JoJoJoJoJoJoJo JoJoJoJoJoJoJo merged commit 8485c1b into main Jun 24, 2025
    6 checks passed
    @JoJoJoJoJoJoJo JoJoJoJoJoJoJo deleted the jonathan/fix-goose-builtin-config-compatible branch June 24, 2025 02:36
    mcpm-semantic-release bot pushed a commit that referenced this pull request Jun 24, 2025
    ## [1.13.6](v1.13.5...v1.13.6) (2025-06-24)
    
    ### Bug Fixes
    
    * compatible with goose builtin config ([#185](#185)) ([8485c1b](8485c1b))
    @mcpm-semantic-release
    Copy link

    🎉 This PR is included in version 1.13.6 🎉

    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]: mcpm ls is broken by default for Goose Default configuration

    3 participants