Repository navigation
ROB-4013 MCP auth health check: auto-detect + enable GitHub context (supersedes #2117) - #2118
Merged
Merged
Conversation
MCP servers may return a tool list even with invalid credentials (e.g., bad GitHub token), causing them to appear "connected" when authentication is actually broken. This adds an optional health_check_tool config field that specifies a read-only tool to invoke during health check. If the tool call fails (e.g., 401 Unauthorized), the MCP server is marked as unhealthy instead of connected. - Add health_check_tool field to MCPConfig and StdioMCPConfig - Implement _run_health_check_tool() to invoke the tool after loading - Enable health_check_tool: get_me for GitHub MCP in helm chart - Enable health_check_tool: get_current_user for GitLab MCP - Add comprehensive unit tests for the new functionality Slack: https://robustaco.slack.com/archives/C047B4M8Y7K/p1780404981710549 https://claude.ai/code/session_01Cmrtm14E7JiDsG3DnQwCAJ Signed-off-by: Claude <noreply@anthropic.com>
- Add warning logs for health check failures and exceptions - Use existing _extract_text_from_content_block helper for error extraction - Update health_check_tool examples to use lightweight tools only - Fix unused variable warnings in tests (msg -> _) https://claude.ai/code/session_01Cmrtm14E7JiDsG3DnQwCAJ Signed-off-by: Claude <noreply@anthropic.com>
MCP servers (e.g. GitHub) return their full tool list even when the provided credential is invalid, so listing tools alone never proves the token works. PR #2117 added an opt-in `health_check_tool` config field, but Robusta-generated toolset configs do not set it, so a bad GitHub PAT still shows the toolset as enabled in HolmesToolsStatus. This makes the auth health check automatic: when no `health_check_tool` is configured, fall back to a small allowlist of well-known read-only identity tools (get_me, get_current_user, get_authenticated_user, whoami). If the server exposes one, invoke it with empty args during prerequisite evaluation. A bad token then fails the health check and the toolset is correctly marked disabled, with the underlying error surfaced. An explicitly configured `health_check_tool` still takes precedence. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: alonelish <alon.elish@gmail.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
|
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 implements MCP health-check tool auto-detection to validate server authentication at startup. It adds a configurable health-check tool mechanism with automatic fallback to known identity tools, comprehensive test coverage for all code paths, and user documentation with updated Helm defaults. ChangesMCP Health Check Tool Auto-Detection
Sequence DiagramsNone applicable—this PR extends existing initialization and execution paths with tool selection and invocation logic that is straightforward for reference in code. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![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: 1
🤖 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/mcp/toolset_mcp.py`:
- Around line 965-973: The health-check path currently auto-selects a tool via
health_check_tool_name = self._mcp_config.health_check_tool or
self._auto_detect_health_check_tool() but then raises generic "health check
failed - ..." errors; update all prerequisite/health-check failure handling
(wherever the health-check invocation and error is raised) to include the
selected tool name (health_check_tool_name) and the exact invocation parameters
used (e.g., tool identifier, any query/command, time ranges, filters, and the
full API error/response). Ensure the error message construction includes
health_check_tool_name and the parameters/response objects (rather than
swallowing them) so callers receive the tool name and full detail per the
toolset contract; apply the same change to the other health-check block(s)
referenced around the auto-detect logic.
🪄 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: 18e205c6-5219-43cd-95cb-bd9ba8f977c0
📒 Files selected for processing (2)
holmes/plugins/toolsets/mcp/toolset_mcp.pytests/test_mcp_toolset.py
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:f8dcbb97
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:f8dcbb97 me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:f8dcbb97
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:f8dcbb97
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:f8dcbb97
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:f8dcbb97 me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:f8dcbb97
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:f8dcbb97Patch 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:f8dcbb97 \
--set operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set operator.image=holmes-operator-dev:f8dcbb97Robusta 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:f8dcbb97 \
--set holmes.operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.operator.image=holmes-operator-dev:f8dcbb97 |
The check_first_item_role validator on ChatRequestBaseModel is a Pydantic mode="before" validator, so it receives the raw input rather than a guaranteed dict. FastAPI hands it the raw request body (bytes) instead of a parsed dict when the client omits the "Content-Type: application/json" header, which crashed with "'bytes' object has no attribute 'get'" and surfaced as a 500 ASGI exception during data-source chat sessions. The validator now JSON-parses bytes/str input (so a valid body still works without the header) and returns non-dict input untouched (so a genuinely malformed body yields a clean Pydantic 422 instead of a 500). Adds tests/core/test_models.py covering dict input, bytes/str JSON bodies, system-role enforcement via the bytes path, and the garbage-bytes -> clean ValidationError case. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: alonelish <alon.elish@gmail.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
holmes/core/models.py (1)
298-307:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
conversation_history[0]before calling.get().This still throws a server-side
AttributeErrorfor payloads like{"conversation_history": ["bad"]}. Afterjson.loads,conversation_historycan be a list whose first item is not a dict, so Line 307 crashes before Pydantic can return a cleanValidationError.Proposed fix
conversation_history = values.get("conversation_history") if ( conversation_history and isinstance(conversation_history, list) and len(conversation_history) > 0 ): first_item = conversation_history[0] + if not isinstance(first_item, dict): + return values if not first_item.get("role") == "system": raise ValueError( "The first item in conversation_history must contain 'role': 'system'" )🤖 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/models.py` around lines 298 - 307, The code assumes conversation_history[0] is a dict and calls .get() which raises AttributeError for non-dict items (e.g., ["bad"]); update the guard in the validator in holmes/core/models.py to check the type of first_item before using .get(): ensure first_item is a dict (isinstance(first_item, dict)) and only then inspect first_item.get("role") (or treat non-dict entries as invalid/skip them) so the function returns a ValidationError path instead of crashing.
🤖 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.
Outside diff comments:
In `@holmes/core/models.py`:
- Around line 298-307: The code assumes conversation_history[0] is a dict and
calls .get() which raises AttributeError for non-dict items (e.g., ["bad"]);
update the guard in the validator in holmes/core/models.py to check the type of
first_item before using .get(): ensure first_item is a dict
(isinstance(first_item, dict)) and only then inspect first_item.get("role") (or
treat non-dict entries as invalid/skip them) so the function returns a
ValidationError path instead of crashing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 82fdfd38-96d1-4b5e-ac20-7acfe2384d44
📒 Files selected for processing (2)
holmes/core/models.pytests/core/test_models.py
The MCP auth health check auto-detects a read-only identity tool (get_me, get_current_user, ...) to validate credentials. But the GitHub MCP server only exposes get_me when its 'context' toolset is enabled, and the chart default for mcpAddons.github.config.toolsets was "repos,issues,pull_requests,actions" — which excludes 'context'. So get_me was never present, auto-detection found nothing, the health check was silently skipped, and a bad PAT still showed the toolset as enabled. - Add 'context' to the default GitHub toolsets so get_me is exposed and a bad token is caught at startup. 'context' is read-only and lightweight. - Log (debug) in _auto_detect_health_check_tool when no identity tool is exposed, so a skipped health check is diagnosable instead of silent. - Add a test asserting the skipped-check log is emitted. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: alonelish <alon.elish@gmail.com>
alonelish
changed the title
alonelish
changed the title
- MCP health check failures now include the tool name and params in the
error message ("health check tool '<name>' with params {} failed - ...")
so an auto-detected check is not opaque, per the toolset error-detail
contract. Updated the matching test assertions.
- check_first_item_role: guard against a conversation_history whose first
item is not a dict (e.g. {"conversation_history": ["bad"]}), which would
otherwise crash on first_item.get(). Now returns the value untouched so
Pydantic raises a clean ValidationError instead of a 500. Added a test.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: alonelish <alon.elish@gmail.com>
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
📂 Previous Runs📜 #2 · Run @ __a299234__ (#26866469577) — Jun 3, 06:01 UTC✅ Results of HolmesGPT evalsAutomatically triggered by commit a299234 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:
|
| Status | Test case | Time | Turns | Tools | Cost | Total tokens | Input | Max input | Output | Max output | Cached | Non-cached | Reasoning | Compactions | Src |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| ✅ | 09_crashpod | 43.4s | 5 | 11 | $0.2866 | 108,591 | 106,056 | 24,312 | 2,535 | 1,128 | 80,783 | 25,273 | 344 | — | src |
| ✅ | 101_loki_historical_logs_pod_deleted | 87.7s | 7 | 17 | $0.4395 | 175,736 | 170,179 | 29,683 | 5,557 | 1,014 | 139,426 | 30,753 | 1,408 | — | src |
| ✅ | 112_find_pvcs_by_uuid | 21.1s | 3 | 4 | $0.2051 | 61,048 | 59,852 | 21,821 | 1,196 | 591 | 37,774 | 22,078 | 285 | — | src |
| ✅ | 12_job_crashing | 43.8s | 7 | 14 | $0.3169 | 156,356 | 153,819 | 24,776 | 2,537 | 865 | 127,866 | 25,953 | 156 | — | src |
| ✅ | 176_network_policy_blocking_traffic_no_skills | 58.5s | 5 | 14 | $0.3214 | 114,316 | 111,070 | 26,032 | 3,246 | 921 | 83,736 | 27,334 | 659 | — | src |
| ✅ | 227_count_configmaps_per_namespace[0] | 21.7s | 4 | 9 | $0.2032 | 76,691 | 75,566 | 20,659 | 1,125 | 591 | 54,902 | 20,664 | 53 | — | src |
| ✅ | 243_pod_names_contain_service | 39.5s | 4 | 10 | $0.2533 | 81,987 | 79,777 | 22,593 | 2,210 | 921 | 56,026 | 23,751 | 260 | — | src |
| ✅ | 24_misconfigured_pvc | 45.5s | 5 | 16 | $0.3053 | 110,319 | 107,408 | 24,870 | 2,911 | 1,097 | 80,745 | 26,663 | 216 | — | src |
| ✅ | 43_current_datetime_from_prompt | 6.1s | 1 | — | $0.1189 | 16,999 | 16,899 | 16,899 | 100 | 100 | 0 | 16,899 | 60 | — | src |
| ✅ | 51_logs_summarize_errors | 22.6s | 4 | 5 | $0.2025 | 77,377 | 76,395 | 21,096 | 982 | 328 | 55,294 | 21,101 | 32 | — | src |
| ✅ | 61_exact_match_counting | 12.7s | 3 | 3 | $0.1518 | 52,722 | 52,360 | 17,873 | 362 | 215 | 34,483 | 17,877 | 31 | — | src |
| Total | 36.6s avg | 4.4 avg | 10.3 avg | $2.8045 | 1,032,142 | 1,009,381 | 29,683 | 22,761 | 1,128 | 751,035 | 258,346 | 3,504 | — |
Benchmark Comparison Details
Master baseline: latest master-* experiment (post-merge regression eval)
Status: 11 test/model combinations loaded
- master-26866452925 (created: 2026-06-03)
Benchmark baseline: latest ci-benchmark experiment on master
Status: 177 test/model combinations loaded
- ci-benchmark-26701915314 (created: 2026-05-31)
Time comparison (seconds):
| Test case | This branch | master (1h ago) | Δ vs master | benchmark (3d ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | 43.4s | 39.5s | ±0% | 45.0s | ±0% |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | 87.7s | 80.6s | ±0% | 85.9s | ±0% |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | 21.1s | 20.2s | ±0% | 21.4s | ±0% |
| 12_job_crashing (opus-4.6) 📄 | 43.8s | 43.3s | ±0% | 38.6s | ↑13% |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | 58.5s | 43.8s | ↑34% | 53.9s | ±0% |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | 21.7s | 19.5s | ↑11% | 20.8s | ±0% |
| 243_pod_names_contain_service (opus-4.6) 📄 | 39.5s | 34.5s | ↑15% | 33.6s | ↑18% |
| 24_misconfigured_pvc (opus-4.6) 📄 | 45.5s | 43.2s | ±0% | 37.0s | ↑23% |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | 6.1s | 3.5s | ↑74% | 3.6s | ↑71% |
| 51_logs_summarize_errors (opus-4.6) 📄 | 22.6s | 21.5s | ±0% | 21.9s | ±0% |
| 61_exact_match_counting (opus-4.6) 📄 | 12.7s | 7.0s | ↑80% | 10.0s | ↑27% |
| Total (all, n=11) | 36.6s | 32.4s | — | 33.8s | — |
| Comparable (m=11, b=11) | 36.6s | 32.4s | ↑13% | 33.8s | ±0% |
Cost comparison:
| Test case | This branch | master (1h ago) | Δ vs master | benchmark (3d ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | $0.2866 | $0.2846 | ±0% | $0.3318 | ↓14% |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | $0.4395 | $0.4337 | ±0% | $0.4631 | ±0% |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | $0.2051 | $0.2067 | ±0% | $0.2066 | ±0% |
| 12_job_crashing (opus-4.6) 📄 | $0.3169 | $0.3157 | ±0% | $0.3107 | ±0% |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | $0.3214 | $0.3128 | ±0% | $0.3426 | ±0% |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | $0.2032 | $0.2060 | ±0% | $0.2031 | ±0% |
| 243_pod_names_contain_service (opus-4.6) 📄 | $0.2533 | $0.2613 | ±0% | $0.2521 | ±0% |
| 24_misconfigured_pvc (opus-4.6) 📄 | $0.3053 | $0.2987 | ±0% | $0.2904 | ±0% |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | $0.1189 | $0.0121 | ↑882% | $0.1187 | ±0% |
| 51_logs_summarize_errors (opus-4.6) 📄 | $0.2025 | $0.2022 | ±0% | $0.2015 | ±0% |
| 61_exact_match_counting (opus-4.6) 📄 | $0.1518 | $0.1337 | ↑14% | $0.1518 | ±0% |
| Total (all, n=11) | $0.2550 | $0.2425 | — | $0.2611 | — |
| Comparable (m=11, b=11) | $0.2550 | $0.2425 | ±0% | $0.2611 | ±0% |
Total tokens comparison:
| Test case | This branch | master (1h ago) | Δ vs master | benchmark (3d ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | 108,591 | 108,307 | ±0% | 156,516 | ↓31% |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | 175,736 | 176,746 | ±0% | 204,760 | ↓14% |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | 61,048 | 61,245 | ±0% | 61,198 | ±0% |
| 12_job_crashing (opus-4.6) 📄 | 156,356 | 136,243 | ↑15% | 135,931 | ↑15% |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | 114,316 | 113,471 | ±0% | 140,458 | ↓19% |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | 76,691 | 76,813 | ±0% | 76,661 | ±0% |
| 243_pod_names_contain_service (opus-4.6) 📄 | 81,987 | 103,798 | ↓21% | 82,231 | ±0% |
| 24_misconfigured_pvc (opus-4.6) 📄 | 110,319 | 107,999 | ±0% | 107,468 | ±0% |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | 16,999 | 17,001 | ±0% | 16,989 | ±0% |
| 51_logs_summarize_errors (opus-4.6) 📄 | 77,377 | 77,181 | ±0% | 76,727 | ±0% |
| 61_exact_match_counting (opus-4.6) 📄 | 52,722 | 34,378 | ↑53% | 52,723 | ±0% |
| Total (all, n=11) | 93,831 | 92,107 | — | 101,060 | — |
| Comparable (m=11, b=11) | 93,831 | 92,107 | ±0% | 101,060 | ±0% |
Cached tokens comparison:
| Test case | This branch | master (1h ago) | Δ vs master | benchmark (3d ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | 80,783 | 80,592 | ±0% | 126,090 | ↓36% |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | 139,426 | 138,229 | ±0% | 166,234 | ↓16% |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | 37,774 | 37,829 | ±0% | 37,847 | ±0% |
| 12_job_crashing (opus-4.6) 📄 | 127,866 | 106,474 | ↑20% | 106,794 | ↑20% |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | 83,736 | 82,406 | ±0% | 108,926 | ↓23% |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | 54,902 | 54,645 | ±0% | 54,888 | ±0% |
| 243_pod_names_contain_service (opus-4.6) 📄 | 56,026 | 78,182 | ↓28% | 56,547 | ±0% |
| 24_misconfigured_pvc (opus-4.6) 📄 | 80,745 | 79,037 | ±0% | 78,838 | ±0% |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | — | 16,896 | — | — | — |
| 51_logs_summarize_errors (opus-4.6) 📄 | 55,294 | 55,186 | ±0% | 54,936 | ±0% |
| 61_exact_match_counting (opus-4.6) 📄 | 34,483 | 16,905 | ↑104% | 34,484 | ±0% |
| Total (all, n=11) | 68,276 | 67,853 | — | 75,053 | — |
| Comparable (m=10, b=10) | 75,104 | 72,948 | ±0% | 82,558 | ±0% |
Turns comparison:
| Test case | This branch | master (1h ago) | Δ vs master | benchmark (3d ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | 5 | 5 | ±0% | 7 | ↓29% |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | 7 | 7 | ±0% | 8 | ↓12% |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | 3 | 3 | ±0% | 3 | ±0% |
| 12_job_crashing (opus-4.6) 📄 | 7 | 6 | ↑17% | 6 | ↑17% |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | 5 | 5 | ±0% | 6 | ↓17% |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | 4 | 4 | ±0% | 4 | ±0% |
| 243_pod_names_contain_service (opus-4.6) 📄 | 4 | 5 | ↓20% | 4 | ±0% |
| 24_misconfigured_pvc (opus-4.6) 📄 | 5 | 5 | ±0% | 5 | ±0% |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | 1 | 1 | ±0% | 1 | ±0% |
| 51_logs_summarize_errors (opus-4.6) 📄 | 4 | 4 | ±0% | 4 | ±0% |
| 61_exact_match_counting (opus-4.6) 📄 | 3 | 2 | ↑50% | 3 | ±0% |
| Total (all, n=11) | 4.4 | 4.3 | — | 4.6 | — |
| Comparable (m=11, b=11) | 4.4 | 4.3 | ±0% | 4.6 | ±0% |
Tool calls comparison:
| Test case | This branch | master (1h ago) | Δ vs master | benchmark (3d ago) | Δ vs benchmark |
|---|---|---|---|---|---|
| 09_crashpod (opus-4.6) 📄 | 11 | 11 | ±0% | 13 | ↓15% |
| 101_loki_historical_logs_pod_deleted (opus-4.6) 📄 | 17 | 18 | ±0% | 18 | ±0% |
| 112_find_pvcs_by_uuid (opus-4.6) 📄 | 4 | 4 | ±0% | 4 | ±0% |
| 12_job_crashing (opus-4.6) 📄 | 14 | 15 | ±0% | 14 | ±0% |
| 176_network_policy_blocking_traffic_no_skills (opus-4.6) 📄 | 14 | 13 | ±0% | 14 | ±0% |
| 227_count_configmaps_per_namespace[0] (opus-4.6) 📄 | 9 | 9 | ±0% | 9 | ±0% |
| 243_pod_names_contain_service (opus-4.6) 📄 | 10 | 10 | ±0% | 10 | ±0% |
| 24_misconfigured_pvc (opus-4.6) 📄 | 16 | 15 | ±0% | 15 | ±0% |
| 43_current_datetime_from_prompt (opus-4.6) 📄 | — | — | — | — | — |
| 51_logs_summarize_errors (opus-4.6) 📄 | 5 | 5 | ±0% | 5 | ±0% |
| 61_exact_match_counting (opus-4.6) 📄 | 3 | 1 | ↑200% | 3 | ±0% |
| Total (all, n=11) | 9.4 | 10.1 | — | 10.5 | — |
| Comparable (m=10, b=10) | 10.3 | 10.1 | ±0% | 10.5 | ±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/pedantic-hertz-7bfea3 -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, 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, 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, haiku-4.5, kimi-2.5, kimi-2.5-openrouter, opus-4.5, opus-4.6, opus-4.7, 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/pedantic-hertz-7bfea3 -f markers=regression -f filter=
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_mcp_toolset.py (1)
2779-2823: ⚡ Quick winConsider adding test coverage for allowlist priority order.
The test suite comprehensively covers health-check scenarios but doesn't explicitly verify that when multiple allowlisted identity tools are exposed (without explicit
health_check_toolconfig), the first tool inDEFAULT_HEALTH_CHECK_TOOLSis chosen.For example, add a test where the server exposes both
get_meandget_current_user, verify thatget_meis auto-selected because it appears first in the allowlist. This would explicitly test the priority order documented in the allowlist.Suggested test case
+ def test_auto_detect_respects_allowlist_priority( + self, monkeypatch, suppress_migration_warnings + ): + """When multiple allowlisted identity tools are exposed, the first in + DEFAULT_HEALTH_CHECK_TOOLS is chosen.""" + toolset = RemoteMCPToolset( + name="github", + description="GitHub MCP", + config={"url": "http://localhost:8000"}, # no health_check_tool + ) + + async def mock_get_server_tools(): + return ListToolsResult( + tools=[ + Tool( + name="get_current_user", # second in allowlist + inputSchema={"type": "object", "properties": {}}, + description="Get current user", + ), + Tool( + name="get_me", # first in allowlist + inputSchema={"type": "object", "properties": {}}, + description="Get authenticated user", + ), + ] + ) + + monkeypatch.setattr(toolset, "_get_server_tools", mock_get_server_tools) + + called_with = {} + + async def mock_call_health_check(tool_name): + called_with["tool_name"] = tool_name + return CallToolResult(content=[TextContent(type="text", text='{"login": "user"}')]) + + monkeypatch.setattr(toolset, "_call_health_check_tool_async", mock_call_health_check) + + ok, _ = toolset.prerequisites_callable(config=toolset.config) + assert ok is True + # Verify get_me is chosen over get_current_user because it's first in the allowlist + assert called_with["tool_name"] == "get_me"🤖 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_mcp_toolset.py` around lines 2779 - 2823, Add a new test in tests/test_mcp_toolset.py that exercises RemoteMCPToolset.prerequisites_callable when no explicit health_check_tool is configured but the server exposes multiple allowlisted identity tools; mock RemoteMCPToolset._get_server_tools to return both "get_me" and another allowlisted tool like "get_current_user", mock _call_health_check_tool_async to capture which tool was invoked, then assert that the chosen tool equals the first entry in DEFAULT_HEALTH_CHECK_TOOLS (e.g., "get_me") and that the health-check path and error message behavior remain unchanged; reference RemoteMCPToolset, DEFAULT_HEALTH_CHECK_TOOLS, _get_server_tools, _call_health_check_tool_async, and prerequisites_callable to locate and implement the test.
🤖 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_mcp_toolset.py`:
- Around line 2779-2823: Add a new test in tests/test_mcp_toolset.py that
exercises RemoteMCPToolset.prerequisites_callable when no explicit
health_check_tool is configured but the server exposes multiple allowlisted
identity tools; mock RemoteMCPToolset._get_server_tools to return both "get_me"
and another allowlisted tool like "get_current_user", mock
_call_health_check_tool_async to capture which tool was invoked, then assert
that the chosen tool equals the first entry in DEFAULT_HEALTH_CHECK_TOOLS (e.g.,
"get_me") and that the health-check path and error message behavior remain
unchanged; reference RemoteMCPToolset, DEFAULT_HEALTH_CHECK_TOOLS,
_get_server_tools, _call_health_check_tool_async, and prerequisites_callable to
locate and implement the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 591de10f-00fa-48d6-8d01-bd0a060a4290
📒 Files selected for processing (4)
holmes/core/models.pyholmes/plugins/toolsets/mcp/toolset_mcp.pytests/core/test_models.pytests/test_mcp_toolset.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/core/test_models.py
- holmes/plugins/toolsets/mcp/toolset_mcp.py
The check_first_item_role / bytes-body fix (holmes/core/models.py and tests/core/test_models.py) is unrelated to the MCP auth-health-check work in this PR, so it has been split out into #2120 to keep each PR focused. This commit reverts those two files back to master state here; the MCP changes (toolset_mcp.py, values.yaml, toolset-config.yaml, test_mcp_toolset.py) remain. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: alonelish <alon.elish@gmail.com>
alonelish
changed the title
Adds a "Validating Authentication at Startup" subsection to the remote MCP servers page covering the auto-detected identity tools (get_me, get_current_user, get_authenticated_user, whoami) and the explicit health_check_tool config field, including the read-only / no-required-args requirement and the skip-when-absent behavior. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: alonelish <alon.elish@gmail.com>
test_skipped_health_check_is_logged asserted on a caplog-captured DEBUG record, but the diagnostic log is emitted via the root logger, so capture depends on global logging state (e.g. a logging.disable() left by another test, or xdist worker ordering). It passed on 3.11 but failed on 3.12. Replace it with test_auto_detect_returns_none_when_no_identity_tool, which asserts the underlying behavior (_auto_detect_health_check_tool() returns None when the server exposes no allowlisted identity tool) — the condition that drives both the skip and the log — deterministically, with no logging capture. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: alonelish <alon.elish@gmail.com>
…into claude/pedantic-hertz-7bfea3
RoiGlinik
approved these changes
Jun 3, 2026
This was referenced Jun 5, 2026
5 tasks done
moshemorad
added a commit
that referenced
this pull request
Jun 25, 2026
## Summary GitHub App installation tokens 403 on `GET /user`, so the `get_me` health check added in #2118 always fails for the `github-app-mcp` addon. Holmes then disables the toolset on startup — making the GitHub App MCP unusable since 2026-06-03. This PR fixes it on the chart side, scoped to the App-auth branch: - `helm/holmes/templates/toolset-config.yaml`: drop the hardcoded `health_check_tool: get_me` for the App-auth `$githubConfig`. - `helm/holmes/templates/mcp-servers/github/deployment.yaml`: strip `context` from `GITHUB_TOOLSETS` so the upstream server doesn't advertise `get_me` to Holmes auto-detect. - `helm/holmes/values.yaml`: comment note that 'context' is auto-stripped for App auth. Net effect for App auth: Holmes auto-detect finds no identity tool, skips the auth health check (`holmes/plugins/toolsets/mcp/toolset_mcp.py:1004-1010`), and the toolset comes up enabled. PAT auth is untouched — `get_me` is a valid token-validity probe there and is preserved. ## Why a values.yaml workaround alone doesn't work I initially suggested dropping `context` via a values override. That doesn't fix it: the chart hardcodes `health_check_tool: get_me`, which then fails with "tool not found" instead of 403 — toolset still disabled. The hardcoded health_check_tool also can't be set to empty from values: sprig `merge` treats `""` as unset, so the chart value wins. The chart fix is the only working path. ## Test plan - [x] `helm template` renders correctly for App auth (no `context`, no `health_check_tool`) - [x] `helm template` renders correctly for PAT auth (context preserved, `health_check_tool: get_me` preserved) - [x] Edge case: `toolsets: "repos,context,issues"` for App auth renders cleanly as `"repos,issues"` (no double comma) - [x] End-to-end reproduction on a real cluster using a junk PAT + default chart: Holmes logs `❌ Toolset github: ... health check failed (tool: get_me): ... 401 Bad credentials` and lists `github` in Disabled toolsets — same Holmes codepath the customer hits with 403 - [x] End-to-end fix verified on the same cluster by patching the in-cluster ConfigMaps to simulate the chart change: Holmes logs `✅ Toolset github`, `github` moves to Enabled toolsets 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved GitHub addon configuration by automatically cleaning the selected toolsets before applying them, avoiding invalid combinations. * Skips an auth health check that isn’t supported for GitHub App authentication, reducing avoidable 403 errors during startup or validation. * **Documentation** * Updated the Helm values notes to clarify that, with GitHub App auth, the `context` toolset is automatically stripped and why the `get_me` probe is omitted. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Roi Glinik <groi.tech@gmail.com> Co-authored-by: moshemorad <moshemorad12340@gmail.com>
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
Makes MCP toolset authentication validation work end-to-end.
From #2117 (@aantn) —
health_check_toolfoundationhealth_check_toolconfig field to MCP toolsets. MCP servers (e.g. GitHub) return their full tool list even with an invalid credential, so listing tools never proves auth. When set, the named read-only tool is invoked during prerequisite evaluation; a failure (e.g. 401) marks the toolset disabled instead of "connected". Wiresget_me/get_current_userinto the helm chart for the built-in GitHub / GitLab addons.Follow-up (this PR)
Auto-detect the health check tool. Toolset configs generated outside the helm template (e.g. Robusta's
custom_toolset.yaml) don't sethealth_check_tool, so the check was skipped. When unset, Holmes now falls back to an allowlist of well-known read-only identity tools (get_me,get_current_user,get_authenticated_user,whoami) and invokes the first one the server exposes. Explicit config still takes precedence.Enable the GitHub
contexttoolset by default.get_meonly exists when the GitHub MCP server'scontexttoolset is enabled, but the chart default (repos,issues,pull_requests,actions) excluded it — so auto-detection found nothing. Default is nowrepos,issues,pull_requests,actions,context(read-only/lightweight). Adds a debug log when no identity tool is exposed, so a skipped check is diagnosable.Detailed failure messages. Health-check failures now include the tool name and params (
health check tool '<name>' with params {} failed - ...), per the toolset error-detail contract (CodeRabbit).Testing
tests/test_mcp_toolset.py: health-check, auto-detect, skipped-check-logging, and error-message tests (TestMCPHealthCheckTool). Full suite passing.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Chores