feat: Support llm-based message summarization by introducing Transformer mechanism - #695
Conversation
|
|
WalkthroughAdds a transformer subsystem (models, registry, llm_summarize), integrates transformers into Tool/Toolset execution with StructuredToolResult and caching, propagates a global fast_model from CLI/ENV/Config into llm_summarize configs, updates plugin YAMLs to declare llm_summarize, adds docs and examples, and introduces extensive unit and integration tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI as CLI (holmes/main.py)
participant Config as Config
participant TSM as ToolsetManager
participant TS as Toolset(s)
participant Tool as Tool
participant TF as Transformer(s)
participant LLM as Fast LLM (optional)
User->>CLI: ask ... --fast-model X
CLI->>Config: load_from_file(..., fast_model=X)
Config->>TSM: instantiate(global_fast_model=X)
TSM->>TS: load toolsets
TSM->>TS: inject global_fast_model into llm_summarize configs
User->>Tool: invoke()
Tool->>Tool: run command -> StructuredToolResult
Tool->>TF: _apply_transformers(result.data)
alt transformers apply
TF->>TF: should_apply?
alt llm_summarize applies
TF->>LLM: completion(prompt + input)
LLM-->>TF: summary
TF-->>Tool: transformed text
else other transformer / skip / error
TF-->>Tool: original or handled error
end
Tool-->>User: StructuredToolResult (transformed)
else none
Tool-->>User: StructuredToolResult (original)
end
sequenceDiagram
autonumber
participant Source as Env/CLI/File
participant Config
participant TSM as ToolsetManager
participant TS as Toolset(s)
Source->>Config: fast_model resolved (env/cli/file)
Config->>TSM: ToolsetManager(global_fast_model)
TSM->>TS: load toolsets
TSM->>TS: inject global_fast_model into llm_summarize configs (toolset & tools)
TS-->>TSM: updated toolsets
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🔭 Outside diff range comments (1)
tests/core/transformers/test_validation.py (1)
309-323: Fix incorrect error message assertion in validation test.The test creates a config with
invalid_param="invalid"but checks for "required_param is missing" error. Based on the MockConfigTransformer implementation, this should check for "invalid_param has invalid value".Apply this fix:
# Should contain underlying error details from the validation chain error_str = str(exc_info.value) - assert "required_param is missing" in error_str + assert "invalid_param has invalid value" in error_str
🧹 Nitpick comments (15)
holmes/plugins/toolsets/aks-node-health.yaml (1)
10-10: Fix trailing spaces.Static analysis detected trailing spaces on this line.
- +holmes/core/transformers/registry.py (1)
109-110: Global registry instance pattern.While the singleton pattern is convenient, be aware that it makes testing more difficult as state persists between tests. Consider documenting that tests should call
registry.clear()in teardown.tests/core/test_transformer_backwards_compatibility.py (1)
94-97: Combine nestedwithstatements.For cleaner code, combine the nested context managers.
Apply this diff:
- with patch("holmes.config.load_model_from_file") as mock_load: - mock_load.return_value = Config(**mock_config_content) - - with patch("pathlib.Path.exists") as mock_exists: - mock_exists.return_value = True + with ( + patch("holmes.config.load_model_from_file") as mock_load, + patch("pathlib.Path.exists") as mock_exists + ): + mock_load.return_value = Config(**mock_config_content) + mock_exists.return_value = Truetests/core/transformers/test_llm_summarize.py (2)
15-27: Consider extracting the mock helper to reduce code duplication.The
create_mock_llmmethod is duplicated between the unit test and integration test classes. Consider extracting this to a module-level helper function or a base test class to follow the DRY principle.+def create_mock_llm(response_content: str = "Summarized content"): + """Create a mock LLM that returns the specified response.""" + mock_llm = Mock() + mock_response = Mock() + mock_choice = Mock() + mock_message = Mock() + + mock_message.content = response_content + mock_choice.message = mock_message + mock_response.choices = [mock_choice] + mock_llm.completion.return_value = mock_response + + return mock_llm + class TestLLMSummarizeTransformer: """Test cases for LLMSummarizeTransformer class.""" - def create_mock_llm(self, response_content: str = "Summarized content"): - """Create a mock LLM that returns the specified response.""" - mock_llm = Mock() - mock_response = Mock() - mock_choice = Mock() - mock_message = Mock() - - mock_message.content = response_content - mock_choice.message = mock_message - mock_response.choices = [mock_choice] - mock_llm.completion.return_value = mock_response - - return mock_llm + def create_mock_llm(self, response_content: str = "Summarized content"): + """Create a mock LLM that returns the specified response.""" + return create_mock_llm(response_content)
405-405: Fix type comparison as suggested by static analysis.The static analysis tool correctly identifies that using
==for type comparison should be replaced withisinstance()for better type checking practices.- assert exc_info.value.__cause__.__class__ == ConnectionError + assert isinstance(exc_info.value.__cause__, ConnectionError)tests/plugins/toolsets/test_aks_transformers.py (2)
14-25: Reduce path construction duplication.The YAML file path construction is repeated multiple times throughout the test file. Consider extracting this to helper methods or class-level fixtures to improve maintainability.
+ @classmethod + def get_aks_node_health_yaml_path(cls): + """Get path to aks-node-health.yaml file.""" + current_dir = os.path.dirname(os.path.abspath(__file__)) + return os.path.join( + current_dir, "..", "..", "..", "holmes", "plugins", "toolsets", "aks-node-health.yaml" + ) + + @classmethod + def get_aks_yaml_path(cls): + """Get path to aks.yaml file.""" + current_dir = os.path.dirname(os.path.abspath(__file__)) + return os.path.join( + current_dir, "..", "..", "..", "holmes", "plugins", "toolsets", "aks.yaml" + ) def test_load_aks_node_health_yaml_with_transformers(self): """Test loading the aks-node-health.yaml file with transformer configs.""" - # Find the actual aks-node-health.yaml file - current_dir = os.path.dirname(os.path.abspath(__file__)) - aks_node_health_yaml_path = os.path.join( - current_dir, - "..", - "..", - "..", - "holmes", - "plugins", - "toolsets", - "aks-node-health.yaml", - ) + aks_node_health_yaml_path = self.get_aks_node_health_yaml_path()Also applies to: 74-77, 128-138, 164-166, 187-196, 221-223
30-44: Consider extracting tool finding logic to helper methods.The pattern of iterating through toolsets and tools to find specific named entities is repeated. Consider creating helper methods to reduce code duplication and improve readability.
+ def find_toolset_by_name(self, toolsets, name): + """Find toolset by name in the provided list.""" + for toolset in toolsets: + if toolset.name == name: + return toolset + return None + + def find_tool_by_name(self, toolset, name): + """Find tool by name in the provided toolset.""" + for tool in toolset.tools: + if tool.name == name: + return tool + return None # Find the aks/node-health toolset - aks_node_health = None - for toolset in toolsets: - if toolset.name == "aks/node-health": - aks_node_health = toolset - break + aks_node_health = self.find_toolset_by_name(toolsets, "aks/node-health")Also applies to: 56-69, 91-105, 107-123
tests/integration/test_tool_execution_pipeline.py (2)
128-170: Effective test for transformer failure recovery.The test properly validates that tool execution continues when a transformer fails and that subsequent transformers are still applied. Good use of the finally block for cleanup.
Consider moving the inline comment to a separate line for better readability:
- }, # This should still work + }, + # This should still work
265-265: Remove unnecessary f-string.The kubectl_output is already a string, so the f-string is redundant.
- command=f"echo '{kubectl_output}'", + command=f"echo {kubectl_output!r}",tests/config_class/test_config_transformers.py (1)
18-23: Consider extracting common mocking setup.The mocking pattern is repeated in every test. Consider using a pytest fixture to reduce duplication.
@pytest.fixture def mock_config_dependencies(): """Common mocking setup for Config tests.""" with patch("holmes.__init__.get_version", return_value="1.0.0"), \ patch("holmes.clients.robusta_client.fetch_holmes_info", return_value=None), \ patch("holmes.config.parse_models_file", return_value={}), \ patch("holmes.common.env_vars.ROBUSTA_AI", False): yield def test_transformer_config_fields_exist(mock_config_dependencies): """Test that Config class has the new transformer configuration fields.""" from holmes.config import Config # ... rest of testholmes/core/transformers/validation.py (1)
69-72: Use consistent terminology in error messages.The parameter is named
transformer_configsbut the error message says "Transforms".raise TransformerValidationError( - "Transforms must be a list of transformer configurations" + "transformer_configs must be a list of transformer configurations" )tests/integration/test_config_merging_integration.py (2)
131-131: Remove unnecessary parentheses in assert statement.- assert(specific_tool.transformer_configs) is not None + assert specific_tool.transformer_configs is not None
149-149: Remove unnecessary parentheses in assert statement.- assert(generic_tool.transformer_configs) is not None + assert generic_tool.transformer_configs is not Nonetests/plugins/toolsets/test_kubernetes_transformers.py (2)
17-28: Consider using pathlib for more robust path construction.The current path construction using multiple
os.path.joincalls with ".." is fragile and could break if the test file location changes. Consider usingpathlib.Pathfor more robust path handling.-# Find the actual kubernetes.yaml file -current_dir = os.path.dirname(os.path.abspath(__file__)) -kubernetes_yaml_path = os.path.join( - current_dir, - "..", - "..", - "..", - "holmes", - "plugins", - "toolsets", - "kubernetes.yaml", -) +# Find the actual kubernetes.yaml file +from pathlib import Path +current_file = Path(__file__) +kubernetes_yaml_path = current_file.parent.parent.parent.parent / "holmes" / "plugins" / "toolsets" / "kubernetes.yaml"
33-50: Extract repetitive toolset and tool finding logic into helper methods.The pattern of iterating through lists to find toolsets and tools by name is repeated multiple times. Consider extracting these into helper methods to reduce duplication.
Add these helper methods to the test class:
def _find_toolset(self, toolsets: List, name: str): """Find a toolset by name.""" for toolset in toolsets: if toolset.name == name: return toolset return None def _find_tool(self, toolset, name: str): """Find a tool within a toolset by name.""" for tool in toolset.tools: if tool.name == name: return tool return NoneThen use them:
-# Find the kubernetes/core toolset -kubernetes_core = None -for toolset in toolsets: - if toolset.name == "kubernetes/core": - kubernetes_core = toolset - break - -assert kubernetes_core is not None, "kubernetes/core toolset not found" - -# Test kubectl_describe has transformer config -kubectl_describe = None -for tool in kubernetes_core.tools: - if tool.name == "kubectl_describe": - kubectl_describe = tool - break - -assert kubectl_describe is not None, "kubectl_describe tool not found" +# Find the kubernetes/core toolset +kubernetes_core = self._find_toolset(toolsets, "kubernetes/core") +assert kubernetes_core is not None, "kubernetes/core toolset not found" + +# Test kubectl_describe has transformer config +kubectl_describe = self._find_tool(kubernetes_core, "kubectl_describe") +assert kubectl_describe is not None, "kubectl_describe tool not found"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between be51687 and 565465513ed6d2eafa9f2022356d44aea42e0681.
📒 Files selected for processing (32)
README.md(1 hunks)config.example.yaml(1 hunks)docs/transformers.md(1 hunks)holmes/config.py(5 hunks)holmes/core/tools.py(5 hunks)holmes/core/toolset_manager.py(6 hunks)holmes/core/transformers/__init__.py(1 hunks)holmes/core/transformers/base.py(1 hunks)holmes/core/transformers/llm_summarize.py(1 hunks)holmes/core/transformers/registry.py(1 hunks)holmes/core/transformers/validation.py(1 hunks)holmes/main.py(3 hunks)holmes/plugins/toolsets/aks-node-health.yaml(3 hunks)holmes/plugins/toolsets/aks.yaml(3 hunks)holmes/plugins/toolsets/kubernetes.yaml(3 hunks)holmes/plugins/toolsets/kubernetes_logs.yaml(2 hunks)holmes/utils/config_utils.py(1 hunks)tests/config_class/test_config_transformers.py(1 hunks)tests/core/test_config_transformers.py(1 hunks)tests/core/test_tool_transformers.py(1 hunks)tests/core/test_toolset_manager.py(1 hunks)tests/core/test_transformer_backwards_compatibility.py(1 hunks)tests/core/transformers/__init__.py(1 hunks)tests/core/transformers/test_llm_summarize.py(1 hunks)tests/core/transformers/test_transformers.py(1 hunks)tests/core/transformers/test_validation.py(1 hunks)tests/integration/test_config_merging_integration.py(1 hunks)tests/integration/test_kubernetes_transformer_execution.py(1 hunks)tests/integration/test_tool_execution_pipeline.py(1 hunks)tests/plugins/toolsets/test_aks_transformers.py(1 hunks)tests/plugins/toolsets/test_kubernetes_transformers.py(1 hunks)tests/utils/test_config_utils.py(1 hunks)
🧠 Learnings (14)
README.md (3)
Learnt from: nherment
PR: #610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025-07-08T08:45:41.069Z
Learning: The robusta-dev/holmesgpt codebase has comprehensive existing validation for Azure environment variables (AZURE_API_BASE, AZURE_API_KEY, AZURE_API_VERSION) and MODEL in tests/llm/utils/classifiers.py, tests/llm/conftest.py, and holmes/core/llm.py. Don't suggest adding redundant validation logic.
Learnt from: Sheeproid
PR: #586
File: tests/llm/fixtures/test_ask_holmes/03_what_is_the_command_to_port_forward/test_case.yaml:4-4
Timestamp: 2025-07-02T10:27:17.231Z
Learning: In LLM-as-judge test cases for HolmesGPT, expected outputs should be descriptive rather than prescriptive when testing for flexible responses like port numbers. Using specific values in expected outputs can cause unnecessary test failures when the AI generates different but equally valid responses.
Learnt from: nherment
PR: #408
File: holmes/plugins/toolsets/kubernetes_logs.py:90-97
Timestamp: 2025-05-15T05:13:43.169Z
Learning: In the Kubernetes logs toolset for Holmes, both current and previous logs are intentionally fetched and combined for each pod, even though this requires more API calls. This design ensures all logs are captured even when pods restart but retain their name, providing complete diagnostic information.
holmes/plugins/toolsets/kubernetes_logs.yaml (2)
Learnt from: nherment
PR: #408
File: holmes/plugins/toolsets/kubernetes_logs.py:90-97
Timestamp: 2025-05-15T05:13:43.169Z
Learning: In the Kubernetes logs toolset for Holmes, both current and previous logs are intentionally fetched and combined for each pod, even though this requires more API calls. This design ensures all logs are captured even when pods restart but retain their name, providing complete diagnostic information.
Learnt from: nherment
PR: #408
File: holmes/plugins/toolsets/kubernetes_logs.py:100-102
Timestamp: 2025-05-15T05:14:06.519Z
Learning: The fetch_logs method in KubernetesLogsToolset is designed to apply the limit parameter after filtering and combining both current and previous logs, rather than using the API's tail_lines parameter, to ensure the limit applies to the final combined log set.
holmes/plugins/toolsets/kubernetes.yaml (1)
Learnt from: nherment
PR: #408
File: holmes/plugins/toolsets/kubernetes_logs.py:90-97
Timestamp: 2025-05-15T05:13:43.169Z
Learning: In the Kubernetes logs toolset for Holmes, both current and previous logs are intentionally fetched and combined for each pod, even though this requires more API calls. This design ensures all logs are captured even when pods restart but retain their name, providing complete diagnostic information.
tests/core/test_transformer_backwards_compatibility.py (1)
Learnt from: nherment
PR: #535
File: holmes/plugins/toolsets/bash/bash_toolset.py:207-209
Timestamp: 2025-06-24T05:51:04.543Z
Learning: The init_config method in toolsets should be idempotent - safely callable multiple times without errors. self.config should maintain consistent typing (not alternate between dict and config object types) throughout the object lifecycle.
holmes/core/toolset_manager.py (1)
Learnt from: nherment
PR: #535
File: holmes/plugins/toolsets/bash/bash_toolset.py:207-209
Timestamp: 2025-06-24T05:51:04.543Z
Learning: The init_config method in toolsets should be idempotent - safely callable multiple times without errors. self.config should maintain consistent typing (not alternate between dict and config object types) throughout the object lifecycle.
tests/core/transformers/test_llm_summarize.py (1)
Learnt from: Sheeproid
PR: #586
File: tests/llm/fixtures/test_ask_holmes/03_what_is_the_command_to_port_forward/test_case.yaml:4-4
Timestamp: 2025-07-02T10:27:17.231Z
Learning: In LLM-as-judge test cases for HolmesGPT, expected outputs should be descriptive rather than prescriptive when testing for flexible responses like port numbers. Using specific values in expected outputs can cause unnecessary test failures when the AI generates different but equally valid responses.
tests/config_class/test_config_transformers.py (1)
Learnt from: nherment
PR: #610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025-07-08T08:45:41.069Z
Learning: The robusta-dev/holmesgpt codebase has comprehensive existing validation for Azure environment variables (AZURE_API_BASE, AZURE_API_KEY, AZURE_API_VERSION) and MODEL in tests/llm/utils/classifiers.py, tests/llm/conftest.py, and holmes/core/llm.py. Don't suggest adding redundant validation logic.
tests/integration/test_tool_execution_pipeline.py (1)
Learnt from: Sheeproid
PR: #586
File: tests/llm/fixtures/test_ask_holmes/03_what_is_the_command_to_port_forward/test_case.yaml:4-4
Timestamp: 2025-07-02T10:27:17.231Z
Learning: In LLM-as-judge test cases for HolmesGPT, expected outputs should be descriptive rather than prescriptive when testing for flexible responses like port numbers. Using specific values in expected outputs can cause unnecessary test failures when the AI generates different but equally valid responses.
tests/core/test_config_transformers.py (1)
Learnt from: nherment
PR: #535
File: holmes/plugins/toolsets/bash/bash_toolset.py:207-209
Timestamp: 2025-06-24T05:51:04.543Z
Learning: The init_config method in toolsets should be idempotent - safely callable multiple times without errors. self.config should maintain consistent typing (not alternate between dict and config object types) throughout the object lifecycle.
holmes/core/transformers/validation.py (2)
Learnt from: nherment
PR: #610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025-07-08T08:45:41.069Z
Learning: The robusta-dev/holmesgpt codebase has comprehensive existing validation for Azure environment variables (AZURE_API_BASE, AZURE_API_KEY, AZURE_API_VERSION) and MODEL in tests/llm/utils/classifiers.py, tests/llm/conftest.py, and holmes/core/llm.py. Don't suggest adding redundant validation logic.
Learnt from: nherment
PR: #610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025-07-08T08:45:41.069Z
Learning: When suggesting improvements to environment variable handling in robusta-dev/holmesgpt, check first if validation logic already exists rather than reimplementing it.
tests/core/transformers/test_validation.py (1)
Learnt from: nherment
PR: #610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025-07-08T08:45:41.069Z
Learning: The robusta-dev/holmesgpt codebase has comprehensive existing validation for Azure environment variables (AZURE_API_BASE, AZURE_API_KEY, AZURE_API_VERSION) and MODEL in tests/llm/utils/classifiers.py, tests/llm/conftest.py, and holmes/core/llm.py. Don't suggest adding redundant validation logic.
holmes/config.py (1)
Learnt from: nherment
PR: #610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025-07-08T08:45:41.069Z
Learning: The robusta-dev/holmesgpt codebase has comprehensive existing validation for Azure environment variables (AZURE_API_BASE, AZURE_API_KEY, AZURE_API_VERSION) and MODEL in tests/llm/utils/classifiers.py, tests/llm/conftest.py, and holmes/core/llm.py. Don't suggest adding redundant validation logic.
tests/integration/test_config_merging_integration.py (1)
Learnt from: nherment
PR: #535
File: holmes/plugins/toolsets/bash/bash_toolset.py:207-209
Timestamp: 2025-06-24T05:51:04.543Z
Learning: The init_config method in toolsets should be idempotent - safely callable multiple times without errors. self.config should maintain consistent typing (not alternate between dict and config object types) throughout the object lifecycle.
tests/plugins/toolsets/test_kubernetes_transformers.py (1)
Learnt from: Sheeproid
PR: #586
File: tests/llm/fixtures/test_ask_holmes/03_what_is_the_command_to_port_forward/test_case.yaml:4-4
Timestamp: 2025-07-02T10:27:17.231Z
Learning: In LLM-as-judge test cases for HolmesGPT, expected outputs should be descriptive rather than prescriptive when testing for flexible responses like port numbers. Using specific values in expected outputs can cause unnecessary test failures when the AI generates different but equally valid responses.
🧬 Code Graph Analysis (5)
holmes/core/transformers/__init__.py (4)
holmes/core/transformers/base.py (2)
BaseTransformer(17-84)TransformerError(11-14)holmes/core/transformers/registry.py (2)
TransformerRegistry(9-106)register(20-39)holmes/core/transformers/llm_summarize.py (1)
LLMSummarizeTransformer(14-157)holmes/core/transformers/validation.py (5)
TransformerValidationError(13-16)validate_transformer_config(19-56)validate_transformer_configs(59-80)validate_tool_transformer_configs(83-105)safe_validate_tool_transformer_configs(108-126)
holmes/core/transformers/registry.py (1)
holmes/core/transformers/base.py (3)
BaseTransformer(17-84)TransformerError(11-14)name(77-84)
holmes/core/transformers/validation.py (1)
holmes/core/transformers/registry.py (3)
is_registered(83-93)list_transformers(95-102)create_transformer(56-81)
tests/plugins/toolsets/test_aks_transformers.py (3)
holmes/plugins/toolsets/__init__.py (1)
load_toolsets_from_file(41-55)holmes/core/transformers/llm_summarize.py (1)
name(155-157)holmes/core/transformers/base.py (1)
name(77-84)
tests/utils/test_config_utils.py (1)
holmes/utils/config_utils.py (1)
merge_transformer_configs(8-69)
🪛 YAMLlint (1.37.1)
holmes/plugins/toolsets/aks-node-health.yaml
[error] 10-10: trailing spaces
(trailing-spaces)
holmes/plugins/toolsets/aks.yaml
[error] 10-10: trailing spaces
(trailing-spaces)
🪛 Ruff (0.12.2)
tests/integration/test_kubernetes_transformer_execution.py
509-509: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
tests/core/test_transformer_backwards_compatibility.py
94-97: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
holmes/core/transformers/base.py
64-74: BaseTransformer._validate_config is an empty method in an abstract base class, but has no abstract decorator
(B027)
tests/core/transformers/test_transformers.py
25-29: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
tests/core/test_tool_transformers.py
656-656: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
tests/core/transformers/test_llm_summarize.py
405-405: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
holmes/core/tools.py
145-149: Use a single if statement instead of nested if statements
(SIM102)
235-235: Local variable size_change is assigned to but never used
Remove assignment to unused variable size_change
(F841)
🪛 markdownlint-cli2 (0.17.2)
docs/transformers.md
119-119: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
151-151: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
161-161: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
171-171: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
181-181: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🧰 Additional context used
🧠 Learnings (14)
README.md (3)
Learnt from: nherment
PR: #610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025-07-08T08:45:41.069Z
Learning: The robusta-dev/holmesgpt codebase has comprehensive existing validation for Azure environment variables (AZURE_API_BASE, AZURE_API_KEY, AZURE_API_VERSION) and MODEL in tests/llm/utils/classifiers.py, tests/llm/conftest.py, and holmes/core/llm.py. Don't suggest adding redundant validation logic.
Learnt from: Sheeproid
PR: #586
File: tests/llm/fixtures/test_ask_holmes/03_what_is_the_command_to_port_forward/test_case.yaml:4-4
Timestamp: 2025-07-02T10:27:17.231Z
Learning: In LLM-as-judge test cases for HolmesGPT, expected outputs should be descriptive rather than prescriptive when testing for flexible responses like port numbers. Using specific values in expected outputs can cause unnecessary test failures when the AI generates different but equally valid responses.
Learnt from: nherment
PR: #408
File: holmes/plugins/toolsets/kubernetes_logs.py:90-97
Timestamp: 2025-05-15T05:13:43.169Z
Learning: In the Kubernetes logs toolset for Holmes, both current and previous logs are intentionally fetched and combined for each pod, even though this requires more API calls. This design ensures all logs are captured even when pods restart but retain their name, providing complete diagnostic information.
holmes/plugins/toolsets/kubernetes_logs.yaml (2)
Learnt from: nherment
PR: #408
File: holmes/plugins/toolsets/kubernetes_logs.py:90-97
Timestamp: 2025-05-15T05:13:43.169Z
Learning: In the Kubernetes logs toolset for Holmes, both current and previous logs are intentionally fetched and combined for each pod, even though this requires more API calls. This design ensures all logs are captured even when pods restart but retain their name, providing complete diagnostic information.
Learnt from: nherment
PR: #408
File: holmes/plugins/toolsets/kubernetes_logs.py:100-102
Timestamp: 2025-05-15T05:14:06.519Z
Learning: The fetch_logs method in KubernetesLogsToolset is designed to apply the limit parameter after filtering and combining both current and previous logs, rather than using the API's tail_lines parameter, to ensure the limit applies to the final combined log set.
holmes/plugins/toolsets/kubernetes.yaml (1)
Learnt from: nherment
PR: #408
File: holmes/plugins/toolsets/kubernetes_logs.py:90-97
Timestamp: 2025-05-15T05:13:43.169Z
Learning: In the Kubernetes logs toolset for Holmes, both current and previous logs are intentionally fetched and combined for each pod, even though this requires more API calls. This design ensures all logs are captured even when pods restart but retain their name, providing complete diagnostic information.
tests/core/test_transformer_backwards_compatibility.py (1)
Learnt from: nherment
PR: #535
File: holmes/plugins/toolsets/bash/bash_toolset.py:207-209
Timestamp: 2025-06-24T05:51:04.543Z
Learning: The init_config method in toolsets should be idempotent - safely callable multiple times without errors. self.config should maintain consistent typing (not alternate between dict and config object types) throughout the object lifecycle.
holmes/core/toolset_manager.py (1)
Learnt from: nherment
PR: #535
File: holmes/plugins/toolsets/bash/bash_toolset.py:207-209
Timestamp: 2025-06-24T05:51:04.543Z
Learning: The init_config method in toolsets should be idempotent - safely callable multiple times without errors. self.config should maintain consistent typing (not alternate between dict and config object types) throughout the object lifecycle.
tests/core/transformers/test_llm_summarize.py (1)
Learnt from: Sheeproid
PR: #586
File: tests/llm/fixtures/test_ask_holmes/03_what_is_the_command_to_port_forward/test_case.yaml:4-4
Timestamp: 2025-07-02T10:27:17.231Z
Learning: In LLM-as-judge test cases for HolmesGPT, expected outputs should be descriptive rather than prescriptive when testing for flexible responses like port numbers. Using specific values in expected outputs can cause unnecessary test failures when the AI generates different but equally valid responses.
tests/config_class/test_config_transformers.py (1)
Learnt from: nherment
PR: #610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025-07-08T08:45:41.069Z
Learning: The robusta-dev/holmesgpt codebase has comprehensive existing validation for Azure environment variables (AZURE_API_BASE, AZURE_API_KEY, AZURE_API_VERSION) and MODEL in tests/llm/utils/classifiers.py, tests/llm/conftest.py, and holmes/core/llm.py. Don't suggest adding redundant validation logic.
tests/integration/test_tool_execution_pipeline.py (1)
Learnt from: Sheeproid
PR: #586
File: tests/llm/fixtures/test_ask_holmes/03_what_is_the_command_to_port_forward/test_case.yaml:4-4
Timestamp: 2025-07-02T10:27:17.231Z
Learning: In LLM-as-judge test cases for HolmesGPT, expected outputs should be descriptive rather than prescriptive when testing for flexible responses like port numbers. Using specific values in expected outputs can cause unnecessary test failures when the AI generates different but equally valid responses.
tests/core/test_config_transformers.py (1)
Learnt from: nherment
PR: #535
File: holmes/plugins/toolsets/bash/bash_toolset.py:207-209
Timestamp: 2025-06-24T05:51:04.543Z
Learning: The init_config method in toolsets should be idempotent - safely callable multiple times without errors. self.config should maintain consistent typing (not alternate between dict and config object types) throughout the object lifecycle.
holmes/core/transformers/validation.py (2)
Learnt from: nherment
PR: #610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025-07-08T08:45:41.069Z
Learning: The robusta-dev/holmesgpt codebase has comprehensive existing validation for Azure environment variables (AZURE_API_BASE, AZURE_API_KEY, AZURE_API_VERSION) and MODEL in tests/llm/utils/classifiers.py, tests/llm/conftest.py, and holmes/core/llm.py. Don't suggest adding redundant validation logic.
Learnt from: nherment
PR: #610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025-07-08T08:45:41.069Z
Learning: When suggesting improvements to environment variable handling in robusta-dev/holmesgpt, check first if validation logic already exists rather than reimplementing it.
tests/core/transformers/test_validation.py (1)
Learnt from: nherment
PR: #610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025-07-08T08:45:41.069Z
Learning: The robusta-dev/holmesgpt codebase has comprehensive existing validation for Azure environment variables (AZURE_API_BASE, AZURE_API_KEY, AZURE_API_VERSION) and MODEL in tests/llm/utils/classifiers.py, tests/llm/conftest.py, and holmes/core/llm.py. Don't suggest adding redundant validation logic.
holmes/config.py (1)
Learnt from: nherment
PR: #610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025-07-08T08:45:41.069Z
Learning: The robusta-dev/holmesgpt codebase has comprehensive existing validation for Azure environment variables (AZURE_API_BASE, AZURE_API_KEY, AZURE_API_VERSION) and MODEL in tests/llm/utils/classifiers.py, tests/llm/conftest.py, and holmes/core/llm.py. Don't suggest adding redundant validation logic.
tests/integration/test_config_merging_integration.py (1)
Learnt from: nherment
PR: #535
File: holmes/plugins/toolsets/bash/bash_toolset.py:207-209
Timestamp: 2025-06-24T05:51:04.543Z
Learning: The init_config method in toolsets should be idempotent - safely callable multiple times without errors. self.config should maintain consistent typing (not alternate between dict and config object types) throughout the object lifecycle.
tests/plugins/toolsets/test_kubernetes_transformers.py (1)
Learnt from: Sheeproid
PR: #586
File: tests/llm/fixtures/test_ask_holmes/03_what_is_the_command_to_port_forward/test_case.yaml:4-4
Timestamp: 2025-07-02T10:27:17.231Z
Learning: In LLM-as-judge test cases for HolmesGPT, expected outputs should be descriptive rather than prescriptive when testing for flexible responses like port numbers. Using specific values in expected outputs can cause unnecessary test failures when the AI generates different but equally valid responses.
🧬 Code Graph Analysis (5)
holmes/core/transformers/__init__.py (4)
holmes/core/transformers/base.py (2)
BaseTransformer(17-84)TransformerError(11-14)holmes/core/transformers/registry.py (2)
TransformerRegistry(9-106)register(20-39)holmes/core/transformers/llm_summarize.py (1)
LLMSummarizeTransformer(14-157)holmes/core/transformers/validation.py (5)
TransformerValidationError(13-16)validate_transformer_config(19-56)validate_transformer_configs(59-80)validate_tool_transformer_configs(83-105)safe_validate_tool_transformer_configs(108-126)
holmes/core/transformers/registry.py (1)
holmes/core/transformers/base.py (3)
BaseTransformer(17-84)TransformerError(11-14)name(77-84)
holmes/core/transformers/validation.py (1)
holmes/core/transformers/registry.py (3)
is_registered(83-93)list_transformers(95-102)create_transformer(56-81)
tests/plugins/toolsets/test_aks_transformers.py (3)
holmes/plugins/toolsets/__init__.py (1)
load_toolsets_from_file(41-55)holmes/core/transformers/llm_summarize.py (1)
name(155-157)holmes/core/transformers/base.py (1)
name(77-84)
tests/utils/test_config_utils.py (1)
holmes/utils/config_utils.py (1)
merge_transformer_configs(8-69)
🪛 YAMLlint (1.37.1)
holmes/plugins/toolsets/aks-node-health.yaml
[error] 10-10: trailing spaces
(trailing-spaces)
holmes/plugins/toolsets/aks.yaml
[error] 10-10: trailing spaces
(trailing-spaces)
🪛 Ruff (0.12.2)
tests/integration/test_kubernetes_transformer_execution.py
509-509: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
tests/core/test_transformer_backwards_compatibility.py
94-97: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
holmes/core/transformers/base.py
64-74: BaseTransformer._validate_config is an empty method in an abstract base class, but has no abstract decorator
(B027)
tests/core/transformers/test_transformers.py
25-29: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
tests/core/test_tool_transformers.py
656-656: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
tests/core/transformers/test_llm_summarize.py
405-405: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
holmes/core/tools.py
145-149: Use a single if statement instead of nested if statements
(SIM102)
235-235: Local variable size_change is assigned to but never used
Remove assignment to unused variable size_change
(F841)
🪛 markdownlint-cli2 (0.17.2)
docs/transformers.md
119-119: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
151-151: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
161-161: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
171-171: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
181-181: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (87)
tests/core/transformers/__init__.py (1)
1-1: LGTM! Clean test module initialization.The docstring clearly identifies the purpose of the init file for transformer tests.
README.md (1)
315-321: Excellent documentation addition for the new transformer feature.The section effectively introduces the transformer concept, explains its purpose for managing context window limits, and provides appropriate links to detailed documentation. The placement under config file usage is logical.
config.example.yaml (1)
7-11: Well-documented configuration options for the new transformer feature.The comments clearly explain the purpose and benefits of the fast model summarization feature. The example values (gpt-4o-mini, 1000 character threshold) are sensible defaults.
holmes/plugins/toolsets/aks-node-health.yaml (4)
20-29: Excellent transformer configuration for node status summarization.The llm_summarize configuration is well-designed with:
- Appropriate input threshold (800 characters)
- Detailed prompt focusing on health issues and troubleshooting
- Specific guidance to group healthy nodes and highlight concerning conditions
- Request for exact node names for follow-up investigation
35-45: Well-crafted transformer prompt for node description output.The prompt effectively guides summarization to focus on key troubleshooting areas: node conditions, resource usage, taints/labels, events, and system information. The higher threshold (1200 characters) is appropriate for the more detailed kubectl describe output.
61-71: Comprehensive transformer configuration for Azure Activity Log.The configuration appropriately handles the verbose nature of Azure Activity Logs with:
- Higher threshold (1500 characters) suitable for log output
- Focused prompt on administrative actions and errors
- Grouping of similar activities and time pattern analysis
- Request for specific operation names and correlation IDs
102-112: Appropriate transformer setup for VMSS command output.The configuration handles command execution output effectively with a balanced threshold (1000 characters) and a prompt that covers execution status, diagnostics, performance metrics, and actionable findings.
holmes/utils/config_utils.py (4)
8-32: Well-designed function signature and input validation.The function properly handles edge cases with early returns for None/empty inputs, has clear type hints, and includes comprehensive docstring explaining the merging logic.
34-44: Efficient conversion to dictionary format for merging.The conversion from list-of-dictionaries to dictionary format makes the merging logic cleaner and more efficient. The nested loops properly extract transformer types and configurations.
45-65: Correct field-level merging with proper precedence.The merging logic correctly:
- Starts with base configuration
- Applies override configuration with higher precedence
- Handles transformer types that exist only in base or override
- Maintains all transformer configurations
66-69: Proper conversion back to original list format.The final conversion back to the list-of-dictionaries format maintains compatibility with the expected data structure while preserving the merged configurations.
holmes/core/transformers/__init__.py (3)
8-17: LGTM: Clean import organization and comprehensive API exposure.The imports are well-organized and cover all the essential components of the transformer system. The validation utilities are properly imported to support configuration validation throughout the system.
19-20: LGTM: Proper registration of built-in transformer.The built-in
llm_summarizetransformer is correctly registered with the registry, making it immediately available for use across the system.
22-33: LGTM: Comprehensive all definition.The
__all__list properly exposes all the necessary components for external usage, ensuring a clean public API for the transformer system.holmes/plugins/toolsets/kubernetes_logs.yaml (3)
11-14: LGTM: Clear documentation of transformer usage.The comment effectively explains the purpose and benefits of using transformer configs with llm_summarize for log tools, helping users understand why these configurations are present.
31-41: LGTM: Well-designed transformer configuration for single pod logs.The configuration is appropriate with:
- Reasonable 1000-character threshold for triggering summarization
- Comprehensive prompt covering key log analysis areas (errors, patterns, performance)
- Focus on searchable keywords for drill-down capability
46-56: LGTM: Tailored configuration for multi-container logs.The prompt is appropriately customized for multi-container scenarios, emphasizing:
- Container-specific error analysis
- Inter-container communication patterns
- Resource usage patterns by container
This differentiation makes sense given the different nature of multi-container log analysis.
holmes/core/transformers/llm_summarize.py (6)
29-35: LGTM: Well-designed default prompt for operational data summarization.The default prompt effectively guides the LLM to focus on actionable information while preserving searchability through exact keywords and IDs. The emphasis on "extraction over abstraction" aligns well with operational debugging needs.
51-60: LGTM: Robust LLM instantiation with graceful fallback.The code properly handles fast model creation failures by logging warnings and setting
_fast_llmto None, allowing the transformer to gracefully skip summarization when the fast model is unavailable.
62-77: LGTM: Comprehensive configuration validation.The validation covers all configuration parameters with appropriate type and value checks:
- Non-negative integer for input_threshold
- Non-empty string validation for prompt and fast_model
79-107: LGTM: Sound conditional logic for transformer application.The
should_applymethod correctly implements the two-stage check:
- Fast model availability
- Input length threshold comparison
The debug logging provides good observability for troubleshooting.
109-152: LGTM: Robust transformation implementation with proper error handling.The transform method:
- Validates fast model availability upfront
- Constructs appropriate prompts
- Handles LLM completion properly
- Validates non-empty responses
- Provides comprehensive error logging and exception chaining
154-157: LGTM: Consistent transformer naming.The name property returns the expected transformer identifier used in registration.
holmes/core/transformers/base.py (6)
11-14: LGTM: Simple and appropriate exception class.The TransformerError exception provides a clear way to indicate transformer operation failures.
25-33: LGTM: Proper initialization with config validation.The constructor correctly initializes configuration and calls validation, following good initialization patterns.
35-49: LGTM: Well-defined abstract transformation method.The abstract method signature and documentation clearly define the transformation contract with appropriate error handling specification.
51-62: LGTM: Clear conditional application method.The should_apply method provides a clean way for transformers to determine when they should be applied to input text.
64-74: Address static analysis hint: _validate_config design is intentionally non-abstract.The static analysis tool flags this as potentially needing an abstract decorator, but the current design is correct. This method provides an optional validation hook that subclasses can override if needed, while allowing transformers without special validation to inherit the empty implementation.
The design is intentional and appropriate for this use case.
76-84: LGTM: Sensible default name implementation.Using the class name as the default transformer name is a reasonable approach, while allowing subclasses to override if needed.
holmes/plugins/toolsets/kubernetes.yaml (4)
11-14: LGTM: Clear explanation of transformer usage.The comment effectively communicates the purpose and benefits of using transformers for kubectl commands, helping users understand the feature.
24-33: LGTM: Appropriate transformer configuration for kubectl describe.The configuration is well-suited for describe outputs with:
- Reasonable 1000-character threshold
- Focus on actionable items and health indicators
- Emphasis on exact field names for searchability
42-52: LGTM: Well-tailored configuration for namespace-scoped resources.The prompt appropriately focuses on:
- Aggregate descriptions for similar resources
- Highlighting outliers and errors
- Providing searchable keywords for drill-down
This aligns well with typical kubectl get output analysis needs.
57-67: LGTM: Consistent configuration for cluster-wide resources.The configuration maintains consistency with the namespace-scoped version while being appropriate for cluster-wide resource analysis. The prompt covers the same key areas with suitable language for cluster-wide scope.
holmes/plugins/toolsets/aks.yaml (3)
11-13: Good documentation for transformer usage.The comment clearly explains that these tools use the llm_summarize transformer to handle large JSON outputs when a fast model is configured.
25-36: Well-structured transformer configuration for AKS cluster details.The transformer configuration is properly structured with:
- Appropriate threshold (1500 chars) for cluster configuration data
- Comprehensive prompt covering all key aspects of AKS cluster information
- Clear focus on operational and security-relevant details
42-52: Effective summarization setup for cluster listings.The configuration appropriately:
- Uses a lower threshold (1000) suitable for list outputs
- Focuses on comparison and grouping of clusters
- Highlights error states and version issues
holmes/core/transformers/registry.py (2)
31-39: Robust validation in register method.Good implementation with proper type checking and duplicate prevention.
78-81: Excellent error handling with exception chaining.The use of
from epreserves the original exception context, which is helpful for debugging.tests/core/test_transformer_backwards_compatibility.py (1)
11-254: Comprehensive backwards compatibility test coverage.Excellent test suite that thoroughly verifies backwards compatibility across all major components:
- Configuration loading scenarios
- Toolset and tool behavior without transformer configs
- Preservation of existing configurations
- Multiple toolset types (YAML, Python)
- Constructor compatibility
This ensures the new transformer feature doesn't break existing functionality.
tests/integration/test_kubernetes_transformer_execution.py (3)
15-31: Well-designed mock transformer for testing.The
MockSummarizeTransformereffectively simulates the summarization behavior with clear output that includes original length information, making it easy to verify in tests.
282-351: Excellent transformer failure handling test.This test thoroughly verifies that:
- Tool execution continues when transformers fail
- Original output is preserved
- Appropriate warnings are logged
- The system degrades gracefully
This is critical for production reliability.
389-454: Comprehensive test for transformer chaining.Good verification that multiple transformers are applied in sequence, with each transformer's output becoming the input for the next.
holmes/main.py (2)
166-171: Well-integrated CLI options for transformer configuration.The new options follow the existing pattern and provide clear help text:
--fast-model: Optional fast model specification--summarize-threshold: Configurable threshold with documented defaultThese integrate seamlessly with the transformer feature.
300-301: Proper parameter propagation to configuration.The new parameters are correctly added to the
askcommand signature and passed through toConfig.load_from_file, maintaining the existing configuration loading pattern.Also applies to: 343-344
tests/core/transformers/test_llm_summarize.py (2)
350-371: Excellent integration test coverage with realistic scenario.The end-to-end test effectively simulates a real kubectl output scenario with appropriate assertions for both length-based application and content verification. This provides good confidence in the transformer's behavior in production scenarios.
413-433: Well-designed parametric test for threshold scenarios.The test cases comprehensively cover edge cases around threshold boundaries, including zero threshold, empty input, exact threshold matches, and various length scenarios. This ensures robust threshold logic validation.
holmes/core/toolset_manager.py (4)
31-35: LGTM! Clean constructor extension.The addition of the
global_transformer_configsparameter follows the existing pattern and maintains backward compatibility with the defaultNonevalue.
116-119: Good integration point for global config application.Applying global transformer configs after assembling all toolsets but before prerequisite checks is the correct placement. This ensures all toolsets (built-in, configured, and custom) receive the global configurations consistently.
431-453: Excellent implementation of config merging with tool propagation.The method correctly:
- Returns early when no global configs are present
- Uses the utility function for intelligent merging respecting precedence
- Propagates merged configs to individual tools within toolsets
- Handles the case where toolsets might not have tools
This ensures consistent transformer configuration inheritance throughout the hierarchy.
267-268: Consistent application of global configs to CLI custom toolsets.Good to see that CLI custom toolsets also receive global transformer configs, maintaining consistency with other toolset loading paths.
tests/plugins/toolsets/test_aks_transformers.py (2)
125-180: Comprehensive prompt validation with domain-specific terms.Excellent test coverage for verifying that transformer prompts contain expected domain-specific keywords. This ensures the summarization will focus on relevant AKS concepts.
181-245: Well-designed threshold validation tests.The tests effectively validate that different tools have appropriate thresholds based on their expected output complexity (e.g., higher thresholds for detailed JSON outputs like
aks_get_cluster).holmes/config.py (5)
72-74: Well-designed field additions with sensible defaults.The new transformer-related fields follow established patterns:
fast_modelas optional string for specifying alternative modelssummarize_thresholdwith a reasonable default of 1000 characterstransformer_configsas optional list for explicit configuration
163-179: Excellent auto-generation logic with proper conditions.The method correctly:
- Only generates configs when
fast_modelis provided buttransformer_configsis not already set- Creates a properly structured transformer config for the
llm_summarizetransformer- Includes appropriate debug logging for transparency
- Uses the configured threshold value
This provides a convenient way for users to enable summarization without complex configuration.
160-161: Appropriate placement of auto-generation call.Calling
_auto_generate_transformer_configs()inmodel_post_initensures the transformer configs are prepared early in the initialization process, after all fields are set but before the configuration is used.
147-147: Correct integration with ToolsetManager.Passing
transformer_configsasglobal_transformer_configsto theToolsetManagerenables the global configuration inheritance throughout the toolset hierarchy.
225-226: Consistent environment variable support.Adding the new fields to
load_from_envmaintains consistency with the existing environment-based configuration pattern.tests/core/test_toolset_manager.py (4)
310-339: Excellent test for config merging with precedence verification.This test effectively verifies that:
- Global transformer configs are merged with existing toolset configs
- Toolset-specific values take precedence over global ones (input_threshold: 1000 vs 500)
- Global values are used when not present in toolset configs (fast_model)
- Existing toolset values are preserved (prompt)
The test structure and assertions clearly validate the merging behavior.
341-354: Good coverage of global config application to empty toolsets.This test ensures that toolsets without existing transformer configs properly receive the global configurations, which is an important edge case.
373-396: Comprehensive test for different transformer types coexistence.This test verifies that different transformer types can coexist in the configuration, which is important for future extensibility when additional transformer types are added.
398-427: Valuable integration test for end-to-end workflow.This test verifies that the global transformer config merging works correctly within the complete toolset loading pipeline, providing confidence that the feature works in realistic scenarios.
tests/integration/test_tool_execution_pipeline.py (3)
15-29: Well-designed mock transformer for testing.The mock transformer effectively simulates the LLM summarization behavior with configurable thresholds and simple text truncation logic, making it suitable for integration testing.
56-77: Comprehensive test for YAML tool transformer integration.The test effectively validates that transformers are applied to YAML tools with appropriate threshold checking and output summarization.
171-194: Clear test for conditional transformer logic.The test effectively validates that transformers respect their threshold configuration and are not applied to short outputs.
tests/utils/test_config_utils.py (2)
8-12: Correct edge case test.Properly validates that merging two None configs returns None.
14-150: Comprehensive test coverage for merge_transformer_configs.The test suite thoroughly covers all merging scenarios including edge cases, field-level merging with proper precedence, multiple transformer types, and complex configurations. The tests are well-structured and provide confidence in the merging logic.
tests/config_class/test_config_transformers.py (1)
153-249: Well-designed tests for transformer config auto-generation.The tests comprehensively cover the auto-generation behavior including when to generate configs, preservation of existing configs, CLI overrides, and default threshold handling.
tests/integration/test_config_merging_integration.py (1)
12-274: Excellent integration test coverage for config merging.The tests comprehensively cover the configuration merging functionality with realistic scenarios including:
- Real-world Kubernetes toolset configurations
- Three-level inheritance (global → toolset → tool)
- Multiple transformer types
- Backward compatibility
- No regression cases
The test design effectively validates the complete merging workflow.
tests/plugins/toolsets/test_kubernetes_transformers.py (1)
432-436: Good use of next() with generator expressions.This is a cleaner pattern than the loops used in the previous test class. Consider applying this pattern consistently across all test methods for finding toolsets and tools.
tests/core/test_config_transformers.py (2)
10-75: Well-structured tests for Config transformer functionality.The test class provides good coverage of Config's transformer configuration handling, including edge cases like None and empty lists. The use of mocking to verify ToolsetManager interaction is appropriate.
77-212: Comprehensive testing of ToolsetManager transformer inheritance.Excellent test coverage of the transformer configuration inheritance logic. The use of
Mock(spec=...)ensures type safety, and the tests clearly document the expected behavior, including the important note that tool-level inheritance is handled byToolset.preprocess_tools().tests/core/transformers/test_transformers.py (1)
50-329: Excellent test coverage for transformer infrastructure.The tests comprehensively cover the transformer base classes and registry, including:
- Configuration validation
- Abstract method enforcement
- Registry operations (register, unregister, create)
- Error handling and edge cases
- Integration scenarios
The test organization and use of mock classes is exemplary.
holmes/core/tools.py (2)
183-263: Robust transformer application implementation.The
_apply_transformersmethod is well-implemented with:
- Proper error handling that distinguishes between expected TransformerError and unexpected exceptions
- Resilient behavior that continues processing other transformers if one fails
- Detailed logging for debugging and monitoring
- Proper result copying to avoid mutating the original
The decision to continue processing after transformer failures is appropriate for maintaining system reliability.
484-507: Well-designed transformer configuration inheritance.The implementation in
preprocess_toolscorrectly handles transformer configuration inheritance from toolset to individual tools. The use ofmerge_transformer_configsensures that tool-level configurations can override toolset-level defaults, which follows the principle of least surprise.tests/core/test_tool_transformers.py (6)
39-90: Well-structured test coverage for tool transformer field acceptance.The test class properly validates that both Tool and YAMLTool accept the transformer_configs field, with appropriate tests for tools with and without transformers.
92-153: Excellent test coverage for toolset transformer propagation.The tests effectively validate the transformer inheritance behavior, ensuring tools without transformers inherit from the toolset while tools with their own transformers maintain them.
155-258: Comprehensive transformer validation test coverage.The test class effectively covers all validation scenarios with proper test isolation through setup/teardown methods. The validation tests include edge cases and error conditions.
260-324: Good integration testing for tool validation behavior.The tests properly validate that tools handle invalid transformer configurations gracefully, clearing them while logging appropriate warnings.
326-373: Essential backward compatibility tests.These tests ensure that the new transformer feature maintains backward compatibility with existing tools and toolsets that don't use transformers.
375-654: Comprehensive tool execution pipeline tests.Excellent test coverage for transformer execution including success cases, error handling, chaining, conditional application, result preservation, and performance logging. The tests thoroughly validate the transformer integration in the tool execution pipeline.
Also applies to: 657-694
tests/core/transformers/test_validation.py (7)
1-43: Well-designed mock transformers for validation testing.The mock transformer classes effectively simulate different validation scenarios - one that always validates successfully and another with configurable validation rules.
45-53: Basic exception test coverage.Simple but necessary test ensuring the TransformerValidationError exception works correctly.
55-128: Thorough test coverage for transformer config validation.The test class comprehensively covers all validation scenarios including edge cases, with proper test isolation through setup/teardown methods.
130-185: Comprehensive list validation tests.The tests effectively cover all list validation scenarios and properly verify that error messages include the index of invalid configurations.
187-215: Good tool-specific validation tests.The tests ensure that tool names are properly included in error messages, which is important for debugging.
217-263: Effective safe validation testing with proper logging verification.The tests properly verify both return values and logging behavior, ensuring warnings contain relevant debugging information.
265-308: Well-designed integration tests for validation scenarios.The integration tests effectively validate complex configurations and mixed valid/invalid scenarios, ensuring proper error handling at the correct indices.
|
If it's hard to review I can split it into multiple parts. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (14)
holmes/plugins/toolsets/aks-node-health.yaml (1)
10-10: Remove trailing spaces.Line 10 contains only trailing spaces that should be removed.
holmes/plugins/toolsets/aks.yaml (1)
10-10: Remove trailing spaces.Line 10 contains only trailing spaces that should be removed.
tests/integration/test_kubernetes_transformer_execution.py (1)
509-509: Remove unused variable assignment.The
resultvariable is assigned but never used.tests/integration/test_tool_execution_pipeline.py (5)
39-41: Avoid accessing private registry attributes.Accessing the private
_transformersattribute breaks encapsulation and could fail if the registry implementation changes.Consider storing the transformer class reference using public methods:
- # Store the original transformer class for restoration - self._original_llm_summarize = registry._transformers["llm_summarize"] + # Store the original transformer class for restoration + # Could use registry.create_transformer to verify it exists + try: + test_instance = registry.create_transformer("llm_summarize", {}) + self._original_llm_summarize = test_instance.__class__ + except Exception: + self._original_llm_summarize = None
123-126: Fix the original size calculation to match test data.The original size calculation doesn't match the actual test data structure (7 different entries × 20 repetitions).
- original_size = len( - "\n".join(["2024-01-01 10:00:00 INFO Starting application"] * 140) - ) + # Calculate actual original size based on test data + log_entries = [ + "2024-01-01 10:00:00 INFO Starting application", + "2024-01-01 10:00:01 INFO Loading configuration", + "2024-01-01 10:00:02 DEBUG Database connection established", + "2024-01-01 10:00:03 INFO Server listening on port 8080", + "2024-01-01 10:01:00 ERROR Failed to process request: timeout", + "2024-01-01 10:01:01 WARN Retrying request", + "2024-01-01 10:01:02 INFO Request processed successfully", + ] * 20 + original_size = len("\n".join(log_entries))
236-237: Make timing assertion more robust.The assertion
"in 0."assumes sub-second execution, which could fail on slow systems.- assert "in 0." in performance_log # Should show elapsed time + # Should show elapsed time in format "in X.XXXs" + assert " in " in performance_log and "s" in performance_log
278-278: Fix incorrect length comparison.The kubectl_output was already multiplied by 5, so multiplying again in the assertion is incorrect.
- assert len(result.data) < len(kubectl_output) * 5 # Much shorter than original + assert len(result.data) < len(kubectl_output) # Much shorter than original
298-300: Make assertion more deterministic.The OR condition makes the test less predictable. Since the threshold is 10 and the message is longer, it should always be summarized.
- assert ( - "SUMMARIZED:" in result.data - or "Important debugging information" in result.data - ) + # Message is longer than threshold (10), so it should be summarized + assert "SUMMARIZED:" in result.data + assert "Important debugging information" not in result.datatests/core/transformers/test_transformers.py (1)
24-31: Simplify nested if statements in validation logic.The static analysis correctly identifies that the nested if statements can be combined for better readability.
def _validate_config(self) -> None: - if "threshold" in self.config: - if ( - not isinstance(self.config["threshold"], int) - or self.config["threshold"] < 0 - ): - raise ValueError("Threshold must be a non-negative integer") + if "threshold" in self.config and ( + not isinstance(self.config["threshold"], int) + or self.config["threshold"] < 0 + ): + raise ValueError("Threshold must be a non-negative integer")tests/core/test_tool_transformers.py (1)
655-656: Remove unused variable assignment.The
resultvariable is assigned but never used in this test method.Apply this fix:
- with patch("holmes.core.tools.logging") as mock_logging: - result = tool.invoke({}) + with patch("holmes.core.tools.logging") as mock_logging: + tool.invoke({})holmes/core/tools.py (2)
142-154: Simplify nested if statements in transformer validation.The static analysis correctly identifies that the nested if statements can be combined.
@model_validator(mode="after") def validate_transformers(self): """Validate transformer configurations during tool creation.""" - if self.transformer_configs is not None: - # Use safe validation to log warnings instead of failing - if not safe_validate_tool_transformer_configs( - self.name, self.transformer_configs - ): - # If validation fails, clear transforms to prevent runtime errors - logging.warning(f"Clearing invalid transforms for tool '{self.name}'") - self.transformer_configs = None + if self.transformer_configs is not None and not safe_validate_tool_transformer_configs( + self.name, self.transformer_configs + ): + # If validation fails, clear transforms to prevent runtime errors + logging.warning(f"Clearing invalid transforms for tool '{self.name}'") + self.transformer_configs = None return self
236-244: Remove unused size_change variable.The
size_changevariable is calculated but never used. Either use it in logging or remove it.# Let the transformer provide its own logging message if it wants to post_transform_size = len(transformed_data) -size_change = post_transform_size - pre_transform_size # Generic logging - transformers can override this with their own specific metrics logging.info( f"Applied transformer '{transformer_name}' to tool '{self.name}' output " - f"in {transform_elapsed:.2f}s (output size: {post_transform_size:,} characters)" + f"in {transform_elapsed:.2f}s (output size: {pre_transform_size:,} → {post_transform_size:,} characters)" )docs/transformers.md (2)
119-127: Add language specification to fenced code blocks.The static analysis correctly identifies that these code blocks are missing language specifications. While they appear to be plain text output examples, adding appropriate language identifiers will improve readability.
-``` +```text Summarize this operational data focusing on: - What needs attention or immediate action - Group similar entries into a single line and description - Make sure to mention outliers, errors, and non-standard patterns - List normal/healthy patterns as aggregate descriptions - When listing problematic entries, also try to use aggregate descriptions when possible - When possible, mention exact keywords, IDs, or patterns so the user can filter/search the original data and drill down on the parts they care about -``` +```
151-187: Add language specification to output example code blocks.These example output blocks should have language specifications for proper formatting.
**Without Transformer:** -``` +```text NAME READY STATUS RESTARTS AGE IP NODE pod-1 1/1 Running 0 5d 10.1.1.1 node-1 pod-2 1/1 Running 0 5d 10.1.1.2 node-1 pod-3 1/1 Running 0 5d 10.1.1.3 node-2 pod-4 0/1 CrashLoopBackOff 15 1h 10.1.1.4 node-2 [... 100 more similar pods ...] -``` +``` **With Transformer:** -``` +```text Found 104 pods across 2 nodes: - 103 pods are healthy and running (age: 5d, on node-1 and node-2) - 1 pod in CrashLoopBackOff state: pod-4 (15 restarts, 1h old, IP 10.1.1.4, node-2) - Search with "grep pod-4" or "grep CrashLoopBackOff" to drill down on the problematic pod -``` +``` ### Log Analysis **Without Transformer:** -``` +```text 2024-01-15T10:30:01Z INFO Starting application... 2024-01-15T10:30:02Z INFO Database connection established 2024-01-15T10:30:03Z INFO Loading configuration... [... 1000 similar INFO logs ...] 2024-01-15T10:35:15Z ERROR Failed to connect to Redis: connection timeout 2024-01-15T10:35:16Z WARN Retrying Redis connection (attempt 1/3) -``` +``` **With Transformer:** -``` +```text Log analysis (2024-01-15 10:30-10:35): - 1000+ INFO messages showing normal application startup and operations - 1 ERROR: Redis connection timeout at 10:35:15Z - 1 WARN: Redis retry attempt at 10:35:16Z - Search with "grep ERROR" or "grep Redis" to investigate the connection issue -``` +```
🧹 Nitpick comments (5)
tests/core/test_transformer_backwards_compatibility.py (2)
77-82: Combine nestedwithstatements.The nested
withstatements can be combined for cleaner code:- with patch("holmes.config.load_model_from_file") as mock_load: - mock_load.return_value = Config(**mock_config_content) - - with patch("pathlib.Path.exists") as mock_exists: - mock_exists.return_value = True + with patch("holmes.config.load_model_from_file") as mock_load, \ + patch("pathlib.Path.exists") as mock_exists: + mock_load.return_value = Config(**mock_config_content) + mock_exists.return_value = True
94-99: Combine nestedwithstatements.The nested
withstatements can be combined:- with patch.dict("os.environ", test_env): - with patch( - "holmes.config.Config._Config__get_cluster_name" - ) as mock_cluster: - mock_cluster.return_value = "test-cluster" + with patch.dict("os.environ", test_env), \ + patch("holmes.config.Config._Config__get_cluster_name") as mock_cluster: + mock_cluster.return_value = "test-cluster"tests/core/transformers/test_llm_summarize.py (2)
15-28: Consider extracting duplicatecreate_mock_llmhelper.Both test classes define identical
create_mock_llmmethods. Consider extracting this to a module-level function to follow DRY principles.+def create_mock_llm(response_content: str = "Summarized content"): + """Create a mock LLM that returns the specified response.""" + mock_llm = Mock() + mock_response = Mock() + mock_choice = Mock() + mock_message = Mock() + + mock_message.content = response_content + mock_choice.message = mock_message + mock_response.choices = [mock_choice] + mock_llm.completion.return_value = mock_response + + return mock_llm + + class TestLLMSummarizeTransformer: """Test cases for LLMSummarizeTransformer class.""" - def create_mock_llm(self, response_content: str = "Summarized content"): - """Create a mock LLM that returns the specified response.""" - mock_llm = Mock() - mock_response = Mock() - mock_choice = Mock() - mock_message = Mock() - - mock_message.content = response_content - mock_choice.message = mock_message - mock_response.choices = [mock_choice] - mock_llm.completion.return_value = mock_response - - return mock_llmAlso applies to: 324-337
403-407: Useisinstance()for exception type checking.Replace class comparison with
isinstance()for more Pythonic and robust type checking.# Verify error is properly wrapped and chained assert "Failed to summarize content with fast model" in str(exc_info.value) - assert exc_info.value.__cause__.__class__ == ConnectionError + assert isinstance(exc_info.value.__cause__, ConnectionError) assert "Network timeout" in str(exc_info.value.__cause__)tests/core/transformers/test_validation.py (1)
323-323: Add newline at end of filePython files should end with a newline character.
error_str = str(exc_info.value) assert "required_param is missing" in error_str +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 565465513ed6d2eafa9f2022356d44aea42e0681 and 8a2df1931ee254688ead2509b7dbfe56b4520822.
📒 Files selected for processing (29)
README.md(1 hunks)config.example.yaml(1 hunks)docs/transformers.md(1 hunks)holmes/config.py(6 hunks)holmes/core/tools.py(5 hunks)holmes/core/toolset_manager.py(6 hunks)holmes/core/transformers/__init__.py(1 hunks)holmes/core/transformers/base.py(1 hunks)holmes/core/transformers/llm_summarize.py(1 hunks)holmes/core/transformers/registry.py(1 hunks)holmes/core/transformers/validation.py(1 hunks)holmes/main.py(3 hunks)holmes/plugins/toolsets/aks-node-health.yaml(3 hunks)holmes/plugins/toolsets/aks.yaml(3 hunks)holmes/plugins/toolsets/kubernetes.yaml(3 hunks)holmes/plugins/toolsets/kubernetes_logs.yaml(2 hunks)holmes/utils/config_utils.py(1 hunks)tests/config_class/test_config_transformers.py(1 hunks)tests/core/test_config_transformers.py(1 hunks)tests/core/test_tool_transformers.py(1 hunks)tests/core/test_toolset_manager.py(1 hunks)tests/core/test_transformer_backwards_compatibility.py(1 hunks)tests/core/transformers/__init__.py(1 hunks)tests/core/transformers/test_llm_summarize.py(1 hunks)tests/core/transformers/test_transformers.py(1 hunks)tests/core/transformers/test_validation.py(1 hunks)tests/integration/test_config_merging_integration.py(1 hunks)tests/integration/test_kubernetes_transformer_execution.py(1 hunks)tests/integration/test_tool_execution_pipeline.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- README.md
- holmes/core/transformers/init.py
🚧 Files skipped from review as they are similar to previous changes (13)
- tests/core/transformers/init.py
- config.example.yaml
- holmes/utils/config_utils.py
- holmes/plugins/toolsets/kubernetes_logs.yaml
- holmes/core/transformers/llm_summarize.py
- holmes/core/transformers/validation.py
- holmes/plugins/toolsets/kubernetes.yaml
- tests/integration/test_config_merging_integration.py
- tests/core/test_toolset_manager.py
- tests/config_class/test_config_transformers.py
- holmes/config.py
- holmes/main.py
- holmes/core/transformers/registry.py
🧰 Additional context used
🧠 Learnings (7)
tests/integration/test_tool_execution_pipeline.py (1)
Learnt from: Sheeproid
PR: #586
File: tests/llm/fixtures/test_ask_holmes/03_what_is_the_command_to_port_forward/test_case.yaml:4-4
Timestamp: 2025-07-02T10:27:17.231Z
Learning: In LLM-as-judge test cases for HolmesGPT, expected outputs should be descriptive rather than prescriptive when testing for flexible responses like port numbers. Using specific values in expected outputs can cause unnecessary test failures when the AI generates different but equally valid responses.
tests/core/transformers/test_llm_summarize.py (1)
Learnt from: Sheeproid
PR: #586
File: tests/llm/fixtures/test_ask_holmes/03_what_is_the_command_to_port_forward/test_case.yaml:4-4
Timestamp: 2025-07-02T10:27:17.231Z
Learning: In LLM-as-judge test cases for HolmesGPT, expected outputs should be descriptive rather than prescriptive when testing for flexible responses like port numbers. Using specific values in expected outputs can cause unnecessary test failures when the AI generates different but equally valid responses.
tests/core/test_config_transformers.py (1)
Learnt from: nherment
PR: #535
File: holmes/plugins/toolsets/bash/bash_toolset.py:207-209
Timestamp: 2025-06-24T05:51:04.543Z
Learning: The init_config method in toolsets should be idempotent - safely callable multiple times without errors. self.config should maintain consistent typing (not alternate between dict and config object types) throughout the object lifecycle.
holmes/core/tools.py (2)
Learnt from: nherment
PR: #610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025-07-08T08:45:41.069Z
Learning: The robusta-dev/holmesgpt codebase has comprehensive existing validation for Azure environment variables (AZURE_API_BASE, AZURE_API_KEY, AZURE_API_VERSION) and MODEL in tests/llm/utils/classifiers.py, tests/llm/conftest.py, and holmes/core/llm.py. Don't suggest adding redundant validation logic.
Learnt from: nherment
PR: #610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025-07-08T08:45:41.069Z
Learning: When suggesting improvements to environment variable handling in robusta-dev/holmesgpt, check first if validation logic already exists rather than reimplementing it.
tests/core/transformers/test_validation.py (1)
Learnt from: nherment
PR: #610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025-07-08T08:45:41.069Z
Learning: The robusta-dev/holmesgpt codebase has comprehensive existing validation for Azure environment variables (AZURE_API_BASE, AZURE_API_KEY, AZURE_API_VERSION) and MODEL in tests/llm/utils/classifiers.py, tests/llm/conftest.py, and holmes/core/llm.py. Don't suggest adding redundant validation logic.
holmes/core/toolset_manager.py (1)
Learnt from: nherment
PR: #535
File: holmes/plugins/toolsets/bash/bash_toolset.py:207-209
Timestamp: 2025-06-24T05:51:04.543Z
Learning: The init_config method in toolsets should be idempotent - safely callable multiple times without errors. self.config should maintain consistent typing (not alternate between dict and config object types) throughout the object lifecycle.
tests/core/test_transformer_backwards_compatibility.py (1)
Learnt from: nherment
PR: #535
File: holmes/plugins/toolsets/bash/bash_toolset.py:207-209
Timestamp: 2025-06-24T05:51:04.543Z
Learning: The init_config method in toolsets should be idempotent - safely callable multiple times without errors. self.config should maintain consistent typing (not alternate between dict and config object types) throughout the object lifecycle.
🧬 Code Graph Analysis (2)
tests/integration/test_kubernetes_transformer_execution.py (7)
holmes/plugins/toolsets/__init__.py (1)
load_toolsets_from_file(48-62)holmes/core/tools.py (3)
StructuredToolResult(51-74)ToolResultStatus(29-48)invoke(162-184)holmes/core/transformers/base.py (4)
BaseTransformer(17-84)transform(36-49)should_apply(52-62)name(77-84)holmes/core/transformers/llm_summarize.py (3)
transform(109-152)should_apply(79-107)name(155-157)tests/core/test_tool_transformers.py (12)
transform(32-33)transform(465-466)transform(512-513)transform(555-556)should_apply(35-36)should_apply(468-469)should_apply(515-516)should_apply(558-560)test_transformer_failure_handling(460-506)FailingTransformer(464-469)test_multiple_transformers_chaining(508-549)SecondTransformer(511-516)tests/core/transformers/test_transformers.py (7)
transform(14-15)transform(32-33)transform(43-44)should_apply(17-18)should_apply(35-37)should_apply(46-47)FailingTransformer(40-47)holmes/core/transformers/registry.py (3)
register(20-39)is_registered(83-93)unregister(41-54)
tests/core/transformers/test_llm_summarize.py (2)
holmes/core/transformers/llm_summarize.py (4)
LLMSummarizeTransformer(14-157)name(155-157)should_apply(79-107)transform(109-152)holmes/core/transformers/base.py (4)
TransformerError(11-14)name(77-84)should_apply(52-62)transform(36-49)
🪛 Ruff (0.12.2)
tests/integration/test_kubernetes_transformer_execution.py
509-509: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
tests/core/transformers/test_llm_summarize.py
405-405: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
tests/core/test_tool_transformers.py
656-656: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
holmes/core/tools.py
145-149: Use a single if statement instead of nested if statements
(SIM102)
238-238: Local variable size_change is assigned to but never used
Remove assignment to unused variable size_change
(F841)
tests/core/transformers/test_transformers.py
25-29: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
holmes/core/toolset_manager.py
126-126: Undefined name enabled_toolsets
(F821)
129-129: Undefined name enabled_toolsets
(F821)
holmes/core/transformers/base.py
64-74: BaseTransformer._validate_config is an empty method in an abstract base class, but has no abstract decorator
(B027)
tests/core/test_transformer_backwards_compatibility.py
94-97: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
🪛 markdownlint-cli2 (0.17.2)
docs/transformers.md
119-119: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
151-151: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
161-161: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
171-171: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
181-181: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 YAMLlint (1.37.1)
holmes/plugins/toolsets/aks-node-health.yaml
[error] 10-10: trailing spaces
(trailing-spaces)
holmes/plugins/toolsets/aks.yaml
[error] 10-10: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Pre-commit checks
🔇 Additional comments (27)
holmes/plugins/toolsets/aks-node-health.yaml (4)
20-29: LGTM! Well-structured transformer configuration for node status.The
llm_summarizeconfiguration is appropriate with a reasonable threshold and a focused prompt that prioritizes problematic nodes while preserving important details like node names.
35-45: LGTM! Comprehensive transformer configuration for node description.The higher threshold of 1200 characters is appropriate for verbose node descriptions, and the prompt effectively covers all critical troubleshooting aspects.
61-71: LGTM! Effective transformer configuration for activity logs.The 1500 character threshold is appropriate for potentially verbose activity logs, and the prompt effectively focuses on operational issues while preserving crucial identifiers like correlation IDs.
102-112: LGTM! Practical transformer configuration for VMSS commands.The configuration effectively handles shell command outputs with appropriate focus on execution status, errors, and actionable findings.
holmes/core/transformers/base.py (2)
64-74: Consider the design choice for_validate_configmethod.The static analysis tool correctly identifies that
_validate_configis an empty method in an abstract base class without the@abstractmethoddecorator. However, this appears to be an intentional design choice to provide an optional validation hook that subclasses can override only when needed, rather than forcing all implementations to define it.This pattern is reasonable and provides flexibility. If you want to make the intent clearer, consider adding a comment in the docstring like "Subclasses may override this method to add validation logic."
1-84: LGTM! Well-designed abstract base class for transformers.The
BaseTransformerclass provides a clean and extensible interface for tool output transformers with:
- Clear abstract methods for transformation logic
- Optional configuration support
- Sensible defaults for the name property
- Comprehensive docstrings
holmes/plugins/toolsets/aks.yaml (4)
25-36: LGTM! Comprehensive transformer configuration for AKS cluster details.The 1500 character threshold is appropriate for detailed cluster configurations, and the prompt effectively covers all critical aspects including status, networking, security, and potential issues.
42-52: LGTM! Effective transformer configuration for cluster listings.The configuration appropriately handles multiple clusters with focus on comparison, version consistency, and error states. The grouping approach is particularly useful for large cluster lists.
58-69: LGTM! Well-structured transformer configuration for node pools.The configuration effectively summarizes node pool information with appropriate focus on scaling, resource allocation, and configuration details that matter for troubleshooting.
126-137: LGTM! Security-focused transformer configuration for NSG rules.The configuration effectively prioritizes security concerns with appropriate focus on permissive rules and AKS connectivity impact. The distinction between default and custom rules is particularly useful.
tests/core/test_transformer_backwards_compatibility.py (1)
1-254: LGTM! Comprehensive backwards compatibility test suite.This test suite effectively ensures that the new transformer feature maintains backwards compatibility with existing configurations and code. The tests cover all critical paths including Config loading, Toolset/Tool creation, and ToolsetManager initialization.
tests/integration/test_kubernetes_transformer_execution.py (1)
1-528: LGTM! Excellent integration test coverage for transformer execution.This test suite provides comprehensive integration testing of the transformer feature with Kubernetes tools, covering:
- Large vs small output handling
- Error scenarios and graceful failure handling
- Multiple transformer chaining
- Performance metrics logging
- Real YAML toolset loading and execution
The tests effectively validate the end-to-end transformer pipeline.
holmes/core/toolset_manager.py (3)
5-5: LGTM! Clean implementation of global transformer config support.The import additions and constructor parameter are properly typed and follow the existing patterns in the codebase.
Also applies to: 14-14, 32-32, 36-36
281-284: Good placement of global config application for CLI toolsets.Applying global transformer configs to CLI custom toolsets ensures consistent behavior across all toolset types.
448-471: Well-designed configuration inheritance implementation.The method correctly implements the configuration precedence hierarchy (tool > toolset > global) and handles edge cases gracefully. The two-phase merging ensures tools inherit updated toolset configs after global merge.
tests/core/transformers/test_llm_summarize.py (1)
29-320: Excellent test coverage for LLMSummarizeTransformer.The tests comprehensively cover:
- Initialization with various configurations
- Config validation for all parameters
- Conditional application based on thresholds
- Success and error scenarios
- Prompt construction and customization
- Edge cases like empty/whitespace responses
tests/core/test_config_transformers.py (2)
77-182: Well-structured tests for configuration inheritance.The tests correctly verify:
- Global configs are applied only when toolset configs are absent
- Existing toolset/tool configs are preserved (not overridden)
- Tool inheritance is properly delegated to
Toolset.preprocess_tools()The clear comments explaining the inheritance behavior are particularly helpful.
214-265: Excellent integration test for configuration inheritance priority.The test comprehensively validates the complete inheritance chain (global → toolset → tool) with clear assertions for each scenario. The test structure makes the expected behavior very clear.
tests/core/transformers/test_transformers.py (1)
50-329: Comprehensive test coverage for transformer infrastructure.The test suite provides excellent coverage:
- Abstract method enforcement
- Registry operations (register, unregister, create, list)
- Error handling and validation
- Instance isolation
- Integration scenarios
The mock transformers (
MockTransformer,ThresholdTransformer,FailingTransformer) effectively test different aspects of the transformer system.tests/core/transformers/test_validation.py (8)
1-17: LGTM!Import structure is clean and follows Python conventions with stdlib imports first, followed by project imports.
19-43: Well-designed mock transformers for testing!The mock classes effectively simulate both valid and invalid configuration scenarios needed for comprehensive validation testing.
45-53: LGTM!Simple and effective test for the custom exception class.
55-128: Comprehensive test coverage for transformer config validation!Excellent use of setup/teardown methods for test isolation and thorough coverage of edge cases including type errors, empty configs, and validation failures.
130-185: LGTM!Well-structured tests for list validation with appropriate coverage of edge cases and error index reporting.
187-215: LGTM!Focused test coverage for tool-specific transformer validation with proper error context verification.
217-263: Excellent testing of safe validation with logging verification!Good use of mocking to verify that warnings are properly logged with relevant context when validation fails.
265-322: Well-designed integration tests!Excellent coverage of complex scenarios and error propagation through the validation stack.
|
related: #522 |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (14)
holmes/plugins/toolsets/aks-node-health.yaml (1)
10-10: Remove trailing whitespace.Line 10 contains only trailing spaces that should be removed.
Apply this diff to remove the trailing spaces:
- +holmes/plugins/toolsets/aks.yaml (1)
10-10: Remove trailing whitespace.Line 10 contains only trailing spaces that should be removed.
Apply this diff to remove the trailing spaces:
- +tests/integration/test_kubernetes_transformer_execution.py (1)
509-509: Remove unused variable assignment.The
resultvariable is assigned but never used.Apply this diff:
- result = tool.invoke({}) + tool.invoke({})tests/integration/test_tool_execution_pipeline.py (5)
39-41: Avoid accessing private registry attributes.Accessing the private
_transformersattribute breaks encapsulation and could fail if the registry implementation changes.Consider storing the transformer class reference using public methods:
- # Store the original transformer class for restoration - self._original_llm_summarize = registry._transformers["llm_summarize"] + # Store the original transformer class for restoration + # Could use registry.create_transformer to verify it exists + try: + test_instance = registry.create_transformer("llm_summarize", {}) + self._original_llm_summarize = test_instance.__class__ + except Exception: + self._original_llm_summarize = None
123-126: Fix the original size calculation to match test data.The original size calculation doesn't match the actual test data structure (7 different entries × 20 repetitions).
- original_size = len( - "\n".join(["2024-01-01 10:00:00 INFO Starting application"] * 140) - ) + # Calculate actual original size based on test data + log_entries = [ + "2024-01-01 10:00:00 INFO Starting application", + "2024-01-01 10:00:01 INFO Loading configuration", + "2024-01-01 10:00:02 DEBUG Database connection established", + "2024-01-01 10:00:03 INFO Server listening on port 8080", + "2024-01-01 10:01:00 ERROR Failed to process request: timeout", + "2024-01-01 10:01:01 WARN Retrying request", + "2024-01-01 10:01:02 INFO Request processed successfully", + ] * 20 + original_size = len("\n".join(log_entries))
236-237: Make timing assertion more robust.The assertion
"in 0."assumes sub-second execution, which could fail on slow systems.- assert "in 0." in performance_log # Should show elapsed time + # Should show elapsed time in format "in X.XXXs" + assert " in " in performance_log and "s" in performance_log
278-278: Fix incorrect length comparison.The kubectl_output was already multiplied by 5, so multiplying again in the assertion is incorrect.
- assert len(result.data) < len(kubectl_output) * 5 # Much shorter than original + assert len(result.data) < len(kubectl_output) # Much shorter than original
298-300: Make assertion more deterministic.The OR condition makes the test less predictable. Since the threshold is 10 and the message is longer, it should always be summarized.
- assert ( - "SUMMARIZED:" in result.data - or "Important debugging information" in result.data - ) + # Message is longer than threshold (10), so it should be summarized + assert "SUMMARIZED:" in result.data + assert "Important debugging information" not in result.dataholmes/core/tools.py (2)
142-154: Simplify nested if statements in transformer validation.The static analysis correctly identifies that the nested if statements can be combined.
@model_validator(mode="after") def validate_transformers(self): """Validate transformer configurations during tool creation.""" - if self.transformer_configs is not None: - # Use safe validation to log warnings instead of failing - if not safe_validate_tool_transformer_configs( - self.name, self.transformer_configs - ): - # If validation fails, clear transforms to prevent runtime errors - logging.warning(f"Clearing invalid transforms for tool '{self.name}'") - self.transformer_configs = None + if self.transformer_configs is not None and not safe_validate_tool_transformer_configs( + self.name, self.transformer_configs + ): + # If validation fails, clear transforms to prevent runtime errors + logging.warning(f"Clearing invalid transforms for tool '{self.name}'") + self.transformer_configs = None return self
236-244: Remove unused size_change variable.The
size_changevariable is calculated but never used. Either use it in logging or remove it.# Let the transformer provide its own logging message if it wants to post_transform_size = len(transformed_data) -size_change = post_transform_size - pre_transform_size # Generic logging - transformers can override this with their own specific metrics logging.info( f"Applied transformer '{transformer_name}' to tool '{self.name}' output " - f"in {transform_elapsed:.2f}s (output size: {post_transform_size:,} characters)" + f"in {transform_elapsed:.2f}s (output size: {pre_transform_size:,} → {post_transform_size:,} characters)" )tests/core/test_tool_transformers.py (1)
655-656: Remove unused variable assignment.The
resultvariable is assigned but never used in this test method.- with patch("holmes.core.tools.logging") as mock_logging: - result = tool.invoke({}) + with patch("holmes.core.tools.logging") as mock_logging: + tool.invoke({})tests/core/transformers/test_transformers.py (1)
24-31: Simplify nested if statements in validation logic.The nested if statements can be combined for better readability, as identified by static analysis.
def _validate_config(self) -> None: - if "threshold" in self.config: - if ( - not isinstance(self.config["threshold"], int) - or self.config["threshold"] < 0 - ): - raise ValueError("Threshold must be a non-negative integer") + if "threshold" in self.config and ( + not isinstance(self.config["threshold"], int) + or self.config["threshold"] < 0 + ): + raise ValueError("Threshold must be a non-negative integer")docs/transformers.md (2)
119-127: Add language specification to fenced code blocks.The static analysis correctly identifies that these code blocks are missing language specifications for proper formatting.
-``` +```text Summarize this operational data focusing on: - What needs attention or immediate action - Group similar entries into a single line and description - Make sure to mention outliers, errors, and non-standard patterns - List normal/healthy patterns as aggregate descriptions - When listing problematic entries, also try to use aggregate descriptions when possible - When possible, mention exact keywords, IDs, or patterns so the user can filter/search the original data and drill down on the parts they care about -``` +```
151-187: Add language specification to output example code blocks.These example output blocks should have language specifications for proper formatting.
**Without Transformer:** -``` +```text NAME READY STATUS RESTARTS AGE IP NODE pod-1 1/1 Running 0 5d 10.1.1.1 node-1 pod-2 1/1 Running 0 5d 10.1.1.2 node-1 pod-3 1/1 Running 0 5d 10.1.1.3 node-2 pod-4 0/1 CrashLoopBackOff 15 1h 10.1.1.4 node-2 [... 100 more similar pods ...] -``` +``` **With Transformer:** -``` +```text Found 104 pods across 2 nodes: - 103 pods are healthy and running (age: 5d, on node-1 and node-2) - 1 pod in CrashLoopBackOff state: pod-4 (15 restarts, 1h old, IP 10.1.1.4, node-2) - Search with "grep pod-4" or "grep CrashLoopBackOff" to drill down on the problematic pod -``` +``` ### Log Analysis **Without Transformer:** -``` +```text 2024-01-15T10:30:01Z INFO Starting application... 2024-01-15T10:30:02Z INFO Database connection established 2024-01-15T10:30:03Z INFO Loading configuration... [... 1000 similar INFO logs ...] 2024-01-15T10:35:15Z ERROR Failed to connect to Redis: connection timeout 2024-01-15T10:35:16Z WARN Retrying Redis connection (attempt 1/3) -``` +``` **With Transformer:** -``` +```text Log analysis (2024-01-15 10:30-10:35): - 1000+ INFO messages showing normal application startup and operations - 1 ERROR: Redis connection timeout at 10:35:15Z - 1 WARN: Redis retry attempt at 10:35:16Z - Search with "grep ERROR" or "grep Redis" to investigate the connection issue -``` +```
🧹 Nitpick comments (3)
tests/core/test_transformer_backwards_compatibility.py (1)
94-99: Combine nested with statements for cleaner code.Apply this diff to combine the nested
withstatements:- with patch.dict("os.environ", test_env): - with patch( - "holmes.config.Config._Config__get_cluster_name" - ) as mock_cluster: - mock_cluster.return_value = "test-cluster" + with ( + patch.dict("os.environ", test_env), + patch("holmes.config.Config._Config__get_cluster_name") as mock_cluster + ): + mock_cluster.return_value = "test-cluster"tests/plugins/toolsets/test_aks_transformers.py (2)
15-25: Consider using a more robust path resolution method.The current path construction with multiple ".." is fragile and could break if the test file location changes. Consider using a utility function or constant for the base path.
- current_dir = os.path.dirname(os.path.abspath(__file__)) - aks_node_health_yaml_path = os.path.join( - current_dir, - "..", - "..", - "..", - "holmes", - "plugins", - "toolsets", - "aks-node-health.yaml", - ) + # Consider defining this as a class constant or utility function + from pathlib import Path + current_file = Path(__file__) + project_root = current_file.parents[3] # Go up 3 levels to project root + aks_node_health_yaml_path = project_root / "holmes" / "plugins" / "toolsets" / "aks-node-health.yaml" + aks_node_health_yaml_path = str(aks_node_health_yaml_path)
31-36: Simplify toolset and tool lookups using built-in functions.The manual iteration pattern can be simplified using
next()with a generator expression, which is more Pythonic and concise.- # Find the aks/node-health toolset - aks_node_health = None - for toolset in toolsets: - if toolset.name == "aks/node-health": - aks_node_health = toolset - break + # Find the aks/node-health toolset + aks_node_health = next((ts for ts in toolsets if ts.name == "aks/node-health"), None)Apply the same pattern for finding tools:
- check_node_status = None - for tool in aks_node_health.tools: - if tool.name == "check_node_status": - check_node_status = tool - break + check_node_status = next((tool for tool in aks_node_health.tools if tool.name == "check_node_status"), None)Also applies to: 40-45
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 8a2df1931ee254688ead2509b7dbfe56b4520822 and 412a0b4e5be80446443958856a86657eb2134ce5.
📒 Files selected for processing (32)
README.md(1 hunks)config.example.yaml(1 hunks)docs/transformers.md(1 hunks)holmes/config.py(6 hunks)holmes/core/tools.py(5 hunks)holmes/core/toolset_manager.py(7 hunks)holmes/core/transformers/__init__.py(1 hunks)holmes/core/transformers/base.py(1 hunks)holmes/core/transformers/llm_summarize.py(1 hunks)holmes/core/transformers/registry.py(1 hunks)holmes/core/transformers/validation.py(1 hunks)holmes/main.py(3 hunks)holmes/plugins/toolsets/aks-node-health.yaml(3 hunks)holmes/plugins/toolsets/aks.yaml(3 hunks)holmes/plugins/toolsets/kubernetes.yaml(3 hunks)holmes/plugins/toolsets/kubernetes_logs.yaml(2 hunks)holmes/utils/config_utils.py(1 hunks)tests/config_class/test_config_transformers.py(1 hunks)tests/core/test_config_transformers.py(1 hunks)tests/core/test_tool_transformers.py(1 hunks)tests/core/test_toolset_manager.py(1 hunks)tests/core/test_transformer_backwards_compatibility.py(1 hunks)tests/core/transformers/__init__.py(1 hunks)tests/core/transformers/test_llm_summarize.py(1 hunks)tests/core/transformers/test_transformers.py(1 hunks)tests/core/transformers/test_validation.py(1 hunks)tests/integration/test_config_merging_integration.py(1 hunks)tests/integration/test_kubernetes_transformer_execution.py(1 hunks)tests/integration/test_tool_execution_pipeline.py(1 hunks)tests/plugins/toolsets/test_aks_transformers.py(1 hunks)tests/plugins/toolsets/test_kubernetes_transformers.py(1 hunks)tests/utils/test_config_utils.py(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- config.example.yaml
- holmes/core/transformers/init.py
- holmes/core/transformers/llm_summarize.py
🚧 Files skipped from review as they are similar to previous changes (16)
- README.md
- tests/core/transformers/init.py
- holmes/utils/config_utils.py
- holmes/core/toolset_manager.py
- holmes/plugins/toolsets/kubernetes_logs.yaml
- holmes/core/transformers/validation.py
- holmes/config.py
- holmes/plugins/toolsets/kubernetes.yaml
- holmes/main.py
- holmes/core/transformers/registry.py
- tests/integration/test_config_merging_integration.py
- tests/plugins/toolsets/test_kubernetes_transformers.py
- tests/utils/test_config_utils.py
- tests/core/test_toolset_manager.py
- tests/core/test_config_transformers.py
- tests/config_class/test_config_transformers.py
🧰 Additional context used
🧠 Learnings (5)
tests/core/transformers/test_validation.py (1)
Learnt from: nherment
PR: #610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025-07-08T08:45:41.069Z
Learning: The robusta-dev/holmesgpt codebase has comprehensive existing validation for Azure environment variables (AZURE_API_BASE, AZURE_API_KEY, AZURE_API_VERSION) and MODEL in tests/llm/utils/classifiers.py, tests/llm/conftest.py, and holmes/core/llm.py. Don't suggest adding redundant validation logic.
tests/integration/test_tool_execution_pipeline.py (1)
Learnt from: Sheeproid
PR: #586
File: tests/llm/fixtures/test_ask_holmes/03_what_is_the_command_to_port_forward/test_case.yaml:4-4
Timestamp: 2025-07-02T10:27:17.231Z
Learning: In LLM-as-judge test cases for HolmesGPT, expected outputs should be descriptive rather than prescriptive when testing for flexible responses like port numbers. Using specific values in expected outputs can cause unnecessary test failures when the AI generates different but equally valid responses.
holmes/core/tools.py (3)
Learnt from: nherment
PR: #535
File: holmes/plugins/toolsets/bash/bash_toolset.py:207-209
Timestamp: 2025-06-24T05:51:04.543Z
Learning: The init_config method in toolsets should be idempotent - safely callable multiple times without errors. self.config should maintain consistent typing (not alternate between dict and config object types) throughout the object lifecycle.
Learnt from: nherment
PR: #610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025-07-08T08:45:41.069Z
Learning: The robusta-dev/holmesgpt codebase has comprehensive existing validation for Azure environment variables (AZURE_API_BASE, AZURE_API_KEY, AZURE_API_VERSION) and MODEL in tests/llm/utils/classifiers.py, tests/llm/conftest.py, and holmes/core/llm.py. Don't suggest adding redundant validation logic.
Learnt from: nherment
PR: #610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025-07-08T08:45:41.069Z
Learning: When suggesting improvements to environment variable handling in robusta-dev/holmesgpt, check first if validation logic already exists rather than reimplementing it.
tests/core/test_transformer_backwards_compatibility.py (1)
Learnt from: nherment
PR: #535
File: holmes/plugins/toolsets/bash/bash_toolset.py:207-209
Timestamp: 2025-06-24T05:51:04.543Z
Learning: The init_config method in toolsets should be idempotent - safely callable multiple times without errors. self.config should maintain consistent typing (not alternate between dict and config object types) throughout the object lifecycle.
tests/core/transformers/test_llm_summarize.py (1)
Learnt from: Sheeproid
PR: #586
File: tests/llm/fixtures/test_ask_holmes/03_what_is_the_command_to_port_forward/test_case.yaml:4-4
Timestamp: 2025-07-02T10:27:17.231Z
Learning: In LLM-as-judge test cases for HolmesGPT, expected outputs should be descriptive rather than prescriptive when testing for flexible responses like port numbers. Using specific values in expected outputs can cause unnecessary test failures when the AI generates different but equally valid responses.
🧬 Code Graph Analysis (5)
tests/plugins/toolsets/test_aks_transformers.py (3)
holmes/plugins/toolsets/__init__.py (1)
load_toolsets_from_file(48-62)holmes/core/transformers/base.py (1)
name(77-84)holmes/core/transformers/llm_summarize.py (1)
name(155-157)
holmes/core/tools.py (5)
holmes/core/transformers/validation.py (1)
safe_validate_tool_transformer_configs(108-126)holmes/core/transformers/base.py (4)
TransformerError(11-14)name(77-84)should_apply(52-62)transform(36-49)holmes/utils/config_utils.py (1)
merge_transformer_configs(8-69)holmes/core/transformers/llm_summarize.py (3)
name(155-157)should_apply(79-107)transform(109-152)holmes/core/transformers/registry.py (1)
create_transformer(56-81)
holmes/core/transformers/base.py (4)
holmes/core/transformers/llm_summarize.py (4)
_validate_config(62-77)transform(109-152)should_apply(79-107)name(155-157)tests/core/transformers/test_validation.py (5)
_validate_config(32-36)transform(22-23)transform(38-39)should_apply(25-26)should_apply(41-42)tests/core/test_tool_transformers.py (8)
transform(32-33)transform(465-466)transform(512-513)transform(555-556)should_apply(35-36)should_apply(468-469)should_apply(515-516)should_apply(558-560)tests/integration/test_tool_execution_pipeline.py (6)
transform(18-23)transform(132-133)transform(199-203)should_apply(25-28)should_apply(135-136)should_apply(205-206)
tests/core/transformers/test_llm_summarize.py (2)
holmes/core/transformers/llm_summarize.py (4)
LLMSummarizeTransformer(14-157)name(155-157)should_apply(79-107)transform(109-152)holmes/core/transformers/base.py (4)
TransformerError(11-14)name(77-84)should_apply(52-62)transform(36-49)
tests/integration/test_kubernetes_transformer_execution.py (6)
holmes/plugins/toolsets/__init__.py (1)
load_toolsets_from_file(48-62)holmes/core/tools.py (3)
StructuredToolResult(51-74)ToolResultStatus(29-48)invoke(162-184)holmes/core/transformers/base.py (4)
BaseTransformer(17-84)transform(36-49)should_apply(52-62)name(77-84)tests/core/test_tool_transformers.py (12)
transform(32-33)transform(465-466)transform(512-513)transform(555-556)should_apply(35-36)should_apply(468-469)should_apply(515-516)should_apply(558-560)test_transformer_failure_handling(460-506)FailingTransformer(464-469)test_multiple_transformers_chaining(508-549)SecondTransformer(511-516)tests/core/transformers/test_transformers.py (7)
transform(14-15)transform(32-33)transform(43-44)should_apply(17-18)should_apply(35-37)should_apply(46-47)FailingTransformer(40-47)holmes/core/transformers/registry.py (3)
register(20-39)is_registered(83-93)unregister(41-54)
🪛 Ruff (0.12.2)
tests/core/transformers/test_transformers.py
25-29: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
tests/core/test_tool_transformers.py
656-656: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
holmes/core/tools.py
145-149: Use a single if statement instead of nested if statements
(SIM102)
238-238: Local variable size_change is assigned to but never used
Remove assignment to unused variable size_change
(F841)
holmes/core/transformers/base.py
64-74: BaseTransformer._validate_config is an empty method in an abstract base class, but has no abstract decorator
(B027)
tests/core/test_transformer_backwards_compatibility.py
94-97: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
tests/core/transformers/test_llm_summarize.py
405-405: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
tests/integration/test_kubernetes_transformer_execution.py
509-509: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
🪛 markdownlint-cli2 (0.17.2)
docs/transformers.md
119-119: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
151-151: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
161-161: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
171-171: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
181-181: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 YAMLlint (1.37.1)
holmes/plugins/toolsets/aks-node-health.yaml
[error] 10-10: trailing spaces
(trailing-spaces)
holmes/plugins/toolsets/aks.yaml
[error] 10-10: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Pre-commit checks
🔇 Additional comments (29)
holmes/plugins/toolsets/aks-node-health.yaml (4)
20-29: Well-structured transformer configuration for node status summarization.The transformer configuration appropriately focuses on critical node health information with a reasonable threshold.
35-45: Comprehensive transformer configuration for node description.The configuration effectively targets key troubleshooting information from node descriptions with an appropriate threshold for typical kubectl describe output.
61-71: Effective transformer configuration for activity log analysis.The configuration appropriately targets administrative actions and failures with a suitable threshold for Azure Activity Log verbosity.
102-112: Practical transformer configuration for VMSS command output.The configuration effectively summarizes command execution results with focus on diagnostic information and actionable findings.
holmes/core/transformers/base.py (1)
1-85: Well-designed abstract base class for transformers.The base class provides a clean interface for tool output transformers with:
- Clear abstract methods for transformation logic
- Optional configuration validation pattern
- Appropriate exception type for error handling
- Sensible default name property
The empty
_validate_configmethod is intentionally non-abstract to allow transformers to optionally implement validation, as demonstrated by implementations likeLLMSummarizeTransformer.holmes/plugins/toolsets/aks.yaml (1)
25-137: Excellent transformer configurations for AKS toolset.All transformer configurations are well-designed with:
- Appropriate thresholds matching expected output sizes
- Comprehensive prompts targeting tool-specific information
- Strong focus on operational, security, and performance insights
- Clear guidance for preserving critical details during summarization
tests/core/test_transformer_backwards_compatibility.py (1)
1-254: Comprehensive backwards compatibility test coverage.The test suite thoroughly verifies that the transformer feature maintains backwards compatibility with:
- Existing configurations without transformer_configs
- Legacy toolsets and tools
- Configuration loading mechanisms
- Tool execution behavior
- Toolset manager initialization
Well-structured tests with appropriate mocking and clear assertions.
tests/integration/test_kubernetes_transformer_execution.py (1)
1-528: Excellent integration test coverage for transformer execution.The test suite provides comprehensive coverage of:
- Transformer application based on output size thresholds
- Error handling and graceful degradation
- Multiple transformer chaining
- Performance metric logging
- Real YAML toolset loading and execution
Well-structured tests with proper mocking, setup/teardown, and temporary file handling.
tests/core/transformers/test_transformers.py (6)
11-18: Well-structured mock transformer for testing.The
MockTransformerimplementation is clean and provides good test coverage for the base transformer functionality with a simple length-based condition.
40-48: Good error handling test mock.The
FailingTransformerprovides excellent coverage for error scenarios and will help ensure the system handles transformer failures gracefully.
53-123: Comprehensive base transformer test coverage.The
TestBaseTransformerclass provides excellent coverage of all core functionality including initialization, configuration validation, abstract method enforcement, and error handling scenarios.
125-247: Thorough registry functionality testing.The
TestTransformerRegistryclass covers all critical registry operations including registration/unregistration, validation, instance creation, and isolation between instances. The test coverage is comprehensive.
249-305: Excellent integration test scenarios.The integration tests validate end-to-end workflows and demonstrate proper interaction between transformers and the registry system. The multiple transformer instance test is particularly valuable.
307-329: Comprehensive error handling validation.The
TestTransformerErrorclass properly validates the custom exception behavior and integration with the registry system. The comment about transformer error handling at the tool execution level provides good context.tests/core/transformers/test_validation.py (7)
19-43: Well-designed mock transformers for validation testing.The mock transformer classes (
MockValidTransformerandMockConfigTransformer) are well-structured for testing different validation scenarios, including config requirements and validation failures.
45-53: Proper exception testing.The
TestTransformerValidationErrorclass correctly validates the custom validation exception behavior.
55-128: Comprehensive single config validation testing.The
TestValidateTransformerConfigclass provides thorough coverage of all validation scenarios including valid configs, invalid types, empty configs, multiple transformers, unknown transformers, and invalid transformer-specific configurations.
130-185: Excellent list validation coverage.The
TestValidateTransformsListclass (note: class name suggeststransforms_listbut teststransformer_configs) thoroughly tests validation of transformer configuration lists including edge cases and error scenarios with proper error indexing.
187-215: Good tool-specific validation testing.The
TestValidateToolTransformsclass properly tests tool-specific validation with appropriate error messaging that includes tool names for better debugging.
217-263: Robust safe validation testing with logging verification.The
TestSafeValidateToolTransformsclass excellently tests the safe validation functionality with proper logging verification using mocks. The tests ensure both return values and logging behavior are correct.
265-323: Excellent integration and error propagation testing.The
TestValidationIntegrationclass provides comprehensive integration testing with complex scenarios and validates proper error propagation through the call stack with detailed error messages.docs/transformers.md (8)
1-8: Clear and comprehensive overview.The introduction effectively explains the transformer concept and primary use case. The overview sets appropriate expectations for the feature.
11-33: Well-structured global configuration section.The configuration examples cover all deployment scenarios (CLI, environment variables, config file) with a helpful parameter table. The examples are practical and easy to follow.
34-96: Excellent tool-level configuration examples.The YAML and Python toolset examples are comprehensive and show both basic and advanced configuration patterns. The code examples are realistic and demonstrate proper usage.
97-128: Thorough LLM Summarize Transformer documentation.The behavior description and configuration options are clearly explained. The default prompt is well-designed for operational diagnostics.
129-145: Helpful guidance on transformer usage.The "When to Use Transformers" section provides clear guidance with practical examples of good and poor candidates. This will help users make informed decisions about when to apply transformers.
189-212: Valuable best practices guidance.The best practices section covers all critical aspects including prompt design, configuration, and tool integration. The advice is practical and actionable.
213-234: Comprehensive troubleshooting section.The troubleshooting guide addresses common issues users are likely to encounter with specific, actionable solutions. This will reduce support burden and improve user experience.
235-273: Clear migration guidance with examples.The migration guide provides practical before-and-after examples for both YAML and Python toolsets. The examples are realistic and easy to follow.
|
manual testing example: |
|
In some cases, the summarized output is larger than original content. Do we need to refine our default prompt to aviod this? Or do we need to return the original content directly? Thoughts? |
moshemorad
left a comment
moshemorad
left a comment
There was a problem hiding this comment.
Awesome work, left couple of comments. If something isn't clear please contact me and let's discuss it.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
holmes/config.py (1)
231-236: Same unused-import warning applies hereAdd
as _or# noqa: F401to keep the linter quiet.
🧹 Nitpick comments (10)
tests/core/test_transformer_backwards_compatibility.py (1)
105-109: Collapse nestedwithblocksRuff’s SIM117 warns here. Chain the contexts in a single
withto keep the test concise.tests/config_class/test_config_transformers.py (2)
24-30: Remove unusedTransformerimport
Transformeris imported only to satisfy forward-ref rebuild but never referenced, triggering Ruff F401.
Either drop the import or alias it:from holmes.core.tools import Transformer as _Transformer # noqa: F401.
335-338: AvoidRaw prints clutter test output; rely on assertions or logging capture instead.
holmes/config.py (1)
205-210: Silence unused-import warning
Transformeris imported only for its side-effect of resolving forward refs, so Ruff flags it unused.
Either alias it to_or add# noqa: F401to the import line.tests/integration/test_tool_execution_pipeline.py (1)
335-338: Drop debug printsLeaving debugging
tests/integration/test_config_merging_integration.py (1)
69-71: Avoid relying on the private_list_all_toolsetsAPI in integration testsUsing a leading-underscore method ties the test to internal implementation details and risks breakage on refactor.
Prefer the public surface (e.g.,ToolsetManager.list_toolsets()if exposed) or add a small public helper rather than reaching into privates.tests/core/test_tool_transformers.py (2)
171-178: Duplicate transformer registration across tests can race under xdistEach test manually
registry.register(MockTransformer)/unregister. Running test files in parallel may hit “already registered” errors.Define a
@pytest.fixture(scope="session", autouse=True)that handles one-time registration or useregistry.is_registeredguard with a unique name per test.
748-755: Patch the module-scoped logger, not the rootloggingmodule
with patch("logging.warning")patches the global logging function, possibly affecting unrelated threads/tests.
Patchholmes.core.tools.logging(as done elsewhere in the file) for isolation:-with patch("logging.warning") as mock_warning: +with patch("holmes.core.tools.logging") as mock_warning:tests/core/test_config_transformers.py (1)
10-13: RebuildingConfigmodel inside the test is brittleInjecting
Transformerinto the module namespace and callingConfig.model_rebuild()tightly couples the test to pydantic internals.Prefer importing
Transformerin the production module or exposing a helper to rebuild models centrally, keeping tests simpler and forward-compatible.tests/core/transformers/test_transformers.py (1)
29-32: Nameconfigfield inThresholdTransformermay shadowBaseModel.config()in Pydantic v1While Pydantic v2 dropped the
.config()method, codebases still using v1 or mixing versions can face accidental shadowing.
Consider renaming the field (e.g.,settings) for future-proofing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 412a0b4e5be80446443958856a86657eb2134ce5 and 66406b4ae117f977de0c6813fce9471d02cde419.
📒 Files selected for processing (27)
README.md(1 hunks)config.example.yaml(1 hunks)docs/transformers.md(1 hunks)holmes/config.py(7 hunks)holmes/core/tools.py(6 hunks)holmes/core/toolset_manager.py(8 hunks)holmes/core/transformers/__init__.py(1 hunks)holmes/core/transformers/base.py(1 hunks)holmes/core/transformers/llm_summarize.py(1 hunks)holmes/core/transformers/registry.py(1 hunks)holmes/main.py(3 hunks)holmes/plugins/toolsets/aks-node-health.yaml(2 hunks)holmes/plugins/toolsets/aks.yaml(3 hunks)holmes/plugins/toolsets/kubernetes.yaml(3 hunks)holmes/plugins/toolsets/kubernetes_logs.yaml(2 hunks)holmes/utils/config_utils.py(1 hunks)tests/config_class/test_config_transformers.py(1 hunks)tests/core/test_config_transformers.py(1 hunks)tests/core/test_tool_transformers.py(1 hunks)tests/core/test_toolset_manager.py(1 hunks)tests/core/test_transformer_backwards_compatibility.py(1 hunks)tests/core/transformers/__init__.py(1 hunks)tests/core/transformers/test_llm_summarize.py(1 hunks)tests/core/transformers/test_transformers.py(1 hunks)tests/integration/test_config_merging_integration.py(1 hunks)tests/integration/test_kubernetes_transformer_execution.py(1 hunks)tests/integration/test_tool_execution_pipeline.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (17)
- tests/core/transformers/init.py
- README.md
- config.example.yaml
- holmes/main.py
- holmes/utils/config_utils.py
- holmes/core/transformers/init.py
- holmes/plugins/toolsets/kubernetes_logs.yaml
- holmes/plugins/toolsets/aks-node-health.yaml
- holmes/plugins/toolsets/aks.yaml
- tests/core/test_toolset_manager.py
- holmes/core/transformers/llm_summarize.py
- tests/integration/test_kubernetes_transformer_execution.py
- tests/core/transformers/test_llm_summarize.py
- holmes/core/toolset_manager.py
- holmes/core/transformers/registry.py
- holmes/plugins/toolsets/kubernetes.yaml
- docs/transformers.md
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.py: ALWAYS place Python imports at the top of the file, not inside functions or methods
Use Ruff for formatting and linting (configured in pyproject.toml)
Type hints required (mypy configuration in pyproject.toml)
Files:
tests/integration/test_tool_execution_pipeline.pyholmes/core/transformers/base.pytests/core/test_config_transformers.pytests/core/test_tool_transformers.pytests/integration/test_config_merging_integration.pytests/core/transformers/test_transformers.pyholmes/core/tools.pyholmes/config.pytests/config_class/test_config_transformers.pytests/core/test_transformer_backwards_compatibility.py
🧠 Learnings (10)
📓 Common learnings
Learnt from: nilo19
PR: robusta-dev/holmesgpt#695
File: holmes/core/transformers/registry.py:9-19
Timestamp: 2025-08-08T06:15:30.763Z
Learning: holmes/core/transformers/registry.py: TransformerRegistry is intentionally single-threaded and not designed to be thread-safe; avoid proposing locks unless multi-threaded access is introduced later.
📚 Learning: 2025-08-06T08:36:24.917Z
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-06T08:36:24.917Z
Learning: New toolsets require integration tests with mocks
Applied to files:
tests/integration/test_tool_execution_pipeline.pytests/core/test_config_transformers.pytests/core/test_tool_transformers.pytests/integration/test_config_merging_integration.pytests/core/transformers/test_transformers.pytests/config_class/test_config_transformers.pytests/core/test_transformer_backwards_compatibility.py
📚 Learning: 2025-08-08T06:15:30.763Z
Learnt from: nilo19
PR: robusta-dev/holmesgpt#695
File: holmes/core/transformers/registry.py:9-19
Timestamp: 2025-08-08T06:15:30.763Z
Learning: holmes/core/transformers/registry.py: TransformerRegistry is intentionally single-threaded and not designed to be thread-safe; avoid proposing locks unless multi-threaded access is introduced later.
Applied to files:
tests/integration/test_tool_execution_pipeline.pyholmes/core/transformers/base.pytests/core/test_tool_transformers.pytests/core/transformers/test_transformers.pyholmes/core/tools.pyholmes/config.pytests/core/test_transformer_backwards_compatibility.py
📚 Learning: 2025-07-02T10:27:17.231Z
Learnt from: Sheeproid
PR: robusta-dev/holmesgpt#586
File: tests/llm/fixtures/test_ask_holmes/03_what_is_the_command_to_port_forward/test_case.yaml:4-4
Timestamp: 2025-07-02T10:27:17.231Z
Learning: In LLM-as-judge test cases for HolmesGPT, expected outputs should be descriptive rather than prescriptive when testing for flexible responses like port numbers. Using specific values in expected outputs can cause unnecessary test failures when the AI generates different but equally valid responses.
Applied to files:
tests/integration/test_tool_execution_pipeline.pytests/config_class/test_config_transformers.py
📚 Learning: 2025-08-06T08:36:24.917Z
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-06T08:36:24.917Z
Learning: All new features require unit tests
Applied to files:
tests/integration/test_tool_execution_pipeline.pytests/core/test_tool_transformers.pytests/core/transformers/test_transformers.pytests/config_class/test_config_transformers.py
📚 Learning: 2025-07-08T08:45:41.069Z
Learnt from: nherment
PR: robusta-dev/holmesgpt#610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025-07-08T08:45:41.069Z
Learning: When suggesting improvements to environment variable handling in robusta-dev/holmesgpt, check first if validation logic already exists rather than reimplementing it.
Applied to files:
tests/integration/test_tool_execution_pipeline.pyholmes/core/tools.pytests/config_class/test_config_transformers.py
📚 Learning: 2025-07-17T20:00:38.391Z
Learnt from: moshemorad
PR: robusta-dev/holmesgpt#391
File: holmes/common/env_vars.py:25-25
Timestamp: 2025-07-17T20:00:38.391Z
Learning: For the robusta-dev/holmesgpt repository, do not flag FBT003 (boolean positional value in function call) as an issue. The team is okay with boolean positional arguments in function calls.
Applied to files:
tests/integration/test_tool_execution_pipeline.pytests/config_class/test_config_transformers.py
📚 Learning: 2025-06-24T05:51:04.543Z
Learnt from: nherment
PR: robusta-dev/holmesgpt#535
File: holmes/plugins/toolsets/bash/bash_toolset.py:207-209
Timestamp: 2025-06-24T05:51:04.543Z
Learning: The init_config method in toolsets should be idempotent - safely callable multiple times without errors. self.config should maintain consistent typing (not alternate between dict and config object types) throughout the object lifecycle.
Applied to files:
tests/core/test_config_transformers.pytests/core/test_tool_transformers.pytests/integration/test_config_merging_integration.pyholmes/core/tools.pytests/core/test_transformer_backwards_compatibility.py
📚 Learning: 2025-07-08T08:45:41.069Z
Learnt from: nherment
PR: robusta-dev/holmesgpt#610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025-07-08T08:45:41.069Z
Learning: The robusta-dev/holmesgpt codebase has comprehensive existing validation for Azure environment variables (AZURE_API_BASE, AZURE_API_KEY, AZURE_API_VERSION) and MODEL in tests/llm/utils/classifiers.py, tests/llm/conftest.py, and holmes/core/llm.py. Don't suggest adding redundant validation logic.
Applied to files:
holmes/core/tools.pytests/config_class/test_config_transformers.py
📚 Learning: 2025-06-05T12:23:27.634Z
Learnt from: nherment
PR: robusta-dev/holmesgpt#436
File: tests/llm/utils/mock_utils.py:240-249
Timestamp: 2025-06-05T12:23:27.634Z
Learning: The holmesgpt project uses Python >= 3.10 and prefers modern type hint syntax like `list[str]`, `dict[str, int]` over importing equivalent types from the typing module like `List[str]`, `Dict[str, int]`.
Applied to files:
holmes/core/tools.py
🧬 Code Graph Analysis (3)
holmes/core/transformers/base.py (1)
holmes/core/transformers/llm_summarize.py (3)
transform(102-145)should_apply(73-100)name(148-150)
tests/core/transformers/test_transformers.py (2)
holmes/core/transformers/base.py (5)
BaseTransformer(17-62)TransformerError(11-14)transform(26-39)should_apply(42-52)name(55-62)holmes/core/transformers/registry.py (7)
TransformerRegistry(9-118)register(20-46)is_registered(95-105)list_transformers(107-114)unregister(48-61)create_transformer(63-93)clear(116-118)
tests/core/test_transformer_backwards_compatibility.py (3)
holmes/config.py (4)
Config(71-529)toolset_manager(128-137)load_from_file(194-227)load_from_env(230-267)holmes/core/toolset_manager.py (2)
ToolsetManager(23-485)_apply_global_transformers(460-485)holmes/core/tools.py (10)
Toolset(487-643)Tool(175-324)Transformer(151-172)preprocess_tools(537-562)StructuredToolResult(70-93)_invoke(319-320)_invoke(371-399)get_parameterized_one_liner(323-324)get_parameterized_one_liner(350-357)invoke(222-246)
🪛 Ruff (0.12.2)
holmes/config.py
206-206: holmes.core.tools.Transformer imported but unused
Remove unused import: holmes.core.tools.Transformer
(F401)
232-232: holmes.core.tools.Transformer imported but unused
Remove unused import: holmes.core.tools.Transformer
(F401)
tests/config_class/test_config_transformers.py
26-26: holmes.core.tools.Transformer imported but unused
Remove unused import: holmes.core.tools.Transformer
(F401)
154-154: holmes.core.tools.Transformer imported but unused
Remove unused import: holmes.core.tools.Transformer
(F401)
tests/core/test_transformer_backwards_compatibility.py
105-108: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Pre-commit checks
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
tests/config_class/test_config_transformers.py (3)
25-30: Remove unused forward-ref scaffolding; keep just model_rebuild if neededThe inline import is unused and triggers Ruff F401. Pydantic resolves the forward reference via Config internals; you can drop the import and keep model_rebuild for clarity.
- # Import Transformer class to resolve forward reference - from holmes.core.tools import Transformer - - # Rebuild the model to resolve forward references - Config.model_rebuild() + # Rebuild the model to resolve forward references (Config internally imports Transformer) + Config.model_rebuild()
31-35: Also assert transformers field presence and defaultThe test name/docstring implies verifying all transformer-related fields. Add checks for the transformers attribute too.
# Test default values config = Config() assert hasattr(config, "fast_model") assert config.fast_model is None + assert hasattr(config, "transformers") + assert config.transformers is None
154-158: Unused import: removeTransformer(Ruff F401)The import here isn’t used in this test. Removing it will satisfy Ruff without changing behavior.
- from holmes.core.tools import Transformer - - # Rebuild the model to resolve forward references - Config.model_rebuild() + # Rebuild the model to resolve forward references (not strictly necessary here) + Config.model_rebuild()tests/integration/test_tool_execution_pipeline.py (2)
97-98: Docstring doesn’t match behaviorThe test uses a single transformer. Either add another transformer in sequence or adjust the docstring for accuracy.
- """Test Python tool with multiple transformers in sequence.""" + """Test Python tool with a summarization transformer."""
335-339: Remove debug prints and assert truncation marker for determinismDrop noisy prints and add an explicit check for the truncation marker to assert summarization happened, while still preserving the keyword presence assertion (aligns with PR objective to keep searchable terms).
- # Transformation applied but structure preserved - print(f"DEBUG: Raw result.data = {repr(result.data)}") - print(f"DEBUG: result.data length = {len(result.data)}") - assert "SUMMARIZED:" in result.data - assert "Important debugging information" in result.data + # Transformation applied but structure preserved + assert "SUMMARIZED:" in result.data + assert "(truncated " in result.data + # Key phrase preserved to keep searchability + assert "Important debugging information" in result.dataholmes/core/tools.py (2)
233-244: Simplify:hasattrnot needed; always a StructuredToolResult
transformed_resultis always a StructuredToolResult, so you can call get_stringified_data() directly.- output_str = ( - transformed_result.get_stringified_data() - if hasattr(transformed_result, "get_stringified_data") - else str(transformed_result) - ) + output_str = transformed_result.get_stringified_data()
246-314: Confirm intent: transformers replace structureddatawith string summariesCurrent logic stringifies, transforms, and then overwrites
datawith the transformed string. If a tool returns structured data (dict/model), this loses structure for downstream consumers.Options:
- Only transform when
isinstance(result.data, str), skipping for structured payloads.- Or preserve original structure and store summary in a new field (e.g.,
summary) while leavingdataintact.If you want, I can draft a minimal patch implementing the first option.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 66406b4ae117f977de0c6813fce9471d02cde419 and c86c06ba808a34b0e2d02ca435be16476be1f0c5.
📒 Files selected for processing (5)
docs/transformers.md(1 hunks)holmes/core/tools.py(6 hunks)holmes/core/transformers/base.py(1 hunks)tests/config_class/test_config_transformers.py(1 hunks)tests/integration/test_tool_execution_pipeline.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- holmes/core/transformers/base.py
- docs/transformers.md
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.py: ALWAYS place Python imports at the top of the file, not inside functions or methods
Use Ruff for formatting and linting (configured in pyproject.toml)
Type hints required (mypy configuration in pyproject.toml)
Files:
tests/integration/test_tool_execution_pipeline.pyholmes/core/tools.pytests/config_class/test_config_transformers.py
🧠 Learnings (11)
📓 Common learnings
Learnt from: nilo19
PR: robusta-dev/holmesgpt#695
File: holmes/core/transformers/registry.py:9-19
Timestamp: 2025-08-08T06:15:30.763Z
Learning: holmes/core/transformers/registry.py: TransformerRegistry is intentionally single-threaded and not designed to be thread-safe; avoid proposing locks unless multi-threaded access is introduced later.
📚 Learning: 2025-08-06T08:36:24.917Z
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-06T08:36:24.917Z
Learning: New toolsets require integration tests with mocks
Applied to files:
tests/integration/test_tool_execution_pipeline.pytests/config_class/test_config_transformers.py
📚 Learning: 2025-08-08T06:15:30.763Z
Learnt from: nilo19
PR: robusta-dev/holmesgpt#695
File: holmes/core/transformers/registry.py:9-19
Timestamp: 2025-08-08T06:15:30.763Z
Learning: holmes/core/transformers/registry.py: TransformerRegistry is intentionally single-threaded and not designed to be thread-safe; avoid proposing locks unless multi-threaded access is introduced later.
Applied to files:
tests/integration/test_tool_execution_pipeline.pyholmes/core/tools.py
📚 Learning: 2025-07-02T10:27:17.231Z
Learnt from: Sheeproid
PR: robusta-dev/holmesgpt#586
File: tests/llm/fixtures/test_ask_holmes/03_what_is_the_command_to_port_forward/test_case.yaml:4-4
Timestamp: 2025-07-02T10:27:17.231Z
Learning: In LLM-as-judge test cases for HolmesGPT, expected outputs should be descriptive rather than prescriptive when testing for flexible responses like port numbers. Using specific values in expected outputs can cause unnecessary test failures when the AI generates different but equally valid responses.
Applied to files:
tests/integration/test_tool_execution_pipeline.pytests/config_class/test_config_transformers.py
📚 Learning: 2025-08-06T08:36:24.917Z
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-06T08:36:24.917Z
Learning: All new features require unit tests
Applied to files:
tests/integration/test_tool_execution_pipeline.pytests/config_class/test_config_transformers.py
📚 Learning: 2025-07-08T08:45:41.069Z
Learnt from: nherment
PR: robusta-dev/holmesgpt#610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025-07-08T08:45:41.069Z
Learning: When suggesting improvements to environment variable handling in robusta-dev/holmesgpt, check first if validation logic already exists rather than reimplementing it.
Applied to files:
tests/integration/test_tool_execution_pipeline.pyholmes/core/tools.pytests/config_class/test_config_transformers.py
📚 Learning: 2025-07-17T20:00:38.391Z
Learnt from: moshemorad
PR: robusta-dev/holmesgpt#391
File: holmes/common/env_vars.py:25-25
Timestamp: 2025-07-17T20:00:38.391Z
Learning: For the robusta-dev/holmesgpt repository, do not flag FBT003 (boolean positional value in function call) as an issue. The team is okay with boolean positional arguments in function calls.
Applied to files:
tests/integration/test_tool_execution_pipeline.pytests/config_class/test_config_transformers.py
📚 Learning: 2025-07-08T08:45:41.069Z
Learnt from: nherment
PR: robusta-dev/holmesgpt#610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025-07-08T08:45:41.069Z
Learning: The robusta-dev/holmesgpt codebase has comprehensive existing validation for Azure environment variables (AZURE_API_BASE, AZURE_API_KEY, AZURE_API_VERSION) and MODEL in tests/llm/utils/classifiers.py, tests/llm/conftest.py, and holmes/core/llm.py. Don't suggest adding redundant validation logic.
Applied to files:
tests/integration/test_tool_execution_pipeline.pyholmes/core/tools.pytests/config_class/test_config_transformers.py
📚 Learning: 2025-06-24T05:51:04.543Z
Learnt from: nherment
PR: robusta-dev/holmesgpt#535
File: holmes/plugins/toolsets/bash/bash_toolset.py:207-209
Timestamp: 2025-06-24T05:51:04.543Z
Learning: The init_config method in toolsets should be idempotent - safely callable multiple times without errors. self.config should maintain consistent typing (not alternate between dict and config object types) throughout the object lifecycle.
Applied to files:
holmes/core/tools.py
📚 Learning: 2025-08-06T08:36:24.917Z
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-06T08:36:24.917Z
Learning: Applies to **/*.py : ALWAYS place Python imports at the top of the file, not inside functions or methods
Applied to files:
holmes/core/tools.py
📚 Learning: 2025-06-05T12:23:27.634Z
Learnt from: nherment
PR: robusta-dev/holmesgpt#436
File: tests/llm/utils/mock_utils.py:240-249
Timestamp: 2025-06-05T12:23:27.634Z
Learning: The holmesgpt project uses Python >= 3.10 and prefers modern type hint syntax like `list[str]`, `dict[str, int]` over importing equivalent types from the typing module like `List[str]`, `Dict[str, int]`.
Applied to files:
holmes/core/tools.py
🧬 Code Graph Analysis (2)
holmes/core/tools.py (3)
holmes/core/transformers/base.py (5)
TransformerError(11-14)BaseTransformer(17-62)name(55-62)should_apply(42-52)transform(26-39)holmes/utils/config_utils.py (1)
merge_transformers(11-80)holmes/core/transformers/registry.py (2)
is_registered(95-105)create_transformer(63-93)
tests/config_class/test_config_transformers.py (3)
holmes/config.py (3)
Config(71-529)load_from_file(194-227)load_from_env(230-267)holmes/core/tools.py (1)
Transformer(151-170)holmes/core/transformers/llm_summarize.py (1)
name(148-150)
🪛 Ruff (0.12.2)
tests/config_class/test_config_transformers.py
26-26: holmes.core.tools.Transformer imported but unused
Remove unused import: holmes.core.tools.Transformer
(F401)
154-154: holmes.core.tools.Transformer imported but unused
Remove unused import: holmes.core.tools.Transformer
(F401)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Pre-commit checks
🔇 Additional comments (4)
tests/config_class/test_config_transformers.py (1)
195-201: LGTM: preserves existing transformers when fast_model is setGood assertion that auto-generation does not override explicit configs. Matches
_auto_generate_transformerscontract.tests/integration/test_tool_execution_pipeline.py (1)
45-61: Great use of public registry API and clean setup/teardownCapturing the original class via create_transformer and unregistering/registering through public methods keeps tests resilient to registry internals.
holmes/core/tools.py (2)
285-292: Nice: detailed size delta in logsGood visibility into pre/post sizes and delta. This will help diagnose summarizer efficacy and catch regressions.
545-556: Merging toolset-level and tool-level transformers: solid application of merge_transformersMerging at validation-time keeps runtime lean and respects override precedence. Looks good.
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (1)
holmes/plugins/toolsets/kubernetes.yaml (1)
76-79: Inconsistency: kubectl_get_yaml lacks a transformer but summary claims it was addedThe AI summary says kubectl_get_yaml was augmented with llm_summarize, but the code shows no transformers block here. If intended, align with others:
- name: "kubectl_get_yaml" description: "Run `kubectl get -o yaml` on a single Kubernetes resource" command: "kubectl get -o yaml {{ kind }} {{ name}}{% if namespace %} -n {{ namespace }}{% endif %}" + transformers: + - name: llm_summarize + config: + input_threshold: 1000 + prompt: | + Summarize this kubectl YAML focusing on: + - Conditions/status, errors/warnings, and reasons + - Key spec fields likely to affect behavior (env, resources, selectors, probes, images) + - Only include diffs/anomalies and omit defaults/boilerplate + - Provide exact field paths (e.g., spec.containers[].image) for grep/drill-down
♻️ Duplicate comments (4)
tests/config_class/test_config_transformers.py (2)
61-80: LGTM: env-backed config loadingFAST_MODEL and MODEL are properly asserted via load_from_env. Matches our earlier guidance to avoid source inspection.
127-141: Good: behavior-based env var test for load_from_envThis addresses past feedback by testing actual behavior rather than inspecting source.
holmes/core/tools.py (2)
245-259: Include percentage change in output-size loggingAdd percentage to improve observability (also reuses size_change). This was suggested earlier and remains valuable.
elapsed = time.time() - start_time output_str = ( transformed_result.get_stringified_data() if hasattr(transformed_result, "get_stringified_data") else str(transformed_result) ) show_hint = f"/show {tool_number}" if tool_number else "/show" line_count = output_str.count("\n") + 1 if output_str else 0 - logging.info( - f" [dim]Finished {tool_number_str}in {elapsed:.2f}s, output length: {len(output_str):,} characters ({line_count:,} lines) - {show_hint} to view contents[/dim]" - ) + logging.info( + f" [dim]Finished {tool_number_str}in {elapsed:.2f}s, output length: {len(output_str):,} characters " + f"({line_count:,} lines) - {show_hint} to view contents[/dim]" + )
323-327: Structured data loss when replacing data with string summaryReplacing data with transformed text discards original structure (dict/model). Consider storing the summary in a separate field (e.g., summary) or only transforming when data is str.
🧹 Nitpick comments (9)
tests/config_class/test_config_transformers.py (1)
181-199: Optional: strengthen preservation assertionCurrent check uses equality; consider identity check to prove the list wasn’t copied or mutated in place.
- assert config.transformers == existing_configs + assert config.transformers is existing_configs + assert [t.name for t in config.transformers] == ["custom_transformer"]holmes/core/toolset_manager.py (3)
38-46: Remove redundant assignment to self.toolsetsInitialize once with a single expression.
- self.toolsets = toolsets - self.toolsets = toolsets or {} + self.toolsets = toolsets or {}
452-458: Remove duplicate assignment when adding new toolsetsLine 457 repeats the assignment; safe to drop.
else: existing_toolsets_by_name[new_toolset.name] = new_toolset - existing_toolsets_by_name[new_toolset.name] = new_toolset
459-592: Move local imports and unify logger per guidelines; avoid function-scope importsProject guidelines require imports at top of file. Also reuse a module-level logger instead of re-defining.
- import logging - from holmes.core.transformers import registry - - logger = logging.getLogger(__name__) + # at module top: + # import logging + # from holmes.core.transformers import registry + # logger = logging.getLogger(__name__)If a top-level import triggers cycles, keep the runtime import but add a comment explaining why and suppress via a noqa, otherwise please move them to the top-level.
If you suspect circular imports with a top-level registry import, ping and I’ll propose an alternative injection path that uses Toolset/Tool factory hooks.
holmes/utils/config_utils.py (1)
11-16: Follow import-at-top guideline; avoid function-scope importMove the runtime import of Transformer to the top to match repository guidelines.
+from holmes.core.tools import Transformer @@ - # Create new transformer with merged config - from holmes.core.tools import Transformer - merged_transformer = Transformer( name=transformer_name, config=merged_config )If this introduces a cycle in your environment, add an explanatory comment and a targeted noqa; otherwise, keep imports at top per guidelines.
If you want, I can provide a quick script to detect potential import cycles in the affected modules.
Also applies to: 42-50, 75-81
holmes/config.py (2)
147-149: LGTM: auto-generate llm_summarize when only fast_model is providedAuto-generation avoids forcing users to touch YAML. Consider moving the runtime import of Transformer to the module top per guidelines (if no cycles).
- if self.fast_model and not self.transformers: - from holmes.core.tools import Transformer + if self.fast_model and not self.transformers: self.transformers = [ - Transformer( + Transformer( name="llm_summarize", config={ "fast_model": self.fast_model, }, ) ]Also applies to: 169-188
205-209: Remove stale comments about importsThese comments reference importing Transformer, but we only need model_rebuild() here. Trim for clarity.
- # Import Transformer class to resolve forward reference - # Rebuild the model to resolve forward references cls.model_rebuild()Also applies to: 229-234
holmes/core/transformers/llm_summarize.py (1)
80-87: Reduce log verbosity on normal initSwitching the “created fast LLM instance” log from info to debug avoids noisy logs in normal paths.
holmes/core/tools.py (1)
188-197: Move inner import out; use the module-level loggingThere’s a module-level import for logging already. Remove the inner import and keep using the module-level symbol.
- def model_post_init(self, __context) -> None: - """Initialize transformer instances once during tool creation for better performance.""" - import logging - + def model_post_init(self, __context) -> None: + """Initialize transformer instances once during tool creation for better performance.""" logger = logging.getLogger(__name__)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between c86c06ba808a34b0e2d02ca435be16476be1f0c5 and 41cf2bd915098a72b00a2e951e56bd1ba023dff9.
📒 Files selected for processing (9)
holmes/config.py(7 hunks)holmes/core/tools.py(6 hunks)holmes/core/toolset_manager.py(8 hunks)holmes/core/transformers/llm_summarize.py(1 hunks)holmes/plugins/toolsets/kubernetes.yaml(4 hunks)holmes/utils/config_utils.py(1 hunks)tests/config_class/test_config_transformers.py(1 hunks)tests/integration/test_config_merging_integration.py(1 hunks)tests/integration/test_tool_execution_pipeline.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration/test_tool_execution_pipeline.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.py: ALWAYS place Python imports at the top of the file, not inside functions or methods
Use Ruff for formatting and linting (configured in pyproject.toml)
Type hints required (mypy configuration in pyproject.toml)
Pre-commit hooks enforce quality checks
Don't add convenience logs that give away the problem
Don't write logs that directly state the issue
Ensure historical timestamps are properly handled in logs (especially with Loki)
Files:
holmes/core/toolset_manager.pytests/config_class/test_config_transformers.pyholmes/utils/config_utils.pyholmes/core/transformers/llm_summarize.pyholmes/core/tools.pyholmes/config.pytests/integration/test_config_merging_integration.py
tests/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Tests must match source structure under tests/
Files:
tests/config_class/test_config_transformers.pytests/integration/test_config_merging_integration.py
holmes/plugins/toolsets/**/*.yaml
📄 CodeRabbit Inference Engine (CLAUDE.md)
holmes/plugins/toolsets/**/*.yaml: Toolsets must be located at holmes/plugins/toolsets/{name}.yaml or {name}/
When configuring toolsets in toolsets.yaml files, ALL toolset-specific configuration must go under a config field
The only valid top-level fields for toolsets in YAML are: enabled, name, description, additional_instructions, prerequisites, tools, docs_url, icon_url, installation_instructions, config, url (for MCP toolsets only)
Files:
holmes/plugins/toolsets/kubernetes.yaml
**/*.yaml
📄 CodeRabbit Inference Engine (CLAUDE.md)
ALWAYS use Secrets for scripts, not inline manifests or ConfigMaps (prevents code visibility with kubectl describe)
Files:
holmes/plugins/toolsets/kubernetes.yaml
🧠 Learnings (6)
📚 Learning: 2025-08-10T06:02:54.308Z
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-10T06:02:54.308Z
Learning: Applies to holmes/plugins/toolsets/**/*.yaml : Toolsets must be located at holmes/plugins/toolsets/{name}.yaml or {name}/
Applied to files:
holmes/core/toolset_manager.py
📚 Learning: 2025-07-08T08:45:41.069Z
Learnt from: nherment
PR: robusta-dev/holmesgpt#610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025-07-08T08:45:41.069Z
Learning: The robusta-dev/holmesgpt codebase has comprehensive existing validation for Azure environment variables (AZURE_API_BASE, AZURE_API_KEY, AZURE_API_VERSION) and MODEL in tests/llm/utils/classifiers.py, tests/llm/conftest.py, and holmes/core/llm.py. Don't suggest adding redundant validation logic.
Applied to files:
tests/config_class/test_config_transformers.py
📚 Learning: 2025-07-08T08:45:41.069Z
Learnt from: nherment
PR: robusta-dev/holmesgpt#610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025-07-08T08:45:41.069Z
Learning: When suggesting improvements to environment variable handling in robusta-dev/holmesgpt, check first if validation logic already exists rather than reimplementing it.
Applied to files:
tests/config_class/test_config_transformers.py
📚 Learning: 2025-08-08T06:15:30.763Z
Learnt from: nilo19
PR: robusta-dev/holmesgpt#695
File: holmes/core/transformers/registry.py:9-19
Timestamp: 2025-08-08T06:15:30.763Z
Learning: holmes/core/transformers/registry.py: TransformerRegistry is intentionally single-threaded and not designed to be thread-safe; avoid proposing locks unless multi-threaded access is introduced later.
Applied to files:
holmes/core/tools.py
📚 Learning: 2025-06-05T12:23:27.634Z
Learnt from: nherment
PR: robusta-dev/holmesgpt#436
File: tests/llm/utils/mock_utils.py:240-249
Timestamp: 2025-06-05T12:23:27.634Z
Learning: The holmesgpt project uses Python >= 3.10 and prefers modern type hint syntax like `list[str]`, `dict[str, int]` over importing equivalent types from the typing module like `List[str]`, `Dict[str, int]`.
Applied to files:
holmes/core/tools.py
📚 Learning: 2025-08-10T06:02:54.308Z
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-10T06:02:54.308Z
Learning: New toolsets require integration tests
Applied to files:
tests/integration/test_config_merging_integration.py
🧬 Code Graph Analysis (7)
holmes/core/toolset_manager.py (4)
holmes/core/tools.py (2)
check_prerequisites(590-645)Toolset(500-656)holmes/core/transformers/llm_summarize.py (1)
name(176-178)holmes/core/transformers/base.py (1)
name(55-62)holmes/core/transformers/registry.py (1)
create_transformer(63-93)
tests/config_class/test_config_transformers.py (3)
holmes/config.py (3)
Config(71-527)load_from_file(194-226)load_from_env(229-265)holmes/core/transformers/llm_summarize.py (1)
name(176-178)holmes/core/tools.py (1)
Transformer(151-170)
holmes/utils/config_utils.py (3)
holmes/core/tools.py (1)
Transformer(151-170)holmes/core/transformers/llm_summarize.py (1)
name(176-178)holmes/core/transformers/base.py (1)
name(55-62)
holmes/core/transformers/llm_summarize.py (3)
holmes/core/transformers/base.py (5)
BaseTransformer(17-62)TransformerError(11-14)should_apply(42-52)transform(26-39)name(55-62)holmes/core/llm.py (2)
DefaultLLM(60-256)LLM(29-57)holmes/config.py (1)
model_post_init(139-148)
holmes/core/tools.py (3)
holmes/core/transformers/base.py (5)
TransformerError(11-14)BaseTransformer(17-62)name(55-62)should_apply(42-52)transform(26-39)holmes/core/transformers/llm_summarize.py (4)
name(176-178)model_post_init(65-93)should_apply(95-128)transform(130-173)holmes/core/transformers/registry.py (2)
is_registered(95-105)create_transformer(63-93)
holmes/config.py (3)
holmes/core/tools.py (1)
Transformer(151-170)holmes/core/transformers/llm_summarize.py (1)
name(176-178)holmes/core/transformers/base.py (1)
name(55-62)
tests/integration/test_config_merging_integration.py (5)
holmes/core/tools.py (5)
YAMLTool(340-479)YAMLToolset(659-668)ToolsetTag(133-136)Transformer(151-170)check_prerequisites(590-645)holmes/config.py (1)
toolset_manager(128-137)holmes/core/toolset_manager.py (2)
ToolsetManager(22-591)_list_all_toolsets(73-142)holmes/core/transformers/llm_summarize.py (1)
name(176-178)holmes/core/transformers/base.py (1)
name(55-62)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Pre-commit checks
🔇 Additional comments (15)
tests/config_class/test_config_transformers.py (6)
15-34: LGTM: default fields and forward-ref rebuild coveredBehavior-based assertions for fast_model default and forward-ref rebuild are fine. No issues.
45-59: LGTM: file-backed config loadingCovers YAML path, values, and cleanup. Good isolation with delete=False + unlink in finally.
82-101: LGTM: CLI overrides take precedenceThe precedence of CLI fast_model over file-configured fast_model is verified.
103-125: LGTM: backward compatibilityAsserting defaults and older config schema behavior is correct and future-proofing.
143-163: LGTM: auto-generation when fast_model is providedCovers llm_summarize auto-generation with the configured fast_model.
201-221: LGTM: CLI fast_model triggers auto-generationCovers the CLI-based generation path, consistent with Config._auto_generate_transformers.
holmes/core/toolset_manager.py (1)
126-133: Good: apply fast_model injection before prereq checksInjecting into final toolset list pre-prereq ensures consistent downstream behavior with or without caching.
Please confirm that _inject_fast_model_into_transformers only adds global_fast_model when "fast_model" is absent on the transformer. Current logic looks correct for this guarantee.
holmes/utils/config_utils.py (1)
11-41: Clarify semantics of only_merge_when_override_existsCurrent implementation only skips base when override_transformers is entirely None/empty. If the intended behavior is “apply base only for transformer types that also exist in overrides,” we should gate per-transformer. Otherwise, current behavior matches the docstring.
Would you like this function to drop base-only transformer types when overrides exist but omit that specific type? If yes, I’ll send a patch that filters base-only names out when overrides are present.
holmes/config.py (2)
71-88: LGTM: new fields (fast_model, transformers) added correctlyForward ref to Transformer is handled via model_rebuild in loaders. Defaults make sense.
127-136: Propagating fast_model to ToolsetManager is correctglobal_fast_model=self.fast_model enables cross-toolset injection where per-transformer fast_model is missing.
holmes/plugins/toolsets/kubernetes.yaml (1)
11-14: Good documentation noteClear explanation of when and why summarization happens. No issues.
holmes/core/tools.py (1)
164-171: Transformer registry validation looks goodLightweight validation with graceful degradation if unregistered; aligns with registry’s design. No change needed.
tests/integration/test_config_merging_integration.py (3)
54-87: Solid CLI fast-model injection testGood end-to-end coverage of global_fast_model injection into tool transformers.
138-152: Three-level precedence test looks correctVerifies tool override > toolset > global with fast_model propagation. LGTM.
296-346: Add a test for non-expanding fallback (mock a growing summary)Once the non-expanding fallback is implemented, add a test that mocks llm_summarize.transform to return a longer string and asserts the original output is preserved.
@@ def test_toolset_with_only_tool_level_transformers_gets_fast_model(): @@ global_fast_model = "gpt-4o-mini" @@ - with patch("holmes.core.toolset_manager.load_builtin_toolsets") as mock_load: + with patch("holmes.core.toolset_manager.load_builtin_toolsets") as mock_load, \ + patch("holmes.core.transformers.registry.create_transformer") as mock_create: mock_load.return_value = [toolset_without_toolset_transformers] + # Mock a transformer instance whose transform expands the content + class GrowingTransformer: + name = "llm_summarize" + def should_apply(self, input_text: str) -> bool: return True + def transform(self, input_text: str) -> str: return input_text + "\n" + ("X" * 1000) + mock_create.return_value = GrowingTransformer() @@ toolsets = manager._list_all_toolsets(check_prerequisites=False) @@ - # This assertion should now PASS, proving the issue is fixed + # This assertion should now PASS, proving the issue is fixed assert "global_fast_model" in config_dict["llm_summarize"], ( @@ + # And the runtime should keep original data if the summary expanded + # (Would require invoking the tool and observing unchanged output)If you’d like, I can wire a small functional test that invokes a dummy tool and asserts the non-expansion fallback.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (4)
holmes/core/tools.py (3)
300-307: Enhance transformer logging with percentage change.Include percent change for quick diagnostics (earlier feedback also suggested this).
post_transform_size = len(transformed_data) size_change = post_transform_size - pre_transform_size - logging.info( - f"Applied transformer '{transformer_instance.name}' to tool '{self.name}' output " - f"in {transform_elapsed:.2f}s (size: {pre_transform_size:,} → {post_transform_size:,} chars, " - f"change: {size_change:+,})" - ) + pct_change = ( + (size_change / pre_transform_size * 100.0) if pre_transform_size > 0 else 0.0 + ) + logging.info( + f"Applied transformer '{transformer_instance.name}' to tool '{self.name}' output " + f"in {transform_elapsed:.2f}s (size: {pre_transform_size:,} → {post_transform_size:,} chars, " + f"Δ {size_change:+,} chars, {pct_change:+.1f}%)" + )
275-279: Avoid clobbering structured outputs (preserve non-string data).Current logic stringifies any data and replaces
datawith a string, losing structure for dicts/models. Consider skipping transformation unlessdatais already a string; alternatively, store the transformed text in a separate field.- original_data = result.get_stringified_data() + # Only transform when the original payload is textual; preserve structured data + if result.data is not None and not isinstance(result.data, str): + logging.debug( + f"Skipping transformers for tool '{self.name}': non-string data (preserving structure)" + ) + return result + original_data = result.get_stringified_data()If you prefer to always run summarizers, consider adding a new field (e.g.,
summary) to StructuredToolResult and keepdataunchanged.
322-327: Add non-expanding fallback for llm_summarize.Per PR discussion, summaries can sometimes exceed original size. Add a guard to keep the original when that happens.
- if transformers_applied: - # Create a copy of the result with transformed data - result_dict = result.model_dump(exclude={"data"}) - result_dict["data"] = transformed_data - return StructuredToolResult(**result_dict) + if transformers_applied: + final_size = len(transformed_data) + original_size = len(original_data) + # If llm_summarize expanded the content, keep the original to avoid regressions + if final_size > original_size and any(name == "llm_summarize" for name in transformers_applied): + logging.info( + f"Skipping expanded summary for tool '{self.name}': {original_size:,} → {final_size:,} (keeping original)" + ) + return result + # Create a copy of the result with transformed data + result_dict = result.model_dump(exclude={"data"}) + result_dict["data"] = transformed_data + return StructuredToolResult(**result_dict)Optionally gate via config (e.g., only_if_smaller: true) on the summarizer.
tests/integration/test_config_merging_integration.py (1)
274-276: Avoid brittle BaseModel equality in tests; compare configs instead.Direct equality on Pydantic models is flaky. Compare normalized dicts by name.
- # Should remain unchanged - assert result_toolset.transformers == toolset_configs + # Should remain unchanged + assert result_toolset.transformers is not None + got = {t.name: t.config for t in result_toolset.transformers} + exp = {t.name: t.config for t in toolset_configs} + assert got == exp
🧹 Nitpick comments (10)
tests/core/test_toolset_manager.py (1)
421-448: LGTM: scope-limited injection to llm_summarize onlyConfirms non-llm transformers are untouched. Consider also adding a case where llm_summarize already has fast_model to assert no global_fast_model is added.
+def test_no_injection_when_llm_has_existing_fast_model(): + from holmes.core.tools import Transformer + toolset = YAMLToolset( + name="test_toolset", + tags=[ToolsetTag.CORE], + description="Test toolset", + transformers=[Transformer(name="llm_summarize", config={"fast_model": "existing"})], + ) + manager = ToolsetManager(global_fast_model="gpt-4o-mini") + manager._inject_fast_model_into_transformers([toolset]) + assert "global_fast_model" not in toolset.transformers[0].config + assert toolset.transformers[0].config["fast_model"] == "existing"tests/core/test_transformer_backwards_compatibility.py (1)
53-61: Misleading test name: it asserts injection, not “unchanged”Rename to reflect intent.
-def test_existing_toolsets_with_transformers_unchanged(self): - """Test that existing toolsets with transformers get global_fast_model injection.""" +def test_existing_toolsets_with_transformers_get_injection(self): + """Test that existing toolsets with transformers get global_fast_model injection."""holmes/config.py (2)
23-28: Use a concrete type instead of a string forward-ref for transformersSince Transformer can be imported at module scope safely, prefer a concrete annotation. This also lets you delete the TYPE_CHECKING-only import.
-if TYPE_CHECKING: - from holmes.core.llm import LLM - from holmes.core.supabase_dal import SupabaseDal - from holmes.core.tool_calling_llm import IssueInvestigator, ToolCallingLLM - from holmes.core.tools import Transformer +if TYPE_CHECKING: + from holmes.core.llm import LLM + from holmes.core.supabase_dal import SupabaseDal + from holmes.core.tool_calling_llm import IssueInvestigator, ToolCallingLLMAnd update the field:
- transformers: Optional[List["Transformer"]] = None + transformers: Optional[List[Transformer]] = NonePlease confirm there’s no circular import between holmes.core.tools and holmes.config. If there is, we can switch to validated assignment with dicts or a local import in a dedicated module initializer instead.
205-209: Stale comments about forward-ref resolutionThe “Import Transformer class to resolve forward reference” comments are no longer accurate if you import Transformer at module level. Remove to avoid confusion; cls.model_rebuild() is still fine to keep.
- # Import Transformer class to resolve forward reference - - # Rebuild the model to resolve forward references + # Rebuild the model to resolve forward references cls.model_rebuild()And similarly in load_from_env.
Also applies to: 228-234
tests/core/test_config_transformers.py (1)
127-141: Extend env test to assert auto-generation when FAST_MODEL is setSince model_post_init auto-generates transformers when fast_model is present and transformers is None, assert that here too.
with patch.dict(os.environ, {"FAST_MODEL": "test-model"}): config = Config.load_from_env() assert config.fast_model == "test-model" + # Auto-generation should kick in + assert config.transformers is not None + assert len(config.transformers) == 1 + assert config.transformers[0].name == "llm_summarize" + assert config.transformers[0].config["fast_model"] == "test-model"holmes/core/tools.py (2)
248-253: Simplify: StructuredToolResult always has get_stringified_data().The hasattr guard is unnecessary and adds noise.
- output_str = ( - transformed_result.get_stringified_data() - if hasattr(transformed_result, "get_stringified_data") - else str(transformed_result) - ) + output_str = transformed_result.get_stringified_data()
181-186: Prefer modern built-in generics for new annotations (Python ≥3.10).New fields can adopt
list[...] | Noneinstead ofOptional[List[...]]to match project style.- transformers: Optional[List[Transformer]] = None + transformers: list[Transformer] | None = None @@ - _transformer_instances: Optional[List["BaseTransformer"]] = PrivateAttr( + _transformer_instances: list["BaseTransformer"] | None = PrivateAttr( default=None ) @@ - transformers: Optional[List[Transformer]] = None + transformers: list[Transformer] | None = NoneAlso applies to: 526-526, 184-186
tests/integration/test_tool_execution_pipeline.py (2)
99-124: Use built-in generics for type hints (Python ≥3.10).Switch
Dicttodictfor consistency with project style.- def _invoke(self, params: Dict) -> StructuredToolResult: + def _invoke(self, params: dict) -> StructuredToolResult: @@ - def get_parameterized_one_liner(self, params: Dict) -> str: + def get_parameterized_one_liner(self, params: dict) -> str:
337-339: Remove debug prints from tests.The prints add noise to CI logs and aren't needed.
- print(f"DEBUG: Raw result.data = {repr(result.data)}") - print(f"DEBUG: result.data length = {len(result.data)}")tests/config_class/test_config_transformers.py (1)
25-29: Remove stale comment (no Transformer import here).This comment suggests importing Transformer but none is imported in this block. It can be removed to avoid confusion.
- # Import Transformer class to resolve forward reference - - # Rebuild the model to resolve forward references - Config.model_rebuild() + # Rebuild the model to resolve forward references + Config.model_rebuild()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 41cf2bd915098a72b00a2e951e56bd1ba023dff9 and 489f9ad6b13eeb2ede952d9363de7b5110ab0de2.
📒 Files selected for processing (11)
holmes/config.py(7 hunks)holmes/core/tools.py(6 hunks)holmes/core/toolset_manager.py(8 hunks)holmes/core/transformers/llm_summarize.py(1 hunks)holmes/utils/config_utils.py(1 hunks)tests/config_class/test_config_transformers.py(1 hunks)tests/core/test_config_transformers.py(1 hunks)tests/core/test_toolset_manager.py(1 hunks)tests/core/test_transformer_backwards_compatibility.py(1 hunks)tests/integration/test_config_merging_integration.py(1 hunks)tests/integration/test_tool_execution_pipeline.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- holmes/core/toolset_manager.py
- holmes/core/transformers/llm_summarize.py
- holmes/utils/config_utils.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.py: ALWAYS place Python imports at the top of the file, not inside functions or methods
Use Ruff for formatting and linting (configured in pyproject.toml)
Type hints required (mypy configuration in pyproject.toml)
Pre-commit hooks enforce quality checks
Don't add convenience logs that give away the problem
Don't write logs that directly state the issue
Ensure historical timestamps are properly handled in logs (especially with Loki)
Files:
tests/core/test_config_transformers.pyholmes/core/tools.pytests/core/test_transformer_backwards_compatibility.pytests/core/test_toolset_manager.pytests/integration/test_tool_execution_pipeline.pytests/config_class/test_config_transformers.pytests/integration/test_config_merging_integration.pyholmes/config.py
tests/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Tests must match source structure under tests/
Files:
tests/core/test_config_transformers.pytests/core/test_transformer_backwards_compatibility.pytests/core/test_toolset_manager.pytests/integration/test_tool_execution_pipeline.pytests/config_class/test_config_transformers.pytests/integration/test_config_merging_integration.py
🧠 Learnings (5)
📚 Learning: 2025-08-08T06:15:30.763Z
Learnt from: nilo19
PR: robusta-dev/holmesgpt#695
File: holmes/core/transformers/registry.py:9-19
Timestamp: 2025-08-08T06:15:30.763Z
Learning: holmes/core/transformers/registry.py: TransformerRegistry is intentionally single-threaded and not designed to be thread-safe; avoid proposing locks unless multi-threaded access is introduced later.
Applied to files:
holmes/core/tools.pytests/integration/test_tool_execution_pipeline.py
📚 Learning: 2025-06-05T12:23:27.634Z
Learnt from: nherment
PR: robusta-dev/holmesgpt#436
File: tests/llm/utils/mock_utils.py:240-249
Timestamp: 2025-06-05T12:23:27.634Z
Learning: The holmesgpt project uses Python >= 3.10 and prefers modern type hint syntax like `list[str]`, `dict[str, int]` over importing equivalent types from the typing module like `List[str]`, `Dict[str, int]`.
Applied to files:
holmes/core/tools.py
📚 Learning: 2025-08-10T06:02:54.308Z
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-10T06:02:54.308Z
Learning: New toolsets require integration tests
Applied to files:
tests/core/test_toolset_manager.pytests/integration/test_config_merging_integration.py
📚 Learning: 2025-07-08T08:45:41.069Z
Learnt from: nherment
PR: robusta-dev/holmesgpt#610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025-07-08T08:45:41.069Z
Learning: The robusta-dev/holmesgpt codebase has comprehensive existing validation for Azure environment variables (AZURE_API_BASE, AZURE_API_KEY, AZURE_API_VERSION) and MODEL in tests/llm/utils/classifiers.py, tests/llm/conftest.py, and holmes/core/llm.py. Don't suggest adding redundant validation logic.
Applied to files:
tests/config_class/test_config_transformers.py
📚 Learning: 2025-07-08T08:45:41.069Z
Learnt from: nherment
PR: robusta-dev/holmesgpt#610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025-07-08T08:45:41.069Z
Learning: When suggesting improvements to environment variable handling in robusta-dev/holmesgpt, check first if validation logic already exists rather than reimplementing it.
Applied to files:
tests/config_class/test_config_transformers.py
🧬 Code Graph Analysis (6)
holmes/core/tools.py (5)
holmes/core/transformers/base.py (5)
TransformerError(11-14)BaseTransformer(17-62)name(55-62)should_apply(42-52)transform(26-39)holmes/utils/config_utils.py (1)
merge_transformers(11-91)holmes/core/transformers/llm_summarize.py (4)
name(176-178)model_post_init(65-93)should_apply(95-128)transform(130-173)holmes/core/transformers/registry.py (2)
is_registered(95-105)create_transformer(63-93)holmes/plugins/toolsets/git.py (1)
error(404-409)
tests/core/test_toolset_manager.py (3)
holmes/core/tools.py (4)
Transformer(151-170)YAMLToolset(659-668)ToolsetTag(133-136)check_prerequisites(590-645)holmes/core/transformers/llm_summarize.py (1)
name(176-178)holmes/core/toolset_manager.py (2)
_inject_fast_model_into_transformers(459-591)_list_all_toolsets(73-142)
tests/integration/test_tool_execution_pipeline.py (5)
holmes/core/tools.py (6)
Tool(173-337)YAMLTool(340-479)StructuredToolResult(70-93)ToolResultStatus(48-67)Transformer(151-170)invoke(235-259)holmes/core/transformers/base.py (5)
BaseTransformer(17-62)TransformerError(11-14)transform(26-39)should_apply(42-52)name(55-62)holmes/core/transformers/llm_summarize.py (3)
transform(130-173)should_apply(95-128)name(176-178)tests/core/test_tool_transformers.py (12)
transform(27-28)transform(374-375)transform(425-426)transform(475-476)should_apply(30-31)should_apply(377-378)should_apply(428-429)should_apply(478-480)name(34-35)name(381-382)name(432-433)name(483-484)holmes/core/transformers/registry.py (4)
is_registered(95-105)create_transformer(63-93)unregister(48-61)register(20-46)
tests/config_class/test_config_transformers.py (3)
holmes/config.py (3)
Config(71-527)load_from_file(194-226)load_from_env(229-265)holmes/core/transformers/llm_summarize.py (1)
name(176-178)holmes/core/tools.py (1)
Transformer(151-170)
tests/integration/test_config_merging_integration.py (2)
holmes/core/tools.py (5)
YAMLTool(340-479)YAMLToolset(659-668)ToolsetTag(133-136)Transformer(151-170)check_prerequisites(590-645)holmes/core/toolset_manager.py (1)
_list_all_toolsets(73-142)
holmes/config.py (4)
holmes/core/tools.py (1)
Transformer(151-170)holmes/core/transformers/base.py (1)
name(55-62)tests/integration/test_tool_execution_pipeline.py (3)
name(38-39)name(167-168)name(239-240)tests/integration/test_kubernetes_transformer_execution.py (3)
name(31-32)name(294-295)name(400-401)
🪛 Ruff (0.12.2)
tests/core/test_transformer_backwards_compatibility.py
119-122: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Pre-commit checks
🔇 Additional comments (21)
tests/core/test_toolset_manager.py (4)
350-381: LGTM: verifies injection into existing llm_summarize configs while preserving other keysAsserts global_fast_model injection and preserves prompt/input_threshold. Looks correct.
382-397: LGTM: no injection when toolset has no transformersCorrectly exercises the early-exit path.
399-419: LGTM: no injection without a global_fast_modelRightly ensures no mutation occurs when manager has no fast model.
450-483: LGTM: global_fast_model flows through to LLMSummarizeTransformer and is consumed correctly
- Verified in
holmes/core/transformers/llm_summarize.pythat
effective_fast_model = self.fast_model or self.global_fast_model
and that a fast LLM instance is created when onlyglobal_fast_modelis set.- Integration test in
test_list_all_toolsets_applies_fast_model_injectionalready confirms the injection and preserves original config.Approved.
tests/core/test_transformer_backwards_compatibility.py (7)
11-14: Forward-ref resolution setup is fineSeeding Transformer in module globals and calling Config.model_rebuild() is a pragmatic way to keep tests isolated.
93-114: LGTM: file-based config backward compatibilityMocks load properly; asserts no transformers field required. Good coverage.
115-130: LGTM: env-based config backward compatibilityCovers private name-mangled patch for cluster name; verifies new fields absent by default.
131-185: LGTM: Toolset.preprocess_tools inheritance still works with Transformer objectsSolid coverage of inheritance and non-overwrite semantics.
186-211: LGTM: YAML toolsets without transformers still instantiate cleanlyEnsures legacy YAMLs remain valid.
240-261: LGTM: Tool.execute flow remains unchanged without transformersValidates no-regression in basic execution path.
262-283: LGTM: ToolsetManager constructor remains backward compatibleCovers old/new usages for global_fast_model.
holmes/config.py (1)
135-136: Wiring global_fast_model to ToolsetManager: LGTMThis is the right propagation point for CLI/env-provided fast_model.
tests/core/test_config_transformers.py (9)
15-34: LGTM: field presence and defaultsAsserts fast_model exists and defaults to None. Good.
36-59: LGTM: file-based loadingCovers YAML parsing and merging.
61-80: LGTM: env-based loadingVerifies FAST_MODEL and MODEL ingestion paths.
82-101: LGTM: CLI overrides file valuesCorrect precedence behavior.
103-125: LGTM: backward compatibility without new fieldsEnsures old configs still work.
143-163: LGTM: auto-generation when fast_model is providedAsserts transformer name and config mapping. Good.
165-179: LGTM: no auto-generation without fast_modelCovers negative path.
181-199: LGTM: respects existing transformers and doesn’t overrideCovers preservation semantics.
201-221: LGTM: CLI-provided fast_model triggers auto-generationValidates the no-file path as used by CLI.
|
@moshemorad can you please check again? |
|
@moshemorad do you have further comments and suggestions? |
|
The evals failed due to /home/runner/work/_temp/4e9d851b-e595-44b8-a105-8caa99be5076.sh: line 4: poetry: command not found |
@nilo19 done review and approved. |
|
@nilo19 might be some issue with installation. rerunning it. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
holmes/plugins/toolsets/kubernetes.yaml (1)
89-129: Memory unit conversion: remove invalid ‘m’ unit, handle scientific notation robustly, warn on unknown units.In Kubernetes, ‘m’ is for CPU, not memory. Also broaden the scientific-notation regex and emit a stderr warning for unknown units; treat unknowns as bytes fallback. Apply similarly to the namespaced variant below.
awk ' function convert_to_mib(value) { - if (value ~ /^[0-9]+e[0-9]+$/) return (value + 0) / (1024 * 1024); # Scientific notation - if (value ~ /m$/) return (value + 0) / (1024^2 * 1000); # Millibytes (m) + # Accept integers/floats (optional scientific notation) as bytes + if (value ~ /^[0-9]+(\.[0-9]+)?([eE][+-]?[0-9]+)?$/) + return (value + 0) / (1024 * 1024); + + v = tolower(value) - if (value ~ /Ei$/) return (value + 0) * 1024^6 / (1024^2); # Binary units - if (value ~ /Pi$/) return (value + 0) * 1024^5 / (1024^2); - if (value ~ /Ti$/) return (value + 0) * 1024^4 / (1024^2); - if (value ~ /Gi$/) return (value + 0) * 1024^3 / (1024^2); - if (value ~ /Mi$/) return (value + 0); - if (value ~ /Ki$/) return (value + 0) / 1024; - if (value ~ /E$/) return (value + 0) * 1000^6 / (1024^2); # Decimal units - if (value ~ /P$/) return (value + 0) * 1000^5 / (1024^2); - if (value ~ /T$/) return (value + 0) * 1000^4 / (1024^2); - if (value ~ /G$/) return (value + 0) * 1000^3 / (1024^2); - if (value ~ /M$/) return (value + 0) * 1000^2 / (1024^2); - if (value ~ /k$/) return (value + 0) * 1000 / (1024^2); + if (v ~ /ei$/) return (v + 0) * 1024^6 / (1024^2); # Binary units + if (v ~ /pi$/) return (v + 0) * 1024^5 / (1024^2); + if (v ~ /ti$/) return (v + 0) * 1024^4 / (1024^2); + if (v ~ /gi$/) return (v + 0) * 1024^3 / (1024^2); + if (v ~ /mi$/) return (v + 0); + if (v ~ /ki$/) return (v + 0) / 1024; + if (v ~ /e$/) return (v + 0) * 1000^6 / (1024^2); # Decimal units + if (v ~ /p$/) return (v + 0) * 1000^5 / (1024^2); + if (v ~ /t$/) return (v + 0) * 1000^4 / (1024^2); + if (v ~ /g$/) return (v + 0) * 1000^3 / (1024^2); + if (v ~ /m$/) return (v + 0) * 1000^2 / (1024^2); + if (v ~ /k$/) return (v + 0) * 1000 / (1024^2); + if (v ~ /[a-z]+$/) { + print "Warning: unknown memory unit: " value > "/dev/stderr"; + } return (value + 0) / (1024 * 1024); # Default: bytes }Note: Please mirror this change in the kubectl_memory_requests_namespace variant for consistency.
🧹 Nitpick comments (1)
holmes/plugins/toolsets/kubernetes.yaml (1)
173-191: jq prompt: minor structure tweak for scanability (optional).Consider starting with a 1–2 line “Summary” and ending with “Next steps” (max 3 bullets) to improve operator triage; cap total bullets to ~6.
prompt: | Summarize this jq query output focusing on: + - Begin with a 1–2 line Summary - Key patterns and commonalities in the data - Notable outliers, anomalies, or items that need attention - Group similar results into aggregate descriptions when possible - Highlight any empty results, null values, or missing data - When applicable, mention specific resource names, namespaces, or values that stand out - Organize findings in a structured way that helps with troubleshooting - Be concise: aim for ≤ 50% of the original text; prioritize aggregates and actionable outliers - Include grep-ready keys/values; avoid repeating entire objects or unchanged defaults + - End with Next steps (≤ 3 bullets); total bullets ≤ ~6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
holmes/config.py(4 hunks)holmes/main.py(3 hunks)holmes/plugins/toolsets/kubernetes.yaml(4 hunks)holmes/plugins/toolsets/kubernetes_logs.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- holmes/main.py
- holmes/plugins/toolsets/kubernetes_logs.yaml
- holmes/config.py
🧰 Additional context used
📓 Path-based instructions (1)
holmes/plugins/toolsets/**
📄 CodeRabbit inference engine (CLAUDE.md)
Toolsets must live under holmes/plugins/toolsets as either {name}.yaml or a {name}/ directory
Files:
holmes/plugins/toolsets/kubernetes.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Pre-commit checks
- GitHub Check: llm_evals
🔇 Additional comments (2)
holmes/plugins/toolsets/kubernetes.yaml (2)
24-37: Prompt looks good; concise + grep-friendly constraints added.The added bullets should curb expansion and keep summaries actionable.
46-58: LGTM on namespace “get” prompt; non-expansion guidance is clear.
|
@moshemorad ready for mergine. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
holmes/core/tools.py (3)
181-216: Consider using more specific exception handling.While the current broad exception handling ensures robustness during initialization, consider catching more specific exceptions for better debugging.
Apply this diff to improve exception specificity:
try: # Create transformer instance once and cache it transformer_instance = registry.create_transformer( transformer.name, transformer.config ) self._transformer_instances.append(transformer_instance) logger.debug( f"Initialized transformer '{transformer.name}' for tool '{self.name}'" ) - except Exception as e: + except (KeyError, TransformerError) as e: logger.warning( f"Failed to initialize transformer '{transformer.name}' for tool '{self.name}': {e}" ) # Continue with other transformers, don't fail the entire initialization continue + except Exception as e: + logger.error( + f"Unexpected error initializing transformer '{transformer.name}' for tool '{self.name}': {e}", + exc_info=True + ) + continue
441-445: Use logging.exception for better error tracking.When catching exceptions, use
logging.exceptionto preserve the full stack trace.except subprocess.CalledProcessError as e: - logger.error( + logger.exception( f"Failed to apply additional instructions: {self.additional_instructions}. " f"Error: {e.stderr}" ) return f"Error applying additional instructions: {e.stderr}"
565-649: Consider extracting transformer conversion logic to reduce duplication.The transformer conversion logic is duplicated for toolset and tool levels. Consider extracting it to a helper method.
Extract the duplicated transformer conversion logic:
+ @staticmethod + def _convert_transformers_to_objects(transformers_list): + """Convert raw dict transformers to Transformer objects.""" + if not transformers_list: + return None + + converted = [] + for t in transformers_list: + if isinstance(t, dict): + try: + transformer_obj = Transformer(**t) + # Check if transformer is registered + if not registry.is_registered(transformer_obj.name): + logger.warning( + f"Invalid transformer configuration: Transformer '{transformer_obj.name}' is not registered" + ) + continue + converted.append(transformer_obj) + except Exception as e: + logger.warning(f"Invalid transformer configuration: {e}") + continue + else: + # Already a Transformer object + converted.append(t) + return converted if converted else None @model_validator(mode="before") def preprocess_tools(cls, values): additional_instructions = values.get("additional_instructions", "") transformers = values.get("transformers", None) tools_data = values.get("tools", []) - # Convert raw dict transformers to Transformer objects BEFORE merging - if transformers: - converted_transformers = [] - for t in transformers: - if isinstance(t, dict): - try: - transformer_obj = Transformer(**t) - # Check if transformer is registered - from holmes.core.transformers import registry - - if not registry.is_registered(transformer_obj.name): - logger.warning( - f"Invalid toolset transformer configuration: Transformer '{transformer_obj.name}' is not registered" - ) - continue # Skip invalid transformer - converted_transformers.append(transformer_obj) - except Exception as e: - # Log warning and skip invalid transformer - logger.warning( - f"Invalid toolset transformer configuration: {e}" - ) - continue - else: - # Already a Transformer object - converted_transformers.append(t) - transformers = converted_transformers if converted_transformers else None + # Convert raw dict transformers to Transformer objects BEFORE merging + transformers = cls._convert_transformers_to_objects(transformers) tools = [] for tool in tools_data: if isinstance(tool, dict): tool["additional_instructions"] = additional_instructions # Convert tool-level transformers to Transformer objects tool_transformers = tool.get("transformers") - if tool_transformers: - converted_tool_transformers = [] - for t in tool_transformers: - if isinstance(t, dict): - try: - transformer_obj = Transformer(**t) - # Check if transformer is registered - from holmes.core.transformers import registry - - if not registry.is_registered(transformer_obj.name): - logger.warning( - f"Invalid tool transformer configuration: Transformer '{transformer_obj.name}' is not registered" - ) - continue # Skip invalid transformer - converted_tool_transformers.append(transformer_obj) - except Exception as e: - # Log warning and skip invalid transformer - logger.warning( - f"Invalid tool transformer configuration: {e}" - ) - continue - else: - # Already a Transformer object - converted_tool_transformers.append(t) - tool_transformers = ( - converted_tool_transformers - if converted_tool_transformers - else None - ) + tool_transformers = cls._convert_transformers_to_objects(tool_transformers)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
holmes/core/tools.py(10 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always place Python imports at the top of the file, not inside functions or methods
Type hints are required (mypy is configured in pyproject.toml)
Files:
holmes/core/tools.py
🧠 Learnings (3)
📚 Learning: 2025-08-08T06:15:30.784Z
Learnt from: nilo19
PR: robusta-dev/holmesgpt#695
File: holmes/core/transformers/registry.py:9-19
Timestamp: 2025-08-08T06:15:30.784Z
Learning: holmes/core/transformers/registry.py: TransformerRegistry is intentionally single-threaded and not designed to be thread-safe; avoid proposing locks unless multi-threaded access is introduced later.
Applied to files:
holmes/core/tools.py
📚 Learning: 2025-06-05T12:23:27.634Z
Learnt from: nherment
PR: robusta-dev/holmesgpt#436
File: tests/llm/utils/mock_utils.py:240-249
Timestamp: 2025-06-05T12:23:27.634Z
Learning: The holmesgpt project uses Python >= 3.10 and prefers modern type hint syntax like `list[str]`, `dict[str, int]` over importing equivalent types from the typing module like `List[str]`, `Dict[str, int]`.
Applied to files:
holmes/core/tools.py
📚 Learning: 2025-08-11T05:45:22.721Z
Learnt from: nilo19
PR: robusta-dev/holmesgpt#695
File: holmes/core/tools.py:286-306
Timestamp: 2025-08-11T05:45:22.721Z
Learning: In the holmes/core/transformers/base.py file, the `BaseTransformer.name` is a property method decorated with `property`, which means it should be accessed as an attribute without parentheses (e.g., `transformer_instance.name`, not `transformer_instance.name()`). The property returns the transformer's class name by default.
Applied to files:
holmes/core/tools.py
🧬 Code graph analysis (1)
holmes/core/tools.py (5)
holmes/core/transformers/base.py (5)
TransformerError(11-14)BaseTransformer(17-62)name(55-62)should_apply(42-52)transform(26-39)holmes/core/transformers/transformer.py (1)
Transformer(12-31)holmes/utils/config_utils.py (1)
merge_transformers(11-91)holmes/core/transformers/llm_summarize.py (4)
model_post_init(65-91)name(172-174)should_apply(93-124)transform(126-169)holmes/core/transformers/registry.py (2)
create_transformer(63-93)is_registered(95-105)
🪛 Ruff (0.12.2)
holmes/core/tools.py
207-207: Do not catch blind exception: Exception
(BLE001)
322-322: Do not catch blind exception: Exception
(BLE001)
323-325: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
441-444: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
586-586: Do not catch blind exception: Exception
(BLE001)
619-619: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
🔇 Additional comments (7)
holmes/core/tools.py (7)
11-21: LGTM! Type imports properly modernized.The changes align with the project's Python 3.10+ requirements, adding necessary imports for transformer integration.
36-40: LGTM! Transformer imports properly structured.The imports correctly separate the registry and error handling components while avoiding circular dependencies.
42-44: LGTM! TYPE_CHECKING guard prevents circular import.Using TYPE_CHECKING for BaseTransformer is the correct approach to avoid runtime circular imports while maintaining type safety.
174-180: LGTM! Proper use of PrivateAttr for transformer instance caching.The private attribute correctly stores transformer instances initialized once during model creation, improving performance by avoiding repeated instantiation.
239-253: LGTM! Clean integration of transformer pipeline.The implementation correctly applies transformers after tool execution and properly handles the transformed result.
406-408: LGTM! Logging modernization applied correctly.Successfully migrated from direct logging calls to module-level logger.
715-719: LGTM! Prerequisites logging properly modernized.Successfully migrated prerequisite checks to use the module-level logger.
|
@nilo19 let's address AI's comments, and fix the lint issues. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
tests/integration/test_tool_execution_pipeline.py (5)
57-61: Narrow the exception scope in setup.Catching Exception is too broad; constrain to the registry’s documented errors.
Apply:
- except Exception: + except (KeyError, TransformerError): self._original_llm_summarize = None
101-103: Rename test: it configures a single transformer, not “multiple”.Either add another transformer or rename for accuracy. Renaming is simpler here.
- def test_python_tool_with_multiple_transformers_integration(self): - """Test Python tool with multiple transformers in sequence.""" + def test_python_tool_with_transformer_integration(self): + """Test Python tool with a transformer in the pipeline."""
266-277: Make elapsed-time assertion robust and use mock call args.Use regex to match “in X.XXs” regardless of duration and access args via Mock API.
- info_calls = [call[0][0] for call in mock_logging.info.call_args_list] - performance_log = next( - (call for call in info_calls if "Applied transformer" in call), None - ) + info_calls = [c.args[0] for c in mock_logging.info.call_args_list] + performance_log = next((c for c in info_calls if "Applied transformer" in c), None) assert performance_log is not None assert "slow_transformer" in performance_log assert "performance_test_tool" in performance_log - assert ( - " in " in performance_log and "s " in performance_log - ) # Should show elapsed time pattern like "in X.XXs" + assert re.search(r"in\\s+\\d+\\.\\d+s\\b", performance_log), "Missing elapsed time like 'in 0.12s'" assert "size:" in performance_log # Should show size informationAlso add the missing import at the top of the file:
-from typing import Dict -import time +from typing import Dict +import re +import time
203-205: Check logged warning messages via call args, not str(call).This avoids relying on mock’s repr format.
- warning_calls = [call for call in mock_logging.warning.call_args_list] - assert any("failing_transformer" in str(call) for call in warning_calls) + warning_msgs = [c.args[0] for c in mock_logging.warning.call_args_list] + assert any("failing_transformer" in msg for msg in warning_msgs)
317-323: Optional: add a test for “summary not smaller → revert” policy.You already implemented this behavior in holmes/core/tools.py; add an integration test to lock it in.
Add the following test in this module (restores the original mock afterward):
def test_llm_summarize_reverts_when_summary_not_smaller(self): class ExpandingLLMSummarizeTransformer(BaseTransformer): def transform(self, input_text: str) -> str: # Intentionally make the "summary" larger return input_text + " EXTRA" def should_apply(self, input_text: str) -> bool: return True @property def name(self) -> str: return "llm_summarize" # Temporarily swap in the expanding transformer registry.unregister("llm_summarize") registry.register(ExpandingLLMSummarizeTransformer) try: tool = YAMLTool( name="revert_test", description="Ensure revert when summary grows", command="echo 'short text'", transformers=[Transformer(name="llm_summarize", config={})], ) result = tool.invoke({}) assert result.status == ToolResultStatus.SUCCESS # Revert should have kept original; the expanding marker must not appear assert "EXTRA" not in result.data finally: # Restore the mock summarizer used by this test class if registry.is_registered("llm_summarize"): registry.unregister("llm_summarize") registry.register(MockLLMSummarizeTransformer)tests/core/test_tool_transformers.py (1)
197-236: Strengthen invalid-transformers test with an instance check (optional).You already validate warnings and success; also asserting that only valid instances are cached would make the test crisper.
with patch("holmes.core.tools.logger.warning") as mock_logger_warning: tool = ConcreteTestTool( name="test_tool", description="Test tool", transformers=invalid_transforms, ) @@ assert len(warning_calls) > 0 - # Tool should still work despite invalid transformer + # Tool should still work despite invalid transformer result = tool.invoke({}) assert result.status == ToolResultStatus.SUCCESS + # Only the valid transformer should be cached + assert tool._transformer_instances is not None + assert [t.name for t in tool._transformer_instances] == ["mock_transformer"]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
tests/core/test_tool_transformers.py(1 hunks)tests/integration/test_tool_execution_pipeline.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always place Python imports at the top of the file, not inside functions or methods
Type hints are required (mypy is configured in pyproject.toml)
Files:
tests/core/test_tool_transformers.pytests/integration/test_tool_execution_pipeline.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Do not use invalid pytest markers; only use markers/tags declared in pyproject.toml
Files:
tests/core/test_tool_transformers.pytests/integration/test_tool_execution_pipeline.py
🧠 Learnings (1)
📚 Learning: 2025-08-08T06:15:30.784Z
Learnt from: nilo19
PR: robusta-dev/holmesgpt#695
File: holmes/core/transformers/registry.py:9-19
Timestamp: 2025-08-08T06:15:30.784Z
Learning: holmes/core/transformers/registry.py: TransformerRegistry is intentionally single-threaded and not designed to be thread-safe; avoid proposing locks unless multi-threaded access is introduced later.
Applied to files:
tests/integration/test_tool_execution_pipeline.py
🧬 Code graph analysis (2)
tests/core/test_tool_transformers.py (4)
holmes/core/tools.py (9)
Tool(162-350)YAMLTool(353-494)StructuredToolResult(78-102)ToolResultStatus(51-75)_invoke(339-346)_invoke(397-427)get_parameterized_one_liner(349-350)get_parameterized_one_liner(376-383)invoke(225-252)holmes/core/transformers/base.py (5)
TransformerError(11-14)BaseTransformer(17-62)transform(26-39)should_apply(42-52)name(55-62)holmes/core/transformers/transformer.py (1)
Transformer(12-31)holmes/core/transformers/registry.py (4)
register(20-46)is_registered(95-105)unregister(48-61)create_transformer(63-93)
tests/integration/test_tool_execution_pipeline.py (4)
holmes/core/tools.py (5)
Tool(162-350)YAMLTool(353-494)StructuredToolResult(78-102)ToolResultStatus(51-75)invoke(225-252)holmes/core/transformers/base.py (5)
BaseTransformer(17-62)TransformerError(11-14)transform(26-39)should_apply(42-52)name(55-62)holmes/core/transformers/transformer.py (1)
Transformer(12-31)holmes/core/transformers/registry.py (4)
is_registered(95-105)create_transformer(63-93)unregister(48-61)register(20-46)
🪛 Ruff (0.12.2)
tests/core/test_tool_transformers.py
49-49: Unused method argument: params
(ARG002)
49-49: Unused method argument: user_approved
(ARG002)
52-52: Unused method argument: params
(ARG002)
61-61: Use of assert detected
(S101)
76-76: Use of assert detected
(S101)
82-82: Unused method argument: params
(ARG002)
82-82: Unused method argument: user_approved
(ARG002)
85-85: Unused method argument: params
(ARG002)
92-92: Use of assert detected
(S101)
111-111: Use of assert detected
(S101)
123-123: Use of assert detected
(S101)
158-158: Use of assert detected
(S101)
162-162: Use of assert detected
(S101)
163-163: Use of assert detected
(S101)
165-165: Use of assert detected
(S101)
185-185: Unused method argument: params
(ARG002)
185-185: Unused method argument: user_approved
(ARG002)
188-188: Unused method argument: params
(ARG002)
195-195: Use of assert detected
(S101)
204-204: Unused method argument: params
(ARG002)
204-204: Unused method argument: user_approved
(ARG002)
211-211: Unused method argument: params
(ARG002)
234-234: Use of assert detected
(S101)
238-238: Use of assert detected
(S101)
251-251: Use of assert detected
(S101)
265-265: Use of assert detected
(S101)
266-266: Use of assert detected
(S101)
274-274: Use of assert detected
(S101)
275-275: Use of assert detected
(S101)
299-299: Use of assert detected
(S101)
300-300: Use of assert detected
(S101)
323-323: Unused method argument: params
(ARG002)
323-323: Unused method argument: user_approved
(ARG002)
330-330: Unused method argument: params
(ARG002)
340-340: Use of assert detected
(S101)
341-341: Use of assert detected
(S101)
342-342: Use of assert detected
(S101)
343-343: Use of assert detected
(S101)
351-351: Unused method argument: params
(ARG002)
351-351: Unused method argument: user_approved
(ARG002)
359-359: Unused method argument: params
(ARG002)
369-369: Use of assert detected
(S101)
370-370: Use of assert detected
(S101)
371-371: Use of assert detected
(S101)
379-379: Unused method argument: params
(ARG002)
379-379: Unused method argument: user_approved
(ARG002)
383-383: Unused method argument: params
(ARG002)
393-393: Use of assert detected
(S101)
394-394: Use of assert detected
(S101)
401-401: Unused method argument: input_text
(ARG002)
402-402: Avoid specifying long messages outside the exception class
(TRY003)
404-404: Unused method argument: input_text
(ARG002)
418-418: Unused method argument: params
(ARG002)
418-418: Unused method argument: user_approved
(ARG002)
424-424: Unused method argument: params
(ARG002)
437-437: Use of assert detected
(S101)
438-438: Use of assert detected
(S101)
443-443: Use of assert detected
(S101)
444-444: Use of assert detected
(S101)
457-457: Unused method argument: input_text
(ARG002)
474-474: Unused method argument: params
(ARG002)
474-474: Unused method argument: user_approved
(ARG002)
481-481: Unused method argument: params
(ARG002)
493-493: Use of assert detected
(S101)
494-494: Use of assert detected
(S101)
495-495: Use of assert detected
(S101)
496-496: Use of assert detected
(S101)
524-524: Unused method argument: user_approved
(ARG002)
531-531: Unused method argument: params
(ARG002)
542-542: Use of assert detected
(S101)
543-543: Use of assert detected
(S101)
552-552: Use of assert detected
(S101)
553-553: Use of assert detected
(S101)
565-565: Unused method argument: params
(ARG002)
565-565: Unused method argument: user_approved
(ARG002)
576-576: Unused method argument: params
(ARG002)
586-586: Use of assert detected
(S101)
587-587: Use of assert detected
(S101)
590-590: Use of assert detected
(S101)
591-591: Use of assert detected
(S101)
592-592: Use of assert detected
(S101)
593-593: Use of assert detected
(S101)
594-594: Use of assert detected
(S101)
602-602: Unused method argument: params
(ARG002)
602-602: Unused method argument: user_approved
(ARG002)
609-609: Unused method argument: params
(ARG002)
625-625: Use of assert detected
(S101)
626-626: Use of assert detected
(S101)
627-627: Use of assert detected
(S101)
628-628: Use of assert detected
(S101)
629-629: Use of assert detected
(S101)
636-636: Unused method argument: params
(ARG002)
636-636: Unused method argument: user_approved
(ARG002)
643-643: Unused method argument: params
(ARG002)
655-655: Use of assert detected
(S101)
656-656: Use of assert detected
(S101)
680-680: Unused method argument: params
(ARG002)
680-680: Unused method argument: user_approved
(ARG002)
687-687: Unused method argument: params
(ARG002)
696-696: Use of assert detected
(S101)
697-697: Use of assert detected
(S101)
698-698: Use of assert detected
(S101)
702-702: Use of assert detected
(S101)
703-703: Use of assert detected
(S101)
711-711: Unused method argument: params
(ARG002)
711-711: Unused method argument: user_approved
(ARG002)
718-718: Unused method argument: params
(ARG002)
735-735: Use of assert detected
(S101)
736-736: Use of assert detected
(S101)
739-739: Use of assert detected
(S101)
740-740: Use of assert detected
(S101)
741-741: Use of assert detected
(S101)
748-748: Unused method argument: params
(ARG002)
748-748: Unused method argument: user_approved
(ARG002)
755-755: Unused method argument: params
(ARG002)
766-766: Use of assert detected
(S101)
770-770: Use of assert detected
(S101)
771-771: Use of assert detected
(S101)
784-784: Unused method argument: params
(ARG002)
784-784: Unused method argument: user_approved
(ARG002)
791-791: Unused method argument: params
(ARG002)
807-807: Use of assert detected
(S101)
810-810: Use of assert detected
(S101)
811-811: Use of assert detected
(S101)
812-812: Use of assert detected
(S101)
816-816: Use of assert detected
(S101)
817-817: Use of assert detected
(S101)
825-825: Unused method argument: params
(ARG002)
825-825: Unused method argument: user_approved
(ARG002)
832-832: Unused method argument: params
(ARG002)
840-840: Use of assert detected
(S101)
844-844: Use of assert detected
(S101)
845-845: Use of assert detected
(S101)
853-853: Unused method argument: params
(ARG002)
853-853: Unused method argument: user_approved
(ARG002)
860-860: Unused method argument: params
(ARG002)
tests/integration/test_tool_execution_pipeline.py
60-60: Do not catch blind exception: Exception
(BLE001)
91-91: Use of assert detected
(S101)
94-94: Use of assert detected
(S101)
95-95: Use of assert detected
(S101)
96-96: Use of assert detected
(S101)
99-99: Use of assert detected
(S101)
108-108: Unused method argument: params
(ARG002)
108-108: Unused method argument: user_approved
(ARG002)
129-129: Unused method argument: params
(ARG002)
143-143: Use of assert detected
(S101)
146-146: Use of assert detected
(S101)
147-147: Use of assert detected
(S101)
161-161: Use of assert detected
(S101)
167-167: Unused method argument: input_text
(ARG002)
168-168: Avoid specifying long messages outside the exception class
(TRY003)
170-170: Unused method argument: input_text
(ARG002)
196-196: Use of assert detected
(S101)
200-200: Use of assert detected
(S101)
204-204: Use of assert detected
(S101)
226-226: Use of assert detected
(S101)
229-229: Use of assert detected
(S101)
230-230: Use of assert detected
(S101)
240-240: Unused method argument: input_text
(ARG002)
261-261: Use of assert detected
(S101)
264-264: Use of assert detected
(S101)
272-272: Use of assert detected
(S101)
273-273: Use of assert detected
(S101)
274-274: Use of assert detected
(S101)
275-275: Use of assert detected
(S101)
278-278: Use of assert detected
(S101)
315-315: Use of assert detected
(S101)
318-318: Use of assert detected
(S101)
321-321: Use of assert detected
(S101)
337-337: Use of assert detected
(S101)
338-338: Use of assert detected
(S101)
339-339: Use of assert detected
(S101)
342-342: Use of assert detected
(S101)
343-343: Use of assert detected
(S101)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Pre-commit checks
- GitHub Check: llm_evals
🔇 Additional comments (5)
tests/integration/test_tool_execution_pipeline.py (2)
54-63: Good: uses public registry API instead of private attributes.Switching to create_transformer(...) and unregister(...) preserves encapsulation and avoids brittle test coupling to internals.
341-343: Nice: asserts preserve debugging keywords post-summarization.This aligns with the PR goal to keep searchable keywords/error details in summaries.
tests/core/test_tool_transformers.py (3)
220-235: Correct logger patched.Patching holmes.core.tools.logger.warning matches the production logging path; assertions will observe real warnings.
795-807: Same here: right logger target.Consistent use of holmes.core.tools.logger.warning ensures reliable warning capture during initialization failures.
867-879: Good: verifies transformers aren’t recreated on invoke.Wrapping registry.create_transformer and asserting zero calls during invocations hardens the caching guarantee.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/core/transformers/test_llm_summarize.py (1)
394-396: Good fix: use isinstance for exception cause checks.This addresses earlier feedback and follows best practices for type checking in tests.
🧹 Nitpick comments (12)
tests/core/transformers/test_llm_summarize.py (12)
430-435: Silence Ruff ARG001: mark unused parameter.Rename the unused api_key arg to underscore to satisfy Ruff.
- def mock_llm_side_effect(model, api_key): + def mock_llm_side_effect(model, _api_key): if model == "gpt-4o-mini": return mock_llm1 elif model == "gpt-3.5-turbo": return mock_llm2 return Mock()
531-539: Silence Ruff ARG002 and add return type hints (tests still follow the Tool interface).Underscore unused params and annotate return types.
- class TestTool(Tool): - def _invoke(self, params, user_approved: bool = False): + class TestTool(Tool): + def _invoke(self, _params, _user_approved: bool = False) -> StructuredToolResult: return StructuredToolResult( status=ToolResultStatus.SUCCESS, data="Original short data" ) - def get_parameterized_one_liner(self, params): + def get_parameterized_one_liner(self, _params) -> str: return "test command"
574-583: Repeat: underscore unused params and add return type hints in this helper Tool.Keeps tests lint-clean and matches the abstract API.
- class TestTool(Tool): - def _invoke(self, params, user_approved: bool = False): + class TestTool(Tool): + def _invoke(self, _params, _user_approved: bool = False) -> StructuredToolResult: return StructuredToolResult( status=ToolResultStatus.SUCCESS, data=original_data, # This will be the input to the transformer ) - def get_parameterized_one_liner(self, params): + def get_parameterized_one_liner(self, _params) -> str: return "test command"
632-641: Repeat: underscore unused params and add return type hints in the third helper Tool.Same rationale as above.
- class TestTool(Tool): - def _invoke(self, params, user_approved: bool = False): + class TestTool(Tool): + def _invoke(self, _params, _user_approved: bool = False) -> StructuredToolResult: return StructuredToolResult( status=ToolResultStatus.SUCCESS, data=original_data ) - def get_parameterized_one_liner(self, params): + def get_parameterized_one_liner(self, _params) -> str: return "test command"
16-18: Add return type hint for helper factory.Minor type clarity; keeps in line with repo typing rules.
- def create_mock_llm(self, response_content: str = "Summarized content"): + def create_mock_llm(self, response_content: str = "Summarized content") -> Mock:
313-315: Add return type hint for the second helper factory.Mirror the change above.
- def create_mock_llm(self, response_content: str = "Summarized content"): + def create_mock_llm(self, response_content: str = "Summarized content") -> Mock:
585-586: Fix misleading comment (only fast_model is configured here).Avoid implying global_fast_model is used in this test.
- # Create tool with llm_summarize transformer (low threshold so it applies) - # Need to ensure fast_model is configured AND global_fast_model fallback + # Create tool with llm_summarize transformer (low threshold so it applies) + # Ensure fast_model is configured; global_fast_model fallback is handled elsewhere
403-423: Prefer pytest parametrize for clearer case reporting.Parametrization improves failure diagnostics per case.
@pytest.mark.parametrize( "threshold,input_text,expected", [ (0, "x", True), (1, "", False), (5, "short", False), (5, "longer", True), (1000, "x" * 999, False), (1000, "x" * 1000, False), (1000, "x" * 1001, True), ], ) @patch("holmes.core.transformers.llm_summarize.DefaultLLM") def test_different_threshold_scenarios(self, mock_default_llm, threshold, input_text, expected): mock_default_llm.return_value = self.create_mock_llm("summary") transformer = LLMSummarizeTransformer(fast_model="gpt-4o-mini", input_threshold=threshold) assert transformer.should_apply(input_text) == expected
362-381: Optional: assert the LLM was not invoked for small input.Tightens the contract that we don’t call the model when should_apply() is False.
# Should not apply assert not transformer.should_apply(small_output) + mock_llm.completion.assert_not_called()
1-3: Ruff S101/ARG00x in tests: choose suppression strategy.Tests intentionally use
assertand unused args; either ignore these in tests or add per-file suppression.Option A (pyproject):
[tool.ruff.lint.per-file-ignores] "tests/**/*.py" = ["S101", "ARG001", "ARG002"]Option B (file header):
# ruff: noqa: S101, ARG001, ARG002 """ Unit tests for LLMSummarizeTransformer. """Confirm which approach matches your lint policy.
85-97: Add a test for global_fast_model fallback.We exercise fast_model directly but not the fallback path. Adding this keeps coverage aligned with the feature description.
@patch("holmes.core.transformers.llm_summarize.DefaultLLM") def test_init_with_global_fast_model_fallback(self, mock_default_llm): mock_default_llm.return_value = self.create_mock_llm() transformer = LLMSummarizeTransformer(global_fast_model="gpt-4o-mini", api_key="k") assert transformer.fast_model is None assert transformer._fast_llm is not None mock_default_llm.assert_called_once_with("gpt-4o-mini", "k")
16-28: Deduplicate create_mock_llm helpers via a module-level fixture.Removes duplication and centralizes the mock response shape.
Example (module scope):
import pytest from unittest.mock import Mock @pytest.fixture def make_mock_llm(): def _make(response_content: str = "Summarized content") -> Mock: mock_llm = Mock() mock_response = Mock() mock_choice = Mock() mock_message = Mock() mock_message.content = response_content mock_choice.message = mock_message mock_response.choices = [mock_choice] mock_llm.completion.return_value = mock_response return mock_llm return _makeThen replace
self.create_mock_llm(...)withmake_mock_llm(...)and drop both class-local helpers.Also applies to: 313-327
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/core/transformers/test_llm_summarize.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always place Python imports at the top of the file, not inside functions or methods
Type hints are required (mypy is configured in pyproject.toml)
Files:
tests/core/transformers/test_llm_summarize.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Do not use invalid pytest markers; only use markers/tags declared in pyproject.toml
Files:
tests/core/transformers/test_llm_summarize.py
🧬 Code graph analysis (1)
tests/core/transformers/test_llm_summarize.py (3)
holmes/core/transformers/llm_summarize.py (4)
LLMSummarizeTransformer(15-174)name(172-174)should_apply(93-124)transform(126-169)holmes/core/transformers/base.py (4)
TransformerError(11-14)name(55-62)should_apply(42-52)transform(26-39)holmes/core/tools.py (5)
Tool(162-350)StructuredToolResult(78-102)_invoke(339-346)_invoke(397-427)invoke(225-252)
🪛 Ruff (0.12.2)
tests/core/transformers/test_llm_summarize.py
33-33: Use of assert detected
(S101)
34-34: Use of assert detected
(S101)
35-35: Use of assert detected
(S101)
36-36: Use of assert detected
(S101)
37-37: Use of assert detected
(S101)
52-52: Use of assert detected
(S101)
53-53: Use of assert detected
(S101)
54-54: Use of assert detected
(S101)
55-55: Use of assert detected
(S101)
56-56: Use of assert detected
(S101)
67-67: Use of assert detected
(S101)
77-77: Use of assert detected
(S101)
83-83: Use of assert detected
(S101)
93-93: Use of assert detected
(S101)
94-94: Use of assert detected
(S101)
95-95: Use of assert detected
(S101)
136-136: Use of assert detected
(S101)
137-137: Use of assert detected
(S101)
149-149: Use of assert detected
(S101)
150-150: Use of assert detected
(S101)
164-164: Use of assert detected
(S101)
165-165: Use of assert detected
(S101)
166-166: Use of assert detected
(S101)
178-178: Use of assert detected
(S101)
183-183: Use of assert detected
(S101)
184-184: Use of assert detected
(S101)
185-185: Use of assert detected
(S101)
186-186: Use of assert detected
(S101)
201-201: Use of assert detected
(S101)
205-205: Use of assert detected
(S101)
206-206: Use of assert detected
(S101)
216-216: Use of assert detected
(S101)
281-281: Use of assert detected
(S101)
282-282: Use of assert detected
(S101)
283-283: Use of assert detected
(S101)
286-286: Use of assert detected
(S101)
305-305: Use of assert detected
(S101)
306-306: Use of assert detected
(S101)
307-307: Use of assert detected
(S101)
351-351: Use of assert detected
(S101)
355-355: Use of assert detected
(S101)
360-360: Use of assert detected
(S101)
375-375: Use of assert detected
(S101)
379-379: Use of assert detected
(S101)
394-394: Use of assert detected
(S101)
395-395: Use of assert detected
(S101)
396-396: Use of assert detected
(S101)
420-420: Use of assert detected
(S101)
430-430: Unused function argument: api_key
(ARG001)
454-454: Use of assert detected
(S101)
455-455: Use of assert detected
(S101)
461-461: Use of assert detected
(S101)
462-462: Use of assert detected
(S101)
471-471: Use of assert detected
(S101)
472-472: Use of assert detected
(S101)
494-494: Use of assert detected
(S101)
500-500: Use of assert detected
(S101)
501-501: Use of assert detected
(S101)
514-514: Use of assert detected
(S101)
516-516: Use of assert detected
(S101)
517-517: Use of assert detected
(S101)
532-532: Unused method argument: params
(ARG002)
532-532: Unused method argument: user_approved
(ARG002)
537-537: Unused method argument: params
(ARG002)
557-557: Use of assert detected
(S101)
558-558: Use of assert detected
(S101)
575-575: Unused method argument: params
(ARG002)
575-575: Unused method argument: user_approved
(ARG002)
581-581: Unused method argument: params
(ARG002)
606-606: Use of assert detected
(S101)
607-607: Use of assert detected
(S101)
615-615: Use of assert detected
(S101)
634-634: Unused method argument: params
(ARG002)
634-634: Unused method argument: user_approved
(ARG002)
639-639: Unused method argument: params
(ARG002)
664-664: Use of assert detected
(S101)
665-665: Use of assert detected
(S101)
672-672: Use of assert detected
(S101)
698-698: Use of assert detected
(S101)
699-699: Use of assert detected
(S101)
706-706: Use of assert detected
(S101)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Pre-commit checks
- GitHub Check: llm_evals
|
@mainred @moshemorad can you help rerun the ci? |
4 participants
Design doc: https://www.notion.so/Fast-Model-Summarization-for-Large-Tool-Output-21f18f5dfd9b8045b7faf3368b6bf6ac
Summary
Key Features
Global Configuration:
Tool Integration:
Smart Behavior:
Files Changed
Benefits
🤖 Generated with https://claude.ai/code
Co-Authored-By: Claude noreply@anthropic.com