ROB-3410 Disable unconfigured toolsets by default in CLI - #1658
Merged
Merged
Conversation
Instead of blindly enabling all toolsets in CLI mode, only auto-enable toolsets that have a reasonable chance of initializing: - No config required (YAML toolsets, simple Python toolsets) → enable - All config fields have defaults (e.g. Prometheus) → enable - Config provided by user → enable - Required config fields with no config provided → disable This avoids slow and noisy prerequisite checks for toolsets like Elasticsearch, Grafana, ServiceNow etc. when no config is provided. https://claude.ai/code/session_01XFmKgriPCWgsAZ6B6fCkBg Signed-off-by: Claude <noreply@anthropic.com>
Contributor
📂 Previous Runs📜 Run @ 97a474e (#22713968495)✅ Results of HolmesGPT evalsAutomatically triggered by commit 97a474e on branch Results of HolmesGPT evals
📜 Run @ f6fd301 (#22713644290)✅ Results of HolmesGPT evalsAutomatically triggered by commit f6fd301 on branch Results of HolmesGPT evals
📜 Run @ bc64db8 (#22671174264)✅ Results of HolmesGPT evalsAutomatically triggered by commit bc64db8 on branch Results of HolmesGPT evals
✅ Results of HolmesGPT evalsAutomatically triggered by commit 0e48ad1 on branch Results of HolmesGPT evals
Benchmark comparison unavailable: No ci-benchmark experiments found Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: No ci-benchmark experiments found Comparison indicators:
📖 Legend
🔄 Re-run evals manually
Option 1: Comment on this PR with Or with more options (one per line): Run evals on a different branch (e.g., master) for comparison:
Quick re-run: Use Option 2: Trigger via GitHub Actions UI → "Run workflow" Option 3: Add PR labels to include extra evals in automatic regression runs:
Examples: 🏷️ Valid tags
Commands: CLI: |
Contributor
|
✅ Docker images ready for
Use these tags to pull the images for testing. 📋 Copy commandsgcloud auth configure-docker us-central1-docker.pkg.dev
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:7c13d55a
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:7c13d55a me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:7c13d55a
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:7c13d55a
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:7c13d55a
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:7c13d55a me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:7c13d55a
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:7c13d55aPatch Helm values in one line (choose the chart you use): HolmesGPT chart: helm upgrade --install holmesgpt ./helm/holmes \
--set registry=me-west1-docker.pkg.dev/robusta-development/development \
--set image=holmes-dev:7c13d55a \
--set operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set operator.image=holmes-operator-dev:7c13d55aRobusta wrapper chart: helm upgrade --install robusta robusta/robusta \
--reuse-values \
--set holmes.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.image=holmes-dev:7c13d55a \
--set holmes.operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.operator.image=holmes-operator-dev:7c13d55a |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds Toolset.auto-enable decision logic via a new Changes
Sequence Diagram(s)sequenceDiagram
participant Manager as ToolsetManager
participant Toolset as Toolset
participant Model as ToolsetConfigModel
Manager->>Toolset: iterate toolsets (enable_all_toolsets)
Toolset->>Toolset: should_auto_enable()
Toolset->>Model: if config_classes present -> has_required_fields()
Model-->>Toolset: returns True/False
alt has_required == True and no config provided
Toolset-->>Manager: return False (cannot auto-enable)
Toolset->>Manager: debug log "requires configuration"
else
Toolset-->>Manager: return True (auto-enable)
Manager->>Toolset: set enabled = True
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
🔬 CLI Performance Benchmark🟡 Startup Time (no LLM)Measures
🟡 Full CLI with LLMMeasures
PR: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/test_toolset_auto_enable.py (1)
1-1: Consider placing this test module under source-mirrored test paths.This file tests behavior from
holmes/core/tools.pyandholmes/utils/pydantic_utils.py; placing it under matchingtests/core/and/ortests/utils/paths would align with project test layout conventions.As per coding guidelines,
tests/**: “Tests should match source structure undertests/”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_toolset_auto_enable.py` at line 1, The test module is placed at tests/test_toolset_auto_enable.py but exercises code in holmes/core/tools.py and holmes/utils/pydantic_utils.py; move the file into a source-mirrored location (e.g., tests/core/test_toolset_auto_enable.py or tests/utils/test_toolset_auto_enable.py) so the tests mirror the package structure, and update any imports or test discovery paths accordingly (ensure references to functions/classes in holmes.core.tools and holmes.utils.pydantic_utils remain correct after relocating).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@holmes/core/tools.py`:
- Around line 806-807: The current truthiness check "if self.config: return
True" treats an explicitly provided empty config (e.g., {}) as not provided;
change the check to explicitly compare against None (use "if self.config is not
None") so the method recognizes an explicitly provided empty config as provided;
update the condition in the method that references self.config to use "is not
None" rather than a truthy check.
In `@holmes/utils/pydantic_utils.py`:
- Around line 27-28: The fallback assignment for PydanticUndefined currently
sits under a broad "except Exception:" which can hide unrelated errors; change
that catch to "except ImportError:" (which covers ModuleNotFoundError) so only
import failures trigger the fallback where PydanticUndefined is defined, keeping
the rest of the code unchanged (look for the try/except that defines
PydanticUndefined in pydantic_utils.py).
In `@tests/test_toolset_auto_enable.py`:
- Around line 52-55: The helper is mutating the shared Toolset class by
assigning config_classes on type(toolset), which can leak between tests; instead
dynamically create a per-test subclass of Toolset (e.g., subclass_name =
type("TestToolset", (Toolset,), {"config_classes": config_classes}) ) and
instantiate that subclass so only the subclass's class attribute is modified;
update the helper to instantiate the new subclass rather than setting
type(toolset).config_classes to ensure test isolation for
Toolset.config_classes.
---
Nitpick comments:
In `@tests/test_toolset_auto_enable.py`:
- Line 1: The test module is placed at tests/test_toolset_auto_enable.py but
exercises code in holmes/core/tools.py and holmes/utils/pydantic_utils.py; move
the file into a source-mirrored location (e.g.,
tests/core/test_toolset_auto_enable.py or
tests/utils/test_toolset_auto_enable.py) so the tests mirror the package
structure, and update any imports or test discovery paths accordingly (ensure
references to functions/classes in holmes.core.tools and
holmes.utils.pydantic_utils remain correct after relocating).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e0a8f022-1134-4362-9219-e9192858e365
📒 Files selected for processing (4)
holmes/core/tools.pyholmes/core/toolset_manager.pyholmes/utils/pydantic_utils.pytests/test_toolset_auto_enable.py
- Use `is not None` instead of truthiness for self.config so an
explicitly provided empty config ({}) is recognized as provided.
- Create per-test Toolset subclasses in tests to avoid leaking
config_classes between test cases.
- Add test for empty-config edge case.
https://claude.ai/code/session_01XFmKgriPCWgsAZ6B6fCkBg
Signed-off-by: Claude <noreply@anthropic.com>
…ed-toolsets-SuEEV
Import PydanticUndefined from holmes.utils.pydantic_utils instead of duplicating the try/except block from pydantic_core. https://claude.ai/code/session_01XFmKgriPCWgsAZ6B6fCkBg Signed-off-by: Claude <noreply@anthropic.com>
naomi-robusta
changed the title
naomi-robusta
changed the title
aantn
enabled auto-merge (squash)
March 8, 2026 09:33
aantn
approved these changes
Mar 8, 2026
This was referenced Mar 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR implements intelligent auto-enable logic for toolsets that respects their configuration requirements. Toolsets are now only auto-enabled if they either have no required configuration, have all optional configuration fields, or have been provided with the necessary configuration by the user.
Key Changes
Added
ToolsetConfig.has_required_fields()method (holmes/utils/pydantic_utils.py)Added
Toolset.should_auto_enable()method (holmes/core/tools.py)Updated
ToolsetManager._list_all_toolsets()(holmes/core/toolset_manager.py)enable_all_toolsetslogic to useshould_auto_enable()instead of blindly enabling all toolsetsAdded comprehensive test suite (
tests/test_toolset_auto_enable.py)has_required_fields()with various config scenariosshould_auto_enable()covering all decision pathsImplementation Details
The solution uses Pydantic's
model_fieldsintrospection to detect required fields by checking for the presence of defaults or default factories. ThePydanticUndefinedsentinel frompydantic_coreis used to distinguish between explicitly setNonedefaults and truly required fields.The auto-enable logic is conservative: toolsets with required configuration are only enabled if that configuration has been explicitly provided, preventing silent failures from missing required settings.
https://claude.ai/code/session_01XFmKgriPCWgsAZ6B6fCkBg
Summary by CodeRabbit