fix(toolset): fix toolset override in config - #512
Conversation
WalkthroughThe changes revise the logic for enabling toolsets in the toolset manager. Comments and docstrings were updated for clarity, and the timing of enabling toolsets was adjusted so that built-in toolsets are enabled earlier in the process. Tests were added and modified to verify the new enabling and disabling logic for both built-in and custom toolsets. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ToolsetManager
participant BuiltinLoader
participant ConfigLoader
User->>ToolsetManager: _list_all_toolsets()
ToolsetManager->>BuiltinLoader: Load built-in toolsets
BuiltinLoader-->>ToolsetManager: Built-in toolsets
ToolsetManager->>ToolsetManager: Enable all built-in toolsets if enable_all_toolsets is True
ToolsetManager->>ConfigLoader: Load custom toolsets from config
ConfigLoader-->>ToolsetManager: Custom toolsets
ToolsetManager->>ToolsetManager: Set custom toolsets enabled by default unless explicitly disabled
ToolsetManager->>ToolsetManager: Merge and filter toolsets as needed
ToolsetManager-->>User: List of toolsets with correct enabled/disabled status
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
holmes/core/toolset_manager.py (1)
142-147: Reduce branching – streamline default-enable handling
The four-lineif/elseblock can be collapsed without altering semantics:- if toolset_config.get("enabled", True) is False: - toolset_config["enabled"] = False - else: - toolset_config["enabled"] = True + # enabled by default unless explicitly set to False + toolset_config["enabled"] = toolset_config.get("enabled", True) is not FalseFewer branches → simpler reasoning, same result.
tests/core/test_toolset_manager.py (2)
9-15: Keep tests independent of concreteYAMLToolsetimplementation
Importing and instantiatingYAMLToolsetties the test suite to an internal helper class. A plainMagicMock(ornamedtuple) would exercise the manager just as well while decoupling the test from future refactors insideholmes.core.tools.
76-94: Prefer YAML for temporary custom-toolset file
The file is parsed via a YAML loader; writing JSON works (YAML ⊇ JSON) but hurts readability. Tiny tweak:- data = {"toolsets": {"builtin": {"enabled": False}}} - json.dump(data, tmpfile, indent=2) + yaml.safe_dump({"toolsets": {"builtin": {"enabled": False}}}, tmpfile)Keeps format consistent with expected input and avoids the extra
jsonimport.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
holmes/core/tools.py(0 hunks)holmes/core/toolset_manager.py(2 hunks)tests/core/test_toolset_manager.py(2 hunks)
💤 Files with no reviewable changes (1)
- holmes/core/tools.py
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build (3.11)
- GitHub Check: build (3.10)
- GitHub Check: build (3.12)
- GitHub Check: build (3.12)
🔇 Additional comments (3)
holmes/core/toolset_manager.py (1)
82-85: Good relocation ofenable_all_toolsetslogic
Enabling every built-in as soon as they’re loaded makes theenable_all_toolsetsswitch deterministic and prevents late overrides from silently re-enabling a toolset. The later config merge still has the authority to flipenabled=False, so behaviour is preserved.tests/core/test_toolset_manager.py (2)
52-52: Test data change is sensible
Switching from anenabledflag to a benign field (description) removes accidental side-effects and focuses the test on merge behaviour. Looks good.
59-74: Solid regression test for builtin override
The new case correctly verifies that user config can disable a built-in toolset. Mocking keeps the scope narrow and fast.
2 participants
before this change, disabling the built-in toolset from the toolset from config or custom toolset don't work. Fix this by setting the built-in enablement before it is override by custom or config toolset.
Test
This config
will give