Add support for multiple Azure MCP instances with backward compatibility - #2278
Open
Avi-Robusta wants to merge 7 commits into
Open
Add support for multiple Azure MCP instances with backward compatibility#2278Avi-Robusta wants to merge 7 commits into
Avi-Robusta wants to merge 7 commits into
Conversation
- Add azureInstances array to values.yaml for configuring multiple Azure MCP servers - Each instance can have its own tenant ID, subscription ID, and credentials - Each instance gets a unique pod, service account, and network policy - Each instance requires a unique service port for isolation - Maintain full backward compatibility with existing single azure configuration - Update deployment template to iterate over instances or render single instance - Update network policy template to support multiple instances - Update documentation with multi-instance example and configuration guide Signed-off-by: Claude <noreply@anthropic.com>
- Add Example 1: Multiple instances with Workload Identity (AKS, no secrets) - Add Example 2: Multiple instances with Service Principal (non-AKS, requires secrets) - Add Example 3: Mixed setup with both authentication methods - Include secret creation instructions for service-principal auth - Add authentication method comparison table - Clarify that secrets are only required for service-principal auth - Document when to use each authentication method Signed-off-by: Claude <noreply@anthropic.com>
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
WalkthroughThis PR adds multi-instance Azure MCP rendering and validation across Helm values, templates, and documentation. It also updates MCP tool execution to preserve server tool identity, resolve naming collisions, and keep health-check and OAuth flows aligned with resolved tool names. ChangesAzure MCP multi-instance support
MCP tool identity and collision handling
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
- Update toolset-config.yaml to detect and configure multiple Azure instances - Each instance gets its own MCP server entry: azure_prod, azure_staging, etc. - Each entry includes: - Unique name based on instance name - Description with tenant ID for clarity - Correct URL with instance name and port - Instance-specific LLM instructions (or default if not set) - Maintain backward compatibility with single azure configuration - Holmes now properly discovers and routes to each Azure MCP instance Signed-off-by: Claude <noreply@anthropic.com>
- Update Example 1: Show production vs staging instructions for Workload Identity - Update Example 2: Show production vs staging instructions for Service Principal - Update Example 3: Show mixed setup with environment-specific instructions - Add dedicated section explaining llmInstructions field - Document how to use custom instructions to: - Distinguish environments (prod vs staging) - Set investigation scope per tenant - Define escalation paths - Provide context-specific guidance - Show that llmInstructions is optional (uses default if omitted) - Provide real examples of prod (cautious) vs staging (exploratory) instructions Signed-off-by: Claude <noreply@anthropic.com>
![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: 4
🤖 Prompt for all review comments with 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.
Inline comments:
In `@docs/data-sources/builtin-toolsets/azure-mcp.md`:
- Around line 557-560: Add an explicit language tag to the final fenced example
in the Azure MCP docs so the markdownlint MD040 check passes. Update the
unlabeled code fence containing the sample prompts to use an explicit language
such as text, keeping the example content unchanged and consistent with the
surrounding markdown formatting.
In `@helm/holmes/templates/mcp-servers/azure/deployment.yaml`:
- Around line 9-10: The ConfigMap metadata block is using .Release inside the
azureInstances range, so it resolves against the current instance instead of the
root context. Update the ConfigMap name, namespace, and labels in the ConfigMap
section to use $.Release.Name and $.Release.Namespace, matching the existing
correct usage in the ServiceAccount, Deployment, and Service templates, so the
rendered ConfigMap name and namespace stay valid and match the Deployment
reference.
- Around line 25-26: The Azure deployment template has multiple authMethod
conditionals that check .config.authMethod directly, which can skip
workload-identity wiring when the value is unset. Update every eq comparison in
the azure deployment template to use the same defaulted expression as
AZ_AUTH_METHOD, i.e. apply default "workload-identity" before the comparison in
the service-account, pod, and single-instance blocks. This should be done
consistently in the conditional branches around the authMethod checks so the
AZURE_CLIENT_ID env var and azure.workload.identity/use labels are still
rendered when authMethod is omitted.
- Line 26: The Azure deployment template’s read-only handling is inconsistent
because `default` treats `false` as empty, so `READ_ONLY_MODE` can render as
`"true"` even when `readOnlyMode` is explicitly false, and the `--readonly` arg
logic in the same template also disagrees when the value is unset. Update the
`READ_ONLY_MODE` rendering and the `--readonly` condition in the
`deployment.yaml` template to preserve explicit `false` while still defaulting
nil to true, using the existing `readOnlyMode` expression in the Azure
deployment block and mirroring the same fix in the single-instance block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ab0ed5f0-8ca8-4c5d-9edc-bf6a5c3f0b4f
📒 Files selected for processing (4)
docs/data-sources/builtin-toolsets/azure-mcp.mdhelm/holmes/templates/mcp-servers/azure/deployment.yamlhelm/holmes/templates/mcp-servers/azure/networkpolicy.yamlhelm/holmes/values.yaml
Comment on lines
+557
to
+560
| ``` | ||
| "List all resource groups in the prod tenant" | ||
| "Get VM details from the staging subscription" | ||
| ``` |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language tag to the final fenced example.
The unlabeled code fence here trips the repo’s markdownlint MD040 check. Use text (or another explicit language) to keep the docs lint-clean.
Fix
-```
+```text
"List all resource groups in the prod tenant"
"Get VM details from the staging subscription"
</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
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 557-557: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 557-557: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
🤖 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/azure-mcp.md` around lines 557 - 560, Add
an explicit language tag to the final fenced example in the Azure MCP docs so
the markdownlint MD040 check passes. Update the unlabeled code fence containing
the sample prompts to use an explicit language such as text, keeping the example
content unchanged and consistent with the surrounding markdown formatting.
Source: Linters/SAST tools
Comment on lines
+25
to
+26
| AZ_AUTH_METHOD: {{ .config.authMethod | default "workload-identity" | quote }} | ||
| READ_ONLY_MODE: {{ .config.readOnlyMode | default "true" | quote }} |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add default "workload-identity" to all authMethod conditionals — otherwise unset authMethod causes silent auth failure.
The ConfigMap (line 25) defaults AZ_AUTH_METHOD to "workload-identity", but the eq checks on lines 41, 89, 145, 149, and 161 compare .config.authMethod directly without a default. When authMethod is unset, eq nil "workload-identity" is false, so:
- No
AZURE_CLIENT_IDenv var is injected (line 146 skipped). - No
azure.workload.identity/use: "true"label is set on the SA or pod (lines 42, 90 skipped).
The application reads AZ_AUTH_METHOD: "workload-identity" from the ConfigMap but has no client ID, causing an authentication failure. The same pattern exists in the single-instance block (lines 271, 319, 375, 379, 391).
🐛 Proposed fix — apply default to all authMethod checks
- {{- if eq .config.authMethod "workload-identity" }}
+ {{- if eq (.config.authMethod | default "workload-identity") "workload-identity" }}
azure.workload.identity/use: "true"
{{- end }}Repeat the same (.config.authMethod | default "workload-identity") substitution on lines 89, 145, 149, and 161.
Also applies to: 41-41, 89-89, 145-145, 149-149, 161-161
🤖 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 `@helm/holmes/templates/mcp-servers/azure/deployment.yaml` around lines 25 -
26, The Azure deployment template has multiple authMethod conditionals that
check .config.authMethod directly, which can skip workload-identity wiring when
the value is unset. Update every eq comparison in the azure deployment template
to use the same defaulted expression as AZ_AUTH_METHOD, i.e. apply default
"workload-identity" before the comparison in the service-account, pod, and
single-instance blocks. This should be done consistently in the conditional
branches around the authMethod checks so the AZURE_CLIENT_ID env var and
azure.workload.identity/use labels are still rendered when authMethod is
omitted.
| AZURE_TENANT_ID: {{ .config.tenantId | quote }} | ||
| AZURE_SUBSCRIPTION_ID: {{ .config.subscriptionId | quote }} | ||
| AZ_AUTH_METHOD: {{ .config.authMethod | default "workload-identity" | quote }} | ||
| READ_ONLY_MODE: {{ .config.readOnlyMode | default "true" | quote }} |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
readOnlyMode: false renders as "true" in ConfigMap — Sprig default treats boolean false as empty.
Sprig's default function considers false "empty," so {{ .config.readOnlyMode | default "true" }} returns "true" when the user explicitly sets readOnlyMode: false. The ConfigMap incorrectly says READ_ONLY_MODE: "true" while the --readonly CLI flag (line 113) is correctly omitted (if false is falsy). This inconsistency means the env var and CLI flag disagree.
Additionally, when readOnlyMode is unset (nil), the ConfigMap says "true" (correct default) but {{- if .config.readOnlyMode }} is falsy, so --readonly is not passed — the CLI flag and env var disagree in the opposite direction.
The same bug exists in the single-instance block (lines 256, 343).
🐛 Proposed fix — pipe through toString before default
- READ_ONLY_MODE: {{ .config.readOnlyMode | default "true" | quote }}
+ READ_ONLY_MODE: {{ .config.readOnlyMode | toString | default "true" | quote }}toString converts false → "false" (non-empty, so default returns it) and nil → "" (empty, so default returns "true").
For the args (line 113), use the same default so nil and true both pass --readonly:
- {{- if .config.readOnlyMode }}
+ {{- if (eq .config.readOnlyMode false | not) }}
- "--readonly"
{{- end }}Also applies to: 113-113
🤖 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 `@helm/holmes/templates/mcp-servers/azure/deployment.yaml` at line 26, The
Azure deployment template’s read-only handling is inconsistent because `default`
treats `false` as empty, so `READ_ONLY_MODE` can render as `"true"` even when
`readOnlyMode` is explicitly false, and the `--readonly` arg logic in the same
template also disagrees when the value is unset. Update the `READ_ONLY_MODE`
rendering and the `--readonly` condition in the `deployment.yaml` template to
preserve explicit `false` while still defaulting nil to true, using the existing
`readOnlyMode` expression in the Azure deployment block and mirroring the same
fix in the single-instance block.
- Validate each instance has unique name - Validate each instance has unique service port - Validate required fields: tenantId, subscriptionId, authMethod - Validate authMethod is one of: workload-identity, managed-identity, service-principal - Validate service-principal instances specify secretName - Validate workload-identity instances specify clientId - Chart render fails early with clear error messages for invalid configs Signed-off-by: Claude <noreply@anthropic.com>
- Use $ prefix for root context references inside range loops - Fix .Release.Name and .Release.Namespace references in multi-instance deployment - Separate llmInstructions into variable to avoid syntax errors in dict literals - Verified with helm template - all instances render correctly Signed-off-by: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
helm/holmes/templates/toolset-config.yaml (1)
37-54: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueEdge case: all-disabled
azureInstanceswithazure.enabled: truesilently skips Azure MCP configuration.If
azureInstancesis a non-empty list (truthy) but all entries haveenabled: false, theifon line 38 is true, the range iterates but the innerif .enabledon line 40 skips all entries, and theelse ifon line 55 never executes. This means no Azure MCP servers are configured even thoughazure.enabled: trueis set. Consider documenting this behavior or adding a fallback.♻️ Optional: add fallback for all-disabled instances
{{- $anyEnabled := false -}} {{- range .Values.mcpAddons.azureInstances }} {{- if .enabled }} + {{- $anyEnabled = true }} {{- $llmInstructions := .llmInstructions | default (include "holmes.azureMcp.llmInstructions" $) }} {{- $azureInstanceMcpServers := dict (printf "azure_%s" .name) (dict "description" (printf "Azure API MCP Server (%s) - access to %s tenant. Execute Azure CLI commands." .name .config.tenantId) "config" (dict "url" (printf "http://%s-azure-%s-mcp-server.%s.svc.cluster.local:%d/mcp" $.Release.Name .name (.config.namespace | default $.Release.Namespace) (int .service.port | default 8000)) "mode" "streamable-http" "icon_url" "https://raw.githubusercontent.com/gilbarbara/logos/de2c1f96ff6e74ea7ea979b43202e8d4b863c655/logos/microsoft-azure.svg" ) "llm_instructions" ($llmInstructions | trim) ) }} {{- $mcpServers = merge $mcpServers $azureInstanceMcpServers }} {{- end }} {{- end }} - {{- else if .Values.mcpAddons.azure.enabled }} + {{- else if and (not $anyEnabled) .Values.mcpAddons.azure.enabled }}🤖 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 `@helm/holmes/templates/toolset-config.yaml` around lines 37 - 54, The Azure MCP selection in toolset-config.yaml can skip configuration when .Values.mcpAddons.azureInstances is present but every entry is disabled, because the range only merges servers inside the .enabled branch and never falls back to the azure.enabled path. Update the logic around the azureInstances loop and the azure.enabled fallback so that all-disabled instances either still trigger the default Azure MCP configuration or are explicitly documented as intentionally skipping setup; use the existing symbols $mcpServers, $azureInstanceMcpServers, and holmes.azureMcp.llmInstructions to keep the behavior consistent.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@docs/data-sources/builtin-toolsets/azure-mcp.md`:
- Around line 651-668: The fenced YAML example in the Azure MCP docs is
triggering MD046 because it does not match the repo’s expected code-block style.
Update the example in the documentation section containing azureInstances and
llmInstructions to use the same fenced/indented format used elsewhere in the
repo, or adjust the markdownlint rule only if that style change is intentional.
Keep the content and structure of the YAML example intact while making its
formatting consistent.
In `@helm/holmes/templates/validate.yaml`:
- Around line 50-52: The azureInstances name check only verifies presence in
validate.yaml, so invalid values can still flow into Kubernetes resource names
and fail later. Update the validation in the template logic around the existing
.name guard to also enforce DNS-1123-compatible names for each
mcpAddons.azureInstances entry, using the same name field that feeds the
resource and config key interpolation. Keep the existing missing-name failure,
and add a separate format failure in the validation block so invalid names are
rejected before Helm renders the resources.
- Around line 75-80: The validation in validate.yaml only checks
.config.clientId for authMethod "workload-identity"; update the same guard
around the validation block so it also rejects missing clientId for
"managed-identity". Keep the existing fail path and message style in the
validate template, and make sure the condition in the validation logic covers
both auth methods used by the deployment template that injects AZURE_CLIENT_ID.
---
Nitpick comments:
In `@helm/holmes/templates/toolset-config.yaml`:
- Around line 37-54: The Azure MCP selection in toolset-config.yaml can skip
configuration when .Values.mcpAddons.azureInstances is present but every entry
is disabled, because the range only merges servers inside the .enabled branch
and never falls back to the azure.enabled path. Update the logic around the
azureInstances loop and the azure.enabled fallback so that all-disabled
instances either still trigger the default Azure MCP configuration or are
explicitly documented as intentionally skipping setup; use the existing symbols
$mcpServers, $azureInstanceMcpServers, and holmes.azureMcp.llmInstructions to
keep the behavior consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bc491ba7-2a29-41ae-937e-35c02911f33d
📒 Files selected for processing (4)
docs/data-sources/builtin-toolsets/azure-mcp.mdhelm/holmes/templates/mcp-servers/azure/deployment.yamlhelm/holmes/templates/toolset-config.yamlhelm/holmes/templates/validate.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- helm/holmes/templates/mcp-servers/azure/deployment.yaml
Comment on lines
+651
to
+668
| ```yaml | ||
| azureInstances: | ||
| - name: "prod" | ||
| # ... config ... | ||
| llmInstructions: | | ||
| # CRITICAL: Production environment | ||
| - Always verify changes in Activity Log first | ||
| - Report findings before taking action | ||
| - Check maintenance windows before suggesting changes | ||
|
|
||
| - name: "staging" | ||
| # ... config ... | ||
| llmInstructions: | | ||
| # SAFE: Staging environment | ||
| - Feel free to investigate thoroughly | ||
| - Safe to gather detailed diagnostics | ||
| - No approval needed for diagnostics | ||
| ``` |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Match the repo’s expected code-block style here.
markdownlint-cli2 is flagging this fenced YAML example as MD046, so it likely needs to be indented in this section (or the lint rule needs to be updated intentionally).
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 651-651: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
🤖 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/azure-mcp.md` around lines 651 - 668, The
fenced YAML example in the Azure MCP docs is triggering MD046 because it does
not match the repo’s expected code-block style. Update the example in the
documentation section containing azureInstances and llmInstructions to use the
same fenced/indented format used elsewhere in the repo, or adjust the
markdownlint rule only if that style change is intentional. Keep the content and
structure of the YAML example intact while making its formatting consistent.
Source: Linters/SAST tools
Comment on lines
+50
to
+52
| {{- if not .name }} | ||
| {{- fail "mcpAddons.azureInstances: each instance must have a 'name' field" }} | ||
| {{- end }} |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add instance name format validation to prevent Kubernetes resource creation failures.
The name is interpolated into Kubernetes resource names (e.g., {release}-azure-{name}-mcp-server, azure-{name}-mcp-sa) and toolset config keys (azure_{name}). Kubernetes requires DNS-1123 subdomain names (lowercase alphanumeric with hyphens). A name like Prod or prod_east would pass the current not .name check but fail at resource creation with a confusing API error.
Suggest adding a regex validation:
🛡️ Proposed fix: add name format validation
{{- if not .name }}
{{- fail "mcpAddons.azureInstances: each instance must have a 'name' field" }}
{{- end }}
+ {{- if not (regexMatch "^[a-z0-9]([-a-z0-9]*[a-z0-9])?$" .name) }}
+ {{- fail (printf "mcpAddons.azureInstances[%s]: invalid instance name %q. Names must be lowercase alphanumeric with hyphens (DNS-1123 subdomain), matching ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$" .name .name) }}
+ {{- end }}
{{- if not .config.tenantId }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {{- if not .name }} | |
| {{- fail "mcpAddons.azureInstances: each instance must have a 'name' field" }} | |
| {{- end }} | |
| {{- if not .name }} | |
| {{- fail "mcpAddons.azureInstances: each instance must have a 'name' field" }} | |
| {{- end }} | |
| {{- if not (regexMatch "^[a-z0-9]([-a-z0-9]*[a-z0-9])?$" .name) }} | |
| {{- fail (printf "mcpAddons.azureInstances[%s]: invalid instance name %q. Names must be lowercase alphanumeric with hyphens (DNS-1123 subdomain), matching ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$" .name .name) }} | |
| {{- end }} |
🤖 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 `@helm/holmes/templates/validate.yaml` around lines 50 - 52, The azureInstances
name check only verifies presence in validate.yaml, so invalid values can still
flow into Kubernetes resource names and fail later. Update the validation in the
template logic around the existing .name guard to also enforce
DNS-1123-compatible names for each mcpAddons.azureInstances entry, using the
same name field that feeds the resource and config key interpolation. Keep the
existing missing-name failure, and add a separate format failure in the
validation block so invalid names are rejected before Helm renders the
resources.
Comment on lines
+75
to
+80
| {{- /* Validate workload-identity requires clientId */ -}} | ||
| {{- if eq .config.authMethod "workload-identity" }} | ||
| {{- if not .config.clientId }} | ||
| {{- fail (printf "mcpAddons.azureInstances[%s]: when using authMethod 'workload-identity', 'config.clientId' is required" .name) }} | ||
| {{- end }} | ||
| {{- end }} |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if managed-identity has any specific env var or annotation handling in deployment.yaml
rg -n -C3 'managed-identity' helm/holmes/templates/mcp-servers/azure/deployment.yamlRepository: HolmesGPT/holmesgpt
Length of output: 849
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## validate.yaml"
sed -n '65,90p' helm/holmes/templates/validate.yaml | cat -n
echo
echo "## values.yaml references to clientId"
rg -n -C2 'clientId|workload-identity|managed-identity' helm/holmes/values.yaml helm/holmes/templates -g '!**/node_modules/**'Repository: HolmesGPT/holmesgpt
Length of output: 13814
Enforce clientId for managed-identity too. helm/holmes/templates/validate.yaml:75-80 should reject missing .config.clientId for both workload-identity and managed-identity, since the deployment template injects AZURE_CLIENT_ID for both paths.
🤖 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 `@helm/holmes/templates/validate.yaml` around lines 75 - 80, The validation in
validate.yaml only checks .config.clientId for authMethod "workload-identity";
update the same guard around the validation block so it also rejects missing
clientId for "managed-identity". Keep the existing fail path and message style
in the validate template, and make sure the condition in the validation logic
covers both auth methods used by the deployment template that injects
AZURE_CLIENT_ID.
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:e113b79a4
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:e113b79a4 me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:e113b79a4
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:e113b79a4
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:e113b79a4
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:e113b79a4 me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:e113b79a4
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:e113b79a4Patch 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:e113b79a4 \
--set operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set operator.image=holmes-operator-dev:e113b79a4Robusta 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:e113b79a4 \
--set holmes.operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.operator.image=holmes-operator-dev:e113b79a4 |
Multiple MCP servers exposing an identically-named tool (e.g. two Azure
instances both exposing call_az) collided in ToolExecutor.tools_by_name,
so only one server was reachable.
Add a pure resolve_tool_name_collisions() in the executor that namespaces
a tool as {toolset}__{tool} ONLY when its raw name collides across enabled
toolsets; single-instance names are unchanged. RemoteMCPTool keeps the raw
server name in mcp_tool_name, used for call_tool() and internal checks
(run_gcloud_command, OAuth connect placeholder, health-check) so they keep
working when a name is namespaced. The prefix is sanitized to a valid
function name.
Collision-only alternative to the reverted #1876/#1914, which prefixed
every MCP tool unconditionally and broke hardcoded name checks. Adds
collision / no-collision / mixed / resolver / sanitization / OAuth-connect
tests.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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/mcp/toolset_mcp.py (1)
1158-1184: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPreserve endpoint failure details in the OAuth reachability error.
The two probe exceptions are swallowed, and the final “no HTTP response” error omits which endpoint failed and why. Return the attempted method/URL plus exception details so the LLM/user can self-correct connection issues. As per coding guidelines, toolsets must return detailed error messages from underlying APIs, including exact query/command and full API error details.
Proposed fix
+ endpoint_errors: list[str] = [] responses: list[httpx.Response] = [] try: + endpoint = f"{url}/.well-known/oauth-protected-resource" r = httpx.get( - f"{url}/.well-known/oauth-protected-resource", + endpoint, timeout=10, verify=self._mcp_config.verify_ssl, follow_redirects=False, ) responses.append(r) - except Exception: - pass + except Exception as e: + endpoint_errors.append( + f"GET {endpoint} failed: {_extract_root_error_message(e)}" + ) try: r2 = httpx.post( url, @@ ) responses.append(r2) - except Exception: - pass + except Exception as e: + endpoint_errors.append( + f"POST {url} failed: {_extract_root_error_message(e)}" + ) if not responses: return ( False, - f"MCP server {self.name} unreachable: no HTTP response from either endpoint", + f"MCP server {self.name} unreachable: no HTTP response from either endpoint. " + f"Attempts: {'; '.join(endpoint_errors)}", )🤖 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/mcp/toolset_mcp.py` around lines 1158 - 1184, The OAuth reachability check in the MCP toolset is swallowing both probe failures and then returning a generic “no HTTP response” message. Update the reachability logic in the MCP server check to capture the exception from each httpx.get and httpx.post attempt, and include the attempted method, URL, and exception details in the returned error when no response is obtained. Keep the fix localized around the response collection path in toolset_mcp.py so the final message from the reachability helper is explicit about which endpoint failed and why.Sources: Coding guidelines, Linters/SAST tools
🤖 Prompt for all review comments with 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.
Inline comments:
In `@holmes/core/tools_utils/tool_executor.py`:
- Around line 48-49: The sanitized tool name in ToolExecutor can collide when
different Toolset names normalize to the same prefix, causing one tool to be
overwritten and become unreachable. Update the name generation in
ToolExecutor.resolve_tools (the prefix sanitization and resolved.append logic)
to guarantee uniqueness across toolsets, such as by detecting duplicate
sanitized prefixes and appending a stable disambiguator before storing the
exposed name.
In `@holmes/plugins/toolsets/mcp/toolset_mcp.py`:
- Around line 1279-1288: The OAuth discovery path in the MCP toolset should not
succeed when both `oauth_config.client_id` and
`oauth_config.registration_endpoint` are missing. Update the discovery logic in
the method that handles this check so that the `logging.warning` branch also
returns failure instead of continuing, while preserving the existing
deferred-runtime behavior when `registration_endpoint` is present. Ensure the
placeholder registration is only allowed when a real client ID exists or DCR is
available.
---
Outside diff comments:
In `@holmes/plugins/toolsets/mcp/toolset_mcp.py`:
- Around line 1158-1184: The OAuth reachability check in the MCP toolset is
swallowing both probe failures and then returning a generic “no HTTP response”
message. Update the reachability logic in the MCP server check to capture the
exception from each httpx.get and httpx.post attempt, and include the attempted
method, URL, and exception details in the returned error when no response is
obtained. Keep the fix localized around the response collection path in
toolset_mcp.py so the final message from the reachability helper is explicit
about which endpoint failed and why.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8592c6f1-6274-4590-9e37-4615677c625e
📒 Files selected for processing (3)
holmes/core/tools_utils/tool_executor.pyholmes/plugins/toolsets/mcp/toolset_mcp.pytests/test_mcp_toolset.py
Comment on lines
+48
to
+49
| prefix = re.sub(r"[^a-zA-Z0-9]+", "_", ts.name).strip("_") | ||
| resolved.append((ts, tool, f"{prefix}__{r}")) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Avoid silent overwrites when sanitized prefixes collide.
Line 48 can normalize distinct toolset names to the same prefix, e.g. azure-prod and azure_prod, so Line 49 can produce the same exposed name for both tools. ToolExecutor then overwrites one of them, making one MCP server unreachable for that tool.
Proposed fix
+import hashlib
import re
from typing import Dict, List, Optional, Tuple
@@
- resolved: List[Tuple[Toolset, "Tool", str]] = []
+ resolved: List[Tuple[Toolset, "Tool", str]] = []
+ used_resolved_names: set[str] = set()
@@
- prefix = re.sub(r"[^a-zA-Z0-9]+", "_", ts.name).strip("_")
- resolved.append((ts, tool, f"{prefix}__{r}"))
+ prefix = re.sub(r"[^a-zA-Z0-9]+", "_", ts.name).strip("_") or "toolset"
+ resolved_name = f"{prefix}__{r}"
+ if resolved_name in used_resolved_names:
+ suffix = hashlib.sha1(ts.name.encode("utf-8")).hexdigest()[:8]
+ resolved_name = f"{prefix}_{suffix}__{r}"
+ used_resolved_names.add(resolved_name)
+ resolved.append((ts, tool, resolved_name))
else:
- resolved.append((ts, tool, r))
+ used_resolved_names.add(r)
+ resolved.append((ts, tool, r))📝 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
| prefix = re.sub(r"[^a-zA-Z0-9]+", "_", ts.name).strip("_") | |
| resolved.append((ts, tool, f"{prefix}__{r}")) | |
| prefix = re.sub(r"[^a-zA-Z0-9]+", "_", ts.name).strip("_") or "toolset" | |
| resolved_name = f"{prefix}__{r}" | |
| if resolved_name in used_resolved_names: | |
| suffix = hashlib.sha1(ts.name.encode("utf-8")).hexdigest()[:8] | |
| resolved_name = f"{prefix}_{suffix}__{r}" | |
| used_resolved_names.add(resolved_name) | |
| resolved.append((ts, tool, resolved_name)) |
🤖 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/core/tools_utils/tool_executor.py` around lines 48 - 49, The sanitized
tool name in ToolExecutor can collide when different Toolset names normalize to
the same prefix, causing one tool to be overwritten and become unreachable.
Update the name generation in ToolExecutor.resolve_tools (the prefix
sanitization and resolved.append logic) to guarantee uniqueness across toolsets,
such as by detecting duplicate sanitized prefixes and appending a stable
disambiguator before storing the exposed name.
Comment on lines
1279
to
+1288
| if not oauth_config.client_id: | ||
| if oauth_config.registration_endpoint: | ||
| logging.debug("OAuth discovery %s: no client_id, DCR deferred to runtime", self.name) | ||
| logging.debug( | ||
| "OAuth discovery %s: no client_id, DCR deferred to runtime", | ||
| self.name, | ||
| ) | ||
| else: | ||
| logging.warning("OAuth discovery %s: no client_id and no DCR endpoint", self.name) | ||
| logging.warning( | ||
| "OAuth discovery %s: no client_id and no DCR endpoint", self.name | ||
| ) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail discovery when neither client_id nor DCR is available.
If client_id is missing and the metadata has no registration_endpoint, the method logs a warning but still returns True; the placeholder is registered and the OAuth flow later receives client_id=None.
Proposed fix
if not oauth_config.client_id:
if oauth_config.registration_endpoint:
logging.debug(
"OAuth discovery %s: no client_id, DCR deferred to runtime",
self.name,
)
else:
logging.warning(
"OAuth discovery %s: no client_id and no DCR endpoint", self.name
)
+ return False📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if not oauth_config.client_id: | |
| if oauth_config.registration_endpoint: | |
| logging.debug("OAuth discovery %s: no client_id, DCR deferred to runtime", self.name) | |
| logging.debug( | |
| "OAuth discovery %s: no client_id, DCR deferred to runtime", | |
| self.name, | |
| ) | |
| else: | |
| logging.warning("OAuth discovery %s: no client_id and no DCR endpoint", self.name) | |
| logging.warning( | |
| "OAuth discovery %s: no client_id and no DCR endpoint", self.name | |
| ) | |
| if not oauth_config.client_id: | |
| if oauth_config.registration_endpoint: | |
| logging.debug( | |
| "OAuth discovery %s: no client_id, DCR deferred to runtime", | |
| self.name, | |
| ) | |
| else: | |
| logging.warning( | |
| "OAuth discovery %s: no client_id and no DCR endpoint", self.name | |
| ) | |
| return False |
🤖 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/mcp/toolset_mcp.py` around lines 1279 - 1288, The
OAuth discovery path in the MCP toolset should not succeed when both
`oauth_config.client_id` and `oauth_config.registration_endpoint` are missing.
Update the discovery logic in the method that handles this check so that the
`logging.warning` branch also returns failure instead of continuing, while
preserving the existing deferred-runtime behavior when `registration_endpoint`
is present. Ensure the placeholder registration is only allowed when a real
client ID exists or DCR is available.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Claude noreply@anthropic.com
Summary by CodeRabbit