Repository navigation
Json mixin for tools - #1276
Json mixin for tools#1276aantn wants to merge 5 commits into
Conversation
Signed-off-by: Codex <codex@openai.com>
✅ Results of HolmesGPT evalsAutomatically triggered by commit b24cdd9 Results of HolmesGPT evals
📖 Legend
🔄 Re-run evals manually
Option 1: Comment on this PR with Or with more options (one per line):
Option 2: Trigger via GitHub Actions UI → "Run workflow" 🏷️ Valid markers
📋 Valid eval names (use with filter)test_ask_holmes:
test_investigate:
|
WalkthroughAdds a JsonFilterMixin that provides jsonpath, jq, and depth-truncation controls and integrates it into Grafana, OpenSearch, and Coralogix tools; updates docs, tests, fixtures, and runtime deps to exercise and document response-size filtering. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/Invoker
participant Tool as Tool (Grafana / OpenSearch / Coralogix)
participant Remote as Remote API
participant Filter as JsonFilterMixin
participant Result as StructuredToolResult
Client->>Tool: invoke(params)
Tool->>Remote: fetch raw data (HTTP API)
Remote-->>Tool: raw JSON / object
Note right of Tool: wrap response in StructuredToolResult
Tool->>Filter: filter_result(result, params)
rect rgb(220,240,255)
Note over Filter: Extraction Phase (applied first)
Filter->>Filter: parse string → JSON (if needed)
Filter->>Filter: apply jsonpath (optional)
Filter->>Filter: apply jq (optional)
end
rect rgb(240,220,255)
Note over Filter: Truncation Phase
Filter->>Filter: truncate to max_depth (optional)
end
alt success
Filter-->>Tool: filtered StructuredToolResult
Tool->>Client: return filtered result (url adjusted if applicable)
else error
Filter-->>Tool: ERROR StructuredToolResult (message)
Tool->>Client: return error result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
holmes/plugins/toolsets/**📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (2)📚 Learning: 2025-12-29T08:35:37.668ZApplied to files:
📚 Learning: 2025-12-29T08:35:37.668ZApplied to files:
🧬 Code graph analysis (1)holmes/plugins/toolsets/coralogix/toolset_coralogix.py (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (2)
Comment |
|
✅ 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:f7a65a2
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:f7a65a2 me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:f7a65a2
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:f7a65a2Patch 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:f7a65a2Robusta 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:f7a65a2 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
docs/data-sources/builtin-toolsets/grafanadashboards.md (1)
61-69: Clear documentation of filtering controls.The new section effectively explains the three filtering options and their interaction. The note about filters running before depth truncation is particularly helpful.
Optional style refinement
Consider replacing "very big" with a more specific phrase like "large" or "substantial" for a more professional tone:
-Large dashboards can produce very big JSON payloads. To keep tool outputs readable you can: +Large dashboards can produce substantial JSON payloads. To keep tool outputs readable you can:holmes/plugins/toolsets/opensearch/opensearch.py (1)
68-71: Pre-existing bug: Variable shadowing causes incorrect client resolution.The inner comprehension shadows the outer loop variable
client, causing theany()to check all clients instead of just the current one. This means the function may return the wrong client when hosts overlap.This is existing code outside the PR's scope, but consider fixing it:
🔎 Proposed fix
for client in clients: - found = any(host in client.hosts for client in clients) + found = host in client.hosts if found: return client
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
docs/data-sources/builtin-toolsets/grafanadashboards.mdevals_per_branch.jsonholmes/plugins/toolsets/grafana/toolset_grafana.pyholmes/plugins/toolsets/json_filter_mixin.pyholmes/plugins/toolsets/opensearch/opensearch.pypyproject.tomltests/llm/fixtures/test_ask_holmes/170_grafana_dashboard_filters/generate_dashboard.pytests/llm/fixtures/test_ask_holmes/170_grafana_dashboard_filters/grafana.yamltests/llm/fixtures/test_ask_holmes/170_grafana_dashboard_filters/test_case.yamltests/llm/fixtures/test_ask_holmes/170_grafana_dashboard_filters/toolsets.yamltests/plugins/toolsets/grafana/test_dashboard_filters.py
🧰 Additional context used
📓 Path-based instructions (6)
tests/llm/**/*.yaml
📄 CodeRabbit inference engine (CLAUDE.md)
tests/llm/**/*.yaml: Use dedicated namespace per test:app-<testid>(e.g.,app-177) to prevent conflicts when tests run simultaneously
All pod names must be unique across tests - never reuse pod names between tests
Never use:latestcontainer tags in test infrastructure - use specific versions likegrafana/grafana:12.3.1
Files:
tests/llm/fixtures/test_ask_holmes/170_grafana_dashboard_filters/toolsets.yamltests/llm/fixtures/test_ask_holmes/170_grafana_dashboard_filters/test_case.yamltests/llm/fixtures/test_ask_holmes/170_grafana_dashboard_filters/grafana.yaml
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml)
Type hints required (mypy configuration in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
tests/plugins/toolsets/grafana/test_dashboard_filters.pyholmes/plugins/toolsets/json_filter_mixin.pyholmes/plugins/toolsets/grafana/toolset_grafana.pytests/llm/fixtures/test_ask_holmes/170_grafana_dashboard_filters/generate_dashboard.pyholmes/plugins/toolsets/opensearch/opensearch.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Tests: match source structure under
tests/
Files:
tests/plugins/toolsets/grafana/test_dashboard_filters.pytests/llm/fixtures/test_ask_holmes/170_grafana_dashboard_filters/generate_dashboard.py
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
When writing documentation in the docs/ directory, always add a blank line between a header/bold text and a list, otherwise MkDocs won't render the list properly
Files:
docs/data-sources/builtin-toolsets/grafanadashboards.md
tests/llm/**/test_case.yaml
📄 CodeRabbit inference engine (CLAUDE.md)
Never use names that hint at the problem or expected behavior in resource naming (e.g., avoid
broken-pod,test-project-that-does-not-exist,crashloop-app). Use neutral names that don't give away what the LLM should discover
Files:
tests/llm/fixtures/test_ask_holmes/170_grafana_dashboard_filters/test_case.yaml
holmes/plugins/toolsets/**
📄 CodeRabbit inference engine (CLAUDE.md)
Toolsets: organize as
holmes/plugins/toolsets/{name}.yamlor{name}/directories
Files:
holmes/plugins/toolsets/json_filter_mixin.pyholmes/plugins/toolsets/grafana/toolset_grafana.pyholmes/plugins/toolsets/opensearch/opensearch.py
🧠 Learnings (3)
📚 Learning: 2025-12-29T08:35:37.668Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T08:35:37.668Z
Learning: Applies to holmes/plugins/toolsets/** : Toolsets: organize as `holmes/plugins/toolsets/{name}.yaml` or `{name}/` directories
Applied to files:
tests/llm/fixtures/test_ask_holmes/170_grafana_dashboard_filters/toolsets.yamlholmes/plugins/toolsets/opensearch/opensearch.py
📚 Learning: 2025-12-29T08:35:37.668Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T08:35:37.668Z
Learning: Applies to holmes/plugins/toolsets/**/*.yaml : All toolsets MUST return detailed error messages from underlying APIs to enable LLM self-correction, including exact query/command executed, time ranges and parameters used, and full API error response
Applied to files:
tests/llm/fixtures/test_ask_holmes/170_grafana_dashboard_filters/toolsets.yaml
📚 Learning: 2025-12-29T08:35:37.668Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T08:35:37.668Z
Learning: Applies to tests/llm/**/*.yaml : Never use `:latest` container tags in test infrastructure - use specific versions like `grafana/grafana:12.3.1`
Applied to files:
tests/llm/fixtures/test_ask_holmes/170_grafana_dashboard_filters/test_case.yamltests/llm/fixtures/test_ask_holmes/170_grafana_dashboard_filters/grafana.yaml
🧬 Code graph analysis (3)
holmes/plugins/toolsets/json_filter_mixin.py (1)
holmes/core/tools.py (3)
StructuredToolResult(79-105)StructuredToolResultStatus(52-76)ToolParameter(157-163)
holmes/plugins/toolsets/grafana/toolset_grafana.py (1)
holmes/plugins/toolsets/json_filter_mixin.py (3)
JsonFilterMixin(60-122)extend_parameters(82-85)filter_result(109-122)
holmes/plugins/toolsets/opensearch/opensearch.py (3)
holmes/plugins/toolsets/json_filter_mixin.py (3)
JsonFilterMixin(60-122)extend_parameters(82-85)filter_result(109-122)holmes/core/tools.py (4)
ToolParameter(157-163)ToolInvokeContext(166-174)StructuredToolResult(79-105)StructuredToolResultStatus(52-76)holmes/plugins/toolsets/utils.py (1)
toolset_name_for_one_liner(232-236)
🪛 Checkov (3.2.334)
tests/llm/fixtures/test_ask_holmes/170_grafana_dashboard_filters/grafana.yaml
[medium] 1-68: Containers should not run with allowPrivilegeEscalation
(CKV_K8S_20)
[medium] 1-68: Minimize the admission of root containers
(CKV_K8S_23)
🪛 LanguageTool
docs/data-sources/builtin-toolsets/grafanadashboards.md
[style] ~63-~63: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...onse size Large dashboards can produce very big JSON payloads. To keep tool outputs rea...
(EN_WEAK_ADJECTIVE)
🪛 Ruff (0.14.10)
tests/plugins/toolsets/grafana/test_dashboard_filters.py
16-16: Unused lambda argument: endpoint
(ARG005)
holmes/plugins/toolsets/json_filter_mixin.py
42-42: Consider moving this statement to an else block
(TRY300)
43-43: Do not catch blind exception: Exception
(BLE001)
54-54: Consider moving this statement to an else block
(TRY300)
55-55: Do not catch blind exception: Exception
(BLE001)
63-79: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
92-92: Do not catch blind exception: Exception
(BLE001)
holmes/plugins/toolsets/grafana/toolset_grafana.py
283-283: Unused method argument: context
(ARG002)
holmes/plugins/toolsets/opensearch/opensearch.py
100-100: Unused method argument: context
(ARG002)
166-166: Unused method argument: context
(ARG002)
190-190: Unused method argument: context
(ARG002)
🪛 YAMLlint (1.37.1)
tests/llm/fixtures/test_ask_holmes/170_grafana_dashboard_filters/grafana.yaml
[error] 20-20: duplication of key "apiVersion" in mapping
(key-duplicates)
[error] 21-21: duplication of key "kind" in mapping
(key-duplicates)
[error] 22-22: duplication of key "metadata" in mapping
(key-duplicates)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build (3.10)
- GitHub Check: build (3.12)
- GitHub Check: build (3.11)
- GitHub Check: llm_evals
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (24)
evals_per_branch.json (1)
1-5: LGTM!The eval tracking structure is correct and the path matches the new test fixture directory introduced in this PR.
tests/llm/fixtures/test_ask_holmes/170_grafana_dashboard_filters/toolsets.yaml (1)
1-5: LGTM!The toolset configuration correctly enables Grafana dashboards with the URL matching the port forward configuration in test_case.yaml (local_port: 3030).
tests/llm/fixtures/test_ask_holmes/170_grafana_dashboard_filters/grafana.yaml (1)
7-7: Good adherence to test infrastructure guidelines.The manifest correctly uses:
- Namespace
app-170matching the test ID pattern- Specific image version
grafana/grafana:10.4.2instead of:latestThese practices prevent test conflicts and ensure reproducibility.
Also applies to: 26-26, 39-39, 75-75
tests/plugins/toolsets/grafana/test_dashboard_filters.py (2)
12-23: Helper function provides clean test setup.The
_build_toolhelper effectively creates a test instance with a mocked Grafana request. The unusedendpointparameter in the lambda (line 16) is acceptable for test mocks where we only need to return canned data.
25-70: Comprehensive test coverage for filtering behavior.The test suite effectively validates:
- Depth truncation mechanics
- JSONPath and jq filter application
- Error handling for invalid expressions
- Correct filter ordering (filters before depth)
This provides strong confidence in the filtering pipeline.
tests/llm/fixtures/test_ask_holmes/170_grafana_dashboard_filters/test_case.yaml (3)
7-7: Good adherence to test isolation guidelines.The test correctly uses namespace
app-170matching the test ID to prevent conflicts when tests run simultaneously, as per coding guidelines.Also applies to: 16-16
36-60: Robust polling strategy for pod readiness.The dual polling loops (for pod scheduling and readiness) with proper timeouts, retry delays, and error handling provide reliable setup in CI/CD environments where pod provisioning times vary.
25-25: Thegenerate_dashboard.pyfile exists in the test fixture directory as expected.holmes/plugins/toolsets/json_filter_mixin.py (4)
13-34: Clean recursive depth truncation implementation.The
_truncate_to_depthfunction correctly handles dictionaries, lists, and the boundary case where depth is None or negative. The truncation marker provides clear feedback about what was omitted.
36-58: Robust filter implementations with proper error handling.Both JSONPath and jq filters:
- Return single values when there's one match, arrays for multiple matches
- Provide descriptive error messages on failure
- Use defensive exception handling appropriate for tool operations
87-108: Correct filter pipeline ordering matches documentation.The implementation applies filters in the documented order:
- Parse JSON if needed
- Apply jsonpath if specified
- Apply jq if specified
- Truncate to max_depth
This aligns with the docs stating "filters run before depth truncation."
109-122: Clean error propagation in filter_result.The method correctly:
- Preserves original result metadata (url, invocation, icon_url) when filtering fails
- Returns ERROR status with descriptive error message
- Updates data field with filtered result on success
pyproject.toml (1)
58-59: Address critical security vulnerability in jq dependency.jq 1.8.0 (specified via ^1.8.0) contains CVE-2025-49014, a heap use-after-free vulnerability in the
f_strflocaltimefunction. Upgrade to jq 1.9.1 or later. jsonpath-ng 1.6.1 has no known vulnerabilities.tests/llm/fixtures/test_ask_holmes/170_grafana_dashboard_filters/generate_dashboard.py (3)
4-28: LGTM!The
build_panelfunction is well-structured with appropriate type hints and produces valid Grafana panel JSON. The grid positioning logic correctly maps panels to a 4-column layout.
31-78: LGTM!The dashboard generation logic correctly builds a configurable number of panels with a nested row structure for depth filtering tests. The grid positioning and nested panel structure align well with the PR's testing objectives.
81-82: LGTM!The main block provides a simple CLI interface for generating dashboard JSON, useful for debugging or manual fixture creation.
holmes/plugins/toolsets/opensearch/opensearch.py (4)
83-108: LGTM!The
ListShardsclass correctly integratesJsonFilterMixin:
- MRO places
JsonFilterMixinfirst, ensuring its methods are accessible- Parameters are extended properly via
extend_parameters- The shards API now requests JSON format (
params={"format": "json"})- Result filtering is applied before return
The static analysis hint about unused
contextis a false positive—the parameter is part of theToolinterface contract.
115-142: LGTM!The
GetClusterSettingsclass correctly follows the mixin pattern, extending parameters and applying result filtering. The raw response dict is now suitable for JSON filtering operations.
149-174: LGTM!The
GetClusterHealthclass follows the established pattern consistently with correct mixin integration and result filtering.
181-197: LGTM!The
ListOpenSearchHostsclass correctly usesextend_parameters({})to include only the filter parameters (max_depth, jsonpath, jq) since no tool-specific parameters are needed. The hosts list is JSON-friendly and suitable for filtering.holmes/plugins/toolsets/grafana/toolset_grafana.py (4)
21-21: LGTM!Import of
JsonFilterMixinis correctly placed at the top of the file, following the coding guidelines.
241-268: LGTM!The
GetDashboardByUIDclass correctly integratesJsonFilterMixin:
- Parameters are properly extended with the filter parameters
- URL is computed before filtering and then assigned to the filtered result
- The fallback to
result.urlpreserves the original URL ifdashboard_urlfails to buildThe filtering flow is clean: fetch → filter → set URL → return.
274-294: LGTM with minor observation.The
GetHomeDashboardclass correctly integrates filtering. Note that the URL fallback differs fromGetDashboardByUID:
GetDashboardByUID: falls back toresult.urlGetHomeDashboard: falls back toNoneThis is likely intentional since the home dashboard endpoint URL (
/api/dashboards/home) isn't meaningful for users to navigate to, whereas the computed dashboard URL is.
128-181: Design observation:SearchDashboardsintentionally omits filtering.The search endpoint returns lightweight dashboard metadata rather than full dashboard JSON, so filtering controls are less critical here. If response sizes become problematic in the future, the mixin can be added following the same pattern.
| filter_parameters: Dict[str, ToolParameter] = { | ||
| "max_depth": ToolParameter( | ||
| description="Maximum nesting depth to return from the JSON (0 returns only top-level keys). Leave empty for full response.", | ||
| type="integer", | ||
| required=False, | ||
| ), | ||
| "jsonpath": ToolParameter( | ||
| description="Optional jsonpath expression to extract specific parts of the JSON.", | ||
| type="string", | ||
| required=False, | ||
| ), | ||
| "jq": ToolParameter( | ||
| description="Optional jq filter to apply to the JSON.", | ||
| type="string", | ||
| required=False, | ||
| ), | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Annotate mutable class attribute with ClassVar.
The filter_parameters dictionary is shared across all instances and should be annotated with typing.ClassVar for proper type checking and to make the class-level scope explicit.
🔎 Proposed fix
Add the import at the top of the file:
-from typing import Any, Dict, Optional, Tuple
+from typing import Any, ClassVar, Dict, Optional, TupleThen annotate the class attribute:
class JsonFilterMixin:
"""Opt-in mixin for tools that return JSON and want filtering controls."""
- filter_parameters: Dict[str, ToolParameter] = {
+ filter_parameters: ClassVar[Dict[str, ToolParameter]] = {
"max_depth": ToolParameter(📝 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.
| filter_parameters: Dict[str, ToolParameter] = { | |
| "max_depth": ToolParameter( | |
| description="Maximum nesting depth to return from the JSON (0 returns only top-level keys). Leave empty for full response.", | |
| type="integer", | |
| required=False, | |
| ), | |
| "jsonpath": ToolParameter( | |
| description="Optional jsonpath expression to extract specific parts of the JSON.", | |
| type="string", | |
| required=False, | |
| ), | |
| "jq": ToolParameter( | |
| description="Optional jq filter to apply to the JSON.", | |
| type="string", | |
| required=False, | |
| ), | |
| } | |
| filter_parameters: ClassVar[Dict[str, ToolParameter]] = { | |
| "max_depth": ToolParameter( | |
| description="Maximum nesting depth to return from the JSON (0 returns only top-level keys). Leave empty for full response.", | |
| type="integer", | |
| required=False, | |
| ), | |
| "jsonpath": ToolParameter( | |
| description="Optional jsonpath expression to extract specific parts of the JSON.", | |
| type="string", | |
| required=False, | |
| ), | |
| "jq": ToolParameter( | |
| description="Optional jq filter to apply to the JSON.", | |
| type="string", | |
| required=False, | |
| ), | |
| } |
🧰 Tools
🪛 Ruff (0.14.10)
63-79: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🤖 Prompt for AI Agents
In holmes/plugins/toolsets/json_filter_mixin.py around lines 63 to 79, the
class-level dict filter_parameters is a mutable attribute shared across
instances and should be annotated with typing.ClassVar for correct static
typing; add ClassVar to the file imports (from typing import ClassVar) and
change the filter_parameters annotation to use ClassVar[Dict[str,
ToolParameter]] so the attribute is explicitly class-scoped and recognized as
non-instance-mutable by type checkers.
| apiVersion: apps/v1 | ||
| kind: Deployment | ||
| metadata: | ||
| name: grafana | ||
| labels: | ||
| app: grafana | ||
| namespace: app-170 | ||
| spec: |
There was a problem hiding this comment.
Add YAML document separator between resources.
The second resource (Deployment) starts without a --- separator, causing YAML key duplication errors. Multi-document YAML files require explicit document separators.
🔎 Proposed fix
path: /var/lib/grafana/dashboards
+---
apiVersion: apps/v1
kind: Deployment
metadata:🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 20-20: duplication of key "apiVersion" in mapping
(key-duplicates)
[error] 21-21: duplication of key "kind" in mapping
(key-duplicates)
[error] 22-22: duplication of key "metadata" in mapping
(key-duplicates)
🤖 Prompt for AI Agents
In tests/llm/fixtures/test_ask_holmes/170_grafana_dashboard_filters/grafana.yaml
around lines 20 to 27, the second resource (Deployment) begins immediately after
the previous resource without a YAML document separator, causing key duplication
and parsing errors; insert a line with three hyphens (---) immediately before
the Deployment resource to separate documents (ensure there's a single blank
line before/after separator if desired) so each Kubernetes resource is its own
YAML document.
aantn
changed the title
Signed-off-by: Codex <codex@openai.com>
✅ Results of HolmesGPT evalsAutomatically triggered by commit a8e93ff Results of HolmesGPT evals
📖 Legend
🔄 Re-run evals manually
Option 1: Comment on this PR with Or with more options (one per line):
Option 2: Trigger via GitHub Actions UI → "Run workflow" 🏷️ Valid markers
📋 Valid eval names (use with filter)test_ask_holmes:
test_investigate:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
holmes/plugins/toolsets/json_filter_mixin.py (1)
63-79: Annotate mutable class attribute withClassVar.The
filter_parametersdictionary is a class-level attribute shared across all instances and should be annotated withtyping.ClassVarfor proper type checking.🔎 Proposed fix
Add
ClassVarto the import statement:-from typing import Any, Dict, Optional, Tuple +from typing import Any, ClassVar, Dict, Optional, TupleThen annotate the class attribute:
- filter_parameters: Dict[str, ToolParameter] = { + filter_parameters: ClassVar[Dict[str, ToolParameter]] = {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
holmes/plugins/toolsets/json_filter_mixin.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml)
Type hints required (mypy configuration in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
holmes/plugins/toolsets/json_filter_mixin.py
holmes/plugins/toolsets/**
📄 CodeRabbit inference engine (CLAUDE.md)
Toolsets: organize as
holmes/plugins/toolsets/{name}.yamlor{name}/directories
Files:
holmes/plugins/toolsets/json_filter_mixin.py
🧬 Code graph analysis (1)
holmes/plugins/toolsets/json_filter_mixin.py (1)
holmes/core/tools.py (3)
StructuredToolResult(79-105)StructuredToolResultStatus(52-76)ToolParameter(157-163)
🪛 Ruff (0.14.10)
holmes/plugins/toolsets/json_filter_mixin.py
42-42: Consider moving this statement to an else block
(TRY300)
43-43: Do not catch blind exception: Exception
(BLE001)
54-54: Consider moving this statement to an else block
(TRY300)
55-55: Do not catch blind exception: Exception
(BLE001)
63-79: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
92-92: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build (3.10)
- GitHub Check: build (3.12)
- GitHub Check: build (3.11)
- GitHub Check: llm_evals
- GitHub Check: build
🔇 Additional comments (4)
holmes/plugins/toolsets/json_filter_mixin.py (4)
1-10: LGTM!The imports are correctly organized and placed at the top of the file as required by the coding guidelines.
13-57: LGTM!The helper functions implement robust filtering with appropriate error handling. The broad exception catching is justified here since these functions process user-provided filter expressions against external libraries (jq, jsonpath-ng).
81-85: LGTM!The
extend_parametersmethod correctly merges filter parameters with existing tool parameters, allowing existing parameters to take precedence.
87-107: LGTM!The filtering pipeline correctly applies jsonpath, jq, and depth truncation in sequence, with appropriate error propagation.
| def filter_result(self, result: StructuredToolResult, params: Dict) -> StructuredToolResult: | ||
| base_result = ( | ||
| result | ||
| if isinstance(result, StructuredToolResult) | ||
| else StructuredToolResult( | ||
| status=getattr(result, "status", StructuredToolResultStatus.SUCCESS), | ||
| data=getattr(result, "data", None), | ||
| params=getattr(result, "params", params), | ||
| url=getattr(result, "url", None), | ||
| invocation=getattr(result, "invocation", None), | ||
| icon_url=getattr(result, "icon_url", None), | ||
| ) | ||
| ) | ||
|
|
||
| filtered_data, error = self._filter_result_data(base_result.data, params) | ||
| if error: | ||
| return StructuredToolResult( | ||
| status=StructuredToolResultStatus.ERROR, | ||
| error=error, | ||
| params=params, | ||
| url=base_result.url, | ||
| invocation=base_result.invocation, | ||
| icon_url=base_result.icon_url, | ||
| ) | ||
|
|
||
| base_result.data = filtered_data | ||
| return base_result |
There was a problem hiding this comment.
Avoid mutating the input result object.
Line 134 mutates base_result.data directly. When the input result is already a StructuredToolResult (line 111), base_result is a reference to the same object, causing unexpected side effects for callers who may reuse the result object.
🔎 Proposed fix to return a new instance instead of mutating
- base_result.data = filtered_data
- return base_result
+ return StructuredToolResult(
+ status=base_result.status,
+ data=filtered_data,
+ params=base_result.params,
+ url=base_result.url,
+ invocation=base_result.invocation,
+ icon_url=base_result.icon_url,
+ error=base_result.error,
+ return_code=base_result.return_code,
+ )Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
holmes/plugins/toolsets/json_filter_mixin.py around lines 109 to 135: the method
currently mutates base_result.data which, when the incoming result is already a
StructuredToolResult, alters the caller's object; instead of assigning
filtered_data into base_result, construct and return a new StructuredToolResult
instance copying all fields from base_result (status, params, url, invocation,
icon_url, error if any) but with data set to filtered_data so the original
result is not mutated.
Signed-off-by: Codex <codex@openai.com>
✅ Results of HolmesGPT evalsAutomatically triggered by commit bd8a435 Results of HolmesGPT evals
📖 Legend
🔄 Re-run evals manually
Option 1: Comment on this PR with Or with more options (one per line):
Option 2: Trigger via GitHub Actions UI → "Run workflow" 🏷️ Valid markers
📋 Valid eval names (use with filter)test_ask_holmes:
test_investigate:
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
holmes/plugins/toolsets/json_filter_mixin.py (2)
63-79: Annotate mutable class attribute withClassVar.This issue has already been identified in a previous review. The
filter_parametersdictionary should be annotated withtyping.ClassVarfor proper type checking.
120-149: Avoid mutating the input result object.This issue has already been identified in a previous review. Line 148 mutates
base_result.datadirectly, which can cause unexpected side effects when the inputresultis already aStructuredToolResult. The previous review recommends returning a newStructuredToolResultinstance instead of mutating the existing one.
🧹 Nitpick comments (2)
holmes/plugins/toolsets/json_filter_mixin.py (2)
36-45: Consider clarifying empty match behavior.When the jsonpath expression matches no elements, the function returns an empty list
[]. This could be ambiguous—did the filter match nothing, or did it match an empty array? Consider either:
- Documenting this behavior explicitly, or
- Returning a more explicit indicator (e.g.,
Nonewith a descriptive message) when no matches are found.
48-57: Consider clarifying empty match behavior.Similar to the jsonpath filter, when the jq expression produces no results, an empty list
[]is returned. The same ambiguity applies here. Consider documenting this behavior or returning a more explicit indicator when no results are produced.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
holmes/plugins/toolsets/json_filter_mixin.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml)
Type hints required (mypy configuration in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
holmes/plugins/toolsets/json_filter_mixin.py
holmes/plugins/toolsets/**
📄 CodeRabbit inference engine (CLAUDE.md)
Toolsets: organize as
holmes/plugins/toolsets/{name}.yamlor{name}/directories
Files:
holmes/plugins/toolsets/json_filter_mixin.py
🧬 Code graph analysis (1)
holmes/plugins/toolsets/json_filter_mixin.py (1)
holmes/core/tools.py (3)
StructuredToolResult(79-105)StructuredToolResultStatus(52-76)ToolParameter(157-163)
🪛 Ruff (0.14.10)
holmes/plugins/toolsets/json_filter_mixin.py
42-42: Consider moving this statement to an else block
(TRY300)
43-43: Do not catch blind exception: Exception
(BLE001)
54-54: Consider moving this statement to an else block
(TRY300)
55-55: Do not catch blind exception: Exception
(BLE001)
63-79: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
92-92: Do not catch blind exception: Exception
(BLE001)
117-117: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build (3.12)
- GitHub Check: build (3.10)
- GitHub Check: build (3.11)
- GitHub Check: llm_evals
- GitHub Check: build
🔇 Additional comments (5)
holmes/plugins/toolsets/json_filter_mixin.py (5)
1-10: LGTM! Imports are properly organized.The imports follow the coding guidelines with all imports at the top of the file, and appropriate
type: ignorecomments for untyped external libraries.
13-33: LGTM! Depth truncation logic is correct.The recursive truncation correctly handles dictionaries, lists, and primitive values while respecting the maximum depth constraint.
81-85: LGTM! Parameter merging is clean and non-mutating.The method correctly creates a new dictionary and merges parameters without side effects.
87-107: LGTM! Filter pipeline is well-structured.The sequential application of jsonpath, jq, and depth truncation with proper error handling is clear and correct. The graceful handling of non-JSON strings (lines 92-94) is appropriate.
109-118: LGTM! Defensive string normalization for test compatibility.The method safely handles conversion to strings with appropriate fallbacks. Based on the usage in
filter_result, this appears primarily designed to handle MagicMock objects in tests, which is a reasonable defensive pattern.
|
/eval |
|
@aantn Your eval run has finished. ✅ Completed successfully 👆 See updated results above. |
|
/eval |
|
@aantn Your eval run has finished. ✅ Completed successfully 👆 See updated results above. |
Signed-off-by: Codex <codex@openai.com>
✅ Results of HolmesGPT evalsAutomatically triggered by commit e2ce0c6 Results of HolmesGPT evals
📖 Legend
🔄 Re-run evals manually
Option 1: Comment on this PR with Or with more options (one per line):
Option 2: Trigger via GitHub Actions UI → "Run workflow" 🏷️ Valid markers
📋 Valid eval names (use with filter)test_ask_holmes:
test_investigate:
|
…-tool-to-handle-large-2mkyd9
✅ Results of HolmesGPT evalsAutomatically triggered by commit 8ca3b82 Results of HolmesGPT evals
📖 Legend
🔄 Re-run evals manually
Option 1: Comment on this PR with Or with more options (one per line):
Option 2: Trigger via GitHub Actions UI → "Run workflow" 🏷️ Valid markers
📋 Valid eval names (use with filter)test_ask_holmes:
test_investigate:
|
|
/eval |
🧪 Manual Eval Results
Results of HolmesGPT evals
📖 Legend
🔄 Re-run evals manually
Option 1: Comment on this PR with Or with more options (one per line):
Option 2: Trigger via GitHub Actions UI → "Run workflow" 🏷️ Valid markers
📋 Valid eval names (use with filter)test_ask_holmes:
test_investigate:
|
There was a problem hiding this comment.
Actionable comments posted: 0
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/coralogix/toolset_coralogix.py (1)
62-101: Addextend_parameters()call to expose filter parameters.The class inherits from
JsonFilterMixinbut doesn't callextend_parameters()to merge the filter parameters (max_depth,jsonpath,jq) with the tool's existing parameters. This prevents users from controlling response depth and applying JSON filtering.Update line 68 to use
parameters=self.extend_parameters({...})instead of passing the parameters dict directly, consistent with how the Grafana and OpenSearch toolsets integrate the mixin.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
holmes/plugins/toolsets/coralogix/toolset_coralogix.pypyproject.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml)
Type hints required (mypy configuration in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
holmes/plugins/toolsets/coralogix/toolset_coralogix.py
holmes/plugins/toolsets/**
📄 CodeRabbit inference engine (CLAUDE.md)
Toolsets: organize as
holmes/plugins/toolsets/{name}.yamlor{name}/directories
Files:
holmes/plugins/toolsets/coralogix/toolset_coralogix.py
🧠 Learnings (2)
📚 Learning: 2025-12-29T08:35:37.668Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T08:35:37.668Z
Learning: Applies to holmes/plugins/toolsets/** : Toolsets: organize as `holmes/plugins/toolsets/{name}.yaml` or `{name}/` directories
Applied to files:
holmes/plugins/toolsets/coralogix/toolset_coralogix.py
📚 Learning: 2025-12-29T08:35:37.668Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T08:35:37.668Z
Learning: Applies to holmes/plugins/toolsets/**/*.yaml : All toolsets MUST return detailed error messages from underlying APIs to enable LLM self-correction, including exact query/command executed, time ranges and parameters used, and full API error response
Applied to files:
holmes/plugins/toolsets/coralogix/toolset_coralogix.py
🧬 Code graph analysis (1)
holmes/plugins/toolsets/coralogix/toolset_coralogix.py (2)
holmes/plugins/toolsets/json_filter_mixin.py (2)
JsonFilterMixin(60-149)filter_result(120-149)holmes/core/tools.py (2)
Tool(178-370)StructuredToolResult(80-106)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build (3.11)
- GitHub Check: build (3.12)
- GitHub Check: build (3.10)
- GitHub Check: llm_evals
🔇 Additional comments (2)
holmes/plugins/toolsets/coralogix/toolset_coralogix.py (2)
16-16: LGTM: Import statement correct.The JsonFilterMixin import is properly placed at the top of the file and is used in the class signature.
170-176: LGTM: Result filtering integration correct.The result handling correctly:
- Wraps the data in a
StructuredToolResult- Passes it through
self.filter_result(wrapped, params)for filtering- Returns the filtered result
The data structure change from a JSON string to a dictionary is appropriate for enabling JSON-based filtering.
Note: This assumes filter parameters are properly exposed via
extend_parameters()(see previous comment).
|
Dev Docker images are ready for this commit:
Use either tag to pull the image for testing. |
|
@aantn Your eval run has finished. ✅ Completed successfully 👆 See updated results above. |
1 participant
Summary
Testing
Codex Task
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.