feat: report json schema of toolset config to db - #1327
Conversation
Wraps installation_instructions as JSON containing: - instructions: The markdown installation instructions - config_schema: JSON Schema for toolset configuration - example_config: Example configuration values This enables the frontend to render a form-based UI for toolset configuration while maintaining backwards compatibility. Changes: - Add config_class ClassVar to Toolset base class - Add get_config_schema() method to Toolset - Update holmes_sync_toolsets to wrap instructions as JSON - Add config_class to RabbitMQ, Coralogix, Prometheus, OpenSearch toolsets
WalkthroughAdds a typed config surface to toolsets: introduces Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/test_holmes_sync_toolsets.py (1)
107-111: LGTM! Test correctly validates the JSON wrapping feature.The test properly verifies that
installation_instructionsis now wrapped in JSON format with the expected structure. The assertions correctly expectNonevalues forconfig_schemaandexample_configsinceSampleToolsetdoesn't define aconfig_class.Consider adding a test case with a toolset that defines
config_classto verify non-None schema handling, though this may be covered elsewhere.holmes/utils/holmes_sync_toolsets.py (1)
84-92: LGTM! Integration logic correctly wraps instructions with schema.The code properly extracts config schema and example configuration from the toolset, then wraps them with installation instructions.
Line 91 can be simplified:
🔎 Minor simplification
wrapped_instructions = wrap_installation_instructions_with_schema( instructions=toolset.installation_instructions, config_schema=config_schema, - example_config=example_config if example_config else None, + example_config=example_config or None, )Or even just
example_config=example_configsince the function acceptsOptional.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
holmes/core/tools.pyholmes/plugins/toolsets/coralogix/toolset_coralogix.pyholmes/plugins/toolsets/opensearch/opensearch.pyholmes/plugins/toolsets/prometheus/prometheus.pyholmes/plugins/toolsets/rabbitmq/toolset_rabbitmq.pyholmes/utils/holmes_sync_toolsets.pytests/test_holmes_sync_toolsets.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always place Python imports at the top of the file, not inside functions or methods
Use Ruff for formatting and linting with configuration in pyproject.toml
Type hints required (mypy configuration in pyproject.toml)
Pre-commit hooks enforce quality checks on Python files
Files:
tests/test_holmes_sync_toolsets.pyholmes/plugins/toolsets/coralogix/toolset_coralogix.pyholmes/plugins/toolsets/opensearch/opensearch.pyholmes/plugins/toolsets/prometheus/prometheus.pyholmes/plugins/toolsets/rabbitmq/toolset_rabbitmq.pyholmes/core/tools.pyholmes/utils/holmes_sync_toolsets.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Tests should match source structure under tests/
Live execution is now enabled by default to ensure tests match real-world behavior
Files:
tests/test_holmes_sync_toolsets.py
holmes/plugins/toolsets/**/*.{py,yaml}
📄 CodeRabbit inference engine (CLAUDE.md)
holmes/plugins/toolsets/**/*.{py,yaml}: All toolsets MUST return detailed error messages from underlying APIs to enable LLM self-correction, including exact query/command executed, time ranges/parameters/filters used, and full API error response (status code and message)
For 'no data' responses in toolsets, specify what was searched and where
Never return unbounded data from APIs - always include filter parameters on tools that query collections
Files:
holmes/plugins/toolsets/coralogix/toolset_coralogix.pyholmes/plugins/toolsets/opensearch/opensearch.pyholmes/plugins/toolsets/prometheus/prometheus.pyholmes/plugins/toolsets/rabbitmq/toolset_rabbitmq.py
holmes/plugins/toolsets/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
holmes/plugins/toolsets/**/*.py: Use requests library for HTTP calls in Python toolsets, not specialized client libraries like opensearchpy
Implement simple Pydantic config class with validation for Python toolsets
Include health check in prerequisites_callable() method for Python toolsets
Each tool in Python toolsets should be a thin wrapper around a single API endpoint
Use JsonFilterMixin for client-side filtering when server-side filtering is not possible, adding max_depth and jq parameters
Maintain backwards compatibility in toolset config using Pydantic's extra='allow' when renaming config fields
Only define current field names in toolset config schema with extra='allow' to avoid polluting model_dump() output with deprecated fields
Use @model_validator(mode='after') in toolset config to map old field names to new names and log deprecation warnings
Bash toolset validates commands for safety
Files:
holmes/plugins/toolsets/coralogix/toolset_coralogix.pyholmes/plugins/toolsets/opensearch/opensearch.pyholmes/plugins/toolsets/prometheus/prometheus.pyholmes/plugins/toolsets/rabbitmq/toolset_rabbitmq.py
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.208Z
Learning: Applies to holmes/plugins/toolsets/**/*.py : Implement simple Pydantic config class with validation for Python toolsets
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.208Z
Learning: Applies to holmes/plugins/toolsets/**/*.py : Only define current field names in toolset config schema with extra='allow' to avoid polluting model_dump() output with deprecated fields
📚 Learning: 2026-01-05T11:14:20.209Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.209Z
Learning: New toolsets require integration tests
Applied to files:
tests/test_holmes_sync_toolsets.py
📚 Learning: 2026-01-05T11:14:20.208Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.208Z
Learning: Applies to holmes/plugins/toolsets/**/*.py : Maintain backwards compatibility in toolset config using Pydantic's extra='allow' when renaming config fields
Applied to files:
holmes/plugins/toolsets/coralogix/toolset_coralogix.pyholmes/plugins/toolsets/opensearch/opensearch.pyholmes/plugins/toolsets/prometheus/prometheus.pyholmes/plugins/toolsets/rabbitmq/toolset_rabbitmq.pyholmes/core/tools.py
📚 Learning: 2026-01-05T11:14:20.208Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.208Z
Learning: Applies to holmes/plugins/toolsets/**/*.py : Implement simple Pydantic config class with validation for Python toolsets
Applied to files:
holmes/plugins/toolsets/coralogix/toolset_coralogix.pyholmes/plugins/toolsets/opensearch/opensearch.pyholmes/plugins/toolsets/prometheus/prometheus.pyholmes/plugins/toolsets/rabbitmq/toolset_rabbitmq.pyholmes/core/tools.py
📚 Learning: 2026-01-05T11:14:20.208Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.208Z
Learning: Applies to holmes/plugins/toolsets/**/*.py : Only define current field names in toolset config schema with extra='allow' to avoid polluting model_dump() output with deprecated fields
Applied to files:
holmes/plugins/toolsets/coralogix/toolset_coralogix.pyholmes/plugins/toolsets/prometheus/prometheus.pyholmes/plugins/toolsets/rabbitmq/toolset_rabbitmq.pyholmes/core/tools.pyholmes/utils/holmes_sync_toolsets.py
📚 Learning: 2026-01-05T11:14:20.208Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.208Z
Learning: Applies to holmes/plugins/toolsets/**/*.py : Use model_validator(mode='after') in toolset config to map old field names to new names and log deprecation warnings
Applied to files:
holmes/plugins/toolsets/rabbitmq/toolset_rabbitmq.pyholmes/core/tools.py
🧬 Code graph analysis (2)
holmes/plugins/toolsets/coralogix/toolset_coralogix.py (1)
holmes/plugins/toolsets/coralogix/utils.py (1)
CoralogixConfig(27-31)
holmes/utils/holmes_sync_toolsets.py (1)
holmes/core/tools.py (5)
get_config_schema(770-777)get_example_config(767-768)get_example_config(805-806)get_example_config(838-839)ToolsetDBModel(842-852)
🪛 Ruff (0.14.10)
holmes/utils/holmes_sync_toolsets.py
29-29: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (12)
holmes/plugins/toolsets/rabbitmq/toolset_rabbitmq.py (1)
3-3: LGTM! Clean type annotation for config class.The addition of
ClassVar[Type[RabbitMQConfig]]properly declares the configuration class at the type level, enabling theget_config_schema()method introduced in the baseToolsetclass to generate JSON schemas for frontend form rendering.Also applies to: 132-133
holmes/plugins/toolsets/opensearch/opensearch.py (1)
2-2: LGTM! Consistent with project-wide pattern.The
config_classattribute follows the same pattern as other toolsets in this PR, properly annotated withClassVar[Type[OpenSearchConfig]]to expose the configuration schema.Also applies to: 194-194
holmes/plugins/toolsets/coralogix/toolset_coralogix.py (1)
3-3: LGTM! Clean implementation.The configuration class binding is correctly implemented with proper type annotations, aligning with the standardized approach across all toolsets in this PR.
Also applies to: 184-185
holmes/core/tools.py (3)
15-15: LGTM! Necessary imports for class variable annotations.The
ClassVarandTypeimports support the new configuration class binding pattern.Also applies to: 21-21
543-543: LGTM! Well-designed class variable for backward compatibility.The
config_classclass variable is optional (defaulting toNone), which allows existing toolsets without configuration classes to continue working without modification.
770-778: LGTM! Safe schema retrieval method.The
get_config_schema()method properly checks for the existence ofconfig_classbefore attempting to generate the JSON schema, preventingAttributeErrorfor toolsets that don't define a configuration class.holmes/plugins/toolsets/prometheus/prometheus.py (1)
6-6: Verify schema completeness for AMPConfig.The changes correctly add the
config_classattribute pointing toPrometheusConfig. However, the runtimeconfigattribute (line 1496) can be eitherPrometheusConfigorAMPConfig(which extendsPrometheusConfigwith AWS-specific fields likeaws_region,aws_access_key, etc.).Since
get_config_schema()will return the schema forPrometheusConfigonly, AWS-specific fields won't appear in the generated schema. This may be intentional if AMPConfig is considered an advanced/alternate configuration path, but please confirm this won't cause issues for users trying to configure AWS Managed Prometheus through a form-based UI.Consider whether
AMPConfigshould have its own schema exposure mechanism, or if the basePrometheusConfigschema is sufficient for the intended use case.Also applies to: 1495-1495
tests/test_holmes_sync_toolsets.py (1)
1-1: LGTM!The
jsonimport is correctly placed at the top of the file and is necessary for parsing the JSON-wrapped installation instructions in the updated test.holmes/utils/holmes_sync_toolsets.py (4)
1-1: LGTM!The import additions are correctly placed and necessary for the new JSON wrapping functionality and type hints.
Also applies to: 4-4
25-29: LGTM!The custom JSON serializer follows a common pattern and appropriately handles non-serializable objects by falling back to their string representation. This is suitable for config schemas and example configurations.
The static analysis hint (TRY003) appears to be a false positive—the error message is concise and appropriate.
32-54: LGTM! Well-documented wrapping function.The function correctly wraps installation instructions with configuration schema and example. The docstring clearly explains the structure and backwards compatibility approach for frontend parsing.
96-96: LGTM! Correct handling of wrapped instructions.The code properly excludes the original
installation_instructionsfrom the model dump and replaces it with the JSON-wrapped version. This ensures the database receives the enhanced format containing instructions, config schema, and example configuration.Also applies to: 101-101
Remove example_config from installation_instructions wrapper and rely solely on JSON Schema with Field annotations for examples. Changes: - Remove example_config parameter from wrap_installation_instructions_with_schema - Add Field(examples=[...], description="...") to config models: - RabbitMQClusterConfig, RabbitMQConfig - PrometheusConfig - CoralogixConfig, CoralogixLabelsConfig - GrafanaConfig, GrafanaTempoConfig, GrafanaTempoLabelsConfig - Update test assertions Frontend can now extract examples from config_schema["properties"]["field"]["examples"]
Resolved conflicts in: - prometheus/prometheus.py: Added Field annotations to new field names - rabbitmq/api.py: Merged imports - rabbitmq/toolset_rabbitmq.py: Merged imports with config_class support
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📂 Previous Runs📜 Run @ 8685098 (#21334361701)✅ Results of HolmesGPT evalsAutomatically triggered by commit 8685098 on branch Results of HolmesGPT evals
Time/Cost columns show % change vs historical average (↑slower/costlier, ↓faster/cheaper). Changes under 10% shown as ±0%. Historical Comparison DetailsFilter: excluding branch 'claude/document-holmesgpt-data-reporting-axz0d' Status: Success - 20 test/model combinations loaded Experiments compared (30):
Comparison indicators:
📜 Run @ 28a1606 (#21334163952)✅ Results of HolmesGPT evalsAutomatically triggered by commit 28a1606 on branch Results of HolmesGPT evals
Time/Cost columns show % change vs historical average (↑slower/costlier, ↓faster/cheaper). Changes under 10% shown as ±0%. Historical Comparison DetailsFilter: excluding branch 'claude/document-holmesgpt-data-reporting-axz0d' Status: Success - 20 test/model combinations loaded Experiments compared (30):
Comparison indicators:
📜 Run @ b1a82bd (#21333734585)✅ Results of HolmesGPT evalsAutomatically triggered by commit b1a82bd on branch Results of HolmesGPT evals
Time/Cost columns show % change vs historical average (↑slower/costlier, ↓faster/cheaper). Changes under 10% shown as ±0%. Historical Comparison DetailsFilter: excluding branch 'claude/document-holmesgpt-data-reporting-axz0d' Status: Success - 20 test/model combinations loaded Experiments compared (30):
Comparison indicators:
📜 Run @ f165ff0 (#21333393626)✅ Results of HolmesGPT evalsAutomatically triggered by commit f165ff0 on branch 📜 Run @ 052a1ae (#21333242854)✅ Results of HolmesGPT evalsAutomatically triggered by commit 052a1ae on branch ✅ Results of HolmesGPT evalsAutomatically triggered by commit 0cf6fdf on branch Results of HolmesGPT evals
Time/Cost columns show % change vs historical average (↑slower/costlier, ↓faster/cheaper). Changes under 10% shown as ±0%. Historical Comparison DetailsFilter: excluding branch 'claude/document-holmesgpt-data-reporting-axz0d' Status: Success - 20 test/model combinations loaded Experiments compared (30):
Comparison indicators:
📖 Legend
🔄 Re-run evals manually
Option 1: Comment on this PR with Or with more options (one per line): Run evals on a different branch (e.g., master) for comparison:
Quick re-run: Use Option 2: Trigger via GitHub Actions UI → "Run workflow" 🏷️ Valid markers
Commands: CLI: |
|
✅ 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:6c93cf9
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:6c93cf9 me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:6c93cf9
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:6c93cf9Patch 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:6c93cf9Robusta 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:6c93cf9 |
- Remove get_example_config() method from Toolset base class and all implementations - Update installation instructions template to not include example configs - Frontend will now generate config examples from the JSON Schema instead - Update PR template to reference config_class for schema generation - Remove all test methods that tested get_example_config() Signed-off-by: Claude <noreply@anthropic.com>
Resolved conflicts: - tools.py: Added restricted_tools/approval_required_tools, removed get_example_config - grafana/common.py: Added verify_ssl field with Field annotation - grafana/base_grafana_toolset.py: Removed get_example_config - newrelic.py: Removed get_example_config - prometheus.py: Added AzurePrometheusConfig to union, kept config_class - rabbitmq/api.py: Merged Field annotations with verify_ssl rename and model_validator Signed-off-by: Claude <noreply@anthropic.com>
- Remove default_toolset_installation_guide.jinja2 template - Frontend can now generate all instructions from config schema + toolset metadata - Simplify holmes_sync_toolsets.py by removing render function - Update test to use toolset.installation_instructions directly Signed-off-by: Claude <noreply@anthropic.com>
…ons field - Remove installation_instructions field from Toolset and ToolsetYamlFromConfig - Send config_schema JSON directly in installation_instructions DB field - Remove wrapper function - schema is now the direct value - Frontend can parse JSON schema directly for form generation Signed-off-by: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@holmes/utils/holmes_sync_toolsets.py`:
- Around line 54-60: The code treats an empty but valid config schema as missing
by using a truthy check; change the conditional that builds config_schema_json
to explicitly check for None (use config_schema is not None) so that empty
structures returned by toolset.get_config_schema() are preserved and still
JSON-serialized via json.dumps(..., default=_json_serializer); update references
around config_schema, config_schema_json and toolset.get_config_schema()
accordingly.
🧹 Nitpick comments (1)
holmes/utils/holmes_sync_toolsets.py (1)
22-26: Tighten serializer fallback to avoid masking schema issues.
Every object has__str__, so this will stringify any non‑serializable object and can hide real schema problems. Consider limiting to known safe types (e.g., datetime/Enum/Path) and raising otherwise.♻️ Proposed refactor
+from datetime import date, datetime +from enum import Enum +from pathlib import Path + def _json_serializer(obj: Any) -> Any: """Custom JSON serializer for objects not serializable by default json code.""" - if hasattr(obj, "__str__"): - return str(obj) + if isinstance(obj, (datetime, date, Enum, Path)): + return str(obj) raise TypeError(f"Object of type {type(obj).__name__} is not JSON serializable")
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
holmes/core/tools.pyholmes/utils/holmes_sync_toolsets.pytests/test_holmes_sync_toolsets.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always place Python imports at the top of the file, not inside functions or methods
Use Ruff for formatting and linting with configuration in pyproject.toml
Type hints required (mypy configuration in pyproject.toml)
Pre-commit hooks enforce quality checks on Python files
Files:
holmes/core/tools.pytests/test_holmes_sync_toolsets.pyholmes/utils/holmes_sync_toolsets.py
**/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php}: Use semantic, descriptive names for variables, functions, and components
Write clear, concise comments that explain 'why' rather than 'what'
Files:
holmes/core/tools.pytests/test_holmes_sync_toolsets.pyholmes/utils/holmes_sync_toolsets.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Tests should match source structure under tests/
Live execution is now enabled by default to ensure tests match real-world behavior
Files:
tests/test_holmes_sync_toolsets.py
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.222Z
Learning: Applies to holmes/plugins/toolsets/**/*.py : Only define current field names in toolset config schema with extra='allow' to avoid polluting model_dump() output with deprecated fields
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.222Z
Learning: Applies to holmes/plugins/toolsets/**/*.py : Maintain backwards compatibility in toolset config using Pydantic's extra='allow' when renaming config fields
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.222Z
Learning: Applies to holmes/plugins/toolsets/**/*.py : Implement simple Pydantic config class with validation for Python toolsets
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.222Z
Learning: Applies to holmes/plugins/toolsets/**/*.py : Use model_validator(mode='after') in toolset config to map old field names to new names and log deprecation warnings
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.222Z
Learning: Applies to tests/llm/fixtures/**/*.yaml : Toolset config in eval tests must go under config field: toolsets.toolset_name.enabled.config
📚 Learning: 2026-01-05T11:14:20.222Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.222Z
Learning: Applies to holmes/plugins/toolsets/**/*.py : Only define current field names in toolset config schema with extra='allow' to avoid polluting model_dump() output with deprecated fields
Applied to files:
holmes/core/tools.pytests/test_holmes_sync_toolsets.pyholmes/utils/holmes_sync_toolsets.py
📚 Learning: 2026-01-05T11:14:20.222Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.222Z
Learning: Applies to holmes/plugins/toolsets/**/*.py : Maintain backwards compatibility in toolset config using Pydantic's extra='allow' when renaming config fields
Applied to files:
holmes/core/tools.pytests/test_holmes_sync_toolsets.pyholmes/utils/holmes_sync_toolsets.py
📚 Learning: 2026-01-05T11:14:20.222Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.222Z
Learning: Applies to holmes/plugins/toolsets/**/*.py : Implement simple Pydantic config class with validation for Python toolsets
Applied to files:
holmes/core/tools.pytests/test_holmes_sync_toolsets.pyholmes/utils/holmes_sync_toolsets.py
📚 Learning: 2026-01-05T11:14:20.222Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.222Z
Learning: Applies to holmes/plugins/toolsets/**/*.py : Use model_validator(mode='after') in toolset config to map old field names to new names and log deprecation warnings
Applied to files:
holmes/core/tools.py
📚 Learning: 2026-01-05T11:14:20.222Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.222Z
Learning: Applies to tests/llm/fixtures/**/*.yaml : Custom toolsets for eval tests: Create separate toolsets.yaml file, never put toolset config in test_case.yaml
Applied to files:
holmes/core/tools.pytests/test_holmes_sync_toolsets.py
📚 Learning: 2026-01-05T11:14:20.222Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.222Z
Learning: Applies to tests/llm/fixtures/**/*.yaml : Toolset config in eval tests must go under config field: toolsets.toolset_name.enabled.config
Applied to files:
holmes/core/tools.py
📚 Learning: 2026-01-05T11:14:20.222Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.222Z
Learning: Applies to holmes/plugins/toolsets/**/*.py : Use JsonFilterMixin for client-side filtering when server-side filtering is not possible, adding max_depth and jq parameters
Applied to files:
holmes/utils/holmes_sync_toolsets.py
📚 Learning: 2026-01-05T11:14:20.222Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.222Z
Learning: Applies to holmes/plugins/toolsets/**/*.{py,yaml} : All toolsets MUST return detailed error messages from underlying APIs to enable LLM self-correction, including exact query/command executed, time ranges/parameters/filters used, and full API error response (status code and message)
Applied to files:
holmes/utils/holmes_sync_toolsets.py
🧬 Code graph analysis (1)
holmes/utils/holmes_sync_toolsets.py (1)
holmes/core/tools.py (2)
get_config_schema(826-833)ToolsetDBModel(894-904)
🪛 Ruff (0.14.11)
holmes/utils/holmes_sync_toolsets.py
26-26: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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: llm_evals
- GitHub Check: build (3.11)
- GitHub Check: build (3.12)
- GitHub Check: build (3.10)
🔇 Additional comments (3)
tests/test_holmes_sync_toolsets.py (2)
45-46: LGTM — minimal SampleToolset stub is fine.
92-115: LGTM — test now validates null schema path.holmes/core/tools.py (1)
593-833: LGTM — config_class + schema accessor fit the new schema-driven flow.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Add config_class ClassVar and Field annotations with descriptions to all toolsets for JSON Schema generation and frontend form support. Toolsets updated: - newrelic: NewrelicConfig with Field annotations - kafka: KafkaConfig with Field annotations - git: GitHubConfig with Field annotations - datadog/logs: DatadogLogsConfig (already had annotations) - datadog/metrics: DatadogMetricsConfig (already had annotations) - datadog/traces: DatadogTracesConfig (already had annotations) - datadog/general: DatadogGeneralConfig (already had annotations) - bash: BashExecutorConfig with Field annotations - atlas_mongodb: MongoDBConfig with Field annotations - azure_sql: AzureSQLConfig with Field annotations Signed-off-by: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@holmes/plugins/toolsets/bash/common/config.py`:
- Around line 16-19: The Field for allowed_images currently uses a mutable
default (Field(default=[])) which can cause shared-state bugs; update the Field
for allowed_images to use default_factory=list instead (keeping the type
list[KubectlImageConfig] and the description intact) so each model instance gets
a fresh list; locate the allowed_images Field definition in the file and mirror
the pattern used elsewhere in this module (e.g., the other Field that already
uses default_factory=list).
In `@holmes/plugins/toolsets/datadog/toolset_datadog_logs.py`:
- Around line 66-67: Update the Pydantic config classes to add Field metadata so
schema generation and frontend forms get useful labels and examples: in
DatadogBaseConfig add Field(...) with description and example for dd_api_key,
dd_app_key, and site_api_url; in DatadogLogsConfig add Field(...) with
description and example for indexes (e.g., list of index names), storage_tiers
(e.g., list or mapping of tier names), compact_logs (boolean description), and
default_limit (integer description and example). Keep the existing types and
defaults, import Field from pydantic if not already imported, and attach the
description and example parameters to each field declaration (e.g., dd_api_key:
str = Field(..., description="...", example="...")).
- Line 28: Replace the incorrect import of ClassVar and Type from
holmes.core.tools with imports from the typing module: update the import
statement that currently reads "from holmes.core.tools import ClassVar, Type" to
import ClassVar and Type from typing (consolidate with existing typing imports
on the module), ensuring any references to ClassVar and Type in this file (e.g.,
class/type annotations) remain unchanged.
In `@holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py`:
- Around line 9-20: The file imports typing constructs ClassVar and Type from
holmes.core.tools which causes ImportError; update the import statements so
ClassVar and Type are imported from the typing module (e.g., add "from typing
import ClassVar, Type" and remove ClassVar and Type from the holmes.core.tools
import list), and ensure any annotations in this module (e.g., on classes or
functions referencing ClassVar or Type) continue to use those names from the
typing import.
🧹 Nitpick comments (5)
holmes/plugins/toolsets/kafka.py (1)
42-67: Well-structured config with good Field metadata.The
Field()annotations provide clear descriptions for JSON Schema generation. Thekafka_brokerfield includes an example which is useful for the frontend form rendering.Consider adding
examplesto thenamefield as well, since it's a required field and an example would help users understand expected values:♻️ Optional enhancement
class KafkaClusterConfig(BaseModel): - name: str = Field(description="Name identifier for this Kafka cluster") + name: str = Field( + description="Name identifier for this Kafka cluster", + examples=["production-kafka", "staging-kafka"], + )holmes/plugins/toolsets/azure_sql/azure_base_toolset.py (1)
25-38: Add a validator to prevent partial service‑principal configs.
Fail fast if only one or two of the SP fields are provided.♻️ Proposed refactor
-from pydantic import BaseModel, ConfigDict, Field +from pydantic import BaseModel, ConfigDict, Field, model_validator @@ class AzureSQLConfig(BaseModel): database: AzureSQLDatabaseConfig = Field( description="Azure SQL database connection details", ) @@ client_secret: Optional[str] = Field( default=None, description="Azure AD client secret (required for service principal auth)", ) + + `@model_validator`(mode="after") + def _validate_service_principal(self): + creds = (self.tenant_id, self.client_id, self.client_secret) + if any(creds) and not all(creds): + raise ValueError( + "tenant_id, client_id, and client_secret must all be set for service principal auth" + ) + return selfholmes/plugins/toolsets/bash/bash_toolset.py (1)
14-26: ImportClassVarandTypefromtyping, not from intermediate modules.While
holmes.core.toolscurrently re-exports these, importing directly from thetypingmodule follows best practices by using the canonical source. This also reduces unnecessary coupling between modules.♻️ Proposed change
-from typing import Any, Dict, Optional +from typing import Any, ClassVar, Dict, Optional, Type @@ -from holmes.core.tools import ( - ApprovalRequirement, - CallablePrerequisite, - ClassVar, - StructuredToolResult, - StructuredToolResultStatus, - Tool, - ToolInvokeContext, - ToolParameter, - Toolset, - ToolsetTag, - Type, -) +from holmes.core.tools import ( + ApprovalRequirement, + CallablePrerequisite, + StructuredToolResult, + StructuredToolResultStatus, + Tool, + ToolInvokeContext, + ToolParameter, + Toolset, + ToolsetTag, +)holmes/plugins/toolsets/git.py (1)
4-20: ImportClassVarandTypefromtypinginstead ofholmes.core.tools.While
holmes.core.toolsdoes make these available in its namespace, importing them directly fromtypingis cleaner, more explicit, and reduces fragile reliance on module internals.♻️ Suggested change
-from typing import Any, Dict, List, Optional, Tuple +from typing import Any, ClassVar, Dict, List, Optional, Tuple, Type import requests # type: ignore from pydantic import BaseModel, Field from holmes.core.tools import ( CallablePrerequisite, - ClassVar, StructuredToolResult, StructuredToolResultStatus, Tool, ToolInvokeContext, ToolParameter, Toolset, ToolsetTag, - Type, )holmes/plugins/toolsets/atlas_mongodb/mongodb_atlas.py (1)
7-23: ImportClassVarandTypedirectly from thetypingmodule instead ofholmes.core.tools.
ClassVarandTypeare standard library types thatholmes.core.toolsre-exports implicitly (without explicit__all__declaration). Importing directly fromtypingavoids unnecessary coupling and follows Python best practices of importing from canonical sources.♻️ Proposed refactor
-from typing import Any, Dict, List, Tuple +from typing import Any, ClassVar, Dict, List, Tuple, Type ... -from holmes.core.tools import ( - CallablePrerequisite, - ClassVar, - StructuredToolResult, - StructuredToolResultStatus, - Tool, - ToolInvokeContext, - ToolParameter, - Toolset, - ToolsetTag, - Type, -) +from holmes.core.tools import ( + CallablePrerequisite, + StructuredToolResult, + StructuredToolResultStatus, + Tool, + ToolInvokeContext, + ToolParameter, + Toolset, + ToolsetTag, +)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
holmes/plugins/toolsets/atlas_mongodb/mongodb_atlas.pyholmes/plugins/toolsets/azure_sql/azure_base_toolset.pyholmes/plugins/toolsets/bash/bash_toolset.pyholmes/plugins/toolsets/bash/common/config.pyholmes/plugins/toolsets/datadog/toolset_datadog_general.pyholmes/plugins/toolsets/datadog/toolset_datadog_logs.pyholmes/plugins/toolsets/datadog/toolset_datadog_metrics.pyholmes/plugins/toolsets/datadog/toolset_datadog_traces.pyholmes/plugins/toolsets/git.pyholmes/plugins/toolsets/kafka.pyholmes/plugins/toolsets/newrelic/newrelic.py
🚧 Files skipped from review as they are similar to previous changes (2)
- holmes/plugins/toolsets/datadog/toolset_datadog_general.py
- holmes/plugins/toolsets/newrelic/newrelic.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always place Python imports at the top of the file, not inside functions or methods
Use Ruff for formatting and linting with configuration in pyproject.toml
Type hints required (mypy configuration in pyproject.toml)
Pre-commit hooks enforce quality checks on Python files
Files:
holmes/plugins/toolsets/atlas_mongodb/mongodb_atlas.pyholmes/plugins/toolsets/azure_sql/azure_base_toolset.pyholmes/plugins/toolsets/git.pyholmes/plugins/toolsets/datadog/toolset_datadog_metrics.pyholmes/plugins/toolsets/datadog/toolset_datadog_traces.pyholmes/plugins/toolsets/bash/bash_toolset.pyholmes/plugins/toolsets/datadog/toolset_datadog_logs.pyholmes/plugins/toolsets/bash/common/config.pyholmes/plugins/toolsets/kafka.py
holmes/plugins/toolsets/**/*.{py,yaml}
📄 CodeRabbit inference engine (CLAUDE.md)
holmes/plugins/toolsets/**/*.{py,yaml}: All toolsets MUST return detailed error messages from underlying APIs to enable LLM self-correction, including exact query/command executed, time ranges/parameters/filters used, and full API error response (status code and message)
For 'no data' responses in toolsets, specify what was searched and where
Never return unbounded data from APIs - always include filter parameters on tools that query collections
Files:
holmes/plugins/toolsets/atlas_mongodb/mongodb_atlas.pyholmes/plugins/toolsets/azure_sql/azure_base_toolset.pyholmes/plugins/toolsets/git.pyholmes/plugins/toolsets/datadog/toolset_datadog_metrics.pyholmes/plugins/toolsets/datadog/toolset_datadog_traces.pyholmes/plugins/toolsets/bash/bash_toolset.pyholmes/plugins/toolsets/datadog/toolset_datadog_logs.pyholmes/plugins/toolsets/bash/common/config.pyholmes/plugins/toolsets/kafka.py
holmes/plugins/toolsets/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
holmes/plugins/toolsets/**/*.py: Use requests library for HTTP calls in Python toolsets, not specialized client libraries like opensearchpy
Implement simple Pydantic config class with validation for Python toolsets
Include health check in prerequisites_callable() method for Python toolsets
Each tool in Python toolsets should be a thin wrapper around a single API endpoint
Use JsonFilterMixin for client-side filtering when server-side filtering is not possible, adding max_depth and jq parameters
Maintain backwards compatibility in toolset config using Pydantic's extra='allow' when renaming config fields
Only define current field names in toolset config schema with extra='allow' to avoid polluting model_dump() output with deprecated fields
Use@model_validator(mode='after') in toolset config to map old field names to new names and log deprecation warnings
Bash toolset validates commands for safety
Files:
holmes/plugins/toolsets/atlas_mongodb/mongodb_atlas.pyholmes/plugins/toolsets/azure_sql/azure_base_toolset.pyholmes/plugins/toolsets/git.pyholmes/plugins/toolsets/datadog/toolset_datadog_metrics.pyholmes/plugins/toolsets/datadog/toolset_datadog_traces.pyholmes/plugins/toolsets/bash/bash_toolset.pyholmes/plugins/toolsets/datadog/toolset_datadog_logs.pyholmes/plugins/toolsets/bash/common/config.pyholmes/plugins/toolsets/kafka.py
**/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php}: Use semantic, descriptive names for variables, functions, and components
Write clear, concise comments that explain 'why' rather than 'what'
Files:
holmes/plugins/toolsets/atlas_mongodb/mongodb_atlas.pyholmes/plugins/toolsets/azure_sql/azure_base_toolset.pyholmes/plugins/toolsets/git.pyholmes/plugins/toolsets/datadog/toolset_datadog_metrics.pyholmes/plugins/toolsets/datadog/toolset_datadog_traces.pyholmes/plugins/toolsets/bash/bash_toolset.pyholmes/plugins/toolsets/datadog/toolset_datadog_logs.pyholmes/plugins/toolsets/bash/common/config.pyholmes/plugins/toolsets/kafka.py
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.222Z
Learning: Applies to holmes/plugins/toolsets/**/*.py : Maintain backwards compatibility in toolset config using Pydantic's extra='allow' when renaming config fields
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.222Z
Learning: Applies to holmes/plugins/toolsets/**/*.py : Implement simple Pydantic config class with validation for Python toolsets
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.222Z
Learning: Applies to holmes/plugins/toolsets/**/*.py : Only define current field names in toolset config schema with extra='allow' to avoid polluting model_dump() output with deprecated fields
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.222Z
Learning: Applies to holmes/plugins/toolsets/**/*.py : Use model_validator(mode='after') in toolset config to map old field names to new names and log deprecation warnings
📚 Learning: 2026-01-05T11:14:20.222Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.222Z
Learning: Applies to holmes/plugins/toolsets/**/*.py : Maintain backwards compatibility in toolset config using Pydantic's extra='allow' when renaming config fields
Applied to files:
holmes/plugins/toolsets/atlas_mongodb/mongodb_atlas.pyholmes/plugins/toolsets/azure_sql/azure_base_toolset.pyholmes/plugins/toolsets/git.pyholmes/plugins/toolsets/datadog/toolset_datadog_metrics.pyholmes/plugins/toolsets/datadog/toolset_datadog_traces.pyholmes/plugins/toolsets/bash/bash_toolset.pyholmes/plugins/toolsets/datadog/toolset_datadog_logs.pyholmes/plugins/toolsets/bash/common/config.pyholmes/plugins/toolsets/kafka.py
📚 Learning: 2026-01-05T11:14:20.222Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.222Z
Learning: Applies to holmes/plugins/toolsets/**/*.py : Only define current field names in toolset config schema with extra='allow' to avoid polluting model_dump() output with deprecated fields
Applied to files:
holmes/plugins/toolsets/atlas_mongodb/mongodb_atlas.pyholmes/plugins/toolsets/azure_sql/azure_base_toolset.pyholmes/plugins/toolsets/git.pyholmes/plugins/toolsets/datadog/toolset_datadog_metrics.pyholmes/plugins/toolsets/datadog/toolset_datadog_traces.pyholmes/plugins/toolsets/bash/bash_toolset.pyholmes/plugins/toolsets/bash/common/config.pyholmes/plugins/toolsets/kafka.py
📚 Learning: 2026-01-05T11:14:20.222Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.222Z
Learning: Applies to holmes/plugins/toolsets/**/*.py : Implement simple Pydantic config class with validation for Python toolsets
Applied to files:
holmes/plugins/toolsets/atlas_mongodb/mongodb_atlas.pyholmes/plugins/toolsets/azure_sql/azure_base_toolset.pyholmes/plugins/toolsets/git.pyholmes/plugins/toolsets/datadog/toolset_datadog_metrics.pyholmes/plugins/toolsets/datadog/toolset_datadog_traces.pyholmes/plugins/toolsets/bash/bash_toolset.pyholmes/plugins/toolsets/datadog/toolset_datadog_logs.pyholmes/plugins/toolsets/bash/common/config.pyholmes/plugins/toolsets/kafka.py
📚 Learning: 2026-01-05T11:14:20.222Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.222Z
Learning: Applies to holmes/plugins/toolsets/**/*.py : Each tool in Python toolsets should be a thin wrapper around a single API endpoint
Applied to files:
holmes/plugins/toolsets/atlas_mongodb/mongodb_atlas.pyholmes/plugins/toolsets/git.pyholmes/plugins/toolsets/datadog/toolset_datadog_metrics.pyholmes/plugins/toolsets/datadog/toolset_datadog_traces.py
📚 Learning: 2026-01-05T11:14:20.222Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.222Z
Learning: Applies to holmes/plugins/toolsets/**/*.{py,yaml} : Never return unbounded data from APIs - always include filter parameters on tools that query collections
Applied to files:
holmes/plugins/toolsets/atlas_mongodb/mongodb_atlas.pyholmes/plugins/toolsets/datadog/toolset_datadog_metrics.pyholmes/plugins/toolsets/datadog/toolset_datadog_traces.py
📚 Learning: 2026-01-05T11:14:20.222Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.222Z
Learning: Applies to holmes/plugins/toolsets/**/*.py : Use requests library for HTTP calls in Python toolsets, not specialized client libraries like opensearchpy
Applied to files:
holmes/plugins/toolsets/atlas_mongodb/mongodb_atlas.pyholmes/plugins/toolsets/git.py
📚 Learning: 2026-01-05T11:14:20.222Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.222Z
Learning: Applies to holmes/plugins/toolsets/**/*.py : Use model_validator(mode='after') in toolset config to map old field names to new names and log deprecation warnings
Applied to files:
holmes/plugins/toolsets/azure_sql/azure_base_toolset.pyholmes/plugins/toolsets/bash/common/config.pyholmes/plugins/toolsets/kafka.py
📚 Learning: 2026-01-05T11:14:20.222Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.222Z
Learning: Applies to holmes/plugins/toolsets/**/*.py : Include health check in prerequisites_callable() method for Python toolsets
Applied to files:
holmes/plugins/toolsets/datadog/toolset_datadog_metrics.pyholmes/plugins/toolsets/datadog/toolset_datadog_traces.pyholmes/plugins/toolsets/bash/bash_toolset.py
📚 Learning: 2026-01-05T11:14:20.222Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.222Z
Learning: Applies to holmes/plugins/toolsets/**/*.{py,yaml} : All toolsets MUST return detailed error messages from underlying APIs to enable LLM self-correction, including exact query/command executed, time ranges/parameters/filters used, and full API error response (status code and message)
Applied to files:
holmes/plugins/toolsets/datadog/toolset_datadog_metrics.pyholmes/plugins/toolsets/datadog/toolset_datadog_traces.py
📚 Learning: 2026-01-05T11:14:20.222Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.222Z
Learning: Applies to holmes/plugins/toolsets/**/*.py : Bash toolset validates commands for safety
Applied to files:
holmes/plugins/toolsets/datadog/toolset_datadog_metrics.pyholmes/plugins/toolsets/bash/bash_toolset.py
📚 Learning: 2026-01-05T11:14:20.222Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.222Z
Learning: Applies to holmes/plugins/toolsets/**/kubernetes*.py : RBAC permissions are respected for Kubernetes access
Applied to files:
holmes/plugins/toolsets/bash/common/config.py
🧬 Code graph analysis (6)
holmes/plugins/toolsets/atlas_mongodb/mongodb_atlas.py (2)
holmes/core/tools.py (5)
StructuredToolResult(83-109)StructuredToolResultStatus(56-80)Tool(169-418)ToolInvokeContext(158-166)ToolParameter(149-155)holmes/plugins/toolsets/utils.py (1)
toolset_name_for_one_liner(232-236)
holmes/plugins/toolsets/azure_sql/azure_base_toolset.py (1)
holmes/core/tools.py (2)
Tool(169-418)Toolset(590-850)
holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py (2)
holmes/core/tools.py (5)
StructuredToolResult(83-109)StructuredToolResultStatus(56-80)Tool(169-418)ToolParameter(149-155)Toolset(590-850)holmes/plugins/toolsets/datadog/datadog_models.py (1)
DatadogMetricsConfig(28-31)
holmes/plugins/toolsets/bash/bash_toolset.py (2)
holmes/core/tools.py (3)
Tool(169-418)Toolset(590-850)ToolsetTag(137-140)holmes/plugins/toolsets/bash/common/config.py (1)
BashExecutorConfig(22-26)
holmes/plugins/toolsets/datadog/toolset_datadog_logs.py (1)
holmes/plugins/toolsets/datadog/datadog_models.py (1)
DatadogLogsConfig(40-50)
holmes/plugins/toolsets/kafka.py (2)
holmes/core/tools.py (7)
CallablePrerequisite(577-578)StructuredToolResult(83-109)StructuredToolResultStatus(56-80)Tool(169-418)ToolParameter(149-155)Toolset(590-850)ToolsetTag(137-140)holmes/plugins/toolsets/utils.py (2)
get_param_or_raise(225-229)toolset_name_for_one_liner(232-236)
⏰ 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.10)
- GitHub Check: build (3.11)
- GitHub Check: build (3.12)
- GitHub Check: llm_evals
🔇 Additional comments (12)
holmes/plugins/toolsets/datadog/toolset_datadog_traces.py (2)
12-23: LGTM on imports.The
ClassVarandTypeimports fromholmes.core.toolsare correctly added to support the newconfig_classclass variable declaration pattern.
49-54: LGTM on config_class declaration.The
config_classclass variable correctly referencesDatadogTracesConfig, enabling JSON Schema generation for frontend form rendering. This aligns with the broader PR pattern of replacingget_example_config()methods with the class-levelconfig_classhook.holmes/plugins/toolsets/kafka.py (3)
24-37: LGTM on imports.The imports for
Field,ClassVar, andTypeare correctly added to support the enhanced config model definitions andconfig_classdeclaration.
70-73: LGTM on KafkaConfig field definition.The
Field()annotation with description is appropriate for thekafka_clusterslist.
587-592: LGTM on config_class declaration.The
config_classclass variable correctly referencesKafkaConfig, enabling JSON Schema generation. The placement beforemodel_configand instance attributes is appropriate.holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py (1)
659-661: config_class hook looks good.
This aligns the toolset with the schema-driven config flow.holmes/plugins/toolsets/azure_sql/azure_base_toolset.py (3)
3-5: LGTM for the schema-related imports.
10-21: Nice schema metadata for DB fields.
42-44: Good addition of the config schema hook.holmes/plugins/toolsets/git.py (1)
24-44: No changes needed. TheGitHubConfigdoes not have deprecated or renamed fields, so addingConfigDict(extra="allow")is unnecessary. The codebase pattern (RabbitMQ, Prometheus) shows thatextra="allow"is used specifically when maintaining backwards compatibility during field renames, paired with a@model_validator(mode="after")to map old names to new ones. Without field migrations, addingextra="allow"alone would pollutemodel_dump()output per the project learnings.Likely an incorrect or invalid review comment.
holmes/plugins/toolsets/atlas_mongodb/mongodb_atlas.py (2)
28-37: Schema descriptions look good.Field descriptions should improve the generated config schema for form rendering.
Please verify that the schema generation path (e.g.,
Toolset.get_config_schema()) includes PydanticFieldmetadata so these descriptions appear in the JSON sent to the DB.
42-42:config_classhook is wired correctly.This cleanly exposes the config model for schema reporting.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@holmes/utils/holmes_sync_toolsets.py`:
- Around line 54-70: The code currently sets installation_instructions to only
the JSON schema from toolset.get_config_schema(), dropping the original markdown
instructions and example config; change the block that builds ToolsetDBModel so
installation_instructions is a JSON payload that contains the original
instructions plus the schema and example config (e.g., {"instructions":
toolset.installation_instructions, "schema": config_schema, "example_config":
toolset.example_config} serialized via json.dumps with _json_serializer) instead
of just config_schema_json; update references around ToolsetDBModel(...) and its
model_dump() usage so the frontend receives the combined JSON object under
installation_instructions.
🧹 Nitpick comments (1)
holmes/utils/holmes_sync_toolsets.py (1)
22-27:__str__guard is redundant and hides unexpected types.
Every object has__str__, so theTypeErrorbranch is unreachable. Either simplify toreturn str(obj)or whitelist supported types to avoid silently stringifying unexpected objects.♻️ Suggested simplification (no behavior change)
def _json_serializer(obj: Any) -> Any: """Custom JSON serializer for objects not serializable by default json code.""" - if hasattr(obj, "__str__"): - return str(obj) - raise TypeError(f"Object of type {type(obj).__name__} is not JSON serializable") + return str(obj)
18bf829 to
a2f08fe
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@holmes/plugins/toolsets/kafka.py`:
- Around line 42-84: The Pydantic models KafkaClusterConfig and KafkaConfig
currently use the default extra behavior which drops unknown keys; update both
models to allow unknown keys by adding a Pydantic config to each model that sets
extra="allow" (i.e., add an inner Config class or the appropriate pydantic v2
model_config mapping depending on your pydantic version) so KafkaClusterConfig
and KafkaConfig retain unknown fields for backwards compatibility.
♻️ Duplicate comments (4)
holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py (1)
9-20: ImportClassVarandTypefromtypingmodule instead ofholmes.core.tools.The past review flagged this as addressed, but the current code still imports
ClassVar(line 11) andType(line 19) fromholmes.core.tools. These are standard typing constructs that should be imported from thetypingmodule. This creates unnecessary indirection and is inconsistent with other toolsets in the codebase.#!/bin/bash # Verify the actual import pattern in the file rg -n "from typing import" holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py rg -n "ClassVar|Type" holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py | head -10Suggested fix
-from typing import Any, Dict, Optional, Tuple +from typing import Any, ClassVar, Dict, Optional, Tuple, Type from pydantic import AnyUrl from holmes.core.tools import ( CallablePrerequisite, - ClassVar, StructuredToolResult, StructuredToolResultStatus, Tool, ToolInvokeContext, ToolParameter, Toolset, ToolsetTag, - Type, )holmes/plugins/toolsets/datadog/toolset_datadog_logs.py (1)
28-28: ImportClassVarandTypefromtypingand consolidate with existing typing imports.Two issues with this import:
ClassVarandTypeshould be imported from thetypingmodule, notholmes.core.tools- Per coding guidelines, imports should be at the top of the file — consolidate with the existing
typingimports on line 4Suggested fix
-from typing import Any, Dict, Optional, Tuple +from typing import Any, ClassVar, Dict, Optional, Tuple, TypeAnd remove line 28:
-from holmes.core.tools import ClassVar, Typeholmes/utils/holmes_sync_toolsets.py (2)
54-70: Installation instructions are dropped; wrap schema with instructions and example config.Per the PR objectives,
installation_instructionsshould be a JSON object containinginstructions,config_schema, andexample_configfor frontend form rendering while maintaining backwards compatibility. The current implementation only sends the raw config schema, dropping the original markdown instructions entirely.🐛 Proposed fix (preserve instructions + schema)
# Get config schema for frontend form generation config_schema = toolset.get_config_schema() - config_schema_json = ( - json.dumps(config_schema, default=_json_serializer) - if config_schema - else None - ) + + # Build installation payload with instructions, schema, and example config + installation_payload = None + if hasattr(toolset, 'installation_instructions') or config_schema is not None: + installation_payload = { + "instructions": getattr(toolset, 'installation_instructions', None), + "config_schema": config_schema, + "example_config": getattr(toolset, 'get_example_config', lambda: None)(), + } + + installation_json = ( + json.dumps(installation_payload, default=_json_serializer) + if installation_payload is not None + else None + ) db_toolsets.append( ToolsetDBModel( **toolset.model_dump(exclude_none=True), toolset_name=toolset.name, cluster_id=config.cluster_name, account_id=dal.account_id, updated_at=updated_at, - installation_instructions=config_schema_json, + installation_instructions=installation_json, ).model_dump() )
56-60: Don't treat empty schema as "missing".The truthy check
if config_schemawill drop valid but empty schemas (e.g.,{}). Use an explicitis not Nonecheck to preserve intent. This was flagged in a prior review and marked as addressed, but the code still shows the truthy check.🐛 Proposed fix
config_schema_json = ( json.dumps(config_schema, default=_json_serializer) - if config_schema + if config_schema is not None else None )
🧹 Nitpick comments (3)
holmes/plugins/toolsets/elasticsearch/elasticsearch.py (1)
22-63: Well-structured config with rich metadata for schema generation.The Field definitions with descriptions and examples are well-implemented and will generate useful JSON Schema for the frontend UI. The env variable templating pattern (
{{ env.ELASTICSEARCH_API_KEY }}) in the examples is a good practice for sensitive configuration.Consider adding
extra='allow'for future backwards compatibility.Based on learnings, toolset configs should use
extra='allow'to maintain backwards compatibility when fields are renamed in the future. This prevents validation errors if users have old config fields.♻️ Optional: Add extra='allow' for future-proofing
class ElasticsearchConfig(BaseModel): """Configuration for Elasticsearch/OpenSearch API access. ... """ + model_config = ConfigDict(extra='allow') url: str = Field(holmes/plugins/toolsets/rabbitmq/toolset_rabbitmq.py (1)
28-40: Consider addingextra="allow"for consistency and future backwards compatibility.
RabbitMQClusterConfiginapi.pyusesConfigDict(extra="allow")to support deprecated field handling, butRabbitMQConfighere does not. While there are no deprecated fields at this level currently, adding it would maintain consistency with the nested config and prepare for potential future field renames.Based on learnings,
extra='allow'is the standard pattern for maintaining backwards compatibility in toolset configs.♻️ Suggested change
+from pydantic import BaseModel, ConfigDict, Field -from pydantic import BaseModel, Field class RabbitMQConfig(BaseModel): + model_config = ConfigDict(extra="allow") + clusters: List[RabbitMQClusterConfig] = Field(holmes/plugins/toolsets/prometheus/prometheus.py (1)
1760-1760: Consider exposing AMP/Azure fields in the generated schema.
config_classpoints toPrometheusConfig, but the toolset acceptsAMPConfigandAzurePrometheusConfigbased on input. If the frontend form relies solely on this schema, AWS/Azure-specific fields won’t be discoverable. Consider a union/wrapper schema or explicitly documenting that those modes require manual JSON config. Please verify the intended UX for AMP/Azure users.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@holmes/utils/pydantic_utils.py`:
- Around line 62-115: The function build_config_example currently reads examples
before defaults, but the docstring requires defaults/default_factory be applied
first; modify build_config_example so that for each field (iterate
model_cls.model_fields and use field_info), you first check default
(field_info.default != PydanticUndefined) and then default_factory
(field_info.default_factory) and set example_value from those (catch exceptions
from default_factory and fall back), only after those checks consult examples
(field_info.examples or field_info.json_schema_extra.get("examples")) and take
the first example if present, then proceed with nested model handling via
annotation -> _extract_base_model_subclass and final fallback to
f"your_{field_name}", preserving existing exclude handling and BaseModel
instance behavior.
♻️ Duplicate comments (2)
holmes/plugins/toolsets/bash/common/config.py (1)
18-22: Preferdefault_factory=listfor mutable defaults (re-flag).Line 18 uses a mutable default list; use
default_factory=listto avoid shared-state pitfalls.♻️ Proposed fix
allowed_images: list[KubectlImageConfig] = Field( - default=[], + default_factory=list, description="List of allowed container images for kubectl run", examples=[[build_config_example(KubectlImageConfig)]] )Pydantic v2 Field default_factory mutable defaults best practicesholmes/utils/holmes_sync_toolsets.py (1)
54-77: Installation payload drops instructions and schema (regression vs PR objective).
installation_instructionsis now only the example JSON. This drops the existing markdown instructions and the config schema, andconfig_schema_jsonis computed but unused. Also, the truthy checks will discard valid empty schemas/examples. Please wrap instructions + schema + example into one JSON payload and serialize that (use explicitis not Nonechecks).🐛 Proposed fix (wrap instructions + schema + example)
- config_schema = toolset.get_config_schema() - config_schema_json = ( - json.dumps(config_schema, default=_json_serializer) - if config_schema - else None - ) - - config_example = toolset.get_config_example() - config_example_json = ( - json.dumps(config_example, default=_json_serializer) - if config_example - else None - ) + config_schema = toolset.get_config_schema() + config_example = toolset.get_config_example() + instructions = getattr(toolset, "installation_instructions", None) + + installation_payload = ( + { + "instructions": instructions, + "config_schema": config_schema, + "example_config": config_example, + } + if instructions is not None + or config_schema is not None + or config_example is not None + else None + ) + installation_instructions_json = ( + json.dumps(installation_payload, default=_json_serializer) + if installation_payload is not None + else None + ) @@ - installation_instructions=config_example_json, + installation_instructions=installation_instructions_json,
🧹 Nitpick comments (2)
holmes/utils/pydantic_utils.py (2)
13-17: Narrow the exception toImportErrorto avoid masking real failures.Catching
Exceptionhere can hide genuine import-time errors insidepydantic_core. PreferImportErrorfor the compatibility shim.♻️ Proposed refinement
-try: - # pydantic v2 - from pydantic_core import PydanticUndefined # type: ignore -except Exception: # pragma: no cover - PydanticUndefined = object() # type: ignore +try: + # pydantic v2 + from pydantic_core import PydanticUndefined # type: ignore +except ImportError: # pragma: no cover + PydanticUndefined = object() # type: ignore
117-146: Consider handling PEP 604 unions (A | None) in BaseModel extraction.
get_origin()returnstypes.UnionTypeforA | Bin newer Python versions, so Optional-style annotations using|may be skipped.♻️ Suggested enhancement
+from types import UnionType @@ - if origin is Union: + if origin in (Union, UnionType): args = [a for a in get_args(annotation) if a is not type(None)] # noqa: E721 if len(args) == 1: return _extract_base_model_subclass(args[0]) return None
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@tests/plugins/toolsets/atlas_mongodb/test_mongodb_atlas.py`:
- Around line 5-10: The test function
test_build_config_example_mongodb_atlas_config is missing an explicit return
type annotation; update its signature to include "-> None" (e.g., def
test_build_config_example_mongodb_atlas_config() -> None:) to satisfy the
project's type-hinting requirement while keeping the body unchanged; reference
symbols: test_build_config_example_mongodb_atlas_config, build_config_example,
MongoDBConfig.
In `@tests/plugins/toolsets/bash/test_bash.py`:
- Around line 5-15: The test function
test_build_config_example_bash_executor_config is missing an explicit return
type; update its signature to include "-> None" to satisfy the project's typing
requirements. Locate the function definition for
test_build_config_example_bash_executor_config and add the return type
annotation while keeping the body unchanged; ensure any imports or test
frameworks remain unaffected.
In `@tests/plugins/toolsets/prometheus/test_prometheus.py`:
- Around line 52-72: The test expects an Authorization header example but
build_config_example applies the dataclass default/default_factory (headers
default_factory=dict -> {}), so update the test to assert example["headers"] ==
{} (or otherwise remove the Authorization expectation) instead of the current
Authorization value; locate the assertion in
test_build_config_example_prometheus_config referencing PrometheusConfig and the
"headers" key and change it to match the helper's current behavior.
- Around line 1-2: The test currently imports PrometheusConfig from the external
library; update the import to use the toolset's own PrometheusConfig class
(holmes.plugins.toolsets.prometheus.prometheus.PrometheusConfig) so the test
validates the toolset schema and Field metadata; keep using build_config_example
from holmes.utils.pydantic_utils and ensure any references to PrometheusConfig
in the test (e.g., in test functions) now refer to the toolset class name
PrometheusConfig.
In `@tests/plugins/toolsets/rabbitmq/test_rabbitmq.py`:
- Around line 13-16: The test currently asserts a hardcoded password literal
("holmes_password") which triggers Ruff S105; to suppress this, introduce a
local constant like TEST_RABBITMQ_PASSWORD = "holmes_password" with a trailing
comment `# noqa: S105` (or alternatively append ` # noqa: S105` to the password
assertion) and then use that constant in the assertion (cluster0["password"] ==
TEST_RABBITMQ_PASSWORD) so the linter warning is silenced while keeping the test
semantics; update references in tests/plugins/toolsets/rabbitmq/test_rabbitmq.py
around the cluster0 assertions accordingly.
🧹 Nitpick comments (3)
tests/plugins/toolsets/datadog/test_toolset_datadog_general.py (1)
180-189: LGTM!The test correctly validates the
build_config_exampleoutput forDatadogGeneralConfig, including the general-specific fieldsmax_response_sizeandallow_custom_endpoints.Minor observation: This test is a standalone function outside
TestDatadogGeneralToolset, whereas similar tests in other files (e.g.,test_datadog_metrics.py) are placed inside their respective test classes. Consider moving it inside the class for consistency, though this is purely stylistic.tests/plugins/toolsets/datadog/logs/test_datadog_logs_utils.py (1)
1-3: Imports should follow standard ordering: stdlib first, then third-party, then local.The
pytestimport should be placed before theholmespackage imports.Suggested fix
+import pytest + from holmes.plugins.toolsets.datadog.datadog_models import DataDogStorageTier, DatadogLogsConfig from holmes.utils.pydantic_utils import build_config_example -import pytesttests/plugins/toolsets/grafana/test_grafana.py (1)
1-10: Consider reordering imports to follow PEP 8 conventions.Local
holmes.*imports appear before the third-partypytestimport. PEP 8 recommends: standard library → third-party → local application imports.Suggested import order
-from holmes.plugins.toolsets.grafana.common import GrafanaConfig, GrafanaTempoConfig -from holmes.utils.pydantic_utils import build_config_example import pytest from holmes.core.tools import ToolsetStatusEnum +from holmes.plugins.toolsets.grafana.common import GrafanaConfig, GrafanaTempoConfig from holmes.plugins.toolsets.grafana.loki.toolset_grafana_loki import ( GrafanaLokiToolset, ) from holmes.plugins.toolsets.grafana.toolset_grafana import GrafanaDashboardConfig, GrafanaToolset +from holmes.utils.pydantic_utils import build_config_example from tests.plugins.toolsets.grafana.conftest import check_service_runningNote: If Ruff is configured in pre-commit hooks, this should be auto-fixed.
|
examples |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/plugins/toolsets/grafana/test_grafana.py`:
- Around line 1-8: Remove the unused imports GrafanaTempoConfig and
GrafanaDashboardConfig from the test file import block; specifically delete the
references to GrafanaTempoConfig (from holmes.plugins.toolsets.grafana.common)
and GrafanaDashboardConfig (from
holmes.plugins.toolsets.grafana.toolset_grafana) so only the required imports
(pytest, ToolsetStatusEnum, GrafanaLokiToolset, GrafanaToolset) remain.
🧹 Nitpick comments (1)
tests/plugins/toolsets/grafana/test_grafana_common.py (1)
1-7: Consider reordering imports to follow PEP8 conventions.The
pytestimport (third-party) is placed between application imports. Standard convention groups imports as: standard library → third-party → local application.Suggested import order
from typing import Dict, Optional +import pytest + from holmes.plugins.toolsets.grafana.toolset_grafana import GrafanaDashboardConfig from holmes.utils.pydantic_utils import build_config_example -import pytest - from holmes.plugins.toolsets.grafana.common import GrafanaConfig, GrafanaTempoConfig, build_headers
<!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Deployment image pull policy is now configurable via chart values instead of being hard-coded. It defaults to "IfNotPresent", allowing operators to change the pull behavior at deploy/upgrade time without code changes. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: drfaust92 <ilia.lazebnik@gmail.com>
Fix failing windows builds with flag to support longer file names Signed-off-by: Roi Glinik <groi.tech@gmail.com>
…change (#1375) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Removed multiple outdated test fixtures for New Relic NRQL query operations, including queries for crashpod logs, Prometheus logs, and related infrastructure data * Added test fixture for TCP connectivity checks against Prometheus server * **Chores** * Updated CI/CD workflow configuration to streamline Windows build process <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Roi Glinik <groi.tech@gmail.com>
<!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added troubleshooting for SSL certificate verification errors with GitHub Enterprise, plus step‑by‑step deployment examples and updated host examples to use https://. * **New Features** * Support for supplying a custom CA certificate to GitHub MCP deployments across deployment methods. * Option to bypass SSL certificate verification for self‑signed/internal CA setups. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Claude <noreply@anthropic.com> Signed-off-by: Arik Alon <alon.arik@gmail.com> Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Naomi Caren <naomi@robusta.dev>
Signed-off-by: Naomi Caren <naomi@robusta.dev>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@holmes/plugins/toolsets/internet/internet.py`:
- Around line 194-196: The current expression accesses
self.toolset.config.additional_headers without ensuring config exists, causing
AttributeError when prerequisites_callable left self.config as None; update the
logic to first get the config (e.g., config = self.toolset.config or
self.config) and then derive additional_headers using a safe check like "if
config and config.additional_headers else {}" so you only access
additional_headers when config is not None; adjust references in the same scope
(look for additional_headers, self.toolset.config, and prerequisites_callable)
accordingly.
🧹 Nitpick comments (3)
holmes/plugins/toolsets/azure_sql/azure_base_toolset.py (1)
32-46: Consider adding cross-field validation for service principal auth fields.The three auth fields (
tenant_id,client_id,client_secret) are individually optional but described as required together for service principal auth. Amodel_validatorcould enforce that either all three are provided or none are, catching misconfigurations early.♻️ Suggested enhancement
+from pydantic import model_validator class AzureSQLConfig(BaseModel): database: AzureSQLDatabaseConfig = Field( description="Azure SQL database connection details", ) tenant_id: Optional[str] = Field( default=None, description="Azure AD tenant ID (required for service principal auth)", examples=["{{ env.AZURE_TENANT_ID }}"], ) client_id: Optional[str] = Field( default=None, description="Azure AD client/application ID (required for service principal auth)", examples=["{{ env.AZURE_CLIENT_ID }}"], ) client_secret: Optional[str] = Field( default=None, description="Azure AD client secret (required for service principal auth)", examples=["{{ env.AZURE_CLIENT_SECRET }}"], ) + + `@model_validator`(mode="after") + def validate_service_principal_auth(self) -> "AzureSQLConfig": + auth_fields = [self.tenant_id, self.client_id, self.client_secret] + provided = [f for f in auth_fields if f is not None] + if provided and len(provided) != 3: + raise ValueError( + "Service principal auth requires all of: tenant_id, client_id, client_secret" + ) + return selfholmes/plugins/toolsets/internet/internet.py (2)
224-233: Missing blank line between class definitions.PEP 8 recommends two blank lines between top-level class definitions for readability.
Suggested fix
{"Authorization": "Bearer <token>"}, ] ) + + class InternetBaseToolset(Toolset):
261-265: Consider always initializing config to avoidNonestate.When
configis falsy, returning early leavesself.configasNone. A more robust approach is to always initialize with defaults.Proposed fix
def prerequisites_callable(self, config: Dict[str, Any]) -> Tuple[bool, str]: - if not config: - return True, "" - self.config = InternetBaseToolsetConfig(**config) + self.config = InternetBaseToolsetConfig(**(config or {})) return True, ""This approach ensures
self.configis always a validInternetBaseToolsetConfiginstance with defaults applied, eliminating the need forNonechecks elsewhere.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@holmes/plugins/toolsets/datadog/toolset_datadog_traces.py`:
- Around line 12-23: The import list currently pulls ClassVar and Type from
holmes.core.tools; change it to import ClassVar and Type directly from the
typing module and remove them from the holmes.core.tools import list—keep all
other symbols (CallablePrerequisite, StructuredToolResult,
StructuredToolResultStatus, Tool, ToolInvokeContext, ToolParameter, Toolset,
ToolsetTag) imported from holmes.core.tools so references to ClassVar and Type
used elsewhere in this file (e.g., any annotations) come from typing instead of
holmes.core.tools.
In `@holmes/plugins/toolsets/kafka.py`:
- Around line 28-36: The imports currently pull ClassVar and Type from
holmes.core.tools; update the import statements so ClassVar and Type are
imported from the standard typing module instead (e.g., add "from typing import
ClassVar, Type") and remove ClassVar and Type from the holmes.core.tools import
list (which contains StructuredToolResult, Tool, Toolset, etc.) so only the
typing constructs come from typing and the tool classes remain imported from
holmes.core.tools.
In `@holmes/plugins/toolsets/mcp/toolset_mcp.py`:
- Around line 98-115: prerequisites_callable currently defaults to SSE when mode
is missing, which makes dicts that include stdio-specific keys like "command"
validate against MCPConfig (requiring url) and fail; update
prerequisites_callable to infer and set the correct mode before instantiating
the config: if the incoming config dict contains "command" or "args" treat it as
StdioMCPConfig (set mode=MCPMode.STDIO) and if it contains "url" treat it as
SseMCPConfig (set mode=MCPMode.SSE), then instantiate the appropriate class (or
alternatively make the mode Field required on MCPConfig/StdioMCPConfig so
missing mode cannot occur); reference MCPMode, MCPConfig, StdioMCPConfig and the
prerequisites_callable function when applying the change.
♻️ Duplicate comments (5)
holmes/plugins/toolsets/datadog/toolset_datadog_metrics.py (1)
9-20: ImportClassVar/Typefromtyping(ruff RUF012).Same issue as previously noted:
holmes.core.toolsindirection can keep lint warnings.🛠️ Suggested fix
-from typing import Any, Dict, Optional, Tuple +from typing import Any, ClassVar, Dict, Optional, Tuple, Type from holmes.core.tools import ( CallablePrerequisite, - ClassVar, StructuredToolResult, StructuredToolResultStatus, Tool, ToolInvokeContext, ToolParameter, Toolset, ToolsetTag, - Type, )holmes/plugins/toolsets/datadog/toolset_datadog_general.py (1)
10-21: Same ClassVar/Type import issue as in bash_toolset.py.Please import
ClassVarandTypefromtypinghere as well to avoid potential import errors and Ruff RUF012.Also applies to: 203-204
holmes/plugins/toolsets/atlas_mongodb/mongodb_atlas.py (1)
10-43: Same ClassVar/Type import issue as in bash_toolset.py.Please import
ClassVarandTypefromtypingto avoid potential import errors and Ruff RUF012.holmes/plugins/toolsets/internet/internet.py (1)
194-196: PotentialAttributeErrorwhenconfigisNone.When
prerequisites_callablereceives an empty config (lines 264-265),self.configremainsNone. Accessingself.toolset.config.additional_headerswill raiseAttributeError.Proposed fix
additional_headers = ( - self.toolset.config.additional_headers if self.toolset.config.additional_headers else {} + self.toolset.config.additional_headers if self.toolset.config and self.toolset.config.additional_headers else {} )holmes/plugins/toolsets/kafka.py (1)
43-86: Addextra="allow"to config models for backwards compatibility.Based on learnings, toolset config models should use
extra="allow"to maintain backwards compatibility when fields are renamed or deprecated in future versions. This allows unknown keys to be preserved rather than silently dropped.♻️ Recommended change
class KafkaClusterConfig(BaseModel): + model_config = ConfigDict(extra="allow") + name: str = Field(class KafkaConfig(BaseModel): + model_config = ConfigDict(extra="allow") + kafka_clusters: List[KafkaClusterConfig] = Field(
🧹 Nitpick comments (3)
holmes/plugins/toolsets/bash/bash_toolset.py (1)
14-26: Consider importingClassVarandTypefromtypingmodule for consistency with Python conventions.While
ClassVarandTypeare currently re-exported fromholmes.core.tools(lines 17, 23), the standard practice in Python is to import these typing constructs directly from thetypingmodule. This improves code clarity and aligns with type-checking best practices.Suggested fix
-from typing import Any, Dict, Optional +from typing import Any, ClassVar, Dict, Optional, Type ... -from holmes.core.tools import ( - ApprovalRequirement, - CallablePrerequisite, - ClassVar, - StructuredToolResult, - StructuredToolResultStatus, - Tool, - ToolInvokeContext, - ToolParameter, - Toolset, - ToolsetTag, - Type, -) +from holmes.core.tools import ( + ApprovalRequirement, + CallablePrerequisite, + StructuredToolResult, + StructuredToolResultStatus, + Tool, + ToolInvokeContext, + ToolParameter, + Toolset, + ToolsetTag, +)Also applies to: line 234
holmes/plugins/toolsets/elasticsearch/elasticsearch.py (1)
70-70: Consider usingListfrom typing for consistency.The
config_classesattribute uses lowercaselist[Type[...]]syntax. While this is valid in Python 3.9+, other toolsets and the baseToolsetclass useList[Type[BaseModel]]from typing. Consider aligning for consistency across the codebase.♻️ Suggested change for consistency
- config_classes: ClassVar[list[Type[ElasticsearchConfig]]] = [ElasticsearchConfig] + config_classes: ClassVar[List[Type[ElasticsearchConfig]]] = [ElasticsearchConfig]holmes/plugins/toolsets/internet/internet.py (1)
233-238: Minor: Missing blank line between class definitions.There's no blank line between
InternetBaseToolsetConfigandInternetBaseToolsetclass definitions. PEP 8 recommends two blank lines between top-level class definitions for readability.♻️ Add blank line
], ) + + class InternetBaseToolset(Toolset):
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/bash/bash_toolset.py (1)
43-87: Critical: Unresolved merge conflict markers will break the file.The file contains unresolved merge conflict markers (
<<<<<<< HEAD,=======,>>>>>>> master) that will cause Python syntax errors. The static analysis tool also flagged this with "Expected a statement" errors.This must be resolved before the PR can be merged. Based on the context, it appears the
HEADside had no content for the docstring Args section, whilemasterincludes the full docstring. You likely want to keep themasterversion:🐛 Fix: Remove merge conflict markers
""" Convert a BashResult to a StructuredToolResult. -<<<<<<< HEAD -======= Args: result: The BashResult from execute_bash_command cmd: The original command (for error messages) timeout: The timeout value (for error messages) params: Parameters to include in the result Returns: StructuredToolResult suitable for the tool response """ if result.timed_out: # ... rest of function ->>>>>>> master
🧹 Nitpick comments (2)
holmes/core/tools.py (1)
836-856: Inconsistent handling of multiple config classes.
get_config_schema()returns schemas for all config classes, butget_config_example()only returns the example for the first config class. Given the commit message mentions "support multiple config classes per toolset," consider returning examples for all config classes for consistency:♻️ Suggested refactor for consistent multi-config handling
def get_config_example(self) -> Optional[Dict[str, Any]]: - """Returns a JSON-serializable example object for the toolset's configuration. + """Returns JSON-serializable example objects for the toolset's configuration. - Returns the example of the first config class (if any), otherwise returns None. + Returns a dict of { config_class_name: example } (if any), otherwise returns None. """ if self.config_classes: - return build_config_example(self.config_classes[0]) + return { + config_cls.__name__: build_config_example(config_cls) + for config_cls in self.config_classes + } return None -Alternatively, if only returning the first example is intentional, update the docstring to explain the rationale.
holmes/plugins/toolsets/bash/bash_toolset.py (1)
12-24: Consider importingClassVarandTypefromtypingdirectly.Importing
ClassVarandTypefromholmes.core.toolsworks (since they're imported at module level there), but it's non-idiomatic. These typing constructs should be imported from their canonical source:♻️ Suggested change
import logging import os -from typing import Any, Dict, Optional +from typing import Any, ClassVar, Dict, Optional, Type from holmes.core.tools import ( ApprovalRequirement, CallablePrerequisite, - ClassVar, StructuredToolResult, StructuredToolResultStatus, Tool, ToolInvokeContext, ToolParameter, Toolset, ToolsetTag, - Type, )
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@holmes/plugins/toolsets/internet/internet.py`:
- Around line 263-265: prerequisites_callable currently instantiates
InternetBaseToolsetConfig(**config) without handling pydantic.ValidationError
which can crash initialization; wrap that construction in a try/except that
catches pydantic.ValidationError (or Exception if ValidationError is not
imported), and on exception set self.config to None (or leave unchanged), and
return (False, "<clear validation error message>") where the message includes
the validation error details; update the function InternetBaseToolsetConfig
usage in prerequisites_callable to return (True, "") only when config validation
succeeds.
In `@tests/test_holmes_sync_toolsets.py`:
- Line 1: Remove the unused import from tests/test_holmes_sync_toolsets.py by
deleting the line that imports the json module (the "import json" statement)
since it's not referenced elsewhere in the file; ensure no other code depends on
json before removing.
♻️ Duplicate comments (1)
holmes/plugins/toolsets/internet/internet.py (1)
194-196: Guard againstNoneconfig when deriving headers.If
_invokeis called beforeprerequisites_callable,self.toolset.configcan beNone, which raisesAttributeError. Add a safe guard.🐛 Proposed fix
- additional_headers = ( - self.toolset.config.additional_headers if self.toolset.config.additional_headers else {} - ) + config = self.toolset.config + additional_headers = ( + config.additional_headers if config and config.additional_headers else {} + )
🧹 Nitpick comments (1)
tests/test_holmes_sync_toolsets.py (1)
92-113: Test assertions don't match test name and comment.The test is named
test_sync_toolsets_with_config_schemaand the comment states "should have null schema", but the assertion only verifiesinstallation_instructions is not None. Consider parsing the JSON structure and verifying theconfig_schemafield is actually null for toolsets withoutconfig_classes.Proposed enhancement to verify config_schema
mock_dal.sync_toolsets.assert_called_once() toolset_data = mock_dal.sync_toolsets.call_args[0][0][0] - assert toolset_data["installation_instructions"] is not None + assert toolset_data["installation_instructions"] is not None + + # Verify the config_schema is null for toolsets without config_classes + instructions_json = json.loads(toolset_data["installation_instructions"]) + assert instructions_json.get("config_schema") is NoneNote: If you apply this fix, the
jsonimport on line 1 would then be needed.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@holmes/plugins/toolsets/internet/internet.py`:
- Around line 263-267: In prerequisites_callable, don't catch a broad Exception
when instantiating InternetBaseToolsetConfig; import and catch
pydantic.ValidationError specifically and preserve other exceptions to surface;
update the except clause in prerequisites_callable to except ValidationError as
e and return the same failure message (using e) so only validation failures are
handled while other errors propagate.
In `@holmes/utils/holmes_sync_toolsets.py`:
- Around line 71-73: The current check skips empty example configs because it
tests truthiness of example_config; change the guard around
toolset.get_config_example() to explicitly test for None (e.g., use "if
example_config is not None:") so empty mappings like {} are preserved and still
assigned via context["example_config"] = yaml.dump(example_config); update the
code path referencing example_config, toolset.get_config_example(), and
context["example_config"] accordingly.
♻️ Duplicate comments (2)
holmes/utils/holmes_sync_toolsets.py (1)
48-58: Wrap installation_instructions as structured JSON (instructions + schema + example).
Right now this block still stores only the rendered markdown, so the frontend can’t reliably parseconfig_schema/example_configas promised by the PR objectives.🐛 Suggested direction
+import json + +def _json_serializer(obj: Any) -> Any: + if hasattr(obj, "isoformat"): + return obj.isoformat() + raise TypeError(f"Object of type {type(obj)} is not JSON serializable") ... - if not toolset.installation_instructions: - instructions = render_default_installation_instructions_for_toolset(toolset) - toolset.installation_instructions = instructions + if not toolset.installation_instructions: + toolset.installation_instructions = render_default_installation_instructions_for_toolset(toolset) + + payload = { + "instructions": toolset.installation_instructions, + "config_schema": toolset.get_config_schema(), + "example_config": toolset.get_config_example(), + } + toolset.installation_instructions = json.dumps(payload, default=_json_serializer)holmes/plugins/toolsets/internet/internet.py (1)
194-196: Guard againstself.toolset.configbeingNone.Line 194 currently dereferences
self.toolset.config.additional_headerswithout a null check; this can raiseAttributeErrorif prerequisites haven’t run or failed.🐛 Suggested fix
- additional_headers = ( - self.toolset.config.additional_headers if self.toolset.config.additional_headers else {} - ) + config = self.toolset.config + additional_headers = ( + config.additional_headers if (config and config.additional_headers) else {} + )
🧹 Nitpick comments (1)
tests/test_holmes_sync_toolsets.py (1)
90-112: Strengthen test to validate JSON payload shape.
The test name says “with_config_schema” but it only checks non-null instructions. Consider parsing the payload and asserting the presence (and nullness) ofconfig_schema/example_configto lock in the frontend contract.
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/internet/notion.py (1)
51-54: Update header access to the newinternet_configfield.
InternetBaseToolsetno longer exposesadditional_headers, so this will raiseAttributeErrorat runtime. Useinternet_configand guard forNone.🐛 Suggested fix
- additional_headers = ( - self.toolset.additional_headers if self.toolset.additional_headers else {} - ) + internet_config = self.toolset.internet_config + additional_headers = ( + internet_config.additional_headers + if internet_config and internet_config.additional_headers + else {} + )
♻️ Duplicate comments (2)
holmes/plugins/toolsets/internet/internet.py (2)
194-196: Guard againstinternet_configbeingNonebefore accessing headers.If prerequisites fail or aren’t called,
self.toolset.internet_configcan beNone, which will raiseAttributeErrorhere. Safer fallback avoids runtime failures.🐛 Suggested fix
- additional_headers = ( - self.toolset.internet_config.additional_headers if self.toolset.internet_config.additional_headers else {} - ) + internet_config = self.toolset.internet_config + additional_headers = ( + internet_config.additional_headers + if internet_config and internet_config.additional_headers + else {} + )
264-268: CatchValidationErrorinstead of broadException.
BaseModelvalidation failures raisepydantic.ValidationError; catchingExceptionmasks unexpected bugs and triggers Ruff BLE001.🧩 Suggested fix
-from pydantic import BaseModel, Field +from pydantic import BaseModel, Field, ValidationError @@ - try: - self.internet_config = InternetBaseToolsetConfig(**(config or {})) - except Exception as e: - return False, f"Failed to parse config: {e}" + try: + self.internet_config = InternetBaseToolsetConfig(**(config or {})) + except ValidationError as e: + return False, f"Failed to parse config: {e}"In Pydantic v2, does BaseModel(...) raise pydantic.ValidationError for invalid input, and is it recommended to catch ValidationError specifically instead of Exception?
7 participants
Wraps installation_instructions as JSON containing:instructions: The markdown installation instructionconfig_schema: JSON Schema for toolset configurationexample_config: Example configuration valuesThis enables the frontend to render a form-based UI for toolset configuration while maintaining backwards compatibility.Changes:
Update holmes_sync_toolsets to wrap instructions as JSONSummary by CodeRabbit
New Features
Documentation
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.