fastmcp icon indicating copy to clipboard operation
fastmcp copied to clipboard

[Draft] Convert BulkToolCaller to middleware

Open strawgate opened this issue 2 months ago โ€ข 5 comments

Summary

Converts the BulkToolCaller from an MCPMixin-based implementation to a ToolInjectionMiddleware, following the pattern established in #2142. This simplifies usage and aligns with other tool injection middleware like PromptToolMiddleware and ResourceToolMiddleware.

Changes

  • Created BulkToolCallerMiddleware extending ToolInjectionMiddleware
  • Added bulk_tool_caller_types.py to avoid circular imports
  • Deprecated original BulkToolCaller with clear migration guidance
  • Maintained backward compatibility by re-exporting types
  • Added comprehensive test suite for middleware

Usage

New approach:

mcp = FastMCP(middleware=[BulkToolCallerMiddleware()])

Old approach (deprecated):

bulk_tool_caller = BulkToolCaller()
bulk_tool_caller.register_tools(mcp)

Closes #2262

๐Ÿค– Generated with Claude Code) | View branch | [View job run](https://github.com/jlowin/fastmcp/actions/runs/18819279688

Summary by CodeRabbit

  • New Features

    • Added bulk tool calling: execute multiple tools or repeated calls with varied arguments; middleware exposes call_tools_bulk and call_tool_bulk with configurable error handling.
  • Deprecations

    • BulkToolCaller class marked deprecated; middleware is the recommended approach. Public request/result types re-exported for backward compatibility.
  • Tests

    • Comprehensive tests covering success, error handling, tag-filter behavior, no-return tools, and deprecation path.

strawgate avatar Oct 26 '25 14:10 strawgate

@coderabbitai please review

strawgate avatar Oct 27 '25 14:10 strawgate

@strawgate: I'll review the changes in this PR right away.

โœ… Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai[bot] avatar Oct 27 '25 14:10 coderabbitai[bot]

[!NOTE]

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

Converts the bulk tool caller into middleware: introduces BulkToolCallerMiddleware with injectable tools call_tools_bulk and call_tool_bulk, new Pydantic types CallToolRequest/CallToolRequestResult, deprecates the old BulkToolCaller while re-exporting types for backward compatibility, and adds tests covering behavior and error handling.

Changes

Cohort / File(s) Summary
New Middleware Types
src/fastmcp/server/middleware/bulk_tool_caller_types.py
Adds CallToolRequest and CallToolRequestResult Pydantic models; CallToolRequestResult.from_call_tool_result() converts a CallToolResult into the richer result that includes tool and arguments.
Middleware Implementation
src/fastmcp/server/middleware/bulk_tool_caller.py
Adds async call_tools_bulk() and call_tool_bulk() functions that sequentially invoke tools via the server tool manager, convert results to CallToolRequestResult, handle per-call exceptions, and honor continue_on_error. Adds BulkToolCallerMiddleware which injects these functions as tools.
Middleware Module Exports
src/fastmcp/server/middleware/__init__.py
Adds __getattr__ and __dir__ to lazily expose BulkToolCallerMiddleware and includes it in __all__ for discovery and type-checking friendliness.
Deprecated Public API
src/fastmcp/contrib/bulk_tool_caller/bulk_tool_caller.py
Introduces deprecated BulkToolCaller class that re-exports CallToolRequest/CallToolRequestResult from middleware types, emits deprecation warnings, updates register_tools() signature to accept mcp_server, and provides call_tools_bulk(), call_tool_bulk(), and _call_tool() methods that proxy to the new behavior.
Tests
tests/server/middleware/test_bulk_tool_caller.py
Adds comprehensive tests for middleware injection, single/multiple call scenarios, error handling with continue_on_error, behavior for tools returning no value, tag-filtering interactions, and the deprecated BulkToolCaller path including deprecation warnings.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant Server
    participant BulkMiddleware as BulkToolCallerMiddleware
    participant ToolManager

    rect rgb(230, 245, 255)
    Note over Client,ToolManager: Middleware flow (call_tool_bulk / call_tools_bulk)
    Client->>Server: invoke bulk tool (call_tool_bulk / call_tools_bulk)
    Server->>BulkMiddleware: dispatch to injected tool
    loop for each call
        BulkMiddleware->>ToolManager: call tool(name, arguments)
        alt success
            ToolManager-->>BulkMiddleware: ToolResult
            BulkMiddleware->>BulkMiddleware: convert ToolResult -> CallToolRequestResult
        else exception
            ToolManager-->>BulkMiddleware: Exception
            BulkMiddleware->>BulkMiddleware: create error CallToolRequestResult
            alt continue_on_error = false
                BulkMiddleware->>BulkMiddleware: break loop
            end
        end
    end
    BulkMiddleware-->>Server: list[CallToolRequestResult]
    Server-->>Client: aggregated results
    end

Estimated code review effort

๐ŸŽฏ 3 (Moderate) | โฑ๏ธ ~25 minutes

  • Review points:
    • Ensure CallToolRequestResult.from_call_tool_result correctly maps isError, content, _meta, and structuredContent.
    • Verify continue_on_error semantics stop iteration when False in both middleware functions.
    • Confirm __getattr__ lazy export does not create import cycles or hide errors in runtime import.
    • Check deprecated BulkToolCaller.register_tools signature and warning behavior for backward compatibility.

Poem

๐Ÿฐ
Tools in a row, I hop and I call,
Batch by batch, I catch each fall.
Old path bows out with a gentle chime,
Middleware hops in, one call at a time. โœจ

Pre-merge checks and finishing touches

โœ… Passed checks (5 passed)
Check name Status Explanation
Title Check โœ… Passed The pull request title "[Draft] Convert BulkToolCaller to middleware" clearly and concisely summarizes the primary change in the changeset. The title accurately describes the main objective of converting the BulkToolCaller from an MCPMixin-based implementation to a ToolInjectionMiddleware, which is the central focus of all modifications. The title is specific enough for developers scanning the repository history to understand the nature of the change without requiring additional context.
Linked Issues Check โœ… Passed The PR successfully addresses all coding requirements from linked issue #2262. The changes create BulkToolCallerMiddleware extending ToolInjectionMiddleware (#2262), implement bulk tool calling as injected middleware functions (call_tools_bulk and call_tool_bulk), maintain backward compatibility through re-exports and deprecation guidance, and include comprehensive test coverage validating the new middleware implementation. The new middleware usage pattern (FastMCP(middleware=[BulkToolCallerMiddleware()])) aligns with other tool injection middleware as requested in the issue, while the deprecated BulkToolCaller path remains functional with proper warnings.
Out of Scope Changes Check โœ… Passed All code changes in the pull request are directly related to the stated objective of converting BulkToolCaller to middleware. The modifications include the new middleware implementation (bulk_tool_caller.py), supporting types module (bulk_tool_caller_types.py), middleware initialization updates (middleware/init.py), deprecation of the original class (bulk_tool_caller/bulk_tool_caller.py), and comprehensive test coverage. No out-of-scope changes or unrelated functionality modifications were identified; all alterations support the core goal of migrating the BulkToolCaller from MCPMixin to ToolInjectionMiddleware pattern.
Description Check โœ… Passed The pull request description contains all essential information about the changes, including a summary section explaining the migration to middleware, a detailed list of changes made, usage examples comparing the new and deprecated approaches, and proper attribution to Claude Code as the AI tool used. Although the description does not strictly follow the template's checklist structure with checked boxes, it provides the critical information needed to understand the PR's purpose, scope, and implementation, including the issue reference (#2262) and testing approach (comprehensive test suite added).
Docstring Coverage โœ… Passed Docstring coverage is 89.66% which is sufficient. The required threshold is 80.00%.
โœจ Finishing touches
  • [ ] ๐Ÿ“ Generate docstrings
๐Ÿงช Generate unit tests (beta)
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment
  • [ ] Commit unit tests in branch claude/issue-2262-20251026-1426

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Oct 27 '25 14:10 coderabbitai[bot]

/marvin can you please address the PR feedback

strawgate avatar Oct 27 '25 15:10 strawgate

Claude finished @strawgate's task โ€”โ€” View job


Addressing PR Feedback โœ…

Successfully addressed all code review feedback from both reviewers.

Changes Made

  • [x] Fixed no_return_tool docstring to document None return value
  • [x] Updated from_call_tool_result to preserve _meta and structuredContent fields
  • [x] Removed unnecessary intermediate CallToolResult creation in both call_tools_bulk and call_tool_bulk
  • [x] Updated deprecated _call_tool method to use the helper properly
  • [x] Added TYPE_CHECKING import and __dir__ function for better IDE/type checker support
  • [x] Updated test snapshots to reflect proper structuredContent preservation

Summary

All reviewer feedback has been addressed:

  1. Field Preservation: The middleware now properly preserves structuredContent from tool results, ensuring no data is lost during conversion. The _meta field is also preserved when present.

  2. Code Simplification: Removed the unnecessary intermediate CallToolResult objects and instead directly create CallToolRequestResult instances with the appropriate fields.

  3. Type Hints: Added TYPE_CHECKING import and __dir__() function to improve static type checking and IDE autocomplete for the lazy-loaded BulkToolCallerMiddleware.

  4. Documentation: Enhanced docstring for no_return_tool test fixture to explicitly document the None return value.

All pre-commit checks pass and all 13 tests pass successfully.

--- |

Test Failure Analysis

Summary: Tests are failing due to MCP SDK schema changes that introduced new fields (, , ), unrelated to the BulkToolCaller middleware changes in this PR.

Root Cause: The failures stem from recent changes to the MCP protocol schema:

  1. ** field added to request params** - Causing logging middleware tests to fail as they expect specific JSON payload sizes
  2. ** field added to sampling messages** - Causing sampling tests to fail with extra field in serialized output
  3. ** added to auth methods** - Causing auth integration tests to fail expecting only

Suggested Solution: Update test snapshots and assertions to accommodate the new MCP SDK schema:

Files to modify:

tests/server/middleware/test_logging.py

  • Update test assertions for to account for the new field
  • Lines affected: ~L110 (was 98, now 110), ~L127 tokens (was 24, now 27)

tests/client/test_sampling.py

  • Update test assertions to expect field in serialized messages
  • Line ~L143: Add to expected message structure

tests/server/test_auth_integration.py

  • Update test to expect both and authentication methods
  • Line ~L40: Change assertion from to
Detailed Analysis

Failure 1: Auth endpoint metadata test

The MCP SDK now includes as an additional authentication method.

Failure 2: Sampling with image test

Sampling messages now include a field at the message level.

Failure 3-6: Logging middleware tests

Request payloads now include field, increasing JSON size by 12 bytes ().

Related Files
    • payload length assertion
    • payload tokens assertion
    • sampling message structure
    • auth methods list

Note: These test failures are not caused by this PR. They affect the main branch and should be addressed separately or this PR should be rebased after the test updates are merged.

Test Failure Analysis

Summary: Tests are failing due to MCP SDK schema changes that introduced new fields (task, _meta, client_secret_basic), unrelated to the BulkToolCaller middleware changes in this PR.

Root Cause: The failures stem from recent changes to the MCP protocol schema:

  1. task field added to request params - Causing logging middleware tests to fail as they expect specific JSON payload sizes
  2. _meta field added to sampling messages - Causing sampling tests to fail with extra field in serialized output
  3. client_secret_basic added to auth methods - Causing auth integration tests to fail expecting only client_secret_post

Suggested Solution: Update test snapshots and assertions to accommodate the new MCP SDK schema:

Files to modify:

tests/server/middleware/test_logging.py

  • Update test assertions for payload_length to account for the new task field
  • Lines affected: approximately L110 (was 98, now 110), approximately L127 tokens (was 24, now 27)

tests/client/test_sampling.py

  • Update test assertions to expect _meta field in serialized messages
  • Line approximately L143: Add _meta field to expected message structure

tests/server/test_auth_integration.py

  • Update test to expect both client_secret_post and client_secret_basic authentication methods
  • Line approximately L40: Change assertion to include both methods

Detailed Analysis

Failure 1: Auth endpoint metadata test

The MCP SDK now includes client_secret_basic as an additional authentication method. Tests expected only client_secret_post.

Failure 2: Sampling with image test

Sampling messages now include a _meta field at the message level that was not present before.

Failure 3-6: Logging middleware tests

Request payloads now include task:null field, increasing JSON size by 12 bytes, affecting payload_length and payload_tokens assertions.

Related Files

  • tests/server/middleware/test_logging.py - lines 108, 127
  • tests/client/test_sampling.py - line 143
  • tests/server/test_auth_integration.py - line 40

Note: These test failures are not caused by this PR. They affect the main branch and should be addressed separately or this PR should be rebased after the test updates are merged.