Conversation
|
|
WalkthroughAdds an allowlist for builtin toolsets: Config gains Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as holmes/main.py
participant Config
participant TM as ToolsetManager
participant Loader as load_builtin_toolsets
User->>CLI: run command (--allowed-builtin-toolsets=...)
CLI->>Config: load_from_file(allowed_builtin_toolsets=CSV)
Config-->>Config: parse/validate -> List[str] or None
CLI->>Config: access config.toolset_manager
Config->>TM: __init__(config=self, ...)
TM->>TM: _get_allowed_builtin_toolsets()
TM->>Loader: load_builtin_toolsets(..., allowed_builtin_toolsets)
Loader-->>TM: filtered builtin toolsets
TM-->>CLI: toolsets
CLI-->>User: execute with resulting toolsets
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
e43409c to
f663fb8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
holmes/plugins/toolsets/__init__.py (1)
102-105: Document the new parameter and clarify ordering semanticsThe new allowed_builtin_toolsets parameter is clear from the code, but a short docstring would help future readers and avoid ambiguity about how ordering is handled (results follow builtin discovery order, not the allowlist’s order).
Consider adding a docstring like:
def load_builtin_toolsets( dal: Optional[SupabaseDal] = None, allowed_builtin_toolsets: Optional[List[str]] = None, ) -> List[Toolset]: """ Load builtin (YAML + Python) toolsets. Args: dal: Supabase DAL for toolsets that need it. allowed_builtin_toolsets: If provided, restrict the loaded toolsets to names in this allowlist. Unknown names are warned and ignored. Note: the resulting order follows builtin discovery order, not the allowlist order. """ ...tests/plugins/test_toolsets_filtering.py (2)
96-99: Reduce timing flakiness; use perf_counter() for better resolutionUsing time.time() can make this test flaky under load. perf_counter() is more appropriate for measuring durations, and a slightly looser threshold makes CI more stable.
Apply:
- avg_baseline = sum(baseline_times) / len(baseline_times) - avg_filtered = sum(filtered_times) / len(filtered_times) + avg_baseline = sum(baseline_times) / len(baseline_times) + avg_filtered = sum(filtered_times) / len(filtered_times) # Should be within reasonable bounds (filtering adds minimal overhead) - # Using 3x multiplier to account for timing variations in test environment - assert avg_filtered < avg_baseline * 3.0 + # Using a conservative multiplier to account for timing variations in CI + assert avg_filtered < avg_baseline * 4.0And above, replace time.time() with time.perf_counter():
- start = time.time() + start = time.perf_counter() - baseline_times.append(time.time() - start) + baseline_times.append(time.perf_counter() - start) ... - start = time.time() + start = time.perf_counter() - filtered_times.append(time.time() - start) + filtered_times.append(time.perf_counter() - start)
46-50: Also assert the warning for unknown namesCapturing the warning makes this test stronger and verifies the log contract.
You can enhance the test like this:
def test_load_builtin_toolsets_invalid_names(caplog): """Test that invalid toolset names are handled gracefully""" with caplog.at_level("WARNING"): filtered = load_builtin_toolsets(allowed_builtin_toolsets=["nonexistent/toolset"]) assert len(filtered) == 0 assert any("Unknown builtin toolsets specified" in rec.message for rec in caplog.records)holmes/core/toolset_manager.py (1)
28-35: Prefer a Protocol over Any for config typing (avoids circular import and improves type-safety)Using Any loses useful checks. A small Protocol avoids circular imports while documenting the expected shape.
Proposed pattern (outside this hunk):
from typing import Optional, List, Protocol class _HasAllowedBuiltinToolsets(Protocol): allowed_builtin_toolsets: Optional[List[str]]Then update the signature and attribute:
- def __init__( - self, - config: Optional[Any] = None, # Config instance + def __init__( + self, + config: Optional[_HasAllowedBuiltinToolsets] = None, # Config-like instancetests/cli/test_allowed_builtin_toolsets.py (1)
270-288: Fix unused loop variableThe loop variable
test_caseis not used within the loop body.Apply this fix to address the static analysis warning:
- for test_case in test_cases: + for _ in test_cases: # Just test that the option is parsed without error # We don't need to execute the full command result = runner.invoke(app, ["ask", "--help"])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between acb3ea2 and f663fb8925494e2af5046e5804133cd6f6307652.
📒 Files selected for processing (10)
holmes/config.py(4 hunks)holmes/core/toolset_manager.py(3 hunks)holmes/main.py(13 hunks)holmes/plugins/toolsets/__init__.py(2 hunks)tests/cli/__init__.py(1 hunks)tests/cli/test_allowed_builtin_toolsets.py(1 hunks)tests/core/test_config_allowed_builtin_toolsets.py(1 hunks)tests/core/test_toolset_manager_integration.py(1 hunks)tests/integration/test_allowed_builtin_toolsets_full.py(1 hunks)tests/plugins/test_toolsets_filtering.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.py: ALWAYS place Python imports at the top of the file, not inside functions or methods
Use Ruff for formatting and linting (configured in pyproject.toml)
Type hints required (mypy configuration in pyproject.toml)
Pre-commit hooks enforce quality checks
Don't add convenience logs that give away the problem
Don't write logs that directly state the issue
Ensure historical timestamps are properly handled in logs (especially with Loki)
Files:
tests/integration/test_allowed_builtin_toolsets_full.pytests/cli/__init__.pytests/core/test_config_allowed_builtin_toolsets.pyholmes/plugins/toolsets/__init__.pytests/plugins/test_toolsets_filtering.pytests/core/test_toolset_manager_integration.pyholmes/config.pyholmes/core/toolset_manager.pyholmes/main.pytests/cli/test_allowed_builtin_toolsets.py
tests/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Tests must match source structure under tests/
Files:
tests/integration/test_allowed_builtin_toolsets_full.pytests/cli/__init__.pytests/core/test_config_allowed_builtin_toolsets.pytests/plugins/test_toolsets_filtering.pytests/core/test_toolset_manager_integration.pytests/cli/test_allowed_builtin_toolsets.py
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-10T06:02:54.321Z
Learning: New toolsets require integration tests
📚 Learning: 2025-08-10T06:02:54.321Z
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-10T06:02:54.321Z
Learning: New toolsets require integration tests
Applied to files:
tests/integration/test_allowed_builtin_toolsets_full.pytests/plugins/test_toolsets_filtering.pytests/core/test_toolset_manager_integration.pytests/cli/test_allowed_builtin_toolsets.py
📚 Learning: 2025-07-08T08:45:41.069Z
Learnt from: nherment
PR: robusta-dev/holmesgpt#610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025-07-08T08:45:41.069Z
Learning: The robusta-dev/holmesgpt codebase has comprehensive existing validation for Azure environment variables (AZURE_API_BASE, AZURE_API_KEY, AZURE_API_VERSION) and MODEL in tests/llm/utils/classifiers.py, tests/llm/conftest.py, and holmes/core/llm.py. Don't suggest adding redundant validation logic.
Applied to files:
holmes/config.py
📚 Learning: 2025-06-24T05:51:04.543Z
Learnt from: nherment
PR: robusta-dev/holmesgpt#535
File: holmes/plugins/toolsets/bash/bash_toolset.py:207-209
Timestamp: 2025-06-24T05:51:04.543Z
Learning: The init_config method in toolsets should be idempotent - safely callable multiple times without errors. self.config should maintain consistent typing (not alternate between dict and config object types) throughout the object lifecycle.
Applied to files:
holmes/core/toolset_manager.py
🧬 Code Graph Analysis (5)
tests/core/test_config_allowed_builtin_toolsets.py (1)
holmes/config.py (2)
Config(70-522)load_from_file(190-227)
holmes/plugins/toolsets/__init__.py (1)
holmes/core/tools.py (1)
Toolset(333-475)
tests/plugins/test_toolsets_filtering.py (1)
holmes/plugins/toolsets/__init__.py (1)
load_builtin_toolsets(102-148)
tests/core/test_toolset_manager_integration.py (3)
holmes/config.py (1)
toolset_manager(147-156)holmes/core/toolset_manager.py (5)
ToolsetManager(19-454)_list_all_toolsets(76-143)_get_allowed_builtin_toolsets(70-74)list_console_toolsets(314-329)list_server_toolsets(332-347)holmes/core/tools.py (1)
ToolsetType(114-117)
holmes/core/toolset_manager.py (1)
holmes/plugins/toolsets/__init__.py (1)
load_builtin_toolsets(102-148)
🪛 Ruff (0.12.2)
tests/cli/test_allowed_builtin_toolsets.py
281-281: Loop control variable test_case not used within loop body
Rename unused test_case to _test_case
(B007)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Pre-commit checks
🔇 Additional comments (11)
tests/cli/__init__.py (1)
1-1: LGTM: package marker is fineThe package marker is appropriate and harmless.
holmes/core/toolset_manager.py (2)
70-75: LGTM: safe accessor for config-driven allowlistThe helper cleanly isolates config access and gracefully handles absence.
92-94: Wiring is correct; consider edge-case semantics when allowlist excludes overridden builtinsPassing allowed_builtin_toolsets into load_builtin_toolsets is correct. One edge case to verify: if a builtin toolset is excluded by allowlist but present in self.toolsets overrides, it will be treated as “custom” (type defaulted to CUSTOMIZED with strict_check=True), which may surprise users and/or fail validation. Confirm if that behavior is desired and covered by tests.
If you want to audit this scenario, add an integration test asserting the behavior when:
- allowlist excludes “kubernetes/logs”
- config overrides “kubernetes/logs: { enabled: true }”
Expected outcome: either
- the override is ignored (preferred); or
- it is treated as a custom toolset and validated strictly (current behavior).
I can draft the test if you confirm the intended semantics.
tests/integration/test_allowed_builtin_toolsets_full.py (1)
1-248: Comprehensive integration test coverage!The integration tests provide thorough coverage of the allowed_builtin_toolsets feature, including backward compatibility, error handling, warning scenarios, and end-to-end flows. The test suite effectively validates the filtering behavior across different components.
holmes/config.py (3)
119-136: Well-designed field validator with proper semanticsThe validator correctly implements the filtering logic, preserving the distinction between
None(no filtering) and[](filter everything), and handles whitespace gracefully.
149-150: Good design: Pass config reference to ToolsetManagerPassing
selfas the config parameter to ToolsetManager establishes proper dependency injection and enables the manager to access configuration settings likeallowed_builtin_toolsets.
208-217: Proper CLI option parsing for comma-separated valuesThe implementation correctly parses the comma-separated string from CLI and handles whitespace, maintaining consistency with the field validator.
holmes/main.py (2)
96-100: Clear and helpful CLI option documentationThe option is well-documented with a clear help message and example usage.
182-183: Consistent propagation of the new option across all commandsThe
allowed_builtin_toolsetsoption is consistently added to all relevant investigate commands and properly passed through toConfig.load_from_file.Also applies to: 249-249, 381-381, 413-413, 520-520, 545-545, 713-713, 738-738, 800-800, 824-824, 888-888, 912-912
tests/core/test_toolset_manager_integration.py (1)
1-252: Excellent test coverage for ToolsetManager integrationThe tests thoroughly validate the filtering behavior, backward compatibility, and integration between Config and ToolsetManager. The test structure is clear and covers all essential scenarios.
tests/cli/test_allowed_builtin_toolsets.py (1)
1-350: Comprehensive CLI testing with proper mockingThe tests thoroughly cover CLI option parsing, help text, and integration across all commands with appropriate mocking to isolate CLI behavior.
f663fb8 to
6011a1b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
holmes/core/toolset_manager.py (3)
36-37: Remove redundant assignment
self.toolsetsis assigned twice in a row. Keep only theor {}form.- self.toolsets = toolsets - self.toolsets = toolsets or {} + self.toolsets = toolsets or {}
70-75: Defensive normalization of config value (optional)If anything upstream accidentally passes a string (pre-parse) instead of list[str], normalize here defensively. Keeps ToolsetManager resilient to integration mis-wirings while still relying on Config to do the right thing.
def _get_allowed_builtin_toolsets(self) -> Optional[List[str]]: """Get allowed builtin toolsets from config.""" - if self._config is None: - return None - return getattr(self._config, "allowed_builtin_toolsets", None) + if self._config is None: + return None + value = getattr(self._config, "allowed_builtin_toolsets", None) + if isinstance(value, str): + # Accept comma-separated strings defensively + return [name.strip() for name in value.split(",") if name.strip()] + return value
453-455: Duplicate insertion into dictThe same dict assignment is performed twice; remove the duplicate line.
else: existing_toolsets_by_name[new_toolset.name] = new_toolset - existing_toolsets_by_name[new_toolset.name] = new_toolsettests/cli/test_allowed_builtin_toolsets.py (1)
356-373: Reduce duplication: factor out a small helper to assert help text contains the option (optional)Many tests repeat the same “option_present” block. Extracting a tiny helper will DRY the suite and ease future changes.
Example helper to add near the top of this file:
def _assert_help_contains_allowed_option(help_output: str) -> None: assert any( [ "--allowed-builtin-toolsets" in help_output, "--allowed-builti" in help_output, # truncated display resilience "allowed-builtin-toolsets" in help_output, ] ), f"Option not found in help output: {help_output}"Then replace repeated blocks with:
help_output = result.stdout _ assert_help_contains_allowed_option(help_output)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between f663fb8925494e2af5046e5804133cd6f6307652 and 6011a1b4aa50379a31a61e7df83e2283c9a7cb95.
📒 Files selected for processing (10)
holmes/config.py(4 hunks)holmes/core/toolset_manager.py(3 hunks)holmes/main.py(13 hunks)holmes/plugins/toolsets/__init__.py(2 hunks)tests/cli/__init__.py(1 hunks)tests/cli/test_allowed_builtin_toolsets.py(1 hunks)tests/core/test_config_allowed_builtin_toolsets.py(1 hunks)tests/core/test_toolset_manager_integration.py(1 hunks)tests/integration/test_allowed_builtin_toolsets_full.py(1 hunks)tests/plugins/test_toolsets_filtering.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/cli/init.py
- tests/plugins/test_toolsets_filtering.py
- tests/integration/test_allowed_builtin_toolsets_full.py
- holmes/plugins/toolsets/init.py
- holmes/main.py
- tests/core/test_config_allowed_builtin_toolsets.py
- holmes/config.py
- tests/core/test_toolset_manager_integration.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.py: ALWAYS place Python imports at the top of the file, not inside functions or methods
Use Ruff for formatting and linting (configured in pyproject.toml)
Type hints required (mypy configuration in pyproject.toml)
Pre-commit hooks enforce quality checks
Don't add convenience logs that give away the problem
Don't write logs that directly state the issue
Ensure historical timestamps are properly handled in logs (especially with Loki)
Files:
tests/cli/test_allowed_builtin_toolsets.pyholmes/core/toolset_manager.py
tests/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Tests must match source structure under tests/
Files:
tests/cli/test_allowed_builtin_toolsets.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-10T06:02:54.321Z
Learning: New toolsets require integration tests
📚 Learning: 2025-08-10T06:02:54.321Z
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-10T06:02:54.321Z
Learning: New toolsets require integration tests
Applied to files:
tests/cli/test_allowed_builtin_toolsets.py
📚 Learning: 2025-06-24T05:51:04.543Z
Learnt from: nherment
PR: robusta-dev/holmesgpt#535
File: holmes/plugins/toolsets/bash/bash_toolset.py:207-209
Timestamp: 2025-06-24T05:51:04.543Z
Learning: The init_config method in toolsets should be idempotent - safely callable multiple times without errors. self.config should maintain consistent typing (not alternate between dict and config object types) throughout the object lifecycle.
Applied to files:
holmes/core/toolset_manager.py
🧬 Code Graph Analysis (1)
holmes/core/toolset_manager.py (1)
holmes/plugins/toolsets/__init__.py (1)
load_builtin_toolsets(102-158)
🪛 Ruff (0.12.2)
tests/cli/test_allowed_builtin_toolsets.py
338-338: Loop control variable test_case not used within loop body
Rename unused test_case to _test_case
(B007)
🔇 Additional comments (4)
holmes/core/toolset_manager.py (2)
92-94: Correctly threads allowlist into builtin loaderGood call to pass
allowed_builtin_toolsetsdirectly toload_builtin_toolsets, aligning ToolsetManager with Config/CLI.
26-35: No internal API break — makingconfigkeyword-only is unnecessary hereRepository scan shows all current instantiations either pass config by keyword or use no args; no positional-only usages were found that would be misinterpreted by moving
configto the front.Key call sites inspected (representative):
- holmes/config.py:149-152 — ToolsetManager(config=self, toolsets=self.toolsets, mcp_servers=self.mcp_servers)
- tests/core/test_toolset_manager.py:21 — ToolsetManager() (fixture)
- tests/core/test_toolset_manager.py:336-339 — ToolsetManager(toolsets=None, mcp_servers=mcp_servers, custom_toolsets=None)
- tests/integration/test_allowed_builtin_toolsets_full.py — multiple calls using ToolsetManager(config=...)
Conclusion: the suggested signature change is not required for this codebase; you can ignore the original critical warning. If ToolsetManager is a public API consumed externally, consider making
configkeyword-only in a deliberate, documented release to avoid surprising downstream users.Likely an incorrect or invalid review comment.
tests/cli/test_allowed_builtin_toolsets.py (2)
8-24: Help coverage looks goodValidates the option is surfaced by the CLI and is resilient to truncated display; solid signal for CLI UX.
414-435: Nicely mirrors the parsing logic in ConfigThis test is a concise and faithful reproduction of the parsing behavior, catching regressions if the semantics change.
6011a1b to
ae7daf5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tests/plugins/test_toolsets_filtering.py (4)
6-13: Also assert order equality for None filter vs defaultNone filter should preserve not just contents but also discovery order. Add an explicit order assertion.
@@ assert len(toolsets_none) == len(toolsets_default) assert {t.name for t in toolsets_none} == {t.name for t in toolsets_default} + # Order should be preserved as well + assert [t.name for t in toolsets_none] == [t.name for t in toolsets_default]
17-20: DRY: factor repeated “discover then skip if empty” into a fixtureYou repeat the same pattern across many tests. A small fixture keeps tests concise and avoids repeated IO.
Add this fixture at module scope:
import pytest @pytest.fixture(scope="module") def builtin_toolsets_or_skip(): toolsets = load_builtin_toolsets() if not toolsets: pytest.skip("No builtin toolsets available") return toolsetsExample refactor for one test:
-def test_load_builtin_toolsets_with_single_filter(): - """Test filtering to single toolset""" - all_toolsets = load_builtin_toolsets() - if not all_toolsets: - pytest.skip("No builtin toolsets available") +def test_load_builtin_toolsets_with_single_filter(builtin_toolsets_or_skip): + """Test filtering to single toolset""" + all_toolsets = builtin_toolsets_or_skipYou can apply the same pattern to the other tests listed in this comment’s line ranges.
Also applies to: 30-33, 56-58, 81-84, 103-105, 138-140, 153-155, 172-174, 185-187, 200-202
47-51: Assert a warning is emitted for invalid names to lock behaviorload_builtin_toolsets warns on unknown names. Asserting the warning prevents silent regressions.
Apply these diffs:
- Import logging at top:
@@ -import pytest -import time -from holmes.plugins.toolsets import load_builtin_toolsets +import logging +import pytest +import time +from holmes.plugins.toolsets import load_builtin_toolsets
- Update the test to use caplog:
-def test_load_builtin_toolsets_invalid_names(): - """Test that invalid toolset names are handled gracefully""" - filtered = load_builtin_toolsets(allowed_builtin_toolsets=["nonexistent/toolset"]) - assert len(filtered) == 0 +def test_load_builtin_toolsets_invalid_names(caplog): + """Test that invalid toolset names are handled gracefully""" + with caplog.at_level(logging.WARNING): + filtered = load_builtin_toolsets(allowed_builtin_toolsets=["nonexistent/toolset"]) + assert len(filtered) == 0 + # Ensure we logged about the unknown names + assert any("Unknown builtin toolsets" in rec.getMessage() for rec in caplog.records)
67-98: Reduce flakiness in the performance test (use perf_counter + tiny warm-up)Use monotonic timing and a quick warm-up to stabilize timings across CI environments.
@@ - # Baseline measurements + # Warm-up (avoid one-time import/cache effects) + load_builtin_toolsets() + # Baseline measurements baseline_times = [] for _ in range(iterations): - start = time.time() + start = time.perf_counter() load_builtin_toolsets() - baseline_times.append(time.time() - start) + baseline_times.append(time.perf_counter() - start) @@ filtered_times = [] for _ in range(iterations): - start = time.time() + start = time.perf_counter() load_builtin_toolsets(allowed_builtin_toolsets=[all_toolsets[0].name]) - filtered_times.append(time.time() - start) + filtered_times.append(time.perf_counter() - start)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 6011a1b4aa50379a31a61e7df83e2283c9a7cb95 and ae7daf5ce6d1928a6bbd27ae95798bc3c6fd5a98.
📒 Files selected for processing (8)
holmes/config.py(4 hunks)holmes/core/toolset_manager.py(3 hunks)holmes/main.py(13 hunks)holmes/plugins/toolsets/__init__.py(2 hunks)tests/core/test_config_allowed_builtin_toolsets.py(1 hunks)tests/core/test_toolset_manager_integration.py(1 hunks)tests/integration/test_allowed_builtin_toolsets_full.py(1 hunks)tests/plugins/test_toolsets_filtering.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/core/test_config_allowed_builtin_toolsets.py
- tests/core/test_toolset_manager_integration.py
- tests/integration/test_allowed_builtin_toolsets_full.py
- holmes/plugins/toolsets/init.py
- holmes/main.py
- holmes/core/toolset_manager.py
- holmes/config.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.py: ALWAYS place Python imports at the top of the file, not inside functions or methods
Use Ruff for formatting and linting (configured in pyproject.toml)
Type hints required (mypy configuration in pyproject.toml)
Pre-commit hooks enforce quality checks
Don't add convenience logs that give away the problem
Don't write logs that directly state the issue
Ensure historical timestamps are properly handled in logs (especially with Loki)
Files:
tests/plugins/test_toolsets_filtering.py
tests/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Tests must match source structure under tests/
Files:
tests/plugins/test_toolsets_filtering.py
🧠 Learnings (1)
📚 Learning: 2025-08-10T06:02:54.321Z
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-10T06:02:54.321Z
Learning: New toolsets require integration tests
Applied to files:
tests/plugins/test_toolsets_filtering.py
🧬 Code Graph Analysis (1)
tests/plugins/test_toolsets_filtering.py (1)
holmes/plugins/toolsets/__init__.py (1)
load_builtin_toolsets(102-158)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Pre-commit checks
🔇 Additional comments (4)
tests/plugins/test_toolsets_filtering.py (4)
1-3: Imports at module top: compliant with guidelinesGood fix. Imports are now module-level as required by project pre-commit rules.
100-116: Property preservation assertions are correctGiven loader behavior sets type and path for builtins uniformly, these checks guard against accidental mutation during filtering.
118-133: DAL parameter interop is covered wellGood that you validate dal=None path and ensure filtering still applies with dal explicitly passed.
197-211: Order preservation test is on pointVerifies discovery-order stability while filtering—aligned with the implementation using set membership but preserving original order.
ae7daf5 to
2392fe1
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
holmes/main.py (2)
950-977: Expose the allowlist on toolset list/refresh commandsFor consistency, let users pass the same flag when listing/refreshing toolsets. Today, only config-file values apply here; adding the CLI option improves parity with the other commands.
Apply:
@toolset_app.command("list") def list_toolsets( - verbose: Optional[List[bool]] = opt_verbose, - config_file: Optional[Path] = opt_config_file, # type: ignore + verbose: Optional[List[bool]] = opt_verbose, + config_file: Optional[Path] = opt_config_file, # type: ignore + allowed_builtin_toolsets: Optional[str] = opt_allowed_builtin_toolsets, ): @@ - config = Config.load_from_file(config_file) + config = Config.load_from_file( + config_file, allowed_builtin_toolsets=allowed_builtin_toolsets + ) @@ @toolset_app.command("refresh") def refresh_toolsets( - verbose: Optional[List[bool]] = opt_verbose, - config_file: Optional[Path] = opt_config_file, # type: ignore + verbose: Optional[List[bool]] = opt_verbose, + config_file: Optional[Path] = opt_config_file, # type: ignore + allowed_builtin_toolsets: Optional[str] = opt_allowed_builtin_toolsets, ): @@ - config = Config.load_from_file(config_file) + config = Config.load_from_file( + config_file, allowed_builtin_toolsets=allowed_builtin_toolsets + )
593-623: Add --allowed-builtin-toolsets to ticket command and forward it through SourceFactory.create_sourceShort: Verified opt_allowed_builtin_toolsets exists (holmes/main.py:96) and is used by other commands, but the ticket command (holmes/main.py ~632) does not accept or forward it. SourceFactory.create_source (holmes/config.py ~535) also doesn't accept it and therefore doesn't pass it to Config.load_from_file. Config.load_from_file already handles allowed_builtin_toolsets, so we should thread the option through both places.
Files to change:
- holmes/main.py — add CLI param to ticket() and forward to SourceFactory.create_source.
- holmes/config.py — extend SourceFactory.create_source signature and pass allowed_builtin_toolsets into Config.load_from_file in both JIRA and PagerDuty branches.
Proposed diffs:
holmes/main.py (ticket command signature + call):
def ticket( @@ post_processing_prompt: Optional[str] = opt_post_processing_prompt, + allowed_builtin_toolsets: Optional[str] = opt_allowed_builtin_toolsets, ): @@ - ticket_source = SourceFactory.create_source( + ticket_source = SourceFactory.create_source( source=source, config_file=config_file, ticket_url=ticket_url, ticket_username=ticket_username, ticket_api_key=ticket_api_key, ticket_id=ticket_id, + allowed_builtin_toolsets=allowed_builtin_toolsets, )holmes/config.py (SourceFactory.create_source signature + Config.load_from_file calls):
class SourceFactory(BaseModel): @staticmethod def create_source( source: SupportedTicketSources, config_file: Optional[Path], ticket_url: Optional[str], ticket_username: Optional[str], ticket_api_key: Optional[str], ticket_id: Optional[str], + allowed_builtin_toolsets: Optional[str] = None, ) -> TicketSource: @@ - config = Config.load_from_file( + config = Config.load_from_file( config_file=config_file, api_key=None, model=None, max_steps=None, jira_url=ticket_url, jira_username=ticket_username, jira_api_key=ticket_api_key, jira_query=None, custom_toolsets=None, custom_runbooks=None, + allowed_builtin_toolsets=allowed_builtin_toolsets, ) @@ - config = Config.load_from_file( + config = Config.load_from_file( config_file=config_file, api_key=None, model=None, max_steps=None, pagerduty_api_key=ticket_api_key, pagerduty_user_email=ticket_username, pagerduty_incident_key=None, custom_toolsets=None, custom_runbooks=None, + allowed_builtin_toolsets=allowed_builtin_toolsets, )Notes:
- SourceFactory.create_source is only used at holmes/main.py:632, so updating its signature is safe.
- Config.load_from_file already parses the CLI string into a list (holmes/config.py lines ~208–214), so no change needed there.
🧹 Nitpick comments (4)
tests/core/test_config_allowed_builtin_toolsets.py (3)
29-49: Prefer parametrize over a for-loop for clearer failuresUsing pytest.mark.parametrize will yield better per-case reporting when a case fails (you’ll see exactly which input failed), and aligns with pytest best practices.
Apply:
-def test_config_handle_whitespace(tmp_path): - """Test handling of whitespace and empty strings""" - # Create a minimal config file for testing CLI argument parsing - config_file = tmp_path / "config.yaml" - config_content = """ -# Test config for whitespace handling -model: "gpt-4o" -""" - config_file.write_text(config_content) - - test_cases = [ - ("kubernetes/core, prometheus/core", ["kubernetes/core", "prometheus/core"]), - ("kubernetes/core, ,prometheus/core", ["kubernetes/core", "prometheus/core"]), - (" kubernetes/core ", ["kubernetes/core"]), - ("", []), - ] - - for input_str, expected in test_cases: - # Use the public API to test the parsing logic - config = Config.load_from_file(config_file, allowed_builtin_toolsets=input_str) - assert config.allowed_builtin_toolsets == expected +@pytest.mark.parametrize( + "input_str,expected", + [ + ("kubernetes/core, prometheus/core", ["kubernetes/core", "prometheus/core"]), + ("kubernetes/core, ,prometheus/core", ["kubernetes/core", "prometheus/core"]), + (" kubernetes/core ", ["kubernetes/core"]), + ("", []), + ], +) +def test_config_handle_whitespace(tmp_path, input_str, expected): + """Test handling of whitespace and empty strings""" + # Create a minimal config file for testing CLI argument parsing + config_file = tmp_path / "config.yaml" + config_file.write_text('model: "gpt-4o"\n') + + # Use the public API to test the parsing logic + config = Config.load_from_file(config_file, allowed_builtin_toolsets=input_str) + assert config.allowed_builtin_toolsets == expectedNote: add
import pytestat the top of this module if not present already.
4-8: Eliminate duplicate coverage for the default None caseBoth tests assert the same behavior (attribute exists and defaults to None). Consolidate into a single test to reduce redundancy and speed up the suite.
Apply:
def test_config_default_none(): """Test that default value is None for backward compatibility""" config = Config() - assert config.allowed_builtin_toolsets is None + assert hasattr(config, "allowed_builtin_toolsets") + assert config.allowed_builtin_toolsets is None @@ -def test_config_backward_compatibility(): - """Test that existing config loading works unchanged""" - config = Config() - assert hasattr(config, "allowed_builtin_toolsets") - assert config.allowed_builtin_toolsets is None +# Removed: duplicate of test_config_default_noneAlso applies to: 52-57
119-131: Consider adding negative-type tests to lock down validation behaviorCurrent tests cover accepted types. It’s useful to assert that invalid types (e.g., int, dict, list with non-str) are rejected by Pydantic validators to prevent accidental regressions.
Example to add elsewhere in this file:
import pytest @pytest.mark.parametrize("bad_value", [123, {"x": "y"}, [1, 2, 3], [None]]) def test_config_field_type_rejection(bad_value): with pytest.raises(Exception): Config(allowed_builtin_toolsets=bad_value) # type: ignore[arg-type]holmes/main.py (1)
96-100: Nit: prefer “built-in” spelling in help for consistencyElsewhere (e.g., ToolsetType) the spelling is “built-in”. Aligning improves UX consistency.
Apply:
-opt_allowed_builtin_toolsets: Optional[str] = typer.Option( +opt_allowed_builtin_toolsets: Optional[str] = typer.Option( None, "--allowed-builtin-toolsets", - help="Comma-separated list of builtin toolsets to allow (e.g., 'kubernetes/core,prometheus/core')", + help="Comma-separated list of built-in toolsets to allow (e.g., 'kubernetes/core,prometheus/core')", )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between ae7daf5ce6d1928a6bbd27ae95798bc3c6fd5a98 and 2392fe1c046b8191207653f14acbe219122a45e8.
📒 Files selected for processing (8)
holmes/config.py(4 hunks)holmes/core/toolset_manager.py(3 hunks)holmes/main.py(13 hunks)holmes/plugins/toolsets/__init__.py(2 hunks)tests/core/test_config_allowed_builtin_toolsets.py(1 hunks)tests/core/test_toolset_manager_integration.py(1 hunks)tests/integration/test_allowed_builtin_toolsets_full.py(1 hunks)tests/plugins/test_toolsets_filtering.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/plugins/test_toolsets_filtering.py
- holmes/plugins/toolsets/init.py
- tests/core/test_toolset_manager_integration.py
- holmes/config.py
- holmes/core/toolset_manager.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.py: ALWAYS place Python imports at the top of the file, not inside functions or methods
Use Ruff for formatting and linting (configured in pyproject.toml)
Type hints required (mypy configuration in pyproject.toml)
Pre-commit hooks enforce quality checks
Don't add convenience logs that give away the problem
Don't write logs that directly state the issue
Ensure historical timestamps are properly handled in logs (especially with Loki)
Files:
tests/core/test_config_allowed_builtin_toolsets.pytests/integration/test_allowed_builtin_toolsets_full.pyholmes/main.py
tests/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Tests must match source structure under tests/
Files:
tests/core/test_config_allowed_builtin_toolsets.pytests/integration/test_allowed_builtin_toolsets_full.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-10T06:02:54.321Z
Learning: New toolsets require integration tests
📚 Learning: 2025-08-10T06:02:54.321Z
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-10T06:02:54.321Z
Learning: New toolsets require integration tests
Applied to files:
tests/integration/test_allowed_builtin_toolsets_full.py
🧬 Code Graph Analysis (2)
tests/core/test_config_allowed_builtin_toolsets.py (1)
holmes/config.py (2)
Config(70-522)load_from_file(190-227)
tests/integration/test_allowed_builtin_toolsets_full.py (4)
holmes/config.py (3)
Config(70-522)toolset_manager(147-156)load_from_file(190-227)holmes/core/toolset_manager.py (3)
_list_all_toolsets(76-143)list_console_toolsets(314-329)list_server_toolsets(332-347)holmes/plugins/toolsets/__init__.py (1)
load_builtin_toolsets(102-158)holmes/core/tools.py (1)
ToolsetType(114-117)
🔇 Additional comments (7)
tests/core/test_config_allowed_builtin_toolsets.py (2)
10-27: Good: exercising the real parsing path via Config.load_from_fileThis test correctly drives the public API and avoids duplicating parsing logic. Creating a minimal config file and overriding via CLI kwargs ensures future parsing changes are covered.
59-75: LGTM: strong coverage on CLI parsing edge-casesThese independently validate handling of None, empty string, and whitespace via the real load_from_file merge path. This aligns with the intended None vs [] semantics in holmes/config.py.
holmes/main.py (2)
96-100: Add CLI allowlist: good defaulting and helpOption is Optional[str] with default None, so it won’t override config-file values. Help text is clear.
182-183: Propagation across commands looks correctYou thread the new option through the CLI signatures and into Config.load_from_file for ask, alertmanager, jira, github, pagerduty, and opsgenie. This matches the parsing contract in Config.load_from_file (string → list).
Also applies to: 243-252, 381-414, 520-546, 713-739, 800-825, 888-913
tests/integration/test_allowed_builtin_toolsets_full.py (3)
136-156: LGTM: order preservation test mirrors loader behaviorVerifies that filtering does not reorder the source discovery order. This matches load_builtin_toolsets semantics.
168-184: LGTM: empty filter semantics validatedConfirms [] ⇒ no built-ins (distinct from None). Non-built-ins aren’t constrained by this feature.
226-248: Solid robustness coverage across edge casesCovers None vs [] vs duplicates vs case sensitivity vs extra path components, ensuring no crashes on odd inputs.
2392fe1 to
f92ea9d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
holmes/plugins/toolsets/__init__.py (2)
139-141: Standardize spelling: “built-in” (hyphenated) in logsConsistency nit: elsewhere the enum uses “built-in” (ToolsetType.BUILTIN.value == "built-in"). Consider standardizing log strings.
Apply within this hunk:
- logging.warning( - f"Unknown builtin toolsets specified: {sorted(invalid_names)}" - ) + logging.warning( + f"Unknown built-in toolsets specified: {sorted(invalid_names)}" + )
106-113: Nit: Align docstring wording with enum (“built-in” vs “builtin”)Minor consistency update for the docstring to match ToolsetType naming.
- Load builtin toolsets from YAML files and Python modules. + Load built-in toolsets from YAML files and Python modules.holmes/main.py (1)
96-100: Nit: Use “built-in” (hyphenated) in CLI help for consistencyAligns user-facing help with enum spelling and logs.
-opt_allowed_builtin_toolsets: Optional[str] = typer.Option( +opt_allowed_builtin_toolsets: Optional[str] = typer.Option( None, "--allowed-builtin-toolsets", - help="Comma-separated list of builtin toolsets to allow (e.g., 'kubernetes/core,prometheus/core')", + help="Comma-separated list of built-in toolsets to allow (e.g., 'kubernetes/core,prometheus/core')", )
🧹 Nitpick comments (4)
tests/plugins/test_toolsets_filtering.py (2)
46-50: Also assert the warning is emitted for invalid toolset namesStrengthen this test by verifying the warning log when invalid names are passed.
-def test_load_builtin_toolsets_invalid_names(): +def test_load_builtin_toolsets_invalid_names(caplog): """Test that invalid toolset names are handled gracefully""" - filtered = load_builtin_toolsets(allowed_builtin_toolsets=["nonexistent/toolset"]) + with caplog.at_level("WARNING"): + filtered = load_builtin_toolsets(allowed_builtin_toolsets=["nonexistent/toolset"]) assert len(filtered) == 0 + # Adjust the expected substring if you keep “builtin” instead of “built-in” + assert any("Unknown built-in toolsets specified" in m for m in caplog.messages)
66-82: Assert explicit invariants (type and path) for filtered built-insSince built-in toolsets are marked with ToolsetType.BUILTIN and path is hidden, assert these invariants directly in addition to property equality.
-import pytest -from holmes.plugins.toolsets import load_builtin_toolsets +import pytest +from holmes.core.tools import ToolsetType +from holmes.plugins.toolsets import load_builtin_toolsets @@ # Check that properties are preserved assert filtered_toolset.name == target_toolset.name - assert filtered_toolset.type == target_toolset.type - assert filtered_toolset.path == target_toolset.path + assert filtered_toolset.type == target_toolset.type + assert filtered_toolset.path == target_toolset.path + # Built-in invariants + assert filtered_toolset.type == ToolsetType.BUILTIN + assert filtered_toolset.path is Nonetests/core/test_toolset_manager_integration.py (2)
91-101: Also assert that a warning was logged for invalid namesThis ensures the warning path is covered in integration, not only at the plugin level.
-def test_toolset_manager_invalid_names(): +def test_toolset_manager_invalid_names(caplog): """Test ToolsetManager with invalid toolset names""" config = Config(allowed_builtin_toolsets=["nonexistent/toolset"]) manager = ToolsetManager(config=config) - toolsets = manager._list_all_toolsets() + with caplog.at_level("WARNING"): + toolsets = manager._list_all_toolsets() builtin_toolsets = [t for t in toolsets if t.type == ToolsetType.BUILTIN] # Should have no builtin toolsets (invalid names filtered out) assert len(builtin_toolsets) == 0 + # Adjust the expected substring if you keep “builtin” instead of “built-in” + assert any("Unknown built-in toolsets specified" in m for m in caplog.messages)
103-118: Use of private method in tests may be brittleMultiple tests call _list_all_toolsets(), which is private API. If a public aggregator exists, prefer it to reduce coupling to internals; otherwise, keep as-is with the understanding it may require updates on refactors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 2392fe1c046b8191207653f14acbe219122a45e8 and f92ea9d07bf6b3609ae053779239d510d8b643ef.
📒 Files selected for processing (8)
holmes/config.py(4 hunks)holmes/core/toolset_manager.py(3 hunks)holmes/main.py(13 hunks)holmes/plugins/toolsets/__init__.py(2 hunks)tests/core/test_config_allowed_builtin_toolsets.py(1 hunks)tests/core/test_toolset_manager_integration.py(1 hunks)tests/integration/test_allowed_builtin_toolsets_full.py(1 hunks)tests/plugins/test_toolsets_filtering.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/core/test_config_allowed_builtin_toolsets.py
- tests/integration/test_allowed_builtin_toolsets_full.py
- holmes/config.py
- holmes/core/toolset_manager.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.py: ALWAYS place Python imports at the top of the file, not inside functions or methods
Use Ruff for formatting and linting (configured in pyproject.toml)
Type hints required (mypy configuration in pyproject.toml)
Pre-commit hooks enforce quality checks
Don't add convenience logs that give away the problem
Don't write logs that directly state the issue
Ensure historical timestamps are properly handled in logs (especially with Loki)
Files:
holmes/plugins/toolsets/__init__.pytests/core/test_toolset_manager_integration.pyholmes/main.pytests/plugins/test_toolsets_filtering.py
tests/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Tests must match source structure under tests/
Files:
tests/core/test_toolset_manager_integration.pytests/plugins/test_toolsets_filtering.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-10T06:02:54.321Z
Learning: New toolsets require integration tests
📚 Learning: 2025-08-10T06:02:54.321Z
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-10T06:02:54.321Z
Learning: New toolsets require integration tests
Applied to files:
tests/core/test_toolset_manager_integration.pytests/plugins/test_toolsets_filtering.py
🧬 Code Graph Analysis (2)
holmes/plugins/toolsets/__init__.py (1)
holmes/core/tools.py (2)
Toolset(333-475)ToolsetType(114-117)
tests/plugins/test_toolsets_filtering.py (1)
holmes/plugins/toolsets/__init__.py (1)
load_builtin_toolsets(102-158)
🔇 Additional comments (6)
holmes/plugins/toolsets/__init__.py (3)
102-113: Docstring explicitly states order semantics — good clarityThe docstring clearly documents that filtering preserves discovery order. This avoids ambiguity around ordering guarantees.
132-151: Filtering logic is correct and preserves discovery order
- Efficient set-based check
- Warns on unknown names
- Preserves discovery order by design (and as documented)
152-158: Correctly marks toolsets as built-in and hides pathsSetting ToolsetType and clearing path is aligned with the model’s semantics and security expectations.
tests/plugins/test_toolsets_filtering.py (1)
163-176: Order preservation test is solidGood coverage verifying that filtering maintains loader (discovery) order.
holmes/main.py (1)
182-183: No change needed — Config.load_from_file already parses CSV into List[str]Confirmed: holmes/config.py defines allowed_builtin_toolsets as Optional[List[str]] and load_from_file converts a CSV string into a List[str]; ToolsetManager accepts Optional[List[str]] and forwards it to load_builtin_toolsets.
Affected locations:
- holmes/config.py
- allowed_builtin_toolsets declaration & validator: ~115–127
- load_from_file signature: ~188–190
- CSV parsing in load_from_file: ~206–216
- holmes/core/toolset_manager.py
- _get_allowed_builtin_toolsets definition: ~70–74
- usage when loading builtin toolsets: ~91–94
No CLI-layer parsing change required.
tests/core/test_toolset_manager_integration.py (1)
148-158: Config.toolset_manager caching test is goodVerifies property-level caching behavior with identity checks.
f92ea9d to
88e2ee1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
tests/core/test_config_allowed_builtin_toolsets.py (1)
59-132: Add precedence tests: CLI should override file; empty CLI value should override to []Strengthen coverage by verifying merge behavior between file and CLI inputs.
Consider adding this test to the module:
def test_config_cli_overrides_file(tmp_path): # File sets a value; CLI should override it config_file = tmp_path / "config.yaml" config_file.write_text('allowed_builtin_toolsets: ["file/toolset"]\n') cfg = Config.load_from_file(config_file, allowed_builtin_toolsets="cli/toolset") assert cfg.allowed_builtin_toolsets == ["cli/toolset"] def test_config_cli_empty_string_overrides_file_to_empty(tmp_path): # File sets a value; CLI empty string should override to [] config_file = tmp_path / "config.yaml" config_file.write_text('allowed_builtin_toolsets: ["file/toolset"]\n') cfg = Config.load_from_file(config_file, allowed_builtin_toolsets="") assert cfg.allowed_builtin_toolsets == []If you want, I can open a follow-up PR adding these tests.
🧹 Nitpick comments (4)
tests/core/test_config_allowed_builtin_toolsets.py (2)
29-50: Parametrize the whitespace parsing cases for clearer failures and less boilerplateUse pytest.parametrize instead of a manual loop; each case will then be reported independently with better diagnostics.
Apply:
+import pytest @@ -def test_config_handle_whitespace(tmp_path): - """Test handling of whitespace and empty strings""" - # Create a minimal config file for testing CLI argument parsing - config_file = tmp_path / "config.yaml" - config_content = """ -# Test config for whitespace handling -model: "gpt-4o" -""" - config_file.write_text(config_content) - - test_cases = [ - ("kubernetes/core, prometheus/core", ["kubernetes/core", "prometheus/core"]), - ("kubernetes/core, ,prometheus/core", ["kubernetes/core", "prometheus/core"]), - (" kubernetes/core ", ["kubernetes/core"]), - ("", []), - ] - - for input_str, expected in test_cases: - # Use the public API to test the parsing logic - config = Config.load_from_file(config_file, allowed_builtin_toolsets=input_str) - assert config.allowed_builtin_toolsets == expected +@pytest.mark.parametrize( + "input_str,expected", + [ + ("kubernetes/core, prometheus/core", ["kubernetes/core", "prometheus/core"]), + ("kubernetes/core, ,prometheus/core", ["kubernetes/core", "prometheus/core"]), + (" kubernetes/core ", ["kubernetes/core"]), + ("", []), + ], +) +def test_config_handle_whitespace(tmp_path, input_str, expected): + """Test handling of whitespace and empty strings""" + # Create a minimal config file for testing CLI argument parsing + config_file = tmp_path / "config.yaml" + config_content = """ +# Test config for whitespace handling +model: "gpt-4o" +""" + config_file.write_text(config_content) + + # Use the public API to test the parsing logic + config = Config.load_from_file(config_file, allowed_builtin_toolsets=input_str) + assert config.allowed_builtin_toolsets == expectedNote: Add the new top-level import if not already present.
52-57: Deduplicate: Merge this with test_config_default_noneThis test repeats the same assertion as Lines 4–8. Fold the hasattr check into the first test and remove this function.
Apply:
-def test_config_backward_compatibility(): - """Test that existing config loading works unchanged""" - config = Config() - assert hasattr(config, "allowed_builtin_toolsets") - assert config.allowed_builtin_toolsets is None +def test_config_default_none(): + """Test that default value is None for backward compatibility""" + config = Config() + assert hasattr(config, "allowed_builtin_toolsets") + assert config.allowed_builtin_toolsets is Nonetests/plugins/test_toolsets_filtering.py (2)
46-50: Also assert the warning for invalid names using caplogThe implementation logs a warning for unknown names. Capture and assert it to lock the behavior.
Apply:
-def test_load_builtin_toolsets_invalid_names(): +def test_load_builtin_toolsets_invalid_names(caplog): """Test that invalid toolset names are handled gracefully""" - filtered = load_builtin_toolsets(allowed_builtin_toolsets=["nonexistent/toolset"]) + with caplog.at_level("WARNING"): + filtered = load_builtin_toolsets( + allowed_builtin_toolsets=["nonexistent/toolset"] + ) assert len(filtered) == 0 + assert any( + "Unknown builtin toolsets specified" in r.message for r in caplog.records + )
66-82: Strengthen property check: assert path is not exposedPer implementation, builtin toolsets should not expose a path. Assert this explicitly.
Apply:
filtered_toolset = filtered[0] @@ assert filtered_toolset.type == target_toolset.type - assert filtered_toolset.path == target_toolset.path + assert filtered_toolset.path == target_toolset.path + # Builtin toolsets must not expose a path + assert filtered_toolset.path is None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between f92ea9d07bf6b3609ae053779239d510d8b643ef and 88e2ee15dac312889bfd3e25168450b25be44fde.
📒 Files selected for processing (8)
holmes/config.py(4 hunks)holmes/core/toolset_manager.py(3 hunks)holmes/main.py(13 hunks)holmes/plugins/toolsets/__init__.py(2 hunks)tests/core/test_config_allowed_builtin_toolsets.py(1 hunks)tests/core/test_toolset_manager_integration.py(1 hunks)tests/integration/test_allowed_builtin_toolsets_full.py(1 hunks)tests/plugins/test_toolsets_filtering.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/integration/test_allowed_builtin_toolsets_full.py
- holmes/main.py
- holmes/core/toolset_manager.py
- holmes/plugins/toolsets/init.py
- tests/core/test_toolset_manager_integration.py
- holmes/config.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.py: ALWAYS place Python imports at the top of the file, not inside functions or methods
Use Ruff for formatting and linting (configured in pyproject.toml)
Type hints required (mypy configuration in pyproject.toml)
Pre-commit hooks enforce quality checks
Don't add convenience logs that give away the problem
Don't write logs that directly state the issue
Ensure historical timestamps are properly handled in logs (especially with Loki)
Files:
tests/core/test_config_allowed_builtin_toolsets.pytests/plugins/test_toolsets_filtering.py
tests/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Tests must match source structure under tests/
Files:
tests/core/test_config_allowed_builtin_toolsets.pytests/plugins/test_toolsets_filtering.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-10T06:02:54.321Z
Learning: New toolsets require integration tests
📚 Learning: 2025-08-10T06:02:54.321Z
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-10T06:02:54.321Z
Learning: New toolsets require integration tests
Applied to files:
tests/plugins/test_toolsets_filtering.py
🧬 Code Graph Analysis (2)
tests/core/test_config_allowed_builtin_toolsets.py (1)
holmes/config.py (2)
Config(70-522)load_from_file(190-227)
tests/plugins/test_toolsets_filtering.py (1)
holmes/plugins/toolsets/__init__.py (1)
load_builtin_toolsets(102-158)
🔇 Additional comments (11)
tests/core/test_config_allowed_builtin_toolsets.py (2)
4-8: LGTM: Backward compatibility default is explicit and correctDefaulting to None is the right choice to preserve existing behavior.
10-27: Good: Test exercises real parsing path via Config.load_from_fileDriving the check through the actual API (with a temp file present) avoids duplicating parsing logic. Well done.
tests/plugins/test_toolsets_filtering.py (9)
5-12: LGTM: No-filter behavior parityEnsures None matches default discovery; correct baseline.
14-25: LGTM: Single-name filteringBasic selection path is covered; skip guard is appropriate when no toolsets exist.
40-44: LGTM: Empty filter returns no toolsetsClear and correct expectation.
52-64: LGTM: Mixed valid/invalid namesCorrectly ensures only valid matches survive.
84-99: LGTM: dal parameter interplayCovers dal=None baseline and filtered path. Looks good.
101-114: LGTM: Case-sensitive matchingGood negative test to prevent accidental case-folding.
116-133: LGTM: Exact-name matchingProtects against substring matches; solid.
135-146: LGTM: All-valid allowlist returns allUseful parity check with baseline discovery.
148-161: LGTM: Duplicate names don’t duplicate resultsConfirms idempotence; nice catch.
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make order assertion robust: reverse the allowlist to catch reordering
As written, the allowlist order matches discovery order, so the test can’t detect an implementation that reorders by the allowlist. Reverse the input and assert discovery order in the output.
Apply:
- target_names = [all_toolsets[0].name, all_toolsets[1].name]
+ # Reverse order to ensure filtering preserves discovery order rather than allowlist order
+ target_names = [all_toolsets[1].name, all_toolsets[0].name]
filtered = load_builtin_toolsets(allowed_builtin_toolsets=target_names)
- assert len(filtered) == 2
- assert {t.name for t in filtered} == set(target_names)
+ assert len(filtered) == 2
+ # Names should match, but order should remain discovery order
+ assert [t.name for t in filtered] == [all_toolsets[0].name, all_toolsets[1].name]🤖 Prompt for AI Agents
In tests/plugins/test_toolsets_filtering.py around lines 27 to 38, the test
currently uses an allowlist in the same order as discovery so it can't detect
reordering; change target_names to use the reverse of the two chosen builtin
toolset names before calling
load_builtin_toolsets(allowed_builtin_toolsets=...), then assert the returned
filtered list preserves discovery order by comparing [t.name for t in filtered]
== target_names (keep or remove the set-based assertion if redundant). Ensure
the test still skips when <2 toolsets.
88e2ee1 to
482e102
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
holmes/plugins/toolsets/__init__.py (3)
106-113: Docstring clarity is good; consider making discovery order deterministicThe order semantics are clearly documented. To ensure determinism across platforms/filesystems, consider sorting the YAML filenames so that “discovery order” is stable.
Outside the selected lines, update the loop to sort filenames:
for filename in sorted(os.listdir(THIS_DIR)): ...
138-141: Nit: friendlier, structured warning messageUse logger parameterization and a comma-separated list for readability.
- if invalid_names: - logging.warning( - f"Unknown builtin toolsets specified: {sorted(invalid_names)}" - ) + if invalid_names: + unknown = ", ".join(sorted(invalid_names)) + logging.warning("Unknown builtin toolsets specified: %s", unknown)
152-156: Clarify misleading comment or enforce it explicitlyThe comment states “disable builtin toolsets by default,” but the code doesn’t change the enabled flag. Either clarify the comment to reflect what’s actually done, or explicitly set enabled = False.
Clarify the comment:
- # disable builtin toolsets by default, and the user can enable them explicitly in config. + # Mark builtin toolsets and hide their internal path; enabling remains explicit in user config.If you prefer enforcing the invariant:
for toolset in all_toolsets: toolset.type = ToolsetType.BUILTIN toolset.enabled = False toolset.path = Nonetests/plugins/test_toolsets_filtering.py (5)
1-2: Import ToolsetType to assert intended invariants (not just “same as target”)Asserting explicit invariants makes the test more robust than comparing two instances that are both post-processed by the loader.
import pytest +from holmes.core.tools import ToolsetType from holmes.plugins.toolsets import load_builtin_toolsets
48-52: Assert warning is emitted for invalid toolsetsCapture logs to ensure an explicit warning is emitted when unknown names are provided.
-def test_load_builtin_toolsets_invalid_names(): +def test_load_builtin_toolsets_invalid_names(caplog): """Test that invalid toolset names are handled gracefully""" - filtered = load_builtin_toolsets(allowed_builtin_toolsets=["nonexistent/toolset"]) + with caplog.at_level("WARNING"): + filtered = load_builtin_toolsets(allowed_builtin_toolsets=["nonexistent/toolset"]) assert len(filtered) == 0 + assert "Unknown builtin toolsets specified" in caplog.text
80-84: Assert invariants explicitly (BUILTIN type and hidden path)Strengthen the test to check the contract stated by the loader (type set to BUILTIN, path cleared), not just equality to another post-processed instance.
- # Check that properties are preserved - assert filtered_toolset.name == target_toolset.name - assert filtered_toolset.type == target_toolset.type - assert filtered_toolset.path == target_toolset.path + # Check invariants on the filtered toolset + assert filtered_toolset.name == target_toolset.name + assert filtered_toolset.type == ToolsetType.BUILTIN + assert filtered_toolset.path is None
137-148: Also assert order is preserved when all names are allowedThis catches accidental reordering when the allowlist contains all toolsets.
assert len(filtered) == len(all_toolsets) assert {t.name for t in filtered} == {t.name for t in all_toolsets} + assert [t.name for t in filtered] == [t.name for t in all_toolsets]
5-176: Reduce test runtime by reusing a cached list of toolsetsMultiple tests repeatedly call load_builtin_toolsets(), which reloads and instantiates all toolsets each time. Introduce a module-scoped fixture to cache results and pass it to tests that don’t depend on side-effects.
Example (apply outside this hunk):
import pytest from holmes.plugins.toolsets import load_builtin_toolsets @pytest.fixture(scope="module") def builtin_toolsets(): ts = load_builtin_toolsets() if not ts: pytest.skip("No builtin toolsets available") return ts def test_load_builtin_toolsets_with_single_filter(builtin_toolsets): first_toolset_name = builtin_toolsets[0].name filtered = load_builtin_toolsets(allowed_builtin_toolsets=[first_toolset_name]) assert len(filtered) == 1 assert filtered[0].name == first_toolset_name
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 88e2ee15dac312889bfd3e25168450b25be44fde and 482e102.
📒 Files selected for processing (8)
holmes/config.py(4 hunks)holmes/core/toolset_manager.py(3 hunks)holmes/main.py(13 hunks)holmes/plugins/toolsets/__init__.py(2 hunks)tests/core/test_config_allowed_builtin_toolsets.py(1 hunks)tests/core/test_toolset_manager_integration.py(1 hunks)tests/integration/test_allowed_builtin_toolsets_full.py(1 hunks)tests/plugins/test_toolsets_filtering.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- holmes/main.py
- tests/core/test_toolset_manager_integration.py
- tests/integration/test_allowed_builtin_toolsets_full.py
- holmes/config.py
- tests/core/test_config_allowed_builtin_toolsets.py
- holmes/core/toolset_manager.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.py: ALWAYS place Python imports at the top of the file, not inside functions or methods
Use Ruff for formatting and linting (configured in pyproject.toml)
Type hints required (mypy configuration in pyproject.toml)
Pre-commit hooks enforce quality checks
Don't add convenience logs that give away the problem
Don't write logs that directly state the issue
Ensure historical timestamps are properly handled in logs (especially with Loki)
Files:
tests/plugins/test_toolsets_filtering.pyholmes/plugins/toolsets/__init__.py
tests/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Tests must match source structure under tests/
Files:
tests/plugins/test_toolsets_filtering.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-10T06:02:54.321Z
Learning: New toolsets require integration tests
🧬 Code Graph Analysis (2)
tests/plugins/test_toolsets_filtering.py (1)
holmes/plugins/toolsets/__init__.py (1)
load_builtin_toolsets(102-158)
holmes/plugins/toolsets/__init__.py (1)
holmes/core/tools.py (2)
Toolset(333-475)ToolsetType(114-117)
| # Apply filter if specified | ||
| if allowed_builtin_toolsets is not None: | ||
| # Convert to set for O(1) lookup performance | ||
| requested_names = set(allowed_builtin_toolsets) | ||
| actual_names = {toolset.name for toolset in all_toolsets} | ||
| invalid_names = requested_names - actual_names | ||
|
|
||
| if invalid_names: | ||
| logging.warning( | ||
| f"Unknown builtin toolsets specified: {sorted(invalid_names)}" | ||
| ) | ||
|
|
||
| # Filter to only allowed toolsets using set for efficient lookup | ||
| # Note: This preserves the original discovery order rather than | ||
| # reordering by the allowlist | ||
| all_toolsets = [ | ||
| toolset | ||
| for toolset in all_toolsets | ||
| if toolset.name in requested_names # O(1) lookup | ||
| ] | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Allowlist still loads/instantiates all toolsets; short-circuit and avoid unnecessary work
Currently, all YAML and Python toolsets are loaded/instantiated and only then filtered. This undermines the goal of speeding up scenarios where most builtins aren’t needed.
Two concrete improvements:
- Early return for an empty allowlist (fast path).
- After loading YAML toolsets, if the allowlist is fully satisfied by YAML names, skip constructing Python toolsets altogether.
Apply a minimal fast-path within this hunk:
# Apply filter if specified
if allowed_builtin_toolsets is not None:
+ # Fast path: empty allowlist means nothing to load
+ if len(allowed_builtin_toolsets) == 0:
+ return []
# Convert to set for O(1) lookup performance
requested_names = set(allowed_builtin_toolsets)Outside the selected lines, consider this structure to skip Python toolsets when not needed:
def load_builtin_toolsets(
dal: Optional[SupabaseDal] = None,
allowed_builtin_toolsets: Optional[List[str]] = None,
) -> List[Toolset]:
logging.debug(f"loading toolsets from {THIS_DIR}")
if allowed_builtin_toolsets is not None:
requested = set(allowed_builtin_toolsets)
if not requested:
return []
# Load YAML toolsets first (sorted for determinism)
all_toolsets: List[Toolset] = []
for filename in sorted(os.listdir(THIS_DIR)):
if not filename.endswith(".yaml"):
continue
if filename == "kubernetes_logs.yaml" and not USE_LEGACY_KUBERNETES_LOGS:
continue
all_toolsets.extend(load_toolsets_from_file(os.path.join(THIS_DIR, filename), strict_check=True))
# If an allowlist is provided and all requested names are covered by YAML, skip Python loading
if allowed_builtin_toolsets is not None:
yaml_names = {t.name for t in all_toolsets}
if requested.issubset(yaml_names):
filtered = [t for t in all_toolsets if t.name in requested]
for t in filtered:
t.type = ToolsetType.BUILTIN
t.path = None
return filtered
# Otherwise, load Python toolsets and filter as today
all_toolsets.extend(load_python_toolsets(dal=dal))
if allowed_builtin_toolsets is not None:
requested = set(allowed_builtin_toolsets)
invalid = requested - {t.name for t in all_toolsets}
if invalid:
logging.warning("Unknown builtin toolsets specified: %s", ", ".join(sorted(invalid)))
all_toolsets = [t for t in all_toolsets if t.name in requested]
for t in all_toolsets:
t.type = ToolsetType.BUILTIN
t.path = None
return all_toolsets🤖 Prompt for AI Agents
In holmes/plugins/toolsets/__init__.py around lines 131 to 151, the allowlist is
applied after all YAML and Python toolsets are loaded which still instantiates
everything; change the logic to (1) early-return [] when
allowed_builtin_toolsets is provided as an empty list, (2) load YAML toolsets
first, build requested = set(allowed_builtin_toolsets) and if requested is a
subset of the loaded YAML names then filter those YAML toolsets, set each
filtered toolset's type to ToolsetType.BUILTIN and path to None, and return the
filtered list without loading Python toolsets, and (3) otherwise proceed to load
Python toolsets and perform the existing allowlist filtering and unknown-name
logging; ensure lookups use sets for O(1) checks and preserve discovery order
when filtering.
|
/close |
2 participants
This is very useful when the majority of builtin toolsets are useless.