fix: key MCP tool cache by server_names to prevent wrong tools served… - #66
Open
alenjosesr wants to merge 1 commit into
Open
fix: key MCP tool cache by server_names to prevent wrong tools served…#66alenjosesr wants to merge 1 commit into
alenjosesr wants to merge 1 commit into
Conversation
… to subagents The module-level `_cached_tools` list had no cache key, so `server_names` was ignored on a cache hit. The orchestrator's tool list was returned to the analyst subagent (and vice versa) for the full TTL window (300s), causing the analyst to run with `validate_email` instead of `calculate_bmi` + `search_web`. The LLM silently answered from memory, producing hallucinated BMI values with no tool calls. Fix: replace the flat list with a `frozenset`-keyed dict so each unique combination of requested servers gets its own cache entry. - `deep_agent/aegra/mcp.py`: replace `_cached_tools`/`_cached_tools_ts` with `_tool_cache`/`_tool_cache_ts` (dict keyed by frozenset); update lookup and store to use the key - `tests/unit/cache/test_mcp_cache.py`: new — 6 tests covering cache miss, bug regression (×2 directions), correct same-key hit, TTL expiry - `tests/unit/infrastructure/test_mcp.py`: update `_reset_mcp_cache()` to reset the new dict names - `docs/bugs/bug-002-mcp-cache-ignores-server-names.md`: bug report with root cause, reproduction steps, fix diffs, and test impact table Co-authored-by: Cursor <cursoragent@cursor.com>
Member
|
Can you resolve the conflicts and update the branch? |
Member
|
@coderabbitai review |
✅ Action performedReview finished.
|
WalkthroughMCP tool caching now uses dictionaries keyed by each requested server-name set. Cache hits return cached empty results and log the key. Loaded tools and timestamps are stored under the matching key. Unit tests cover separate-server isolation, same-key reuse, TTL expiration, and cache reset behavior. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
🚀 Post-Merge Actions
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Warning |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deep_agent/aegra/mcp.py`:
- Around line 362-369: Update get_mcp_tools in deep_agent/aegra/mcp.py at lines
362-369 to cache empty results by storing an empty tool list and timestamp for
both no-enabled-server and all-failed-server outcomes before returning. Add a
repeated empty-result request in tests/unit/cache/test_mcp_cache.py at lines
241-261 and assert that only one connection attempt occurs.
In `@tests/unit/cache/test_mcp_cache.py`:
- Around line 220-237: Extend test_same_server_names_cache_hit_is_correct to use
multiple server names, then issue a second request with those names reversed and
assert the connection mock call count remains unchanged while the returned tools
remain equivalent. This should verify the cache key is independent of
server-name ordering.
- Around line 241-261: The test suite lacks coverage for caching empty results.
Extend test_expired_cache_refetches_from_network or add a focused test using
get_mcp_tools with a server configuration that returns no tools, invoke it twice
with the same key, and assert the connection mock is called only once while both
results are empty.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9137de71-2478-4449-92b0-e2ca46b4c54b
⛔ Files ignored due to path filters (1)
docs/bugs/bug-002-mcp-cache-ignores-server-names.mdis excluded by!**/*.md
📒 Files selected for processing (3)
deep_agent/aegra/mcp.pytests/unit/cache/test_mcp_cache.pytests/unit/infrastructure/test_mcp.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
redhat-data-and-ai/template-mcp(manual)redhat-data-and-ai/template-ui(manual) → reviewed against open PR#76deep-agentinstead of the default branch
Comment on lines
+362
to
+369
| _tool_cache[cache_key] = tools | ||
| _tool_cache_ts[cache_key] = time.time() | ||
| logger.warning( | ||
| f"Loaded {len(tools)} MCP tool(s): {', '.join(seen)} (cached for {_MCP_TOOL_CACHE_TTL:.0f}s)" | ||
| "Loaded %d MCP tool(s): %s (key=%s, cached for %.0fs)", | ||
| len(tools), | ||
| ", ".join(seen), | ||
| set(cache_key) or "all", | ||
| _MCP_TOOL_CACHE_TTL, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Cache empty results: get_mcp_tools returns before writing empty entries, and the regression suite does not detect the contract failure.
deep_agent/aegra/mcp.py#L362-L369: store an empty list and timestamp for no-enabled-server and all-failed-server results.tests/unit/cache/test_mcp_cache.py#L241-L261: add a repeated empty-result request and assert that only one connection attempt occurs.
📍 Affects 2 files
deep_agent/aegra/mcp.py#L362-L369(this comment)tests/unit/cache/test_mcp_cache.py#L241-L261
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deep_agent/aegra/mcp.py` around lines 362 - 369, Update get_mcp_tools in
deep_agent/aegra/mcp.py at lines 362-369 to cache empty results by storing an
empty tool list and timestamp for both no-enabled-server and all-failed-server
outcomes before returning. Add a repeated empty-result request in
tests/unit/cache/test_mcp_cache.py at lines 241-261 and assert that only one
connection attempt occurs.
Comment on lines
+220
to
+237
| async def test_same_server_names_cache_hit_is_correct(self): | ||
| """Cache hit with the same server_names is correct and expected.""" | ||
| connect_mock = AsyncMock(side_effect=_fake_connect) | ||
|
|
||
| with ( | ||
| patch.object(mcp_module, "_get_server_configs", return_value=_FAKE_SERVERS), | ||
| patch("deep_agent.aegra.mcp._connect_single_server", connect_mock), | ||
| ): | ||
| first = await mcp_module.get_mcp_tools( | ||
| sso_token=None, server_names=["main-mcp"] | ||
| ) | ||
| second = await mcp_module.get_mcp_tools( | ||
| sso_token=None, server_names=["main-mcp"] | ||
| ) | ||
|
|
||
| # Only one real connection attempt — second is a correct cache hit | ||
| assert connect_mock.call_count == 1 | ||
| assert [t.name for t in first] == [t.name for t in second] == ["validate_email"] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover order independence: This test passes with an order-sensitive key, so add a reversed multi-server request and assert that it does not create new connections.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/cache/test_mcp_cache.py` around lines 220 - 237, Extend
test_same_server_names_cache_hit_is_correct to use multiple server names, then
issue a second request with those names reversed and assert the connection mock
call count remains unchanged while the returned tools remain equivalent. This
should verify the cache key is independent of server-name ordering.
Comment on lines
+241
to
+261
| async def test_expired_cache_refetches_from_network(self): | ||
| """After TTL expires the cache is bypassed and tools are re-fetched.""" | ||
| connect_mock = AsyncMock(side_effect=_fake_connect) | ||
|
|
||
| with ( | ||
| patch.object(mcp_module, "_get_server_configs", return_value=_FAKE_SERVERS), | ||
| patch("deep_agent.aegra.mcp._connect_single_server", connect_mock), | ||
| ): | ||
| # Pre-populate with a stale cache entry for the same key | ||
| stale_key = frozenset(["main-mcp"]) | ||
| mcp_module._tool_cache[stale_key] = [_tool("stale_tool")] | ||
| mcp_module._tool_cache_ts[stale_key] = time.time() - 99999 | ||
|
|
||
| tools = await mcp_module.get_mcp_tools( | ||
| sso_token=None, | ||
| server_names=["main-mcp"], | ||
| ) | ||
|
|
||
| # Network was hit despite pre-populated cache (TTL expired) | ||
| assert connect_mock.call_count == 1 | ||
| assert [t.name for t in tools] == ["validate_email"] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test empty-result caching: The suite passes when get_mcp_tools never stores empty results, so add a repeated empty-result request and assert one connection attempt.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/cache/test_mcp_cache.py` around lines 241 - 261, The test suite
lacks coverage for caching empty results. Extend
test_expired_cache_refetches_from_network or add a focused test using
get_mcp_tools with a server configuration that returns no tools, invoke it twice
with the same key, and assert the connection mock is called only once while both
results are empty.
Author
|
Ack. I’ll fix the conflict and update this PR |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The module-level MCP tool cache in
deep_agent/aegra/mcp.pyused a flat list with no key.server_nameswas completely ignored on a cache hit — the first caller's tools were returnedto every subsequent caller for the full TTL window (default 300 s).
In practice,
graph.pycallsget_mcp_tools()twice per request: once for the orchestrator(
server_names=["main-mcp"]) and once insideload_subagents()for the analyst subagent(
server_names=["analytics-mcp"]). The orchestrator's call populated the cache; the analyst'scall hit it and received
[validate_email]instead of[calculate_bmi, search_web].The LLM had no tool to call for BMI analysis and answered from internal knowledge, producing
hallucinated BMI values silently — no error, no log warning, normal-looking response.
Root Cause
Fix
Replace the flat list with a
frozenset-keyed dict so each unique combination of requestedservers gets its own cache entry.
frozensetis used because call order withinserver_namesshould not matter(
["a","b"]and["b","a"]are the same request).frozenset()represents "all enabled servers".Changes
deep_agent/aegra/mcp.pyfrozenset-keyed dict; update lookup and storetests/unit/cache/test_mcp_cache.pytests/unit/infrastructure/test_mcp.py_reset_mcp_cache()to reset new dict namesdocs/bugs/bug-002-mcp-cache-ignores-server-names.mdTest Results
test_cache_miss_fetches_correct_tools_for_main_mcptest_cache_miss_fetches_correct_tools_for_analytics_mcptest_bug2_analyst_gets_own_tools_after_orchestrator_cachedtest_bug2_orchestrator_gets_own_tools_after_analyst_cachedtest_same_server_names_cache_hit_is_correcttest_expired_cache_refetches_from_networkOverall suite: 538 passed → 543 passed. The 5 additional passes are pre-existing tests in
tests/unit/infrastructure/test_mcp.pythat were silently blocked by alangchain_mcp_adaptersimport error (unrelated dependency version mismatch); the
sys.modulespatch in the new testfile unblocked them as a side effect.
How to Reproduce (before fix)
How to Verify the Fix
uv run pytest tests/unit/cache/test_mcp_cache.py -v # 6 passed