new evals for logging and more - #601
Conversation
WalkthroughThis change adds a suite of new test fixtures, manifests, and supporting files for the Holmes system, focusing on Kubernetes log fetching, error handling, namespace resolution, autoscaling confusion, and health check scenarios. It introduces new test cases, Kubernetes manifests, supporting tool output fixtures, and updates to allowed evaluation tags and test markers. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Holmes System
participant Kubernetes Cluster
participant Toolset/CLI
User->>Holmes System: Submit log/diagnostic query
Holmes System->>Kubernetes Cluster: Apply manifest (test setup)
Holmes System->>Toolset/CLI: Run kubectl/tool command (e.g., get pods, fetch logs)
Toolset/CLI->>Kubernetes Cluster: Execute command
Kubernetes Cluster-->>Toolset/CLI: Command output (logs, errors, resource info)
Toolset/CLI-->>Holmes System: Return command output
Holmes System->>Holmes System: Analyze output, match expected behavior
Holmes System->>Kubernetes Cluster: Delete manifest (test teardown)
Holmes System-->>User: Return answer or error message
Estimated code review effort2 (10–30 minutes) Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
tests/llm/fixtures/test_ask_holmes/55_fetch_error_logs_with_errors/test_case.yaml (1)
1-2: Mismatch between prompt (“last 10 ERROR logs”) and expected output (single line)The deployment emits five distinct
ERROR:lines.
Requesting 10 and then expecting only one line (exact match) risks flaky grading. Either:
- Trim the prompt to “show any ERROR log…”, or
- Expand
expected_outputto a multiline block / regex that matches all returned lines.
🧹 Nitpick comments (4)
tests/llm/fixtures/test_ask_holmes/57_fetch_logs_no_namespace_confusion/manifest.yaml (1)
37-38: Consider shortening the sleep to speed-up CI
sleep 300holds the pod for 5 minutes which is longer than most tests need and can slow downkubectl logs/ teardown.
If nothing relies on that length, reduce to e.g.sleep 60.tests/llm/fixtures/test_ask_holmes/57_fetch_logs_no_namespace_confusion/test_case.yaml (1)
2-3: Make the assertion less brittle – use a regex or contains checkThe expected output is a hard-coded full sentence. Any paraphrase (e.g. “deployment video-streamer doesn’t exist in app-57…”) will fail the test even though it is semantically correct. Earlier learnings (#586) warned about this.
Suggestion:
-expected_output: "video-streamer not found in namespace app-57, did you mean namespace test-57?" +expected_output_regex: ".*video-streamer.*app-57.*test-57.*"(or switch to a
contains_anystyle check used in other fixtures).This keeps the test goal (detecting namespace confusion) while allowing wording variations.
tests/llm/fixtures/test_ask_holmes/56_fetch_error_logs_no_errors/test_case.yaml (1)
1-2: Avoid brittle exact-match assertion for “no errors” responseRequiring the model to emit the exact string
No ERROR logs foundcan create false negatives.
Consider asserting that the output contains an indicative phrase or that the list length is zero, rather than strict equality.tests/llm/fixtures/test_ask_holmes/55_fetch_error_logs_with_errors/test_case_with_timeout.yaml.example (1)
5-5: Document why this file is committed and when to use the 120 s timeoutAs an
*.exampleit will never be executed by CI. Add a comment header explaining how/when a developer should copy-rename it, or move it to docs to avoid clutter.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
tests/llm/fixtures/test_ask_holmes/55_fetch_error_logs_with_errors/manifest.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/55_fetch_error_logs_with_errors/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/55_fetch_error_logs_with_errors/test_case_with_timeout.yaml.example(1 hunks)tests/llm/fixtures/test_ask_holmes/56_fetch_error_logs_no_errors/manifest.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/56_fetch_error_logs_no_errors/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/57_fetch_logs_no_namespace_confusion/manifest.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/57_fetch_logs_no_namespace_confusion/test_case.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: nherment
PR: robusta-dev/holmesgpt#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/llm/fixtures/test_ask_holmes/57_fetch_logs_no_namespace_confusion/test_case.yaml (1)
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.
tests/llm/fixtures/test_ask_holmes/56_fetch_error_logs_no_errors/test_case.yaml (1)
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.
tests/llm/fixtures/test_ask_holmes/57_fetch_logs_no_namespace_confusion/manifest.yaml (1)
Learnt from: nherment
PR: robusta-dev/holmesgpt#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/llm/fixtures/test_ask_holmes/55_fetch_error_logs_with_errors/test_case_with_timeout.yaml.example (1)
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.
tests/llm/fixtures/test_ask_holmes/55_fetch_error_logs_with_errors/test_case.yaml (1)
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.
tests/llm/fixtures/test_ask_holmes/55_fetch_error_logs_with_errors/manifest.yaml (1)
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.
tests/llm/fixtures/test_ask_holmes/56_fetch_error_logs_no_errors/manifest.yaml (1)
Learnt from: nherment
PR: robusta-dev/holmesgpt#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.
🪛 Checkov (3.2.334)
tests/llm/fixtures/test_ask_holmes/57_fetch_logs_no_namespace_confusion/manifest.yaml
[MEDIUM] 6-45: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[MEDIUM] 6-45: Minimize the admission of root containers
(CKV_K8S_23)
tests/llm/fixtures/test_ask_holmes/55_fetch_error_logs_with_errors/manifest.yaml
[MEDIUM] 6-45: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[MEDIUM] 6-45: Minimize the admission of root containers
(CKV_K8S_23)
tests/llm/fixtures/test_ask_holmes/56_fetch_error_logs_no_errors/manifest.yaml
[MEDIUM] 6-50: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[MEDIUM] 6-50: Minimize the admission of root containers
(CKV_K8S_23)
⏰ 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
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/llm/fixtures/test_ask_holmes/63_fetch_error_logs_no_errors/manifest.yaml (2)
21-51: Harden the test pod with a non-root security contextEven for fixtures it’s good practice to model secure defaults. Running BusyBox as root with privilege escalation enabled conflicts with CKV_K8S_20/23 and may conceal policy regressions.
containers: - name: app-container image: busybox:1.35 + securityContext: + runAsNonRoot: true + runAsUser: 1000 + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true command: ["/bin/sh"]
23-23: Pin the BusyBox image by digestFloating tags (
busybox:1.35) may unexpectedly change and break the deterministic nature of the fixture. Consider pinning to a content-addressable digest (e.g.busybox@sha256:...) to guarantee reproducible tests.tests/llm/fixtures/test_ask_holmes/62_fetch_error_logs_with_errors/manifest.yaml (2)
21-45: Mirror the security hardening in error-case fixtureSame rationale as the “no-errors” fixture: add an explicit non-root
securityContextto keep the test resource compliant with baseline policies.containers: - name: app-container image: busybox:1.35 + securityContext: + runAsNonRoot: true + runAsUser: 1000 + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true command: ["/bin/sh"]
23-23: Use a digest for the BusyBox imagePinning the image avoids silent upgrades that could alter log output and invalidate snapshot expectations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/llm/fixtures/test_ask_holmes/62_fetch_error_logs_with_errors/manifest.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/62_fetch_error_logs_with_errors/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/63_fetch_error_logs_no_errors/manifest.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/63_fetch_error_logs_no_errors/test_case.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- tests/llm/fixtures/test_ask_holmes/63_fetch_error_logs_no_errors/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/62_fetch_error_logs_with_errors/test_case.yaml
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: nherment
PR: robusta-dev/holmesgpt#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/llm/fixtures/test_ask_holmes/62_fetch_error_logs_with_errors/manifest.yaml (1)
Learnt from: nherment
PR: robusta-dev/holmesgpt#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/llm/fixtures/test_ask_holmes/63_fetch_error_logs_no_errors/manifest.yaml (1)
Learnt from: nherment
PR: robusta-dev/holmesgpt#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.
🪛 Checkov (3.2.334)
tests/llm/fixtures/test_ask_holmes/62_fetch_error_logs_with_errors/manifest.yaml
[MEDIUM] 6-45: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[MEDIUM] 6-45: Minimize the admission of root containers
(CKV_K8S_23)
tests/llm/fixtures/test_ask_holmes/63_fetch_error_logs_no_errors/manifest.yaml
[MEDIUM] 6-50: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[MEDIUM] 6-50: Minimize the admission of root containers
(CKV_K8S_23)
⏰ 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
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
tests/llm/fixtures/test_ask_holmes/65_health_check_followup/manifest.yaml (1)
24-60: Harden the container with an explicitsecurityContext.Checkov surfaced CKV_K8S_20 and CKV_K8S_23: the container currently runs as root and allows privilege escalation. Even in fixtures it’s better to encode secure defaults so copy-pasted samples don’t propagate bad practice.
containers: - name: payment-processor image: busybox:1.35 + securityContext: + runAsNonRoot: true + runAsUser: 1000 + allowPrivilegeEscalation: false + capabilities: + drop: ["ALL"] + seccompProfile: + type: RuntimeDefault
🧹 Nitpick comments (3)
tests/llm/fixtures/test_ask_holmes/65_health_check_followup/manifest.yaml (1)
41-47: Consider adding liveness / readiness probes instead of a blind infinite loop.Relying solely on log heartbeats without K8s probes means the Deployment can’t automatically restart misbehaving pods. A minimal HTTP-GET or
execprobe on port 8080 (even a dummy/healthz) would keep the fixture realistic and future-proof for health-related evaluations.tests/llm/fixtures/test_ask_holmes/64_keda_vs_hpa_confusion/manifest.yaml (2)
24-44: Add a basicsecurityContextto silence container-level security scannersCheckov flags two medium-severity findings (
allowPrivilegeEscalation,run as root).
Even for test fixtures, adding a minimal security context costs almost nothing and keeps CI pipelines green when full-repo scanners are enabled.containers: - name: invoice-generator image: busybox + securityContext: + runAsNonRoot: true + allowPrivilegeEscalation: false + capabilities: + drop: + - ALL ports: - containerPort: 8080
26-28: Pin the BusyBox image to a digest or specific tag
busybox:latestchanges frequently. Pinning avoids test flakiness when the upstream image is updated or removed.Example:
- image: busybox + image: busybox:1.36.1 # or use a sha256 digest
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
tests/llm/fixtures/test_ask_holmes/62_fetch_error_logs_with_errors/kubectl_find_resourcepayment-service_pod.txt.AUTOGENERATED(1 hunks)tests/llm/fixtures/test_ask_holmes/63_fetch_error_logs_no_errors/kubectl_find_resourceuser-api_pod.txt.AUTOGENERATED(1 hunks)tests/llm/fixtures/test_ask_holmes/64_keda_vs_hpa_confusion/kubectl_describedeployment_invoice-generator_invoices-64.txt.AUTOGENERATED(1 hunks)tests/llm/fixtures/test_ask_holmes/64_keda_vs_hpa_confusion/kubectl_find_resourceinvoice-generator_deployment.txt.AUTOGENERATED(1 hunks)tests/llm/fixtures/test_ask_holmes/64_keda_vs_hpa_confusion/kubectl_get_by_kind_in_namespacescaledobject_invoices-64.txt.AUTOGENERATED(1 hunks)tests/llm/fixtures/test_ask_holmes/64_keda_vs_hpa_confusion/manifest.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/64_keda_vs_hpa_confusion/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/65_health_check_followup/kubectl_describepod_payment-processor-598445cb54-xd5s8_production-65.txt.AUTOGENERATED(1 hunks)tests/llm/fixtures/test_ask_holmes/65_health_check_followup/kubectl_get_by_namedeployment_payment-processor_production-65.txt.AUTOGENERATED(1 hunks)tests/llm/fixtures/test_ask_holmes/65_health_check_followup/kubectl_lineage_childrendeployment_payment-processor_production-65.txt.AUTOGENERATED(1 hunks)tests/llm/fixtures/test_ask_holmes/65_health_check_followup/manifest.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/65_health_check_followup/test_case.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (8)
- tests/llm/fixtures/test_ask_holmes/65_health_check_followup/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/65_health_check_followup/kubectl_get_by_namedeployment_payment-processor_production-65.txt.AUTOGENERATED
- tests/llm/fixtures/test_ask_holmes/64_keda_vs_hpa_confusion/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/64_keda_vs_hpa_confusion/kubectl_get_by_kind_in_namespacescaledobject_invoices-64.txt.AUTOGENERATED
- tests/llm/fixtures/test_ask_holmes/65_health_check_followup/kubectl_lineage_childrendeployment_payment-processor_production-65.txt.AUTOGENERATED
- tests/llm/fixtures/test_ask_holmes/64_keda_vs_hpa_confusion/kubectl_find_resourceinvoice-generator_deployment.txt.AUTOGENERATED
- tests/llm/fixtures/test_ask_holmes/64_keda_vs_hpa_confusion/kubectl_describedeployment_invoice-generator_invoices-64.txt.AUTOGENERATED
- tests/llm/fixtures/test_ask_holmes/65_health_check_followup/kubectl_describepod_payment-processor-598445cb54-xd5s8_production-65.txt.AUTOGENERATED
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/llm/fixtures/test_ask_holmes/62_fetch_error_logs_with_errors/kubectl_find_resourcepayment-service_pod.txt.AUTOGENERATED
- tests/llm/fixtures/test_ask_holmes/63_fetch_error_logs_no_errors/kubectl_find_resourceuser-api_pod.txt.AUTOGENERATED
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: nherment
PR: robusta-dev/holmesgpt#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: 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: 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.
🪛 Checkov (3.2.334)
tests/llm/fixtures/test_ask_holmes/64_keda_vs_hpa_confusion/manifest.yaml
[MEDIUM] 6-45: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[MEDIUM] 6-45: Minimize the admission of root containers
(CKV_K8S_23)
tests/llm/fixtures/test_ask_holmes/65_health_check_followup/manifest.yaml
[MEDIUM] 6-59: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[MEDIUM] 6-59: Minimize the admission of root containers
(CKV_K8S_23)
⏰ 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: Pre-commit checks
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
…sta-dev/holmesgpt into add-3-new-log-related-evals
Sheeproid
left a comment
Sheeproid
left a comment
There was a problem hiding this comment.
see the comment about the marker
2 participants
No description provided.