Repository navigation
[ROB-3577] Multi instance toolset base - #2114
Closed
Avi-Robusta wants to merge 12 commits into
Closed
Avi-Robusta wants to merge 12 commits into
Avi-Robusta wants to merge 12 commits into
Conversation
Slack's MCP token endpoint advertises client_secret_post only and returns
HTTP 200 with {"ok": false, "error": ...} on auth failure. The existing
Basic-Auth-then-body fallback in exchange_code_for_tokens only retried on
non-success status, so the body retry never fired and exchanges failed
with "Response missing 'access_token'".
Retry with client_secret in the POST body whenever the first response
lacks an access_token, not just when the status is non-2xx.
Also surface OAuth config drift on MCP tool-load failures: if the cached
token's client_id/token_url no longer match the toolset's config (or, for
servers like Slack that gate per workspace, the token was issued in a
different workspace), log a hint pointing to re-authentication.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: avi@robusta.dev <avi@robusta.dev>
…fix-oauth-client-secret-post-200 Signed-off-by: avi@robusta.dev <avi@robusta.dev>
…tadog
Introduce holmes/plugins/toolsets/multi_instance.py: a composition wrapper that
makes any single-instance toolset multi-instance without changing the toolset.
Wrap a toolset at registration with `multi_instance(ToolsetClass)` and it:
- accepts the child's flat config OR `{<globals>, instances: [...]}`
- builds one child toolset per instance, running each child's own
prerequisites_callable (real validation + health) on a per-instance flat config
(top-level globals merged in; auth-as-a-unit and mTLS-as-a-pair preserved)
- exposes the union of children's tools as routing proxies that strip the generic
`instance` param and delegate to the selected child's tool (approval, coercion
and transformers run on the child untouched)
- adds a `<name>_list_instances` tool only when >1 instance is configured
- aggregates health tolerantly (loads if any instance is reachable)
Converting a toolset is one line at registration and the toolset file is
unchanged. Convert ServiceNow and the four Datadog toolsets as the first
examples.
Backwards compatible: a flat config becomes a single `default` instance with no
`instance` param and no list tool. Tests exercise the wrapped path end-to-end
(routed calls asserted on the wire), not directly-constructed toolsets.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: avi@robusta.dev <avi@robusta.dev>
Wrap both toolsets with `multi_instance(...)` at registration (toolset files unchanged from master). Add `domain` to the list-instances identifying fields so Coralogix (no api_url) reports a useful summary. Each gets the standard 3-part test through the actual wrapper: flat config is backwards compatible (no instance param / list tool); an `instances:` config exposes the `instance` param + `<name>_list_instances`; and a routed call for each instance is asserted on the wire (VictoriaLogs: bearer vs no-auth per host; Coralogix: per-instance domain + Bearer api_key). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
Wrap MongoDBAtlasToolset with `multi_instance(...)` (toolset file unchanged). Instances differ by Atlas project + API keys (same host). The test asserts each child has its own isolated digest session (own keys, distinct Session objects — catching any credential cross-wiring) and that a routed call targets the selected project on the wire. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
Wrap ConfluenceToolset with `multi_instance(...)` (toolset file unchanged). Each instance builds its own internal HTTP toolset bound to that Confluence server, so routing a `confluence_request` to an instance hits THAT server with THAT instance's Bearer PAT — asserted on the wire for both Data-Center PAT instances. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
Wrap PrometheusToolset with `multi_instance(...)` (toolset file unchanged). Instances differ by prometheus_url + additional_headers; a routed query for each instance hits THAT Prometheus with THAT instance's headers (asserted on the wire, e.g. per-instance X-Scope-OrgID). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
Wrap AzureSQLToolset with `multi_instance(...)` (toolset file unchanged). Uses the Azure SDK, so the test patches the credential + API client and asserts per-instance isolation (each child builds its own ClientSecretCredential, AzureSQLAPIClient with its own subscription, and database config) and that a routed call goes through the selected instance's API client querying that instance's subscription/server. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
… instead Elasticsearch (#2087) and Grafana dashboards (#2080) previously grew their own bespoke multi-instance implementations (per-toolset `instances` config, `_get_instance`, `elasticsearch_instance`/`grafana_instance` params, list tools). Revert those toolsets to plain single-instance and make all of them multi-instance the same way as every other toolset — by wrapping at registration: multi_instance(ElasticsearchDataToolset) multi_instance(ElasticsearchClusterToolset) multi_instance(GrafanaToolset) # dashboards multi_instance(GrafanaLokiToolset) multi_instance(GrafanaTempoToolset) The toolset files are now single-instance (identical to pre-#2087/#2080); all multi-instance behavior lives in the shared wrapper. New tests verify, through the wrapper, that flat configs stay backwards compatible and that routing to each instance hits THAT instance's endpoint with its own credentials on the wire (ES: per-instance _cluster/health + ApiKey; Grafana dashboards/loki/tempo: per-instance host + Bearer). The hand-rolled multi-instance tests/docs are removed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
Wrap NewRelicToolset with `multi_instance(...)` (toolset file unchanged). Each instance carries its own API key + account; `enable_multi_account` stays an in-instance feature. A routed NRQL query for each instance uses that instance's Api-Key header and account id (asserted on the wire). OpenSearch query-assist is intentionally NOT wrapped: it has no connection config and makes no API call (it only generates PPL query strings, gated by the OPENSEARCH_URL env var), so multi-instance doesn't apply. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
Add a 'multi-instance' SuperFence (docs/custom_fences.py) that renders the standard Multiple Instances section for a toolset from a one-line import: the fence body supplies the toolset key + a single-instance config example, and it emits the instances: example, the auto-injected 'instance' param and '<toolset>_list_instances' tool note, and a link to the central page. Add the central docs/data-sources/multi-instance-toolsets.md page (config shape, shared defaults, instance param, discovery tool, health, backwards compat, supported-toolset list) and register it in the nav. Include the fence in all 16 converted toolset pages (Elasticsearch, Grafana dashboards/loki/tempo, Prometheus, Datadog, Coralogix, VictoriaLogs, New Relic, Azure SQL, MongoDB Atlas, ServiceNow, Confluence). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
When Grafana was reduced to single-instance for the generic multi_instance wrapper, master's HTTP basic-auth support was dropped — only api_key (Bearer) remained, so username/password configs silently sent no auth and got 401. Port basic auth from master onto the single-instance GrafanaConfig: - add username/password fields + validator (api_key XOR basic; user/pass together) - add build_auth() -> HTTPBasicAuth helper - thread auth= through dashboards (toolset_grafana), loki (loki_api) and tempo (grafana_tempo_api) request paths The wrapper's atomic auth group already merges a top-level username/password into each instance (or lets a per-instance api_key win). Adds basic-auth wire tests; updates tempo_api assertions for the new auth= kwarg. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
Contributor
WalkthroughThis PR unifies multi-instance support across HolmesGPT's toolset ecosystem by introducing a generic ChangesMulti-Instance Architecture
OAuth Token Exchange & Diagnostics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
|
Contributor
✅ Results of HolmesGPT evalsAutomatically triggered by commit 904e97e on branch Results of HolmesGPT evals
Benchmark Comparison DetailsMaster baseline: latest master-* experiment (post-merge regression eval)
Benchmark baseline: latest ci-benchmark experiment on master
Time comparison (seconds):
Cost comparison:
Total tokens comparison:
Cached tokens comparison:
Turns comparison:
Tool calls comparison:
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" Option 3: Add PR labels to include extra evals (applies to both automatic runs and
Examples: 🏷️ Valid tags
🤖 Valid models
Commands: CLI: |
Contributor
|
✅ Docker images ready for
Use these tags to pull the images for testing. 📋 Copy commandsgcloud auth configure-docker us-central1-docker.pkg.dev
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:6dc49b8c
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:6dc49b8c me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:6dc49b8c
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:6dc49b8c
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:6dc49b8c
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:6dc49b8c me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:6dc49b8c
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:6dc49b8cPatch 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:6dc49b8c \
--set operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set operator.image=holmes-operator-dev:6dc49b8cRobusta 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:6dc49b8c \
--set holmes.operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.operator.image=holmes-operator-dev:6dc49b8c |
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/plugins/toolsets/grafana/test_grafana_tempo_api.py (1)
44-60: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd one test for the non-
Noneauth path.These updates only cover the API-key case where
build_auth(...)returnsNone. The newauth=wiring inGrafanaTempoAPIis still untested for username/password configs, so a regression there would pass this suite.As per coding guidelines, "All new Python features require unit tests."
Also applies to: 84-91, 104-111, 141-148, 174-181, 209-216, 245-252, 276-283, 313-320, 338-345, 387-394
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/plugins/toolsets/grafana/test_grafana_tempo_api.py` around lines 44 - 60, Add tests that exercise the non-None auth path by simulating build_auth returning credentials and verifying those credentials are passed through to requests.get; specifically, for GrafanaTempoAPI.query_echo_endpoint (and the other similar API methods listed) patch requests.get, set api.build_auth (or the module-level build_auth used by GrafanaTempoAPI) to return a non-None auth tuple (e.g. ("user","pass")), call the API method, assert the method returns the expected result, and assert mock_get.assert_called_once_with includes auth=("user","pass") instead of None; replicate this pattern for the other test blocks referenced so each API method is covered for both None and non-None auth cases.holmes/plugins/toolsets/grafana/common.py (1)
145-150:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBasic auth is blocked for the Grafana proxy/cloud config variants
GrafanaConfigalready supportsapi_keyorusername+passwordvia_validate_grafana_auth()+build_auth(), but the proxy/cloud subclasses still overrideapi_keyas a requiredstr(no default), sousername/password-only configs fail model validation beforebuild_auth()is reached:
GrafanaLokiProxyConfig(holmes/plugins/toolsets/grafana/common.py~145-150)GrafanaCloudLokiConfig(~205-210)GrafanaTempoProxyConfig(~267-272)GrafanaCloudTempoConfig(~327-332)Suggested direction:
Make overridden `api_key` optional (same as base) so basic auth can work
-class GrafanaLokiProxyConfig(GrafanaConfig): +class GrafanaLokiProxyConfig(GrafanaConfig): @@ - api_key: str = Field( # type: ignore[assignment] + api_key: Optional[str] = Field( # type: ignore[assignment] + default=None, title="API Key", - description="Grafana service account token with Viewer role", + description="Grafana service account token with Viewer role (or use username/password)", examples=["{{ env.GRAFANA_API_KEY }}"], json_schema_extra={"format": "password"}, )Apply the same change to the other three proxy/cloud classes so the proxy/cloud variants align with the basic-auth contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@holmes/plugins/toolsets/grafana/common.py` around lines 145 - 150, The proxy/cloud subclasses (GrafanaLokiProxyConfig, GrafanaCloudLokiConfig, GrafanaTempoProxyConfig, GrafanaCloudTempoConfig) override api_key as a required str which blocks basic auth; change each overridden api_key to be optional (Optional[str]) with a default of None (using Field(default=None, ... ) and keeping the existing title/description/json_schema_extra) so the base GrafanaConfig's _validate_grafana_auth() and build_auth() can accept username/password-only configs.
🧹 Nitpick comments (1)
docs/data-sources/builtin-toolsets/confluence.md (1)
317-324: ⚡ Quick winInclude
subtypein the multi-instance example.This snippet is the copy-pasteable Confluence config for the multi-instance docs, but it currently relies on inference even though this page defines
subtypeas the switch that fixes auth mode and API path prefix. Addsubtype: cloudhere so the example is deterministic.♻️ Proposed doc fix
```multi-instance toolset: confluence name: Confluence +subtype: cloud config: | api_url: "https://yourcompany.atlassian.net" user: "your-email@example.com" api_key: "your-api-token"</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docs/data-sources/builtin-toolsets/confluence.mdaround lines 317 - 324, The
multi-instance Confluence example is missing the subtype field so it doesn't
deterministically select cloud auth/path behavior; update the example
multi-instance block for toolset: confluence to include "subtype: cloud" (i.e.,
add the subtype key alongside toolset: confluence, name: Confluence) so the
snippet explicitly sets cloud mode and fixes auth and API path prefix.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.Inline comments:
In@docs/custom_fences.py:
- Line 224: The constant MULTI_INSTANCE_DOC_URL is an absolute path
("/data-sources/multi-instance-toolsets/") which breaks versioned docs; change
it to a relative path by removing the leading slash (e.g.
"data-sources/multi-instance-toolsets/") so any doc path prefix/version is
preserved when this constant is used to build links; update any code that
consumes MULTI_INSTANCE_DOC_URL to join paths without forcing a root-leading
slash.- Around line 261-265: multi_instance_fence_format currently converts
spec.get("config") to a Python string which turns mappings into invalid
Python-dict syntax; instead detect if spec.get("config") is not already a string
and serialize it to valid YAML (e.g. with yaml.safe_dump) before
trimming—replace config = str(spec.get("config", "")).strip() with logic that if
isinstance(spec.get("config"), str) use it, else set config =
yaml.safe_dump(spec.get("config"), default_flow_style=False).strip() (falling
back to "" when None/empty) so the generated snippet contains valid YAML; update
usages of config in the function accordingly.In
@docs/data-sources/builtin-toolsets/servicenow.md:
- Around line 191-197: Update the YAML example for the servicenow/tables toolset
so the placeholder values are quoted: wrap the api_url and api_key placeholders
in double quotes (i.e., change api_url: and
api_key: to api_url: ""
and api_key: "") so the embedded YAML block (toolset:
servicenow/tables, name: ServiceNow, config: |) is copy-pasteable and consistent
with other docs.In
@holmes/plugins/toolsets/elasticsearch/elasticsearch.py:
- Around line 134-178: The current _perform_health_check always queries
"_cluster/health" which forces cluster-level permissions; add a data-toolset
specific probe and use it for ElasticsearchDataToolset instead of the cluster
probe. Implement a new method (e.g., _perform_index_health_check) that does a
lightweight index-level read such as calling self._make_request("GET",
"_cat/indices?format=json", timeout=10) or a size=0 search on a known index,
handle 401/403/timeout/connection like the existing method, and return a similar
(bool, msg) tuple; then override/route the health-check used during registration
in ElasticsearchDataToolset to call _perform_index_health_check (or call it from
within its own _perform_health_check override) so users with only index-level
read permissions can register the data toolset.In
@holmes/plugins/toolsets/multi_instance.py:
- Around line 64-72: The auth fall-through can still leak top-level auth
discriminator fields into per-instance configs; update the merge logic by making
auth merging child-aware or by adding the discriminator fields (e.g.,
"auth_type", any wrapper-specific selector like "auth_method") to the
_ATOMIC_GROUPS set so that when an instance supplies any credential in the group
(e.g., "username"/"password", "api_key", "bearer_token",
"client_cert"/"client_key") the corresponding discriminator is also excluded
from inheritance; modify the code that uses _ATOMIC_GROUPS (refer to the
_ATOMIC_GROUPS symbol and the merge routine that applies these groups) to ensure
the discriminator fields are treated as part of the atomic unit and thus not
inherited into instance-level configs.- Around line 279-293: The wrapper currently only runs the first
CallablePrerequisite in _run_child_prerequisites and thus can skip other types;
change it to delegate to the child’s full prerequisite evaluation instead of
picking one instance. Locate _run_child_prerequisites and either call the
child's existing prerequisite runner (e.g., child.run_prerequisites(flat_config)
or child.evaluate_prerequisites(flat_config)) to get the combined (ok, msg)
result, or if no such method exists, iterate child.prerequisites and execute
each prerequisite using its public API (e.g., calling .callable(...) for
CallablePrerequisite or the prerequisite's standard validation method),
short-circuiting on failure and aggregating messages so the wrapper preserves
the child Toolset's complete prerequisite semantics.In
@tests/plugins/toolsets/test_multi_instance.py:
- Around line 217-219: The test assigns an unused second value
msgfrom the
call to ts.prerequisites_callable, triggering an unused-variable lint; update
the call site in tests/plugins/toolsets/test_multi_instance.py so you only
capture the needed value (e.g., assign the second return to_or index the
first element) instead of bindingmsg, keeping the call to
ts.prerequisites_callable({"instances": [{"name": "a"}, {"name": "b"}]}) and
preserving the semantics of the test.- Around line 85-88: The helper _wrap currently calls
ts.prerequisites_callable(config) and discards the result; change it to capture
the result and assert setup succeeded before returning the toolset.
Specifically, inside _wrap (which constructs ts via
multi_instance(_FakeToolset)), assign result = ts.prerequisites_callable(config)
and assert that result is truthy (or not None/False) so any setup failure
surfaces at the helper boundary, then return ts.
Outside diff comments:
In@holmes/plugins/toolsets/grafana/common.py:
- Around line 145-150: The proxy/cloud subclasses (GrafanaLokiProxyConfig,
GrafanaCloudLokiConfig, GrafanaTempoProxyConfig, GrafanaCloudTempoConfig)
override api_key as a required str which blocks basic auth; change each
overridden api_key to be optional (Optional[str]) with a default of None (using
Field(default=None, ... ) and keeping the existing
title/description/json_schema_extra) so the base GrafanaConfig's
_validate_grafana_auth() and build_auth() can accept username/password-only
configs.In
@tests/plugins/toolsets/grafana/test_grafana_tempo_api.py:
- Around line 44-60: Add tests that exercise the non-None auth path by
simulating build_auth returning credentials and verifying those credentials are
passed through to requests.get; specifically, for
GrafanaTempoAPI.query_echo_endpoint (and the other similar API methods listed)
patch requests.get, set api.build_auth (or the module-level build_auth used by
GrafanaTempoAPI) to return a non-None auth tuple (e.g. ("user","pass")), call
the API method, assert the method returns the expected result, and assert
mock_get.assert_called_once_with includes auth=("user","pass") instead of None;
replicate this pattern for the other test blocks referenced so each API method
is covered for both None and non-None auth cases.
Nitpick comments:
In@docs/data-sources/builtin-toolsets/confluence.md:
- Around line 317-324: The multi-instance Confluence example is missing the
subtype field so it doesn't deterministically select cloud auth/path behavior;
update the example multi-instance block for toolset: confluence to include
"subtype: cloud" (i.e., add the subtype key alongside toolset: confluence, name:
Confluence) so the snippet explicitly sets cloud mode and fixes auth and API
path prefix.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `f7fa0edf-4627-4d60-8bca-b85905914996` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 5dc109fbaa31d597436fd3efd5a051f65c26c450 and 904e97ea1244af29b11beefe685bf90c26d92e4d. </details> <details> <summary>📒 Files selected for processing (45)</summary> * `docs/custom_fences.py` * `docs/data-sources/.nav.yml` * `docs/data-sources/builtin-toolsets/azure-sql.md` * `docs/data-sources/builtin-toolsets/confluence.md` * `docs/data-sources/builtin-toolsets/coralogix-logs.md` * `docs/data-sources/builtin-toolsets/datadog.md` * `docs/data-sources/builtin-toolsets/elasticsearch.md` * `docs/data-sources/builtin-toolsets/grafanadashboards.md` * `docs/data-sources/builtin-toolsets/grafanaloki.md` * `docs/data-sources/builtin-toolsets/grafanatempo.md` * `docs/data-sources/builtin-toolsets/mongodb-atlas.md` * `docs/data-sources/builtin-toolsets/newrelic.md` * `docs/data-sources/builtin-toolsets/prometheus.md` * `docs/data-sources/builtin-toolsets/servicenow.md` * `docs/data-sources/builtin-toolsets/victorialogs.md` * `docs/data-sources/multi-instance-toolsets.md` * `holmes/core/oauth_config.py` * `holmes/core/tools_utils/oauth_tool_connector.py` * `holmes/plugins/toolsets/__init__.py` * `holmes/plugins/toolsets/elasticsearch/elasticsearch.py` * `holmes/plugins/toolsets/grafana/base_grafana_toolset.py` * `holmes/plugins/toolsets/grafana/common.py` * `holmes/plugins/toolsets/grafana/grafana_tempo_api.py` * `holmes/plugins/toolsets/grafana/loki/toolset_grafana_loki.py` * `holmes/plugins/toolsets/grafana/loki_api.py` * `holmes/plugins/toolsets/grafana/toolset_grafana.py` * `holmes/plugins/toolsets/multi_instance.py` * `mkdocs.yml` * `tests/plugins/toolsets/azure_sql/test_azure_sql_multi_instance.py` * `tests/plugins/toolsets/datadog/test_datadog_multi_instance.py` * `tests/plugins/toolsets/elasticsearch/test_elasticsearch_multi_instance.py` * `tests/plugins/toolsets/grafana/test_grafana_multi_instance.py` * `tests/plugins/toolsets/grafana/test_grafana_tempo_api.py` * `tests/plugins/toolsets/newrelic/test_newrelic_multi_instance.py` * `tests/plugins/toolsets/test_confluence_multi_instance.py` * `tests/plugins/toolsets/test_coralogix_multi_instance.py` * `tests/plugins/toolsets/test_elasticsearch_mtls.py` * `tests/plugins/toolsets/test_json_filter_mixin.py` * `tests/plugins/toolsets/test_mongodb_atlas_multi_instance.py` * `tests/plugins/toolsets/test_multi_instance.py` * `tests/plugins/toolsets/test_prometheus_multi_instance.py` * `tests/plugins/toolsets/test_servicenow_multi_instance.py` * `tests/plugins/toolsets/test_verify_tool_urls.py` * `tests/plugins/toolsets/test_victorialogs_multi_instance.py` * `tests/test_mcp_oauth.py` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
|
|
||
| # Central page that documents how multi-instance toolsets work. Linked from every | ||
| # rendered ``multi-instance`` block so each toolset page doesn't repeat the prose. | ||
| MULTI_INSTANCE_DOC_URL = "/data-sources/multi-instance-toolsets/" |
Contributor
There was a problem hiding this comment.
Make this link version-relative.
Hard-coding /data-sources/multi-instance-toolsets/ drops the current docs path prefix, so pages rendered under a versioned docs URL will jump to the site root/latest instead of staying on the same version.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/custom_fences.py` at line 224, The constant MULTI_INSTANCE_DOC_URL is an
absolute path ("/data-sources/multi-instance-toolsets/") which breaks versioned
docs; change it to a relative path by removing the leading slash (e.g.
"data-sources/multi-instance-toolsets/") so any doc path prefix/version is
preserved when this constant is used to build links; update any code that
consumes MULTI_INSTANCE_DOC_URL to join paths without forcing a root-leading
slash.
Comment on lines
+261
to
+265
| spec = yaml.safe_load(source) or {} | ||
| toolset = str(spec.get("toolset", "")).strip() | ||
| name = str(spec.get("name") or toolset or "this").strip() | ||
| config = str(spec.get("config", "")).strip() | ||
| if not toolset or not config: |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import yaml
source = """
toolset: grafana/dashboards
config:
api_url: https://prod-grafana.example.com
api_key: token
"""
spec = yaml.safe_load(source)
print(type(spec).__name__)
print(type(spec["config"]).__name__)
print(str(spec["config"]))
PYRepository: HolmesGPT/holmesgpt
Length of output: 141
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant section around the reported line numbers
nl -ba docs/custom_fences.py | sed -n '230,310p'
# Find where _reindent is defined/used and whether config is inserted into a YAML snippet
rg -n "_reindent\(" -S docs/custom_fences.py || true
rg -n "config" docs/custom_fences.py | head -n 80Repository: HolmesGPT/holmesgpt
Length of output: 106
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Display the referenced section with line numbers (using cat -n instead of nl)
cat -n docs/custom_fences.py | sed -n '230,310p'
# Trace how config is used/rendered
rg -n "config" docs/custom_fences.py | head -n 200
# Trace for reindent helper usage
rg -n "_reindent\(" docs/custom_fences.py || true
rg -n "def _reindent" -n docs/custom_fences.py || trueRepository: HolmesGPT/holmesgpt
Length of output: 6112
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import yaml
def _reindent(text: str, spaces: int) -> str:
lines = text.strip("\n").split("\n")
nonempty = [ln for ln in lines if ln.strip()]
base = min((len(ln) - len(ln.lstrip()) for ln in nonempty), default=0)
pad = " " * spaces
return "\n".join(pad + ln[base:] if ln.strip() else "" for ln in lines)
def render(source: str):
spec = yaml.safe_load(source) or {}
toolset = str(spec.get("toolset", "")).strip()
config = str(spec.get("config", "")).strip()
fields = _reindent(config, 10)
yaml_example = (
"toolsets:\n"
f" {toolset}:\n"
" enabled: true\n"
" config:\n"
" instances:\n"
f" - name: prod\n{fields}\n"
f" - name: staging\n{fields}\n"
)
return yaml_example
# config provided as a YAML mapping (not a block scalar)
source = """
toolset: grafana/dashboards
config:
api_url: https://prod-grafana.example.com
api_key: token
"""
y = render(source)
print("=== rendered ===")
print(y)
print("=== safe_load result ===")
try:
obj = yaml.safe_load(y)
print("parsed:", type(obj).__name__)
except Exception as e:
print("parse_error:", type(e).__name__, str(e)[:200])
PYRepository: HolmesGPT/holmesgpt
Length of output: 613
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import yaml
def _reindent(text: str, spaces: int) -> str:
lines = text.strip("\n").split("\n")
nonempty = [ln for ln in lines if ln.strip()]
base = min((len(ln) - len(ln.lstrip()) for ln in nonempty), default=0)
pad = " " * spaces
return "\n".join(pad + ln[base:] if ln.strip() else "" for ln in lines)
def render_with_fix(source: str):
spec = yaml.safe_load(source) or {}
if not isinstance(spec, dict):
raise ValueError("multi-instance fence body must be a YAML mapping")
toolset = str(spec.get("toolset", "")).strip()
name = str(spec.get("name") or toolset or "this").strip()
raw_config = spec.get("config", "")
if isinstance(raw_config, str):
config = raw_config.strip()
elif raw_config:
config = yaml.safe_dump(raw_config, sort_keys=False).rstrip()
else:
config = ""
if not toolset or not config:
raise ValueError("multi-instance fence requires 'toolset' and 'config' keys in its YAML body")
fields = _reindent(config, 10)
yaml_example = (
"toolsets:\n"
f" {toolset}:\n"
" enabled: true\n"
" config:\n"
" instances:\n"
f" - name: prod\n{fields}\n"
f" - name: staging\n{fields}\n"
)
return yaml_example
source = """
toolset: grafana/dashboards
config:
api_url: https://prod-grafana.example.com
api_key: token
"""
rendered = render_with_fix(source)
print(rendered)
yaml.safe_load(rendered)
print("safe_load: OK")
PYRepository: HolmesGPT/holmesgpt
Length of output: 356
Fix invalid YAML when config is a mapping in docs/custom_fences.py.
multi_instance_fence_format() parses the fence body with yaml.safe_load(), but then does config = str(spec.get("config", "")).strip(). When config is a YAML mapping, str(dict) renders Python dict syntax (e.g. {'api_url': ...}), producing invalid YAML in the generated snippet (lines 261-265).
Possible fix
- spec = yaml.safe_load(source) or {}
- toolset = str(spec.get("toolset", "")).strip()
- name = str(spec.get("name") or toolset or "this").strip()
- config = str(spec.get("config", "")).strip()
+ spec = yaml.safe_load(source) or {}
+ if not isinstance(spec, dict):
+ raise ValueError("multi-instance fence body must be a YAML mapping")
+
+ toolset = str(spec.get("toolset", "")).strip()
+ name = str(spec.get("name") or toolset or "this").strip()
+
+ raw_config = spec.get("config", "")
+ if isinstance(raw_config, str):
+ config = raw_config.strip()
+ elif raw_config:
+ config = yaml.safe_dump(raw_config, sort_keys=False).rstrip()
+ else:
+ config = ""🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/custom_fences.py` around lines 261 - 265, multi_instance_fence_format
currently converts spec.get("config") to a Python string which turns mappings
into invalid Python-dict syntax; instead detect if spec.get("config") is not
already a string and serialize it to valid YAML (e.g. with yaml.safe_dump)
before trimming—replace config = str(spec.get("config", "")).strip() with logic
that if isinstance(spec.get("config"), str) use it, else set config =
yaml.safe_dump(spec.get("config"), default_flow_style=False).strip() (falling
back to "" when None/empty) so the generated snippet contains valid YAML; update
usages of config in the function accordingly.
Comment on lines
+191
to
+197
| ```multi-instance | ||
| toolset: servicenow/tables | ||
| name: ServiceNow | ||
| config: | | ||
| api_url: <your servicenow instance URL> | ||
| api_key: <your servicenow API key> | ||
| ``` |
Contributor
There was a problem hiding this comment.
Quote the placeholder values in this YAML example.
As written, the embedded YAML is not copy-pasteable because the placeholder values contain spaces. Please quote them here like the other docs examples do.
Suggested fix
```multi-instance
toolset: servicenow/tables
name: ServiceNow
config: |
- api_url: <your servicenow instance URL>
- api_key: <your servicenow API key>
+ api_url: "<your servicenow instance URL>"
+ api_key: "<your servicenow API key>"</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/data-sources/builtin-toolsets/servicenow.md` around lines 191 - 197,
Update the YAML example for the servicenow/tables toolset so the placeholder
values are quoted: wrap the api_url and api_key placeholders in double quotes
(i.e., change api_url: <your servicenow instance URL> and api_key: <your
servicenow API key> to api_url: "<your servicenow instance URL>" and api_key:
"<your servicenow API key>") so the embedded YAML block (toolset:
servicenow/tables, name: ServiceNow, config: |) is copy-pasteable and consistent
with other docs.
Comment on lines
134
to
+178
| def _perform_health_check(self) -> Tuple[bool, str]: | ||
| """Probe `_cluster/health` on each configured instance. | ||
|
|
||
| Tolerant: succeeds as long as at least one instance is reachable; the | ||
| toolset still loads with the healthy ones. Each failure is captured | ||
| with the instance name, status code, and response body so the LLM (and | ||
| any human reading the status string) can self-correct. | ||
| """ | ||
| failures: List[str] = [] | ||
| successes: List[str] = [] | ||
| for instance in self._instances.values(): | ||
| ok, msg = self._health_check_instance(instance) | ||
| if ok: | ||
| successes.append(msg) | ||
| else: | ||
| failures.append(msg) | ||
| return self._aggregate_health_results(failures, successes) | ||
|
|
||
| def _health_check_instance( | ||
| self, instance: ElasticsearchInstance | ||
| ) -> Tuple[bool, str]: | ||
| """Perform a health check by querying cluster health.""" | ||
| try: | ||
| data = self._make_request(instance, "GET", "_cluster/health", timeout=10) | ||
| cluster_name = data.get("cluster_name", "unknown") | ||
| status = data.get("status", "unknown") | ||
| response = self._make_request("GET", "_cluster/health", timeout=10) | ||
| cluster_name = response.get("cluster_name", "unknown") | ||
| status = response.get("status", "unknown") | ||
| return ( | ||
| True, | ||
| f"[{instance.name}] Connected to '{cluster_name}' (status: {status})", | ||
| f"Connected to Elasticsearch cluster '{cluster_name}' (status: {status})", | ||
| ) | ||
| except requests.exceptions.HTTPError as e: | ||
| status_code = e.response.status_code | ||
| body = e.response.text[:500] if e.response is not None else "" | ||
| if status_code == 401: | ||
| if e.response.status_code == 401: | ||
| return ( | ||
| False, | ||
| f"[{instance.name}] Authentication failed for {instance.api_url}. " | ||
| "Check api_key or username/password.", | ||
| "Elasticsearch authentication failed. Check your API key or credentials.", | ||
| ) | ||
| if status_code == 403: | ||
| elif e.response.status_code == 403: | ||
| return ( | ||
| False, | ||
| f"[{instance.name}] Access denied at {instance.api_url}. " | ||
| "Credentials lack cluster access.", | ||
| "Elasticsearch access denied. Ensure your credentials have cluster access.", | ||
| ) | ||
| else: | ||
| return ( | ||
| False, | ||
| f"Elasticsearch API error: {e.response.status_code} - {e.response.text}", | ||
| ) | ||
| return ( | ||
| False, | ||
| f"[{instance.name}] HTTP {status_code} from {instance.api_url}: {body}", | ||
| ) | ||
| except requests.exceptions.SSLError as e: | ||
| error_msg = str(e) | ||
| if ( | ||
| "certificate required" in error_msg.lower() | ||
| or "sslcertverificationerror" in error_msg.lower() | ||
| ): | ||
| if "certificate required" in error_msg.lower() or "sslcertverificationerror" in error_msg.lower(): | ||
| return ( | ||
| False, | ||
| f"[{instance.name}] SSL/TLS error at {instance.api_url}: {error_msg}. " | ||
| f"Elasticsearch SSL/TLS error: {error_msg}. " | ||
| "If the server requires mTLS, configure client_cert and client_key. " | ||
| "If using a private CA, set the CERTIFICATE env var (base64-encoded CA cert).", | ||
| ) | ||
| return False, f"[{instance.name}] SSL error at {instance.api_url}: {error_msg}" | ||
| except requests.exceptions.ConnectionError as e: | ||
| return False, f"Elasticsearch SSL error: {error_msg}" | ||
| except requests.exceptions.ConnectionError: | ||
| return ( | ||
| False, | ||
| f"[{instance.name}] Failed to connect to {instance.api_url}: {e}", | ||
| f"Failed to connect to Elasticsearch at {self.elasticsearch_config.api_url}", | ||
| ) | ||
| except requests.exceptions.Timeout: | ||
| return False, f"[{instance.name}] Health check timed out for {instance.api_url}" | ||
| return False, "Elasticsearch health check timed out" | ||
| except Exception as e: | ||
| return False, f"[{instance.name}] Health check failed: {str(e)}" | ||
|
|
||
| def _aggregate_health_results( | ||
| self, failures: List[str], successes: List[str] | ||
| ) -> Tuple[bool, str]: | ||
| """Tolerant aggregation: succeed if any instance is reachable. | ||
|
|
||
| Returns `(True, summary)` when at least one instance is healthy; the | ||
| summary lists healthy connections and notes any failures so they're | ||
| visible in the toolset status. Returns `(False, joined_errors)` only | ||
| when every instance failed. | ||
| """ | ||
| total = len(failures) + len(successes) | ||
| if not successes: | ||
| return False, "\n".join(failures) or "No Elasticsearch instances configured" | ||
| if failures: | ||
| logger.warning( | ||
| f"{self.name}: {len(successes)}/{total} instance(s) healthy. " | ||
| f"Failed: {failures}" | ||
| ) | ||
| return True, ( | ||
| "; ".join(successes) | ||
| + "; failed: " | ||
| + " | ".join(failures) | ||
| ) | ||
| return True, "; ".join(successes) | ||
|
|
||
| def _get_instance(self, params: Dict[str, Any]) -> ElasticsearchInstance: | ||
| """Resolve which Elasticsearch instance a tool call should target. | ||
|
|
||
| Auto-selects when only one is configured. Otherwise requires | ||
| `elasticsearch_instance` in params. Raises `ValueError` with a helpful | ||
| message listing the configured names when missing or unknown. | ||
| """ | ||
| configured = sorted(self._instances) | ||
| requested = params.get("elasticsearch_instance") | ||
| if not requested: | ||
| if len(self._instances) == 1: | ||
| return next(iter(self._instances.values())) | ||
| raise ValueError( | ||
| f"`elasticsearch_instance` is required (configured: {configured})" | ||
| ) | ||
| if requested not in self._instances: | ||
| raise ValueError( | ||
| f"Unknown elasticsearch_instance '{requested}'. Configured: {configured}" | ||
| ) | ||
| return self._instances[requested] | ||
| return False, f"Elasticsearch health check failed: {str(e)}" |
Contributor
There was a problem hiding this comment.
The shared health check now requires cluster access for the data toolset.
ElasticsearchDataToolset inherits this prerequisite, but _perform_health_check() always probes _cluster/health. That turns the data toolset into a cluster-permission tool during registration: a user with only index-level read access will get the 403 branch here and the toolset never enables, even though its actual search/mapping surface could still work. This needs a data-toolset-specific probe or an overridden prerequisite path.
🧰 Tools
🪛 Ruff (0.15.15)
[warning] 177-177: Do not catch blind exception: Exception
(BLE001)
[warning] 178-178: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@holmes/plugins/toolsets/elasticsearch/elasticsearch.py` around lines 134 -
178, The current _perform_health_check always queries "_cluster/health" which
forces cluster-level permissions; add a data-toolset specific probe and use it
for ElasticsearchDataToolset instead of the cluster probe. Implement a new
method (e.g., _perform_index_health_check) that does a lightweight index-level
read such as calling self._make_request("GET", "_cat/indices?format=json",
timeout=10) or a size=0 search on a known index, handle
401/403/timeout/connection like the existing method, and return a similar (bool,
msg) tuple; then override/route the health-check used during registration in
ElasticsearchDataToolset to call _perform_index_health_check (or call it from
within its own _perform_health_check override) so users with only index-level
read permissions can register the data toolset.
Comment on lines
+64
to
+72
| # Field groups inherited from the top-level globals as an atomic unit: if an | ||
| # instance sets ANY field in a group, it inherits NONE of that group. This | ||
| # reproduces the existing fall-through semantics generically — auth methods are | ||
| # mutually exclusive (api_key XOR basic XOR bearer) and mTLS is a cert/key pair — | ||
| # so a global default never gets cross-wired into an instance that picked another. | ||
| _ATOMIC_GROUPS: List[set] = [ | ||
| {"api_key", "username", "password", "bearer_token"}, | ||
| {"client_cert", "client_key"}, | ||
| ] |
Contributor
There was a problem hiding this comment.
Auth fall-through is not atomic for toolsets that use an auth discriminator.
This merge drops inherited credential fields, but it still leaks top-level auth selectors like auth_type into per-instance configs. That creates mixed configs for wrapped toolsets such as Confluence, e.g. an instance can override to basic auth via username/password and still inherit auth_type="bearer" from the parent. Please make auth merging child-aware, or include the discriminator fields in the atomic auth group so the wrapper actually preserves “auth-as-a-unit.”
Also applies to: 78-89
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@holmes/plugins/toolsets/multi_instance.py` around lines 64 - 72, The auth
fall-through can still leak top-level auth discriminator fields into
per-instance configs; update the merge logic by making auth merging child-aware
or by adding the discriminator fields (e.g., "auth_type", any wrapper-specific
selector like "auth_method") to the _ATOMIC_GROUPS set so that when an instance
supplies any credential in the group (e.g., "username"/"password", "api_key",
"bearer_token", "client_cert"/"client_key") the corresponding discriminator is
also excluded from inheritance; modify the code that uses _ATOMIC_GROUPS (refer
to the _ATOMIC_GROUPS symbol and the merge routine that applies these groups) to
ensure the discriminator fields are treated as part of the atomic unit and thus
not inherited into instance-level configs.
Comment on lines
+279
to
+293
| def _run_child_prerequisites( | ||
| self, child: Toolset, flat_config: Dict[str, Any] | ||
| ) -> Tuple[bool, str]: | ||
| """Run the child's own callable prerequisite (validation + health) on a flat config.""" | ||
| callable_prereq = next( | ||
| (p for p in child.prerequisites if isinstance(p, CallablePrerequisite)), None | ||
| ) | ||
| if callable_prereq is None: | ||
| child.config = flat_config | ||
| return True, "" | ||
| try: | ||
| ok, msg = callable_prereq.callable(flat_config) | ||
| return bool(ok), msg or "" | ||
| except Exception as e: | ||
| return False, str(e) |
Contributor
There was a problem hiding this comment.
Wrapped toolsets can bypass child prerequisites entirely.
_run_child_prerequisites() only executes the first CallablePrerequisite. If the child has a StaticPrerequisite, multiple prerequisites, or any non-callable gating, the wrapper reports success without honoring that contract. That changes the behavior of every toolset registered via multi_instance(...) from “use the child’s prerequisite logic” to “use only its first callable check.” Please delegate to the child’s full prerequisite evaluation instead of cherry-picking one prerequisite type.
🧰 Tools
🪛 Ruff (0.15.15)
[warning] 292-292: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@holmes/plugins/toolsets/multi_instance.py` around lines 279 - 293, The
wrapper currently only runs the first CallablePrerequisite in
_run_child_prerequisites and thus can skip other types; change it to delegate to
the child’s full prerequisite evaluation instead of picking one instance. Locate
_run_child_prerequisites and either call the child's existing prerequisite
runner (e.g., child.run_prerequisites(flat_config) or
child.evaluate_prerequisites(flat_config)) to get the combined (ok, msg) result,
or if no such method exists, iterate child.prerequisites and execute each
prerequisite using its public API (e.g., calling .callable(...) for
CallablePrerequisite or the prerequisite's standard validation method),
short-circuiting on failure and aggregating messages so the wrapper preserves
the child Toolset's complete prerequisite semantics.
Comment on lines
+85
to
+88
| def _wrap(config: dict): | ||
| ts = multi_instance(_FakeToolset) | ||
| ts.prerequisites_callable(config) | ||
| return ts |
Contributor
There was a problem hiding this comment.
Assert wrapper setup succeeds in the shared helper.
_wrap() drops the result of prerequisites_callable(), so any setup regression will fail later in unrelated assertions instead of at the setup boundary.
Suggested fix
def _wrap(config: dict):
ts = multi_instance(_FakeToolset)
- ts.prerequisites_callable(config)
+ ok, msg = ts.prerequisites_callable(config)
+ assert ok is True, msg
return ts🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/plugins/toolsets/test_multi_instance.py` around lines 85 - 88, The
helper _wrap currently calls ts.prerequisites_callable(config) and discards the
result; change it to capture the result and assert setup succeeded before
returning the toolset. Specifically, inside _wrap (which constructs ts via
multi_instance(_FakeToolset)), assign result = ts.prerequisites_callable(config)
and assert that result is truthy (or not None/False) so any setup failure
surfaces at the helper boundary, then return ts.
Comment on lines
+217
to
+219
| ok, msg = ts.prerequisites_callable( | ||
| {"instances": [{"name": "a"}, {"name": "b"}]} # no api_url -> both fail | ||
| ) |
Contributor
There was a problem hiding this comment.
Remove the unused msg binding.
This new test currently trips Ruff's unused-variable check.
Suggested fix
- ok, msg = ts.prerequisites_callable(
+ ok, _msg = ts.prerequisites_callable(
{"instances": [{"name": "a"}, {"name": "b"}]} # no api_url -> both fail
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ok, msg = ts.prerequisites_callable( | |
| {"instances": [{"name": "a"}, {"name": "b"}]} # no api_url -> both fail | |
| ) | |
| ok, _msg = ts.prerequisites_callable( | |
| {"instances": [{"name": "a"}, {"name": "b"}]} # no api_url -> both fail | |
| ) |
🧰 Tools
🪛 Ruff (0.15.15)
[warning] 217-217: Unpacked variable msg is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/plugins/toolsets/test_multi_instance.py` around lines 217 - 219, The
test assigns an unused second value `msg` from the call to
ts.prerequisites_callable, triggering an unused-variable lint; update the call
site in tests/plugins/toolsets/test_multi_instance.py so you only capture the
needed value (e.g., assign the second return to `_` or index the first element)
instead of binding `msg`, keeping the call to
ts.prerequisites_callable({"instances": [{"name": "a"}, {"name": "b"}]}) and
preserving the semantics of the test.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes