Skip to content

Conversation

@ryoppippi
Copy link
Member

@ryoppippi ryoppippi commented Nov 11, 2025

Implements fetch_tools() method with MCP-backed dynamic tool discovery, matching the Node SDK implementation.

Related: StackOneHQ/stackone-ai-node#114

Copilot AI review requested due to automatic review settings November 11, 2025 15:52
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 implements MCP (Model Context Protocol) integration for dynamic tool discovery in the Python SDK, achieving feature parity with the Node SDK. The changes enable runtime tool fetching from the StackOne API without requiring SDK updates.

Key changes:

  • Added RPC-backed tool execution through the /actions/rpc endpoint via new _StackOneRpcTool class
  • Implemented MCP client integration with pagination support using streamablehttp_client transport
  • Added comprehensive test suite with 12 tests covering account, provider, and action filtering scenarios

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/test_toolset_mcp.py New comprehensive test suite with 12 tests organized into TestAccountFiltering and TestProviderAndActionFiltering classes
tests/test_toolset.py Removed obsolete non-MCP fetch_tools tests, cleaned up imports
stackone_ai/toolset.py Core MCP implementation including _StackOneRpcTool, _fetch_mcp_tools, thread-safe async wrapper, and enhanced fetch_tools() method
stackone_ai/feedback/tool.py Added type ignore comments for Pydantic field validators and improved code formatting
README.md Updated documentation with MCP feature information and installation instructions

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

try:
from mcp import types as mcp_types
from mcp.client.session import ClientSession
from mcp.client.streamable_http import streamablehttp_client
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

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

The import path mcp.client.streamable_http appears to be incorrect. Based on the MCP Python SDK package structure, this should be mcp.client.streamablehttp (without the underscore). The correct import should be from mcp.client.streamablehttp import streamablehttp_client.

Suggested change
from mcp.client.streamable_http import streamablehttp_client
from mcp.client.streamablehttp import streamablehttp_client

Copilot uses AI. Check for mistakes.
def runner() -> None:
try:
result["value"] = asyncio.run(awaitable)
except BaseException as exc: # pragma: no cover - surfaced in caller context
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

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

Except block directly handles BaseException.

Suggested change
except BaseException as exc: # pragma: no cover - surfaced in caller context
except Exception as exc: # pragma: no cover - surfaced in caller context

Copilot uses AI. Check for mistakes.
@ryoppippi ryoppippi force-pushed the ENG-11393-add-fetch-tools-in-python-sdk branch from f195fb5 to 764c149 Compare November 11, 2025 15:54
Add MCP (Model Context Protocol) integration to enable dynamic tool fetching
at runtime, matching the Node SDK functionality. This allows users to pull
the latest tool definitions from the StackOne API without SDK updates.

Key Features:
- Dynamic tool discovery via MCP server endpoint
- _StackOneRpcTool class for RPC-backed execution at /actions/rpc
- Account filtering with set_accounts() and account_ids parameter
- Provider filtering (case-insensitive, prefix-based)
- Action filtering with glob pattern support
- Thread-safe async execution wrapper (_run_async)
- Proper authentication with Basic Auth headers

Implementation Details:
- MCP client integration with streamablehttp transport
- Tool catalog fetching with pagination support (cursor-based)
- Schema normalization for proper nullable field handling
- RPC payload construction with body/headers/path/query support
- Account scoping via x-account-id header
- User-Agent tracking for SDK version telemetry

Dependencies:
- Requires optional 'mcp' extra: pip install 'stackone-ai[mcp]'
- Raises ToolsetConfigError if MCP dependencies not available

The implementation maintains API parity with the Node SDK's
stackone.mcp-fetch functionality while following Python idioms.
Add 12 new tests covering MCP-backed tool fetching to match Node SDK test
coverage in stackone.mcp-fetch.spec.ts. Tests are organized into logical
groups for account and provider/action filtering scenarios.

Test Coverage:

Account Filtering (TestAccountFiltering):
- fetch_tools_with_single_account_id: Single account filtering
- fetch_tools_with_multiple_account_ids: Multiple account filtering
- fetch_tools_with_set_accounts: Using set_accounts() method
- fetch_tools_account_ids_override_set_accounts: Parameter precedence
- fetch_tools_with_constructor_account_id: Constructor-based account
- fetch_tools_with_empty_account_ids: Empty list handling

Provider/Action Filtering (TestProviderAndActionFiltering):
- fetch_tools_with_provider_filter: Single provider filtering
- fetch_tools_with_multiple_providers: Multiple provider filtering
- fetch_tools_with_action_glob_pattern: Glob pattern matching
- fetch_tools_with_exact_action_match: Exact action name matching
- fetch_tools_with_provider_and_action_filters: Combined filtering
- fetch_tools_with_exclusion_pattern: Negative glob patterns

Removed:
- Deleted old fetch_tools tests that used non-MCP approach
- These tests were testing the old implementation that loaded from
  OpenAPI specs instead of the MCP server

The test suite uses comprehensive mocking of MCP client components:
- ClientSession for MCP protocol communication
- streamablehttp_client for HTTP transport
- Tool list responses with pagination support
- Proper async context manager handling

All tests verify:
- Correct tool count after filtering
- Proper tool presence/absence
- Account ID preservation in tool context
- Filter precedence and interaction
@ryoppippi ryoppippi force-pushed the ENG-11393-add-fetch-tools-in-python-sdk branch from 764c149 to 1cecc8a Compare November 11, 2025 15:55
@ryoppippi ryoppippi force-pushed the ENG-11393-add-fetch-tools-in-python-sdk branch from 1cecc8a to a127c6c Compare November 11, 2025 15:57
Copy link

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

No issues found across 4 files

Copy link
Contributor

@NicolasBelissent NicolasBelissent left a comment

Choose a reason for hiding this comment

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

LGTM

import threading
import warnings
from typing import Any
from collections.abc import Coroutine
Copy link
Contributor

Choose a reason for hiding this comment

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

never heard of these they good?

Copy link
Member Author

Choose a reason for hiding this comment

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

@ryoppippi ryoppippi merged commit d72ca80 into main Nov 12, 2025
6 checks passed
@ryoppippi ryoppippi deleted the ENG-11393-add-fetch-tools-in-python-sdk branch November 12, 2025 10:01
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.

3 participants