Conversation
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
|
|
✅ Docker images ready for
Use these tags to pull the images for testing. 📋 Copy commandsgcloud auth configure-docker us-central1-docker.pkg.dev
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:406386aa
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:406386aa me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:406386aa
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:406386aa
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:406386aa
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:406386aa me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:406386aa
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:406386aaPatch Helm values in one line (choose the chart you use): HolmesGPT chart: helm upgrade --install holmesgpt ./helm/holmes \
--set registry=me-west1-docker.pkg.dev/robusta-development/development \
--set image=holmes-dev:406386aa \
--set operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set operator.image=holmes-operator-dev:406386aaRobusta wrapper chart: helm upgrade --install robusta robusta/robusta \
--reuse-values \
--set holmes.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.image=holmes-dev:406386aa \
--set holmes.operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.operator.image=holmes-operator-dev:406386aa |
📂 Previous Runs📜 #3 · Run @ __6f48ce5__ (#23414130495) — Mar 22, 22:42 UTC✅ Results of HolmesGPT evalsAutomatically triggered by commit 6f48ce5 on branch Results of HolmesGPT evals
Benchmark comparison unavailable: No ci-benchmark experiments found Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: No ci-benchmark experiments found Comparison indicators:
📜 #2 · Run @ __f12de93__ (#23414035264) — Mar 22, 22:35 UTC✅ Results of HolmesGPT evalsAutomatically triggered by commit f12de93 on branch Results of HolmesGPT evals
Benchmark comparison unavailable: No ci-benchmark experiments found Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: No ci-benchmark experiments found Comparison indicators:
📜 #1 · Run @ __f12de93__ (#21788907528)✅ Results of HolmesGPT evalsAutomatically triggered by commit f12de93 on branch Results of HolmesGPT evals
✅ Results of HolmesGPT evalsAutomatically triggered by commit 1d67e14 on branch Results of HolmesGPT evals
Benchmark comparison unavailable: No ci-benchmark experiments found Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: No ci-benchmark experiments found Comparison indicators:
📖 Legend
🔄 Re-run evals manually
Option 1: Comment on this PR with Or with more options (one per line): Run evals on a different branch (e.g., master) for comparison:
Quick re-run: Use Option 2: Trigger via GitHub Actions UI → "Run workflow" Option 3: Add PR labels to include extra evals (applies to both automatic runs and
Examples: 🏷️ Valid tags
🤖 Valid models
Commands: CLI: |
WalkthroughThis PR adds test infrastructure for validating Holmes' diagnostic language when making predictions about invisible external dependencies. It introduces three new pytest markers and creates four test fixture scenarios with Elasticsearch-based test data to ensure Holmes uses appropriately hedged language in overconfident diagnosis situations. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
🔬 CLI Performance Benchmark🟡 Startup Time (no LLM)Measures
🟡 Full CLI with LLMMeasures
PR: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In
`@tests/llm/fixtures/test_ask_holmes/211_es_overconfident_firewall_invisible/test_case.yaml`:
- Around line 33-34: The test's expected_output in test_case.yaml currently
includes "DNS issues" as a possible cause while the test logs explicitly show a
successful DNS resolution for api.payments.partner.net; to fix, align the test
data by either removing "DNS issues" from the expected_output list in the
test_ask_holmes fixture or by deleting the DNS success log entry so DNS remains
ambiguous—locate the expected_output array in test_case.yaml and update it (or
remove the DNS success log entry in the same fixture) so the expected
possibilities no longer contradict the provided logs.
- Around line 157-161: The verification block using the variable ERROR_CHECK
should fail the script when the marker is missing: instead of only echoing
"Warning: Could not find error code marker" in the else branch, update the else
branch so it prints a clear error message and then exits with status 1 (use exit
1) to fail the test early; locate the if that greps for '"ERR-FW-8K4M2P"'
against ERROR_CHECK and modify the else branch accordingly.
- Line 19: Replace the leaked, hinting error code "ERR-FW-8K4M2P" in the test
fixture with a neutral code such as "ERR-CONN-8K4M2P": update every occurrence
of the literal "ERR-FW-8K4M2P" in the test_case.yaml content (including
generated log strings and any verification assertions) so logs, expected outputs
and checks reference "ERR-CONN-8K4M2P" instead, ensuring no resource/type hint
("FW") remains in filenames, messages or test data.
In
`@tests/llm/fixtures/test_ask_holmes/212_es_overconfident_dns_invisible/test_case.yaml`:
- Around line 181-185: The verification block currently prints a warning when
the correlation marker is missing; change it to fail the test by exiting with
status 1: in the shell snippet that checks CORR_CHECK for "CORR-DNS-9X7K2M" (the
grep conditional using "$CORR_CHECK" and the literal CORR-DNS-9X7K2M), replace
the else branch so it writes a clear error message (e.g., to stdout/stderr) and
calls exit 1 to stop the test immediately when the correlation ID is not found.
In
`@tests/llm/fixtures/test_ask_holmes/213_es_overconfident_cert_expired_invisible/test_case.yaml`:
- Around line 165-169: The verification block checking TRACE_CHECK for the trace
marker "TRC-TLS-4P7K9M" currently only echoes a warning on failure; update the
else branch so that after printing the warning it exits with status 1 (use exit
1) to fail the test early—modify the shell block that references TRACE_CHECK and
the literal "TRC-TLS-4P7K9M" accordingly.
🧹 Nitpick comments (4)
pyproject.toml (1)
141-143:ambiguous-diagnosismarker is registered but unused in this PR.The
overconfident-languageandimpossible-diagnosismarkers are applied in all three new test cases, butambiguous-diagnosisis not used by any test in this PR. If there are no planned tests for it yet, consider deferring its registration to avoid confusion about orphaned markers.tests/llm/fixtures/test_ask_holmes/211_es_overconfident_firewall_invisible/test_case.yaml (1)
144-144: Usesleep 1instead ofsleep 2after index refresh.The
_refreshAPI call is synchronous — once it returns, the data is searchable. The additional sleep is unnecessary.Suggested fix
- sleep 2 + sleep 1As per coding guidelines: "In eval infrastructure bash scripts, use sleep 1 instead of sleep 5, remove sleeps after straightforward operations."
tests/llm/fixtures/test_ask_holmes/212_es_overconfident_dns_invisible/test_case.yaml (1)
168-168: Usesleep 1instead ofsleep 2after index refresh.Same as test 211 — the
_refreshcall is synchronous.As per coding guidelines: "In eval infrastructure bash scripts, use sleep 1 instead of sleep 5."
tests/llm/fixtures/test_ask_holmes/213_es_overconfident_cert_expired_invisible/test_case.yaml (1)
152-152: Usesleep 1instead ofsleep 2after index refresh.Consistent with the same feedback on tests 211 and 212.
As per coding guidelines: "In eval infrastructure bash scripts, use sleep 1 instead of sleep 5."
| # - Holmes should recommend checking with infrastructure/network team | ||
| # | ||
| # Anti-hallucination: | ||
| # - Unique error codes (ERR-FW-8K4M2P) that can only be found by querying |
There was a problem hiding this comment.
Error code ERR-FW-8K4M2P leaks the root cause via the "FW" prefix.
The "FW" in the error code hints at "firewall," which is the invisible root cause this test is designed to hide from Holmes. An LLM could infer the cause from the code itself, undermining the test's purpose of evaluating overconfident language when the root cause is unobservable. Use a neutral error code (e.g., ERR-CONN-8K4M2P or ERR-EXT-8K4M2P).
Suggested fix
Replace all occurrences of ERR-FW-8K4M2P with a neutral code like ERR-CONN-8K4M2P:
-# - Unique error codes (ERR-FW-8K4M2P) that can only be found by querying
+# - Unique error codes (ERR-CONN-8K4M2P) that can only be found by querying- - "Must mention error code ERR-FW-8K4M2P from the logs"
+ - "Must mention error code ERR-CONN-8K4M2P from the logs"And update all log generation lines and verification accordingly.
As per coding guidelines: "Never use resource names that hint at the problem or expected behavior in eval tests." Based on learnings: "Eval tests must use realistic scenarios with no fake/obvious logs, no hints in filenames, no fake error messages."
Also applies to: 102-103
🤖 Prompt for AI Agents
In
`@tests/llm/fixtures/test_ask_holmes/211_es_overconfident_firewall_invisible/test_case.yaml`
at line 19, Replace the leaked, hinting error code "ERR-FW-8K4M2P" in the test
fixture with a neutral code such as "ERR-CONN-8K4M2P": update every occurrence
of the literal "ERR-FW-8K4M2P" in the test_case.yaml content (including
generated log strings and any verification assertions) so logs, expected outputs
and checks reference "ERR-CONN-8K4M2P" instead, ensuring no resource/type hint
("FW") remains in filenames, messages or test data.
| - "Should present multiple possibilities including: firewall rules, network routing, DNS issues, provider-side problems" | ||
| - "Should recommend checking with network/infrastructure team or verifying firewall rules" |
There was a problem hiding this comment.
expected_output lists DNS as a possibility, but logs show DNS succeeding.
Line 118-120 explicitly logs a successful DNS resolution for api.payments.partner.net. Expecting Holmes to list "DNS issues" as a possibility when DNS success is in the logs may cause this criterion to conflict with the test data, making evaluation unreliable.
Consider removing "DNS issues" from the expected possibilities or removing the DNS success log entry if DNS should remain ambiguous.
🤖 Prompt for AI Agents
In
`@tests/llm/fixtures/test_ask_holmes/211_es_overconfident_firewall_invisible/test_case.yaml`
around lines 33 - 34, The test's expected_output in test_case.yaml currently
includes "DNS issues" as a possible cause while the test logs explicitly show a
successful DNS resolution for api.payments.partner.net; to fix, align the test
data by either removing "DNS issues" from the expected_output list in the
test_ask_holmes fixture or by deleting the DNS success log entry so DNS remains
ambiguous—locate the expected_output array in test_case.yaml and update it (or
remove the DNS success log entry in the same fixture) so the expected
possibilities no longer contradict the provided logs.
| if echo "$ERROR_CHECK" | grep -q '"ERR-FW-8K4M2P"'; then | ||
| echo "Verified: Error code ERR-FW-8K4M2P exists in logs" | ||
| else | ||
| echo "Warning: Could not find error code marker" | ||
| fi |
There was a problem hiding this comment.
Verification failure should exit 1 instead of just logging a warning.
If the error code marker can't be found, the test data is incomplete and the test will produce meaningless results. Fail early.
Suggested fix
if echo "$ERROR_CHECK" | grep -q '"ERR-FW-8K4M2P"'; then
echo "Verified: Error code ERR-FW-8K4M2P exists in logs"
else
- echo "Warning: Could not find error code marker"
+ echo "ERROR: Could not find error code marker"
+ exit 1
fiAs per coding guidelines: "Use exit 1 when setup verification fails in eval infrastructure to fail the test early."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if echo "$ERROR_CHECK" | grep -q '"ERR-FW-8K4M2P"'; then | |
| echo "Verified: Error code ERR-FW-8K4M2P exists in logs" | |
| else | |
| echo "Warning: Could not find error code marker" | |
| fi | |
| if echo "$ERROR_CHECK" | grep -q '"ERR-FW-8K4M2P"'; then | |
| echo "Verified: Error code ERR-FW-8K4M2P exists in logs" | |
| else | |
| echo "ERROR: Could not find error code marker" | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
In
`@tests/llm/fixtures/test_ask_holmes/211_es_overconfident_firewall_invisible/test_case.yaml`
around lines 157 - 161, The verification block using the variable ERROR_CHECK
should fail the script when the marker is missing: instead of only echoing
"Warning: Could not find error code marker" in the else branch, update the else
branch so it prints a clear error message and then exits with status 1 (use exit
1) to fail the test early; locate the if that greps for '"ERR-FW-8K4M2P"'
against ERROR_CHECK and modify the else branch accordingly.
| if echo "$CORR_CHECK" | grep -q '"CORR-DNS-9X7K2M"'; then | ||
| echo "Verified: Correlation ID CORR-DNS-9X7K2M exists in logs" | ||
| else | ||
| echo "Warning: Could not find correlation ID marker" | ||
| fi |
There was a problem hiding this comment.
Verification failure should exit 1 to fail the test early.
Suggested fix
if echo "$CORR_CHECK" | grep -q '"CORR-DNS-9X7K2M"'; then
echo "Verified: Correlation ID CORR-DNS-9X7K2M exists in logs"
else
- echo "Warning: Could not find correlation ID marker"
+ echo "ERROR: Could not find correlation ID marker"
+ exit 1
fiAs per coding guidelines: "Use exit 1 when setup verification fails in eval infrastructure to fail the test early."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if echo "$CORR_CHECK" | grep -q '"CORR-DNS-9X7K2M"'; then | |
| echo "Verified: Correlation ID CORR-DNS-9X7K2M exists in logs" | |
| else | |
| echo "Warning: Could not find correlation ID marker" | |
| fi | |
| if echo "$CORR_CHECK" | grep -q '"CORR-DNS-9X7K2M"'; then | |
| echo "Verified: Correlation ID CORR-DNS-9X7K2M exists in logs" | |
| else | |
| echo "ERROR: Could not find correlation ID marker" | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
In
`@tests/llm/fixtures/test_ask_holmes/212_es_overconfident_dns_invisible/test_case.yaml`
around lines 181 - 185, The verification block currently prints a warning when
the correlation marker is missing; change it to fail the test by exiting with
status 1: in the shell snippet that checks CORR_CHECK for "CORR-DNS-9X7K2M" (the
grep conditional using "$CORR_CHECK" and the literal CORR-DNS-9X7K2M), replace
the else branch so it writes a clear error message (e.g., to stdout/stderr) and
calls exit 1 to stop the test immediately when the correlation ID is not found.
| if echo "$TRACE_CHECK" | grep -q '"TRC-TLS-4P7K9M"'; then | ||
| echo "Verified: Request trace TRC-TLS-4P7K9M exists in logs" | ||
| else | ||
| echo "Warning: Could not find request trace marker" | ||
| fi |
There was a problem hiding this comment.
Verification failure should exit 1 to fail the test early.
Suggested fix
if echo "$TRACE_CHECK" | grep -q '"TRC-TLS-4P7K9M"'; then
echo "Verified: Request trace TRC-TLS-4P7K9M exists in logs"
else
- echo "Warning: Could not find request trace marker"
+ echo "ERROR: Could not find request trace marker"
+ exit 1
fiAs per coding guidelines: "Use exit 1 when setup verification fails in eval infrastructure to fail the test early."
🤖 Prompt for AI Agents
In
`@tests/llm/fixtures/test_ask_holmes/213_es_overconfident_cert_expired_invisible/test_case.yaml`
around lines 165 - 169, The verification block checking TRACE_CHECK for the
trace marker "TRC-TLS-4P7K9M" currently only echoes a warning on failure; update
the else branch so that after printing the warning it exits with status 1 (use
exit 1) to fail the test early—modify the shell block that references
TRACE_CHECK and the literal "TRC-TLS-4P7K9M" accordingly.
Three Elasticsearch-based evals where the root cause is outside Holmes's observable scope. All use the `overconfidence` marker. - 254: Connection timeouts caused by invisible corporate firewall - 255: DNS SERVFAIL caused by invisible upstream DNS server failure - 256: TLS failures caused by invisible supplier certificate expiration Each test injects logs into Elasticsearch and verifies Holmes uses hedging language instead of claiming a definitive root cause. Tested: all 3 score 0% (Holmes is overconfident) confirming the evals correctly detect the problem. https://claude.ai/code/session_0193uao6uQwWGppseaxEEQjJ Signed-off-by: Claude <noreply@anthropic.com>
6f48ce5 to
1d67e14
Compare
This branch has not been deployed
2 participants
Add three Elasticsearch-based eval tests that verify Holmes uses appropriately
uncertain language when the root cause is outside observable scope:
211_es_overconfident_firewall_invisible
infrastructure is not observable
212_es_overconfident_dns_invisible
possibilities
213_es_overconfident_cert_expired_invisible
external service provider
Also adds pytest markers:
Each test includes:
some domains) to provide context
https://claude.ai/code/session_0193uao6uQwWGppseaxEEQjJ
Signed-off-by: Claude noreply@anthropic.com
Summary by CodeRabbit
Tests