Skip to content

fix: selected_tools=[] should return no tools, not all tools#225

Open
AdityaChauhanX07 wants to merge 1 commit intomesa:mainfrom
AdityaChauhanX07:fix/selected-tools-empty-list-148
Open

fix: selected_tools=[] should return no tools, not all tools#225
AdityaChauhanX07 wants to merge 1 commit intomesa:mainfrom
AdityaChauhanX07:fix/selected-tools-empty-list-148

Conversation

@AdityaChauhanX07
Copy link

@AdityaChauhanX07 AdityaChauhanX07 commented Mar 15, 2026

Closes #148

get_all_tools_schema() checks if selected_tools: to decide whether to filter tools.
The problem is that [] is falsy in Python, so selected_tools=[] hits the else branch 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_agent model passes selected_tools=[] with a system prompt saying "never use tools," but agents were still calling teleport_to_location because the schemas were getting injected anyway.

The fix is changing if selected_tools: to if 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_list which 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."

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 15, 2026

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3983f9a3-4086-4898-9adc-dce4b1fe2798

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📝 Coding Plan
  • Generate coding plan for human review comments

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Tip

You can disable the changed files summary in the walkthrough.

Disable the reviews.changed_files_summary setting to disable the changed files summary in the walkthrough.

@abhinavk0220
Copy link

abhinavk0220 commented Mar 19, 2026

Peer Review selected_tools=[] should return no tools, not all tools

Reviewed mesa_llm/tools/tool_manager.py and tests/test_tools/test_tool_manager.py in full.


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): if selected_tools: treats [] as falsy and falls through to return ALL tools.

After (correct): if selected_tools is not None: an empty list is not None, so it correctly returns an empty list when the caller explicitly passed [].

The distinction between None (caller did not specify, give me everything) and [] (caller explicitly wants no tools) is a standard Python API contract. is not None is the correct idiom here. ✅


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 len(empty_list_schemas) == len(all_schemas) to len(empty_list_schemas) == 0 is correct. ✅


Downstream impact: worth checking for breaking callers

Any call site that previously passed selected_tools=[] expecting all tools back will now silently receive zero tools instead. A quick grep across the codebase for selected_tools=[] is worth running to confirm no reasoning module or agent relies on the old behavior. The fix is semantically correct but worth flagging as a breaking change in the PR description.


None path still returns all tools ✅

The existing test_get_all_tools_schema_none confirms that get_all_tools_schema(None) and get_all_tools_schema() (no argument) still return all tools after this fix. ✅


Code style improvement

The original had an unnecessary else after a return. The new version removes it and flattens the control flow cleaner. ✅


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 selected_tools=[] returning all tools. Good work.

@AdityaChauhanX07
Copy link
Author

Thanks for the review. Good point on the breaking change note - though I'd argue any caller passing selected_tools=[] and expecting all tools back was relying on buggy behavior, not intentional API design. The test itself was documented as "current behavior" not "intended behavior." But worth mentioning, so I'll add a note to the PR description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ToolManager.get_all_tools_schema() exposes every tool to the LLM regardless of agent state

2 participants