codeant-ai-for-open-source[bot] commented on code in PR #41607: URL: https://github.com/apache/superset/pull/41607#discussion_r3503620176
########## tests/unit_tests/mcp_service/test_tool_replaces.py: ########## @@ -0,0 +1,163 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Tests for the ``replaces`` parameter on the MCP ``@tool`` decorator. + +``replaces`` lets an extension swap out a host tool while keeping the same +name visible to the LLM: the original tool is removed from the registry and +the decorated function is registered under the replaced name, bypassing the +usual extension-namespace prefixing. +""" + +import logging +from typing import Any, Callable +from unittest.mock import MagicMock, patch + +import pytest + +from superset.core.mcp.core_mcp_injection import ( + _remove_tool_for_replacement, + _resolve_tool_name, + create_tool_decorator, +) +from superset.mcp_service.app import mcp as real_mcp + +_CONTEXT_PATCH_TARGET = ( + "superset.core.mcp.core_mcp_injection.get_current_extension_context" +) Review Comment: **Suggestion:** Add an explicit type annotation for this module-level constant to satisfy the type-hint requirement for annotatable variables. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> The new module-level constant is an annotatable variable and has no type hint, which matches the Python type-hint requirement for newly added code. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=25594d3f8704447e9331c9068e7cdb00&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=25594d3f8704447e9331c9068e7cdb00&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/mcp_service/test_tool_replaces.py **Line:** 38:40 **Comment:** *Custom Rule: Add an explicit type annotation for this module-level constant to satisfy the type-hint requirement for annotatable variables. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41607&comment_hash=3f06f70e15b3a8c09b7aedabcd57ca414961535fef7572a6240473d794bde03d&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41607&comment_hash=3f06f70e15b3a8c09b7aedabcd57ca414961535fef7572a6240473d794bde03d&reaction=dislike'>👎</a> ########## tests/unit_tests/mcp_service/test_tool_replaces.py: ########## @@ -0,0 +1,163 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Tests for the ``replaces`` parameter on the MCP ``@tool`` decorator. + +``replaces`` lets an extension swap out a host tool while keeping the same +name visible to the LLM: the original tool is removed from the registry and +the decorated function is registered under the replaced name, bypassing the +usual extension-namespace prefixing. +""" + +import logging +from typing import Any, Callable +from unittest.mock import MagicMock, patch + +import pytest + +from superset.core.mcp.core_mcp_injection import ( + _remove_tool_for_replacement, + _resolve_tool_name, + create_tool_decorator, +) +from superset.mcp_service.app import mcp as real_mcp + +_CONTEXT_PATCH_TARGET = ( + "superset.core.mcp.core_mcp_injection.get_current_extension_context" +) + + +# --------------------------------------------------------------------------- +# _resolve_tool_name +# --------------------------------------------------------------------------- + + +def test_resolve_tool_name_without_replaces_uses_prefixed_id() -> None: + """With no ``replaces``, the existing prefixing behavior is unchanged.""" + with patch(_CONTEXT_PATCH_TARGET, return_value=None): + tool_name, context_type = _resolve_tool_name("my_tool", None) + + assert tool_name == "my_tool" + assert context_type == "host" + + +def test_resolve_tool_name_with_replaces_bypasses_extension_prefix() -> None: + """``replaces`` is used verbatim even inside an active extension context, + so the decorated function overrides the host tool name rather than being + registered under an extension-namespaced name.""" + mock_context = MagicMock() Review Comment: **Suggestion:** Add a type annotation for this local mock variable since its type is known and can be declared. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> This new local variable is unannotated even though it is a clearly annotatable Python variable introduced in the PR, so it fits the type-hint rule. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6002f78d5dcc41688abfefb411a94fa1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=6002f78d5dcc41688abfefb411a94fa1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/mcp_service/test_tool_replaces.py **Line:** 61:61 **Comment:** *Custom Rule: Add a type annotation for this local mock variable since its type is known and can be declared. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41607&comment_hash=2a54d997bc8d7f4df55d691e906853367ba39cc99ac8ea851fa755951f3ce095&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41607&comment_hash=2a54d997bc8d7f4df55d691e906853367ba39cc99ac8ea851fa755951f3ce095&reaction=dislike'>👎</a> ########## tests/unit_tests/mcp_service/test_tool_replaces.py: ########## @@ -0,0 +1,163 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Tests for the ``replaces`` parameter on the MCP ``@tool`` decorator. + +``replaces`` lets an extension swap out a host tool while keeping the same +name visible to the LLM: the original tool is removed from the registry and +the decorated function is registered under the replaced name, bypassing the +usual extension-namespace prefixing. +""" + +import logging +from typing import Any, Callable +from unittest.mock import MagicMock, patch + +import pytest + +from superset.core.mcp.core_mcp_injection import ( + _remove_tool_for_replacement, + _resolve_tool_name, + create_tool_decorator, +) +from superset.mcp_service.app import mcp as real_mcp + +_CONTEXT_PATCH_TARGET = ( + "superset.core.mcp.core_mcp_injection.get_current_extension_context" +) + + +# --------------------------------------------------------------------------- +# _resolve_tool_name +# --------------------------------------------------------------------------- + + +def test_resolve_tool_name_without_replaces_uses_prefixed_id() -> None: + """With no ``replaces``, the existing prefixing behavior is unchanged.""" + with patch(_CONTEXT_PATCH_TARGET, return_value=None): + tool_name, context_type = _resolve_tool_name("my_tool", None) + + assert tool_name == "my_tool" + assert context_type == "host" + + +def test_resolve_tool_name_with_replaces_bypasses_extension_prefix() -> None: + """``replaces`` is used verbatim even inside an active extension context, + so the decorated function overrides the host tool name rather than being + registered under an extension-namespaced name.""" + mock_context = MagicMock() + mock_context.manifest.publisher = "acme" + mock_context.manifest.name = "analytics" + + with patch(_CONTEXT_PATCH_TARGET, return_value=mock_context): + tool_name, context_type = _resolve_tool_name("my_tool", "get_instance_info") + + assert tool_name == "get_instance_info" + assert context_type == "host" + + +# --------------------------------------------------------------------------- +# _remove_tool_for_replacement +# --------------------------------------------------------------------------- + + +def test_remove_tool_for_replacement_removes_existing_tool() -> None: + """The named tool is removed from the registry via ``mcp.remove_tool``.""" + mock_mcp = MagicMock() Review Comment: **Suggestion:** Add a type annotation for this mock variable to comply with the rule requiring hints on annotatable variables. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> This is another newly added, clearly annotatable local variable without a type hint, so the type-hint rule applies. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a0b862d593e640a4834b5393abef4b2b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=a0b862d593e640a4834b5393abef4b2b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/mcp_service/test_tool_replaces.py **Line:** 79:79 **Comment:** *Custom Rule: Add a type annotation for this mock variable to comply with the rule requiring hints on annotatable variables. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41607&comment_hash=b8198f029246eb6c19a73dd60b2307316cc9464335d393016e197536b20fbd90&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41607&comment_hash=b8198f029246eb6c19a73dd60b2307316cc9464335d393016e197536b20fbd90&reaction=dislike'>👎</a> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
