feat: cache toolset status and add toolset management tool command #459
Conversation
Summary by CodeRabbit
WalkthroughThis update centralizes toolset management by introducing a Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Config
participant ToolsetManager
participant Toolset
participant Cache
CLI->>Config: load_from_file(config_file, custom_toolsets)
Config->>ToolsetManager: initialize(toolsets, custom_toolsets)
Config->>ToolsetManager: .load_toolset_with_status()
ToolsetManager->>Cache: load cached toolset status
ToolsetManager->>Toolset: check prerequisites (if refresh needed)
Toolset->>ToolsetManager: return status, error
ToolsetManager->>Cache: update cache if needed
ToolsetManager->>Config: return list of Toolset objects with status
Config->>CLI: provide toolsets for CLI commands
CLI->>CLI: pretty_print_toolset_status(toolsets)
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
✨ 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: 15
🔭 Outside diff range comments (2)
holmes/core/tools.py (2)
490-490:⚠️ Potential issueFix incorrect attribute reference after refactoring.
The code references
ts._pathbut the attribute has been changed from private to public aspath.Apply this diff to fix the attribute reference:
- f"Overriding existing tool '{tool.name} with new tool from {ts.name} at {ts._path}'!" + f"Overriding existing tool '{tool.name} with new tool from {ts.name} at {ts.path}'!"
339-349: 💡 Verification agent🧩 Analysis chain
Verify type consistency in override_with method.
The method signature changed from accepting
ToolsetYamlFromConfigtoYAMLToolset. However, based on the class structure and the docstring mentioning "ToolsetYamlFromConfig loaded from custom config", this might be incorrect.
🏁 Script executed:
#!/bin/bash # Description: Check how override_with is called in the codebase to verify the correct parameter type # Search for calls to override_with method rg -A 3 "\.override_with\(" # Search for instantiations of ToolsetYamlFromConfig rg -A 5 "ToolsetYamlFromConfig\("Length of output: 1387
🏁 Script executed:
#!/bin/bash # Search for YAMLToolset definition rg -n "YAMLToolset" . # Show override_with usage context in toolset_manager.py rg -C 5 "override_with" -n holmes/core/toolset_manager.py # Locate new_toolset declaration and type in toolset_manager.py rg -n "new_toolset" -n holmes/core/toolset_manager.pyLength of output: 1903
Update docstring in override_with to reference YAMLToolset
The parameter type
YAMLToolsetis correct and matches all call sites (e.g. intoolset_manager.py). The docstring still refers to the oldToolsetYamlFromConfigand should be updated.• File:
holmes/core/tools.py, around line 339
• Change the docstring from mentioningToolsetYamlFromConfigtoYAMLToolsetSuggested diff:
def override_with(self, override: "YAMLToolset") -> None: - """ - Overrides the current attributes with values from the ToolsetYamlFromConfig loaded from custom config - if they are not None. - """ + """ + Overrides the current attributes with values from the YAMLToolset loaded from custom config + if they are not None. + """ for field, value in override.model_dump( exclude_unset=True, exclude=("name"), # type: ignore ).items(): if field in self.model_fields and value not in (None, [], {}, ""): setattr(self, field, value)
🧹 Nitpick comments (5)
holmes/utils/env.py (1)
23-30: Consider refactoring to reduce code duplication.The pattern for handling
strandSecretStris very similar and could be refactored to reduce duplication.Consider extracting a helper function:
def process_string_value(value, is_secret=False): str_value = value.get_secret_value() if is_secret else value env_var_value = get_env_replacement(str_value) if env_var_value: return SecretStr(env_var_value) if is_secret else env_var_value return valueThen simplify the main logic:
- if isinstance(value, str): - env_var_value = get_env_replacement(value) - if env_var_value: - values[key] = env_var_value - elif isinstance(value, SecretStr): - env_var_value = get_env_replacement(value.get_secret_value()) - if env_var_value: - values[key] = SecretStr(env_var_value) + if isinstance(value, str): + values[key] = process_string_value(value, is_secret=False) + elif isinstance(value, SecretStr): + values[key] = process_string_value(value, is_secret=True)holmes/plugins/toolsets/__init__.py (1)
98-104: Simplify the boolean return statement.The function can be simplified by directly returning the boolean condition.
Apply this diff as suggested by static analysis:
-def is_old_toolset_config( - toolsets: Union[dict[str, dict[str, Any]], List[dict[str, Any]]], -) -> bool: - # old config is a list of toolsets - if isinstance(toolsets, list): - return True - return False +def is_old_toolset_config( + toolsets: Union[dict[str, dict[str, Any]], List[dict[str, Any]]], +) -> bool: + # old config is a list of toolsets + return isinstance(toolsets, list)🧰 Tools
🪛 Ruff (0.11.9)
102-104: Return the condition
bool(isinstance(toolsets, list))directlyReplace with
return bool(isinstance(toolsets, list))(SIM103)
holmes/core/toolset_manager.py (2)
19-20: Address TODO: Make toolset status cache location configurable.The hardcoded path might not work in all environments (e.g., containers, CI/CD).
Would you like me to implement a solution that reads from an environment variable with this as the default? For example:
DEFAULT_TOOLSET_STATUS_LOCATION = os.environ.get( "HOLMES_TOOLSET_STATUS_LOCATION", os.path.expanduser("~/.holmes/toolsets_status.json") )
311-311: Simplify dictionary membership check.Remove redundant
.keys()call when checking dictionary membership.- if new_toolset.name in existing_toolsets_by_name.keys(): + if new_toolset.name in existing_toolsets_by_name:🧰 Tools
🪛 Ruff (0.11.9)
311-311: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
holmes/core/tools.py (1)
334-337: Consider the implications of removing encapsulation.The refactoring from private attributes (
_path,_status,_error) to public fields removes encapsulation, which could lead to:
- Direct modification of these fields from external code without validation
- Breaking changes for any code that relied on getter/setter methods
- Potential consistency issues if these fields are modified without proper state management
While this aligns with the PR's goal of direct attribute access, ensure that:
- All consuming code has been updated
- These fields won't be accidentally modified in ways that break invariants
- The simplified approach doesn't compromise the integrity of toolset state management
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
holmes/config.py(10 hunks)holmes/core/resource_instruction.py(1 hunks)holmes/core/supabase_dal.py(1 hunks)holmes/core/tool_calling_llm.py(1 hunks)holmes/core/tools.py(11 hunks)holmes/core/toolset_manager.py(1 hunks)holmes/main.py(19 hunks)holmes/plugins/prompts/_fetch_logs.jinja2(3 hunks)holmes/plugins/prompts/_toolsets_instructions.jinja2(2 hunks)holmes/plugins/toolsets/__init__.py(2 hunks)holmes/plugins/toolsets/consts.py(1 hunks)holmes/plugins/toolsets/coralogix/toolset_coralogix_logs.py(3 hunks)holmes/plugins/toolsets/grafana/base_grafana_toolset.py(1 hunks)holmes/plugins/toolsets/grafana/toolset_grafana_tempo.py(1 hunks)holmes/plugins/toolsets/kafka.py(1 hunks)holmes/plugins/toolsets/opensearch/opensearch.py(1 hunks)holmes/plugins/toolsets/prometheus/prometheus.py(1 hunks)holmes/plugins/toolsets/utils.py(1 hunks)holmes/utils/default_toolset_installation_guide.jinja2(1 hunks)holmes/utils/env.py(1 hunks)holmes/utils/holmes_sync_toolsets.py(3 hunks)holmes/utils/pydantic_utils.py(3 hunks)tests/llm/utils/mock_toolset.py(2 hunks)tests/plugins/toolsets/grafana/test_grafana_loki.py(2 hunks)tests/plugins/toolsets/grafana/test_grafana_tempo.py(2 hunks)tests/plugins/toolsets/test_notion.py(2 hunks)tests/plugins/toolsets/test_prometheus_integration.py(2 hunks)tests/plugins/toolsets/test_tool_kafka.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (11)
tests/plugins/toolsets/test_notion.py (2)
holmes/core/tools.py (1)
ToolsetStatusEnum(84-87)holmes/plugins/toolsets/internet/notion.py (2)
FetchNotion(17-108)NotionToolset(111-127)
tests/plugins/toolsets/grafana/test_grafana_loki.py (3)
holmes/core/tools.py (1)
ToolsetStatusEnum(84-87)holmes/plugins/toolsets/grafana/grafana_api.py (1)
get_health(20-33)holmes/plugins/toolsets/grafana/toolset_grafana_loki.py (2)
GetLokiLogsForResource(130-205)GrafanaLokiConfig(33-34)
holmes/plugins/toolsets/grafana/base_grafana_toolset.py (1)
holmes/core/tools.py (4)
CallablePrerequisite(288-289)Tool(108-138)Toolset(301-445)ToolsetTag(90-93)
tests/plugins/toolsets/test_tool_kafka.py (2)
holmes/core/tools.py (1)
ToolsetStatusEnum(84-87)holmes/plugins/toolsets/kafka.py (6)
DescribeConsumerGroup(184-241)DescribeTopic(289-355)FindConsumerGroupsByTopic(369-456)KafkaToolset(480-559)ListKafkaConsumers(117-181)ListTopics(244-286)
tests/plugins/toolsets/grafana/test_grafana_tempo.py (3)
holmes/core/tools.py (1)
ToolsetStatusEnum(84-87)holmes/plugins/toolsets/grafana/grafana_api.py (1)
get_health(20-33)holmes/plugins/toolsets/grafana/toolset_grafana_tempo.py (1)
GetTempoTraces(69-193)
holmes/plugins/toolsets/grafana/toolset_grafana_tempo.py (6)
holmes/common/env_vars.py (1)
load_bool(5-7)holmes/core/tools.py (4)
StructuredToolResult(27-50)Tool(108-138)ToolParameter(102-105)ToolResultStatus(21-24)holmes/plugins/toolsets/grafana/common.py (3)
GrafanaConfig(9-21)build_headers(24-35)get_base_url(51-55)holmes/plugins/toolsets/grafana/tempo_api.py (2)
query_tempo_trace_by_id(80-124)query_tempo_traces(54-77)holmes/plugins/toolsets/grafana/trace_parser.py (1)
format_traces_list(177-195)holmes/plugins/toolsets/utils.py (2)
get_param_or_raise(128-132)process_timestamps_to_int(79-125)
holmes/core/tool_calling_llm.py (8)
tests/llm/utils/mock_utils.py (1)
Message(33-34)holmes/core/investigation_structured_output.py (2)
parse_markdown_into_sections_from_hash_sign(101-141)process_response_into_sections(213-226)holmes/core/issue.py (1)
Issue(13-54)holmes/core/llm.py (1)
LLM(30-58)holmes/core/resource_instruction.py (1)
ResourceInstructions(15-17)holmes/core/runbooks.py (1)
RunbookManager(7-26)holmes/core/tools.py (3)
StructuredToolResult(27-50)ToolExecutor(468-513)ToolResultStatus(21-24)holmes/plugins/prompts/__init__.py (1)
load_and_render_prompt(26-43)
holmes/plugins/toolsets/kafka.py (2)
holmes/core/tools.py (7)
CallablePrerequisite(288-289)StructuredToolResult(27-50)Tool(108-138)ToolParameter(102-105)ToolResultStatus(21-24)Toolset(301-445)ToolsetTag(90-93)holmes/plugins/toolsets/utils.py (1)
get_param_or_raise(128-132)
holmes/core/supabase_dal.py (2)
holmes/core/resource_instruction.py (2)
ResourceInstructionDocument(6-12)ResourceInstructions(15-17)holmes/utils/definitions.py (1)
RobustaConfig(11-13)
holmes/utils/env.py (1)
holmes/config.py (2)
get_env_replacement(43-53)replace_env_vars_values(56-80)
holmes/main.py (6)
holmes/config.py (6)
Config(101-431)SourceFactory(442-523)SupportedTicketSources(38-40)create_console_issue_investigator(305-316)load_from_file(185-198)toolset_manager(159-164)holmes/core/tool_calling_llm.py (1)
LLMResult(100-112)holmes/core/tools.py (1)
Toolset(301-445)holmes/plugins/prompts/__init__.py (1)
load_and_render_prompt(26-43)holmes/utils/file_utils.py (1)
write_json_file(7-16)holmes/core/toolset_manager.py (1)
load_toolset_with_status(172-207)
🪛 GitHub Actions: Build and test HolmesGPT
tests/llm/utils/mock_toolset.py
[error] 1-1: ImportError: cannot import name 'parse_toolsets_file' from 'holmes.config' in holmes/config.py
🪛 GitHub Actions: Evaluate LLM test cases
holmes/config.py
[error] 1-1: ImportError: cannot import name 'parse_toolsets_file' from 'holmes.config'. This caused test loading failure in tests/llm/conftest.py.
🪛 Ruff (0.11.9)
holmes/core/toolset_manager.py
67-79: Combine if branches using logical or operator
Combine if branches
(SIM114)
311-311: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
holmes/plugins/toolsets/__init__.py
102-104: Return the condition bool(isinstance(toolsets, list)) directly
Replace with return bool(isinstance(toolsets, list))
(SIM103)
🔇 Additional comments (41)
holmes/utils/default_toolset_installation_guide.jinja2 (1)
7-7: Good catch on the typo fix!The correction from "intrgration" to "integration" improves the documentation quality and user experience.
holmes/plugins/toolsets/consts.py (1)
1-5: Excellent refactoring to centralize string constants!This follows best practices by:
- Eliminating string literal duplication across toolset modules
- Ensuring consistent error messaging and parameter descriptions
- Supporting the broader toolset management centralization effort
The constant names are descriptive and their purposes are clear.
holmes/utils/pydantic_utils.py (3)
2-3: Good import reorganization for better readability.Moving the
Pathimport and reordering typing imports improves code organization.
40-40: Excellent type safety improvement.Changing the parameter type from
strtoPathmakes the function signature more explicit and type-safe while maintaining backward compatibility.
51-51: Nice formatting fix in the error message.Removing the extra space before the file path improves the error message formatting.
tests/plugins/toolsets/test_prometheus_integration.py (2)
6-8: Good formatting improvements.The simplified import statement and additional blank line improve code readability and follow Python style conventions.
23-23: 💡 Verification agent🧩 Analysis chain
Verify the status API change is consistent with the refactor.
The change from
toolset.get_status()totoolset.statusaligns with the broader refactor mentioned in the AI summary where status methods were replaced by public attributes. This should improve the API consistency.Let me verify that this change is consistent across the codebase and that the
statusattribute is properly defined:
🏁 Script executed:
#!/bin/bash # Description: Verify that toolset status is accessed via attribute instead of method calls echo "=== Searching for any remaining get_status() method calls ===" rg "\.get_status\(\)" --type py echo "=== Searching for status attribute access in toolset contexts ===" rg "toolset\.status" --type py -A 2 -B 2 echo "=== Checking if status is defined as a property or attribute in toolset classes ===" ast-grep --pattern $'class $_ { $$$ status = $_ $$$ }' ast-grep --pattern $'class $_ { $$$ @property def status($_): $$$ $$$ }'Length of output: 7396
Replace remaining
.get_status()calls with thestatusattributeThe refactor to use
toolset.statusis correct, but there are still legacy method calls in tests that must be updated:• tests/test_holmes_sync_toolsets.py
• tests/test_check_prerequisites.py
• tests/plugins/toolsets/test_tool_kafka.pyExample diff:
- assert toolset.get_status() == ToolsetStatusEnum.ENABLED + assert toolset.status == ToolsetStatusEnum.ENABLEDAnd in test_tool_kafka.py:
- assert kafka_toolset.status() == ToolsetStatusEnum.ENABLED + assert kafka_toolset.status == ToolsetStatusEnum.ENABLEDLikely an incorrect or invalid review comment.
tests/plugins/toolsets/grafana/test_grafana_loki.py (2)
6-10: LGTM! Import organization and new imports are appropriate.The addition of
ToolsetStatusEnumimport andGrafanaLokiConfigimport align with the updated status handling and provide better type safety for the test assertions.
41-41: LGTM! Status access pattern updated correctly.The change from
toolset.get_status()totoolset.statusis consistent with the broader refactor to expose toolset status as a direct attribute instead of through a getter method. This improves code consistency across the codebase.holmes/plugins/prompts/_fetch_logs.jinja2 (1)
7-7: LGTM! Template status checks updated consistently.All four toolset status checks have been correctly updated from
get_status()method calls to directstatusattribute access. The template logic remains intact and will continue to properly filter enabled toolsets for log fetching instructions.Also applies to: 17-17, 26-26, 31-31
holmes/plugins/toolsets/prometheus/prometheus.py (1)
1-31: LGTM! Import organization improved.The import statements have been properly reorganized and grouped for better clarity:
- Standard library imports (json, logging, os, re, string, time, typing, urllib.parse)
- Third-party imports (requests, pydantic)
- Internal imports (holmes.core.tools, holmes.plugins.toolsets modules)
The addition of specific imports like
RequestException,StructuredToolResult, andToolResultStatusappears to align with their usage throughout the file. The import fromconstsmodule follows the centralization pattern mentioned in the PR objectives.tests/plugins/toolsets/test_notion.py (2)
5-6: LGTM! Import improvements enhance type safety.The addition of
ToolsetStatusEnumimport provides proper type safety for status comparisons, and the reordering of imports (FetchNotion before NotionToolset) follows Python import conventions.
23-23: LGTM! Status access pattern updated correctly.The change from
toolset.get_status()totoolset.statusis consistent with the toolset management refactor. The assertion logic remains the same while using the new direct attribute access pattern.holmes/utils/holmes_sync_toolsets.py (2)
15-15: LGTM! Status access pattern updated correctly.The change from
toolset.get_status().valuetotoolset.status.valuealigns with the broader refactor to use direct attribute access instead of method calls for toolset status.Also applies to: 18-18
57-57: LGTM! Consistent status attribute access.The update to use
toolset.statusdirectly instead oftoolset.get_status()is consistent with the refactoring effort across the codebase.holmes/plugins/toolsets/grafana/base_grafana_toolset.py (1)
2-5: LGTM! Import reorganization improves code organization.The consolidation of imports and relocation of
TOOLSET_CONFIG_MISSING_ERRORto theconstsmodule enhances code organization and follows good practices for constant management.tests/plugins/toolsets/test_tool_kafka.py (1)
4-8: LGTM! Import reorganization enhances readability.The consolidation and grouping of imports improves code organization.
Also applies to: 14-16
tests/plugins/toolsets/grafana/test_grafana_tempo.py (2)
3-3: LGTM! Import reorganization improves readability.The grouping of related imports enhances code organization.
Also applies to: 5-5, 7-7, 9-9
84-84: LGTM! Status access pattern updated correctly.The change from
toolset.get_status()totoolset.statuscorrectly follows the refactor pattern to use direct attribute access.holmes/plugins/toolsets/utils.py (1)
3-5: LGTM: Import reorganization follows best practices.The reordering of imports to group typing imports after standard library and before third-party imports follows Python import conventions and improves code organization.
holmes/plugins/toolsets/opensearch/opensearch.py (1)
4-16: LGTM: Import reorganization improves code structure.The consolidation of imports from
holmes.core.toolsand the move ofTOOLSET_CONFIG_MISSING_ERRORto the newconstsmodule aligns with the PR's goal of centralizing constants and improving import organization.holmes/plugins/toolsets/grafana/toolset_grafana_tempo.py (2)
3-26: LGTM: Import reorganization follows best practices.The grouping of imports (standard library, third-party, local modules) improves code readability and follows Python conventions.
30-30: LGTM: Local constant definition is appropriate.Defining
ONE_HOUR_IN_SECONDS = 3600locally instead of importing from utils makes sense since this constant is only used within this file. The value is correct (3600 seconds = 1 hour).holmes/plugins/toolsets/coralogix/toolset_coralogix_logs.py (2)
12-15: LGTM: Constants moved to centralized location.Moving
STANDARD_END_DATETIME_TOOL_PARAM_DESCRIPTIONandTOOLSET_CONFIG_MISSING_ERRORto the newconstsmodule aligns with the PR's goal of centralizing shared constants.
114-116: LGTM: Formatting improvement without logic change.The reformatting of the conditional expression to a single line improves readability while maintaining the same logic: return ERROR status if
logs_data.errorexists, otherwise return SUCCESS.holmes/core/tool_calling_llm.py (1)
5-34: LGTM! Import reorganization improves code clarity.The import statements have been properly reorganized and consolidated, grouping related imports together. The relocation of
ResourceInstructionsto the newholmes.core.resource_instructionmodule follows good separation of concerns principles by isolating data models into their own module.tests/llm/utils/mock_toolset.py (1)
2-8: LGTM! Import organization follows standard conventions.The imports have been properly organized with standard library imports first, followed by third-party imports, and then local imports. The addition of required modules (logging, os, re) is appropriate.
holmes/plugins/toolsets/kafka.py (3)
2-2: LGTM! Import consolidation improves readability.Consolidating multiple
typingimports into a single line reduces clutter and follows standard import organization practices.
10-20: LGTM! Import organization follows logical grouping.The reordering of confluent_kafka.admin imports groups related classes together, and separating the
_TopicPartitionimport on its own line improves clarity since it uses a different import pattern.
23-33: LGTM! Import reorganization and constant centralization.The reorganization of imports from
holmes.core.toolsgroups related classes together logically. The change to importTOOLSET_CONFIG_MISSING_ERRORfromholmes.plugins.toolsets.constsinstead ofutilsaligns with the broader effort to centralize constants in dedicated modules, improving maintainability.holmes/core/resource_instruction.py (1)
1-18: LGTM! Clean modularization of resource instruction models.The new module properly separates resource instruction data models from the main tool calling logic. Both Pydantic models are well-defined:
ResourceInstructionDocumenthas a clear purpose with good documentation explaining its role in providing investigation context via URLsResourceInstructionsproperly aggregates instructions and documents with appropriate default valuesThis modularization improves code organization and follows the single responsibility principle.
holmes/core/supabase_dal.py (1)
1-33: LGTM! Import organization improvements.The import reorganization improves code structure by:
- Grouping related imports together (datetime imports, postgrest imports)
- Following standard Python import ordering conventions
- Reflecting the new module structure with
ResourceInstructionDocumentandResourceInstructionsmoved to their dedicated moduleholmes/plugins/toolsets/__init__.py (1)
44-47: Good error handling for empty YAML files.The explicit check for
Noneand raising a descriptive error message improves debugging experience when configuration files are empty or invalid.holmes/core/toolset_manager.py (2)
127-140: Good error handling for prerequisite checks.The approach of logging detailed errors while storing user-friendly messages is excellent. Continuing to process other toolsets even when one fails ensures resilience.
266-298: Well-structured custom toolset loading with proper validation.The method handles both toolsets and MCP servers configurations correctly, with appropriate error handling for missing files and invalid configurations.
holmes/main.py (2)
1147-1164: Clean table formatting implementation.The use of Rich tables for displaying toolset status is well-implemented and provides a good user experience.
142-152: Good improvement using Path types for file paths.The change from
Optional[str]toOptional[Path]for file paths improves type safety and is consistent throughout the codebase.holmes/config.py (2)
158-165: Well-implemented lazy initialization for ToolsetManager.The property pattern with lazy initialization is a good design choice that delays ToolsetManager creation until it's actually needed.
184-199: Good type improvement for config file handling.The change to use
Pathinstead ofstrfor the config file parameter improves type safety and is consistent with the changes in main.py.holmes/core/tools.py (2)
1-12: Import organization looks good!The imports have been properly sorted using isort as mentioned in the PR objectives, improving code organization.
517-546: Well-structured partial override class!The new
ToolsetYamlFromConfigclass provides a clean way to handle partial overrides of built-in toolsets, with:
- Clear documentation explaining its purpose
- Appropriate use of optional fields for override scenarios
- Defaults that align with the configuration-driven approach (enabled=True by default)
This design aligns well with the PR objectives of centralizing toolset management and configuration.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
holmes/core/toolset_manager.py (4)
55-57: Fix incorrect docstring for server_tool_tags property.The docstring incorrectly refers to "CLI tools" when it should mention "server tools".
70-82: Simplify conditional branches using logical OR.The separate if/elif branches can be combined for better readability as suggested in previous reviews.
🧰 Tools
🪛 Ruff (0.11.9)
70-82: Combine
ifbranches using logicaloroperatorCombine
ifbranches(SIM114)
159-170: Consider using the existing write_json_file utility or add error handling.The file writing operation lacks error handling as noted in previous reviews. Consider using the existing utility function.
🧰 Tools
🪛 GitHub Actions: Build and test HolmesGPT
[error] 159-160: No overload variant of "dirname" matches argument type "Optional[list[Any]]" [call-overload]
[error] 161-161: No overload variant of "open" matches argument types "Optional[list[Any]]", "str" [call-overload]
185-187: Add error handling for cache file loading.The JSON loading could fail if the cache file is corrupted or has permission issues, as noted in previous reviews.
🧰 Tools
🪛 GitHub Actions: Build and test HolmesGPT
[error] 186-186: No overload variant of "open" matches argument types "Optional[list[Any]]", "str" [call-overload]
🧹 Nitpick comments (1)
holmes/core/toolset_manager.py (1)
312-312: Remove unnecessary.keys()call.Checking membership directly in the dictionary is more efficient and readable.
- if new_toolset.name in existing_toolsets_by_name.keys(): + if new_toolset.name in existing_toolsets_by_name:🧰 Tools
🪛 Ruff (0.11.9)
312-312: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
holmes/core/toolset_manager.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
holmes/core/toolset_manager.py
70-82: Combine if branches using logical or operator
Combine if branches
(SIM114)
312-312: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
🪛 GitHub Actions: Build and test HolmesGPT
holmes/core/toolset_manager.py
[error] 42-42: Incompatible types in assignment (expression has type "Optional[str]", variable has type "Optional[list[Any]]") [assignment]
[error] 159-160: No overload variant of "dirname" matches argument type "Optional[list[Any]]" [call-overload]
[error] 161-161: No overload variant of "open" matches argument types "Optional[list[Any]]", "str" [call-overload]
[error] 181-181: Argument 1 to "exists" has incompatible type "Optional[list[Any]]"; expected "Union[int, Union[str, bytes, PathLike[str], PathLike[bytes]]]" [arg-type]
[error] 186-186: No overload variant of "open" matches argument types "Optional[list[Any]]", "str" [call-overload]
🔇 Additional comments (2)
holmes/core/toolset_manager.py (2)
118-124: Verify custom toolset override requirements.The current implementation prevents custom toolsets from overriding built-in toolsets, but previous comments suggest this might be needed for server deployments. Please clarify if this restriction should be configurable or if certain contexts should allow overrides.
22-316: Well-structured toolset management implementation.The
ToolsetManagerclass provides a clean separation of concerns for toolset lifecycle management, with good integration of caching, status tracking, and filtering capabilities. The design properly centralizes toolset operations that were previously scattered across the codebase.🧰 Tools
🪛 Ruff (0.11.9)
70-82: Combine
ifbranches using logicaloroperatorCombine
ifbranches(SIM114)
312-312: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
🪛 GitHub Actions: Build and test HolmesGPT
[error] 42-42: Incompatible types in assignment (expression has type "Optional[str]", variable has type "Optional[list[Any]]") [assignment]
[error] 159-160: No overload variant of "dirname" matches argument type "Optional[list[Any]]" [call-overload]
[error] 161-161: No overload variant of "open" matches argument types "Optional[list[Any]]", "str" [call-overload]
[error] 181-181: Argument 1 to "exists" has incompatible type "Optional[list[Any]]"; expected "Union[int, Union[str, bytes, PathLike[str], PathLike[bytes]]]" [arg-type]
[error] 186-186: No overload variant of "open" matches argument types "Optional[list[Any]]", "str" [call-overload]
mainred
force-pushed
the
cache-and-manage-toolset
branch
from
3745f08 to
be53a9b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
holmes/plugins/toolsets/__init__.py (1)
131-140:⚠️ Potential issueFix critical logic and type annotation issues.
The function has several critical issues:
typevariable is undefined on line 131, which will cause a NameError- The logic flow is broken - missing
elifkeywords- Type annotation inconsistencies as noted in previous reviews
Apply this diff to fix the issues:
try: - if type is ToolsetType.MCP: - validated_toolset: RemoteMCPToolset = RemoteMCPToolset( - **config, name=name - ) - if strict_check: - validated_toolset: YAMLToolset = YAMLToolset(**config, name=name) # type: ignore + # Determine toolset type from config + toolset_type = config.get('type', ToolsetType.YAML) + + if toolset_type == ToolsetType.MCP: + validated_toolset = RemoteMCPToolset(**config, name=name) + elif strict_check: + validated_toolset = YAMLToolset(**config, name=name) else: - validated_toolset: ToolsetYamlFromConfig = ToolsetYamlFromConfig( # type: ignore - **config, name=name - ) + validated_toolset = ToolsetYamlFromConfig(**config, name=name)holmes/main.py (1)
1120-1122: Fix typo in command help text.The help text contains a typo where "build-int" should be "built-in".
- """ - List build-int and custom toolsets status of CLI - """ + """ + List built-in and custom toolsets status of CLI + """holmes/core/toolset_manager.py (4)
49-55: Fix incorrect docstring for server_tool_tags property.The docstring incorrectly refers to "CLI tools" when it should mention "server tools".
- """ - Returns the list of toolset tags that are relevant for CLI tools. - A toolset is considered a CLI tool if it has any of UI tool tags: - """ + """ + Returns the list of toolset tags that are relevant for server tools. + A toolset is considered a server tool if it has any of server tool tags. + """
67-79: Simplify conditional branches using logical OR.The separate if/elif branches can be combined for better readability.
- if ( - isinstance(toolset_config["enabled"], bool) - and not toolset_config["enabled"] - ): - return False - # Normally benedict should translate the value of enabled to bool when well assigned, - # but in case it doesn't, and to avoid unexpected behavior, we check if the value is a - # string and if it is 'false' (case insensitive). - elif ( - isinstance(toolset_config["enabled"], str) - and toolset_config["enabled"].lower() == "false" - ): - return False + if ( + (isinstance(toolset_config["enabled"], bool) and not toolset_config["enabled"]) + or (isinstance(toolset_config["enabled"], str) and toolset_config["enabled"].lower() == "false") + ): + return False🧰 Tools
🪛 Ruff (0.11.9)
67-79: Combine
ifbranches using logicaloroperatorCombine
ifbranches(SIM114)
180-194: Consider using the existing write_json_file utility.The file writing operation lacks error handling. Consider using the existing
write_json_fileutility fromholmes.utils.file_utilswhich already has proper error handling.+ from holmes.utils.file_utils import write_json_file - if self.toolset_status_location and not os.path.exists( - os.path.dirname(self.toolset_status_location) - ): - os.makedirs(os.path.dirname(self.toolset_status_location)) - with open(self.toolset_status_location, "w") as f: - toolset_status = [ - json.loads( - toolset.model_dump_json( - include={"name", "status", "enabled", "type", "path", "error"} - ) - ) - for toolset in all_toolsets - ] - json.dump(toolset_status, f, indent=2) + toolset_status = [ + json.loads( + toolset.model_dump_json( + include={"name", "status", "enabled", "type", "path", "error"} + ) + ) + for toolset in all_toolsets + ] + write_json_file(self.toolset_status_location, toolset_status)
208-210: Add error handling for cache file loading.The JSON loading could fail if the cache file is corrupted or has permission issues.
cached_toolsets: List[dict[str, Any]] = [] - with open(self.toolset_status_location, "r") as f: - cached_toolsets = json.load(f) + try: + with open(self.toolset_status_location, "r") as f: + cached_toolsets = json.load(f) + except (json.JSONDecodeError, IOError) as e: + logging.error(f"Failed to load toolset cache: {e}. Refreshing status.") + self.refresh_toolset_status(dal) + with open(self.toolset_status_location, "r") as f: + cached_toolsets = json.load(f)holmes/config.py (1)
1-12:⚠️ Potential issueCritical: Tests still reference removed
parse_toolsets_fileThe removal of
parse_toolsets_filefromholmes.confighas left tests failing intests/llm/utils/mock_toolset.py. Please update this file to use the new parser API or correct import to fix the pipeline failure.🧰 Tools
🪛 GitHub Actions: Evaluate LLM test cases
[error] 1-1: ImportError: cannot import name 'parse_toolsets_file' from 'holmes.config'. This caused the test run to fail.
🧹 Nitpick comments (5)
holmes/plugins/toolsets/__init__.py (1)
100-107: Consider simplifying the boolean return as suggested by static analysis.The function can be simplified by returning the condition directly.
Apply this diff:
-def is_old_toolset_config( - toolsets: Union[dict[str, dict[str, Any]], List[dict[str, Any]]], -) -> bool: - # old config is a list of toolsets - if isinstance(toolsets, list): - return True - return False +def is_old_toolset_config( + toolsets: Union[dict[str, dict[str, Any]], List[dict[str, Any]]], +) -> bool: + # old config is a list of toolsets + return isinstance(toolsets, list)🧰 Tools
🪛 Ruff (0.11.9)
104-106: Return the condition
bool(isinstance(toolsets, list))directlyReplace with
return bool(isinstance(toolsets, list))(SIM103)
holmes/main.py (1)
1135-1137: Fix typo in refresh command help text.The help text contains a typo where "build-in" should be "built-in".
- """ - Refresh build-in and custom toolsets status of CLI - """ + """ + Refresh built-in and custom toolsets status of CLI + """holmes/core/toolset_manager.py (3)
240-240: Remove unnecessary .keys() call for performance.Use
key in dictinstead ofkey in dict.keys()for better performance.- if custom_toolset_from_cli in toolsets_status_by_name.keys(): + if custom_toolset_from_cli.name in toolsets_status_by_name:🧰 Tools
🪛 Ruff (0.11.9)
240-240: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
375-375: Remove unnecessary .keys() call for performance.Use
key in dictinstead ofkey in dict.keys()for better performance.- if new_toolset.name in existing_toolsets_by_name.keys(): + if new_toolset.name in existing_toolsets_by_name:🧰 Tools
🪛 Ruff (0.11.9)
375-375: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
292-294: Improve exception chaining for better debugging.Use
raise ... from eto preserve the original exception context for better debugging.except Exception as e: - raise ValueError( - f"Failed to load toolsets from {toolset_path}, error: {e}" - ) + raise ValueError( + f"Failed to load toolsets from {toolset_path}, error: {e}" + ) from e🧰 Tools
🪛 Ruff (0.11.9)
292-294: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
holmes/config.py(11 hunks)holmes/core/resource_instruction.py(1 hunks)holmes/core/supabase_dal.py(1 hunks)holmes/core/tool_calling_llm.py(1 hunks)holmes/core/tools.py(11 hunks)holmes/core/toolset_manager.py(1 hunks)holmes/main.py(20 hunks)holmes/plugins/prompts/_fetch_logs.jinja2(3 hunks)holmes/plugins/prompts/_toolsets_instructions.jinja2(2 hunks)holmes/plugins/toolsets/__init__.py(2 hunks)holmes/plugins/toolsets/consts.py(1 hunks)holmes/plugins/toolsets/coralogix/toolset_coralogix_logs.py(3 hunks)holmes/plugins/toolsets/grafana/base_grafana_toolset.py(1 hunks)holmes/plugins/toolsets/grafana/toolset_grafana_tempo.py(1 hunks)holmes/plugins/toolsets/kafka.py(1 hunks)holmes/plugins/toolsets/opensearch/opensearch.py(1 hunks)holmes/plugins/toolsets/prometheus/prometheus.py(1 hunks)holmes/plugins/toolsets/utils.py(1 hunks)holmes/utils/default_toolset_installation_guide.jinja2(1 hunks)holmes/utils/definitions.py(1 hunks)holmes/utils/env.py(1 hunks)holmes/utils/holmes_sync_toolsets.py(3 hunks)holmes/utils/pydantic_utils.py(3 hunks)tests/config_class/test_config_load_custom_toolsets.py(0 hunks)tests/config_class/test_config_load_remote_mcp_server.py(2 hunks)tests/core/test_toolset_manager.py(1 hunks)tests/llm/utils/mock_toolset.py(2 hunks)tests/plugins/toolsets/grafana/test_grafana_loki.py(2 hunks)tests/plugins/toolsets/grafana/test_grafana_tempo.py(2 hunks)tests/plugins/toolsets/test_load_config.py(1 hunks)tests/plugins/toolsets/test_notion.py(2 hunks)tests/plugins/toolsets/test_prometheus_integration.py(2 hunks)tests/plugins/toolsets/test_tool_kafka.py(2 hunks)tests/test_check_prerequisites.py(11 hunks)tests/test_holmes_sync_toolsets.py(2 hunks)
💤 Files with no reviewable changes (1)
- tests/config_class/test_config_load_custom_toolsets.py
✅ Files skipped from review due to trivial changes (7)
- holmes/utils/default_toolset_installation_guide.jinja2
- holmes/utils/definitions.py
- tests/plugins/toolsets/test_prometheus_integration.py
- holmes/plugins/toolsets/utils.py
- holmes/utils/holmes_sync_toolsets.py
- tests/test_check_prerequisites.py
- holmes/core/resource_instruction.py
🚧 Files skipped from review as they are similar to previous changes (19)
- holmes/plugins/prompts/_fetch_logs.jinja2
- holmes/plugins/toolsets/grafana/base_grafana_toolset.py
- tests/plugins/toolsets/test_tool_kafka.py
- holmes/plugins/toolsets/opensearch/opensearch.py
- tests/plugins/toolsets/test_notion.py
- holmes/plugins/toolsets/coralogix/toolset_coralogix_logs.py
- tests/plugins/toolsets/grafana/test_grafana_loki.py
- holmes/plugins/toolsets/prometheus/prometheus.py
- holmes/plugins/toolsets/consts.py
- tests/plugins/toolsets/grafana/test_grafana_tempo.py
- tests/llm/utils/mock_toolset.py
- holmes/core/supabase_dal.py
- holmes/core/tool_calling_llm.py
- holmes/plugins/toolsets/grafana/toolset_grafana_tempo.py
- holmes/plugins/prompts/_toolsets_instructions.jinja2
- holmes/plugins/toolsets/kafka.py
- holmes/utils/pydantic_utils.py
- holmes/utils/env.py
- holmes/core/tools.py
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/config_class/test_config_load_remote_mcp_server.py (1)
holmes/plugins/toolsets/__init__.py (1)
load_toolsets_from_config(109-153)
holmes/main.py (5)
holmes/__init__.py (1)
get_version(14-70)holmes/config.py (4)
Config(101-438)create_console_issue_investigator(312-323)load_from_file(192-205)toolset_manager(164-171)holmes/core/tools.py (1)
Toolset(301-445)holmes/utils/file_utils.py (1)
write_json_file(7-16)holmes/core/toolset_manager.py (1)
load_toolset_with_status(196-246)
🪛 Ruff (0.11.9)
holmes/plugins/toolsets/__init__.py
104-106: Return the condition bool(isinstance(toolsets, list)) directly
Replace with return bool(isinstance(toolsets, list))
(SIM103)
holmes/core/toolset_manager.py
67-79: Combine if branches using logical or operator
Combine if branches
(SIM114)
240-240: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
292-294: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
375-375: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
🪛 GitHub Actions: Evaluate LLM test cases
holmes/config.py
[error] 1-1: ImportError: cannot import name 'parse_toolsets_file' from 'holmes.config'. This caused the test run to fail.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (3.12)
🔇 Additional comments (20)
tests/config_class/test_config_load_remote_mcp_server.py (2)
6-6: LGTM! Import updated correctly for refactored toolset loading.The import change from
load_toolsets_definitionstoload_toolsets_from_configcorrectly aligns with the refactoring that moved toolset loading functionality to theholmes.plugins.toolsetsmodule.
51-53: LGTM! Function call updated correctly with appropriate parameters.The function call has been properly updated to use
load_toolsets_from_configwithstrict_check=False, which aligns with the new function signature and maintains the same test behavior.tests/test_holmes_sync_toolsets.py (2)
1-2: LGTM! Import reorganization improves code organization.The imports have been properly reorganized by grouping related imports together (e.g., standard library imports at the top, related tool imports together), which improves code readability and follows Python best practices.
Also applies to: 5-5, 8-8, 10-11, 19-20
355-360: LGTM! Updated to use direct attribute access.The changes correctly update from method calls (
get_status(),get_error()) to direct attribute access (status,error), which aligns with the refactoring where these became public attributes on the Toolset class.tests/plugins/toolsets/test_load_config.py (3)
9-26: LGTM! Comprehensive test for old format detection.This test correctly verifies that the function properly rejects old list-based toolset configurations and raises an appropriate error with a helpful message.
28-55: LGTM! Good test coverage for multiple old format toolsets.This test ensures that the old format detection works correctly even when multiple toolsets are provided in the deprecated format.
71-94: LGTM! Excellent test for environment variable substitution.This test thoroughly validates:
- Loading toolsets in the new dictionary-based format
- Environment variable substitution in configuration values
- Proper environment cleanup in the finally block
The test ensures that environment variables are correctly resolved and injected into the toolset configuration.
holmes/plugins/toolsets/__init__.py (2)
36-51: LGTM! Well-refactored file loading function.The
load_toolsets_from_filefunction has been properly refactored to:
- Parse YAML files with proper error handling for empty/invalid files
- Extract the "toolsets" dictionary from the parsed YAML
- Delegate validation to the new
load_toolsets_from_configfunctionThis separation of concerns improves maintainability.
89-96: LGTM! Good default behavior for built-in toolsets.The logic to disable built-in toolsets by default and mark them with appropriate metadata (type, is_default, path=None) provides better control over toolset activation and prevents exposing internal paths.
holmes/main.py (4)
14-44: LGTM: Import additions support new functionality.The new imports for
json,logging,Table, andwrite_json_fileare appropriately added to support the new toolset management CLI commands and their table display functionality.
141-145: LGTM: Config option type change aligns with Path usage.The change from
Optional[str]toOptional[Path]for the config file option is consistent with the broader refactoring to usePathobjects for file paths throughout the codebase.
1144-1161: LGTM: Well-implemented table display function.The
pretty_print_toolset_statusfunction correctly creates a Rich table with appropriate columns and handles missing fields gracefully using.get()with default values.
383-383: LGTM: Method call changes align with ToolsetManager refactoring.The removal of the
allowed_toolsetsparameter and the use ofcustom_toolsets_from_clialign with the centralized toolset management approach usingToolsetManager.Also applies to: 589-589, 718-718, 834-834, 909-909, 993-993, 1079-1079
tests/core/test_toolset_manager.py (1)
1-278: LGTM: Comprehensive test coverage for ToolsetManager.This test file provides excellent coverage of the
ToolsetManagerfunctionality including:
- Utility methods and property access
- Loading and merging toolsets from various sources
- Caching and status management
- Error handling for invalid configurations
- Edge cases with missing files and empty configurations
The tests use appropriate mocking, temporary files, and parametrization to ensure reliable and isolated testing.
holmes/core/toolset_manager.py (2)
27-27: LGTM: Type annotation is correctly fixed.The
toolset_status_locationfield is now correctly typed asFilePath, which matches its usage throughout the code as a file path.
16-40: LGTM: Well-designed centralized toolset management.The
ToolsetManagerclass provides excellent centralization of toolset management functionality, with clear separation of concerns for loading, merging, caching, and filtering toolsets. The design properly handles different toolset types and sources.holmes/config.py (4)
163-171: LGTM: Clean ToolsetManager integration.The
toolset_managerproperty provides a clean integration point for the centralized toolset management, lazily initializing theToolsetManagerwith the appropriate configuration parameters.
191-205: LGTM: Improved config loading with better typing and logging.The changes to
load_from_fileprovide better type safety withPathparameters, improved debug logging, and clearer variable naming. The logic remains correct while being more maintainable.
256-285: LGTM: Simplified tool executor creation using ToolsetManager.The refactoring of
create_console_tool_executorandcreate_tool_executormethods properly delegates toolset management toToolsetManager, removing manual filtering logic and theallowed_toolsetsparameter. This simplification improves maintainability while preserving functionality.🧰 Tools
🪛 Ruff (0.11.9)
282-282: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
453-453: LGTM: Consistent Path type usage in SourceFactory.The change to use
Optional[Path]for theconfig_fileparameter is consistent with the broader refactoring to usePathobjects for file paths throughout the codebase.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
holmes/core/toolset_manager.py (4)
60-62: Fix incorrect docstring for server_tool_tags property.The docstring incorrectly refers to "CLI tools" when it should mention "server tools".
75-87: Simplify conditional branches using logical OR.The separate if/elif branches can be combined for better readability.
🧰 Tools
🪛 Ruff (0.11.9)
75-87: Combine
ifbranches using logicaloroperatorCombine
ifbranches(SIM114)
174-200: Consider using the existing write_json_file utility or add error handling.The file writing operation lacks error handling. Consider using the existing
write_json_fileutility fromholmes.utils.file_utilswhich already has proper error handling.
202-252: Add error handling for cache file loading.The JSON loading could fail if the cache file is corrupted or has permission issues.
🧰 Tools
🪛 Ruff (0.11.9)
246-246: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
🧹 Nitpick comments (2)
holmes/core/toolset_manager.py (2)
246-246: Usekey in dictinstead ofkey in dict.keys().This is more efficient and pythonic.
- if custom_toolset_from_cli in toolsets_status_by_name.keys(): + if custom_toolset_from_cli.name in toolsets_status_by_name:🧰 Tools
🪛 Ruff (0.11.9)
246-246: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
381-381: Usekey in dictinstead ofkey in dict.keys().This is more efficient and pythonic.
- if new_toolset.name in existing_toolsets_by_name.keys(): + if new_toolset.name in existing_toolsets_by_name:🧰 Tools
🪛 Ruff (0.11.9)
381-381: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
holmes/core/toolset_manager.py(1 hunks)holmes/utils/definitions.py(1 hunks)tests/llm/utils/mock_toolset.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- holmes/utils/definitions.py
- tests/llm/utils/mock_toolset.py
🧰 Additional context used
🪛 Ruff (0.11.9)
holmes/core/toolset_manager.py
75-87: Combine if branches using logical or operator
Combine if branches
(SIM114)
246-246: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
298-300: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
381-381: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (3.12)
- GitHub Check: build (3.10)
- GitHub Check: build (3.11)
🔇 Additional comments (9)
holmes/core/toolset_manager.py (9)
1-15: LGTM - Clean imports and module organization.The imports are well-organized and the default cache location is appropriately set using
os.path.expanduser().
17-29: LGTM - Well-defined class structure.The class attributes are properly typed and the docstring clearly explains the purpose of the ToolsetManager.
30-47: LGTM - Robust initialization with automatic custom toolset discovery.The constructor properly handles the automatic inclusion of
CUSTOM_TOOLSET_LOCATIONwhen it exists, which is useful for containerized environments.
91-138: LGTM - Comprehensive toolset loading with proper override hierarchy.The method correctly implements the override hierarchy: built-in → configured → custom toolsets. The prerequisite checking logic is well-implemented with proper error handling.
140-172: LGTM - Proper separation of built-in and custom toolset handling.The method correctly distinguishes between built-in and custom toolsets, applying appropriate validation levels (strict vs non-strict) based on the toolset type.
254-278: LGTM - Clear and efficient filtering methods.Both methods properly filter toolsets based on their tags and use the cached status for efficiency.
280-330: LGTM - Robust custom toolset loading with conflict detection.The method properly handles YAML parsing, validates required keys, and includes conflict checking for built-in toolsets when requested.
🧰 Tools
🪛 Ruff (0.11.9)
298-300: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
332-369: LGTM - Clear custom toolset loading implementation.The method includes helpful documentation with configuration examples and properly delegates to the path-based loading method.
371-384: LGTM - Simple and effective toolset merging logic.The method correctly handles both overriding existing toolsets and adding new ones.
🧰 Tools
🪛 Ruff (0.11.9)
381-381: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
holmes/utils/env.py (1)
33-33: Fix missing assignment for recursive dictionary processing.The recursive call to
replace_env_vars_values(value)doesn't assign the result back to the dictionary, which means nested dictionaries won't be updated correctly.Apply this diff:
- replace_env_vars_values(value) + values[key] = replace_env_vars_values(value)holmes/main.py (1)
1121-1121: Fix typo in command help text.- List build-in and custom toolsets status of CLI + List built-in and custom toolsets status of CLIholmes/config.py (1)
1-491: Critical: Acknowledge the duplicate comment about breaking test imports.As noted in the previous review, the removal of
parse_toolsets_filebreaks test imports. This critical issue must be resolved.The previous review correctly identified that tests in
tests/llm/utils/mock_toolset.pystill import the removedparse_toolsets_filefunction, causing test failures. Please update the test file to use the newToolsetManagerAPI or the appropriate replacement function.🧰 Tools
🪛 Ruff (0.11.9)
47-49: Return the condition
bool(isinstance(toolsets, list))directlyReplace with
return bool(isinstance(toolsets, list))(SIM103)
55-55: Loop control variable
modelnot used within loop bodyRename unused
modelto_model(B007)
242-242: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
🪛 Pylint (3.3.7)
[convention] 100-100: Line too long (113/100)
(C0301)
[convention] 101-101: Line too long (101/100)
(C0301)
[convention] 102-102: Line too long (115/100)
(C0301)
[convention] 146-146: Line too long (123/100)
(C0301)
[convention] 218-218: Line too long (107/100)
(C0301)
[convention] 223-223: Line too long (107/100)
(C0301)
[convention] 242-242: Line too long (111/100)
(C0301)
[convention] 422-422: Line too long (104/100)
(C0301)
[convention] 450-450: Line too long (186/100)
(C0301)
[convention] 480-480: Line too long (198/100)
(C0301)
[warning] 188-188: TODO
(W0511)
[convention] 1-1: Missing module docstring
(C0114)
[error] 10-10: Unable to import 'pydantic'
(E0401)
[convention] 38-38: Missing class docstring
(C0115)
[convention] 43-43: Missing function or method docstring
(C0116)
[convention] 52-52: Missing function or method docstring
(C0116)
[warning] 55-55: Unused variable 'model'
(W0612)
[convention] 61-61: Missing class docstring
(C0115)
[convention] 116-116: Missing function or method docstring
(C0116)
[convention] 124-124: Missing function or method docstring
(C0116)
[convention] 133-133: Missing function or method docstring
(C0116)
[warning] 142-142: Use lazy % formatting in logging functions
(W1203)
[convention] 152-152: Missing function or method docstring
(C0116)
[warning] 154-154: Use lazy % formatting in logging functions
(W1203)
[error] 159-159: Possibly using variable 'config_from_file' before assignment
(E0606)
[warning] 162-162: Use lazy % formatting in logging functions
(W1203)
[convention] 168-168: Missing function or method docstring
(C0116)
[warning] 205-205: Use lazy % formatting in logging functions
(W1203)
[warning] 208-208: Use lazy % formatting in logging functions
(W1203)
[warning] 209-209: Using open without explicitly specifying an encoding
(W1514)
[warning] 241-243: Use lazy % formatting in logging functions
(W1203)
[refactor] 242-242: Unnecessary use of a comprehension, use list(self._server_tool_executor.tools_by_name.keys()) instead.
(R1721)
[convention] 242-242: Consider iterating the dictionary directly instead of calling .keys()
(C0201)
[convention] 247-247: Missing function or method docstring
(C0116)
[convention] 253-253: Missing function or method docstring
(C0116)
[convention] 259-259: Missing function or method docstring
(C0116)
[convention] 272-272: Missing function or method docstring
(C0116)
[convention] 285-285: Missing function or method docstring
(C0116)
[convention] 297-297: Missing function or method docstring
(C0116)
[convention] 307-307: Missing function or method docstring
(C0116)
[convention] 317-317: Missing function or method docstring
(C0116)
[convention] 338-338: Missing function or method docstring
(C0116)
[convention] 348-348: Missing function or method docstring
(C0116)
[convention] 362-362: Missing function or method docstring
(C0116)
[convention] 371-371: Missing function or method docstring
(C0116)
[convention] 394-394: Missing function or method docstring
(C0116)
[warning] 136-136: Attribute '_model_list' defined outside init
(W0201)
[convention] 401-401: Missing class docstring
(C0115)
[refactor] 401-401: Too few public methods (0/2)
(R0903)
[convention] 409-409: Missing class docstring
(C0115)
[convention] 411-411: Missing function or method docstring
(C0116)
[refactor] 411-411: Too many arguments (6/5)
(R0913)
[refactor] 411-411: Too many positional arguments (6/5)
(R0917)
[refactor] 425-490: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
[refactor] 409-409: Too few public methods (1/2)
(R0903)
🧹 Nitpick comments (5)
holmes/utils/env.py (1)
1-7: Add module docstring for better documentation.Consider adding a module docstring to describe the purpose of this utility module.
+""" +Utility module for environment variable replacement in nested data structures. + +This module provides functions to find and replace environment variable patterns +like {{ env.VAR_NAME }} in strings, dictionaries, and lists with their actual +environment variable values. +""" + import logging🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
[error] 6-6: Unable to import 'pydantic'
(E0401)
holmes/core/tools.py (1)
427-429: Consider using membership test for cleaner comparison.The static analysis suggestion to use
inoperator for multiple comparisons would improve readability.- if ( - self.status == ToolsetStatusEnum.DISABLED - or self.status == ToolsetStatusEnum.FAILED - ): + if self.status in (ToolsetStatusEnum.DISABLED, ToolsetStatusEnum.FAILED):🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 427-428: Consider merging these comparisons with 'in' by using 'self.status in (ToolsetStatusEnum.DISABLED, ToolsetStatusEnum.FAILED)'. Use a set instead if elements are hashable.
(R1714)
holmes/config.py (3)
100-104: Long line violations and clear documentation.The comments clearly explain the distinction between
custom_toolsetsandcustom_toolsets_from_cli, which is helpful for understanding the configuration hierarchy.Consider breaking the long comments to comply with line length limits:
- # custom_toolsets is defined in the config file, and can override built-in toolsets as stable custom toolsets - # While custom_toolsets_from_cli is defined in the CLI and should not override built-in toolsets. - # custom_toolsets status can be cached while custom_toolsets_from_cli should be loaded every time the CLI runs. + # custom_toolsets is defined in the config file, and can override built-in toolsets + # as stable custom toolsets. While custom_toolsets_from_cli is defined in the CLI + # and should not override built-in toolsets. + # custom_toolsets status can be cached while custom_toolsets_from_cli should be + # loaded every time the CLI runs.🧰 Tools
🪛 Pylint (3.3.7)
[convention] 100-100: Line too long (113/100)
(C0301)
[convention] 101-101: Line too long (101/100)
(C0301)
[convention] 102-102: Line too long (115/100)
(C0301)
123-131: Add docstring for the toolset_manager property.The lazy initialization pattern is correct, but the property lacks documentation.
Add a docstring to explain the property's purpose:
@property def toolset_manager(self) -> ToolsetManager: + """ + Lazily initializes and returns the ToolsetManager instance. + + The manager handles built-in, configured, and custom toolsets, + providing filtered lists for CLI and server contexts. + """ if not self._toolset_manager:🧰 Tools
🪛 Pylint (3.3.7)
[convention] 124-124: Missing function or method docstring
(C0116)
216-227: Simplified toolset loading with centralized management.The refactoring to use
ToolsetManagersimplifies the method significantly and centralizes toolset management logic, which is a good architectural improvement.Consider adding a docstring to explain the updated behavior and fix the line length:
def create_console_tool_executor(self, dal: Optional[SupabaseDal]) -> ToolExecutor: """ Creates a ToolExecutor instance configured for CLI usage. This executor manages the available tools and their execution in the command-line interface. The method loads toolsets in this order, with later sources overriding earlier ones: 1. Built-in toolsets (tagged as CORE or CLI) - 2. toolsets from config file will override and be merged into built-in toolsets with the same name. - 3. Custom toolsets from config files which can not override built-in toolsets + 2. Toolsets from config file will override and be merged into built-in toolsets + with the same name. + 3. Custom toolsets from config files which cannot override built-in toolsets """ - cli_toolsets = self.toolset_manager.list_enabled_console_toolsets(dal=dal) + cli_toolsets = self.toolset_manager.list_enabled_console_toolsets(dal=dal) return ToolExecutor(cli_toolsets)🧰 Tools
🪛 Pylint (3.3.7)
[convention] 218-218: Line too long (107/100)
(C0301)
[convention] 223-223: Line too long (107/100)
(C0301)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
holmes/config.py(11 hunks)holmes/core/tools.py(9 hunks)holmes/main.py(20 hunks)holmes/utils/env.py(1 hunks)tests/plugins/toolsets/test_tool_kafka.py(2 hunks)tests/test_issue_investigator.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/test_issue_investigator.py
🧰 Additional context used
🪛 Pylint (3.3.7)
holmes/main.py
[convention] 14-14: Import "import json" should be placed at the top of the module
(C0413)
[convention] 15-15: Import "import logging" should be placed at the top of the module
(C0413)
[convention] 16-16: Import "import socket" should be placed at the top of the module
(C0413)
[convention] 17-17: Import "import uuid" should be placed at the top of the module
(C0413)
[convention] 18-18: Import "import warnings" should be placed at the top of the module
(C0413)
[convention] 19-19: Import "from enum import Enum" should be placed at the top of the module
(C0413)
[convention] 20-20: Import "from pathlib import Path" should be placed at the top of the module
(C0413)
[convention] 21-21: Import "from typing import List, Optional" should be placed at the top of the module
(C0413)
[error] 23-23: Unable to import 'typer'
(E0401)
[convention] 23-23: Import "import typer" should be placed at the top of the module
(C0413)
[convention] 24-24: Import "from rich.console import Console" should be placed at the top of the module
(C0413)
[convention] 25-25: Import "from rich.logging import RichHandler" should be placed at the top of the module
(C0413)
[convention] 26-26: Import "from rich.markdown import Markdown" should be placed at the top of the module
(C0413)
[convention] 27-27: Import "from rich.rule import Rule" should be placed at the top of the module
(C0413)
[convention] 28-28: Import "from rich.table import Table" should be placed at the top of the module
(C0413)
[convention] 30-30: Import "from holmes import get_version" should be placed at the top of the module
(C0413)
[convention] 31-36: Import "from holmes.config import DEFAULT_CONFIG_LOCATION, Config, SourceFactory, SupportedTicketSources" should be placed at the top of the module
(C0413)
[convention] 37-37: Import "from holmes.core.resource_instruction import ResourceInstructionDocument" should be placed at the top of the module
(C0413)
[convention] 38-38: Import "from holmes.core.tool_calling_llm import LLMResult, ToolCallingLLM" should be placed at the top of the module
(C0413)
[convention] 39-39: Import "from holmes.core.tools import Toolset" should be placed at the top of the module
(C0413)
[convention] 40-40: Import "from holmes.plugins.destinations import DestinationType" should be placed at the top of the module
(C0413)
[convention] 41-41: Import "from holmes.plugins.interfaces import Issue" should be placed at the top of the module
(C0413)
[convention] 42-42: Import "from holmes.plugins.prompts import load_and_render_prompt" should be placed at the top of the module
(C0413)
[convention] 43-43: Import "from holmes.plugins.sources.opsgenie import OPSGENIE_TEAM_INTEGRATION_KEY_HELP" should be placed at the top of the module
(C0413)
[convention] 44-44: Import "from holmes.utils.file_utils import write_json_file" should be placed at the top of the module
(C0413)
[convention] 14-14: standard import "json" should be placed before first party import "holmes.utils.cert_utils.add_custom_certificate"
(C0411)
[convention] 15-15: standard import "logging" should be placed before first party import "holmes.utils.cert_utils.add_custom_certificate"
(C0411)
[convention] 16-16: standard import "socket" should be placed before first party import "holmes.utils.cert_utils.add_custom_certificate"
(C0411)
[convention] 17-17: standard import "uuid" should be placed before first party import "holmes.utils.cert_utils.add_custom_certificate"
(C0411)
[convention] 18-18: standard import "warnings" should be placed before first party import "holmes.utils.cert_utils.add_custom_certificate"
(C0411)
[convention] 19-19: standard import "enum.Enum" should be placed before first party import "holmes.utils.cert_utils.add_custom_certificate"
(C0411)
[convention] 20-20: standard import "pathlib.Path" should be placed before first party import "holmes.utils.cert_utils.add_custom_certificate"
(C0411)
[convention] 21-21: standard import "typing.List" should be placed before first party import "holmes.utils.cert_utils.add_custom_certificate"
(C0411)
[convention] 23-23: third party import "typer" should be placed before first party import "holmes.utils.cert_utils.add_custom_certificate"
(C0411)
[convention] 24-24: third party import "rich.console.Console" should be placed before first party import "holmes.utils.cert_utils.add_custom_certificate"
(C0411)
[convention] 25-25: third party import "rich.logging.RichHandler" should be placed before first party import "holmes.utils.cert_utils.add_custom_certificate"
(C0411)
[convention] 26-26: third party import "rich.markdown.Markdown" should be placed before first party import "holmes.utils.cert_utils.add_custom_certificate"
(C0411)
[convention] 27-27: third party import "rich.rule.Rule" should be placed before first party import "holmes.utils.cert_utils.add_custom_certificate"
(C0411)
[convention] 28-28: third party import "rich.table.Table" should be placed before first party import "holmes.utils.cert_utils.add_custom_certificate"
(C0411)
[convention] 30-30: Imports from package holmes are not grouped
(C0412)
[convention] 144-144: Line too long (151/100)
(C0301)
[convention] 1158-1158: Line too long (103/100)
(C0301)
[convention] 1144-1144: Missing function or method docstring
(C0116)
holmes/config.py
[convention] 1-1: Missing module docstring
(C0114)
[error] 10-10: Unable to import 'pydantic'
(E0401)
[convention] 100-100: Line too long (113/100)
(C0301)
[convention] 101-101: Line too long (101/100)
(C0301)
[convention] 102-102: Line too long (115/100)
(C0301)
[convention] 124-124: Missing function or method docstring
(C0116)
[convention] 152-152: Missing function or method docstring
(C0116)
[warning] 154-154: Use lazy % formatting in logging functions
(W1203)
[error] 159-159: Possibly using variable 'config_from_file' before assignment
(E0606)
[warning] 162-162: Use lazy % formatting in logging functions
(W1203)
[convention] 218-218: Line too long (107/100)
(C0301)
[convention] 223-223: Line too long (107/100)
(C0301)
holmes/core/tools.py
[convention] 1-1: Missing module docstring
(C0114)
[error] 13-13: Unable to import 'sentry_sdk'
(E0401)
[error] 15-15: Unable to import 'pydantic'
(E0401)
[convention] 305-305: Line too long (107/100)
(C0301)
[convention] 399-399: Line too long (101/100)
(C0301)
[warning] 422-422: Catching too general exception Exception
(W0718)
[refactor] 427-428: Consider merging these comparisons with 'in' by using 'self.status in (ToolsetStatusEnum.DISABLED, ToolsetStatusEnum.FAILED)'. Use a set instead if elements are hashable.
(R1714)
[warning] 430-430: Use lazy % formatting in logging functions
(W1203)
[convention] 523-523: Line too long (104/100)
(C0301)
holmes/utils/env.py
[convention] 1-1: Missing module docstring
(C0114)
[error] 6-6: Unable to import 'pydantic'
(E0401)
[convention] 9-9: Missing function or method docstring
(C0116)
[convention] 22-22: Missing function or method docstring
(C0116)
tests/plugins/toolsets/test_tool_kafka.py
[error] 7-7: Unable to import 'confluent_kafka.admin'
(E0401)
🔇 Additional comments (19)
holmes/utils/env.py (2)
17-17: Good fix for exception type specificity.The change from generic
ExceptiontoValueErrorimproves error handling clarity and follows best practices.
36-45: Good fix for variable naming convention.The change from
itertoitemremoves the shadowing of the built-initer()function and improves code clarity.tests/plugins/toolsets/test_tool_kafka.py (2)
4-17: Good import reorganization.The import statements have been properly reorganized for better clarity and consistency with the codebase structure.
🧰 Tools
🪛 Pylint (3.3.7)
[error] 7-7: Unable to import 'confluent_kafka.admin'
(E0401)
40-40: Status access pattern correctly implemented.The status check now uses direct attribute access (
kafka_toolset.status) which is consistent with the refactor pattern used throughout the codebase where status is accessed as a direct attribute rather than via method calls.holmes/main.py (4)
142-145: Good refactor of config file parameter type.The change from
Optional[str]toOptional[Path]with a default value improves type safety and consistency with the codebase.🧰 Tools
🪛 Pylint (3.3.7)
[convention] 144-144: Line too long (151/100)
(C0301)
383-383: Consistent parameter naming after refactor.The parameter name change to
custom_toolsets_from_clialigns with the centralized toolset management refactor and removal of theallowed_toolsetsconcept.
1115-1142: Well-structured CLI commands for toolset management.The new
listandrefreshcommands provide useful toolset management functionality that integrates well with the newToolsetManagerapproach. The commands correctly useconfig.toolset_manager.load_toolset_with_status()with appropriate refresh parameters.
1144-1161: Clean implementation of toolset status display.The
pretty_print_toolset_statusfunction provides a good user interface for displaying toolset information using Rich tables. The use ofmodel_dump_jsonwith specific field inclusion is appropriate for extracting the status data.🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1158-1158: Line too long (103/100)
(C0301)
[convention] 1144-1144: Missing function or method docstring
(C0116)
holmes/core/tools.py (6)
334-337: Good refactor to public attributes for status management.Converting the private attributes
_path,_status, and_errorto public attributespath,status,error, and adding thetypeattribute improves the API design and aligns with the centralized toolset management approach. This change makes the status information directly accessible without method calls.
97-99: Appropriate enum extensions for toolset types.The addition of
BUILTINandCUSTOMIZEDto theToolsetTypeenum provides clear categorization of toolset sources, which is essential for the new toolset management functionality.
452-459: Sensible defaults for YAMLToolset configuration.Setting
enabled = Trueby default for user-defined YAML toolsets and providing an empty list default for prerequisites makes sense from a usability perspective - users expect their custom toolsets to be enabled unless explicitly disabled.
380-425: Direct attribute assignment improves code clarity.The changes to directly assign to
self.statusandself.errorattributes instead of using private attributes with methods makes the code more straightforward and easier to understand.🧰 Tools
🪛 Pylint (3.3.7)
[convention] 399-399: Line too long (101/100)
(C0301)
[warning] 422-422: Catching too general exception Exception
(W0718)
476-476: Consistent status access in ToolExecutor.The update to use
toolset.statusdirectly instead of a method call is consistent with the public attribute refactor and improves code readability.
518-549: Well-designed partial override mechanism.The
ToolsetYamlFromConfigclass provides a clean way to handle partial toolset configuration overrides without requiring all fields to be specified. The optional field definitions and clear documentation explain its purpose for bypassing pydantic validation while allowing selective field overrides.🧰 Tools
🪛 Pylint (3.3.7)
[convention] 523-523: Line too long (104/100)
(C0301)
holmes/config.py (5)
56-56: Environment variable replacement refactored correctly.The move from inline environment variable replacement to using the dedicated
env_utils.replace_env_vars_valuesfunction improves code organization and reusability.
237-237: Approve delegation to ToolsetManager.The change to use
self.toolset_manager.list_enabled_server_toolsets(dal=dal)correctly delegates toolset filtering to the centralized manager.
248-250: Method signature simplification removes unnecessary parameter.The removal of the
allowed_toolsetsparameter from these methods is consistent with centralizing toolset management inToolsetManager. The toolset filtering is now handled internally.Also applies to: 273-274, 280-280
413-413: Type safety improvement with Path parameter.Changing the
config_fileparameter fromstrtoOptional[Path]improves type safety and is consistent with the updatedConfig.load_from_filemethod signature.
1-31:✅ Verification successful
Import reorganization looks good, but verify new utility modules exist.
The import reorganization with isort is clean and the new imports for
ToolsetManagerand utility modules align with the architectural changes.Let me verify that the new utility modules are available:
🏁 Script executed:
#!/bin/bash # Verify that the new imported modules exist fd -t f "env.py" holmes/utils/ --exec echo "Found: {}" fd -t f "toolset_manager.py" holmes/core/ --exec echo "Found: {}" fd -t f "file_utils.py" holmes/utils/ --exec echo "Found: {}" fd -t f "pydantic_utils.py" holmes/utils/ --exec echo "Found: {}"Length of output: 385
Import verification complete: new utility modules found
All newly referenced utility modules are present in the codebase:
- holmes/utils/env.py
- holmes/core/toolset_manager.py
- holmes/utils/file_utils.py
- holmes/utils/pydantic_utils.py
No further action required—imports are valid and the refactor stands.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
[error] 10-10: Unable to import 'pydantic'
(E0401)
mainred
force-pushed
the
cache-and-manage-toolset
branch
from
2566d02 to
24725ed
Compare
mainred
force-pushed
the
cache-and-manage-toolset
branch
from
b384219 to
d316b6c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (8)
tests/plugins/toolsets/test_tool_kafka.py (1)
40-40: Status access pattern is now correct.The change to
kafka_toolset.status == ToolsetStatusEnum.ENABLED(without parentheses) is consistent with the refactor where status is accessed as a direct attribute rather than a method call.holmes/main.py (1)
1121-1121: Fix typo in command help text.There's a typo where "build-int" should be "built-in".
Apply this diff:
- List build-int and custom toolsets status of CLI + List built-in and custom toolsets status of CLIholmes/core/toolset_manager.py (5)
60-62: Fix incorrect docstring for server_tool_tags property.The docstring incorrectly refers to "CLI tools" when it should mention "server tools".
- """ - Returns the list of toolset tags that are relevant for server tools. - A toolset is considered a server tool if it has any of UI tool tags: - """ + """ + Returns the list of toolset tags that are relevant for server tools. + A toolset is considered a server tool if it has any of server tool tags. + """
79-85: Simplify conditional branches using logical OR.The conditional logic can be streamlined for better readability.
- if ( - isinstance(toolset_config["enabled"], bool) - and not toolset_config["enabled"] - ) or ( - isinstance(toolset_config["enabled"], str) - and toolset_config["enabled"].lower() == "false" - ): - return False + if ( + (isinstance(toolset_config["enabled"], bool) and not toolset_config["enabled"]) + or (isinstance(toolset_config["enabled"], str) and toolset_config["enabled"].lower() == "false") + ): + return False🧰 Tools
🪛 Ruff (0.11.9)
78-85: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
189-198: Consider using the existing write_json_file utility for better error handling.The manual file writing lacks proper error handling. The existing utility provides robust error handling.
- if self.toolset_status_location and not os.path.exists( - os.path.dirname(self.toolset_status_location) - ): - os.makedirs(os.path.dirname(self.toolset_status_location)) - with open(self.toolset_status_location, "w") as f: - toolset_status = [ - json.loads( - toolset.model_dump_json( - include={"name", "status", "enabled", "type", "path", "error"} - ) - ) - for toolset in all_toolsets - ] - json.dump(toolset_status, f, indent=2) + from holmes.utils.file_utils import write_json_file + + toolset_status = [ + json.loads( + toolset.model_dump_json( + include={"name", "status", "enabled", "type", "path", "error"} + ) + ) + for toolset in all_toolsets + ] + write_json_file(self.toolset_status_location, toolset_status)🧰 Tools
🪛 Pylint (3.3.7)
[warning] 189-189: Using open without explicitly specifying an encoding
(W1514)
214-215: Add error handling for cache file loading.The JSON loading could fail if the cache file is corrupted or has permission issues.
- cached_toolsets: List[dict[str, Any]] = [] - with open(self.toolset_status_location, "r") as f: - cached_toolsets = json.load(f) + cached_toolsets: List[dict[str, Any]] = [] + try: + with open(self.toolset_status_location, "r") as f: + cached_toolsets = json.load(f) + except (json.JSONDecodeError, IOError) as e: + logging.error(f"Failed to load toolset cache: {e}. Refreshing status.") + self.refresh_toolset_status(dal) + with open(self.toolset_status_location, "r") as f: + cached_toolsets = json.load(f)🧰 Tools
🪛 Pylint (3.3.7)
[warning] 214-214: Using open without explicitly specifying an encoding
(W1514)
297-299: Improve exception chaining for better error traceability.Use proper exception chaining to preserve the original error context.
- except Exception as e: - raise ValueError( - f"Failed to load toolsets from {toolset_path}, error: {e}" - ) + except Exception as e: + raise ValueError( + f"Failed to load toolsets from {toolset_path}, error: {e}" + ) from eholmes/config.py (1)
163-175:⚠️ Potential issueFix undefined variable usage in load_from_file method.
The variable
config_from_filemay be used before assignment if the file doesn't exist.@classmethod def load_from_file(cls, config_file: Optional[Path], **kwargs) -> "Config": """ Load configuration from file and merge with CLI options. Args: config_file: Path to configuration file **kwargs: CLI options to override config file values Returns: Config instance with merged settings """ + config_from_file = None if config_file is not None and config_file.exists(): logging.debug(f"Loading config from {config_file}") config_from_file = load_model_from_file(cls, config_file) cli_options = {k: v for k, v in kwargs.items() if v is not None and v != []} if config_from_file is None: return cls(**cli_options) logging.debug(f"Overriding config from cli options {cli_options}") merged_config = config_from_file.dict() merged_config.update(cli_options) return cls(**merged_config)🧰 Tools
🪛 Pylint (3.3.7)
[warning] 164-164: Use lazy % formatting in logging functions
(W1203)
[error] 169-169: Possibly using variable 'config_from_file' before assignment
(E0606)
[warning] 172-172: Use lazy % formatting in logging functions
(W1203)
🧹 Nitpick comments (11)
tests/llm/utils/mock_toolset.py (1)
145-153: Remove unused parameter.The
run_liveparameter is not used within the method body and should be removed to eliminate the pylint warning.Apply this diff to remove the unused parameter:
- def _load_toolsets_definitions(self, run_live) -> List[Toolset]: + def _load_toolsets_definitions(self) -> List[Toolset]:Also update the method call on line 158:
- toolset_definitions = self._load_toolsets_definitions(run_live) + toolset_definitions = self._load_toolsets_definitions()🧰 Tools
🪛 Pylint (3.3.7)
[warning] 145-145: Unused argument 'run_live'
(W0613)
holmes/plugins/toolsets/consts.py (1)
1-5: LGTM: Good centralization of toolset constants.The new constants file effectively centralizes common strings used across multiple toolset plugins:
TOOLSET_CONFIG_MISSING_ERROR: Provides a consistent error message for missing configurationsSTANDARD_END_DATETIME_TOOL_PARAM_DESCRIPTION: Standardizes parameter descriptions for datetime fieldsThis approach reduces duplication and ensures consistency across the codebase.
Consider adding a module docstring to improve documentation:
+"""Constants for toolset plugins.""" + TOOLSET_CONFIG_MISSING_ERROR = "The toolset is missing its configuration"🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
holmes/core/resource_instruction.py (1)
1-18: Add module docstring for better documentationThe module is well-structured and the Pydantic models are appropriately defined. Consider adding a module-level docstring to describe the purpose of this module.
+""" +Resource instruction models for investigation context. + +This module defines Pydantic models for representing investigation instructions +and document references that can be used to provide additional context during +Holmes investigations. +""" from typing import List🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
[error] 3-3: Unable to import 'pydantic'
(E0401)
[refactor] 6-6: Too few public methods (0/2)
(R0903)
[convention] 15-15: Missing class docstring
(C0115)
[refactor] 15-15: Too few public methods (0/2)
(R0903)
tests/plugins/toolsets/test_load_config.py (2)
65-68: Use mock values instead of realistic API keysThe test data contains what appears to be a realistic API key format. For security and clarity, consider using clearly fake values in test data.
env_vars = { - "GRAFANA_API_KEY": "glsa_sdj1q2o3prujpqfd", + "GRAFANA_API_KEY": "test_api_key_123", "GRAFANA_URL": "https://my-grafana.com/", }🧰 Tools
🪛 Gitleaks (8.26.0)
66-66: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
1-6: Add module and function docstringsConsider adding a module docstring and docstrings for the test functions to improve code documentation.
+"""Tests for toolset configuration loading functionality.""" import os import pytest🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
holmes/plugins/toolsets/__init__.py (3)
40-48: Add encoding parameter for file operations.The file is opened without explicitly specifying encoding, which can lead to issues on different platforms.
Apply this diff:
- with open(toolsets_path) as file: + with open(toolsets_path, encoding='utf-8') as file:🧰 Tools
🪛 Pylint (3.3.7)
[warning] 40-40: Using open without explicitly specifying an encoding
(W1514)
125-127: Line length and formatting improvement needed.The line is too long and the error message formatting could be improved.
Apply this diff:
- message = "Old toolset config format detected, please update to the new format: https://docs.robusta.dev/master/configuration/holmesgpt/custom_toolsets.html" + message = ( + "Old toolset config format detected, please update to the new format: " + "https://docs.robusta.dev/master/configuration/holmesgpt/custom_toolsets.html" + )🧰 Tools
🪛 Pylint (3.3.7)
[convention] 125-125: Line too long (165/100)
(C0301)
104-106: Simplify boolean return.The condition can be simplified as suggested by static analysis.
Apply this diff:
- if isinstance(toolsets, list): - return True - return False + return isinstance(toolsets, list)🧰 Tools
🪛 Ruff (0.11.9)
104-106: Return the condition
bool(isinstance(toolsets, list))directlyReplace with
return bool(isinstance(toolsets, list))(SIM103)
holmes/core/toolset_manager.py (2)
245-245: Optimize dictionary key checking.Using
.keys()is unnecessary when checking for key existence.- if custom_toolset_from_cli in toolsets_status_by_name.keys(): + if custom_toolset_from_cli.name in toolsets_status_by_name:🧰 Tools
🪛 Ruff (0.11.9)
245-245: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
380-380: Optimize dictionary key checking.Using
.keys()is unnecessary when checking for key existence.- if new_toolset.name in existing_toolsets_by_name.keys(): + if new_toolset.name in existing_toolsets_by_name:🧰 Tools
🪛 Ruff (0.11.9)
380-380: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
holmes/core/tools.py (1)
427-428: Optimize status comparison using 'in' operator.The status comparison can be simplified for better readability.
- if ( - self.status == ToolsetStatusEnum.DISABLED - or self.status == ToolsetStatusEnum.FAILED - ): + if self.status in (ToolsetStatusEnum.DISABLED, ToolsetStatusEnum.FAILED):🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 427-428: Consider merging these comparisons with 'in' by using 'self.status in (ToolsetStatusEnum.DISABLED, ToolsetStatusEnum.FAILED)'. Use a set instead if elements are hashable.
(R1714)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
holmes/config.py(11 hunks)holmes/core/resource_instruction.py(1 hunks)holmes/core/supabase_dal.py(1 hunks)holmes/core/tool_calling_llm.py(1 hunks)holmes/core/tools.py(9 hunks)holmes/core/toolset_manager.py(1 hunks)holmes/main.py(20 hunks)holmes/plugins/prompts/_fetch_logs.jinja2(3 hunks)holmes/plugins/prompts/_toolsets_instructions.jinja2(2 hunks)holmes/plugins/toolsets/__init__.py(2 hunks)holmes/plugins/toolsets/consts.py(1 hunks)holmes/plugins/toolsets/coralogix/toolset_coralogix_logs.py(3 hunks)holmes/plugins/toolsets/grafana/base_grafana_toolset.py(1 hunks)holmes/plugins/toolsets/grafana/toolset_grafana_tempo.py(1 hunks)holmes/plugins/toolsets/kafka.py(1 hunks)holmes/plugins/toolsets/opensearch/opensearch.py(1 hunks)holmes/plugins/toolsets/prometheus/prometheus.py(1 hunks)holmes/plugins/toolsets/utils.py(1 hunks)holmes/utils/default_toolset_installation_guide.jinja2(1 hunks)holmes/utils/definitions.py(1 hunks)holmes/utils/env.py(1 hunks)holmes/utils/holmes_sync_toolsets.py(2 hunks)holmes/utils/pydantic_utils.py(3 hunks)tests/config_class/test_config_load_custom_toolsets.py(0 hunks)tests/config_class/test_config_load_remote_mcp_server.py(2 hunks)tests/core/test_toolset_manager.py(1 hunks)tests/llm/utils/mock_toolset.py(4 hunks)tests/plugins/prompt/test_generic_ask_conversation.py(1 hunks)tests/plugins/toolsets/grafana/test_grafana_loki.py(2 hunks)tests/plugins/toolsets/grafana/test_grafana_tempo.py(2 hunks)tests/plugins/toolsets/test_internet.py(2 hunks)tests/plugins/toolsets/test_load_config.py(1 hunks)tests/plugins/toolsets/test_notion.py(2 hunks)tests/plugins/toolsets/test_prometheus_integration.py(2 hunks)tests/plugins/toolsets/test_tool_kafka.py(2 hunks)tests/test_check_prerequisites.py(10 hunks)tests/test_holmes_sync_toolsets.py(2 hunks)tests/test_issue_investigator.py(1 hunks)
💤 Files with no reviewable changes (1)
- tests/config_class/test_config_load_custom_toolsets.py
✅ Files skipped from review due to trivial changes (3)
- tests/plugins/toolsets/test_prometheus_integration.py
- tests/test_issue_investigator.py
- holmes/utils/default_toolset_installation_guide.jinja2
🚧 Files skipped from review as they are similar to previous changes (7)
- holmes/plugins/prompts/_fetch_logs.jinja2
- holmes/plugins/toolsets/utils.py
- holmes/utils/holmes_sync_toolsets.py
- holmes/plugins/toolsets/grafana/base_grafana_toolset.py
- tests/plugins/toolsets/test_notion.py
- holmes/plugins/prompts/_toolsets_instructions.jinja2
- tests/config_class/test_config_load_remote_mcp_server.py
🧰 Additional context used
🧬 Code Graph Analysis (10)
tests/plugins/prompt/test_generic_ask_conversation.py (1)
holmes/core/tools.py (1)
ToolsetStatusEnum(84-87)
holmes/core/supabase_dal.py (2)
holmes/core/resource_instruction.py (2)
ResourceInstructionDocument(6-12)ResourceInstructions(15-17)holmes/utils/definitions.py (1)
RobustaConfig(11-13)
holmes/core/tool_calling_llm.py (8)
tests/llm/utils/mock_utils.py (1)
Message(33-34)holmes/core/investigation_structured_output.py (3)
is_response_an_incorrect_tool_call(229-264)parse_markdown_into_sections_from_hash_sign(101-141)process_response_into_sections(213-226)holmes/core/issue.py (1)
Issue(13-54)holmes/core/llm.py (1)
LLM(30-58)holmes/core/resource_instruction.py (1)
ResourceInstructions(15-17)holmes/core/runbooks.py (1)
RunbookManager(7-26)holmes/core/tools.py (3)
StructuredToolResult(27-50)ToolExecutor(470-515)ToolResultStatus(21-24)holmes/plugins/prompts/__init__.py (1)
load_and_render_prompt(26-43)
holmes/plugins/toolsets/grafana/toolset_grafana_tempo.py (6)
holmes/common/env_vars.py (1)
load_bool(5-7)holmes/core/tools.py (4)
StructuredToolResult(27-50)Tool(108-138)ToolParameter(102-105)ToolResultStatus(21-24)holmes/plugins/toolsets/grafana/common.py (3)
GrafanaConfig(9-21)build_headers(24-35)get_base_url(51-55)holmes/plugins/toolsets/grafana/tempo_api.py (2)
query_tempo_trace_by_id(80-124)query_tempo_traces(54-77)holmes/plugins/toolsets/grafana/trace_parser.py (1)
format_traces_list(177-195)holmes/plugins/toolsets/utils.py (2)
get_param_or_raise(128-132)process_timestamps_to_int(79-125)
holmes/plugins/toolsets/kafka.py (2)
holmes/core/tools.py (7)
CallablePrerequisite(288-289)StructuredToolResult(27-50)Tool(108-138)ToolParameter(102-105)ToolResultStatus(21-24)Toolset(301-445)ToolsetTag(90-93)holmes/plugins/toolsets/utils.py (1)
get_param_or_raise(128-132)
holmes/plugins/toolsets/opensearch/opensearch.py (1)
holmes/core/tools.py (7)
CallablePrerequisite(288-289)StructuredToolResult(27-50)Tool(108-138)ToolParameter(102-105)ToolResultStatus(21-24)Toolset(301-445)ToolsetTag(90-93)
tests/plugins/toolsets/grafana/test_grafana_loki.py (3)
holmes/core/tools.py (1)
ToolsetStatusEnum(84-87)holmes/plugins/toolsets/grafana/grafana_api.py (1)
get_health(19-34)holmes/plugins/toolsets/grafana/toolset_grafana_loki.py (2)
GetLokiLogsForResource(130-205)GrafanaLokiConfig(33-34)
tests/plugins/toolsets/test_internet.py (1)
holmes/core/tools.py (1)
ToolsetStatusEnum(84-87)
tests/plugins/toolsets/test_load_config.py (1)
holmes/plugins/toolsets/__init__.py (1)
load_toolsets_from_config(109-153)
tests/test_check_prerequisites.py (3)
holmes/core/tools.py (8)
CallablePrerequisite(288-289)StaticPrerequisite(283-285)Tool(108-138)Toolset(301-445)ToolsetCommandPrerequisite(292-294)ToolsetEnvironmentPrerequisite(297-298)ToolsetStatusEnum(84-87)check_prerequisites(379-434)tests/utils/toolsets.py (3)
callable_failure_no_message(12-13)callable_failure_with_message(8-9)callable_success(4-5)tests/test_holmes_sync_toolsets.py (1)
SampleToolset(38-40)
🪛 Pylint (3.3.7)
holmes/plugins/toolsets/__init__.py
[error] 7-7: Unable to import 'pydantic'
(E0401)
[convention] 36-36: Missing function or method docstring
(C0116)
[warning] 40-40: Using open without explicitly specifying an encoding
(W1514)
[convention] 125-125: Line too long (165/100)
(C0301)
[convention] 100-100: Missing function or method docstring
(C0116)
[warning] 150-150: Catching too general exception Exception
(W0718)
[warning] 148-148: Use lazy % formatting in logging functions
(W1203)
holmes/core/toolset_manager.py
[convention] 70-70: Line too long (120/100)
(C0301)
[convention] 71-71: Line too long (118/100)
(C0301)
[convention] 87-87: Line too long (116/100)
(C0301)
[convention] 99-99: Line too long (121/100)
(C0301)
[convention] 108-108: Line too long (106/100)
(C0301)
[convention] 121-121: Line too long (107/100)
(C0301)
[convention] 162-162: Line too long (120/100)
(C0301)
[convention] 176-176: Line too long (115/100)
(C0301)
[convention] 237-237: Line too long (118/100)
(C0301)
[convention] 243-243: Line too long (106/100)
(C0301)
[convention] 247-247: Line too long (109/100)
(C0301)
[convention] 324-324: Line too long (115/100)
(C0301)
[convention] 350-350: Line too long (110/100)
(C0301)
[convention] 1-1: Missing module docstring
(C0114)
[error] 6-6: Unable to import 'benedict'
(E0401)
[error] 7-7: Unable to import 'pydantic'
(E0401)
[warning] 143-143: Unused argument 'dal'
(W0613)
[warning] 189-189: Using open without explicitly specifying an encoding
(W1514)
[warning] 199-199: Use lazy % formatting in logging functions
(W1203)
[warning] 214-214: Using open without explicitly specifying an encoding
(W1514)
[warning] 246-248: Raising too general exception: Exception
(W0719)
[warning] 322-325: Raising too general exception: Exception
(W0719)
holmes/main.py
[convention] 14-14: Import "import json" should be placed at the top of the module
(C0413)
[convention] 15-15: Import "import logging" should be placed at the top of the module
(C0413)
[convention] 16-16: Import "import socket" should be placed at the top of the module
(C0413)
[convention] 17-17: Import "import uuid" should be placed at the top of the module
(C0413)
[convention] 18-18: Import "import warnings" should be placed at the top of the module
(C0413)
[convention] 19-19: Import "from enum import Enum" should be placed at the top of the module
(C0413)
[convention] 20-20: Import "from pathlib import Path" should be placed at the top of the module
(C0413)
[convention] 21-21: Import "from typing import List, Optional" should be placed at the top of the module
(C0413)
[error] 23-23: Unable to import 'typer'
(E0401)
[convention] 23-23: Import "import typer" should be placed at the top of the module
(C0413)
[convention] 24-24: Import "from rich.console import Console" should be placed at the top of the module
(C0413)
[convention] 25-25: Import "from rich.logging import RichHandler" should be placed at the top of the module
(C0413)
[convention] 26-26: Import "from rich.markdown import Markdown" should be placed at the top of the module
(C0413)
[convention] 27-27: Import "from rich.rule import Rule" should be placed at the top of the module
(C0413)
[convention] 28-28: Import "from rich.table import Table" should be placed at the top of the module
(C0413)
[convention] 30-30: Import "from holmes import get_version" should be placed at the top of the module
(C0413)
[convention] 31-36: Import "from holmes.config import DEFAULT_CONFIG_LOCATION, Config, SourceFactory, SupportedTicketSources" should be placed at the top of the module
(C0413)
[convention] 37-37: Import "from holmes.core.resource_instruction import ResourceInstructionDocument" should be placed at the top of the module
(C0413)
[convention] 38-38: Import "from holmes.core.tool_calling_llm import LLMResult, ToolCallingLLM" should be placed at the top of the module
(C0413)
[convention] 39-39: Import "from holmes.core.tools import Toolset" should be placed at the top of the module
(C0413)
[convention] 40-40: Import "from holmes.plugins.destinations import DestinationType" should be placed at the top of the module
(C0413)
[convention] 41-41: Import "from holmes.plugins.interfaces import Issue" should be placed at the top of the module
(C0413)
[convention] 42-42: Import "from holmes.plugins.prompts import load_and_render_prompt" should be placed at the top of the module
(C0413)
[convention] 43-43: Import "from holmes.plugins.sources.opsgenie import OPSGENIE_TEAM_INTEGRATION_KEY_HELP" should be placed at the top of the module
(C0413)
[convention] 44-44: Import "from holmes.utils.file_utils import write_json_file" should be placed at the top of the module
(C0413)
[convention] 14-14: standard import "json" should be placed before first party import "holmes.utils.cert_utils.add_custom_certificate"
(C0411)
[convention] 15-15: standard import "logging" should be placed before first party import "holmes.utils.cert_utils.add_custom_certificate"
(C0411)
[convention] 16-16: standard import "socket" should be placed before first party import "holmes.utils.cert_utils.add_custom_certificate"
(C0411)
[convention] 17-17: standard import "uuid" should be placed before first party import "holmes.utils.cert_utils.add_custom_certificate"
(C0411)
[convention] 18-18: standard import "warnings" should be placed before first party import "holmes.utils.cert_utils.add_custom_certificate"
(C0411)
[convention] 19-19: standard import "enum.Enum" should be placed before first party import "holmes.utils.cert_utils.add_custom_certificate"
(C0411)
[convention] 20-20: standard import "pathlib.Path" should be placed before first party import "holmes.utils.cert_utils.add_custom_certificate"
(C0411)
[convention] 21-21: standard import "typing.List" should be placed before first party import "holmes.utils.cert_utils.add_custom_certificate"
(C0411)
[convention] 23-23: third party import "typer" should be placed before first party import "holmes.utils.cert_utils.add_custom_certificate"
(C0411)
[convention] 24-24: third party import "rich.console.Console" should be placed before first party import "holmes.utils.cert_utils.add_custom_certificate"
(C0411)
[convention] 25-25: third party import "rich.logging.RichHandler" should be placed before first party import "holmes.utils.cert_utils.add_custom_certificate"
(C0411)
[convention] 26-26: third party import "rich.markdown.Markdown" should be placed before first party import "holmes.utils.cert_utils.add_custom_certificate"
(C0411)
[convention] 27-27: third party import "rich.rule.Rule" should be placed before first party import "holmes.utils.cert_utils.add_custom_certificate"
(C0411)
[convention] 28-28: third party import "rich.table.Table" should be placed before first party import "holmes.utils.cert_utils.add_custom_certificate"
(C0411)
[convention] 30-30: Imports from package holmes are not grouped
(C0412)
[convention] 144-144: Line too long (151/100)
(C0301)
[convention] 1158-1158: Line too long (103/100)
(C0301)
[convention] 1144-1144: Missing function or method docstring
(C0116)
holmes/config.py
[convention] 1-1: Missing module docstring
(C0114)
[error] 10-10: Unable to import 'pydantic'
(E0401)
[convention] 100-100: Line too long (113/100)
(C0301)
[convention] 101-101: Line too long (101/100)
(C0301)
[convention] 102-102: Line too long (115/100)
(C0301)
[convention] 124-124: Missing function or method docstring
(C0116)
[warning] 164-164: Use lazy % formatting in logging functions
(W1203)
[error] 169-169: Possibly using variable 'config_from_file' before assignment
(E0606)
[warning] 172-172: Use lazy % formatting in logging functions
(W1203)
[convention] 228-228: Line too long (107/100)
(C0301)
[convention] 233-233: Line too long (107/100)
(C0301)
holmes/core/resource_instruction.py
[convention] 1-1: Missing module docstring
(C0114)
[error] 3-3: Unable to import 'pydantic'
(E0401)
[refactor] 6-6: Too few public methods (0/2)
(R0903)
[convention] 15-15: Missing class docstring
(C0115)
[refactor] 15-15: Too few public methods (0/2)
(R0903)
holmes/core/supabase_dal.py
[error] 12-12: Unable to import 'cachetools'
(E0401)
[error] 13-13: Unable to import 'postgrest._sync.request_builder'
(E0401)
[error] 14-14: Unable to import 'postgrest.exceptions'
(E0401)
[error] 15-15: Unable to import 'postgrest.types'
(E0401)
[error] 16-16: Unable to import 'pydantic'
(E0401)
[error] 17-17: Unable to import 'supabase'
(E0401)
[error] 18-18: Unable to import 'supabase.lib.client_options'
(E0401)
holmes/core/tool_calling_llm.py
[error] 8-8: Unable to import 'sentry_sdk'
(E0401)
[error] 9-9: Unable to import 'litellm.types.utils'
(E0401)
[error] 10-10: Unable to import 'openai'
(E0401)
[error] 11-13: Unable to import 'openai.types.chat.chat_completion_message_tool_call'
(E0401)
[error] 14-14: Unable to import 'pydantic'
(E0401)
[error] 15-15: Unable to import 'pydantic_core'
(E0401)
holmes/core/tools.py
[convention] 1-1: Missing module docstring
(C0114)
[error] 13-13: Unable to import 'sentry_sdk'
(E0401)
[error] 15-15: Unable to import 'pydantic'
(E0401)
[convention] 305-305: Line too long (107/100)
(C0301)
[convention] 399-399: Line too long (101/100)
(C0301)
[warning] 422-422: Catching too general exception Exception
(W0718)
[refactor] 427-428: Consider merging these comparisons with 'in' by using 'self.status in (ToolsetStatusEnum.DISABLED, ToolsetStatusEnum.FAILED)'. Use a set instead if elements are hashable.
(R1714)
[warning] 430-430: Use lazy % formatting in logging functions
(W1203)
[convention] 523-523: Line too long (104/100)
(C0301)
holmes/plugins/toolsets/consts.py
[convention] 1-1: Missing module docstring
(C0114)
holmes/plugins/toolsets/coralogix/toolset_coralogix_logs.py
[error] 3-11: Unable to import 'holmes.core.tools'
(E0401)
[error] 12-15: Unable to import 'holmes.plugins.toolsets.consts'
(E0401)
[error] 29-29: Unable to import 'holmes.plugins.toolsets.utils'
(E0401)
holmes/plugins/toolsets/grafana/toolset_grafana_tempo.py
[error] 6-6: Unable to import 'pydantic'
(E0401)
holmes/plugins/toolsets/kafka.py
[error] 5-19: Unable to import 'confluent_kafka.admin'
(E0401)
[error] 20-20: Unable to import 'confluent_kafka.admin'
(E0401)
[error] 21-21: Unable to import 'pydantic'
(E0401)
holmes/plugins/toolsets/opensearch/opensearch.py
[error] 4-4: Unable to import 'opensearchpy'
(E0401)
[error] 5-5: Unable to import 'pydantic'
(E0401)
holmes/plugins/toolsets/prometheus/prometheus.py
[convention] 1-1: Missing module docstring
(C0114)
[error] 12-12: Unable to import 'pydantic'
(E0401)
[error] 15-23: Unable to import 'holmes.core.tools'
(E0401)
[error] 24-24: Unable to import 'holmes.plugins.toolsets.consts'
(E0401)
[error] 25-25: Unable to import 'holmes.plugins.toolsets.service_discovery'
(E0401)
holmes/utils/definitions.py
[error] 4-4: Unable to import 'pydantic'
(E0401)
holmes/utils/env.py
[convention] 1-1: Missing module docstring
(C0114)
[error] 6-6: Unable to import 'pydantic'
(E0401)
[convention] 9-9: Missing function or method docstring
(C0116)
[convention] 22-22: Missing function or method docstring
(C0116)
holmes/utils/pydantic_utils.py
[error] 5-5: Unable to import 'typer'
(E0401)
[error] 6-6: Unable to import 'benedict'
(E0401)
[error] 7-7: Unable to import 'pydantic'
(E0401)
[convention] 51-51: Line too long (110/100)
(C0301)
tests/core/test_toolset_manager.py
[convention] 1-1: Missing module docstring
(C0114)
[convention] 14-14: Missing function or method docstring
(C0116)
[convention] 30-30: Missing function or method docstring
(C0116)
[convention] 34-34: Missing function or method docstring
(C0116)
[warning] 34-34: Redefining name 'toolset_manager' from outer scope (line 14)
(W0621)
[convention] 40-40: Missing function or method docstring
(C0116)
[warning] 40-40: Redefining name 'toolset_manager' from outer scope (line 14)
(W0621)
[convention] 48-48: Missing function or method docstring
(C0116)
[warning] 49-49: Redefining name 'toolset_manager' from outer scope (line 14)
(W0621)
[warning] 63-63: Access to a protected member _list_all_toolsets of a client class
(W0212)
[convention] 70-70: Missing function or method docstring
(C0116)
[warning] 70-70: Redefining name 'toolset_manager' from outer scope (line 14)
(W0621)
[warning] 94-94: Using open without explicitly specifying an encoding
(W1514)
[convention] 100-100: Missing function or method docstring
(C0116)
[warning] 100-100: Redefining name 'toolset_manager' from outer scope (line 14)
(W0621)
[warning] 122-122: Using open without explicitly specifying an encoding
(W1514)
[convention] 131-131: Missing function or method docstring
(C0116)
[warning] 131-131: Redefining name 'toolset_manager' from outer scope (line 14)
(W0621)
[convention] 141-141: Missing function or method docstring
(C0116)
[warning] 141-141: Redefining name 'toolset_manager' from outer scope (line 14)
(W0621)
[convention] 151-151: Missing function or method docstring
(C0116)
[warning] 151-151: Redefining name 'toolset_manager' from outer scope (line 14)
(W0621)
[convention] 167-167: Missing function or method docstring
(C0116)
[warning] 168-168: Redefining name 'toolset_manager' from outer scope (line 14)
(W0621)
[warning] 168-168: Unused argument 'mock_benedict'
(W0613)
[warning] 168-168: Unused argument 'mock_load_toolsets_from_config'
(W0613)
[convention] 175-175: Missing function or method docstring
(C0116)
[warning] 175-175: Redefining name 'toolset_manager' from outer scope (line 14)
(W0621)
[convention] 181-181: Missing function or method docstring
(C0116)
[convention] 195-195: Missing function or method docstring
(C0116)
[convention] 203-203: Missing function or method docstring
(C0116)
[warning] 203-203: Redefining name 'toolset_manager' from outer scope (line 14)
(W0621)
[convention] 224-224: Missing function or method docstring
(C0116)
[warning] 224-224: Redefining name 'toolset_manager' from outer scope (line 14)
(W0621)
[convention] 247-247: Missing function or method docstring
(C0116)
[warning] 247-247: Redefining name 'toolset_manager' from outer scope (line 14)
(W0621)
[convention] 259-259: Missing function or method docstring
(C0116)
[warning] 259-259: Redefining name 'toolset_manager' from outer scope (line 14)
(W0621)
[convention] 269-269: Missing function or method docstring
(C0116)
[warning] 269-269: Redefining name 'toolset_manager' from outer scope (line 14)
(W0621)
tests/llm/utils/mock_toolset.py
[error] 7-7: Unable to import 'pydantic'
(E0401)
[warning] 145-145: Unused argument 'run_live'
(W0613)
tests/plugins/toolsets/grafana/test_grafana_loki.py
[error] 6-6: Unable to import 'holmes.core.tools'
(E0401)
[error] 7-7: Unable to import 'holmes.plugins.toolsets.grafana.grafana_api'
(E0401)
tests/plugins/toolsets/grafana/test_grafana_tempo.py
[error] 6-6: Unable to import 'holmes.core.tools'
(E0401)
[error] 7-7: Unable to import 'holmes.plugins.toolsets.grafana.grafana_api'
(E0401)
tests/plugins/toolsets/test_internet.py
[error] 7-7: Unable to import 'pydantic'
(E0401)
tests/plugins/toolsets/test_load_config.py
[convention] 1-1: Missing module docstring
(C0114)
[convention] 9-9: Missing function or method docstring
(C0116)
[convention] 28-28: Missing function or method docstring
(C0116)
[convention] 57-57: Constant name "toolsets_config_str" doesn't conform to UPPER_CASE naming style
(C0103)
[convention] 71-71: Missing function or method docstring
(C0116)
tests/plugins/toolsets/test_tool_kafka.py
[error] 7-7: Unable to import 'confluent_kafka.admin'
(E0401)
tests/test_check_prerequisites.py
[convention] 53-53: Missing function or method docstring
(C0116)
[convention] 61-61: Missing function or method docstring
(C0116)
[convention] 114-114: Missing function or method docstring
(C0116)
[convention] 123-123: Missing function or method docstring
(C0116)
[convention] 131-131: Missing function or method docstring
(C0116)
[convention] 139-139: Missing function or method docstring
(C0116)
[convention] 147-147: Missing function or method docstring
(C0116)
tests/test_holmes_sync_toolsets.py
[convention] 1-1: Missing module docstring
(C0114)
🪛 Ruff (0.11.9)
holmes/plugins/toolsets/__init__.py
104-106: Return the condition bool(isinstance(toolsets, list)) directly
Replace with return bool(isinstance(toolsets, list))
(SIM103)
holmes/core/toolset_manager.py
78-85: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
245-245: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
380-380: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
🪛 Gitleaks (8.26.0)
tests/plugins/toolsets/test_load_config.py
66-66: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (43)
holmes/utils/definitions.py (1)
4-4: LGTM: Import reordering improvement.The import reorganization aligns with the PR's objective of using isort for better code organization. The pylint error about being unable to import 'pydantic' appears to be a false positive since pydantic is a standard dependency and the import syntax is correct.
🧰 Tools
🪛 Pylint (3.3.7)
[error] 4-4: Unable to import 'pydantic'
(E0401)
tests/llm/utils/mock_toolset.py (2)
4-11: LGTM: Import updates align with toolset management refactoring.The import changes properly reflect the refactoring where toolset loading functionality has been moved to
holmes.plugins.toolsetsand status handling has been updated in the core tools module.🧰 Tools
🪛 Pylint (3.3.7)
[error] 7-7: Unable to import 'pydantic'
(E0401)
227-227: LGTM: Updated status access pattern.The changes from
get_status()method calls to directstatusattribute access align with the broader refactoring that moved from private methods/attributes to public attributes for toolset status management.Also applies to: 236-236
tests/plugins/toolsets/test_internet.py (2)
3-3: LGTM: Import reordering improvement.The import reorganization follows proper grouping conventions and aligns with the isort-based cleanup mentioned in the PR objectives.
Also applies to: 7-7
116-116: LGTM: Updated to public status attribute.The change from private
_statusto publicstatusattribute is consistent with the broader refactoring that standardized toolset status access patterns across the codebase.tests/plugins/prompt/test_generic_ask_conversation.py (1)
11-11: LGTM: Consistent status attribute migration.The change from private
_statusto publicstatusattribute maintains consistency with the broader refactoring that standardized toolset status access patterns throughout the codebase.tests/plugins/toolsets/grafana/test_grafana_loki.py (2)
3-3: LGTM: Import reorganization is well-structured.The addition of imports for
ToolsetStatusEnum,get_health, andGrafanaLokiConfigproperly supports the test functionality and follows good import organization practices.Also applies to: 6-7, 10-10
40-41: LGTM: Consistent with toolset refactor pattern.The change from method calls (
get_error(),get_status()) to direct attribute access (error,status) is consistent with the broader refactor where toolset status and error are now public attributes instead of getter methods.tests/plugins/toolsets/test_tool_kafka.py (1)
4-6: LGTM: Import reorganization improves code structure.The reordering of imports groups related modules logically and follows good organization practices.
Also applies to: 8-8, 14-16
holmes/plugins/toolsets/opensearch/opensearch.py (1)
4-4: LGTM: Import reorganization and constant centralization.The import reorganization groups related imports logically, and moving
TOOLSET_CONFIG_MISSING_ERRORfromutilstoconstsmodule supports better centralized constant management across the codebase.Also applies to: 6-6, 9-9, 12-12, 16-16
🧰 Tools
🪛 Pylint (3.3.7)
[error] 4-4: Unable to import 'opensearchpy'
(E0401)
holmes/plugins/toolsets/grafana/toolset_grafana_tempo.py (2)
3-3: LGTM: Import reorganization follows good practices.The import reordering groups related imports logically and consolidates imports from the same module for better readability.
Also applies to: 5-7, 10-10, 13-13, 21-24
30-30: LGTM: Local constant definition reduces dependencies.Defining
ONE_HOUR_IN_SECONDS = 3600locally is a good approach that reduces external dependencies for a simple constant value. The value is correct and clearly represents one hour in seconds.tests/test_holmes_sync_toolsets.py (2)
1-2: LGTM: Import reorganization improves code organization.The import statements have been properly reorganized with standard library imports (
os,subprocess) at the top, followed by project imports grouped logically. This enhances readability and follows Python import conventions.Also applies to: 5-5, 8-8, 10-11, 19-20
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
352-352: LGTM: Updated to use direct attribute access.The changes correctly replace method calls (
get_status(),get_error()) with direct attribute access (.status,.error). This aligns with the broader refactor of the Toolset class API and improves simplicity by using public attributes instead of getter methods.Also applies to: 355-355, 357-360
tests/plugins/toolsets/grafana/test_grafana_tempo.py (2)
3-3: LGTM: Import reorganization improves clarity.The import statements have been reorganized to group related imports together, with
get_healthmoved alongside other Grafana-related imports andGetTempoTracesrepositioned within thetoolset_grafana_tempoimports. This enhances code readability and logical grouping.Also applies to: 5-5, 7-7, 9-9
83-84: LGTM: Updated to use direct attribute access.The assertions have been correctly updated to access the
statusanderrorattributes directly instead of callingget_status()andget_error()methods. This change is consistent with the refactored Toolset class API that uses public attributes.holmes/plugins/toolsets/coralogix/toolset_coralogix_logs.py (2)
2-2: LGTM: Centralizing constants improves maintainability.The constants
STANDARD_END_DATETIME_TOOL_PARAM_DESCRIPTIONandTOOLSET_CONFIG_MISSING_ERRORhave been moved fromholmes.plugins.toolsets.utilsto the new dedicatedholmes.plugins.toolsets.constsmodule. This centralization reduces duplication, improves maintainability, and provides a single source of truth for toolset-related constants.Also applies to: 8-8, 12-15, 29-29
114-116: LGTM: Improved code formatting.The return statement formatting has been improved by using a single-line conditional expression within parentheses for the
statusfield. This is more concise and readable while maintaining the same functionality.holmes/utils/pydantic_utils.py (3)
2-3: LGTM: Improved import organization and Path supportGood addition of
Pathimport and reorganization of typing imports for better structure.
40-40: Excellent type safety improvementChanging the parameter type from
strtoPathimproves type safety and aligns with modern Python best practices for file path handling. This change is consistent with the broader refactoring mentioned in the AI summary.
51-51: Minor formatting improvement addressedThe extra space removal in the error message improves consistency.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 51-51: Line too long (110/100)
(C0301)
tests/test_check_prerequisites.py (2)
2-4: LGTM: Improved import organizationGood reorganization of imports by grouping related imports together for better readability.
Also applies to: 7-8, 13-13, 17-18
49-50: Excellent refactor: Direct attribute accessThe systematic replacement of getter methods (
get_status(),get_error()) with direct attribute access (status,error) aligns perfectly with the broader refactor described in the AI summary. This change:
- Simplifies the API by removing unnecessary getter methods
- Maintains consistency with the new
Toolsetclass design- Preserves all existing test logic and coverage
The relevant code snippets from
holmes/core/tools.pyconfirm thatstatusanderrorare now public attributes on theToolsetclass.Also applies to: 57-58, 66-67, 78-79, 98-99, 110-111, 118-119, 127-128, 135-136, 143-144, 151-152, 167-168, 187-188, 211-212, 264-265, 286-287, 291-291
holmes/plugins/toolsets/kafka.py (2)
2-3: LGTM: Improved import organizationExcellent reorganization of imports by:
- Grouping all typing imports together
- Consolidating confluent_kafka imports logically
- Organizing holmes.core.tools imports for better readability
This improves code maintainability without affecting functionality.
Also applies to: 10-10, 20-21, 23-31
32-33: Good modularization: Constants moved to dedicated moduleMoving
TOOLSET_CONFIG_MISSING_ERRORfromutilsto the newconstsmodule improves code organization and aligns with the broader refactoring effort described in the PR objectives. This promotes better separation of concerns.holmes/plugins/toolsets/prometheus/prometheus.py (2)
1-1: LGTM: Well-organized import structureExcellent reorganization of imports by:
- Grouping standard library imports (
json,os,re, etc.) together- Consolidating third-party imports logically
- Organizing holmes.core.tools imports for better readability
This significantly improves code organization and maintainability.
Also applies to: 3-3, 5-5, 8-9, 13-14, 17-17, 20-20
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
24-24: Good modularization: Constants moved to dedicated moduleMoving
STANDARD_END_DATETIME_TOOL_PARAM_DESCRIPTIONfromutilsto the newconstsmodule while keeping utility functions inutilsdemonstrates excellent separation of concerns. This aligns with the broader refactoring effort described in the PR objectives.Also applies to: 29-29
🧰 Tools
🪛 Pylint (3.3.7)
[error] 24-24: Unable to import 'holmes.plugins.toolsets.consts'
(E0401)
holmes/core/tool_calling_llm.py (1)
5-34: LGTM! Excellent import reorganizationThe import statements have been well-organized by grouping related imports together and extracting
ResourceInstructionsandResourceInstructionDocumentto a dedicated module. This improves code modularity and maintainability.🧰 Tools
🪛 Pylint (3.3.7)
[error] 8-8: Unable to import 'sentry_sdk'
(E0401)
[error] 9-9: Unable to import 'litellm.types.utils'
(E0401)
[error] 10-10: Unable to import 'openai'
(E0401)
[error] 11-13: Unable to import 'openai.types.chat.chat_completion_message_tool_call'
(E0401)
[error] 14-14: Unable to import 'pydantic'
(E0401)
[error] 15-15: Unable to import 'pydantic_core'
(E0401)
holmes/core/supabase_dal.py (1)
7-33: LGTM! Import reorganization aligns with module extractionThe import reorganization correctly reflects the extraction of
ResourceInstructionDocumentandResourceInstructionsto the dedicatedholmes.core.resource_instructionmodule. The grouping of imports is also improved.🧰 Tools
🪛 Pylint (3.3.7)
[error] 12-12: Unable to import 'cachetools'
(E0401)
[error] 13-13: Unable to import 'postgrest._sync.request_builder'
(E0401)
[error] 14-14: Unable to import 'postgrest.exceptions'
(E0401)
[error] 15-15: Unable to import 'postgrest.types'
(E0401)
[error] 16-16: Unable to import 'pydantic'
(E0401)
[error] 17-17: Unable to import 'supabase'
(E0401)
[error] 18-18: Unable to import 'supabase.lib.client_options'
(E0401)
tests/plugins/toolsets/test_load_config.py (1)
71-94: LGTM! Comprehensive test coverageThe test properly verifies environment variable substitution in toolset configuration. The setup and teardown of environment variables is handled correctly to avoid test pollution.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 71-71: Missing function or method docstring
(C0116)
holmes/utils/env.py (3)
32-33: LGTM! Assignment issue has been resolved.The recursive call to
replace_env_vars_values(value)is correctly missing assignment since dictionaries are passed by reference and modified in-place. This is the correct implementation.
17-17: Good improvement: Using specific exception type.The change from generic
ExceptiontoValueErroris a good improvement over the previous implementation, making the error handling more specific and meaningful.
36-45: Well implemented list comprehension with proper variable naming.The list comprehension correctly uses
itemas the variable name instead of the problematiciterthat would shadow the built-in function. The logic properly handles nested dictionaries and environment variable replacement in lists.holmes/main.py (3)
1144-1161: Well-implemented toolset status display function.The
pretty_print_toolset_statusfunction is well-structured and provides a clear table format for displaying toolset information. The use ofmodel_dump_jsonwith specific field inclusion is appropriate for this purpose.🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1158-1158: Line too long (103/100)
(C0301)
[convention] 1144-1144: Missing function or method docstring
(C0116)
1115-1142: New toolset management commands are well designed.The new
listandrefreshcommands provide good functionality for managing toolsets. The commands properly initialize logging, load configuration, and handle the toolset manager integration. The separation of concerns between listing and refreshing is appropriate.
141-145: Configuration parameter type change is appropriate.The change from
typer.Option(None, ...)totyper.Option(DEFAULT_CONFIG_LOCATION, ...)for the config file parameter makes the default behavior more explicit and aligns with the centralized configuration management approach.🧰 Tools
🪛 Pylint (3.3.7)
[convention] 144-144: Line too long (151/100)
(C0301)
tests/core/test_toolset_manager.py (1)
1-278: Excellent comprehensive test coverage for ToolsetManager.This test file demonstrates excellent testing practices:
- Good test organization: Clear fixtures, parameterized tests, and logical grouping
- Proper mocking: Effective use of
@patchandMagicMockto isolate dependencies- Comprehensive scenarios: Tests cover success paths, error conditions, edge cases, and different input types
- Realistic test data: Uses temporary files and realistic configuration structures
- Clear assertions: Tests verify both positive and negative outcomes appropriately
The test coverage includes:
- Static method validation (
get_toolset_definition_enabled)- Internal methods (
_list_all_toolsets)- Caching functionality (
refresh_toolset_status,load_toolset_with_status)- Filtering methods (
list_enabled_console_toolsets,list_enabled_server_toolsets)- Custom toolset loading with various file conditions
- Merging and adding toolset logic
This provides strong confidence in the
ToolsetManagerimplementation.🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
[convention] 14-14: Missing function or method docstring
(C0116)
[convention] 30-30: Missing function or method docstring
(C0116)
[convention] 34-34: Missing function or method docstring
(C0116)
[warning] 34-34: Redefining name 'toolset_manager' from outer scope (line 14)
(W0621)
[convention] 40-40: Missing function or method docstring
(C0116)
[warning] 40-40: Redefining name 'toolset_manager' from outer scope (line 14)
(W0621)
[convention] 48-48: Missing function or method docstring
(C0116)
[warning] 49-49: Redefining name 'toolset_manager' from outer scope (line 14)
(W0621)
[warning] 63-63: Access to a protected member _list_all_toolsets of a client class
(W0212)
[convention] 70-70: Missing function or method docstring
(C0116)
[warning] 70-70: Redefining name 'toolset_manager' from outer scope (line 14)
(W0621)
[warning] 94-94: Using open without explicitly specifying an encoding
(W1514)
[convention] 100-100: Missing function or method docstring
(C0116)
[warning] 100-100: Redefining name 'toolset_manager' from outer scope (line 14)
(W0621)
[warning] 122-122: Using open without explicitly specifying an encoding
(W1514)
[convention] 131-131: Missing function or method docstring
(C0116)
[warning] 131-131: Redefining name 'toolset_manager' from outer scope (line 14)
(W0621)
[convention] 141-141: Missing function or method docstring
(C0116)
[warning] 141-141: Redefining name 'toolset_manager' from outer scope (line 14)
(W0621)
[convention] 151-151: Missing function or method docstring
(C0116)
[warning] 151-151: Redefining name 'toolset_manager' from outer scope (line 14)
(W0621)
[convention] 167-167: Missing function or method docstring
(C0116)
[warning] 168-168: Redefining name 'toolset_manager' from outer scope (line 14)
(W0621)
[warning] 168-168: Unused argument 'mock_benedict'
(W0613)
[warning] 168-168: Unused argument 'mock_load_toolsets_from_config'
(W0613)
[convention] 175-175: Missing function or method docstring
(C0116)
[warning] 175-175: Redefining name 'toolset_manager' from outer scope (line 14)
(W0621)
[convention] 181-181: Missing function or method docstring
(C0116)
[convention] 195-195: Missing function or method docstring
(C0116)
[convention] 203-203: Missing function or method docstring
(C0116)
[warning] 203-203: Redefining name 'toolset_manager' from outer scope (line 14)
(W0621)
[convention] 224-224: Missing function or method docstring
(C0116)
[warning] 224-224: Redefining name 'toolset_manager' from outer scope (line 14)
(W0621)
[convention] 247-247: Missing function or method docstring
(C0116)
[warning] 247-247: Redefining name 'toolset_manager' from outer scope (line 14)
(W0621)
[convention] 259-259: Missing function or method docstring
(C0116)
[warning] 259-259: Redefining name 'toolset_manager' from outer scope (line 14)
(W0621)
[convention] 269-269: Missing function or method docstring
(C0116)
[warning] 269-269: Redefining name 'toolset_manager' from outer scope (line 14)
(W0621)
holmes/core/tools.py (3)
97-99: LGTM! New ToolsetType enum values are well-defined.The addition of
BUILTINandCUSTOMIZEDenum values provides clear categorization for different toolset sources.
334-337: LGTM! Public attributes improve accessibility for ToolsetManager.The change from private attributes to public fields enables the ToolsetManager to directly access and cache toolset status information, which is a good design improvement.
518-549: LGTM! ToolsetYamlFromConfig provides flexible configuration override.This class design allows partial overrides of built-in toolsets without requiring all fields to be specified, which improves usability and configuration flexibility.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 523-523: Line too long (104/100)
(C0301)
holmes/config.py (3)
123-131: LGTM! Lazy initialization of ToolsetManager is well-implemented.The property pattern provides clean access to the ToolsetManager while avoiding unnecessary initialization.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 124-124: Missing function or method docstring
(C0116)
226-237: LGTM! Simplified tool executor creation using ToolsetManager.The delegation to
toolset_manager.list_enabled_console_toolsets()removes complex toolset filtering logic and centralizes it appropriately.🧰 Tools
🪛 Pylint (3.3.7)
[convention] 228-228: Line too long (107/100)
(C0301)
[convention] 233-233: Line too long (107/100)
(C0301)
1-12:❌ Incorrect review comment
Address the critical test failure with removed parse_toolsets_file.
Based on past review comments, tests are still trying to import
parse_toolsets_filewhich was removed from this module.
🏁 Script executed:
#!/bin/bash # Search for remaining references to parse_toolsets_file that need updating rg "parse_toolsets_file" --type py -A 2 -B 2Length of output: 44
No references to
parse_toolsets_filedetected – review comment no longer applies.
A ripgrep search across all.pyfiles returned zero occurrences ofparse_toolsets_file, so there are no imports or calls to update.Likely an incorrect or invalid review comment.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
[error] 10-10: Unable to import 'pydantic'
(E0401)
The fix is due to the missed error when we changed private attr in the Toolset Model. So it might raise more issue. Let me know if you bump into anything that need my help. |
This PR is ready for review. Please take another look. Thanks. |
|
Please ignore the conflicts lets focus on the PR change first. I will definitely resolve the conflicts after addressing all comments to reduce the chance of resolving conflicts. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
holmes/core/toolset_manager.py (3)
153-166: 🛠️ Refactor suggestionAdd error handling for file operations.
The file write operation could fail due to permission issues or disk space. Consider adding error handling:
- if self.toolset_status_location and not os.path.exists( - os.path.dirname(self.toolset_status_location) - ): - os.makedirs(os.path.dirname(self.toolset_status_location)) - with open(self.toolset_status_location, "w") as f: - toolset_status = [ - json.loads( - toolset.model_dump_json( - include={"name", "status", "enabled", "type", "path", "error"} - ) - ) - for toolset in all_toolsets - ] - json.dump(toolset_status, f, indent=2) + from holmes.utils.file_utils import write_json_file + + toolset_status = [ + json.loads( + toolset.model_dump_json( + include={"name", "status", "enabled", "type", "path", "error"} + ) + ) + for toolset in all_toolsets + ] + try: + write_json_file(self.toolset_status_location, toolset_status) + except Exception as e: + logging.error(f"Failed to write toolset status cache: {e}") + raise
182-184:⚠️ Potential issueAdd error handling for cache file loading.
The JSON loading could fail if the cache file is corrupted or has permission issues.
logging.info("loading toolset status from cache") cached_toolsets: List[dict[str, Any]] = [] - with open(self.toolset_status_location, "r") as f: - cached_toolsets = json.load(f) + try: + with open(self.toolset_status_location, "r") as f: + cached_toolsets = json.load(f) + except (json.JSONDecodeError, IOError) as e: + logging.error(f"Failed to load toolset cache: {e}. Refreshing status.") + self.refresh_toolset_status(dal) + with open(self.toolset_status_location, "r") as f: + cached_toolsets = json.load(f)
214-217:⚠️ Potential issueFix incorrect comparison between Toolset object and string keys.
The code is comparing a
Toolsetobject with dictionary string keys, which will always evaluate toFalse.for custom_toolset_from_cli in custom_toolsets_from_cli: - if custom_toolset_from_cli in toolsets_status_by_name.keys(): + if custom_toolset_from_cli.name in toolsets_status_by_name: raise ValueError( f"Toolset {custom_toolset_from_cli.name} from cli is already defined in existing toolset" )🧰 Tools
🪛 Ruff (0.11.9)
214-214: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
holmes/config.py (1)
163-171:⚠️ Potential issueFix uninitialized variable usage.
The variable
config_from_filemay not be initialized if the config file is None or doesn't exist, causing a NameError.""" Load configuration from file and merge with CLI options. Args: config_file: Path to configuration file **kwargs: CLI options to override config file values Returns: Config instance with merged settings """ - config_from_file: Optional[Config] = None + config_from_file = None if config_file is not None and config_file.exists(): logging.debug(f"Loading config from {config_file}") config_from_file = load_model_from_file(cls, config_file)
🧹 Nitpick comments (3)
holmes/core/toolset_manager.py (1)
360-360: Simplify dictionary membership check.Remove unnecessary
.keys()call for cleaner code.- if new_toolset.name in existing_toolsets_by_name.keys(): + if new_toolset.name in existing_toolsets_by_name:🧰 Tools
🪛 Ruff (0.11.9)
360-360: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
holmes/core/tools.py (1)
427-428: Simplify status comparison using the 'in' operator.The separate comparisons can be combined for better readability.
- if ( - self.status == ToolsetStatusEnum.DISABLED - or self.status == ToolsetStatusEnum.FAILED - ): + if self.status in (ToolsetStatusEnum.DISABLED, ToolsetStatusEnum.FAILED):🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 427-428: Consider merging these comparisons with 'in' by using 'self.status in (ToolsetStatusEnum.DISABLED, ToolsetStatusEnum.FAILED)'. Use a set instead if elements are hashable.
(R1714)
holmes/config.py (1)
165-165: Use lazy formatting in logging statements.Replace f-strings with lazy formatting for better performance.
- logging.debug(f"Loading config from {config_file}") + logging.debug("Loading config from %s", config_file)Also applies to: 173-173
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
holmes/config.py(10 hunks)holmes/core/tools.py(9 hunks)holmes/core/toolset_manager.py(1 hunks)holmes/main.py(20 hunks)pyproject.toml(1 hunks)tests/llm/utils/mock_toolset.py(4 hunks)tests/plugins/toolsets/test_prometheus_integration.py(2 hunks)tests/test_check_prerequisites.py(10 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/plugins/toolsets/test_prometheus_integration.py
🚧 Files skipped from review as they are similar to previous changes (4)
- pyproject.toml
- tests/test_check_prerequisites.py
- holmes/main.py
- tests/llm/utils/mock_toolset.py
🧰 Additional context used
🪛 Ruff (0.11.9)
holmes/core/toolset_manager.py
214-214: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
360-360: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
🪛 Pylint (3.3.7)
holmes/core/tools.py
[refactor] 427-428: Consider merging these comparisons with 'in' by using 'self.status in (ToolsetStatusEnum.DISABLED, ToolsetStatusEnum.FAILED)'. Use a set instead if elements are hashable.
(R1714)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build (3.12)
- GitHub Check: build (3.10)
- GitHub Check: build (3.11)
- GitHub Check: build (3.12)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
holmes/core/toolset_manager.py (4)
56-59: Fix incorrect docstring for server_tool_tags property.The docstring incorrectly refers to "UI tool tags" when it should mention "server tool tags".
153-166: Consider using the existing write_json_file utility or add error handling.The file writing operation lacks error handling. Consider using the existing
write_json_fileutility fromholmes.utils.file_utilswhich already has proper error handling.
183-184: Add error handling for cache file loading.The JSON loading could fail if the cache file is corrupted or has permission issues.
276-279: Improve exception chaining for better error traceability.Use proper exception chaining to preserve the original error context.
🧹 Nitpick comments (2)
holmes/core/toolset_manager.py (1)
360-360: Remove unnecessary.keys()call.When checking if a key exists in a dictionary, you don't need to call
.keys().- if new_toolset.name in existing_toolsets_by_name.keys(): + if new_toolset.name in existing_toolsets_by_name:🧰 Tools
🪛 Ruff (0.11.9)
360-360: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
holmes/config.py (1)
166-166: Use lazy % formatting in logging functions.Replace f-string logging with lazy formatting to avoid unnecessary string interpolation when the log level is higher than debug.
- logging.debug(f"Loading config from {config_file}") + logging.debug("Loading config from %s", config_file)Also applies to: 174-174
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
holmes/config.py(10 hunks)holmes/core/toolset_manager.py(1 hunks)holmes/main.py(20 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- holmes/main.py
🧰 Additional context used
🪛 Ruff (0.11.9)
holmes/core/toolset_manager.py
360-360: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build (3.12)
- GitHub Check: build (3.10)
- GitHub Check: build (3.11)
- GitHub Check: build (3.12)
🔇 Additional comments (1)
holmes/config.py (1)
228-239: Well-structured refactoring to use ToolsetManager.The method has been nicely simplified by delegating toolset management to the dedicated
ToolsetManagerclass. The documentation clearly explains the loading order.
|
@moshemorad could you please help find the root cause of llm evaluation error. The following error message raised from the pipeline seems to be common issue? Thanks. |
Looks like for some reason it isn't adding the OpenAI key, trying to understand why. |
|
@mainred Looks like the issue is due to forked repos don't use the original repo secrets. @arikalon1 @nherment any idea what we want to do in those cases? |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
holmes/plugins/toolsets/__init__.py (2)
41-41: Specify encoding when opening filesAlways specify the encoding when opening text files to ensure consistent behavior across different platforms.
- with open(toolsets_path) as file: + with open(toolsets_path, encoding="utf-8") as file:
131-149:⚠️ Potential issueFix critical validation logic issues
The validation logic has several critical issues that make it broken:
- Line 134: Comparing string value with enum using
isoperator will always be False- Line 134: The MCP check executes outside the strict_check block, causing validation inconsistency
- Line 138: The strict_check block will always execute after the MCP check
- Type annotations are inconsistent and cause type checking errors
Apply this diff to fix the validation logic:
try: toolset_type = config.get("type", ToolsetType.BUILTIN.value) - # MCP server is not a built-in toolset, so we need to set the type explicitly - if toolset_type is ToolsetType.MCP: - validated_toolset: RemoteMCPToolset = RemoteMCPToolset( - **config, name=name - ) if strict_check: + if toolset_type == ToolsetType.MCP.value: + validated_toolset = RemoteMCPToolset(**config, name=name) + else: - validated_toolset: YAMLToolset = YAMLToolset(**config, name=name) # type: ignore + validated_toolset = YAMLToolset(**config, name=name) else: - validated_toolset: ToolsetYamlFromConfig = ToolsetYamlFromConfig( # type: ignore - **config, name=name - ) + validated_toolset = ToolsetYamlFromConfig(**config, name=name)holmes/main.py (1)
1117-1144: Fix typo in command help textThere's a typo in the help text that was flagged in previous reviews.
""" - List build-in and custom toolsets status of CLI + List built-in and custom toolsets status of CLI """Also apply the same fix to the refresh command:
""" - Refresh build-in and custom toolsets status of CLI + Refresh built-in and custom toolsets status of CLI """
🧹 Nitpick comments (1)
holmes/plugins/toolsets/__init__.py (1)
105-107: Simplify boolean return statementThe function can return the condition directly instead of using an if-else block.
-def is_old_toolset_config( - toolsets: Union[dict[str, dict[str, Any]], List[dict[str, Any]]], -) -> bool: - # old config is a list of toolsets - if isinstance(toolsets, list): - return True - return False +def is_old_toolset_config( + toolsets: Union[dict[str, dict[str, Any]], List[dict[str, Any]]], +) -> bool: + # old config is a list of toolsets + return isinstance(toolsets, list)🧰 Tools
🪛 Ruff (0.11.9)
105-107: Return the condition
bool(isinstance(toolsets, list))directlyReplace with
return bool(isinstance(toolsets, list))(SIM103)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
holmes/main.py(20 hunks)holmes/plugins/toolsets/__init__.py(2 hunks)holmes/utils/env.py(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- holmes/utils/env.py
🧰 Additional context used
🪛 Ruff (0.11.9)
holmes/plugins/toolsets/__init__.py
105-107: Return the condition bool(isinstance(toolsets, list)) directly
Replace with return bool(isinstance(toolsets, list))
(SIM103)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build (3.12)
- GitHub Check: build (3.10)
- GitHub Check: build (3.11)
- GitHub Check: build (3.12)
🔇 Additional comments (3)
holmes/main.py (3)
6-6: LGTM: Appropriate type ignore for external libraryThe
# type: ignorecomment is appropriate for thetabulateexternal library import.
64-70: LGTM: Well-structured CLI group additionThe new toolset management CLI group is properly structured and integrated with the main app.
1146-1166: LGTM: Well-implemented table formatting functionThe
pretty_print_toolset_statusfunction correctly uses OrderedDict to maintain field order and tabulate for clean table output. The JSON serialization approach for field filtering is appropriate.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
holmes/plugins/toolsets/__init__.py (2)
41-41: Specify encoding when opening filesAlways specify the encoding when opening text files to ensure consistent behavior across different platforms.
- with open(toolsets_path) as file: + with open(toolsets_path, encoding="utf-8") as file:
131-142:⚠️ Potential issueFix validation logic and type comparison issues
The validation logic has several critical issues:
- Line 133 uses
isto compare with enum, buttoolset_typeis a string from the config- The MCP toolset validation happens outside the strict_check block
- The same variable
validated_toolsetis assigned different typestoolset_type = config.get("type", ToolsetType.BUILTIN.value) # MCP server is not a built-in toolset, so we need to set the type explicitly - if toolset_type is ToolsetType.MCP: - validated_toolset: RemoteMCPToolset = RemoteMCPToolset( - **config, name=name - ) if strict_check: - validated_toolset: YAMLToolset = YAMLToolset(**config, name=name) # type: ignore + if toolset_type == ToolsetType.MCP.value: + validated_toolset = RemoteMCPToolset(**config, name=name) + else: + validated_toolset = YAMLToolset(**config, name=name) else: - validated_toolset: ToolsetYamlFromConfig = ToolsetYamlFromConfig( # type: ignore - **config, name=name - ) + validated_toolset = ToolsetYamlFromConfig(**config, name=name)holmes/core/toolset_manager.py (3)
56-59: Fix incorrect docstring referenceThe docstring incorrectly refers to "UI tool tags" when it should mention "server tool tags".
""" Returns the list of toolset tags that are relevant for server tools. - A toolset is considered a server tool if it has any of UI tool tags: + A toolset is considered a server tool if it has any of server tool tags. """
213-215:⚠️ Potential issueAdd error handling for cache file loading
The JSON loading could fail if the cache file is corrupted or has permission issues.
logging.info("loading toolset status from cache") cached_toolsets: List[dict[str, Any]] = [] - with open(self.toolset_status_location, "r") as f: - cached_toolsets = json.load(f) + try: + with open(self.toolset_status_location, "r") as f: + cached_toolsets = json.load(f) + except (json.JSONDecodeError, IOError) as e: + logging.error(f"Failed to load toolset cache: {e}. Refreshing status.") + self.refresh_toolset_status(dal, enable_all_toolsets=enable_all_toolsets, toolset_tags=toolset_tags) + with open(self.toolset_status_location, "r") as f: + cached_toolsets = json.load(f)
246-248: 🛠️ Refactor suggestionUse more specific exception type
The generic
Exceptionis too broad. Use a more specific exception type for better error handling.- raise ValueError( + raise ValueError( f"Toolset {custom_toolset_from_cli.name} from cli is already defined in existing toolset" )
🧹 Nitpick comments (4)
holmes/plugins/toolsets/__init__.py (1)
104-106: Simplify return statementThe condition can be returned directly without the if statement.
- if isinstance(toolsets, list): - return True - return False + return isinstance(toolsets, list)🧰 Tools
🪛 Ruff (0.11.9)
104-106: Return the condition
bool(isinstance(toolsets, list))directlyReplace with
return bool(isinstance(toolsets, list))(SIM103)
holmes/core/toolset_manager.py (1)
389-389: Remove unnecessary .keys() callWhen checking membership in a dictionary,
.keys()is redundant.- if new_toolset.name in existing_toolsets_by_name.keys(): + if new_toolset.name in existing_toolsets_by_name:🧰 Tools
🪛 Ruff (0.11.9)
389-389: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
holmes/core/tools.py (1)
427-428: Simplify status check using 'in' operatorUse the
inoperator for cleaner and more readable code.- if ( - self.status == ToolsetStatusEnum.DISABLED - or self.status == ToolsetStatusEnum.FAILED - ): + if self.status in (ToolsetStatusEnum.DISABLED, ToolsetStatusEnum.FAILED):🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 427-428: Consider merging these comparisons with 'in' by using 'self.status in (ToolsetStatusEnum.DISABLED, ToolsetStatusEnum.FAILED)'. Use a set instead if elements are hashable.
(R1714)
holmes/config.py (1)
166-166: Use lazy formatting in logging functionsReplace f-strings with lazy formatting to avoid unnecessary string interpolation when the log level is higher than debug.
- logging.debug(f"Loading config from {config_file}") + logging.debug("Loading config from %s", config_file)Also applies to: 174-174
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
holmes/config.py(10 hunks)holmes/core/tools.py(9 hunks)holmes/core/toolset_manager.py(1 hunks)holmes/main.py(20 hunks)holmes/plugins/toolsets/__init__.py(2 hunks)tests/core/test_toolset_manager.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/core/test_toolset_manager.py
- holmes/main.py
🧰 Additional context used
🪛 Ruff (0.11.9)
holmes/plugins/toolsets/__init__.py
104-106: Return the condition bool(isinstance(toolsets, list)) directly
Replace with return bool(isinstance(toolsets, list))
(SIM103)
holmes/core/toolset_manager.py
389-389: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
🪛 Pylint (3.3.7)
holmes/core/tools.py
[refactor] 427-428: Consider merging these comparisons with 'in' by using 'self.status in (ToolsetStatusEnum.DISABLED, ToolsetStatusEnum.FAILED)'. Use a set instead if elements are hashable.
(R1714)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build (3.11)
- GitHub Check: build (3.12)
- GitHub Check: build (3.10)
- GitHub Check: build (3.12)
🔇 Additional comments (6)
holmes/core/toolset_manager.py (1)
34-36: Good handling of default parameterThe implementation correctly avoids function calls in default parameters by setting the default inside the method body.
holmes/core/tools.py (2)
334-337: Good refactoring to public fieldsThe change from private attributes to public fields simplifies the API and removes unnecessary getters/setters, making the code more Pythonic and easier to use.
508-539: Well-designed class for partial toolset configurationsThe
ToolsetYamlFromConfigclass effectively handles partial YAML configurations for overriding built-in toolsets. The documentation clearly explains its purpose and the default values are appropriate.holmes/config.py (3)
125-133: Excellent use of lazy initialization patternThe
toolset_managerproperty implements lazy initialization effectively, ensuring theToolsetManageris only created when needed. This improves performance and properly delegates all toolset management responsibilities.
228-239: Clean refactoring of tool executor creationThe simplified implementation effectively delegates all toolset management to
ToolsetManager, removing complex loading and merging logic. This makes the code much more maintainable and follows the single responsibility principle.Also applies to: 241-257
425-425: Good type safety improvementChanging
config_fileparameter fromstrtoOptional[Path]improves type safety and aligns with the broader refactoring to usePathobjects throughout the codebase.
fix: #426
fix: #424
mcptoolset type, this PR also introducesbuilt-inandcustomizedtoolset type to differentiate the source of these toolsets