Improve jq filtering - #1615
Open
aantn wants to merge 19 commits into
Open
Improve jq filtering#1615aantn wants to merge 19 commits into
aantn wants to merge 19 commits into
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>
…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>
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>
…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
Keep only jq error handling improvements: - json_filter_mixin.py: Make jq errors non-fatal, return raw data preview + error hint - instructions.jinja2: Add null-safe jq pattern guidance - test_json_filter_mixin.py: Updated tests for new jq error behavior Revert all other changes (toolsets_matrix, test framework changes) back to master. https://claude.ai/code/session_01N5rWgtZrghkJVJtL3xdEGZ Signed-off-by: Claude <noreply@anthropic.com>
Contributor
|
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:
WalkthroughAdded a pytest marker Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 13: Update the test prompts so they explicitly request a graph and the
datadogql embed: replace the string "Show me memory metrics for robusta-runner"
with a prompt that asks for a rendered time-series graph (e.g., "Show me a
time-series graph of memory metrics for robusta-runner, render using datadogql")
and likewise change "What's the memory utilization of the robusta-runner
deployment?" to explicitly request a graph render (e.g., "Render a graph showing
memory utilization for the robusta-runner deployment using datadogql"); ensure
these prompt strings (the exact text instances above) match what the test
expects so Holmes will produce the datadogql embed.
- Line 2: The test prompt in test_case.yaml currently says "generate me a graph
of robusta-runner", which is too generic; update the prompt string to explicitly
request the CPU metric (e.g., ask for a "CPU usage graph" or "CPU metric for
robusta-runner") so the test matches the expected query_datadog_metrics embed
check; locate and replace the prompt value in the test_case.yaml fixture to
mention CPU (the prompt string to change is the one currently set to "generate
me a graph of robusta-runner").
- Around line 1-17: Rename the test folder from 110_cpu_graph_robusta_runner to
110_memory_graph_robusta_runner and update any internal references to that
folder (e.g., CI/test manifests or test discovery entries) so the test name
matches the prompts in test_case.yaml which target memory graphs; ensure the
directory name change is reflected wherever the folder is referenced
(tests/llm/fixtures/... and any sibling test index or metadata) so the suite and
naming convention remain consistent with memory-focused tests like
34_memory_graph and 70_memory_leak_detection.
New pytest marker `jq-filter` covers all 21 evals that use toolsets with JsonFilterMixin (jq/max_depth parameters). This enables targeted testing of jq error handling changes: - Elasticsearch (17 evals): 183a-g, 184-191, 193, 195 - Simple tests (negative examples): cluster health, index discovery, etc. - Stress tests (positive examples): index/shard/mapping explosion, large mappings - Grafana dashboards (3 evals): 177, 178, 179 - Prometheus alerting rules (1 eval): 211 Run with: poetry run pytest -m jq-filter --no-cov https://claude.ai/code/session_01N5rWgtZrghkJVJtL3xdEGZ Signed-off-by: Claude <noreply@anthropic.com>
Contributor
📂 Previous Runs📜 Run @ 5b0c907 (#22807502364)✅ Results of HolmesGPT evalsAutomatically triggered by commit 5b0c907 on branch 📜 Run @ 1414b95 (#22280375835)✅ Results of HolmesGPT evalsAutomatically triggered by commit 1414b95 on branch 📜 Run @ e16755b (#22280363935)✅ Results of HolmesGPT evalsAutomatically triggered by commit e16755b on branch Results of HolmesGPT evals
📜 Run @ e16755b (#22277829898)✅ Results of HolmesGPT evalsAutomatically triggered by commit e16755b on branch Results of HolmesGPT evals
📜 Run @ 914ac5a (#22276894714)✅ Results of HolmesGPT evalsAutomatically triggered by commit 914ac5a on branch Results of HolmesGPT evals
✅ Results of HolmesGPT evalsAutomatically triggered by commit 3289fb9 on branch 📖 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 in automatic regression runs:
Examples: 🏷️ Valid tags
Commands: CLI: |
Contributor
|
✅ 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:85f07893
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:85f07893 me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:85f07893
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:85f07893
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:85f07893
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:85f07893 me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:85f07893
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:85f07893Patch 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:85f07893 \
--set operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set operator.image=holmes-operator-dev:85f07893Robusta 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:85f07893 \
--set holmes.operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.operator.image=holmes-operator-dev:85f07893 |
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
🔬 CLI Performance Benchmark🟡 Startup Time (no LLM)Measures
🟡 Full CLI with LLMMeasures
PR: |
Collaborator
Author
|
/eval |
This comment was marked as outdated.
This comment was marked as outdated.
Collaborator
Author
|
/eval |
The null-safe jq patterns (e.g., (.key // [])[] instead of .key[]) were added as LLM hints but are being reverted as part of the jq-error-handling split. The jq_error field now returns just the raw error string without prescriptive advice. https://claude.ai/code/session_01N5rWgtZrghkJVJtL3xdEGZ Signed-off-by: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@holmes/plugins/toolsets/json_filter_mixin.py`:
- Around line 98-101: The preview generation can raise TypeError when json.dumps
is given non-serializable objects in _filter_result_data; wrap the
json.dumps(truncated, ...) call (and the length check/substring logic for
preview_str) in a try/except that catches TypeError (and optionally ValueError),
and on exception produce a safe fallback preview (e.g., use repr(truncated) or
str(truncated) truncated to max_preview_chars with the same "…(truncated)"
suffix); ensure you update references to preview_str and truncated handling so
the rest of _filter_result_data continues using the fallback string rather than
letting the exception propagate.
Contributor
|
@aantn Your eval run has finished. ✅ Completed successfully 🧪 Manual Eval Results
Results of HolmesGPT evals (branch:
|
| 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 master -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 master -f markers=regression -f filter=
https://claude.ai/code/session_01N5rWgtZrghkJVJtL3xdEGZ Signed-off-by: Claude <noreply@anthropic.com>
…7N9q' into claude/revert-except-jq-errors-A7N9q
Wrap the preview serialization in a try/except for TypeError/ValueError to handle edge cases where non-JSON-serializable objects are passed directly to _filter_result_data. Falls back to repr() on failure. https://claude.ai/code/session_01N5rWgtZrghkJVJtL3xdEGZ Signed-off-by: Claude <noreply@anthropic.com>
moshemorad
added a commit
that referenced
this pull request
Apr 27, 2026
…on (#1947) ## Summary Fixes a silent-truncation bug in `JsonFilterMixin` that made six toolset tools produce false-negative answers (e.g. `elasticsearch_list_indices` reporting "no indices exist" against a cluster with hundreds of matching indices). Full context, reproducer, and live-cluster symptom in #1946. Root cause: when the LLM calls a mixin-backed tool with `max_depth=0` (a plausible value given the old description *"0 returns only top-level keys"*), `_truncate_to_depth` replaces the entire response with the literal string `"...truncated at depth 0"`, but `filter_result()` preserves `status=SUCCESS`. The LLM sees success + empty-looking data and confidently reports the wrong answer with no retry signal. This PR applies the minimal fix — two small edits in one file — plus regression tests: - **Rewrites the `max_depth` tool-schema description** so the LLM stops choosing 0. States the valid range (`>= 1`), points at `jq` for precise extraction, explicitly warns against `0` and negative values. - **Fail-closed in `filter_result()` on `max_depth <= 0`** with a self-corrective `ERROR` message the LLM can act on. The guard only fires when the upstream call succeeded (`status == SUCCESS`), so genuine upstream errors (e.g. HTTP 503 "cluster unreachable") are preserved verbatim — no clobbering of real failures with a parameter error. - Protects all six current consumer tools in a single mixin-level change: `elasticsearch_list_indices`, `elasticsearch_mappings`, `http_request`, `grafana_get_dashboard_by_uid`, `grafana_get_home_dashboard`, `list_prometheus_rules`. Also preemptively protects every toolset being added by #1695 (Datadog, ServiceNow, MongoDB Atlas, RabbitMQ, Coralogix, New Relic, and more) once that PR merges. ## Test plan - [x] `poetry run pytest tests/plugins/toolsets/test_json_filter_mixin.py -v --no-cov` → **9/9 passing** (4 pre-existing tests unchanged, 5 new regression tests) - [x] Standalone sanity check of `_truncate_to_depth` against an Elasticsearch `_cat/indices`–shaped payload: confirms bug at depth 0, confirms known remaining gap at depth 1 on list-of-dicts (see below), confirms real data returned at depth ≥ 2 and `None` - [x] `max_depth<=0` test: returns `ERROR` with self-corrective message - [x] `max_depth=-1` test: same (closes the undocumented negative-means-full escape hatch) - [x] Upstream-error-preservation test: when the mocked call returns `ERROR` and the LLM (hypothetically) passes `max_depth=0`, the upstream error string survives verbatim — no clobber - [x] `max_depth` omitted: full response unchanged (no regression on the happy path) - [x] Description-wording regression test: the string *"0 returns only top-level keys"* can never come back, and the description must mention `>= 1` - [ ] Maintainer to verify against a real Elasticsearch cluster using the three-step reproducer in #1946 ## Known remaining gap (deliberately out of scope — follow-up issue welcome) `max_depth=1` on a **list-of-dicts** response (the shape `_cat/indices` returns) still produces `[sentinel, sentinel, …]` with `status=SUCCESS` — the same silent-truncation class of bug, one level in. Fixing that properly requires a response-envelope redesign (e.g. `{truncated: true, max_depth_used, data, hint}`) that changes the response shape for all six consumer tools and their tests. That is a strictly larger change and deserves its own review and rollback surface, so it is deliberately out of scope here. Documented in #1946 under "Known remaining gap". ## Related PRs - #1695 extends `JsonFilterMixin` to seven additional toolsets but does **not** touch the mixin core. If it merges before this fix, every newly-covered toolset inherits the silent-truncation bug. Complementary to this PR. - #1615 improves `jq` error messaging in the same file but a different region. No behavioral overlap; any conflict is mechanical. ## References Closes #1946 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Bug Fixes** * Added runtime validation for the depth parameter; values of 0 or below now return an error with guidance instead of being processed. * **Documentation** * Updated depth parameter description to clarify that values must be >= 1; omit the parameter for a complete, untruncated response. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Sebastien Villanueva <sebastien.villanueva@gmail.com> Co-authored-by: moshemorad <moshemorad12340@gmail.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://claude.ai/code/session_01N5rWgtZrghkJVJtL3xdEGZ
Summary by CodeRabbit