Skip to content

Conversation

@mdesmet
Copy link
Collaborator

@mdesmet mdesmet commented May 21, 2025

Important

Adds mcp command group with inspect-mcp-server functionality, updates version to 0.0.19, and modifies CI/CD configurations.

  • New Feature:
    • Adds mcp command group in mcp.py with inspect-mcp-server command to handle MCP server configurations.
    • Supports both servers and mcpServers naming conventions.
    • Prompts for input tokens and processes server configurations.
    • Outputs server tool information and copies to clipboard.
  • Version Update:
    • Bumps version from 0.0.18 to 0.0.19 in .bumpversion.cfg, setup.py, docs/conf.py, and __init__.py.
  • Dependencies:
    • Adds mcp~=1.9.0 and pyperclip~=1.8.2 to setup.py.
  • CI/CD:
    • Removes Python 3.9 and PyPy 3.9 jobs from github-actions.yml.

This description was created by Ellipsis for 0536ca5. You can customize this summary. It will automatically update as commits are pushed.

@mdesmet mdesmet requested a review from saravmajestic May 21, 2025 16:10
Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed 7eb88f2 in 1 minute and 20 seconds. Click for details.
  • Reviewed 66 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. setup.py:71
  • Draft comment:
    Minor: Added trailing comma in dependency list improves maintainability.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
2. src/datapilot/core/mcp_utils/mcp.py:39
  • Draft comment:
    Consider preserving a multi-line format for the click.prompt call for clarity, especially if additional options may be added in the future.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
3. src/datapilot/core/mcp_utils/mcp.py:20
  • Draft comment:
    The function name 'create_mcp_proxy' does not match the command name 'inspect-mcp-server'. Consider aligning them to avoid confusion.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. src/datapilot/core/mcp_utils/mcp.py:57
  • Draft comment:
    Using asyncio.run() inside a Click command can lead to issues in environments with an active event loop. Consider refactoring or documenting this limitation.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_eZPlcciACno3ey4v

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Caution

Changes requested ❌

Reviewed everything up to 55056b6 in 2 minutes and 5 seconds. Click for details.
  • Reviewed 134 lines of code in 4 files
  • Skipped 1 files when reviewing.
  • Skipped posting 5 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. setup.py:71
  • Draft comment:
    Dependency 'mcp~=1.9.0' has been added. If MCP features are optional, consider moving this dependency to extras_require to avoid installing it for users who do not use these commands.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
2. src/datapilot/core/mcp_utils/mcp.py:36
  • Draft comment:
    Input definitions expect an 'id' key. Without explicit validation, a missing 'id' will cause a KeyError. Consider adding validation for the expected keys.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The code does use dict.get() with defaults for optional fields but uses direct access for "id". Since "id" appears to be a required field for input definitions (it's used as the key in the inputs dict), this is actually reasonable - we want it to fail fast if a required field is missing. Adding validation would just move the error from a KeyError to a different error. The comment has a point about validation, but failing fast on missing required fields is a valid pattern. The current behavior (KeyError) already effectively validates the field. While explicit validation could make the error message more user-friendly, the current behavior is not incorrect - it appropriately fails when a required field is missing. The comment suggests adding unnecessary validation since the current behavior (failing on missing required fields) is appropriate.
3. src/datapilot/core/mcp_utils/mcp.py:66
  • Draft comment:
    The type hints using 'list[str]' and 'dict[str, str]' require Python 3.9+. Since the project supports Python 3.8, consider using 'typing.List' and 'typing.Dict' or add 'from future import annotations' for compatibility.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment raises a valid technical point about Python version compatibility. Type hints using built-in generics were indeed introduced in Python 3.9. If the project needs to support Python 3.8, this would be a real issue. However, I don't have clear evidence about which Python versions the project needs to support. Without seeing setup.py, pyproject.toml, or other configuration files, I can't verify the Python version requirements. I don't have concrete evidence about the project's Python version requirements. The comment could be making incorrect assumptions about version support. While the technical information about type hints is correct, without clear evidence of Python version requirements, keeping this comment risks introducing unnecessary changes. Following the principle of needing strong evidence to keep a comment, and given we lack clear evidence about Python version requirements, we should remove this comment.
4. src/datapilot/core/mcp_utils/mcp.py:10
  • Draft comment:
    Using 'logging.basicConfig' in a module can override logging settings from other parts of the application. Consider configuring logging in the application entry point to avoid unexpected logging behavior.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a legitimate concern - module-level logging configuration can cause issues in larger applications by overriding settings. However, this is a CLI tool (using click) that appears to be a standalone command, so it may actually be appropriate to configure logging at this level. The comment is somewhat speculative about potential issues rather than pointing out a definite problem. I could be wrong about this being a standalone CLI tool - it might be imported and used as a library. The logging configuration could cause real issues in that case. While that's true, our rules state we should not make speculative comments or comment unless there's clearly a code change required. The current logging setup may be intentional for this CLI tool. Delete the comment as it's speculative about potential issues rather than pointing out a definite problem that needs fixing.
5. src/datapilot/core/mcp_utils/mcp.py:57
  • Draft comment:
    Each server configuration is processed sequentially with individual calls to 'asyncio.run'. If multiple servers are configured, consider running the async tasks concurrently (e.g., using 'asyncio.gather') to improve performance.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50% None

Workflow ID: wflow_fwnNMM4lYZzClSAS

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

"""mcp specific commands."""


@mcp.command("inspect-mcp-server")
Copy link

Choose a reason for hiding this comment

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

The command name inspect-mcp-server does not match the function name create_mcp_proxy. Consider renaming one or the other for clarity and consistency.

Suggested change
@mcp.command("inspect-mcp-server")
@mcp.command("create-mcp-proxy")

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed a8bb136 in 1 minute and 30 seconds. Click for details.
  • Reviewed 21 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/datapilot/core/mcp_utils/mcp.py:125
  • Draft comment:
    The new args mapping replaces '${input:' with '${' unconditionally, dropping any substitution of user input. Confirm that output is meant to show template tokens rather than actual user values.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
2. src/datapilot/core/mcp_utils/mcp.py:129
  • Draft comment:
    The env mapping now uses a simple .replace to strip '${input:' without mapping user inputs, unlike the original nested loop. Ensure this intentional transformation meets your design requirements.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.

Workflow ID: wflow_LikCQy4agjHisATX

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed 8981ac3 in 51 seconds. Click for details.
  • Reviewed 95 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/datapilot/core/mcp_utils/mcp.py:16
  • Draft comment:
    Good cleanup removing the unnecessary empty parentheses in the InputParameter class definition.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
2. src/datapilot/core/mcp_utils/mcp.py:70
  • Draft comment:
    The single-line formatting of click.prompt is concise but may reduce readability if options increase. Consider using multi-line formatting for clarity.
  • Reason this comment was not posted:
    Confidence changes required: 30% <= threshold 50% None
3. src/datapilot/core/mcp_utils/mcp.py:95
  • Draft comment:
    Instead of using dict to convert the dataclass instance to a dict, consider using dataclasses.asdict() for improved clarity and potential future compatibility.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50% None
4. src/datapilot/core/mcp_utils/mcp.py:135
  • Draft comment:
    Using {e!s} to format the exception is acceptable; alternatively, using str(e) might be slightly clearer, though this change is largely stylistic.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_kBMajHdLtDbYACab

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed 4b6f47e in 38 seconds. Click for details.
  • Reviewed 16 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/datapilot/core/mcp_utils/mcp.py:127
  • Draft comment:
    Changed output 'env' from a dict to a list of {key, value} objects. Ensure that downstream consumers or tests expecting a dict are updated accordingly.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 40% <= threshold 50% The comment is asking the author to ensure that downstream consumers or tests are updated, which violates the rule against asking the author to ensure things are tested or reviewed. However, it does provide specific information about a change that could affect other parts of the codebase, which might be useful.

Workflow ID: wflow_NLq6IsMH3Gv0UWyy

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed 3e0add3 in 21 seconds. Click for details.
  • Reviewed 10 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/datapilot/core/mcp_utils/mcp.py:176
  • Draft comment:
    Good use of exception chaining ('from e') to preserve the original error context.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_9K2HaLGa8Ph2S5fG

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed 9087a20 in 1 minute and 4 seconds. Click for details.
  • Reviewed 78 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/github-actions.yml:12
  • Draft comment:
    Removed the Python 3.9 configurations (e.g. 'py39-pydantic28-cover' for ubuntu/windows/macos). Confirm that dropping Python 3.9 support is intentional.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. .github/workflows/github-actions.yml:127
  • Draft comment:
    Removed PyPy 3.9 configurations (e.g. 'pypy39-pydantic28-cover' and related entries). Verify that support for PyPy 3.9 is no longer required.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. .github/workflows/github-actions.yml:124
  • Draft comment:
    The 'check' and 'docs' matrix entries do not specify 'python_arch', yet the setup-python step references it. Consider adding a default (e.g. 'x64') if needed.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_xwvd6zYOtjRFf8Cj

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@anandgupta42 anandgupta42 merged commit 285fc83 into main May 22, 2025
14 of 38 checks passed
Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed 0536ca5 in 35 seconds. Click for details.
  • Reviewed 44 lines of code in 4 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .bumpversion.cfg:2
  • Draft comment:
    Version bump updated to 0.0.19—config appears correct.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
2. docs/conf.py:18
  • Draft comment:
    Documentation version updated to 0.0.19; looks consistent.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
3. setup.py:16
  • Draft comment:
    Package version bumped to 0.0.19; no issues found.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
4. src/datapilot/__init__.py:1
  • Draft comment:
    Module version correctly updated to 0.0.19.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_RhYvKhI8OOGfxdgE

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

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.

4 participants