fix: disabled web search tools still being injected into LLM requests#6526
fix: disabled web search tools still being injected into LLM requests#6526he-yufeng wants to merge 1 commit intoAstrBotDevs:masterfrom
Conversation
edit_web_search_tools filter was fetching tools via get_func() and adding them to the tool set without checking their active status. Tools deactivated by the admin would get re-added on every request. Extract a _try_add helper that checks tool.active before adding, consistent with how persona setup already filters inactive tools. Fixes AstrBotDevs#6506
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 issue where web search tools, despite being deactivated by an administrator, were still being made available to LLM requests. The changes ensure that the system correctly respects the Highlights
Changelog
Activity
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider lifting the
_try_addhelper out ofedit_web_search_toolsinto a reusable utility or method-level helper so it can be shared across similar call sites and is easier to test or extend in the future. - The Baidu
AIsearchbranch now has a manualaisearch_tool.activecheck instead of using_try_add; aligning this path with the helper (or at least the same get-and-check pattern) would keep provider handling consistent and reduce subtle behavioral differences.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider lifting the `_try_add` helper out of `edit_web_search_tools` into a reusable utility or method-level helper so it can be shared across similar call sites and is easier to test or extend in the future.
- The Baidu `AIsearch` branch now has a manual `aisearch_tool.active` check instead of using `_try_add`; aligning this path with the helper (or at least the same get-and-check pattern) would keep provider handling consistent and reduce subtle behavioral differences.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a bug where disabled web search tools were still being added to LLM requests. The introduction of the _try_add helper function to check a tool's active status before adding it is a good refactoring that improves code clarity and prevents duplication. My review includes one suggestion to further improve consistency by applying this new helper function to the baidu_ai_search provider branch as well.
| aisearch_tool = func_tool_mgr.get_func("AIsearch") | ||
| if not aisearch_tool: | ||
| raise ValueError("Cannot get Baidu AI Search MCP tool.") | ||
| tool_set.add_tool(aisearch_tool) | ||
| if aisearch_tool.active: | ||
| tool_set.add_tool(aisearch_tool) |
There was a problem hiding this comment.
While this implementation is correct, it's inconsistent with the other provider branches which use the new _try_add helper function. To improve consistency and make the code easier to maintain, you could use the helper here as well.
The special requirement for the baidu_ai_search provider to raise an error if the tool is missing can be preserved by checking for the tool's existence before calling the helper. Although this results in a second call to get_func, it makes the intent clearer and the overall structure more uniform.
| aisearch_tool = func_tool_mgr.get_func("AIsearch") | |
| if not aisearch_tool: | |
| raise ValueError("Cannot get Baidu AI Search MCP tool.") | |
| tool_set.add_tool(aisearch_tool) | |
| if aisearch_tool.active: | |
| tool_set.add_tool(aisearch_tool) | |
| if not func_tool_mgr.get_func("AIsearch"): | |
| raise ValueError("Cannot get Baidu AI Search MCP tool.") | |
| _try_add("AIsearch") |
|
fixed in #6584 |
Problem
When a user deactivates
web_search_tavily(or any other web search tool) through the admin behavior config panel, the tool still gets added to LLM requests. The model can then call the "disabled" tool as if nothing happened.Root cause:
edit_web_search_toolsfilter fetches tools viafunc_tool_mgr.get_func()which returns the tool object regardless of itsactivestatus, then callstool_set.add_tool()unconditionally. This bypasses the admin deactivation that setstool.active = False.Fix
Add an
activecheck before adding any web search tool. Extracted a_try_addhelper that wraps the get-and-check pattern to keep the provider branches clean:This is consistent with how
astr_main_agent.pyalready filters inactive tools during persona setup (line 362:if not tool.active: persona_toolset.remove_tool(tool.name)).All four provider branches (default, tavily, baidu_ai_search, bocha) are covered.
How to test
tavilyand enable web searchweb_search_tavilyin admin behavior configweb_search_tavilyFixes #6506
Summary by Sourcery
Ensure only active web search tools are injected into LLM toolsets based on the selected provider.
Bug Fixes:
Enhancements: