fix: correct MCP tool cache check operator precedence#628
fix: correct MCP tool cache check operator precedence#628MarvelNwachukwu wants to merge 2 commits intomainfrom
Conversation
Changed `!this.config.cacheConfig?.enabled === false` to `this.config.cacheConfig?.enabled !== false` to fix operator precedence bug. The original expression never evaluated to true, preventing the cache from being used in the default case when cacheConfig is undefined. This matches the correct pattern already used elsewhere in the file.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug where the MCP tool's cache was not being utilized as intended due to an operator precedence error in its configuration check. The fix ensures that the cache enablement logic evaluates correctly, restoring expected caching behavior and improving performance. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a bug related to operator precedence in the MCP tool cache check, ensuring that caching is enabled by default as intended. The fix is clean and consistent with the existing codebase. I have added one suggestion to further improve maintainability by refactoring the duplicated cache-check logic into a helper method.
packages/adk/src/tools/mcp/index.ts
Outdated
| this.tools.length > 0 && | ||
| !this.config.cacheConfig?.enabled === false | ||
| ) { | ||
| if (this.tools.length > 0 && this.config.cacheConfig?.enabled !== false) { |
There was a problem hiding this comment.
This logic to check if the cache is enabled is also used on line 172. To improve readability and avoid duplication, consider extracting this logic into a private helper method.
For example, you could add a method to the McpToolset class:
private isCacheEnabled(): boolean {
return this.config.cacheConfig?.enabled !== false;
}And then use it here and on line 172:
if (this.tools.length > 0 && this.isCacheEnabled()) {
return this.tools;
}There was a problem hiding this comment.
Good call — extracted into a private isCacheEnabled() helper and updated both call sites in eb4ad9e.
Deduplicate the cache-enabled check used in two places by extracting it into a private isCacheEnabled() method.
|
1 participant
Description
Why this matters: When connecting to external tool servers (MCP), ADK caches the list of available tools so it doesn't have to re-fetch them every time. A typo-level bug meant this cache was silently broken — tools were being re-fetched on every call, adding unnecessary latency and server load.
What changed: Fixed a single operator precedence issue (
!x === false→x !== false) that was preventing the cache from ever being used.Type of Change
How Has This Been Tested?
All existing tests pass (478 tests). Build succeeds without errors.
Checklist