fix: selected_tools=[] should return no tools, not all tools#225
fix: selected_tools=[] should return no tools, not all tools#225AdityaChauhanX07 wants to merge 1 commit intomesa:mainfrom
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can disable the changed files summary in the walkthrough.Disable the |
|
Peer Review selected_tools=[] should return no tools, not all tools Reviewed Overall Clean, well-targeted fix for a real semantic bug. The diff is minimal (10 lines) and the logic is exactly right. The bug and fix are correct Before (buggy): After (correct): The distinction between Test updated correctly The test description change from 'empty list returns all tools (current behavior)' to 'empty list returns no tools' reflects the intended semantics. The assertion change from Downstream impact: worth checking for breaking callers Any call site that previously passed None path still returns all tools ✅ The existing Code style improvement The original had an unnecessary Summary Minimal, correct, well-tested. The only addition worth making to the PR description is an explicit note that this is a breaking change for any caller relying on |
|
Thanks for the review. Good point on the breaking change note - though I'd argue any caller passing |
Closes #148
get_all_tools_schema()checksif selected_tools:to decide whether to filter tools.The problem is that
[]is falsy in Python, soselected_tools=[]hits theelsebranch and returns every registered tool - the opposite of what the caller asked for.I ran into this while building models in my GSoC learning space.
My
hello_llm_agentmodel passesselected_tools=[]with a system prompt saying "never use tools," but agents were still callingteleport_to_locationbecause the schemas were getting injected anyway.The fix is changing
if selected_tools:toif selected_tools is not None:so that:None→ all tools (default, unchanged)[]→ no tools (the actual opt-out)["tool_a"]→ just those tools (unchanged)Also updated
test_get_all_tools_schema_empty_listwhich was asserting the buggy behavior as "current behavior."All 275 tests pass, 0 failures.
Note: This is technically a breaking change for any code that passed
selected_tools=[]expecting all tools. The previous behavior was a bug - the test documenting it was labeled "current behavior" not "intended behavior."