Skip to content

Add JSON filtering capability to multiple toolsets - #1695

Open
aantn wants to merge 3 commits into
masterfrom
claude/confluence-integration-research-oP5HD
Open

aantn wants to merge 3 commits into
masterfrom
claude/confluence-integration-research-oP5HD

Conversation

@aantn

@aantn aantn commented Mar 7, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

This PR adds JSON filtering functionality across multiple toolsets by integrating the JsonFilterMixin class. This allows tools to filter and transform their JSON responses based on user-provided parameters, enabling more flexible and targeted data retrieval.

Key Changes

  • Elasticsearch Toolset:

    • Added JsonFilterMixin to ElasticsearchIndexStats and ElasticsearchNodesStats classes
    • Extended parameters using JsonFilterMixin.extend_parameters() to include filtering options
    • Applied filter_result() to responses before returning them
  • Datadog Toolset:

    • Added JsonFilterMixin to BaseDatadogGeneralTool class
    • Extended parameters for both datadog_api_get and datadog_api_post_search tools
    • Applied filtering to successful API responses
  • ServiceNow Toolset:

    • Added JsonFilterMixin to BaseServiceNowTool class
    • Extended parameters for servicenow_get_records and servicenow_get_record tools
    • Applied filtering to query results
  • MongoDB Atlas Toolset:

    • Added JsonFilterMixin to MongoDBAtlasBaseTool class
    • Extended parameters for ReturnProjectSlowQueries and ReturnEventTypeFromProject tools
    • Applied filtering in the return_result() method
  • RabbitMQ Toolset:

    • Added JsonFilterMixin to GetRabbitMQClusterStatus class
    • Extended parameters and applied filtering to cluster status results
  • Coralogix Toolset:

    • Added JsonFilterMixin to ExecuteDataPrimeQuery class
    • Extended parameters and applied filtering to query results
  • New Relic Toolset:

    • Added JsonFilterMixin to ExecuteNRQLQuery class
    • Extended parameters and applied filtering to NRQL query results

Implementation Details

  • All tools now inherit from JsonFilterMixin in addition to their base classes
  • Parameters are extended using JsonFilterMixin.extend_parameters() which adds filtering-related parameters to existing tool parameters
  • Results are filtered by calling self.filter_result(result, params) before returning from _invoke() methods
  • Updated test expectations to account for the new result structure with filtering applied

https://claude.ai/code/session_01K4kVdXWMnMv5ZQMntw9shn

Summary by CodeRabbit

  • Refactor
    • Added uniform JSON-based result filtering across multiple integration toolsets, ensuring responses respect provided filter parameters and consistent parameter handling.
  • Tests
    • Updated test expectations for filtered results and added extensive Datadog end-to-end test fixtures and toolset configurations to validate multi-step scenarios and cleanup.

…pth filtering

Extends JsonFilterMixin (jq + max_depth parameters) to Python toolsets that
return JSON responses but previously had no client-side filtering support.
This helps the LLM constrain large API responses and extract specific fields.

Toolsets updated:
- Elasticsearch: IndexStats, NodesStats
- ServiceNow: GetRecords, GetRecord (via base class)
- Datadog General: DatadogAPIGet, DatadogAPIPostSearch
- NewRelic: ExecuteNRQLQuery
- Coralogix: ExecuteDataPrimeQuery
- MongoDB Atlas: base class + SlowQueries, EventType tools
- RabbitMQ: GetRabbitMQClusterStatus

https://claude.ai/code/session_01K4kVdXWMnMv5ZQMntw9shn
Signed-off-by: Claude <noreply@anthropic.com>
@github-actions

github-actions Bot commented Mar 7, 2026 •

Copy link
Copy Markdown
Contributor

📂 Previous Runs

📜 Run @ a94c2de (#22807630268)

✅ Results of HolmesGPT evals

Automatically triggered by commit a94c2de on branch claude/confluence-integration-research-oP5HD

View workflow logs

Results of HolmesGPT evals

  • ask_holmes: 9/9 test cases were successful, 0 regressions
Status Test case Time Turns Tools Cost Total tokens Input Output Cached Non-cached Reasoning Max output Compactions
✅ 09_crashpod 29.9s 5 12 $0.2457 108,932 106,874 2,058 82,376 24,498 — 579 —
✅ 101_loki_historical_logs_pod_deleted 36.7s 5 11 $0.2481 108,690 106,462 2,228 82,213 24,249 — 725 —
✅ 111_pod_names_contain_service 29.5s 5 12 $0.2344 104,995 103,113 1,882 79,345 23,768 — 453 —
✅ 112_find_pvcs_by_uuid 25.0s 5 7 $0.2219 105,591 104,112 1,479 80,921 23,191 — 588 —
✅ 12_job_crashing 26.8s 5 9 $0.2195 104,657 103,212 1,445 80,235 22,977 — 455 —
✅ 176_network_policy_blocking_traffic_no_runbooks 41.9s 6 17 $0.2966 142,117 139,524 2,593 111,074 28,450 — 766 —
✅ 24_misconfigured_pvc 34.9s 7 14 $0.2639 144,400 142,326 2,074 117,575 24,751 — 469 —
✅ 43_current_datetime_from_prompt 4.4s 1 — $0.1117 17,508 17,388 120 0 17,388 — 120 —
✅ 61_exact_match_counting 15.0s 4 4 $0.1674 77,205 76,668 537 56,551 20,117 — 239 —
Total 27.1s avg 4.8 avg 10.8 avg $2.0092 914,095 899,679 14,416 690,290 209,389 — 766 —
Benchmark Comparison Details

Baseline: latest ci-benchmark experiment on master

Status: Success - 61 test/model combinations loaded

Benchmark experiment:

Time comparison (seconds):

Test case This branch master Diff
09_crashpod (opus-4.5) 29.9s 29.5s ±0%
101_loki_historical_logs_pod_deleted (opus-4.5) 36.7s — —
111_pod_names_contain_service (opus-4.5) 29.5s 34.2s ↓14%
112_find_pvcs_by_uuid (opus-4.5) 25.0s 31.0s ↓19%
12_job_crashing (opus-4.5) 26.8s 29.7s ±0%
176_network_policy_blocking_traffic_no_runbooks (opus-4.5) 41.9s 42.7s ±0%
24_misconfigured_pvc (opus-4.5) 34.9s 38.7s ±0%
43_current_datetime_from_prompt (opus-4.5) 4.4s — —
61_exact_match_counting (opus-4.5) 15.0s — —

Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run.

Comparison indicators:

  • ±0% — diff under 10% (within noise threshold)
  • ↑N%/↓N% — diff 10-25%
  • ↑N%/↓N% — diff over 25% (significant)
📜 Run @ a323f89 (#22807289617)

✅ Results of HolmesGPT evals

Automatically triggered by commit a323f89 on branch claude/confluence-integration-research-oP5HD

View workflow logs

Results of HolmesGPT evals

  • ask_holmes: 9/9 test cases were successful, 0 regressions
Status Test case Time Turns Tools Cost Total tokens Input Output Cached Non-cached Reasoning Max output Compactions
✅ 09_crashpod 28.9s 5 11 $0.2367 107,209 105,510 1,699 80,687 24,823 — 589 —
✅ 101_loki_historical_logs_pod_deleted 43.1s 6 12 $0.2795 133,720 131,033 2,687 105,265 25,768 — 783 —
✅ 111_pod_names_contain_service 29.9s 5 11 $0.2340 104,952 103,079 1,873 79,320 23,759 — 484 —
✅ 112_find_pvcs_by_uuid 27.4s 5 7 $0.2329 105,851 104,129 1,722 80,072 24,057 — 480 —
✅ 12_job_crashing 31.2s 5 12 $0.2449 109,867 107,924 1,943 83,139 24,785 — 461 —
✅ 176_network_policy_blocking_traffic_no_runbooks 40.3s 8 16 $0.3024 182,502 180,143 2,359 153,428 26,715 — 599 —
✅ 24_misconfigured_pvc 31.0s 5 15 $0.2479 106,870 104,809 2,061 79,382 25,427 — 706 —
✅ 43_current_datetime_from_prompt 4.4s 1 — $0.1114 17,499 17,388 111 0 17,388 — 111 —
✅ 61_exact_match_counting 15.6s 4 4 $0.1670 77,170 76,646 524 56,536 20,110 — 226 —
Total 28.0s avg 4.9 avg 11.0 avg $2.0567 945,640 930,661 14,979 717,829 212,832 — 783 —
Benchmark Comparison Details

Baseline: latest ci-benchmark experiment on master

Status: Success - 61 test/model combinations loaded

Benchmark experiment:

Time comparison (seconds):

Test case This branch master Diff
09_crashpod (opus-4.5) 28.9s 29.5s ±0%
101_loki_historical_logs_pod_deleted (opus-4.5) 43.1s — —
111_pod_names_contain_service (opus-4.5) 29.9s 34.2s ↓13%
112_find_pvcs_by_uuid (opus-4.5) 27.4s 31.0s ↓11%
12_job_crashing (opus-4.5) 31.2s 29.7s ±0%
176_network_policy_blocking_traffic_no_runbooks (opus-4.5) 40.3s 42.7s ±0%
24_misconfigured_pvc (opus-4.5) 31.0s 38.7s ↓20%
43_current_datetime_from_prompt (opus-4.5) 4.4s — —
61_exact_match_counting (opus-4.5) 15.6s — —

Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run.

Comparison indicators:

  • ±0% — diff under 10% (within noise threshold)
  • ↑N%/↓N% — diff 10-25%
  • ↑N%/↓N% — diff over 25% (significant)

✅ Results of HolmesGPT evals

Automatically triggered by commit 2a6b3bb on branch claude/confluence-integration-research-oP5HD

View workflow logs

Results of HolmesGPT evals

  • ask_holmes: 9/9 test cases were successful, 0 regressions
Status Test case Time Turns Tools Cost Total tokens Input Output Cached Non-cached Reasoning Max output Compactions
✅ 09_crashpod 26.3s 4 10 $0.2187 83,391 81,689 1,702 58,046 23,643 — 760 —
✅ 101_loki_historical_logs_pod_deleted 46.2s 7 13 $0.3052 156,316 153,396 2,920 126,136 27,260 — 615 —
✅ 111_pod_names_contain_service 31.9s 5 11 $0.2299 104,165 102,289 1,876 79,362 22,927 — 442 —
✅ 112_find_pvcs_by_uuid 25.1s 5 5 $0.2290 107,263 105,970 1,293 80,984 24,986 — 398 —
✅ 12_job_crashing 25.5s 4 7 $0.2148 82,851 81,380 1,471 57,125 24,255 — 496 —
✅ 176_network_policy_blocking_traffic_no_runbooks 46.1s 7 18 $0.3136 161,099 158,375 2,724 129,001 29,374 — 567 —
✅ 24_misconfigured_pvc 39.2s 7 18 $0.2827 150,460 148,152 2,308 121,482 26,670 — 523 —
✅ 43_current_datetime_from_prompt 4.4s 1 — $0.1115 17,503 17,388 115 0 17,388 — 115 —
✅ 61_exact_match_counting 16.1s 4 4 $0.1691 77,419 76,830 589 56,658 20,172 — 291 —
Total 29.0s avg 4.9 avg 10.8 avg $2.0746 940,467 925,469 14,998 708,794 216,675 — 760 —

Benchmark comparison unavailable: No ci-benchmark experiments found

Benchmark Comparison Details

Baseline: latest ci-benchmark experiment on master

Status: No ci-benchmark experiments found

Comparison indicators:

  • ±0% — diff under 10% (within noise threshold)
  • ↑N%/↓N% — diff 10-25%
  • ↑N%/↓N% — diff over 25% (significant)
📖 Legend
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: /eval comments 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/confluence-integration-research-oP5HD -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, db-connectors, 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/confluence-integration-research-oP5HD -f markers=regression -f filter=

@github-actions

github-actions Bot commented Mar 7, 2026 •

Copy link
Copy Markdown
Contributor

✅ Docker images ready for 472b780f (built in 6m 28s)

⚠️ Warning: does not support ARM (ARM images are built on release only - not on every PR)

Use these tags to pull the images for testing.

📋 Copy commands

⚠️ Temporary images are deleted after 30 days. Copy to a permanent registry before using them:

gcloud auth configure-docker us-central1-docker.pkg.dev
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:472b780f
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:472b780f me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:472b780f
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:472b780f
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:472b780f
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:472b780f me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:472b780f
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:472b780f

Patch 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:472b780f \
  --set operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
  --set operator.image=holmes-operator-dev:472b780f

Robusta 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:472b780f \
  --set holmes.operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
  --set holmes.operator.image=holmes-operator-dev:472b780f

@coderabbitai

coderabbitai Bot commented Mar 7, 2026 •

Copy link
Copy Markdown
Contributor

Walkthrough

Applies JsonFilterMixin across multiple toolsets: adds the mixin to tool class inheritance, wraps tool parameter declarations with JsonFilterMixin.extend_parameters(...), and routes returned results through self.filter_result(...) before returning.

Changes

Cohort / File(s) Summary
MongoDB Atlas Toolset
holmes/plugins/toolsets/atlas_mongodb/mongodb_atlas.py
Mixed JsonFilterMixin into MongoDBAtlasBaseTool; wrapped ReturnProjectSlowQueries and ReturnEventTypeFromProject parameters with JsonFilterMixin.extend_parameters(...); return values now pass through self.filter_result(...).
Coralogix Toolset
holmes/plugins/toolsets/coralogix/toolset_coralogix.py
Added JsonFilterMixin to ExecuteDataPrimeQuery, used extend_parameters(...) for parameters, and changed _invoke to build result and return self.filter_result(result, params).
Datadog General Toolset
holmes/plugins/toolsets/datadog/toolset_datadog_general.py
Added JsonFilterMixin to base tool, wrapped DatadogAPIGet and DatadogAPIPostSearch parameters with extend_parameters(...), and apply self.filter_result(...) to successful results.
Elasticsearch Toolset
holmes/plugins/toolsets/elasticsearch/elasticsearch.py
Mixed JsonFilterMixin into ElasticsearchIndexStats and ElasticsearchNodesStats; replaced parameters with extend_parameters(...); updated _invoke to post-process responses via self.filter_result(...) (including metrics-based stats path).
New Relic Toolset
holmes/plugins/toolsets/newrelic/newrelic.py
Added JsonFilterMixin to ExecuteNRQLQuery, wrapped parameters with extend_parameters(...), and return results via self.filter_result(result, params).
RabbitMQ Toolset
holmes/plugins/toolsets/rabbitmq/toolset_rabbitmq.py
Added JsonFilterMixin to GetRabbitMQClusterStatus, wrapped parameters with extend_parameters(...), and changed _invoke to return filtered StructuredToolResult via self.filter_result(...).
ServiceNow Tables Toolset
holmes/plugins/toolsets/servicenow_tables/servicenow_tables.py
Made BaseServiceNowTool inherit JsonFilterMixin; updated GetRecords and GetRecord parameters to extend_parameters(...); apply self.filter_result(...) to returned results.
Tests & Fixtures
tests/plugins/toolsets/datadog/test_toolset_datadog_general.py, conftest.py, tests/llm/fixtures/test_ask_holmes/...
Adjusted one datadog unit-test assertion; added passthrough in conftest.py for raw.githubusercontent.com; added multiple Datadog LLM fixture test cases and toolset configs (new test_case.yaml and toolsets.yaml files for evals 227/228/229).

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested labels

codex, evals-tag-datadog

Suggested reviewers

  • Sheeproid
  • arikalon1
  • moshemorad
🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 23.68% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title "Add JSON filtering capability to multiple toolsets" accurately describes the main change: introducing JsonFilterMixin across multiple toolsets to enable JSON filtering. It is specific, clear, and directly reflects the changeset.

✏️ 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@netlify

netlify Bot commented Mar 7, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for holmes-docs ready!

Name Link
🔨 Latest commit 2a6b3bb
🔍 Latest deploy log https://app.netlify.com/projects/holmes-docs/deploys/69ad98fbdec492000808377b
😎 Deploy Preview https://deploy-preview-1695--holmes-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@github-actions

github-actions Bot commented Mar 7, 2026 •

Copy link
Copy Markdown
Contributor

🔬 CLI Performance Benchmark

🟡 Startup Time (no LLM)

Measures holmes version execution time (imports + initialization)

Metric PR Master Change
Cold Start 10.02s 10.89s -8.0%
Warm Mean 4.61s 4.90s -5.8%
Warm Min 4.59s 4.87s
Warm Max 4.66s 4.91s

🟡 Full CLI with LLM

Measures holmes ask execution time (OpenRouter + Haiku 4.5)

Metric PR Master Change
Cold Start 30.49s 28.95s +5.3%
Warm Mean 6.76s 7.29s -7.4%
Warm Min 6.66s 7.18s
Warm Max 6.86s 7.40s

PR: 472b780f | Master: fb98d099 | Iterations: 5

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/atlas_mongodb/mongodb_atlas.py (1)

338-342: ⚠️ Potential issue | 🟠 Major

Filtering parameters will leak to MongoDB Atlas API.

The params dict is passed directly to session.get() as query parameters. With JsonFilterMixin.extend_parameters() adding jq and max_depth to the parameters, these will be sent to the MongoDB Atlas API, potentially causing request failures if the API rejects unknown parameters.

Compare with ReturnProjectSlowQueries (line 199) which doesn't pass params to the API request.

🐛 Proposed fix: Extract filtering params before API call
     def _invoke(self, params: dict, context: ToolInvokeContext) -> StructuredToolResult:
         try:
             url = self.url.format(projectId=self.toolset.config.get("project_id"))

             now_utc = datetime.now(timezone.utc)
             four_hours_ago = now_utc - timedelta(hours=4)
             iso_timestamp = four_hours_ago.isoformat()
-            params.update({"itemsPerPage": 500, "minDate": iso_timestamp})
+            api_params = {
+                "eventType": params.get("eventType"),
+                "itemsPerPage": 500,
+                "minDate": iso_timestamp,
+            }
             response = self.toolset._session.get(
                 url=url,
-                params=params,
+                params=api_params,
             )
             return self.return_result(response, params)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@holmes/plugins/toolsets/atlas_mongodb/mongodb_atlas.py` around lines 338 -
342, The params dict created in mongodb_atlas.py is being sent directly to
self.toolset._session.get, which leaks JsonFilterMixin.extend_parameters() keys
like jq and max_depth to the MongoDB Atlas API; update the code to separate
filtering args from API query params by calling
JsonFilterMixin.extend_parameters (or reusing the existing params) to collect
filter keys, then build a new api_params (e.g., copy of params) and
remove/filter out jq, max_depth (and any other filter-only keys) before calling
self.toolset._session.get; follow the pattern used in ReturnProjectSlowQueries
to ensure only Atlas-accepted parameters are sent.
🧹 Nitpick comments (2)
holmes/plugins/toolsets/coralogix/toolset_coralogix.py (1)

180-186: Filtering correctly applied to the result.

The filter_result() call properly wraps the StructuredToolResult before returning.

Minor note: The variable result shadows the earlier result from line 142 (the API response tuple). While not a bug since the original is no longer needed after line 179, using a distinct name like structured_result could improve readability.

,

♻️ Optional: Avoid variable shadowing for clarity
-        result = StructuredToolResult(
+        structured_result = StructuredToolResult(
             status=status,
             data=final_result,
             params=params,
             url=explore_url,
         )
-        return self.filter_result(result, params)
+        return self.filter_result(structured_result, params)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@holmes/plugins/toolsets/coralogix/toolset_coralogix.py` around lines 180 -
186, The local variable named result that holds the StructuredToolResult shadows
an earlier variable named result (the API response tuple); rename the
StructuredToolResult variable to a distinct name such as structured_result and
update its creation and the subsequent return call (StructuredToolResult(...)
assigned to structured_result and return self.filter_result(structured_result,
params)) so the earlier API response variable and the new structured result are
not confused when reading functions like StructuredToolResult and filter_result.
holmes/plugins/toolsets/rabbitmq/toolset_rabbitmq.py (1)

64-88: Consider adding JsonFilterMixin to ListConfiguredClusters for consistency.

While ListConfiguredClusters returns bounded configuration data (only user-configured clusters), adding JsonFilterMixin would provide consistency across the toolset and allow users to filter the response if needed. This is optional since the data is inherently bounded by configuration.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@holmes/plugins/toolsets/rabbitmq/toolset_rabbitmq.py` around lines 64 - 88,
Add JsonFilterMixin to the ListConfiguredClusters class definition (e.g., class
ListConfiguredClusters(JsonFilterMixin, BaseRabbitMQTool):) so the tool supports
JSON filtering like other tools; after computing available_clusters, pass that
list through the mixin's JSON filter helper (use the actual mixin method name
found in JsonFilterMixin, e.g., self.filter_json(...) or apply_json_filter(...))
before constructing and returning the StructuredToolResult, and ensure the MRO
places JsonFilterMixin before BaseRabbitMQTool.
🤖 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/atlas_mongodb/mongodb_atlas.py`:
- Around line 338-342: The params dict created in mongodb_atlas.py is being sent
directly to self.toolset._session.get, which leaks
JsonFilterMixin.extend_parameters() keys like jq and max_depth to the MongoDB
Atlas API; update the code to separate filtering args from API query params by
calling JsonFilterMixin.extend_parameters (or reusing the existing params) to
collect filter keys, then build a new api_params (e.g., copy of params) and
remove/filter out jq, max_depth (and any other filter-only keys) before calling
self.toolset._session.get; follow the pattern used in ReturnProjectSlowQueries
to ensure only Atlas-accepted parameters are sent.

---

Nitpick comments:
In `@holmes/plugins/toolsets/coralogix/toolset_coralogix.py`:
- Around line 180-186: The local variable named result that holds the
StructuredToolResult shadows an earlier variable named result (the API response
tuple); rename the StructuredToolResult variable to a distinct name such as
structured_result and update its creation and the subsequent return call
(StructuredToolResult(...) assigned to structured_result and return
self.filter_result(structured_result, params)) so the earlier API response
variable and the new structured result are not confused when reading functions
like StructuredToolResult and filter_result.

In `@holmes/plugins/toolsets/rabbitmq/toolset_rabbitmq.py`:
- Around line 64-88: Add JsonFilterMixin to the ListConfiguredClusters class
definition (e.g., class ListConfiguredClusters(JsonFilterMixin,
BaseRabbitMQTool):) so the tool supports JSON filtering like other tools; after
computing available_clusters, pass that list through the mixin's JSON filter
helper (use the actual mixin method name found in JsonFilterMixin, e.g.,
self.filter_json(...) or apply_json_filter(...)) before constructing and
returning the StructuredToolResult, and ensure the MRO places JsonFilterMixin
before BaseRabbitMQTool.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 049cd631-905a-40be-935f-51f4e4922809

📥 Commits

Reviewing files that changed from the base of the PR and between 98802cb and a323f89.

📒 Files selected for processing (8)
  • holmes/plugins/toolsets/atlas_mongodb/mongodb_atlas.py
  • holmes/plugins/toolsets/coralogix/toolset_coralogix.py
  • holmes/plugins/toolsets/datadog/toolset_datadog_general.py
  • holmes/plugins/toolsets/elasticsearch/elasticsearch.py
  • holmes/plugins/toolsets/newrelic/newrelic.py
  • holmes/plugins/toolsets/rabbitmq/toolset_rabbitmq.py
  • holmes/plugins/toolsets/servicenow_tables/servicenow_tables.py
  • tests/plugins/toolsets/datadog/test_toolset_datadog_general.py

claude added 2 commits March 7, 2026 21:29
… before JsonFilterMixin filtering

ClusterStatus is a Pydantic BaseModel, which jq cannot process directly.
Call model_dump() to convert to a plain dict before passing to filter_result().

https://claude.ai/code/session_01K4kVdXWMnMv5ZQMntw9shn
Signed-off-by: Claude <noreply@anthropic.com>
…API specs

Add three new cloud-only eval tests for the datadog/general toolset which
previously had zero eval coverage:

- 227: Monitor + dashboard correlation (multi-hop API navigation)
- 228: Root cause analysis across multiple monitors (causal reasoning)
- 229: Cross-resource audit with monitors, dashboards, and downtimes
  (hidden verification code in note widget)

All three tests create/cleanup resources via Datadog API (no K8s needed).
Tested with both Sonnet 4.5 and Opus 4.6 via OpenRouter - both pass 100%.

Also add raw.githubusercontent.com to HTTP passthrough in conftest.py, needed
for the datadog/general toolset to fetch Datadog OpenAPI specs during tests.

https://claude.ai/code/session_01K4kVdXWMnMv5ZQMntw9shn
Signed-off-by: Claude <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
tests/llm/fixtures/test_ask_holmes/227_datadog_general_monitor_dashboard/test_case.yaml (1)

18-22: Consider adding include_tool_calls: true for this multi-hop test.

This test requires Holmes to perform 5 distinct API calls (list monitors, get monitor details, list dashboards, get dashboard details, correlate). Adding include_tool_calls: true would verify that Holmes actually invoked the tools rather than potentially hallucinating responses. The other two Datadog tests (228, 229) include this flag.

Suggested addition
 tags:
   - datadog
   - hard
 
+include_tool_calls: true
+
 setup_timeout: 120
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@tests/llm/fixtures/test_ask_holmes/227_datadog_general_monitor_dashboard/test_case.yaml`
around lines 18 - 22, This multi-hop Datadog test is missing the
include_tool_calls flag, so update the test YAML to add include_tool_calls: true
at the top-level of the test definition to ensure Holmes' tool invocations (list
monitors, get monitor details, list dashboards, get dashboard details,
correlate) are validated; locate the file
test_ask_holmes/227_datadog_general_monitor_dashboard/test_case.yaml and add the
include_tool_calls: true key alongside existing keys like tags and
setup_timeout.
🤖 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/229_datadog_general_cross_resource/test_case.yaml`:
- Around line 216-224: The resource validation currently checks M1_ID, M2_ID,
M3_ID, and DASH_ID but omits DT_ID; update the conditional that validates
creation to include DT_ID (i.e., if [ -z "$M1_ID" ] || ... || [ -z "$DASH_ID" ]
|| [ -z "$DT_ID" ]), and add an echo of DT_ID in the failure block so the
downtime ID is logged alongside M1/M2/M3/DASH for debugging; adjust references
to the DT/DT_ID variables in the block to match existing naming.

---

Nitpick comments:
In
`@tests/llm/fixtures/test_ask_holmes/227_datadog_general_monitor_dashboard/test_case.yaml`:
- Around line 18-22: This multi-hop Datadog test is missing the
include_tool_calls flag, so update the test YAML to add include_tool_calls: true
at the top-level of the test definition to ensure Holmes' tool invocations (list
monitors, get monitor details, list dashboards, get dashboard details,
correlate) are validated; locate the file
test_ask_holmes/227_datadog_general_monitor_dashboard/test_case.yaml and add the
include_tool_calls: true key alongside existing keys like tags and
setup_timeout.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e993f727-4910-4cfe-a1a5-2dd3cf3944ab

📥 Commits

Reviewing files that changed from the base of the PR and between a94c2de and 2a6b3bb.

📒 Files selected for processing (7)
  • conftest.py
  • tests/llm/fixtures/test_ask_holmes/227_datadog_general_monitor_dashboard/test_case.yaml
  • tests/llm/fixtures/test_ask_holmes/227_datadog_general_monitor_dashboard/toolsets.yaml
  • tests/llm/fixtures/test_ask_holmes/228_datadog_general_monitor_analysis/test_case.yaml
  • tests/llm/fixtures/test_ask_holmes/228_datadog_general_monitor_analysis/toolsets.yaml
  • tests/llm/fixtures/test_ask_holmes/229_datadog_general_cross_resource/test_case.yaml
  • tests/llm/fixtures/test_ask_holmes/229_datadog_general_cross_resource/toolsets.yaml

Comment on lines +216 to +224
if [ -z "$M1_ID" ] || [ -z "$M2_ID" ] || [ -z "$M3_ID" ] || [ -z "$DASH_ID" ]; then
echo "❌ Failed to create one or more resources"
echo "M1: $M1"
echo "M2: $M2"
echo "M3: $M3"
echo "DASH: $DASH"
echo "DT: $DT"
exit 1
fi

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Missing downtime ID validation.

The validation check verifies M1_ID, M2_ID, M3_ID, and DASH_ID but omits DT_ID. If downtime creation fails silently, the test will proceed but fail on expected outputs related to the weekly recurrence.

Proposed fix
-  if [ -z "$M1_ID" ] || [ -z "$M2_ID" ] || [ -z "$M3_ID" ] || [ -z "$DASH_ID" ]; then
+  if [ -z "$M1_ID" ] || [ -z "$M2_ID" ] || [ -z "$M3_ID" ] || [ -z "$DASH_ID" ] || [ -z "$DT_ID" ]; then
     echo "❌ Failed to create one or more resources"
     echo "M1: $M1"
     echo "M2: $M2"
     echo "M3: $M3"
     echo "DASH: $DASH"
     echo "DT: $DT"
     exit 1
   fi
📝 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.

Suggested change
if [ -z "$M1_ID" ] || [ -z "$M2_ID" ] || [ -z "$M3_ID" ] || [ -z "$DASH_ID" ]; then
echo "❌ Failed to create one or more resources"
echo "M1: $M1"
echo "M2: $M2"
echo "M3: $M3"
echo "DASH: $DASH"
echo "DT: $DT"
exit 1
fi
if [ -z "$M1_ID" ] || [ -z "$M2_ID" ] || [ -z "$M3_ID" ] || [ -z "$DASH_ID" ] || [ -z "$DT_ID" ]; then
echo "❌ Failed to create one or more resources"
echo "M1: $M1"
echo "M2: $M2"
echo "M3: $M3"
echo "DASH: $DASH"
echo "DT: $DT"
exit 1
fi
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@tests/llm/fixtures/test_ask_holmes/229_datadog_general_cross_resource/test_case.yaml`
around lines 216 - 224, The resource validation currently checks M1_ID, M2_ID,
M3_ID, and DASH_ID but omits DT_ID; update the conditional that validates
creation to include DT_ID (i.e., if [ -z "$M1_ID" ] || ... || [ -z "$DASH_ID" ]
|| [ -z "$DT_ID" ]), and add an echo of DT_ID in the failure block so the
downtime ID is logged alongside M1/M2/M3/DASH for debugging; adjust references
to the DT/DT_ID variables in the block to match existing naming.

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

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants