Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions src/agents/mcp/server.py
Original file line number Diff line number Diff line change
Expand Up @@ -965,6 +965,7 @@ def __init__(

# The cache is always dirty at startup, so that we fetch tools at least once
self._cache_dirty = True
self._tools_cache_generation = 0
self._tools_list: list[MCPTool] | None = None

self.tool_filter = tool_filter
Expand Down Expand Up @@ -1101,6 +1102,7 @@ async def __aexit__(self, exc_type, exc_value, traceback):

def invalidate_tools_cache(self):
"""Invalidate the tools cache."""
self._tools_cache_generation += 1
self._cache_dirty = True

def _extract_http_errors_from_exception(self, e: BaseException) -> list[Exception]:
Expand Down Expand Up @@ -1447,6 +1449,7 @@ async def list_tools(
if self.cache_tools_list and not self._cache_dirty and self._tools_list:
tools = self._tools_list
else:
refresh_generation = self._tools_cache_generation
tools = []
cursor: str | None = None
seen_cursors: set[str | None] = set()
Expand Down Expand Up @@ -1495,8 +1498,9 @@ async def fetch_pages() -> bool:
cursor = None
seen_cursors.clear()
del fetch_pages
self._tools_list = tools
self._cache_dirty = False
if refresh_generation == self._tools_cache_generation:
self._tools_list = tools
self._cache_dirty = False
Comment on lines +1501 to +1503

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Keep tool-call validation aligned with refresh results

When invalidate_tools_cache() is called while a refresh is in flight and the server changed the required parameters of an existing tool, this branch leaves the old _tools_list in place while list_tools() returns the newly fetched schema. A subsequent call_tool() validates against that old list in _validate_required_parameters() and can reject a valid invocation for missing a parameter that the returned schema no longer requires, before the request reaches the server. Avoid validating against the dirty prior cache while preserving the pending invalidation.

AGENTS.md reference: AGENTS.md:L149-L149

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Addressed in cbbcd49b. Dirty tool caches are no longer used for local required-parameter validation, so a stale retained schema cannot reject calls while a newer invalidation is pending. The regression now covers the stale refresh result, dirty-cache call-through, the subsequent clean refresh, and restored validation from the new authoritative cache.


# Filter tools based on tool_filter
filtered_tools = tools
Expand Down Expand Up @@ -1596,7 +1600,7 @@ def _validate_required_parameters(
self, tool_name: str, arguments: dict[str, Any] | None
) -> None:
"""Validate required tool parameters from cached MCP tool schemas before invocation."""
if self._tools_list is None:
if self._cache_dirty or self._tools_list is None:
return

tool = next((item for item in self._tools_list if item.name == tool_name), None)
Expand Down
99 changes: 98 additions & 1 deletion tests/mcp/test_caching.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import asyncio
from unittest.mock import AsyncMock, call, patch

import pytest
from mcp.types import PaginatedRequestParams
from mcp.types import CallToolResult, PaginatedRequestParams, TextContent

from agents import Agent
from agents.exceptions import UserError
from agents.mcp import MCPServerStdio
from agents.run_context import RunContextWrapper

Expand Down Expand Up @@ -64,6 +66,101 @@ async def test_server_caching_works(
assert result_tools == tools


@pytest.mark.asyncio
@patch("mcp.client.stdio.stdio_client", return_value=DummyStreamsContextManager())
@patch("mcp.client.session.ClientSession.initialize", new_callable=AsyncMock, return_value=None)
@patch("mcp.client.session.ClientSession.call_tool", new_callable=AsyncMock)
@patch("mcp.client.session.ClientSession.list_tools")
async def test_cache_invalidation_during_refresh_is_preserved(
mock_list_tools: AsyncMock,
mock_call_tool: AsyncMock,
mock_initialize: AsyncMock,
mock_stdio_client,
):
refresh_started = asyncio.Event()
release_refresh = asyncio.Event()
request_count = 0
responses = [
ListToolsResult(
tools=[
MCPTool(
name="tool1",
description="initial",
inputSchema={"required": ["q"]},
),
],
),
ListToolsResult(
tools=[
MCPTool(
name="tool1",
description="before-second-invalidation",
inputSchema={},
),
],
),
ListToolsResult(
tools=[
MCPTool(
name="tool1",
description="after-second-invalidation",
inputSchema={"required": ["latest"]},
),
],
),
]

async def list_tools():
nonlocal request_count
request_count += 1
if request_count == 2:
refresh_started.set()
await release_refresh.wait()
return responses[request_count - 1]

mock_list_tools.side_effect = list_tools
mock_call_tool.return_value = CallToolResult(
content=[TextContent(type="text", text="ok")],
)
server = MCPServerStdio(
params={"command": tee},
cache_tools_list=True,
)

async with server:
initial = await server.list_tools()
assert initial[0].description == "initial"

server.invalidate_tools_cache()
refresh_task = asyncio.create_task(server.list_tools())
try:
await asyncio.wait_for(refresh_started.wait(), timeout=1)

server.invalidate_tools_cache()
release_refresh.set()
refreshed = await asyncio.wait_for(refresh_task, timeout=1)
finally:
release_refresh.set()
if not refresh_task.done():
refresh_task.cancel()
await asyncio.gather(refresh_task, return_exceptions=True)

assert refreshed[0].description == "before-second-invalidation"
assert (server.cached_tools or [])[0].description == "initial"

await server.call_tool("tool1", {})
assert mock_call_tool.call_count == 1

latest = await server.list_tools()
assert latest[0].description == "after-second-invalidation"
assert (server.cached_tools or [])[0].description == "after-second-invalidation"
assert request_count == 3

with pytest.raises(UserError, match="missing required parameters: latest"):
await server.call_tool("tool1", {})
assert mock_call_tool.call_count == 1


@pytest.mark.asyncio
@patch("mcp.client.stdio.stdio_client", return_value=DummyStreamsContextManager())
@patch("mcp.client.session.ClientSession.initialize", new_callable=AsyncMock, return_value=None)
Expand Down
6 changes: 6 additions & 0 deletions tests/mcp/test_client_session_retries.py
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,7 @@ async def test_call_tool_validates_required_parameters_before_remote_call():
},
)
]
server._cache_dirty = False # noqa: SLF001

with pytest.raises(UserError, match="missing required parameters: param_a"):
await server.call_tool("tool", {})
Expand All @@ -246,6 +247,7 @@ async def test_call_tool_with_required_parameters_still_calls_remote_tool():
},
)
]
server._cache_dirty = False # noqa: SLF001

result = await server.call_tool("tool", {"param_a": "value"})
assert isinstance(result, CallToolResult)
Expand All @@ -257,6 +259,7 @@ async def test_call_tool_skips_validation_when_tool_is_missing_from_cache():
session = DummySession()
server = DummyServer(session=session, retries=0)
server._tools_list = [MCPTool(name="different_tool", inputSchema={"required": ["param_a"]})] # noqa: SLF001
server._cache_dirty = False # noqa: SLF001

await server.call_tool("tool", {})
assert session.call_tool_attempts == 1
Expand All @@ -267,6 +270,7 @@ async def test_call_tool_skips_validation_when_required_list_is_absent():
session = DummySession()
server = DummyServer(session=session, retries=0)
server._tools_list = [MCPTool(name="tool", inputSchema={"type": "object"})] # noqa: SLF001
server._cache_dirty = False # noqa: SLF001

await server.call_tool("tool", None)
assert session.call_tool_attempts == 1
Expand All @@ -277,6 +281,7 @@ async def test_call_tool_validates_required_parameters_when_arguments_is_none():
session = DummySession()
server = DummyServer(session=session, retries=0)
server._tools_list = [MCPTool(name="tool", inputSchema={"required": ["param_a"]})] # noqa: SLF001
server._cache_dirty = False # noqa: SLF001

with pytest.raises(UserError, match="missing required parameters: param_a"):
await server.call_tool("tool", None)
Expand All @@ -289,6 +294,7 @@ async def test_call_tool_rejects_non_object_arguments_before_remote_call():
session = DummySession()
server = DummyServer(session=session, retries=0)
server._tools_list = [MCPTool(name="tool", inputSchema={"required": ["param_a"]})] # noqa: SLF001
server._cache_dirty = False # noqa: SLF001

with pytest.raises(UserError, match="arguments must be an object"):
await server.call_tool("tool", cast(dict[str, object] | None, ["bad"]))
Expand Down