Improve HolmesGPT accuracy on questions about itself, configuring tools, and using runbooks - #729
Conversation
WalkthroughThis change set updates HolmesGPT prompt templates for runbook usage, toolset configuration, and permission error handling. It enhances runbook fetching to support multiple search paths, enriches returned runbook content with usage instructions, and adds new troubleshooting runbooks and test fixtures. Documentation and test configurations were also refined. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant HolmesGPT
participant RunbookToolset
participant RunbookFetcher
User->>HolmesGPT: Reports operational issue (e.g., high CPU, error)
HolmesGPT->>RunbookToolset: Search for relevant runbook (with additional search paths)
RunbookToolset->>RunbookFetcher: fetch_runbook(runbook_path, search_paths)
RunbookFetcher->>RunbookToolset: get_runbook_by_path(runbook_path, search_paths)
alt Runbook found
RunbookFetcher-->>HolmesGPT: Return runbook content + usage instructions
HolmesGPT-->>User: Presents runbook steps, follows instructions, reports findings
else Runbook not found
RunbookFetcher-->>HolmesGPT: Return error with attempted search paths
HolmesGPT-->>User: Informs user runbook not found, suggests next steps
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
⏰ 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). (3)
✨ 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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
holmes/core/tools_utils/tool_executor.py (1)
18-18: Replace direct attribute access with a read-only propertyThe TODO hints at hiding
toolsets.
A minimal, backwards-compatible step is:class ToolExecutor: def __init__(self, toolsets: List[Toolset]): - # TODO: expose function for this instead of callers accessing directly - self.toolsets = toolsets + # Keep private; expose via property for read-only access + self._toolsets = toolsets + + @property + def toolsets(self) -> list[Toolset]: + """Return all configured toolsets (read-only).""" + return self._toolsetsThis avoids external mutation while requiring no call-site changes.
tests/llm/utils/test_case_utils.py (1)
290-327: Function implementation is correct but could improve type safety.The message building logic is sound and follows proper patterns for prompt construction. The temporary nature is appropriately documented.
Consider improving the type annotation for better type safety:
- tool_executor: Any, # ToolExecutor type + tool_executor: "ToolExecutor", # Forward reference if needed to avoid circular importsThe function name suffix "2" suggests this might be replacing an existing function - ensure the original is removed once the referenced PR is merged.
tests/llm/test_workload_health.py (1)
62-72: Consider removing commented codeThe dataset upload logic has been commented out. If this functionality is no longer needed, consider removing the commented code entirely to improve code cleanliness.
def get_test_cases(): mh = MockHelper(TEST_CASES_FOLDER) - # dataset_name = braintrust_util.get_dataset_name("health_check") - # if os.environ.get("UPLOAD_DATASET") and os.environ.get("BRAINTRUST_API_KEY"): - # bt_helper = braintrust_util.BraintrustEvalHelper( - # project_name=BRAINTRUST_PROJECT, dataset_name=dataset_name - # ) - # bt_helper.upload_test_cases(mh.load_test_cases()) test_cases = mh.load_workload_health_test_cases() iterations = int(os.environ.get("ITERATIONS", "1")) return [add_tags_to_eval(test_case) for test_case in test_cases] * iterationstests/llm/test_ask_holmes.py (1)
52-58: Good identification of code duplicationThe TODO correctly identifies code duplication across test files. Consider extracting the common test case loading logic into a shared utility function.
Would you like me to help create a shared utility function to eliminate this duplication across the test files?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
.github/workflows/llm-evaluation.yaml(1 hunks)CLAUDE.md(4 hunks)conftest.py(2 hunks)docs/development/evals/index.md(1 hunks)docs/development/evals/writing.md(1 hunks)holmes/core/tools_utils/tool_executor.py(1 hunks)holmes/core/tracing.py(4 hunks)holmes/plugins/prompts/_general_instructions.jinja2(2 hunks)holmes/plugins/prompts/_permission_errors.jinja2(1 hunks)holmes/plugins/prompts/_runbook_instructions.jinja2(1 hunks)holmes/plugins/prompts/_toolsets_instructions.jinja2(3 hunks)holmes/plugins/prompts/generic_ask.jinja2(1 hunks)tests/llm/conftest.py(6 hunks)tests/llm/fixtures/test_ask_holmes/56_kafka_runbook_no_tool/test_case.yaml(1 hunks)tests/llm/test_ask_holmes.py(7 hunks)tests/llm/test_investigate.py(4 hunks)tests/llm/test_workload_health.py(5 hunks)tests/llm/utils/braintrust.py(4 hunks)tests/llm/utils/constants.py(0 hunks)tests/llm/utils/mock_toolset.py(3 hunks)tests/llm/utils/property_manager.py(1 hunks)tests/llm/utils/reporting/terminal_reporter.py(3 hunks)tests/llm/utils/tags.py(1 hunks)tests/llm/utils/test_case_utils.py(4 hunks)tests/llm/utils/test_mock_toolset.py(8 hunks)tests/llm/utils/test_results.py(1 hunks)
💤 Files with no reviewable changes (1)
- tests/llm/utils/constants.py
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml)
Type hints required (mypy configuration in pyproject.toml)
Files:
holmes/core/tools_utils/tool_executor.pytests/llm/utils/test_results.pytests/llm/utils/property_manager.pytests/llm/utils/tags.pytests/llm/utils/reporting/terminal_reporter.pytests/llm/utils/braintrust.pyconftest.pyholmes/core/tracing.pytests/llm/utils/test_case_utils.pytests/llm/test_investigate.pytests/llm/test_ask_holmes.pytests/llm/utils/test_mock_toolset.pytests/llm/test_workload_health.pytests/llm/utils/mock_toolset.pytests/llm/conftest.py
holmes/plugins/prompts/**/*.jinja2
📄 CodeRabbit Inference Engine (CLAUDE.md)
Prompts: holmes/plugins/prompts/{name}.jinja2
Files:
holmes/plugins/prompts/generic_ask.jinja2holmes/plugins/prompts/_general_instructions.jinja2holmes/plugins/prompts/_permission_errors.jinja2holmes/plugins/prompts/_runbook_instructions.jinja2holmes/plugins/prompts/_toolsets_instructions.jinja2
tests/**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
tests/**/*.py: All new features require unit tests
New toolsets require integration tests with mocks
Tests: Match source structure under tests/
Files:
tests/llm/utils/test_results.pytests/llm/utils/property_manager.pytests/llm/utils/tags.pytests/llm/utils/reporting/terminal_reporter.pytests/llm/utils/braintrust.pytests/llm/utils/test_case_utils.pytests/llm/test_investigate.pytests/llm/test_ask_holmes.pytests/llm/utils/test_mock_toolset.pytests/llm/test_workload_health.pytests/llm/utils/mock_toolset.pytests/llm/conftest.py
tests/llm/**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
tests/llm/**/*.py: Complex investigations should have LLM evaluation tests
Each LLM test must use a dedicated namespace app- (e.g., app-01, app-02) to prevent conflicts when tests run simultaneously
Files:
tests/llm/utils/test_results.pytests/llm/utils/property_manager.pytests/llm/utils/tags.pytests/llm/utils/reporting/terminal_reporter.pytests/llm/utils/braintrust.pytests/llm/utils/test_case_utils.pytests/llm/test_investigate.pytests/llm/test_ask_holmes.pytests/llm/utils/test_mock_toolset.pytests/llm/test_workload_health.pytests/llm/utils/mock_toolset.pytests/llm/conftest.py
tests/llm/fixtures/**/*
📄 CodeRabbit Inference Engine (CLAUDE.md)
tests/llm/fixtures/**/*: Mock data: tests/llm/fixtures/{test_name}/
All pod names must be unique across tests (e.g., giant-narwhal, blue-whale, sea-turtle) - never reuse pod names between tests
Never use resource names that hint at the problem or expected behavior (e.g., avoid broken-pod, test-project-that-does-not-exist, crashloop-app). Use neutral names that don't give away what the LLM should discover
Files:
tests/llm/fixtures/test_ask_holmes/56_kafka_runbook_no_tool/test_case.yaml
🧠 Learnings (21)
📓 Common learnings
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.
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.
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: LLM evaluation tests run automatically in CI
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to holmes/plugins/prompts/**/*.jinja2 : Prompts: holmes/plugins/prompts/{name}.jinja2
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.
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to tests/**/*.py : New toolsets require integration tests with mocks
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to tests/llm/**/*.py : Complex investigations should have LLM evaluation tests
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to holmes/plugins/toolsets/**/*.yaml : Toolsets: holmes/plugins/toolsets/{name}.yaml or {name}/
holmes/core/tools_utils/tool_executor.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/plugins/prompts/generic_ask.jinja2 (1)
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to holmes/plugins/prompts/**/*.jinja2 : Prompts: holmes/plugins/prompts/{name}.jinja2
docs/development/evals/index.md (3)
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.
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.
holmes/plugins/prompts/_general_instructions.jinja2 (2)
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to holmes/plugins/prompts/**/*.jinja2 : Prompts: holmes/plugins/prompts/{name}.jinja2
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/prompts/_permission_errors.jinja2 (1)
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to holmes/plugins/prompts/**/*.jinja2 : Prompts: holmes/plugins/prompts/{name}.jinja2
tests/llm/fixtures/test_ask_holmes/56_kafka_runbook_no_tool/test_case.yaml (3)
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: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to holmes/plugins/toolsets/**/*.yaml : Toolsets: holmes/plugins/toolsets/{name}.yaml or {name}/
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to tests/llm/fixtures/**/* : Mock data: tests/llm/fixtures/{test_name}/
docs/development/evals/writing.md (3)
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to tests/llm/fixtures/**/* : Mock data: tests/llm/fixtures/{test_name}/
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to tests/**/*.py : New toolsets require integration tests with mocks
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to tests/llm/fixtures/**/* : Never use resource names that hint at the problem or expected behavior (e.g., avoid broken-pod, test-project-that-does-not-exist, crashloop-app). Use neutral names that don't give away what the LLM should discover
.github/workflows/llm-evaluation.yaml (3)
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: LLM evaluation tests run automatically in CI
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to tests/llm/**/*.py : Complex investigations should have LLM evaluation tests
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to tests/llm/**/*.py : Each LLM test must use a dedicated namespace app- (e.g., app-01, app-02) to prevent conflicts when tests run simultaneously
holmes/plugins/prompts/_runbook_instructions.jinja2 (1)
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to holmes/plugins/prompts/**/*.jinja2 : Prompts: holmes/plugins/prompts/{name}.jinja2
tests/llm/utils/braintrust.py (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: 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.
Learnt from: nherment
PR: #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].
holmes/core/tracing.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: #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].
tests/llm/utils/test_case_utils.py (3)
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to tests/**/*.py : New toolsets require integration tests with mocks
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to holmes/plugins/prompts/**/*.jinja2 : Prompts: holmes/plugins/prompts/{name}.jinja2
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to tests/llm/**/*.py : Complex investigations should have LLM evaluation tests
tests/llm/test_investigate.py (5)
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to tests/**/*.py : New toolsets require integration tests with mocks
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to tests/llm/**/*.py : Complex investigations should have LLM evaluation tests
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to tests/llm/fixtures/**/* : Mock data: tests/llm/fixtures/{test_name}/
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to tests/llm/**/*.py : Each LLM test must use a dedicated namespace app- (e.g., app-01, app-02) to prevent conflicts when tests run simultaneously
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: LLM evaluation tests run automatically in CI
CLAUDE.md (16)
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: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to tests/llm/**/*.py : Complex investigations should have LLM evaluation tests
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: LLM evaluation tests run automatically in CI
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: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to tests/llm/**/*.py : Each LLM test must use a dedicated namespace app- (e.g., app-01, app-02) to prevent conflicts when tests run simultaneously
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to tests/**/*.py : New toolsets require integration tests with mocks
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to tests/llm/fixtures/**/* : Mock data: tests/llm/fixtures/{test_name}/
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to tests/llm/fixtures/**/* : Never use resource names that hint at the problem or expected behavior (e.g., avoid broken-pod, test-project-that-does-not-exist, crashloop-app). Use neutral names that don't give away what the LLM should discover
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to tests/llm/fixtures/**/* : All pod names must be unique across tests (e.g., giant-narwhal, blue-whale, sea-turtle) - never reuse pod names between tests
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to holmes/plugins/prompts/**/*.jinja2 : Prompts: holmes/plugins/prompts/{name}.jinja2
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to **/*.py : Use Ruff for formatting and linting (configured in pyproject.toml)
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to tests/**/*.py : All new features require unit tests
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to **/*.py : Type hints required (mypy configuration in pyproject.toml)
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Keep PRs focused and include tests
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Pre-commit hooks enforce quality checks
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: PRs require maintainer approval
tests/llm/test_ask_holmes.py (4)
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: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to tests/**/*.py : New toolsets require integration tests with mocks
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to tests/llm/**/*.py : Complex investigations should have LLM evaluation tests
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/llm/utils/test_mock_toolset.py (6)
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to tests/**/*.py : New toolsets require integration tests with mocks
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to tests/llm/fixtures/**/* : Mock data: tests/llm/fixtures/{test_name}/
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to tests/**/*.py : All new features require unit tests
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to tests/llm/**/*.py : Complex investigations should have LLM evaluation tests
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to tests/**/*.py : Tests: Match source structure under tests/
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to holmes/plugins/toolsets/**/*.yaml : Toolsets: holmes/plugins/toolsets/{name}.yaml or {name}/
tests/llm/test_workload_health.py (4)
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to tests/**/*.py : New toolsets require integration tests with mocks
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to tests/llm/**/*.py : Complex investigations should have LLM evaluation tests
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to tests/llm/**/*.py : Each LLM test must use a dedicated namespace app- (e.g., app-01, app-02) to prevent conflicts when tests run simultaneously
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to tests/llm/fixtures/**/* : Mock data: tests/llm/fixtures/{test_name}/
tests/llm/utils/mock_toolset.py (2)
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to tests/**/*.py : New toolsets require integration tests with mocks
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/plugins/prompts/_toolsets_instructions.jinja2 (4)
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to holmes/plugins/prompts/**/*.jinja2 : Prompts: holmes/plugins/prompts/{name}.jinja2
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to holmes/plugins/toolsets/**/*.yaml : Toolsets: holmes/plugins/toolsets/{name}.yaml or {name}/
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to holmes/plugins/toolsets/**/*.yaml : Bash toolset validates commands for safety
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/llm/conftest.py (7)
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to tests/llm/**/*.py : Complex investigations should have LLM evaluation tests
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to tests/llm/**/*.py : Each LLM test must use a dedicated namespace app- (e.g., app-01, app-02) to prevent conflicts when tests run simultaneously
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to tests/**/*.py : New toolsets require integration tests with mocks
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to tests/llm/fixtures/**/* : Mock data: tests/llm/fixtures/{test_name}/
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: Applies to tests/**/*.py : All new features require unit tests
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.571Z
Learning: LLM evaluation tests run automatically in CI
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.
🧬 Code Graph Analysis (5)
tests/llm/utils/tags.py (1)
tests/llm/utils/test_case_utils.py (1)
HolmesTestCase(51-68)
tests/llm/utils/reporting/terminal_reporter.py (1)
tests/llm/utils/test_results.py (1)
console_status(99-109)
tests/llm/utils/braintrust.py (2)
holmes/core/tracing.py (1)
DummySpan(44-60)tests/llm/utils/system.py (1)
readable_timestamp(50-51)
conftest.py (2)
tests/llm/conftest.py (1)
show_llm_summary_report(326-360)tests/llm/utils/system.py (1)
readable_timestamp(50-51)
tests/llm/test_workload_health.py (4)
tests/llm/utils/braintrust.py (2)
get_experiment_name(166-171)start_evaluation(106-135)holmes/core/tools_utils/tool_executor.py (1)
ToolExecutor(16-66)tests/llm/utils/test_case_utils.py (2)
MockHelper(101-181)load_workload_health_test_cases(106-107)tests/llm/utils/tags.py (1)
add_tags_to_eval(16-17)
🪛 Ruff (0.12.2)
tests/llm/utils/test_mock_toolset.py
111-111: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
⏰ 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). (3)
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
- GitHub Check: Pre-commit checks
🔇 Additional comments (66)
holmes/plugins/prompts/generic_ask.jinja2 (1)
6-6: Injecting dynamic date/time may harm determinism of LLM testsIncluding
_current_date_time.jinja2means every rendered prompt now changes with each run. That can make snapshot-based tests or judge prompts brittle because the model receives different tokens every execution.
Please verify that:
- The evaluation harness deliberately allows for this variability, or
_current_date_time.jinja2renders a fixed value during tests (e.g., via env-override or freeze-time).Otherwise, consider gating this include behind a stable flag for test runs.
tests/llm/utils/test_results.py (1)
17-17: Ensure newuser_promptis surfaced in all serializers/reportersThe extra field is added here, but
json.dumps(asdict(result)), CSV export, and Slack/HTML reporters still need updating or the value will silently disappear.
Please audit usages ofTestResultto confirmuser_promptis:
- Populated (not always
None)- Emitted in all output formats
- Accounted for in any equality / hash functions
tests/llm/fixtures/test_ask_holmes/56_kafka_runbook_no_tool/test_case.yaml (1)
15-16: Confirm the runner recognisesskip/skip_reasonkeysDifferent fixtures use
mark: skipmeta rather than top-level keys. Double-check that the loader actually parses these fields; otherwise the test will still execute.
If unsupported, migrate to the standardmarks:stanza.holmes/plugins/prompts/_permission_errors.jinja2 (1)
1-7: Good modularisation of permission-error guidanceExtracting these instructions into a shared partial improves reuse and maintainability.
No issues spotted.docs/development/evals/index.md (1)
158-158: LGTM! Clear documentation for the new environment variable.The documentation clearly explains the
ASK_HOLMES_TEST_TYPEvariable and its two modes (cliandserver), helping developers understand how to control the message building flow in ask_holmes tests.tests/llm/utils/property_manager.py (1)
25-30: LGTM! Safe addition of user prompt tracking.The use of
getattrwith a default empty string safely handles cases where theuser_promptattribute doesn't exist. The comment clearly indicates this property is specific toAskHolmesTestCase.tests/llm/utils/reporting/terminal_reporter.py (2)
31-31: LGTM! User prompt display improves test visibility.The addition of the "User Prompt" column with proper text wrapping enhances the debugging experience by showing the initial user input for each test case. The implementation follows the same wrapping pattern as other columns.
Also applies to: 59-63, 96-96
89-90: Consider the impact of disabling analysis generation.The analysis functionality has been disabled, which may reduce debugging capabilities for failed tests. The analysis provided valuable insights into why tests failed and categorized failure types (MockDataError, setup issues, real failures).
Is this temporarily disabled for performance reasons, or is this a permanent change? Consider whether the analysis should be:
- Made optional via an environment variable
- Only generated for failed tests in verbose mode
- Replaced with a simpler failure categorization mechanism
tests/llm/utils/tags.py (1)
16-17: LGTM! Function simplification aligns with the broader refactoring.Removing the
experiment_nameparameter simplifies the function while maintaining all essential functionality. The pytest.param still includes proper marks and test case ID, and experiment handling is now centralized elsewhere in the codebase.holmes/plugins/prompts/_general_instructions.jinja2 (2)
13-13: Enhancement improves runbook utilization.This instruction strategically directs the AI to prioritize runbook consultation for operational issues, which aligns with the PR's objective to improve accuracy on configuration and troubleshooting questions.
38-38: Template modularization improves maintainability.Moving permission error handling to a separate template promotes reusability and follows established patterns seen in other template includes in this file.
conftest.py (2)
1-1: Imports correctly placed and necessary.The
osimport andreadable_timestampfunction import are properly positioned at the top of the file and are used in the new experiment ID generation logic.Also applies to: 4-4
88-94: Robust experiment ID generation with proper fallback handling.The implementation correctly handles the common CI scenario where
os.getlogin()fails by providing sensible fallbacks. The logic avoids overwriting existingEXPERIMENT_IDvalues and creates meaningful unique identifiers.docs/development/evals/writing.md (1)
223-256: Comprehensive documentation for custom runbooks feature.The new section effectively documents the runbook customization capability with clear examples, configuration options, and practical use cases. The YAML example is well-structured and the three configuration modes are clearly explained.
CLAUDE.md (6)
113-113: Improved command documentation clarity.The clarification about using the
-kflag with test names makes the documentation more actionable and precise.
116-119: Important testing guidance properly emphasized.The strong recommendation to use live tools aligns with best practices for ensuring LLM tests match real-world behavior, which is critical for accuracy.
142-144: Clear documentation of new test mode functionality.The documentation effectively explains the new
ASK_HOLMES_TEST_TYPEenvironment variable and its impact on test behavior, helping developers choose the appropriate mode.
212-212: Appropriate emphasis on Python import best practices.The strong guidance about import placement aligns with Python coding standards and helps maintain code quality.
219-219: Consistent reinforcement of critical testing practice.The repeated emphasis on using
RUN_LIVE=trueis justified given its importance for ensuring test reliability and real-world accuracy.
243-246: Consistent documentation of runbook customization.The runbook configuration documentation is concise yet complete, and consistent with the detailed documentation in the writing guide.
holmes/plugins/prompts/_toolsets_instructions.jinja2 (5)
1-2: Header addition improves template organization.The descriptive header clearly indicates the template's purpose and improves overall organization.
25-25: Improvements to disabled toolsets handling.The typo correction, consistent status checking, and explicit placeholder message enhance both code reliability and user experience.
Also applies to: 30-30, 42-44
49-49: Enhanced user guidance for missing integrations.The improved messaging provides users with specific, actionable guidance and direct links to relevant documentation, significantly improving the user experience for integration setup.
Also applies to: 51-52
54-62: Comprehensive configuration guidance enhancement.The detailed decision tree and prioritized documentation links significantly improve the AI's ability to provide accurate, actionable guidance for integration setup questions, directly supporting the PR's objectives.
62-62: Clear guidance on documentation prioritization.The explicit preference for specific toolset documentation ensures users receive the most relevant and helpful guidance.
tests/llm/utils/test_case_utils.py (2)
8-8: LGTM: Import additions support new functionality correctly.The new imports for prompt utilities, runbook catalog, and Rich console are appropriate for the enhanced message building functionality being added.
Also applies to: 13-19, 22-22
74-74: LGTM: Runbooks field addition is well-designed.The optional runbooks field follows the existing model pattern and provides appropriate flexibility for per-test runbook catalog overrides.
tests/llm/conftest.py (4)
20-20: LGTM: Import simplification aligns with configuration centralization.The removal of PROJECT constant and get_experiment_name imports supports the broader refactoring to centralize Braintrust configuration.
247-254: LGTM: Helpful addition for test mode visibility.The ASK_HOLMES_TEST_TYPE display enhances developer experience by showing which test mode is active, with appropriate default handling and user guidance.
415-415: LGTM: User prompt tracking enhances test observability.The consistent addition of user_prompt field collection across all test result paths improves test reporting and debugging capabilities.
Also applies to: 499-499, 522-522
552-552: LGTM: Braintrust URL generation simplification is cleaner.The direct call to
get_braintrust_url()removes duplicate URL construction logic and centralizes the implementation.tests/llm/utils/braintrust.py (4)
9-14: LGTM: Import consolidation centralizes configuration.Moving Braintrust constants to holmes.core.tracing reduces duplication and provides a single source of truth for configuration.
105-105: Good documentation of planned refactoring.The TODO comment clearly indicates the migration path from BraintrustEvalHelper to BraintrustTracer, which helps track technical debt.
167-171: LGTM: Function simplification is cleaner and more direct.The refactored function removes intermediate variables and clearly documents the expected behavior with EXPERIMENT_ID always being set by conftest.py.
201-201: LGTM: URL construction uses centralized constants.Using the imported BRAINTRUST_ORG and BRAINTRUST_PROJECT constants is cleaner than direct environment variable access and maintains consistency.
holmes/core/tracing.py (5)
6-10: LGTM: Environment variable centralization improves configuration management.The module-level constants provide a single source of truth for Braintrust configuration with appropriate defaults and helpful context comments.
85-85: LGTM: Explicit project parameter improves API clarity.Requiring an explicit project parameter makes the API more intentional and aligns with the centralized configuration approach.
91-91: LGTM: Required experiment_name parameter improves API explicitness.Making the experiment_name parameter required ensures callers are intentional about experiment naming, which is good API design.
162-162: LGTM: Excellent simplification using SDK functionality.Using
current_span.link()instead of manual URL construction leverages the Braintrust SDK's built-in functionality, reducing complexity and potential errors.
196-196: LGTM: Default parameter uses centralized configuration.Using BRAINTRUST_PROJECT as the default parameter aligns with the centralized configuration approach while maintaining flexibility.
tests/llm/test_investigate.py (3)
57-57: LGTM: ToolExecutor instantiation updated for API changes.The change from
mock.enabled_toolsetstomock.toolsetsaligns with the MockToolsetManager API evolution and maintains consistency with other test files.
74-83: LGTM: Function simplification improves maintainability.The removal of dataset upload complexity and change of iterations default from 0 to 1 makes the function more straightforward and provides a sensible default behavior.
94-94: LGTM: Parametrization simplification reduces complexity.Removing the experiment_name parameter from test parametrization aligns with the centralized configuration approach and simplifies test execution.
holmes/plugins/prompts/_runbook_instructions.jinja2 (8)
9-19: Clear and actionable runbook instructionsThe critical instructions for handling operational issues are well-structured and emphasize systematic runbook following. The formatting improvement with the added empty line enhances readability.
20-49: Comprehensive handling of missing tools scenariosExcellent addition that addresses the critical scenario of missing tools/integrations during runbook execution. The emphasis on transparency and explicit acknowledgment of limitations will significantly improve user trust and understanding of what HolmesGPT can and cannot do.
50-72: Excellent investigation transparency requirementsThe detailed requirements for documenting investigation steps with specific failure reasons and the concrete example format will greatly enhance user understanding of HolmesGPT's actions. The emphasis on avoiding generic error messages in favor of specific, actionable explanations is particularly valuable.
9-14: Excellent structured approach to mandatory runbook checking.The critical protocol clearly establishes a systematic 4-step process for operational issue handling. The use of "CRITICAL" and "MUST" provides appropriate emphasis, and the requirement to explain skipped steps ensures transparency.
20-49: Comprehensive handling of missing tools scenario.The instructions provide thorough coverage of missing tool scenarios with clear sub-requirements for transparency and user guidance. The distinction between AI-discovered and user-mentioned runbooks shows good attention to different interaction patterns.
50-64: Strong emphasis on investigation transparency.The detailed requirements for documenting investigation steps with specific failure categorization will significantly improve user trust and debugging capability. The prohibition on vague error messages like "Missing data" ensures actionable feedback.
65-72: Excellent concrete example format.The example effectively demonstrates the expected level of detail and visual formatting using checkmarks. The specific failure explanations (e.g., "Database toolset is not enabled") provide clear templates for implementation.
1-1: Proper Jinja2 template structure and syntax.The conditional logic correctly wraps the instructions to only display when runbooks are available, and the whitespace control with
{%- endif -%}properly prevents template artifacts.Also applies to: 73-73
tests/llm/utils/test_mock_toolset.py (5)
5-5: Required import for YAML configuration testsThe yaml import is necessary for the new toolsets configuration tests.
105-112: Correct adaptation to new toolset management approachThe function now properly iterates over all toolsets and checks their status, which aligns with the refactored toolset management where all toolsets are exposed with status information.
340-426: Comprehensive test for YAML-based toolset configurationWell-structured test that thoroughly validates the new YAML configuration loading functionality. The test properly verifies that builtin toolsets are loaded and YAML overrides are correctly applied for both status and custom configurations.
471-471: Consistent updates to use new toolsets propertyAll ToolExecutor instantiations correctly updated to use
mock_toolsets.toolsetsinstead of the removedenabled_toolsetsproperty, aligning with the refactored toolset management.Also applies to: 527-527, 586-586, 618-618
660-706: Important test for default behaviorExcellent test that ensures backward compatibility when no YAML configuration is provided. The verification of default toolsets and proper status enums is thorough.
tests/llm/test_workload_health.py (3)
9-9: Proper centralization of configuration importsThe imports correctly use centralized configuration constants and utilities, improving maintainability.
Also applies to: 18-18
82-82: Correct adaptation to centralized configurationThe changes properly use centralized project and experiment configuration. The TODO comment appropriately flags the inconsistency with other tests for future refactoring.
Also applies to: 104-107
57-57: Consistent update to new toolset managementCorrectly updated to use
mock.toolsetsin line with the refactored toolset management approach.tests/llm/test_ask_holmes.py (3)
9-10: Necessary imports for enhanced functionalityThe new imports properly support the CLI mode and centralized configuration approach.
Also applies to: 17-17, 30-30
68-68: Proper use of centralized tracing configurationThe changes correctly leverage centralized configuration for tracer and experiment setup, improving maintainability.
Also applies to: 111-114
280-336: Well-implemented CLI mode with runbook supportThe new CLI mode implementation is well-structured:
- Clear separation between CLI and server modes
- Proper handling of conversation history limitation in CLI mode
- Flexible runbook configuration with sensible defaults
- Improved variable naming for clarity
The TODO about calling real ask_holmes is a good consideration for future refactoring to ensure test fidelity.
tests/llm/utils/mock_toolset.py (4)
733-734: Correct export list updatesThe export list properly reflects the refactored API, exposing the new configuration classes while removing the deprecated ToolsetConfigurator.
526-537: Clean consolidation of toolset managementThe refactored initialization properly encapsulates all toolset loading, configuration, and wrapping logic within the manager class, improving cohesion and maintainability.
538-583: Well-structured toolset configuration logicThe configuration method properly handles:
- Default toolset enabling
- Custom configuration application
- Mode-specific prerequisite checking (optimization for MOCK/GENERATE modes)
The TODO at line 572 mentions adding a timeout for prerequisite checks. This could be important for preventing hangs in LIVE mode.
584-621: Smart toolset wrapping implementationExcellent design decisions:
- Only wrapping in MOCK/GENERATE modes (no overhead in LIVE mode)
- Excluding runbook toolset from mocking ensures real runbook fetching
- Proper status preservation for wrapped toolsets
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/llm/fixtures/test_ask_holmes/60_count_less_than/manifests.yaml (1)
88-90: Optional hardening: drop root and disable privilege escalation
Static analysis (CKV_K8S_20 / CKV_K8S_23) flags these containers for running as root withallowPrivilegeEscalation. Even in test fixtures, setting a minimalsecurityContextis trivial and benefits real-world examples the LLM might learn from.containers: - name: worker image: busybox + securityContext: + runAsNonRoot: true + allowPrivilegeEscalation: falseNot mandatory for the test to pass, but worth tightening.
holmes/plugins/toolsets/runbook/runbook_fetcher.py (1)
3-3: Consider using modern type hint syntax.The project prefers modern Python type hints over importing from the typing module.
-from typing import Any, Dict, List, Optional +from typing import Any, OptionalThen use built-in types:
list[str]instead ofList[str]dict[str, Any]instead ofDict[str, Any]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
CLAUDE.md(1 hunks)holmes/plugins/prompts/_general_instructions.jinja2(2 hunks)holmes/plugins/prompts/_runbook_instructions.jinja2(1 hunks)holmes/plugins/runbooks/__init__.py(1 hunks)holmes/plugins/toolsets/runbook/runbook_fetcher.py(5 hunks)tests/llm/fixtures/test_ask_holmes/40_disabled_toolset/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/60_count_less_than/manifests.yaml(4 hunks)tests/llm/fixtures/test_ask_holmes/60_count_less_than/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/89_runbook_missing_cloudwatch/lambda_performance_troubleshooting.md(1 hunks)tests/llm/fixtures/test_ask_holmes/89_runbook_missing_cloudwatch/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/90_runbook_basic_selection/application_gateway_troubleshooting.md(1 hunks)tests/llm/fixtures/test_ask_holmes/90_runbook_basic_selection/test_case.yaml(1 hunks)tests/llm/utils/mock_toolset.py(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- tests/llm/fixtures/test_ask_holmes/60_count_less_than/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/90_runbook_basic_selection/application_gateway_troubleshooting.md
- tests/llm/fixtures/test_ask_holmes/90_runbook_basic_selection/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/89_runbook_missing_cloudwatch/lambda_performance_troubleshooting.md
- tests/llm/fixtures/test_ask_holmes/40_disabled_toolset/test_case.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- holmes/plugins/prompts/_general_instructions.jinja2
- tests/llm/utils/mock_toolset.py
- holmes/plugins/prompts/_runbook_instructions.jinja2
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml)
Type hints required (mypy configuration in pyproject.toml)
Files:
holmes/plugins/runbooks/__init__.pyholmes/plugins/toolsets/runbook/runbook_fetcher.py
tests/llm/fixtures/**/*
📄 CodeRabbit Inference Engine (CLAUDE.md)
tests/llm/fixtures/**/*: Mock data: tests/llm/fixtures/{test_name}/
All pod names must be unique across tests (e.g., giant-narwhal, blue-whale, sea-turtle) - never reuse pod names between tests
Never use resource names that hint at the problem or expected behavior (e.g., avoid broken-pod, test-project-that-does-not-exist, crashloop-app). Use neutral names that don't give away what the LLM should discover
Files:
tests/llm/fixtures/test_ask_holmes/89_runbook_missing_cloudwatch/test_case.yamltests/llm/fixtures/test_ask_holmes/60_count_less_than/manifests.yaml
🧠 Learnings (5)
📓 Common learnings
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.
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.
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to holmes/plugins/prompts/**/*.jinja2 : Prompts: holmes/plugins/prompts/{name}.jinja2
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to holmes/plugins/toolsets/**/*.yaml : Toolsets: holmes/plugins/toolsets/{name}.yaml or {name}/
tests/llm/fixtures/test_ask_holmes/89_runbook_missing_cloudwatch/test_case.yaml (3)
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/llm/fixtures/**/* : Never use resource names that hint at the problem or expected behavior (e.g., avoid broken-pod, test-project-that-does-not-exist, crashloop-app). Use neutral names that don't give away what the LLM should discover
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: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/llm/fixtures/**/* : Mock data: tests/llm/fixtures/{test_name}/
holmes/plugins/toolsets/runbook/runbook_fetcher.py (4)
Learnt from: nherment
PR: #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].
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to holmes/plugins/toolsets/**/*.yaml : Toolsets: holmes/plugins/toolsets/{name}.yaml or {name}/
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to holmes/plugins/toolsets/**/*.yaml : Bash toolset validates commands for safety
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.
CLAUDE.md (7)
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/llm/**/*.py : Complex investigations should have LLM evaluation tests
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/llm/**/*.py : Each LLM test must use a dedicated namespace app- (e.g., app-01, app-02) to prevent conflicts when tests run simultaneously
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: LLM evaluation tests run automatically in CI
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/**/*.py : All new features require unit tests
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/**/*.py : New toolsets require integration tests with mocks
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/llm/fixtures/**/* : Mock data: tests/llm/fixtures/{test_name}/
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/llm/fixtures/**/* : Never use resource names that hint at the problem or expected behavior (e.g., avoid broken-pod, test-project-that-does-not-exist, crashloop-app). Use neutral names that don't give away what the LLM should discover
tests/llm/fixtures/test_ask_holmes/60_count_less_than/manifests.yaml (3)
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/llm/fixtures/**/* : All pod names must be unique across tests (e.g., giant-narwhal, blue-whale, sea-turtle) - never reuse pod names between tests
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/llm/fixtures/**/* : Never use resource names that hint at the problem or expected behavior (e.g., avoid broken-pod, test-project-that-does-not-exist, crashloop-app). Use neutral names that don't give away what the LLM should discover
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.
🪛 LanguageTool
CLAUDE.md
[grammar] ~168-~168: Ensure spelling is correct
Context: ...Check in pyproject.toml and NEVER use a market/tag that doesn't exist there. Ask the u...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 Checkov (3.2.334)
tests/llm/fixtures/test_ask_holmes/60_count_less_than/manifests.yaml
[MEDIUM] 116-126: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[MEDIUM] 116-126: Minimize the admission of root containers
(CKV_K8S_23)
🔇 Additional comments (6)
tests/llm/fixtures/test_ask_holmes/60_count_less_than/manifests.yaml (1)
90-90: 1-second sleep may trigger Kubernetes back-off too aggressively – verify test stability
Cutting the delay from several seconds tosleep 1speeds up the test, but on a busy or throttled CI node the containers can restart so quickly that Kubernetes enters exponentialCrashLoopBackOff, changing the restart counts the test relies on.Double-check that the fixture still reaches the intended number of restarts before HolmesGPT inspects it. If flakiness appears, consider
sleep 2-3or explicitly resetting the back-off withterminationGracePeriodSeconds: 0.Also applies to: 102-102, 114-114, 126-126
holmes/plugins/runbooks/__init__.py (1)
97-121: Excellent refactor to support multi-path runbook search.The function has been transformed from simple path concatenation to a robust search utility that can locate runbooks across multiple directories. The implementation correctly:
- Provides sensible defaults (current directory if no search paths specified)
- Searches paths in order and returns first match
- Returns
Nonefor not found rather than raising exceptions- Includes comprehensive docstring with clear parameter descriptions
This change enables flexible runbook deployment scenarios where runbooks can be located in different directories, which aligns well with the test fixture requirements and overall system modularity.
tests/llm/fixtures/test_ask_holmes/89_runbook_missing_cloudwatch/test_case.yaml (1)
1-26: Well-designed test case for runbook with missing toolset scenario.This test case effectively validates the enhanced runbook functionality and follows best practices:
✅ Neutral naming: Uses "payment-processor" which doesn't hint at the problem
✅ Hallucination prevention: Explicitly forbids outputs claiming actual results were performed
✅ Proper configuration: Correctly enables some AWS toolsets while disabling CloudWatch to test the missing integration scenario
✅ Clear expectations: Tests that the system finds the runbook but informs users about missing tools
✅ Appropriate tags: runbooks, transparency, and easy tags align with test contentThe test validates the integrated behavior of runbook fetching, toolset status reporting, and user guidance when critical integrations are missing.
holmes/plugins/toolsets/runbook/runbook_fetcher.py (3)
38-65: Excellent enhancement to support multi-path runbook search.The search path implementation is well-designed:
✅ Flexible configuration: Supports additional search paths from toolset config
✅ Sensible defaults: Falls back to default runbooks directory
✅ Clear error handling: Detailed error messages include all attempted search paths
✅ Proper integration: Uses the refactoredget_runbook_by_pathfunction correctlyThe search order (additional paths first, then default) makes sense for allowing test overrides and custom configurations.
70-104: Excellent runbook content formatting and user guidance.The enhanced content formatting significantly improves the user experience:
✅ Clear structure: XML-like tags and proper indentation make runbooks readable
✅ Prevents confusion: Explicit instructions that content is directions, not results
✅ Comprehensive guidance: Clear instructions on how to follow runbook steps
✅ Helpful example: Detailed example shows expected response format with ✅/❌ indicators
✅ Integration guidance: Instructions on handling missing toolsets with documentation linksThis formatting will help users understand how to properly use runbooks and report their findings.
125-144: Good constructor design for configurable search paths.The RunbookToolset constructor properly handles the optional additional search paths:
✅ Clean interface: Optional parameter with sensible default
✅ Proper storage: Paths stored in config dictionary for tool access
✅ Maintains compatibility: Existing usage continues to work without changesThis design enables the test framework to inject test-specific search paths while maintaining backward compatibility.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
CLAUDE.md (1)
167-169: Typo: “market” → “marker”Minor spelling fix to prevent confusion.
-Check in pyproject.toml and NEVER use a market/tag that doesn't exist there. +Check in pyproject.toml and NEVER use a marker/tag that doesn't exist there.tests/llm/fixtures/test_ask_holmes/60_count_less_than/manifests.yaml (2)
88-92: Crash-loop now fires every second – double-check test harness can cope with the higher restart rateReducing the sleep to
1s means each container will restart almost immediately (before the Kubernetes back-off kicks in).
While this speeds the test, it also:
- Generates many log lines/events that slow
kubectl get eventsand clutter CI output.- Increases the chance that the pod enters
CrashLoopBackOffbefore the test assertion is executed, which can make “count < N restarts” style checks flaky.If the intent is simply to ensure at least one restart, consider a 3-5 s delay or explicitly resetting the back-off with
kubectl rollout restartin the test pre-stage.- command: ["sh", "-c", "sleep 1; exit 1"] + # 3-second grace period keeps restarts deterministic while limiting log noise + command: ["sh", "-c", "sleep 3; exit 1"]Also applies to: 100-104, 112-116, 124-127
88-92: Harden container: disallow privilege escalationStatic analysis (CKV_K8S_20/23) flags the busybox container for running without a
securityContext.
Add a minimal block to prevent inadvertent privilege escalation; it does not affect the test logic.image: busybox - command: ["sh", "-c", "sleep 1; exit 1"] + command: ["sh", "-c", "sleep 1; exit 1"] + securityContext: + allowPrivilegeEscalation: false + runAsNonRoot: trueholmes/plugins/toolsets/runbook/runbook_fetcher.py (1)
2-3: Update type hints to use modern Python 3.10+ syntax.The project uses Python >= 3.10 and prefers modern type hint syntax.
-from typing import Any, Dict, List, Optional +from typing import Any, Dict, OptionalThen update the function signatures to use
list[str]instead ofList[str].tests/llm/fixtures/test_ask_holmes/89_runbook_missing_cloudwatch/lambda_performance_troubleshooting.md (3)
10-15: Add language specification to code block for consistency.For better syntax highlighting and consistency with other code blocks in the file, specify the language for this AWS CLI command block.
-``` +```bash aws cloudwatch get-metric-statistics --namespace <lambda-namespace> \ --metric-name Duration --dimensions Name=FunctionName,Value=<function-name> \ --statistics Average,Maximum --start-time <start-time> \ --end-time <end-time> --period 300 -``` +```
26-31: Add language specification to code block for consistency.For better syntax highlighting and consistency with other code blocks in the file, specify the language for this AWS CLI command block.
-``` +```bash aws cloudwatch get-metric-statistics --namespace <lambda-namespace> \ --metric-name InitDuration --dimensions Name=FunctionName,Value=<function-name> \ --statistics Average,Count --start-time <start-time> \ --end-time <end-time> --period 300 -``` +```
35-40: Add language specification to code block for consistency.For better syntax highlighting and consistency with other code blocks in the file, specify the language for this AWS CLI command block.
-``` +```bash aws cloudwatch get-metric-statistics --namespace <lambda-namespace> \ --metric-name ConcurrentExecutions --dimensions Name=FunctionName,Value=<function-name> \ --statistics Maximum --start-time <start-time> \ --end-time <end-time> --period 60 -``` +```
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
CLAUDE.md(1 hunks)holmes/plugins/prompts/_general_instructions.jinja2(2 hunks)holmes/plugins/prompts/_runbook_instructions.jinja2(1 hunks)holmes/plugins/runbooks/__init__.py(1 hunks)holmes/plugins/toolsets/runbook/runbook_fetcher.py(5 hunks)tests/llm/fixtures/test_ask_holmes/40_disabled_toolset/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/60_count_less_than/manifests.yaml(4 hunks)tests/llm/fixtures/test_ask_holmes/60_count_less_than/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/89_runbook_missing_cloudwatch/lambda_performance_troubleshooting.md(1 hunks)tests/llm/fixtures/test_ask_holmes/89_runbook_missing_cloudwatch/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/90_runbook_basic_selection/application_gateway_troubleshooting.md(1 hunks)tests/llm/fixtures/test_ask_holmes/90_runbook_basic_selection/test_case.yaml(1 hunks)tests/llm/utils/mock_toolset.py(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- tests/llm/fixtures/test_ask_holmes/60_count_less_than/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/40_disabled_toolset/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/90_runbook_basic_selection/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/90_runbook_basic_selection/application_gateway_troubleshooting.md
🚧 Files skipped from review as they are similar to previous changes (3)
- holmes/plugins/prompts/_runbook_instructions.jinja2
- tests/llm/utils/mock_toolset.py
- holmes/plugins/prompts/_general_instructions.jinja2
🧰 Additional context used
📓 Path-based instructions (2)
tests/llm/fixtures/**/*
📄 CodeRabbit Inference Engine (CLAUDE.md)
tests/llm/fixtures/**/*: Mock data: tests/llm/fixtures/{test_name}/
All pod names must be unique across tests (e.g., giant-narwhal, blue-whale, sea-turtle) - never reuse pod names between tests
Never use resource names that hint at the problem or expected behavior (e.g., avoid broken-pod, test-project-that-does-not-exist, crashloop-app). Use neutral names that don't give away what the LLM should discover
Files:
tests/llm/fixtures/test_ask_holmes/89_runbook_missing_cloudwatch/test_case.yamltests/llm/fixtures/test_ask_holmes/60_count_less_than/manifests.yamltests/llm/fixtures/test_ask_holmes/89_runbook_missing_cloudwatch/lambda_performance_troubleshooting.md
**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml)
Type hints required (mypy configuration in pyproject.toml)
Files:
holmes/plugins/toolsets/runbook/runbook_fetcher.pyholmes/plugins/runbooks/__init__.py
🧠 Learnings (5)
📓 Common learnings
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.
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.
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to holmes/plugins/prompts/**/*.jinja2 : Prompts: holmes/plugins/prompts/{name}.jinja2
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to holmes/plugins/toolsets/**/*.yaml : Toolsets: holmes/plugins/toolsets/{name}.yaml or {name}/
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.
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]`.
tests/llm/fixtures/test_ask_holmes/89_runbook_missing_cloudwatch/test_case.yaml (2)
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: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/llm/fixtures/**/* : Never use resource names that hint at the problem or expected behavior (e.g., avoid broken-pod, test-project-that-does-not-exist, crashloop-app). Use neutral names that don't give away what the LLM should discover
holmes/plugins/toolsets/runbook/runbook_fetcher.py (4)
Learnt from: nherment
PR: #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].
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to holmes/plugins/toolsets/**/*.yaml : Toolsets: holmes/plugins/toolsets/{name}.yaml or {name}/
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to holmes/plugins/toolsets/**/*.yaml : Bash toolset validates commands for safety
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.
CLAUDE.md (7)
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/llm/**/*.py : Complex investigations should have LLM evaluation tests
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/llm/**/*.py : Each LLM test must use a dedicated namespace app- (e.g., app-01, app-02) to prevent conflicts when tests run simultaneously
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: LLM evaluation tests run automatically in CI
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/**/*.py : All new features require unit tests
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/**/*.py : New toolsets require integration tests with mocks
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/llm/fixtures/**/* : Mock data: tests/llm/fixtures/{test_name}/
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/llm/fixtures/**/* : Never use resource names that hint at the problem or expected behavior (e.g., avoid broken-pod, test-project-that-does-not-exist, crashloop-app). Use neutral names that don't give away what the LLM should discover
tests/llm/fixtures/test_ask_holmes/60_count_less_than/manifests.yaml (3)
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/llm/fixtures/**/* : All pod names must be unique across tests (e.g., giant-narwhal, blue-whale, sea-turtle) - never reuse pod names between tests
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-27T14:10:31.590Z
Learning: Applies to tests/llm/fixtures/**/* : Never use resource names that hint at the problem or expected behavior (e.g., avoid broken-pod, test-project-that-does-not-exist, crashloop-app). Use neutral names that don't give away what the LLM should discover
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.
🪛 LanguageTool
CLAUDE.md
[grammar] ~168-~168: Ensure spelling is correct
Context: ...Check in pyproject.toml and NEVER use a market/tag that doesn't exist there. Ask the u...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 Checkov (3.2.334)
tests/llm/fixtures/test_ask_holmes/60_count_less_than/manifests.yaml
[MEDIUM] 116-126: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[MEDIUM] 116-126: Minimize the admission of root containers
(CKV_K8S_23)
🪛 markdownlint-cli2 (0.17.2)
tests/llm/fixtures/test_ask_holmes/89_runbook_missing_cloudwatch/lambda_performance_troubleshooting.md
10-10: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
26-26: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
35-35: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (4)
holmes/plugins/runbooks/__init__.py (1)
97-120: LGTM! Well-implemented multi-directory runbook search functionality.The enhanced
get_runbook_by_pathfunction properly supports searching across multiple directories while maintaining backward compatibility. The implementation follows good practices with proper type hints, clear documentation, and sensible defaults.holmes/plugins/toolsets/runbook/runbook_fetcher.py (2)
39-118: Enhanced runbook fetching with multi-path search and structured guidance - excellent implementation!The multi-directory search logic is well-implemented with proper error handling. The structured output with XML-like tags and detailed instructions provides clear guidance for downstream processing. The example format is comprehensive and helps users understand the expected output format.
125-144: Good constructor enhancement for additional search paths.The constructor properly handles optional additional search paths and stores them in the config dictionary for use by the fetcher. The implementation maintains backward compatibility while enabling the new multi-path functionality.
tests/llm/fixtures/test_ask_holmes/89_runbook_missing_cloudwatch/test_case.yaml (1)
1-26: Well-designed test case for runbook functionality with missing integrations.The test case properly follows guidelines by using neutral resource names ("payment-processor") that don't hint at the expected behavior. The expected output is appropriately descriptive and includes good safeguards against hallucination by explicitly failing if specific results are claimed to have been performed. The toolset configuration effectively simulates the scenario of missing CloudWatch integration while enabling other AWS services.
mainred
left a comment
There was a problem hiding this comment.
Thanks for the improvment.
|
Thanks @Sheeproid - good review. As discussed, I will merge as is and open a follow up PR improving further. |
No description provided.