Refactor Kubernetes Remediation MCP: approval-based tool separation - #2148
Merged
Merged
Conversation
Approval is now purely tool-name based via approval_required_tools. The legacy restricted_tools / skill-gating coupling (skills unlocking restricted tools) is removed entirely — it is no longer needed. Core: - tools.py: drop Tool.restricted, Tool._is_restricted(), and the restricted_tools fields on Toolset and ToolsetYamlFromConfig. The approval_required_tools / _check_approval_config / requires_approval path is kept as-is (this is what gates run_kubectl_command). - tool_calling_llm.py: drop _skill_in_use and _should_include_restricted_tools; stop passing include_restricted. Skill fetching itself is untouched. - tool_executor.py / frontend_tools.py: drop the include_restricted param/filter and the _is_restricted override. Helm (mcpAddons.kubernetesRemediation): - values: drop restrictedTools; approvalRequiredTools -> ["run_kubectl_command"]; clusterRole "" (chart creates a scoped role); networkPolicy on by default; image 1.1.0; new config keys (preapproved commands, diagnostic images, file-read paths, allowArbitraryKubectlCommands). - new rbac.yaml: scoped least-privilege ClusterRole (no cluster-admin, no secret access), gated on serviceAccount.create and empty clusterRole. - deployment: wire the new env vars; binding no longer defaults to cluster-admin. - networkpolicy: ingress-only, scoped to Holmes pods in the release namespace. - _helpers.tpl: rewrite llm_instructions around the auto-approved vs approval-gated tool split. - toolset-config: emit approval_required_tools instead of restricted_tools. Docs: rewrite kubernetes-remediation-mcp.md (5-tool table with approval column, scoped RBAC sample, plug-and-play defaults, CLI config without restricted_tools). Tests: tool-name approval gating, and a helm values/template regression check. Signed-off-by: Claude <noreply@anthropic.com>
Backwards compatibility: old configs carrying the removed restricted_tools key now load with a deprecation warning instead of silently dropping it, guiding users to approval_required_tools. Signed-off-by: Claude <noreply@anthropic.com>
![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
📂 Previous Runs
|
| Status | Test case | Time | Turns | Tools | Cost | Total tokens | Input | Max input | Output | Max output | Cached | Non-cached | Reasoning | Compactions | Denied commands | Src |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| ✅ | 09_crashpod | 26.2s | 4 | 8 | $0.2108 | 70,075 | 68,386 | 19,583 | 1,689 | 739 | 48,230 | 20,156 | 125 | — | — | src |
| ✅ | 101_loki_historical_logs_pod_deleted | 38.5s | 4 | 8 | $0.2275 | 69,722 | 67,284 | 19,358 | 2,438 | 867 | 47,637 | 19,647 | 430 | — | — | src |
| ✅ | 112_find_pvcs_by_uuid | 14.4s | 2 | 2 | $0.1563 | 33,188 | 32,266 | 17,944 | 922 | 480 | 14,319 | 17,947 | 294 | — | — | src |
| ✅ | 12_job_crashing | 34.1s | 5 | 10 | $0.2488 | 95,048 | 93,219 | 21,345 | 1,829 | 539 | 69,397 | 23,822 | 172 | — | — | src |
| ✅ | 176_network_policy_blocking_traffic_no_skills | 31.5s | 4 | 10 | $0.2268 | 71,162 | 69,319 | 20,595 | 1,843 | 587 | 47,164 | 22,155 | 369 | — | — | src |
| ✅ | 227_count_configmaps_per_namespace[0] | 14.6s | 3 | 6 | $0.1488 | 46,361 | 45,651 | 16,667 | 710 | 437 | 28,980 | 16,671 | 29 | — | — | src |
| ✅ | 243_pod_names_contain_service | 29.8s | 3 | 7 | $0.1912 | 49,606 | 47,802 | 18,079 | 1,804 | 677 | 29,445 | 18,357 | 301 | — | — | src |
| ✅ | 24_misconfigured_pvc | 29.2s | 5 | 11 | $0.2191 | 86,578 | 84,848 | 19,114 | 1,730 | 412 | 64,925 | 19,923 | 42 | — | — | src |
| ✅ | 254_elasticsearch_dr_test_log_check | 48.0s | 7 | 11 | $0.2580 | 96,445 | 93,206 | 18,428 | 3,239 | 842 | 74,134 | 19,072 | 130 | — | — | src |
| ✅ | 259_wrong_cluster_logs_confusion | 62.9s | 7 | 12 | $0.2969 | 103,554 | 99,267 | 18,652 | 4,287 | 1,592 | 79,319 | 19,948 | 683 | — | — | src |
| ✅ | 260_global_es_remote_cluster_logs | 53.8s | 5 | 9 | $0.2480 | 74,722 | 71,253 | 17,561 | 3,469 | 820 | 53,218 | 18,035 | 630 | — | — | src |
| ✅ | 43_current_datetime_from_prompt | 3.8s | 1 | — | $0.1017 | 14,428 | 14,306 | 14,306 | 122 | 122 | 0 | 14,306 | 78 | — | — | src |
| ✅ | 51_logs_summarize_errors | 17.7s | 3 | 2 | $0.1546 | 46,888 | 46,151 | 17,219 | 737 | 360 | 28,928 | 17,223 | 33 | — | — | src |
| ✅ | 61_exact_match_counting | 7.4s | 2 | 1 | $0.1146 | 29,148 | 28,927 | 14,641 | 221 | 152 | 14,283 | 14,644 | 34 | — | — | src |
| Total | 29.4s avg | 3.9 avg | 7.5 avg | $2.8032 | 886,925 | 861,885 | 21,345 | 25,040 | 1,592 | 599,979 | 261,906 | 3,350 | — | — |
Benchmark Comparison Details
Master baseline: latest master-* experiment (post-merge regression eval)
Status: 14 test/model combinations loaded
- master-27494040539 (created: 2026-06-14)
Benchmark baseline: latest ci-benchmark experiment on master
Status: 17 test/model combinations loaded
- ci-benchmark-27491953079 (created: 2026-06-14)
Time comparison (seconds):
| Test case | This branch | master (2h ago) | Δ vs master | benchmark (3h ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | 26.2s | 21.9s | ↑19% | — | — |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | 38.5s | 47.4s | ↓19% | — | — |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | 14.4s | 14.1s | ±0% | — | — |
| 12_job_crashing (opus-4.6) 📄 | 34.1s | 28.3s | ↑20% | — | — |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | 31.5s | 31.1s | ±0% | — | — |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | 14.6s | 14.4s | ±0% | — | — |
| 243_pod_names_contain_service (opus-4.6) 📄 | 29.8s | 28.7s | ±0% | — | — |
| 24_misconfigured_pvc (opus-4.6) 📄 | 29.2s | 28.0s | ±0% | — | — |
| 254_elasticsearch_dr_test_log_check (opus-4.6) 📄 | 48.0s | 54.1s | ↓11% | — | — |
| 259_wrong_cluster_logs_confusion (opus-4.6) 📄 | 62.9s | 63.4s | ±0% | — | — |
| 260_global_es_remote_cluster_logs (opus-4.6) 📄 | 53.8s | 65.1s | ↓17% | — | — |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | 3.8s | 3.9s | ±0% | — | — |
| 51_logs_summarize_errors (opus-4.6) 📄 | 17.7s | 17.1s | ±0% | — | — |
| 61_exact_match_counting (opus-4.6) 📄 | 7.4s | 7.1s | ±0% | — | — |
| Total (all, n=14) | 29.4s | 30.3s | — | — | — |
| Comparable (m=14, b=0) | 29.4s | 30.3s | ±0% | — | — |
Cost comparison:
| Test case | This branch | master (2h ago) | Δ vs master | benchmark (3h ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | $0.2108 | $0.1800 | ↑17% | — | — |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | $0.2275 | $0.2682 | ↓15% | — | — |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | $0.1563 | $0.1567 | ±0% | — | — |
| 12_job_crashing (opus-4.6) 📄 | $0.2488 | $0.2165 | ↑15% | — | — |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | $0.2268 | $0.2276 | ±0% | — | — |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | $0.1488 | $0.1492 | ±0% | — | — |
| 243_pod_names_contain_service (opus-4.6) 📄 | $0.1912 | $0.1916 | ±0% | — | — |
| 24_misconfigured_pvc (opus-4.6) 📄 | $0.2191 | $0.2136 | ±0% | — | — |
| 254_elasticsearch_dr_test_log_check (opus-4.6) 📄 | $0.2580 | $0.2668 | ±0% | — | — |
| 259_wrong_cluster_logs_confusion (opus-4.6) 📄 | $0.2969 | $0.2996 | ±0% | — | — |
| 260_global_es_remote_cluster_logs (opus-4.6) 📄 | $0.2480 | $0.3025 | ↓18% | — | — |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | $0.1017 | $0.1017 | ±0% | — | — |
| 51_logs_summarize_errors (opus-4.6) 📄 | $0.1546 | $0.1546 | ±0% | — | — |
| 61_exact_match_counting (opus-4.6) 📄 | $0.1146 | $0.1148 | ±0% | — | — |
| Total (all, n=14) | $0.2002 | $0.2031 | — | — | — |
| Comparable (m=14, b=0) | $0.2002 | $0.2031 | ±0% | — | — |
Total tokens comparison:
| Test case | This branch | master (2h ago) | Δ vs master | benchmark (3h ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | 70,075 | 49,629 | ↑41% | — | — |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | 69,722 | 91,718 | ↓24% | — | — |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | 33,188 | 33,198 | ±0% | — | — |
| 12_job_crashing (opus-4.6) 📄 | 95,048 | 71,809 | ↑32% | — | — |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | 71,162 | 73,590 | ±0% | — | — |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | 46,361 | 46,352 | ±0% | — | — |
| 243_pod_names_contain_service (opus-4.6) 📄 | 49,606 | 50,018 | ±0% | — | — |
| 24_misconfigured_pvc (opus-4.6) 📄 | 86,578 | 85,995 | ±0% | — | — |
| 254_elasticsearch_dr_test_log_check (opus-4.6) 📄 | 96,445 | 119,075 | ↓19% | — | — |
| 259_wrong_cluster_logs_confusion (opus-4.6) 📄 | 103,554 | 104,919 | ±0% | — | — |
| 260_global_es_remote_cluster_logs (opus-4.6) 📄 | 74,722 | 139,543 | ↓46% | — | — |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | 14,428 | 14,429 | ±0% | — | — |
| 51_logs_summarize_errors (opus-4.6) 📄 | 46,888 | 46,815 | ±0% | — | — |
| 61_exact_match_counting (opus-4.6) 📄 | 29,148 | 29,162 | ±0% | — | — |
| Total (all, n=14) | 63,352 | 68,304 | — | — | — |
| Comparable (m=14, b=0) | 63,352 | 68,304 | ±0% | — | — |
Cached tokens comparison:
| Test case | This branch | master (2h ago) | Δ vs master | benchmark (3h ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | 48,230 | 29,359 | ↑64% | — | — |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | 47,637 | 66,827 | ↓29% | — | — |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | 14,319 | 14,319 | ±0% | — | — |
| 12_job_crashing (opus-4.6) 📄 | 69,397 | 48,821 | ↑42% | — | — |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | 47,164 | 50,301 | ±0% | — | — |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | 28,980 | 28,979 | ±0% | — | — |
| 243_pod_names_contain_service (opus-4.6) 📄 | 29,445 | 29,440 | ±0% | — | — |
| 24_misconfigured_pvc (opus-4.6) 📄 | 64,925 | 64,473 | ±0% | — | — |
| 254_elasticsearch_dr_test_log_check (opus-4.6) 📄 | 74,134 | 97,757 | ↓24% | — | — |
| 259_wrong_cluster_logs_confusion (opus-4.6) 📄 | 79,319 | 81,004 | ±0% | — | — |
| 260_global_es_remote_cluster_logs (opus-4.6) 📄 | 53,218 | 116,916 | ↓54% | — | — |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | — | — | — | — | — |
| 51_logs_summarize_errors (opus-4.6) 📄 | 28,928 | 28,928 | ±0% | — | — |
| 61_exact_match_counting (opus-4.6) 📄 | 14,283 | 14,283 | ±0% | — | — |
| Total (all, n=14) | 42,856 | 47,958 | — | — | — |
| Comparable (m=13, b=0) | 46,152 | 51,647 | ↓11% | — | — |
Turns comparison:
| Test case | This branch | master (2h ago) | Δ vs master | benchmark (3h ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | 4 | 3 | ↑33% | — | — |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | 4 | 5 | ↓20% | — | — |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | 2 | 2 | ±0% | — | — |
| 12_job_crashing (opus-4.6) 📄 | 5 | 4 | ↑25% | — | — |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | 4 | 4 | ±0% | — | — |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | 3 | 3 | ±0% | — | — |
| 243_pod_names_contain_service (opus-4.6) 📄 | 3 | 3 | ±0% | — | — |
| 24_misconfigured_pvc (opus-4.6) 📄 | 5 | 5 | ±0% | — | — |
| 254_elasticsearch_dr_test_log_check (opus-4.6) 📄 | 7 | 9 | ↓22% | — | — |
| 259_wrong_cluster_logs_confusion (opus-4.6) 📄 | 7 | 7 | ±0% | — | — |
| 260_global_es_remote_cluster_logs (opus-4.6) 📄 | 5 | 10 | ↓50% | — | — |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | 1 | 1 | ±0% | — | — |
| 51_logs_summarize_errors (opus-4.6) 📄 | 3 | 3 | ±0% | — | — |
| 61_exact_match_counting (opus-4.6) 📄 | 2 | 2 | ±0% | — | — |
| Total (all, n=14) | 3.9 | 4.4 | — | — | — |
| Comparable (m=14, b=0) | 3.9 | 4.4 | ±0% | — | — |
Tool calls comparison:
| Test case | This branch | master (2h ago) | Δ vs master | benchmark (3h ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | 8 | 6 | ↑33% | — | — |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | 8 | 10 | ↓20% | — | — |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | 2 | 2 | ±0% | — | — |
| 12_job_crashing (opus-4.6) 📄 | 10 | 9 | ↑11% | — | — |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | 10 | 11 | ±0% | — | — |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | 6 | 6 | ±0% | — | — |
| 243_pod_names_contain_service (opus-4.6) 📄 | 7 | 7 | ±0% | — | — |
| 24_misconfigured_pvc (opus-4.6) 📄 | 11 | 10 | ↑10% | — | — |
| 254_elasticsearch_dr_test_log_check (opus-4.6) 📄 | 11 | 12 | ±0% | — | — |
| 259_wrong_cluster_logs_confusion (opus-4.6) 📄 | 12 | 13 | ±0% | — | — |
| 260_global_es_remote_cluster_logs (opus-4.6) 📄 | 9 | 12 | ↓25% | — | — |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | — | — | — | — | — |
| 51_logs_summarize_errors (opus-4.6) 📄 | 2 | 2 | ±0% | — | — |
| 61_exact_match_counting (opus-4.6) 📄 | 1 | 1 | ±0% | — | — |
| Total (all, n=14) | 6.9 | 7.8 | — | — | — |
| Comparable (m=13, b=0) | 7.5 | 7.8 | ±0% | — | — |
Comparison indicators:
±0%— diff under 10% (within noise threshold)↑N%/↓N%— diff 10-25%↑N%/↓N%— diff over 25% (significant)
📖 Legend
| Icon | Meaning |
|---|---|
| ✅ | The test was successful |
| ➖ | The test was skipped |
| The test failed but is known to be flaky or known to fail | |
| 🚧 | The test had a setup failure (not a code regression) |
| 🔧 | The test failed due to mock data issues (not a code regression) |
| 🚫 | The test was throttled by API rate limits/overload |
| ❌ | The test failed and should be fixed before merging the PR |
🔄 Re-run evals manually
⚠️ Warning:/evalcomments always run using the workflow from master, not from this PR branch. If you modified the GitHub Action (e.g., added secrets or env vars), those changes won't take effect.To test workflow changes, use the GitHub CLI or Actions UI instead:
gh workflow run eval-regression.yaml --repo HolmesGPT/holmesgpt --ref claude/k8s-remediation-mcp-BTz5W -f markers=regression -f filter=
Option 1: Comment on this PR with /eval:
/eval
tags: regression
Or with more options (one per line):
/eval
model: gpt-4o
tags: regression
id: 09_crashpod
iterations: 5
Run evals on a different branch (e.g., master) for comparison:
/eval
branch: master
tags: regression
| Option | Description |
|---|---|
model |
Model(s) to test (default: same as automatic runs) |
tags |
Pytest tags / markers (no default - runs all tests!) |
id |
Eval ID / pytest -k filter (use /list to see valid eval names) |
iterations |
Number of runs, max 10 |
branch |
Run evals on a different branch (for cross-branch comparison) |
Quick re-run: Use /rerun to re-run the most recent /eval on this PR with the same parameters.
Option 2: Trigger via GitHub Actions UI → "Run workflow"
Option 3: Add PR labels to include extra evals (applies to both automatic runs and /eval comments):
| Label | Effect |
|---|---|
evals-tag-<name> |
Run tests with tag <name> alongside regression |
evals-id-<name> |
Run a specific eval by test ID |
evals-model-<name> |
Override the model (use model list name, e.g. sonnet-4.5) |
Examples: evals-tag-easy, evals-id-09_crashpod, evals-model-sonnet-4.5
🏷️ Valid tags
benchmark, chain-of-causation, compaction, confluence, context_window, conversation_worker, coralogix, counting, database, datadog, datetime, db-connectors, easy, elasticsearch, embeds, fast, frontend, grafana, hard, images, integration, kafka, kubernetes, leaked-information, logs, loki, manual, mcp, medium, metrics, multi-cluster, network, newrelic, no-cicd, numerical, one-test, port-forward, prometheus, question-answer, regression, skills, slackbot, storage, token-limit, toolset-limitation, traces, transparency, victorialogs
🤖 Valid models
deepseek-chat, deepseek-r1-reasoner, deepseek-reasoner, deepseek-v3.2-chat, fable-5, gemini-3-flash-preview, gemini-3-pro-preview, gemini-3.1-pro-preview, gpt-4.1, gpt-5.2-high-reasoning, gpt-5.3-codex, gpt-5.4, gpt-5.5, haiku-4.5, kimi-2.5, kimi-2.5-openrouter, opus-4.5, opus-4.6, opus-4.7, opus-4.8, qwen-next-80B-instruct, qwen-next-80B-thinking, sonnet-4.5, sonnet-4.6
Commands: /eval · /rerun · /list
CLI: gh workflow run eval-regression.yaml --repo HolmesGPT/holmesgpt --ref claude/k8s-remediation-mcp-BTz5W -f markers=regression -f filter=
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR centralizes approval gating on ChangesApproval-Based Tool Gating Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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:014415ee3
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:014415ee3 me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:014415ee3
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:014415ee3
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:014415ee3
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:014415ee3 me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:014415ee3
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:014415ee3Patch 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:014415ee3 \
--set operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set operator.image=holmes-operator-dev:014415ee3Robusta 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:014415ee3 \
--set holmes.operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.operator.image=holmes-operator-dev:014415ee3 |
✅ 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.
🧹 Nitpick comments (1)
tests/test_approval_required_tools.py (1)
100-103: ⚡ Quick winMove
ToolsetYamlFromConfigimport to module scope.Line 102 imports inside the test function; move it to the top import block for consistency and simpler static analysis.
Suggested fix
from holmes.core.models import StructuredToolResult, StructuredToolResultStatus +from holmes.core.tools import ToolsetYamlFromConfig @@ def test_deprecated_restricted_tools_is_ignored_with_warning(caplog): """Old configs carrying the removed restricted_tools key load without error.""" - from holmes.core.tools import ToolsetYamlFromConfig - with caplog.at_level("WARNING"):As per coding guidelines, "
**/*.py: Always place Python imports at the top of the file, not inside functions or methods."🤖 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/test_approval_required_tools.py` around lines 100 - 103, The test currently imports ToolsetYamlFromConfig inside the test function test_deprecated_restricted_tools_is_ignored_with_warning; move the import statement for ToolsetYamlFromConfig to the module-level import block at the top of the file so it is imported once for the module (not inside the function), updating any existing top imports to include ToolsetYamlFromConfig from holmes.core.tools and removing the inline import in test_deprecated_restricted_tools_is_ignored_with_warning.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@tests/test_approval_required_tools.py`:
- Around line 100-103: The test currently imports ToolsetYamlFromConfig inside
the test function test_deprecated_restricted_tools_is_ignored_with_warning; move
the import statement for ToolsetYamlFromConfig to the module-level import block
at the top of the file so it is imported once for the module (not inside the
function), updating any existing top imports to include ToolsetYamlFromConfig
from holmes.core.tools and removing the inline import in
test_deprecated_restricted_tools_is_ignored_with_warning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 58f064c9-eccb-45ac-b554-6f5f09063ef8
📒 Files selected for processing (14)
docs/data-sources/builtin-toolsets/kubernetes-remediation-mcp.mdhelm/holmes/templates/mcp-servers/kubernetes-remediation/_helpers.tplhelm/holmes/templates/mcp-servers/kubernetes-remediation/deployment.yamlhelm/holmes/templates/mcp-servers/kubernetes-remediation/networkpolicy.yamlhelm/holmes/templates/mcp-servers/kubernetes-remediation/rbac.yamlhelm/holmes/templates/toolset-config.yamlhelm/holmes/values.yamlholmes/core/tool_calling_llm.pyholmes/core/tools.pyholmes/core/tools_utils/frontend_tools.pyholmes/core/tools_utils/tool_executor.pytests/test_approval_required_tools.pytests/test_kubernetes_remediation_helm.pytests/test_mcp_oauth.py
💤 Files with no reviewable changes (2)
- tests/test_mcp_oauth.py
- holmes/core/tools_utils/frontend_tools.py
Addresses CodeRabbit nitpick / repo guideline to keep imports at file top. Signed-off-by: Claude <noreply@anthropic.com>
- specs/kubernetes-remediation-mcp.md: full design + security review across both repos (tool taxonomy, RBAC/NetworkPolicy, the agent-core removal, the actual security boundaries, residual risks, test/eval coverage) to ease team review. - docs: path-policy row now reflects in-container symlink resolution and the hard /proc,/sys,/dev denial (server PR robusta-dev/holmes-mcp-integrations#24). Signed-off-by: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
specs/kubernetes-remediation-mcp.md (2)
192-199: ⚡ Quick winConsider documenting resource exhaustion risks and mitigations.
§6.3 documents diagnostic pod hardening including memory limits, but does not address:
- Risk of multiple concurrent diagnostic pods consuming cluster resources
- Rate limiting or concurrency controls
- Resource quota interactions in the target namespace
While the 60s timeout (line 94) and per-pod memory caps provide some protection, an LLM could still launch many diagnostic pods in parallel. Consider adding a risk note in §6.4 about resource exhaustion and potential mitigations (e.g., server-side concurrency limits, resource quotas in the MCP server's own namespace).
🤖 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 `@specs/kubernetes-remediation-mcp.md` around lines 192 - 199, Add a short risk note referencing §6.3's run_diagnostic_image hardening that explains the potential for resource exhaustion if many diagnostic pods are launched concurrently and lists mitigations to include in §6.4 — mention the existing 60s timeout and per-pod memory caps, and recommend server-side concurrency/rate limits, namespace ResourceQuota usage, and limiting total concurrent diagnostic pods (or queuing) in the MCP server; tie these recommendations to the run_diagnostic_image behavior so readers can correlate mitigations with the implemented limits.
210-215: ⚡ Quick winThe approval sync dependency is clearly documented but architecturally significant.
The acknowledgment that "the server has no notion of approval and will run
run_kubectl_commandif called directly" (lines 211-212) highlights a fundamental architectural limitation: the security boundary relies on correct configuration across two systems.The documented mitigations are appropriate:
- NetworkPolicy restricts direct access (§3.4)
- Image pinning prevents drift (line 213)
allowArbitraryKubectlCommandstoggle provides a killswitch (lines 214-215)However, consider adding:
- A recommendation to monitor for direct MCP server calls that bypass HolmesGPT (if logging permits)
- A note about the implications if NetworkPolicy is not enforced by the CNI (already noted as "inert where the CNI doesn't enforce" in line 123, but the security implication could be more explicit here)
🤖 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 `@specs/kubernetes-remediation-mcp.md` around lines 210 - 215, The doc currently notes the cross-system approval sync risk and mitigations but should explicitly add two points: (1) recommend operational monitoring for direct MCP server invocations that bypass HolmesGPT—eg. log/alert on calls to run_kubectl_command or unauthenticated access patterns so operators can detect direct MCP server calls; (2) explicitly call out the security impact if NetworkPolicy is not enforced by the CNI (i.e., that NetworkPolicy becomes inert and the approval boundary can be bypassed), and advise mitigations such as enforcing CNI compliance or adding host-level firewall rules; reference HolmesGPT, approval_required_tools, run_kubectl_command and allowArbitraryKubectlCommands when adding these notes.
🤖 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 `@specs/kubernetes-remediation-mcp.md`:
- Around line 157-223: Add a new subsection under "6. Security model — what's
actually enforced" (e.g., 6.6 Audit logging) that documents audit logging
requirements: state whether kubectl executions are logged by HolmesGPT, the MCP
server, and/or rely on Kubernetes audit logs; enumerate captured events (command
invocation, tool name, user/actor, approval decisions, policy/deny hits,
container/pod targets, and execution outcomes); specify retention and
access/monitoring recommendations (retention period, alerting for anomalous
patterns, and where logs are stored/forwarded); and note any gaps or
dependencies (e.g., server must forward to cluster audit or centralized logging)
so the existing sections like "6.4 Residual risks" and "6.5 Things that are
correct by construction" can reference audit mitigations.
- Around line 244-252: Document and track the missing LLM eval coverage
described in §8 by opening a tracking issue that (1) prioritizes writing evals
for the auto-approved tools first (they require no approval hook), (2) specifies
the harness enhancement needed to run approval-gated tools non-interactively (so
run_kubectl_command can be exercised in regression suites), and (3) adds a short
acceptance checklist to the spec listing required LLM checks (tool selection,
command safety, and mutation approval flow); include links to the existing unit
tests that validate authorization so the issue clearly separates auth tests from
desired LLM behavior tests.
---
Nitpick comments:
In `@specs/kubernetes-remediation-mcp.md`:
- Around line 192-199: Add a short risk note referencing §6.3's
run_diagnostic_image hardening that explains the potential for resource
exhaustion if many diagnostic pods are launched concurrently and lists
mitigations to include in §6.4 — mention the existing 60s timeout and per-pod
memory caps, and recommend server-side concurrency/rate limits, namespace
ResourceQuota usage, and limiting total concurrent diagnostic pods (or queuing)
in the MCP server; tie these recommendations to the run_diagnostic_image
behavior so readers can correlate mitigations with the implemented limits.
- Around line 210-215: The doc currently notes the cross-system approval sync
risk and mitigations but should explicitly add two points: (1) recommend
operational monitoring for direct MCP server invocations that bypass
HolmesGPT—eg. log/alert on calls to run_kubectl_command or unauthenticated
access patterns so operators can detect direct MCP server calls; (2) explicitly
call out the security impact if NetworkPolicy is not enforced by the CNI (i.e.,
that NetworkPolicy becomes inert and the approval boundary can be bypassed), and
advise mitigations such as enforcing CNI compliance or adding host-level
firewall rules; reference HolmesGPT, approval_required_tools,
run_kubectl_command and allowArbitraryKubectlCommands when adding these notes.
🪄 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: f7b12ad7-a65a-403d-aa99-64e200989b4d
📒 Files selected for processing (2)
docs/data-sources/builtin-toolsets/kubernetes-remediation-mcp.mdspecs/kubernetes-remediation-mcp.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/data-sources/builtin-toolsets/kubernetes-remediation-mcp.md
Master's multi_instance.py (merged in) propagated the now-removed Toolset.restricted_tools to child toolsets in _forward_overrides, which broke all multi-instance toolset tests (azure_sql/datadog/elasticsearch/...) once this branch's removal met it. Drop the restricted_tools propagation (keep approval_required_tools) and fix the _RoutingTool docstring accordingly. Signed-off-by: Claude <noreply@anthropic.com>
…ecklist Addresses CodeRabbit review on specs/kubernetes-remediation-mcp.md: - §6.6 Audit logging: what the server/HolmesGPT/k8s capture today and the forwarding/retention/alerting gaps. - §6.4: resource-exhaustion risk from concurrent diagnostic pods (+ mitigations), and the NetworkPolicy-inert-without-CNI / direct-call monitoring implications of the cross-system approval boundary. - §8: eval acceptance checklist separating authorization tests from LLM-behavior evals, prioritizing the auto-approved tools and noting the harness hook the gated tool needs. Signed-off-by: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
specs/kubernetes-remediation-mcp.md (2)
223-230: Resource exhaustion mitigations documented as gap.This section honestly documents that global concurrency/rate limits and ResourceQuota/LimitRange are "not yet implemented" and recommends them for multi-tenant or resource-constrained clusters. The risk is clearly described.
Unlike the eval coverage (§8), this section doesn't state whether these mitigations are tracked or planned. For a tool that can launch multiple diagnostic pods, this is a real operational concern for production deployments.
Consider adding a note indicating whether these mitigations are tracked (similar to the "(tracked separately — not blocking this PR)" note in §8) to help operators understand the roadmap.
🤖 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 `@specs/kubernetes-remediation-mcp.md` around lines 223 - 230, The doc notes global concurrency/rate limits and namespace ResourceQuota/LimitRange for run_diagnostic_image are "not yet implemented" but doesn't state whether these mitigations are tracked or planned; update the paragraph that mentions run_diagnostic_image and the recommended ResourceQuota/LimitRange to include a short status note (e.g. "(tracked separately — planned/issue #...)" or "(not currently tracked)") matching the style used in §8 so operators know the roadmap and whether this is blocking.
217-219: Consider adding more specific CNI guidance.The text correctly documents that NetworkPolicy enforcement depends on CNI support and recommends "enforce a NetworkPolicy-capable CNI (or add host/firewall-level restrictions)". For operators evaluating this addon, it might be helpful to briefly enumerate which CNIs support NetworkPolicy (e.g., Calico, Cilium, Weave Net vs. Flannel without additional components) or provide a detection recommendation.
However, this level of detail may be out of scope for a design spec. The critical point—that the approval boundary can be bypassed where NetworkPolicy isn't enforced—is clearly stated.
🤖 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 `@specs/kubernetes-remediation-mcp.md` around lines 217 - 219, Add a short, optional paragraph after the sentence about "enforce a NetworkPolicy-capable CNI (or add host/firewall-level restrictions)" that (1) lists common CNIs that do and do not enforce NetworkPolicy by default (e.g., Calico, Cilium, Weave Net support NetworkPolicy enforcement; Flannel does not without extra components) and (2) suggests a lightweight detection method such as checking for known CNI DaemonSets/Deployments (calico-node, cilium, weave-net) or performing a simple NetworkPolicy enforcement test (create an isolated deny/allow policy and verify connectivity) so operators can quickly validate whether their CNI enforces NetworkPolicy.
🤖 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/plugins/toolsets/multi_instance.py`:
- Around line 97-100: The current check uses "if not raw:" which treats an
explicit empty list for "instances" the same as a missing key and produces a
synthetic "default" instance; instead check for presence vs absence by using "if
raw is None:" (or "if 'instances' not in config:") so an explicit empty list is
preserved and will flow into _aggregate()'s "No instances configured" path;
update the conditional around raw/instances in multi_instance.py (the block that
builds flat and returns [("default", flat)]) to only run when raw is truly
missing, leaving raw == [] untouched.
---
Nitpick comments:
In `@specs/kubernetes-remediation-mcp.md`:
- Around line 223-230: The doc notes global concurrency/rate limits and
namespace ResourceQuota/LimitRange for run_diagnostic_image are "not yet
implemented" but doesn't state whether these mitigations are tracked or planned;
update the paragraph that mentions run_diagnostic_image and the recommended
ResourceQuota/LimitRange to include a short status note (e.g. "(tracked
separately — planned/issue #...)" or "(not currently tracked)") matching the
style used in §8 so operators know the roadmap and whether this is blocking.
- Around line 217-219: Add a short, optional paragraph after the sentence about
"enforce a NetworkPolicy-capable CNI (or add host/firewall-level restrictions)"
that (1) lists common CNIs that do and do not enforce NetworkPolicy by default
(e.g., Calico, Cilium, Weave Net support NetworkPolicy enforcement; Flannel does
not without extra components) and (2) suggests a lightweight detection method
such as checking for known CNI DaemonSets/Deployments (calico-node, cilium,
weave-net) or performing a simple NetworkPolicy enforcement test (create an
isolated deny/allow policy and verify connectivity) so operators can quickly
validate whether their CNI enforces NetworkPolicy.
🪄 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: 23ede510-55f1-49ed-8098-057273d8c4c7
📒 Files selected for processing (3)
helm/holmes/values.yamlholmes/plugins/toolsets/multi_instance.pyspecs/kubernetes-remediation-mcp.md
🚧 Files skipped from review as they are similar to previous changes (1)
- helm/holmes/values.yaml
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (2)
specs/kubernetes-remediation-mcp.md (2)
223-230: Resource exhaustion mitigations documented as gap.This section honestly documents that global concurrency/rate limits and ResourceQuota/LimitRange are "not yet implemented" and recommends them for multi-tenant or resource-constrained clusters. The risk is clearly described.
Unlike the eval coverage (§8), this section doesn't state whether these mitigations are tracked or planned. For a tool that can launch multiple diagnostic pods, this is a real operational concern for production deployments.
Consider adding a note indicating whether these mitigations are tracked (similar to the "(tracked separately — not blocking this PR)" note in §8) to help operators understand the roadmap.
🤖 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 `@specs/kubernetes-remediation-mcp.md` around lines 223 - 230, The doc notes global concurrency/rate limits and namespace ResourceQuota/LimitRange for run_diagnostic_image are "not yet implemented" but doesn't state whether these mitigations are tracked or planned; update the paragraph that mentions run_diagnostic_image and the recommended ResourceQuota/LimitRange to include a short status note (e.g. "(tracked separately — planned/issue #...)" or "(not currently tracked)") matching the style used in §8 so operators know the roadmap and whether this is blocking.
217-219: Consider adding more specific CNI guidance.The text correctly documents that NetworkPolicy enforcement depends on CNI support and recommends "enforce a NetworkPolicy-capable CNI (or add host/firewall-level restrictions)". For operators evaluating this addon, it might be helpful to briefly enumerate which CNIs support NetworkPolicy (e.g., Calico, Cilium, Weave Net vs. Flannel without additional components) or provide a detection recommendation.
However, this level of detail may be out of scope for a design spec. The critical point—that the approval boundary can be bypassed where NetworkPolicy isn't enforced—is clearly stated.
🤖 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 `@specs/kubernetes-remediation-mcp.md` around lines 217 - 219, Add a short, optional paragraph after the sentence about "enforce a NetworkPolicy-capable CNI (or add host/firewall-level restrictions)" that (1) lists common CNIs that do and do not enforce NetworkPolicy by default (e.g., Calico, Cilium, Weave Net support NetworkPolicy enforcement; Flannel does not without extra components) and (2) suggests a lightweight detection method such as checking for known CNI DaemonSets/Deployments (calico-node, cilium, weave-net) or performing a simple NetworkPolicy enforcement test (create an isolated deny/allow policy and verify connectivity) so operators can quickly validate whether their CNI enforces NetworkPolicy.
🤖 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/plugins/toolsets/multi_instance.py`:
- Around line 97-100: The current check uses "if not raw:" which treats an
explicit empty list for "instances" the same as a missing key and produces a
synthetic "default" instance; instead check for presence vs absence by using "if
raw is None:" (or "if 'instances' not in config:") so an explicit empty list is
preserved and will flow into _aggregate()'s "No instances configured" path;
update the conditional around raw/instances in multi_instance.py (the block that
builds flat and returns [("default", flat)]) to only run when raw is truly
missing, leaving raw == [] untouched.
---
Nitpick comments:
In `@specs/kubernetes-remediation-mcp.md`:
- Around line 223-230: The doc notes global concurrency/rate limits and
namespace ResourceQuota/LimitRange for run_diagnostic_image are "not yet
implemented" but doesn't state whether these mitigations are tracked or planned;
update the paragraph that mentions run_diagnostic_image and the recommended
ResourceQuota/LimitRange to include a short status note (e.g. "(tracked
separately — planned/issue #...)" or "(not currently tracked)") matching the
style used in §8 so operators know the roadmap and whether this is blocking.
- Around line 217-219: Add a short, optional paragraph after the sentence about
"enforce a NetworkPolicy-capable CNI (or add host/firewall-level restrictions)"
that (1) lists common CNIs that do and do not enforce NetworkPolicy by default
(e.g., Calico, Cilium, Weave Net support NetworkPolicy enforcement; Flannel does
not without extra components) and (2) suggests a lightweight detection method
such as checking for known CNI DaemonSets/Deployments (calico-node, cilium,
weave-net) or performing a simple NetworkPolicy enforcement test (create an
isolated deny/allow policy and verify connectivity) so operators can quickly
validate whether their CNI enforces NetworkPolicy.
🪄 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: 23ede510-55f1-49ed-8098-057273d8c4c7
📒 Files selected for processing (3)
helm/holmes/values.yamlholmes/plugins/toolsets/multi_instance.pyspecs/kubernetes-remediation-mcp.md
🚧 Files skipped from review as they are similar to previous changes (1)
- helm/holmes/values.yaml
🛑 Comments failed to post (1)
holmes/plugins/toolsets/multi_instance.py (1)
97-100:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't collapse
instances: []into flat single-instance mode.
if not raw:treats an explicit empty list the same as a missinginstanceskey, soinstances: []silently creates a synthetic"default"child from the top-level globals. That bypasses the"No instances configured"path in_aggregate()and can unexpectedly leave a routable endpoint enabled when the config explicitly declared zero instances.Suggested fix
def _parse_instances(config: Dict[str, Any]) -> List[Tuple[str, Dict[str, Any]]]: """Decompose a wrapper config into ordered (instance_name, flat_child_config) pairs. Flat config (no `instances:`) → one instance named `default`. """ raw = config.get("instances") - if not raw: + if raw is None: flat = {k: v for k, v in config.items() if k != "instances"} return [("default", flat)] if not isinstance(raw, list): raise ValueError("`instances` must be a list") + if not raw: + return []🤖 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 97 - 100, The current check uses "if not raw:" which treats an explicit empty list for "instances" the same as a missing key and produces a synthetic "default" instance; instead check for presence vs absence by using "if raw is None:" (or "if 'instances' not in config:") so an explicit empty list is preserved and will flow into _aggregate()'s "No instances configured" path; update the conditional around raw/instances in multi_instance.py (the block that builds flat and returns [("default", flat)]) to only run when raw is truly missing, leaving raw == [] untouched.
… note Address CodeRabbit nitpicks on the k8s-remediation spec (§6.4): - Enumerate which CNIs enforce NetworkPolicy (Calico/Cilium/Antrea/Weave Net vs plain Flannel) plus managed-cluster caveats and a quick detection method, so operators can validate the approval-boundary backstop. - Add a tracking-status note to the resource-exhaustion mitigations matching the §8 style (ResourceQuota/LimitRange available today; server-side concurrency cap is a follow-up, not blocking this PR). Signed-off-by: Claude <noreply@anthropic.com>
…uth/steer-away framing The page was underselling the remediation MCP and steering readers toward the default toolset: - Dropped the shared kubernetes_toolset_picker snippet here (its OAuth/OIDC comparison is relevant on the kubernetes-mcp page, not this one) and replaced it with capability-first positioning. - Lead with what this MCP adds over read-only access (act on the cluster + deeper in-container diagnostics), with a comparison table. - Removed the 'Don't use this server for reads' / 'Prefer the no-approval tools' lines that read as discouragement; reframed the built-in toolset as complementary and the approval split as a positive design property. Signed-off-by: Claude <noreply@anthropic.com>
…ostic_image Track the server-side tool rename (#24) so the docs, spec, Helm LLM instructions, and unit tests reference the tool by its new name, consistent with run_preapproved_kubectl_command. 11 unit tests pass. Signed-off-by: Claude <noreply@anthropic.com>
arikalon1
approved these changes
Jun 24, 2026
arikalon1
enabled auto-merge (squash)
June 24, 2026 23:28
This was referenced Jul 12, 2026
4 of 5 tasks
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
3 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.
Summary
Replaces the legacy
restricted_toolsmechanism with a cleaner approval-based model for the Kubernetes Remediation MCP server. Tools are now categorized as either auto-approved (read-only diagnostics) or approval-gated (mutations), eliminating the need for dual-layer authorization and making approval intent explicit in the toolset configuration.Key Changes
Core Tool Framework (
holmes/core/tools.py)Tool.restrictedfield and_is_restricted()methodToolset.restricted_toolslist fieldrestricted_toolsconfig and silently ignore itapproval_required_toolspatternsTool Execution (
holmes/core/tool_calling_llm.py,holmes/core/tools_utils/tool_executor.py)_skill_in_usetransient state tracking (no longer needed)_should_include_restricted_tools()andinclude_restrictedparameterget_all_tools_openai_format()reset_interaction_state()to a no-op hookKubernetes Remediation MCP Documentation & Helm
Updated
docs/data-sources/builtin-toolsets/kubernetes-remediation-mcp.md:restricted_toolsand dual-layer authorizationHelm values (
helm/holmes/values.yaml):restrictedToolsconfigurationapprovalRequiredToolsto only list["run_kubectl_command"]preapprovedCommands,diagnosticImages,fileReadAllowedPaths,fileReadDeniedPaths,allowArbitraryKubectlCommandsclusterRolefrom"cluster-admin"to""(chart creates scoped role)networkPolicyby default1.1.0New Helm template
helm/holmes/templates/mcp-servers/kubernetes-remediation/rbac.yaml:cluster-admin, nosecretsaccess)serviceAccount.clusterRoleis emptyUpdated Helm deployment (
helm/holmes/templates/mcp-servers/kubernetes-remediation/deployment.yaml):KUBECTL_ALLOWED_IMAGES(replaced byKUBECTL_DIAGNOSTIC_IMAGES)cluster-adminUpdated LLM instructions (
helm/holmes/templates/mcp-servers/kubernetes-remediation/_helpers.tpl):CLI Documentation
docs/data-sources/builtin-toolsets/kubernetes-remediation-mcp.mdwith:cluster-admin)Tests
tests/test_approval_required_tools.py: Validates approval-based tool separation modelrestricted_toolsconfighttps://claude.ai/code/session_01FoqM3sjnrRgPjdqYdqutzK
Summary by CodeRabbit
New Features
Security Improvements
Documentation
Tests