Add toolsets_matrix support for comparing toolset configs on same eval scenarios - #1609
Conversation
…l scenarios
Adds a new `toolsets_matrix` field to test_case.yaml that lists multiple
toolset config filenames. Each file creates a separate test variant with
the same user_prompt, expected_output, and infrastructure - only the
toolset configuration changes. This enables comparing builtin toolsets
vs HTTP toolsets vs MCP on identical scenarios.
Changes:
- HolmesTestCase: add toolsets_matrix, toolsets_config_name, toolsets_config_path fields
- MockHelper: add _expand_toolsets_matrix() post-processing step after test case loading
- MockToolsetManager: accept toolsets_config_path to override default toolsets.yaml resolution
- test_ask_holmes.py, test_investigate.py: pass toolsets_config_path through
Example test_case.yaml usage:
toolsets_matrix:
- toolsets_builtin.yaml
- toolsets_http.yaml
Produces test IDs like: test_ask_holmes[01_test[builtin]-model-env]
https://claude.ai/code/session_014iLLnCruoc4xRE3r8aiXJh
Signed-off-by: Claude <noreply@anthropic.com>
Adds toolsets_http.yaml to 9 Datadog eval tests (91a-91h, 164) with HTTP toolset configs that hit the same Datadog APIs using the generic HTTP toolset instead of the native Datadog toolsets. Each test now runs twice: once with the builtin toolset and once with the HTTP toolset. HTTP toolset auth uses DD-API-KEY header + DD-APPLICATION-KEY via default_headers, with llm_instructions documenting the API endpoints. Coverage: - Metrics API (GET /api/v1/metrics, /api/v1/query, /api/v2/metrics): 91a-91e, 91g - Logs API (POST /api/v2/logs/events/search): 91f, 91h - Traces API (POST /api/v2/spans/events/search, /analytics/aggregate): 164 https://claude.ai/code/session_014iLLnCruoc4xRE3r8aiXJh Signed-off-by: Claude <noreply@anthropic.com>
📂 Previous Runs📜 Run @ f107fa9 (#22273304553)✅ Results of HolmesGPT evalsAutomatically triggered by commit f107fa9 on branch Results of HolmesGPT evals
📜 Run @ 69825bb (#22272984724)✅ Results of HolmesGPT evalsAutomatically triggered by commit 69825bb on branch Results of HolmesGPT evals
|
| Icon | Meaning |
|---|---|
| ✅ | The test was successful |
| ➖ | The test was skipped |
| The test failed but is known to be flaky or known to fail | |
| 🚧 | The test had a setup failure (not a code regression) |
| 🔧 | The test failed due to mock data issues (not a code regression) |
| 🚫 | The test was throttled by API rate limits/overload |
| ❌ | The test failed and should be fixed before merging the PR |
🔄 Re-run evals manually
⚠️ Warning:/evalcomments always run using the workflow from master, not from this PR branch. If you modified the GitHub Action (e.g., added secrets or env vars), those changes won't take effect.To test workflow changes, use the GitHub CLI or Actions UI instead:
gh workflow run eval-regression.yaml --repo HolmesGPT/holmesgpt --ref claude/evals-toolset-matrix-mwdXc -f markers=regression -f filter=
Option 1: Comment on this PR with /eval:
/eval
tags: regression
Or with more options (one per line):
/eval
model: gpt-4o
tags: regression
filter: 09_crashpod
iterations: 5
Run evals on a different branch (e.g., master) for comparison:
/eval
branch: master
tags: regression
| Option | Description |
|---|---|
model |
Model(s) to test (default: same as automatic runs) |
tags |
Pytest tags / markers (no default - runs all tests!) |
filter |
Pytest -k filter (use /list to see valid eval names) |
iterations |
Number of runs, max 10 |
branch |
Run evals on a different branch (for cross-branch comparison) |
Quick re-run: Use /rerun to re-run the most recent /eval on this PR with the same parameters.
Option 2: Trigger via GitHub Actions UI → "Run workflow"
Option 3: Add PR labels to include extra evals in automatic regression runs:
| Label | Effect |
|---|---|
evals-tag-<name> |
Run tests with tag <name> alongside regression |
evals-id-<name> |
Run a specific eval by test ID |
Examples: evals-tag-easy, evals-id-09_crashpod
🏷️ Valid tags
benchmark, chain-of-causation, compaction, confluence, context_window, coralogix, counting, database, datadog, datetime, easy, elasticsearch, embeds, fast, frontend, grafana-dashboard, hard, integration, kafka, kubernetes, leaked-information, logs, loki, medium, metrics, network, newrelic, no-cicd, numerical, one-test, port-forward, prometheus, question-answer, regression, runbooks, slackbot, storage, toolset-limitation, traces, transparency
Commands: /eval · /rerun · /list
CLI: gh workflow run eval-regression.yaml --repo HolmesGPT/holmesgpt --ref claude/evals-toolset-matrix-mwdXc -f markers=regression -f filter=
|
✅ Docker image ready for
Use this tag to pull the image for testing. 📋 Copy commandsgcloud auth configure-docker us-central1-docker.pkg.dev
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:7084e7fb
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:7084e7fb me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:7084e7fb
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:7084e7fbPatch 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:7084e7fbRobusta 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:7084e7fb |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 WalkthroughThis PR introduces a toolsets_matrix feature for test fixtures that enables running the same test case with multiple toolset configurations. It adds a new HTTP-based Datadog Logs API toolset configuration and propagates the toolsets_config_path throughout the test infrastructure to support dynamic toolset loading. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/llm/fixtures/test_ask_holmes/91b_datadog_metrics_pod_exists/test_case.yaml (1)
20-35:⚠️ Potential issue | 🟠 Major
expected_outputis incompatible with thetoolsets_http.yamlmatrix variant — HTTP variant will always fail.The
expected_output(lines 20–26) asserts:
- An embed with
type: "datadogql"andtool_name: "query_datadog_metrics"These tokens are produced exclusively by the builtin Holmes Datadog toolset. The HTTP toolset (
toolsets_http.yaml) calls the Datadog REST API directly and returns raw JSON; it has no mechanism to emitdatadogqlembed markers or surface a tool namedquery_datadog_metrics. As a result, thetoolsets_http.yamlmatrix variant will always fail this assertion, making it broken by design.Options to resolve:
- Separate fixture: Create a dedicated test fixture for the HTTP variant with an
expected_outputthat matches raw data presentation (e.g., asserting metric values are present in the response as text/table, without any embed requirement).- Variant-specific expected outputs: If the framework supports it, define per-variant expected outputs instead of a single shared one.
- Relaxed assertion for shared output: If the single shared
expected_outputis intentional, remove the embed/tool-name assertion and instead assert on the substantive content (e.g., CPU metric values returned), so both variants can satisfy it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/llm/fixtures/test_ask_holmes/91b_datadog_metrics_pod_exists/test_case.yaml` around lines 20 - 35, The current shared expected_output requires an embed with {"type":"datadogql","tool_name":"query_datadog_metrics"} which only the built-in Holmes Datadog toolset emits, so update the tests by splitting the HTTP variant into its own fixture: duplicate this test (the block containing toolsets_matrix and expected_output), in the original keep the existing expected_output and only include the built-in toolset (toolsets.yaml), and in the new HTTP-specific fixture include toolsets_http.yaml and replace the expected_output to assert on raw metric content/JSON presence (e.g., CPU metric keys or values) rather than the datadogql embed or query_datadog_metrics tool_name so both variants can pass.
🧹 Nitpick comments (4)
tests/llm/fixtures/test_ask_holmes/91d_datadog_metrics_historical_pod/toolsets_http.yaml (2)
1-34: Near-identical content duplicated across 9 test fixtures — optional extraction opportunity.
91d,91e, and91g(and the other six variants not shown here) all contain byte-for-byte the samedatadog-metrics-apistanza;91adiffers only by omittingkubernetes/core. If the API host, auth scheme, orllm_instructionsever change, all nine files need updating in lockstep.Consider a shared
toolsets_http_base.yamlsymlinked or referenced from each test directory, or at minimum a comment anchoring the canonical definition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/llm/fixtures/test_ask_holmes/91d_datadog_metrics_historical_pod/toolsets_http.yaml` around lines 1 - 34, The datadog-metrics-api stanza is duplicated across multiple test fixtures; extract the repeated config (the datadog-metrics-api block including endpoints, default_headers, timeout_seconds and llm_instructions) into a single shared file (e.g., a toolsets_http_base.yaml) and update each fixture to reference or symlink that shared file instead of inlining the block; alternatively add a clear canonical comment at the top of each duplicated fixture pointing to the shared canonical definition so future edits happen in one place.
9-9: Hard-coded Datadog site ties all HTTP toolset tests to US5 — consider externalising via env var.
api.us5.datadoghq.comis duplicated verbatim across all ninetoolsets_http.yamlfixtures in this PR. If the test account ever migrates to another region (US1, EU, AP1, …), every file needs a manual update. This is also inconsistent with the credentials themselves, which are already pulled from env vars.♻️ Suggested change
- - hosts: ["api.us5.datadoghq.com"] + - hosts: ["{{env.DATADOG_API_HOST}}"]Then set
DATADOG_API_HOST=api.us5.datadoghq.comin the CI/test environment (alongsideDATADOG_API_KEY/DATADOG_APP_KEY), matching the existingDD_SITEconvention used throughout Datadog tooling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/llm/fixtures/test_ask_holmes/91d_datadog_metrics_historical_pod/toolsets_http.yaml` at line 9, Replace the hard-coded Datadog host string hosts: ["api.us5.datadoghq.com"] in the toolsets_http.yaml fixtures with an environment-driven value (e.g., read DATADOG_API_HOST with a fallback to "api.us5.datadoghq.com"), so tests use process/CI env rather than a literal region; update all nine toolsets_http.yaml fixtures to reference DATADOG_API_HOST and ensure the CI/test environment defines DATADOG_API_HOST alongside existing DATADOG_API_KEY/DATADOG_APP_KEY.tests/llm/test_investigate.py (1)
63-65:getattris unnecessary sincetoolsets_config_pathis now a declared field onHolmesTestCase.Since
toolsets_config_pathis a proper Pydantic field with a default ofNoneonHolmesTestCase(andInvestigateTestCaseinherits from it), you can access it directly asself._test_case.toolsets_config_path. That said,getattris harmless and consistent with the pattern used formock_overrideson line 62 — so this is a nit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/llm/test_investigate.py` around lines 63 - 65, The call to getattr(self._test_case, "toolsets_config_path", None) is unnecessary because toolsets_config_path is a declared Pydantic field on HolmesTestCase (inherited by InvestigateTestCase); replace the getattr usage with direct attribute access self._test_case.toolsets_config_path in the code that constructs the test invocation (the block referencing toolsets_config_path alongside mock_overrides) so the code reads the field directly from the InvestigateTestCase/HolmesTestCase instance.tests/llm/fixtures/test_ask_holmes/91b_datadog_metrics_pod_exists/toolsets_http.yaml (1)
9-9: Hardcoded Datadog region (us5) may diverge from the test environment's actual site.Both
hosts(line 9) and thellm_instructionsbase URL (line 22) are hardcoded toapi.us5.datadoghq.com. The builtin toolset derives its region from the configured Datadog site. If the CI environment targets a different region (e.g., US1api.datadoghq.com), the HTTP toolset will silently fail all requests while the builtin variant succeeds, making cross-variant comparison meaningless.Consider sourcing the region from an environment variable (e.g.,
{{env.DATADOG_SITE}}) and updatingllm_instructionsto reflect the dynamic base URL, consistent with how the builtin toolset resolves its endpoint.Also applies to: 22-22
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/llm/fixtures/test_ask_holmes/91b_datadog_metrics_pod_exists/toolsets_http.yaml` at line 9, The test hardcodes the Datadog region in the HTTP toolset causing mismatch with the builtin variant; update the hosts entry (the hosts key currently set to "api.us5.datadoghq.com") and the llm_instructions base URL (the llm_instructions block) to derive the site from an environment variable (e.g., use DATADOG_SITE) instead of "us5" so both toolset variants use the same dynamic base domain; ensure the hosts value and the base URL are constructed from that env var consistently (same variable name) so CI can override the site.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@tests/llm/fixtures/test_ask_holmes/91b_datadog_metrics_pod_exists/test_case.yaml`:
- Around line 20-35: The current shared expected_output requires an embed with
{"type":"datadogql","tool_name":"query_datadog_metrics"} which only the built-in
Holmes Datadog toolset emits, so update the tests by splitting the HTTP variant
into its own fixture: duplicate this test (the block containing toolsets_matrix
and expected_output), in the original keep the existing expected_output and only
include the built-in toolset (toolsets.yaml), and in the new HTTP-specific
fixture include toolsets_http.yaml and replace the expected_output to assert on
raw metric content/JSON presence (e.g., CPU metric keys or values) rather than
the datadogql embed or query_datadog_metrics tool_name so both variants can
pass.
---
Duplicate comments:
In
`@tests/llm/fixtures/test_ask_holmes/91a_datadog_metrics_no_k8s/toolsets_http.yaml`:
- Line 7: The hosts entry is hard-coded to "api.us5.datadoghq.com"; update the
template to use a configurable Datadog site variable instead (e.g., reference a
variable like datadog_site or an env var DATADOG_SITE with a sensible default)
so the hosts key in toolsets_http.yaml is not fixed to us5—locate the hosts:
["api.us5.datadoghq.com"] line and replace it with a variable reference and
ensure callers set the variable or fallback is provided.
In
`@tests/llm/fixtures/test_ask_holmes/91e_datadog_custom_metrics/toolsets_http.yaml`:
- Line 9: The hosts entry is hard-coded to "api.us5.datadoghq.com"; change it to
use a configurable variable/env placeholder (e.g., DATADOG_SITE or a templated
variable like {{ datadog_site }}) so different Datadog sites can be targeted;
update the hosts line that currently reads hosts: ["api.us5.datadoghq.com"] to
reference that variable and ensure any test harness or CI sets
DATADOG_SITE/defaults accordingly (search for the hosts array in
toolsets_http.yaml to locate the exact spot).
In
`@tests/llm/fixtures/test_ask_holmes/91g_datadog_metrics_mismatched_pod/toolsets_http.yaml`:
- Line 9: The hosts entry currently hard-codes the Datadog site as hosts:
["api.us5.datadoghq.com"]; change this to use a configurable variable (e.g.,
datadog_site or DATADOG_SITE) and construct the hosts list from that variable so
tests/configs can target different Datadog sites; update the YAML entry
referencing hosts to pull from the variable (or environment) instead of the
fixed "api.us5.datadoghq.com" and ensure any test harness that loads this
fixture supplies the variable or env var.
---
Nitpick comments:
In
`@tests/llm/fixtures/test_ask_holmes/91b_datadog_metrics_pod_exists/toolsets_http.yaml`:
- Line 9: The test hardcodes the Datadog region in the HTTP toolset causing
mismatch with the builtin variant; update the hosts entry (the hosts key
currently set to "api.us5.datadoghq.com") and the llm_instructions base URL (the
llm_instructions block) to derive the site from an environment variable (e.g.,
use DATADOG_SITE) instead of "us5" so both toolset variants use the same dynamic
base domain; ensure the hosts value and the base URL are constructed from that
env var consistently (same variable name) so CI can override the site.
In
`@tests/llm/fixtures/test_ask_holmes/91d_datadog_metrics_historical_pod/toolsets_http.yaml`:
- Around line 1-34: The datadog-metrics-api stanza is duplicated across multiple
test fixtures; extract the repeated config (the datadog-metrics-api block
including endpoints, default_headers, timeout_seconds and llm_instructions) into
a single shared file (e.g., a toolsets_http_base.yaml) and update each fixture
to reference or symlink that shared file instead of inlining the block;
alternatively add a clear canonical comment at the top of each duplicated
fixture pointing to the shared canonical definition so future edits happen in
one place.
- Line 9: Replace the hard-coded Datadog host string hosts:
["api.us5.datadoghq.com"] in the toolsets_http.yaml fixtures with an
environment-driven value (e.g., read DATADOG_API_HOST with a fallback to
"api.us5.datadoghq.com"), so tests use process/CI env rather than a literal
region; update all nine toolsets_http.yaml fixtures to reference
DATADOG_API_HOST and ensure the CI/test environment defines DATADOG_API_HOST
alongside existing DATADOG_API_KEY/DATADOG_APP_KEY.
In `@tests/llm/test_investigate.py`:
- Around line 63-65: The call to getattr(self._test_case,
"toolsets_config_path", None) is unnecessary because toolsets_config_path is a
declared Pydantic field on HolmesTestCase (inherited by InvestigateTestCase);
replace the getattr usage with direct attribute access
self._test_case.toolsets_config_path in the code that constructs the test
invocation (the block referencing toolsets_config_path alongside mock_overrides)
so the code reads the field directly from the InvestigateTestCase/HolmesTestCase
instance.
…rect When a jq filter fails (e.g. `.metrics[]` on a null field), instead of returning an empty ERROR that gives the LLM zero information, return the original response (truncated to depth 2) alongside the error hint. This lets the LLM see the actual response shape and retry with a null-safe expression like `(.metrics // [])[]`. Also adds null-safe jq guidance to HTTP toolset instructions. https://claude.ai/code/session_014iLLnCruoc4xRE3r8aiXJh Signed-off-by: Claude <noreply@anthropic.com>
🔬 CLI Performance Benchmark🟡 Startup Time (no LLM)Measures
🟡 Full CLI with LLMMeasures
PR: |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
holmes/plugins/toolsets/json_filter_mixin.py (1)
51-53:⚠️ Potential issue | 🟡 MinorStale
# pragma: no cover— the new test now exercises this path.
test_invalid_jq_returns_data_with_error_hintpasses".["to_invoke, which causesjq.compile(".[")to raise aValueError, so theexceptblock is now reached. Remove the pragma to keep coverage reporting accurate.🧹 Suggested cleanup
- except Exception as exc: # pragma: no cover - defensive + except Exception as exc:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/plugins/toolsets/json_filter_mixin.py` around lines 51 - 53, The except block in json_filter_mixin.py that catches exceptions from jq (seen in the except Exception exc: handler which logs via logger.debug and returns "Invalid jq expression") still has a stale "# pragma: no cover" marker; remove that pragma so test coverage reflects the new test path (e.g., test_invalid_jq_returns_data_with_error_hint triggers jq.compile error and hits this handler). Keep the existing logger.debug("Failed to apply jq filter", exc_info=exc) and the return None, f"Invalid jq expression: {exc}" behavior, but delete the " # pragma: no cover - defensive" comment on the except line.
🧹 Nitpick comments (2)
tests/plugins/toolsets/test_json_filter_mixin.py (1)
42-53: Optional: assertjq_expressionis included in the error payload.The structured fallback includes a
jq_expressionfield (line 99 ofjson_filter_mixin.py), but the test only checksjq_errorandraw_response_preview. Adding a check here pins the full contract and prevents accidental removal of the field.💡 Optional addition
assert "raw_response_preview" in result.data + assert result.data["jq_expression"] == ".["🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/plugins/toolsets/test_json_filter_mixin.py` around lines 42 - 53, Update the test_invalid_jq_returns_data_with_error_hint to also assert the presence (and value) of the jq_expression field in the error payload: when invoking tool._invoke with {"uid": "abc", "jq": ".["}, assert "jq_expression" is in result.data and that result.data["jq_expression"] == ".[" to match the fallback constructed in json_filter_mixin.py (the jq_expression field created around line 99).holmes/plugins/toolsets/json_filter_mixin.py (1)
98-98: Optional: tailor the null-safe hint to runtime errors only.The null-safe advice is always appended to the error message, even for compile-time syntax errors (e.g.,
".[") where(.key // [])[]patterns are irrelevant. The LLM receives the raw{exc}text which already describes the real problem, but the appended hint can be misleading.Consider splitting the hint:
💡 Optional refinement
- "jq_error": f"{error}. Use null-safe patterns like (.key // [])[] instead of .key[] to handle missing/null fields.", + "jq_error": ( + f"{error}. Use null-safe patterns like (.key // [])[] instead of " + ".key[] to handle missing/null fields." + if "null" in str(error).lower() or "null (null)" in str(error).lower() + else str(error) + ),Or simply document the unconditional hint as an intentional, safe-by-default nudge and leave it as-is.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/plugins/toolsets/json_filter_mixin.py` at line 98, The message appended to the "jq_error" field should only include the null-safe hint for runtime missing-field errors, not for compile/parse errors; update the code that constructs the "jq_error" string (the place building f"{error}..." in json_filter_mixin.py, e.g., inside JsonFilterMixin or the method that catches jq exceptions) to detect parse/syntax faults by inspecting the exception type or text (check for keywords like "parse", "syntax", "Unrecognized", or an exception attribute indicating a parse error) and only append "Use null-safe patterns like (.key // [])[] ..." when it is not a parse/compile error; leave the original exception text ({exc}/{error}) intact in all cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@holmes/plugins/toolsets/json_filter_mixin.py`:
- Around line 51-53: The except block in json_filter_mixin.py that catches
exceptions from jq (seen in the except Exception exc: handler which logs via
logger.debug and returns "Invalid jq expression") still has a stale "# pragma:
no cover" marker; remove that pragma so test coverage reflects the new test path
(e.g., test_invalid_jq_returns_data_with_error_hint triggers jq.compile error
and hits this handler). Keep the existing logger.debug("Failed to apply jq
filter", exc_info=exc) and the return None, f"Invalid jq expression: {exc}"
behavior, but delete the " # pragma: no cover - defensive" comment on the except
line.
---
Nitpick comments:
In `@holmes/plugins/toolsets/json_filter_mixin.py`:
- Line 98: The message appended to the "jq_error" field should only include the
null-safe hint for runtime missing-field errors, not for compile/parse errors;
update the code that constructs the "jq_error" string (the place building
f"{error}..." in json_filter_mixin.py, e.g., inside JsonFilterMixin or the
method that catches jq exceptions) to detect parse/syntax faults by inspecting
the exception type or text (check for keywords like "parse", "syntax",
"Unrecognized", or an exception attribute indicating a parse error) and only
append "Use null-safe patterns like (.key // [])[] ..." when it is not a
parse/compile error; leave the original exception text ({exc}/{error}) intact in
all cases.
In `@tests/plugins/toolsets/test_json_filter_mixin.py`:
- Around line 42-53: Update the test_invalid_jq_returns_data_with_error_hint to
also assert the presence (and value) of the jq_expression field in the error
payload: when invoking tool._invoke with {"uid": "abc", "jq": ".["}, assert
"jq_expression" is in result.data and that result.data["jq_expression"] == ".["
to match the fallback constructed in json_filter_mixin.py (the jq_expression
field created around line 99).
The raw_response_preview is now serialized to a JSON string and truncated at 2000 characters, preventing large API responses (e.g. thousands of metrics) from overwhelming the LLM context window. https://claude.ai/code/session_014iLLnCruoc4xRE3r8aiXJh Signed-off-by: Claude <noreply@anthropic.com>
Reduce from 17 prompt variants to one representative prompt. https://claude.ai/code/session_014iLLnCruoc4xRE3r8aiXJh Signed-off-by: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/llm/fixtures/test_ask_holmes/110_cpu_graph_robusta_runner/test_case.yaml (1)
2-8: Expected output is too generic — it only validates embed format, not metric content.Per coding guidelines, eval tests should be specific: e.g., verifying the query targets memory metrics (
system.mem.*or equivalent) and covers the requested 24-hour window. The current assertion would pass even if Holmes queried the wrong metric or wrong time range, as long as anydatadogqlembed is present.Consider tightening the assertion to include, for example, that the metric name in the query relates to memory (e.g.,
container.memory), consistent with the user prompt requesting memory usage. Based on learnings, eval test prompts should test exact values, not just structural format.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/llm/fixtures/test_ask_holmes/110_cpu_graph_robusta_runner/test_case.yaml` around lines 2 - 8, Update the test's expected_output assertion (the YAML key expected_output) to validate not just the presence of a datadogql embed but that the embedded query targets memory metrics and a 24-hour window; specifically require the embed JSON to contain "type":"datadogql", "tool_name":"query_datadog_metrics", and that the query string includes memory-related metric names (e.g., "system.mem", "container.memory", or "container.memory.usage") and a time range or aggregate indicating 24h (e.g., "from: now-24h" or "last 24 hours"); adjust the text so the test will fail if the embed queries unrelated metrics or omits the 24-hour window while keeping the same embed format checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@tests/llm/fixtures/test_ask_holmes/110_cpu_graph_robusta_runner/test_case.yaml`:
- Line 1: The fixture's user_prompt asks for "memory usage" but the fixture
folder is named 110_cpu_graph_robusta_runner indicating a CPU graph test; fix
the semantic mismatch by either updating the user_prompt value to request "CPU
usage" (change the user_prompt string to "Show me robusta-runner CPU usage over
the last 24 hours") or renaming the fixture folder from
110_cpu_graph_robusta_runner to reflect a memory-metric scenario; ensure the
change is applied to the user_prompt field (symbol: user_prompt) or the fixture
folder name (symbol: 110_cpu_graph_robusta_runner) so intent and naming are
consistent.
---
Nitpick comments:
In
`@tests/llm/fixtures/test_ask_holmes/110_cpu_graph_robusta_runner/test_case.yaml`:
- Around line 2-8: Update the test's expected_output assertion (the YAML key
expected_output) to validate not just the presence of a datadogql embed but that
the embedded query targets memory metrics and a 24-hour window; specifically
require the embed JSON to contain "type":"datadogql",
"tool_name":"query_datadog_metrics", and that the query string includes
memory-related metric names (e.g., "system.mem", "container.memory", or
"container.memory.usage") and a time range or aggregate indicating 24h (e.g.,
"from: now-24h" or "last 24 hours"); adjust the text so the test will fail if
the embed queries unrelated metrics or omits the 24-hour window while keeping
the same embed format checks.
…trics The 6 embed tests (91a, 91b, 91c, 91d, 91e, 91g) expect datadogql embeds with tool_name "query_datadog_metrics", which only exists in the builtin Python datadog/metrics toolset. The HTTP toolset has no such tool, so the LLM loops forever trying to produce an impossible output, causing CI to hang for 3+ hours. Kept toolsets_matrix on 91f (logs) and 164 (traces) since those tests don't require the embed format. https://claude.ai/code/session_014iLLnCruoc4xRE3r8aiXJh Signed-off-by: Claude <noreply@anthropic.com>
…c' into claude/evals-toolset-matrix-mwdXc
https://claude.ai/code/session_014iLLnCruoc4xRE3r8aiXJh Signed-off-by: Claude <noreply@anthropic.com>
https://claude.ai/code/session_014iLLnCruoc4xRE3r8aiXJh Signed-off-by: Claude <noreply@anthropic.com>
…c' into claude/evals-toolset-matrix-mwdXc
…folder name - Reverted json_filter_mixin.py, instructions.jinja2, and test_json_filter_mixin.py to master (jq error handling goes in PR2) - Changed 110 user_prompt from "memory usage" to "CPU usage" to match the folder name 110_cpu_graph_robusta_runner https://claude.ai/code/session_014iLLnCruoc4xRE3r8aiXJh Signed-off-by: Claude <noreply@anthropic.com>
…l scenarios (#1609) Adds a new `toolsets_matrix` field to test_case.yaml that lists multiple toolset config filenames. Each file creates a separate test variant with the same user_prompt, expected_output, and infrastructure - only the toolset configuration changes. This enables comparing builtin toolsets vs HTTP toolsets vs MCP on identical scenarios. Changes: - HolmesTestCase: add toolsets_matrix, toolsets_config_name, toolsets_config_path fields - MockHelper: add _expand_toolsets_matrix() post-processing step after test case loading - MockToolsetManager: accept toolsets_config_path to override default toolsets.yaml resolution - test_ask_holmes.py, test_investigate.py: pass toolsets_config_path through Example test_case.yaml usage: toolsets_matrix: - toolsets_builtin.yaml - toolsets_http.yaml Produces test IDs like: test_ask_holmes[01_test[builtin]-model-env] https://claude.ai/code/session_014iLLnCruoc4xRE3r8aiXJh Signed-off-by: Claude <noreply@anthropic.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * HTTP integrations for Datadog Traces, Metrics, and Logs with auth and usage guidance. * Test matrix support to run tests against multiple toolset configurations. * Improved JSON filtering: null-safe guidance and structured error hints when filters fail. * **Tests** * Added and expanded fixtures and test cases to cover the new Datadog integrations and matrixed runs. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Mohse Morad <moshemorad12340@gmail.com>
3 participants
Adds a new
toolsets_matrixfield to test_case.yaml that lists multipletoolset config filenames. Each file creates a separate test variant with
the same user_prompt, expected_output, and infrastructure - only the
toolset configuration changes. This enables comparing builtin toolsets
vs HTTP toolsets vs MCP on identical scenarios.
Changes:
Example test_case.yaml usage:
toolsets_matrix:
- toolsets_builtin.yaml
- toolsets_http.yaml
Produces test IDs like: test_ask_holmes[01_test[builtin]-model-env]
https://claude.ai/code/session_014iLLnCruoc4xRE3r8aiXJh
Signed-off-by: Claude noreply@anthropic.com
Summary by CodeRabbit
New Features
Tests