Repository navigation
Conversation
Implements full OAuth authorization_code flow for MCP toolsets: - Frontend redirects user to IdP, receives auth code - Auth code encrypted with Holmes RSA public key, sent via save_prefixes - Holmes decrypts code, exchanges for token at cluster-internal token_url - Tokens cached per conversation with TTL, auto-evicted on expiry - Access token never leaves the cluster Includes Keycloak + MCP server test infrastructure in K8s for end-to-end OAuth flow verification. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
- Cache key now includes IdP identity (authorization_url + client_id), so MCP servers sharing the same IdP reuse one token per conversation - Fix MCP server crash when token has multiple audiences (list vs string) - Add _find_matching_audience to return correct audience string - Add WARNING-level logs for all token operations (inject, refresh, cache, errors) - Add tests for shared IdP caching and cache key isolation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
- Cache key now includes IdP identity (authorization_url + client_id), so MCP servers sharing the same IdP reuse one token per conversation - Fix MCP server crash when token has multiple audiences (list vs string) - Add _find_matching_audience to return correct audience string - Add WARNING-level logs for all token operations (inject, refresh, cache, errors) - Add tests for shared IdP caching and cache key isolation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
- Add enabled field to MCPOAuthConfig (auto-set when endpoints configured)
- Auto-discover OAuth endpoints: PRM (RFC 9728) + legacy fallback + DCR
- CLI mode: opens browser + local callback server for OAuth login
- Disk token store (~/.holmes/auth/mcp_tokens.json) for CLI persistence
- Dual-mode detection: CLI (no request_context) vs frontend (has headers)
- Live-tested against Atlassian MCP server (31 tools discovered)
- Config is just: oauth: { enabled: true } for auto-discovery servers
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: avi@robusta.dev <avi@robusta.dev>
- Add enabled field to MCPOAuthConfig (auto-set when endpoints configured)
- Auto-discover OAuth endpoints: PRM (RFC 9728) + legacy fallback + DCR
- CLI mode: opens browser + local callback server for OAuth login
- Disk token store (~/.holmes/auth/mcp_tokens.json) for CLI persistence
- Dual-mode detection: CLI (no request_context) vs frontend (has headers)
- Live-tested against Atlassian MCP server (31 tools discovered)
- Config is just: oauth: { enabled: true } for auto-discovery servers
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: avi@robusta.dev <avi@robusta.dev>
…ders The previous detection checked for specific header names (X-Conversation-Id, etc.) which the frontend doesn't always send. Now uses request_context != None as the signal — frontend always passes request_context, CLI never does. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
…ders The previous detection checked for specific header names (X-Conversation-Id, etc.) which the frontend doesn't always send. Now uses request_context != None as the signal — frontend always passes request_context, CLI never does. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
- Don't do DCR at startup (redirect_uri unknown, causes mismatch) - Send registration_endpoint in __oauth_metadata so frontend can DCR with its own redirect_uri - Only include scopes in metadata when non-empty (fixes scope= in URL) - Frontend must: check registration_endpoint, DCR if no client_id, omit scope param when scopes not provided Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
- Don't do DCR at startup (redirect_uri unknown, causes mismatch) - Send registration_endpoint in __oauth_metadata so frontend can DCR with its own redirect_uri - Only include scopes in metadata when non-empty (fixes scope= in URL) - Frontend must: check registration_endpoint, DCR if no client_id, omit scope param when scopes not provided Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
When DCR is deferred to frontend (e.g. Atlassian), the frontend includes
client_id in the encrypted {code, redirect_uri, client_id} payload.
Holmes uses it for the token exchange. Backwards compatible — falls back
to oauth_config.client_id when client_id is not in the payload.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: avi@robusta.dev <avi@robusta.dev>
When DCR is deferred to frontend (e.g. Atlassian), the frontend includes
client_id in the encrypted {code, redirect_uri, client_id} payload.
Holmes uses it for the token exchange. Backwards compatible — falls back
to oauth_config.client_id when client_id is not in the payload.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: avi@robusta.dev <avi@robusta.dev>
Generate RSA keypair once, persist it encrypted with a Fernet key derived from signing_key via HKDF. Same signing_key always produces the same keypair (loaded from disk), surviving Holmes restarts. Falls back to random keypair in CLI mode (no signing_key). - Singleton pattern via get_oauth_key_exchange() - Encrypted at rest with signing_key-derived Fernet key - Graceful fallback when filesystem is read-only - Tests verify: same key -> same keypair (5x), different keys -> different keypairs Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
Generate RSA keypair once, persist it encrypted with a Fernet key derived from signing_key via HKDF. Same signing_key always produces the same keypair (loaded from disk), surviving Holmes restarts. Falls back to random keypair in CLI mode (no signing_key). - Singleton pattern via get_oauth_key_exchange() - Encrypted at rest with signing_key-derived Fernet key - Graceful fallback when filesystem is read-only - Tests verify: same key -> same keypair (5x), different keys -> different keypairs Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
CLI mode now persists the RSA keypair to ~/.holmes/auth/oauth_keypair.enc encrypted with a Fernet key derived from machine identity (hostname + username). File permissions set to 600 (owner-only). This means CLI users don't need to re-authenticate after Holmes restarts. Server mode: encrypted with signing_key (unchanged) CLI mode: encrypted with machine identity, stored in user's home dir Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
CLI mode now persists the RSA keypair to ~/.holmes/auth/oauth_keypair.enc encrypted with a Fernet key derived from machine identity (hostname + username). File permissions set to 600 (owner-only). This means CLI users don't need to re-authenticate after Holmes restarts. Server mode: encrypted with signing_key (unchanged) CLI mode: encrypted with machine identity, stored in user's home dir Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
CLI mode now runs DCR inside _cli_oauth_flow after the callback server starts (so the actual port is known for redirect_uri). Fixes Atlassian CLI flow where client_id is None after deferred discovery. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
CLI mode now runs DCR inside _cli_oauth_flow after the callback server starts (so the actual port is known for redirect_uri). Fixes Atlassian CLI flow where client_id is None after deferred discovery. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
Recompute the OAuth cache key after _cli_oauth_flow returns, since DCR may have changed client_id (e.g. Atlassian auto-discovery). Without this, the token was cached under the pre-DCR key but looked up under the post-DCR key, causing a 401. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
Recompute the OAuth cache key after _cli_oauth_flow returns, since DCR may have changed client_id (e.g. Atlassian auto-discovery). Without this, the token was cached under the pre-DCR key but looked up under the post-DCR key, causing a 401. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
Tests cover: - Full roundtrip: DCR → browser open → callback → token exchange - DCR when client_id is None (Atlassian scenario) - Cache key changes after DCR sets client_id - Failure cases: missing endpoints, no DCR, token exchange failure Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
Tests cover: - Full roundtrip: DCR → browser open → callback → token exchange - DCR when client_id is None (Atlassian scenario) - Cache key changes after DCR sets client_id - Failure cases: missing endpoints, no DCR, token exchange failure Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
- Migration: OAuthTokens table with RLS (account-level access) - DAL methods: get_oauth_token, upsert_oauth_token, delete_oauth_token - Token encrypted at rest with signing_key-derived Fernet key - Signing key hash stored for mismatch detection across clusters - Flow: check in-memory cache → check DB → check disk → prompt user - On successful auth: store to in-memory cache + DB + disk - Warning logged when signing_key_hash mismatch detected (different cluster/config) - set_oauth_dal() wired into create_tool_executor at startup Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
- Migration: OAuthTokens table with RLS (account-level access) - DAL methods: get_oauth_token, upsert_oauth_token, delete_oauth_token - Token encrypted at rest with signing_key-derived Fernet key - Signing key hash stored for mismatch detection across clusters - Flow: check in-memory cache → check DB → check disk → prompt user - On successful auth: store to in-memory cache + DB + disk - Warning logged when signing_key_hash mismatch detected (different cluster/config) - set_oauth_dal() wired into create_tool_executor at startup Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
In-cluster containers have read-only root filesystem. DiskTokenStore now catches OSError during init and disables itself. All get/set/has methods become no-ops when disabled. DB storage is used instead. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
In-cluster containers have read-only root filesystem. DiskTokenStore now catches OSError during init and disables itself. All get/set/has methods become no-ops when disabled. DB storage is used instead. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
…set name Tokens are stored with provider_name=authorization_url but looked up by toolset name. Now both store and lookup use authorization_url consistently. Also added diagnostic log when no DB record found. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
…set name Tokens are stored with provider_name=authorization_url but looked up by toolset name. Now both store and lookup use authorization_url consistently. Also added diagnostic log when no DB record found. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
- token_expiry now tracks refresh token expiry (not access token), since that's what matters for cross-cluster reuse - DB row updated when token is refreshed, keeping it fresh for other clusters Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
- token_expiry now tracks refresh token expiry (not access token), since that's what matters for cross-cluster reuse - DB row updated when token is refreshed, keeping it fresh for other clusters Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
Update oauth-mcp-servers.md to reflect that tokens are persisted to DB (server mode) or disk (CLI mode) and refreshed automatically. Users only need to authenticate once. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
- Default _store to None; only create DiskTokenStore when enable_disk_store() is called (CLI mode via main.py) - Server mode sets DalTokenStore via set_dal() as before - Add null checks on _store before all persistent store operations - Move _get_exchange_manager import to top in tool_calling_llm.py - Move httpx import to top in oauth_tool_connector.py - Fix tests to set mock store on manager Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
- Default _store to None; only create DiskTokenStore when enable_disk_store() is called (CLI mode via main.py) - Server mode sets DalTokenStore via set_dal() as before - Add null checks on _store before all persistent store operations - Move _get_exchange_manager import to top in tool_calling_llm.py - Move httpx import to top in oauth_tool_connector.py - Fix tests to set mock store on manager Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
…n_endpoint - Remove exclude=True from MCPOAuthConfig.registration_endpoint — model_dump(exclude_none=True) handles it naturally - Simplify get_oauth_config to just model_dump(exclude_none=True) - Remove redundant cached token check in _check_oauth_server_reachable — handled by eager_load_oauth_tools at higher level Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
…n_endpoint - Remove exclude=True from MCPOAuthConfig.registration_endpoint — model_dump(exclude_none=True) handles it naturally - Simplify get_oauth_config to just model_dump(exclude_none=True) - Remove redundant cached token check in _check_oauth_server_reachable — handled by eager_load_oauth_tools at higher level Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
Resolved conflicts: - tools.py: kept Dict[str, Any] for meta field from master - toolset_mcp.py: kept MCP_TOOL_CALL_TIMEOUT_SEC import from master + our OAuth imports - pyproject.toml: kept both new pytest markers (manual, token-limit) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
Resolved conflicts: - tools.py: kept Dict[str, Any] for meta field from master - toolset_mcp.py: kept MCP_TOOL_CALL_TIMEOUT_SEC import from master + our OAuth imports - pyproject.toml: kept both new pytest markers (manual, token-limit) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
Tests the floriscornel/teams-mcp server over stdio, exercising delegated Microsoft Graph auth to locate a group chat by topic and read a verification code from its messages. Scope limited to read-only Chat tools (list_chats, get_chat_messages) to keep the required Graph permission set minimal. before_test posts a verification-code message via Graph; expected_output asserts Holmes reports the code. Idempotent by design, no after_test, so parallel CI runs against the shared tenant do not race. Also adds graph.microsoft.com and login.microsoftonline.com passthroughs to conftest.py responses fixture, and wires TEAMS_AUTH_TOKEN and TEAMS_TEST_CHAT_ID into eval-regression.yaml. Signed-off-by: Claude <noreply@anthropic.com>
Previous revision required pre-provisioning a chat and passing TEAMS_TEST_CHAT_ID. The eval now looks up the chat by topic and creates one if missing, using two member UPNs from env (Graph group chats need 3+ participants). Replaces TEAMS_TEST_CHAT_ID with TEAMS_TEST_MEMBER_1/2. Signed-off-by: Claude <noreply@anthropic.com>
Previous revision had before_test create the chat, which meant the eval wasn't actually testing Holmes's ability to create chats via the MCP. Now the prompt asks Holmes to search users, create a group chat with a unique Holmes-chosen topic suffix, and post a verification-code message; include_tool_calls + specific code assert the full flow happened. Auth simplified to a single bundled secret TEAMS_TOKEN_CACHE_TGZ_B64 — a tar.gz of the floriscornel MCP's cache + metadata files containing the refresh token (~90-day lifetime), so CI does not need per-run token minting. Replaces the ephemeral TEAMS_AUTH_TOKEN approach. Each run creates a new chat (unique suffix) so parallel CI runs never race; no after_test to avoid cross-run deletion. Signed-off-by: Claude <noreply@anthropic.com>
Replaces the MCP-based approach with a native Python toolset that mirrors
the thin-wrapper pattern used by servicenow_tables. Five tools covering
the incident-chat-start flow:
- teams_search_users (GET /users $filter=startsWith)
- teams_create_chat (POST /chats)
- teams_send_chat_message(POST /chats/{id}/messages)
- teams_list_chats (GET /me/chats)
- teams_get_chat_messages(GET /chats/{id}/messages)
The toolset takes a pre-minted delegated Graph access token via the
auth_token config field; it does not mint tokens or hardcode a client id,
so callers can use az CLI, MSAL, or their own Entra app registration.
This eliminates the MCP's audience-validation gotcha and the vendored-
fork complexity entirely, and lets tenants that require their
own app registration swap in a different token source with zero code
changes to the toolset.
Rewires the eval to use the native toolset (test dir renamed
257_mcp_teams_chat_message -> 257_teams_chat_message) and switches the
CI secret from TEAMS_TOKEN_CACHE_TGZ_B64 back to the simpler
TEAMS_AUTH_TOKEN. before_test is reduced to a single /me call to
sanity-check the token.
Required delegated scopes: User.Read, User.ReadBasic.All, Chat.ReadWrite,
ChatMessage.Send. No file access, no channel permissions, no
tenant-wide reads.
Signed-off-by: Claude <noreply@anthropic.com>
The eval now names the two users Holmes must add to the incident chat instead of saying "any two users you can find". Makes the test deterministic and tenant-specific (Robusta's tenant), and gives Holmes a concrete lookup task rather than an open-ended directory search. Signed-off-by: Claude <noreply@anthropic.com>
Three related changes bundled:
1. scripts/get_teams_token.py — MSAL-based CLI to mint a Graph delegated
access token via device-code flow against the Microsoft Graph Command
Line Tools public client. Requests only the 4 scopes the Teams toolset
needs (User.Read, User.ReadBasic.All, Chat.ReadWrite, ChatMessage.Send)
and caches the refresh token at ~/.holmes/teams_msal_cache.bin so
subsequent runs silently mint fresh access tokens. Replaces the
docker+floriscornel one-liner as the supported way to get a token.
2. tests/llm/utils/test_case_utils.py — _render_env helper that
substitutes {{ env.VAR }} placeholders on user_prompt and
expected_output at load time, matching the substitution already
available in toolsets.yaml. Lets fixtures reference tenant-specific
values (emails, IDs) without committing them.
3. tests/llm/fixtures/test_ask_holmes/257_teams_chat_message/test_case.yaml
— switch responder emails from hardcoded strings to
{{ env.TEAMS_TEST_MEMBER }} (comma-separated UPNs). CI workflow wires
TEAMS_TEST_MEMBER as a repo secret.
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📜 #5 · Run @ __5df6406__ (#24820034760) — Apr 23, 06:22 UTC✅ Results of HolmesGPT evalsAutomatically triggered by commit 5df6406 on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: Success - 34 test/model combinations loaded Benchmark experiment:
No benchmark data available for comparison. Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run. Comparison indicators:
📜 #4 · Run @ __ca32563__ (#24819599335) — Apr 23, 06:12 UTC✅ Results of HolmesGPT evalsAutomatically triggered by commit ca32563 on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: Success - 34 test/model combinations loaded Benchmark experiment:
No benchmark data available for comparison. Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run. Comparison indicators:
📜 #3 · Run @ __2fbabe0__ (#24819493058) — Apr 23, 06:06 UTC✅ Results of HolmesGPT evalsAutomatically triggered by commit 2fbabe0 on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: Success - 34 test/model combinations loaded Benchmark experiment:
No benchmark data available for comparison. Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run. Comparison indicators:
📜 #2 · Run @ __e1aff03__ (#24818937836) — Apr 23, 05:49 UTC✅ Results of HolmesGPT evalsAutomatically triggered by commit e1aff03 on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: Success - 34 test/model combinations loaded Benchmark experiment:
No benchmark data available for comparison. Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run. Comparison indicators:
📜 #1 · Run @ __a0a5dcf__ (#24818449162) — Apr 23, 05:33 UTC✅ Results of HolmesGPT evalsAutomatically triggered by commit a0a5dcf on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: Success - 34 test/model combinations loaded Benchmark experiment:
No benchmark data available for comparison. Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run. Comparison indicators:
✅ Results of HolmesGPT evalsAutomatically triggered by commit de79ca1 on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: Success - 34 test/model combinations loaded Benchmark experiment:
No benchmark data available for comparison. Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run. Comparison indicators:
📖 Legend
🔄 Re-run evals manually
Option 1: Comment on this PR with Or with more options (one per line): Run evals on a different branch (e.g., master) for comparison:
Quick re-run: Use Option 2: Trigger via GitHub Actions UI → "Run workflow" Option 3: Add PR labels to include extra evals (applies to both automatic runs and
Examples: 🏷️ Valid tags
🤖 Valid models
Commands: CLI: |
Contributor
|
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:
WalkthroughAdds a Microsoft Teams toolset (TeamsToolset) with multiple Graph-backed tools, a CLI to obtain delegated Graph tokens via MSAL device-code flow, CI/test env var exposure for Teams, new Teams integration fixtures/toolset configs, and recursive env-variable rendering when loading test fixtures. Changes
Sequence Diagram(s)sequenceDiagram
participant Holmes as Holmes (LLM)
participant Toolset as TeamsToolset
participant Graph as Microsoft Graph
participant MSAL as get_teams_token (MSAL/device flow)
participant Operator as Operator
Note over MSAL,Operator: Out-of-band token acquisition (device flow)
Operator->>MSAL: authenticate via device flow
MSAL-->>Operator: returns access_token (cached)
Holmes->>Toolset: invoke Teams tool (uses TeamsConfig.auth_token)
Toolset->>Graph: GET /me (prerequisite) with Bearer
Graph-->>Toolset: 200 OK or error
Toolset->>Graph: e.g., GET /users or POST /chats with Bearer
Graph-->>Toolset: responses (users, chat_id, messages)
Toolset-->>Holmes: StructuredToolResult (outputs or errors)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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:4bf1277f
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:4bf1277f me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:4bf1277f
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:4bf1277f
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:4bf1277f
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:4bf1277f me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:4bf1277f
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:4bf1277fPatch 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:4bf1277f \
--set operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set operator.image=holmes-operator-dev:4bf1277fRobusta 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:4bf1277f \
--set holmes.operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.operator.image=holmes-operator-dev:4bf1277f |
aantn
changed the title
![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: 5
🧹 Nitpick comments (1)
conftest.py (1)
290-292: Use regex passthroughs for the Teams cloud endpoints.This cloud eval passthrough should follow the repo pattern and use
re.compile()so subpaths are explicit and consistent.Proposed change
# Allow Microsoft Graph API calls (for Teams MCP integration) - rsps.add_passthru("https://graph.microsoft.com") - rsps.add_passthru("https://login.microsoftonline.com") + rsps.add_passthru(re.compile(r"https://graph\.microsoft\.com/.*")) + rsps.add_passthru(re.compile(r"https://login\.microsoftonline\.com/.*"))As per coding guidelines, "For cloud service evals, add the service's URL pattern to the passthrough list in
conftest.pyusingrsps.add_passthru()withre.compile()for pattern matching".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@conftest.py` around lines 290 - 292, The passthrough entries currently use literal URLs; change the two rsps.add_passthru calls to use regex patterns via re.compile so subpaths are matched consistently (e.g., replace the direct "https://graph.microsoft.com" and "https://login.microsoftonline.com" calls with rsps.add_passthru(re.compile(...)) patterns that include trailing path matching), and ensure re is imported if not already; update the calls around the rsps.add_passthru usages in conftest.py so they follow the repo pattern for cloud service evals.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@holmes/plugins/toolsets/__init__.py`:
- Line 120: The Teams toolset (registered via TeamsToolset()) is missing its
required docs and index entry; add a new
docs/data-sources/builtin-toolsets/teams.md modeled after other builtin toolset
markdown files (describe purpose, setup, config keys, auth, example usage, and
logo reference) and update docs/data-sources/builtin-toolsets/index.md to
include a Teams entry consistent with the existing index format so the
integration is documented like the other ~50 toolsets.
In `@holmes/plugins/toolsets/teams/teams.py`:
- Around line 209-225: The _invoke method currently does
int(params.get("limit")...) which can raise ValueError and allow zero/negative
values to reach $top; change to safely parse and validate "limit" from params
(use try/except for ValueError), default to 10 when missing/invalid, clamp to
the range 1..50 before building the query, and when the provided limit is
non-numeric or <=0 return a StructuredToolResult error rather than letting an
exception bubble; factor this logic into a helper (e.g., _parse_limit or
_validate_limit) and apply the same fix to the other similar invocations
referenced (the methods around the 396-403 and 433-440 ranges) so all Graph
calls use the validated/clamped limit and produce structured errors on invalid
input.
- Around line 77-81: The code uses broad `except Exception` handlers in
prerequisites_callable and other blocks (lines ~173-182, ~291-300) and performs
a local `import json` inside `_extract_graph_error`; change these to import json
at module scope and replace the blanket Exception catches with specific
exception types appropriate to each operation (e.g., for TeamsConfig
instantiation catch pydantic.ValidationError and TypeError in
`prerequisites_callable(self, config: dict[str, Any])`, for JSON parsing catch
`json.JSONDecodeError`, and for dict access catch `KeyError`/`TypeError` in
`_extract_graph_error`); update the error return messages to include the caught
exception instance as before. Ensure all imports (json and any ValidationError)
are at top of module and adjust the three affected methods
(`prerequisites_callable`, `_extract_graph_error`, and the other two similar
try/except blocks) to use these narrower exceptions.
- Around line 268-324: This write operation (_invoke for teams_create_chat) must
gate non-idempotent Graph writes behind user approval: add a
requires_approval(self) method (mirroring the Bash toolset pattern) or at the
start of _invoke check context.user_approved and if not approved return a
StructuredToolResult with status StructuredToolResultStatus.APPROVAL_REQUIRED
and params=params; apply the same pattern to the teams_send_chat_message handler
so both teams_create_chat and teams_send_chat_message never perform POST writes
unless context.user_approved is true (or requires_approval() returns False).
In `@tests/llm/fixtures/test_ask_holmes/257_teams_chat_message/test_case.yaml`:
- Around line 58-76: The before_test setup currently checks TEAMS_AUTH_TOKEN but
does not validate TEAMS_TEST_MEMBER; add a guard in the before_test block to
ensure TEAMS_TEST_MEMBER is set and contains at least two members (e.g., split
on commas or whitespace and count entries >= 2) and fail with a clear error
message if not; reference the TEAMS_TEST_MEMBER variable in the check and keep
the error wording consistent with the existing style used for TEAMS_AUTH_TOKEN
so the test exits early when TEAMS_TEST_MEMBER does not specify a group chat
with two or more members.
---
Nitpick comments:
In `@conftest.py`:
- Around line 290-292: The passthrough entries currently use literal URLs;
change the two rsps.add_passthru calls to use regex patterns via re.compile so
subpaths are matched consistently (e.g., replace the direct
"https://graph.microsoft.com" and "https://login.microsoftonline.com" calls with
rsps.add_passthru(re.compile(...)) patterns that include trailing path
matching), and ensure re is imported if not already; update the calls around the
rsps.add_passthru usages in conftest.py so they follow the repo pattern for
cloud service evals.
🪄 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: 7d5a9ff2-3a77-49eb-b3bf-9592a583f892
📒 Files selected for processing (9)
.github/workflows/eval-regression.yamlconftest.pyholmes/plugins/toolsets/__init__.pyholmes/plugins/toolsets/teams/__init__.pyholmes/plugins/toolsets/teams/teams.pyscripts/get_teams_token.pytests/llm/fixtures/test_ask_holmes/257_teams_chat_message/test_case.yamltests/llm/fixtures/test_ask_holmes/257_teams_chat_message/toolsets.yamltests/llm/utils/test_case_utils.py
| RunbookToolset(dal=dal, additional_search_paths=additional_search_paths), | ||
| AzureSQLToolset(), | ||
| ServiceNowTablesToolset(), | ||
| TeamsToolset(), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Docs/logo candidates for Teams integration:\n'
fd -i 'teams' README.md docs images holmes/plugins/toolsets 2>/dev/null || true
printf '\nBuiltin toolset index references:\n'
rg -n -i -C2 'teams|microsoft teams' README.md docs images holmes/plugins/toolsets 2>/dev/null || trueRepository: HolmesGPT/holmesgpt
Length of output: 18774
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '=== Checking for Teams builtin-toolsets documentation ===\n'
if [ -f "docs/data-sources/builtin-toolsets/teams.md" ]; then
printf 'FOUND: docs/data-sources/builtin-toolsets/teams.md\n'
head -20 "docs/data-sources/builtin-toolsets/teams.md"
else
printf 'MISSING: docs/data-sources/builtin-toolsets/teams.md\n'
fi
printf '\n=== Checking builtin-toolsets index.md for Teams ===\n'
if [ -f "docs/data-sources/builtin-toolsets/index.md" ]; then
rg -n 'teams|Teams' "docs/data-sources/builtin-toolsets/index.md" || printf 'No Teams reference found in index.md\n'
else
printf 'File not found: docs/data-sources/builtin-toolsets/index.md\n'
fi
printf '\n=== Checking walkthrough why-holmesgpt.md for Teams ===\n'
if [ -f "docs/walkthrough/why-holmesgpt.md" ]; then
rg -n 'teams|Teams' "docs/walkthrough/why-holmesgpt.md" || printf 'No Teams reference found in why-holmesgpt.md\n'
else
printf 'File not found: docs/walkthrough/why-holmesgpt.md\n'
fi
printf '\n=== List of existing builtin-toolsets documentation ===\n'
fd '\.md$' docs/data-sources/builtin-toolsets/ 2>/dev/null | sort || printf 'Directory not found or empty\n'Repository: HolmesGPT/holmesgpt
Length of output: 2992
Add missing Teams integration documentation.
The Teams toolset is registered and the logo exists, but critical documentation is incomplete:
docs/data-sources/builtin-toolsets/teams.mdis missing (required for all builtin toolsets)- Teams is not listed in
docs/data-sources/builtin-toolsets/index.md
All other ~50 builtin toolsets have their own .md file in the builtin-toolsets directory following the pattern of existing integrations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@holmes/plugins/toolsets/__init__.py` at line 120, The Teams toolset
(registered via TeamsToolset()) is missing its required docs and index entry;
add a new docs/data-sources/builtin-toolsets/teams.md modeled after other
builtin toolset markdown files (describe purpose, setup, config keys, auth,
example usage, and logo reference) and update
docs/data-sources/builtin-toolsets/index.md to include a Teams entry consistent
with the existing index format so the integration is documented like the other
~50 toolsets.
Comment on lines
+77
to
+81
| def prerequisites_callable(self, config: dict[str, Any]) -> Tuple[bool, str]: | ||
| try: | ||
| self.config = TeamsConfig(**config) | ||
| except Exception as e: | ||
| return False, f"Invalid Teams toolset configuration: {e}" |
Contributor
There was a problem hiding this comment.
Fix the Ruff BLE001 catches and keep imports at module scope.
These broad Exception handlers will violate Ruff, and _extract_graph_error also imports inside a method. Narrow the exceptions and move json to the top.
Proposed cleanup
from abc import ABC
+import json
+from json import JSONDecodeError
from typing import Any, ClassVar, Dict, List, Optional, Tuple, Type, cast
from urllib.parse import urljoin
import requests # type: ignore
-from pydantic import Field
+from pydantic import Field, ValidationError
@@
- except Exception as e:
+ except ValidationError as e:
return False, f"Invalid Teams toolset configuration: {e}"
@@
`@staticmethod`
def _extract_graph_error(body: str) -> str:
try:
- import json
-
parsed = json.loads(body)
err = parsed.get("error", {})
return f"{err.get('code', '')}: {err.get('message', '')}".strip(": ")
- except Exception:
+ except (JSONDecodeError, AttributeError, TypeError):
return "(unparseable error body)"
@@
- except Exception as e:
+ except (requests.exceptions.RequestException, KeyError) as e:
return StructuredToolResult(
status=StructuredToolResultStatus.ERROR,
error=f"Failed to resolve authenticated user id for chat creation: {e}",As per coding guidelines, "Use Ruff for code formatting and linting (configured in pyproject.toml)" and "ALWAYS place Python imports at the top of the file, not inside functions or methods".
Also applies to: 173-182, 291-300
🧰 Tools
🪛 Ruff (0.15.10)
[warning] 80-80: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@holmes/plugins/toolsets/teams/teams.py` around lines 77 - 81, The code uses
broad `except Exception` handlers in prerequisites_callable and other blocks
(lines ~173-182, ~291-300) and performs a local `import json` inside
`_extract_graph_error`; change these to import json at module scope and replace
the blanket Exception catches with specific exception types appropriate to each
operation (e.g., for TeamsConfig instantiation catch pydantic.ValidationError
and TypeError in `prerequisites_callable(self, config: dict[str, Any])`, for
JSON parsing catch `json.JSONDecodeError`, and for dict access catch
`KeyError`/`TypeError` in `_extract_graph_error`); update the error return
messages to include the caught exception instance as before. Ensure all imports
(json and any ValidationError) are at top of module and adjust the three
affected methods (`prerequisites_callable`, `_extract_graph_error`, and the
other two similar try/except blocks) to use these narrower exceptions.
Comment on lines
+209
to
+225
| def _invoke(self, params: dict, context: ToolInvokeContext) -> StructuredToolResult: | ||
| query = params["query"].replace("'", "''") | ||
| limit = min(int(params.get("limit") or 10), 50) | ||
| filter_expr = ( | ||
| f"startsWith(displayName,'{query}') or startsWith(mail,'{query}') " | ||
| f"or startsWith(userPrincipalName,'{query}')" | ||
| ) | ||
| return self._graph_request( | ||
| "GET", | ||
| "/users", | ||
| params_for_result=params, | ||
| query={ | ||
| "$filter": filter_expr, | ||
| "$select": "id,displayName,userPrincipalName,mail", | ||
| "$top": limit, | ||
| }, | ||
| ) |
Contributor
There was a problem hiding this comment.
Return structured errors for invalid limit values.
int(params.get("limit")...) can raise ValueError, and negative/zero values can produce invalid $top requests. Clamp and validate before calling Graph so tool invocations don’t escape as runtime exceptions.
One way to centralize the limit parsing
+def _parse_limit(params: Dict[str, Any], default: int, maximum: int = 50) -> Tuple[Optional[int], Optional[StructuredToolResult]]:
+ raw = params.get("limit", default)
+ try:
+ limit = int(raw)
+ except (TypeError, ValueError):
+ return None, StructuredToolResult(
+ status=StructuredToolResultStatus.ERROR,
+ error=f"limit must be an integer, got {raw!r}",
+ params=params,
+ )
+ if limit < 1:
+ return None, StructuredToolResult(
+ status=StructuredToolResultStatus.ERROR,
+ error=f"limit must be >= 1, got {limit}",
+ params=params,
+ )
+ return min(limit, maximum), None
+
@@
- limit = min(int(params.get("limit") or 10), 50)
+ limit, error = _parse_limit(params, default=10)
+ if error:
+ return error
@@
- query: Dict[str, Any] = {"$top": min(int(params.get("limit") or 20), 50)}
+ limit, error = _parse_limit(params, default=20)
+ if error:
+ return error
+ query: Dict[str, Any] = {"$top": limit}
@@
- query = {"$top": min(int(params.get("limit") or 20), 50)}
+ limit, error = _parse_limit(params, default=20)
+ if error:
+ return error
+ query = {"$top": limit}Also applies to: 396-403, 433-440
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@holmes/plugins/toolsets/teams/teams.py` around lines 209 - 225, The _invoke
method currently does int(params.get("limit")...) which can raise ValueError and
allow zero/negative values to reach $top; change to safely parse and validate
"limit" from params (use try/except for ValueError), default to 10 when
missing/invalid, clamp to the range 1..50 before building the query, and when
the provided limit is non-numeric or <=0 return a StructuredToolResult error
rather than letting an exception bubble; factor this logic into a helper (e.g.,
_parse_limit or _validate_limit) and apply the same fix to the other similar
invocations referenced (the methods around the 396-403 and 433-440 ranges) so
all Graph calls use the validated/clamped limit and produce structured errors on
invalid input.
Comment on lines
+268
to
+324
| def _invoke(self, params: dict, context: ToolInvokeContext) -> StructuredToolResult: | ||
| chat_type = params["chat_type"] | ||
| if chat_type not in ("group", "oneOnOne"): | ||
| return StructuredToolResult( | ||
| status=StructuredToolResultStatus.ERROR, | ||
| error=f"chat_type must be 'group' or 'oneOnOne', got '{chat_type}'", | ||
| params=params, | ||
| ) | ||
|
|
||
| member_ids: List[str] = params["member_ids"] | ||
| if chat_type == "oneOnOne" and len(member_ids) != 1: | ||
| return StructuredToolResult( | ||
| status=StructuredToolResultStatus.ERROR, | ||
| error="oneOnOne chats require exactly 1 member id (the other participant).", | ||
| params=params, | ||
| ) | ||
| if chat_type == "group" and len(member_ids) < 2: | ||
| return StructuredToolResult( | ||
| status=StructuredToolResultStatus.ERROR, | ||
| error="group chats require at least 2 member ids in addition to the authenticated user.", | ||
| params=params, | ||
| ) | ||
|
|
||
| # Resolve the authenticated user's id so we can add ourselves explicitly. | ||
| try: | ||
| me, _ = self._toolset._graph_request("GET", "/me", timeout=10) | ||
| my_id = me["id"] | ||
| except Exception as e: | ||
| return StructuredToolResult( | ||
| status=StructuredToolResultStatus.ERROR, | ||
| error=f"Failed to resolve authenticated user id for chat creation: {e}", | ||
| params=params, | ||
| ) | ||
|
|
||
| all_ids = [my_id] + [m for m in member_ids if m != my_id] | ||
| members = [ | ||
| { | ||
| "@odata.type": "#microsoft.graph.aadUserConversationMember", | ||
| "roles": ["owner"], | ||
| "user@odata.bind": f"{self._toolset.teams_config.graph_base_url.rstrip('/')}/users/{uid}", | ||
| } | ||
| for uid in all_ids | ||
| ] | ||
|
|
||
| body: Dict[str, Any] = {"chatType": chat_type, "members": members} | ||
| if params.get("topic"): | ||
| if chat_type != "group": | ||
| return StructuredToolResult( | ||
| status=StructuredToolResultStatus.ERROR, | ||
| error="Topic can only be set on 'group' chats.", | ||
| params=params, | ||
| ) | ||
| body["topic"] = params["topic"] | ||
|
|
||
| return self._graph_request( | ||
| "POST", "/chats", params_for_result=params, json_body=body | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Tools returning APPROVAL_REQUIRED:\n'
rg -n -C3 'APPROVAL_REQUIRED|user_approved|requires_approval' holmes tests 2>/dev/null || true
printf '\nToolInvokeContext definition:\n'
ast-grep --pattern $'class ToolInvokeContext:
$$$'Repository: HolmesGPT/holmesgpt
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get file size first
wc -l holmes/plugins/toolsets/teams/teams.py
# Check if Teams toolset has requires_approval method
echo "=== Checking for requires_approval in Teams toolset ==="
rg -n "requires_approval|user_approved|APPROVAL_REQUIRED" holmes/plugins/toolsets/teams/
# Check what's at lines 355-363 (second location mentioned in review)
echo -e "\n=== Lines 355-363 in teams.py ==="
sed -n '355,363p' holmes/plugins/toolsets/teams/teams.py
# Look at the full _invoke method and class structure
echo -e "\n=== Teams toolset class and method structure ==="
rg -n "class.*Tool|def _invoke|def requires_approval" holmes/plugins/toolsets/teams/teams.py | head -20Repository: HolmesGPT/holmesgpt
Length of output: 161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show lines 355-363 that the review mentions
echo "=== Lines 355-363 (second issue location) ==="
sed -n '355,365p' holmes/plugins/toolsets/teams/teams.py
# Show the full _invoke method for post messages (around line 355)
echo -e "\n=== Context around post message operations ==="
sed -n '340,380p' holmes/plugins/toolsets/teams/teams.py
# Check what other toolsets with write operations look like
echo -e "\n=== Bash toolset requires_approval pattern ==="
rg -A 10 "def requires_approval" holmes/plugins/toolsets/bash/toolset_bash.py | head -30
# Check how requires_approval is called in framework
echo -e "\n=== Framework approval flow ==="
rg -n "requires_approval" holmes/tool_calling_llm.py | head -10Repository: HolmesGPT/holmesgpt
Length of output: 2245
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find bash toolset files
echo "=== Finding bash toolset files ==="
find holmes/plugins/toolsets -name "*bash*" -type f
# Check Tool base class for requires_approval default behavior
echo -e "\n=== Tool base class ==="
rg -B 5 -A 15 "def requires_approval" holmes/plugins/ | head -60
# Check if requires_approval is optional or mandatory
echo -e "\n=== Where requires_approval is called in framework ==="
rg -B 3 -A 3 "\.requires_approval\(" holmes/ | head -50
# Check the complete Teams post message tool definition
echo -e "\n=== Complete TeamsPostMessage class ==="
sed -n '330,365p' holmes/plugins/toolsets/teams/teams.pyRepository: HolmesGPT/holmesgpt
Length of output: 6158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the base Tool class definition
echo "=== Tool base class requires_approval ==="
rg -B 5 -A 20 "class Tool" holmes/core/tools.py | head -80
# Check how get_approval_config works
echo -e "\n=== Toolset approval config check ==="
rg -B 5 -A 10 "_check_approval_config\|approval_required" holmes/core/tools.py
# Look for any approval handling in TeamsToolset class
echo -e "\n=== TeamsToolset class definition ==="
rg -B 2 -A 30 "^class TeamsToolset" holmes/plugins/toolsets/teams/teams.py
# Check if TeamsCreateChat implements requires_approval
echo -e "\n=== TeamsCreateChat full class ==="
sed -n '100,280p' holmes/plugins/toolsets/teams/teams.py | tail -100Repository: HolmesGPT/holmesgpt
Length of output: 3120
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for the Tool class definition more carefully
echo "=== Tool class in tools.py ==="
rg -n "^class Tool" holmes/core/tools.py
# Read the Tool class
echo -e "\n=== Tool class full definition ==="
sed -n '1,200p' holmes/core/tools.py | tail -150
# Check if requires_approval has a default implementation
echo -e "\n=== requires_approval method in Tool ==="
rg -B 2 -A 15 "def requires_approval" holmes/core/tools.py
# See if any toolsets override it
echo -e "\n=== All toolsets with requires_approval ==="
find holmes/plugins/toolsets -name "*.py" -exec grep -l "def requires_approval" {} \;Repository: HolmesGPT/holmesgpt
Length of output: 6143
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if there's a toolset-level approval config
echo "=== Toolset approval config in Toolset base class ==="
rg -B 5 -A 20 "approval|mutating|write" holmes/core/tools.py | grep -A 20 -B 5 "class Toolset"
# Check if Teams toolset has any approval-related config
echo -e "\n=== TeamsToolset full definition ==="
sed -n '1,100p' holmes/plugins/toolsets/teams/teams.py
# Check how framework handles None from requires_approval
echo -e "\n=== Framework tool invocation logic ==="
rg -B 5 -A 20 "def.*requires_approval\|approval = tool.requires_approval" holmes/core/tool_calling_llm.py | head -50
# Check if there's a way tools signal they're mutating/require approval
echo -e "\n=== Tool metadata or flags ==="
rg -n "mutating|requires_approval|user_approved" holmes/core/tools.py | head -20Repository: HolmesGPT/holmesgpt
Length of output: 4405
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check how approval_required_tools is used
echo "=== approval_required_tools usage ==="
rg -n "approval_required_tools" holmes/
# Check if Teams toolset sets this field
echo -e "\n=== TeamsToolset complete ==="
sed -n '40,150p' holmes/plugins/toolsets/teams/teams.py
# Check framework approval flow in tool_calling_llm
echo -e "\n=== Complete tool invocation approval logic ==="
rg -B 10 -A 15 "approval = tool.requires_approval\|approval_required_tools" holmes/core/tool_calling_llm.py | head -80
# Check if context.user_approved is checked before calling _invoke
echo -e "\n=== user_approved usage in toolset invocations ==="
rg -n "user_approved\|APPROVAL_REQUIRED" holmes/plugins/toolsets/teams/ 2>/dev/null || echo "Not found in teams"
rg -B 3 -A 3 "context.user_approved" holmes/plugins/toolsets/ | head -40Repository: HolmesGPT/holmesgpt
Length of output: 4598
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get line 432 context in tools.py
echo "=== Line 432 context: approval_required_tools usage ==="
sed -n '420,450p' holmes/core/tools.py
# Get line 768 context
echo -e "\n=== Line 768 context ==="
sed -n '765,775p' holmes/core/tools.py
# Search for the actual approval check in Tool class
echo -e "\n=== Tool.check_approval method ==="
rg -B 5 -A 25 "def.*check_approval\|def.*requires_approval" holmes/core/tools.py | head -100
# Check if Teams tools need to be marked in approval_required_tools
echo -e "\n=== Does framework auto-gate based on requires_approval method? ==="
rg -B 10 -A 10 "APPROVAL_REQUIRED.*status" holmes/core/tool_calling_llm.py | head -50Repository: HolmesGPT/holmesgpt
Length of output: 1868
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check bash toolset to confirm it uses either approval_required_tools or requires_approval
echo "=== Bash toolset approval pattern ==="
rg -A 20 "class BashToolset" holmes/plugins/toolsets/bash/bash_toolset.py | head -40
# Check if bash is in approval_required_tools or uses requires_approval
echo -e "\n=== Bash tool requires_approval implementation ==="
rg -B 5 -A 30 "def requires_approval" holmes/plugins/toolsets/bash/bash_toolset.py
# Check if other mutating toolsets use these patterns
echo -e "\n=== Check other toolsets for mutating operations ==="
find holmes/plugins/toolsets -name "*.py" -exec grep -l "approval_required_tools\|def requires_approval" {} \;
# Verify that context.user_approved is NOT checked in Teams
echo -e "\n=== Teams _invoke methods checking user_approved ==="
grep -n "user_approved" holmes/plugins/toolsets/teams/teams.py || echo "Not found - Teams does NOT check user_approved"
# Show the framework's _invoke_llm_tool_call to see if it passes user_approved
echo -e "\n=== Framework passes user_approved to tool _invoke ==="
rg -B 10 -A 10 "_invoke.*user_approved\|tool\._invoke" holmes/core/tool_calling_llm.py | head -60Repository: HolmesGPT/holmesgpt
Length of output: 100
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find bash toolset file properly
echo "=== Finding bash toolset ==="
find holmes/plugins/toolsets -name "*bash*" -type f
# Check BashTool class
echo -e "\n=== BashTool requires_approval ==="
rg -B 5 -A 30 "class BashTool" holmes/plugins/toolsets/bash/bash_toolset.py | head -80
# Check if framework invokes tool._invoke directly or checks approval first
echo -e "\n=== Framework approval flow before _invoke ==="
rg -B 15 -A 5 "status.*APPROVAL_REQUIRED\|StructuredToolResultStatus.APPROVAL_REQUIRED" holmes/core/tool_calling_llm.py | head -100
# Check the actual invocation flow
echo -e "\n=== _invoke_llm_tool_call implementation ==="
rg -B 5 -A 50 "def _invoke_llm_tool_call" holmes/core/tool_calling_llm.py | head -100Repository: HolmesGPT/holmesgpt
Length of output: 270
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read bash toolset file
echo "=== Bash toolset complete ==="
cat holmes/plugins/toolsets/bash/bash_toolset.py | head -200
# Check tool_calling_llm for approval flow
echo -e "\n=== Tool calling LLM approval check ==="
rg -n "requires_approval\|APPROVAL_REQUIRED" holmes/core/tool_calling_llm.py | head -20Repository: HolmesGPT/holmesgpt
Length of output: 7355
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the complete approval flow in tool_calling_llm.py
echo "=== Tool calling LLM approval handling ==="
sed -n '1,100p' holmes/core/tool_calling_llm.py | grep -A 50 "requires_approval\|APPROVAL_REQUIRED" || true
# Search for where approval is checked before/after tool invoke
echo -e "\n=== Full approval checking logic ==="
rg -n "requires_approval\|APPROVAL_REQUIRED\|user_approved" holmes/core/tool_calling_llm.py
# Get the _invoke_llm_tool_call method implementation
echo -e "\n=== _invoke_llm_tool_call complete ==="
rg -B 5 -A 50 "def _invoke_llm_tool_call" holmes/core/tool_calling_llm.py | head -150Repository: HolmesGPT/holmesgpt
Length of output: 143
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the tool calling file
echo "=== Finding tool calling files ==="
find holmes -name "*tool*call*" -type f
# Check what's in holmes/core directory
echo -e "\n=== Holmes core directory ==="
ls -la holmes/core/
# Search more broadly for approval handling
echo -e "\n=== Approval flow anywhere in holmes ==="
rg -l "APPROVAL_REQUIRED.*status\|approval.*before.*invoke" holmes/core/ 2>/dev/null || echo "Not found"
# Check how bash tool actually returns APPROVAL_REQUIRED
echo -e "\n=== Does bash tool return APPROVAL_REQUIRED from _invoke? ==="
sed -n '150,250p' holmes/plugins/toolsets/bash/bash_toolset.py
# Look for where requires_approval is actually called
echo -e "\n=== Where requires_approval is invoked in framework ==="
rg -r "\.requires_approval\(" holmes/core/Repository: HolmesGPT/holmesgpt
Length of output: 7600
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for where requires_approval is called
echo "=== Where requires_approval is called ==="
rg -n "requires_approval\|check_approval" holmes/core/tool_calling_llm.py
# Get the context around those lines
echo -e "\n=== Tool invocation with approval checks ==="
rg -B 10 -A 20 "requires_approval\|check_approval" holmes/core/tool_calling_llm.py | head -150
# Check if tool._invoke is called and how APPROVAL_REQUIRED is returned
echo -e "\n=== tool\._invoke invocation ==="
rg -B 5 -A 15 "tool\._invoke" holmes/core/tool_calling_llm.py | head -100Repository: HolmesGPT/holmesgpt
Length of output: 104
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look at tool_calling_llm.py directly
wc -l holmes/core/tool_calling_llm.py
# Read parts of tool_calling_llm.py searching for approval
echo "=== Searching tool_calling_llm for tool invocation ==="
grep -n "tool\._invoke\|requires_approval\|check_approval\|APPROVAL_REQUIRED" holmes/core/tool_calling_llm.py
# If no results, read the file to understand the flow
echo -e "\n=== Read tool_calling_llm invoke section ==="
sed -n '700,800p' holmes/core/tool_calling_llm.pyRepository: HolmesGPT/holmesgpt
Length of output: 4692
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get context around line 922 where requires_approval is called
echo "=== Line 922 context: requires_approval invocation ==="
sed -n '900,980p' holmes/core/tool_calling_llm.py
# Check what happens after requires_approval returns
echo -e "\n=== After requires_approval check - approval handling ==="
sed -n '920,1000p' holmes/core/tool_calling_llm.pyRepository: HolmesGPT/holmesgpt
Length of output: 7111
Implement approval gating for Teams write operations.
Both teams_create_chat and teams_send_chat_message are non-idempotent writes executed as the delegated user via Microsoft Graph. They must not bypass user approval. Implement a requires_approval() method (following the Bash toolset pattern) to gate these operations, or check context.user_approved in _invoke() and return APPROVAL_REQUIRED if approval is needed.
🧰 Tools
🪛 Ruff (0.15.10)
[warning] 295-295: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@holmes/plugins/toolsets/teams/teams.py` around lines 268 - 324, This write
operation (_invoke for teams_create_chat) must gate non-idempotent Graph writes
behind user approval: add a requires_approval(self) method (mirroring the Bash
toolset pattern) or at the start of _invoke check context.user_approved and if
not approved return a StructuredToolResult with status
StructuredToolResultStatus.APPROVAL_REQUIRED and params=params; apply the same
pattern to the teams_send_chat_message handler so both teams_create_chat and
teams_send_chat_message never perform POST writes unless context.user_approved
is true (or requires_approval() returns False).
Comment on lines
+58
to
+76
| before_test: | | ||
| set -e | ||
|
|
||
| [ -n "${TEAMS_AUTH_TOKEN:-}" ] || { echo "Missing env var: TEAMS_AUTH_TOKEN (delegated Graph access token)"; exit 1; } | ||
|
|
||
| # Quick sanity check: does the token work against /me? | ||
| HTTP=$(curl -sS -o /tmp/teams_eval_me.json -w "%{http_code}" \ | ||
| https://graph.microsoft.com/v1.0/me \ | ||
| -H "Authorization: Bearer ${TEAMS_AUTH_TOKEN}") | ||
|
|
||
| if [ "${HTTP}" != "200" ]; then | ||
| echo "Graph /me returned HTTP ${HTTP}: $(cat /tmp/teams_eval_me.json 2>/dev/null)" | ||
| echo "Check that TEAMS_AUTH_TOKEN is a valid, non-expired Graph delegated access token" | ||
| echo "with scopes: User.Read, User.ReadBasic.All, Chat.ReadWrite, ChatMessage.Send" | ||
| rm -f /tmp/teams_eval_me.json | ||
| exit 1 | ||
| fi | ||
| rm -f /tmp/teams_eval_me.json | ||
| echo "Setup complete" |
Contributor
There was a problem hiding this comment.
Validate TEAMS_TEST_MEMBER before starting the live eval.
Setup currently accepts an empty or single-member responder list, but the prompt requires a group chat and the tool requires at least two other members. Fail fast here to avoid a noisy LLM/Graph failure later.
Proposed setup guard
[ -n "${TEAMS_AUTH_TOKEN:-}" ] || { echo "Missing env var: TEAMS_AUTH_TOKEN (delegated Graph access token)"; exit 1; }
+ [ -n "${TEAMS_TEST_MEMBER:-}" ] || { echo "Missing env var: TEAMS_TEST_MEMBER (comma-separated responder UPNs)"; exit 1; }
+
+ MEMBER_COUNT=$(python3 - <<'PY'
+import os
+members = [m.strip() for m in os.environ.get("TEAMS_TEST_MEMBER", "").split(",") if m.strip()]
+print(len(members))
+PY
+)
+ [ "${MEMBER_COUNT}" -ge 2 ] || { echo "TEAMS_TEST_MEMBER must contain at least 2 comma-separated responders"; exit 1; }
# Quick sanity check: does the token work against /me?📝 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
| before_test: | | |
| set -e | |
| [ -n "${TEAMS_AUTH_TOKEN:-}" ] || { echo "Missing env var: TEAMS_AUTH_TOKEN (delegated Graph access token)"; exit 1; } | |
| # Quick sanity check: does the token work against /me? | |
| HTTP=$(curl -sS -o /tmp/teams_eval_me.json -w "%{http_code}" \ | |
| https://graph.microsoft.com/v1.0/me \ | |
| -H "Authorization: Bearer ${TEAMS_AUTH_TOKEN}") | |
| if [ "${HTTP}" != "200" ]; then | |
| echo "Graph /me returned HTTP ${HTTP}: $(cat /tmp/teams_eval_me.json 2>/dev/null)" | |
| echo "Check that TEAMS_AUTH_TOKEN is a valid, non-expired Graph delegated access token" | |
| echo "with scopes: User.Read, User.ReadBasic.All, Chat.ReadWrite, ChatMessage.Send" | |
| rm -f /tmp/teams_eval_me.json | |
| exit 1 | |
| fi | |
| rm -f /tmp/teams_eval_me.json | |
| echo "Setup complete" | |
| before_test: | | |
| set -e | |
| [ -n "${TEAMS_AUTH_TOKEN:-}" ] || { echo "Missing env var: TEAMS_AUTH_TOKEN (delegated Graph access token)"; exit 1; } | |
| [ -n "${TEAMS_TEST_MEMBER:-}" ] || { echo "Missing env var: TEAMS_TEST_MEMBER (comma-separated responder UPNs)"; exit 1; } | |
| MEMBER_COUNT=$(python3 - <<'PY' | |
| import os | |
| members = [m.strip() for m in os.environ.get("TEAMS_TEST_MEMBER", "").split(",") if m.strip()] | |
| print(len(members)) | |
| PY | |
| ) | |
| [ "${MEMBER_COUNT}" -ge 2 ] || { echo "TEAMS_TEST_MEMBER must contain at least 2 comma-separated responders"; exit 1; } | |
| # Quick sanity check: does the token work against /me? | |
| HTTP=$(curl -sS -o /tmp/teams_eval_me.json -w "%{http_code}" \ | |
| https://graph.microsoft.com/v1.0/me \ | |
| -H "Authorization: Bearer ${TEAMS_AUTH_TOKEN}") | |
| if [ "${HTTP}" != "200" ]; then | |
| echo "Graph /me returned HTTP ${HTTP}: $(cat /tmp/teams_eval_me.json 2>/dev/null)" | |
| echo "Check that TEAMS_AUTH_TOKEN is a valid, non-expired Graph delegated access token" | |
| echo "with scopes: User.Read, User.ReadBasic.All, Chat.ReadWrite, ChatMessage.Send" | |
| rm -f /tmp/teams_eval_me.json | |
| exit 1 | |
| fi | |
| rm -f /tmp/teams_eval_me.json | |
| echo "Setup complete" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/llm/fixtures/test_ask_holmes/257_teams_chat_message/test_case.yaml`
around lines 58 - 76, The before_test setup currently checks TEAMS_AUTH_TOKEN
but does not validate TEAMS_TEST_MEMBER; add a guard in the before_test block to
ensure TEAMS_TEST_MEMBER is set and contains at least two members (e.g., split
on commas or whitespace and count entries >= 2) and fail with a clear error
message if not; reference the TEAMS_TEST_MEMBER variable in the check and keep
the error wording consistent with the existing style used for TEAMS_AUTH_TOKEN
so the test exits early when TEAMS_TEST_MEMBER does not specify a group chat
with two or more members.
Three new tools on the native Teams toolset:
- teams_list_teams (GET /me/joinedTeams)
- teams_create_channel (POST /teams/{id}/channels)
- teams_send_channel_message (POST /teams/{id}/channels/{id}/messages)
Plus eval 258: Holmes lists the joined teams, picks one, creates a
standard channel with a holmes-eval-258-* unique name, and posts two
progressive investigation updates including a verification code. Follows
the same hallucination-proof pattern as eval 257 (include_tool_calls +
specific code in the message).
Adds three delegated scopes to the token requirements and the
get_teams_token.py helper: Team.ReadBasic.All, Channel.Create,
ChannelMessage.Send. Channel.Create typically requires admin consent
in enterprise tenants.
Reentrancy: channels use Holmes-chosen unique names so parallel runs
never collide. Channels persist across runs; document periodic manual
cleanup of holmes-eval-258-* rather than after_test (avoids racing
parallel runs).
Signed-off-by: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
scripts/get_teams_token.py (1)
57-95: LGTM.MSAL device-code flow with cached refresh token, stderr prompts, stdout token — clean. One minor nit:
cache.deserialize(CACHE_FILE.read_text())will raise if the cache file is corrupted (partial write, etc.) and there is no recovery. Consider wrapping it in atry/exceptthat falls back to a fresh cache; otherwise users have to manually delete the file.Optional resiliency tweak
cache = msal.SerializableTokenCache() if CACHE_FILE.exists(): - cache.deserialize(CACHE_FILE.read_text()) + try: + cache.deserialize(CACHE_FILE.read_text()) + except Exception as e: # corrupted cache → start fresh + print(f"Ignoring corrupt MSAL cache ({e}); re-authenticating.", file=sys.stderr) + cache = msal.SerializableTokenCache()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/get_teams_token.py` around lines 57 - 95, The call to cache.deserialize(CACHE_FILE.read_text()) in main can raise if the cache file is corrupted; wrap the deserialize+read in a try/except around the SerializableTokenCache usage (msal.SerializableTokenCache and CACHE_FILE) so that on exception you log a warning to stderr, reset to a fresh cache instance (e.g., create a new msal.SerializableTokenCache()) and continue (optionally delete or overwrite the corrupted CACHE_FILE) so the device-flow fallback and token acquisition logic (app = msal.PublicClientApplication, app.acquire_token_silent, app.initiate_device_flow, app.acquire_token_by_device_flow) can proceed without requiring manual file deletion.tests/llm/fixtures/test_ask_holmes/258_teams_create_channel/test_case.yaml (1)
17-21: Persistent resource accumulation — consider a cleanup strategy.The comment acknowledges that channels persist across runs and will accumulate (
holmes-eval-258-*), leaving manual cleanup to team owners. In CI this means continuous growth of the test team's channel list, which can eventually bump up against Teams' per-team channel limit (200 standard channels) and cause this test to fail in a hard-to-diagnose way. Consider one of:
- A scheduled (nightly/weekly) cleanup job that deletes
holmes-eval-258-*channels older than N hours.- An
after_testthat best-effort deletes the channel created in this run (the PR already captures the channel id in the report), guarded to not fail on 404.Non-blocking, but worth planning before this is widely adopted.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/llm/fixtures/test_ask_holmes/258_teams_create_channel/test_case.yaml` around lines 17 - 21, The test currently leaves created channels matching the holmes-eval-258-* pattern, which will accumulate; add a best-effort cleanup: implement an after_test hook (or a separate scheduled cleanup job) that deletes the channel created by this run using the channel id captured in the test report and ignore 404/permission errors so the test doesn't fail; reference the channel name pattern "holmes-eval-258-*" and the report-stored channel id when locating the resource to delete.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@holmes/plugins/toolsets/teams/teams.py`:
- Around line 58-82: The TeamsToolset class sets docs_url to a page that doesn't
exist; add the missing documentation and index entries so the new Teams
integration is discoverable. Create a new teams.md docs page describing the
Teams toolset and its capabilities, add a Teams entry to the builtin-toolsets
index (including the teams icon), add a short reference to Teams in the
why-holmesgpt walkthrough, and update the project README to list the standalone
Teams integration; ensure the docs_url in TeamsToolset still points to the new
teams docs and keep names consistent with the TeamsToolset class and its tool
names (e.g., TeamsSearchUsers, TeamsCreateChat, TeamsSendChatMessage).
---
Nitpick comments:
In `@scripts/get_teams_token.py`:
- Around line 57-95: The call to cache.deserialize(CACHE_FILE.read_text()) in
main can raise if the cache file is corrupted; wrap the deserialize+read in a
try/except around the SerializableTokenCache usage (msal.SerializableTokenCache
and CACHE_FILE) so that on exception you log a warning to stderr, reset to a
fresh cache instance (e.g., create a new msal.SerializableTokenCache()) and
continue (optionally delete or overwrite the corrupted CACHE_FILE) so the
device-flow fallback and token acquisition logic (app =
msal.PublicClientApplication, app.acquire_token_silent,
app.initiate_device_flow, app.acquire_token_by_device_flow) can proceed without
requiring manual file deletion.
In `@tests/llm/fixtures/test_ask_holmes/258_teams_create_channel/test_case.yaml`:
- Around line 17-21: The test currently leaves created channels matching the
holmes-eval-258-* pattern, which will accumulate; add a best-effort cleanup:
implement an after_test hook (or a separate scheduled cleanup job) that deletes
the channel created by this run using the channel id captured in the test report
and ignore 404/permission errors so the test doesn't fail; reference the channel
name pattern "holmes-eval-258-*" and the report-stored channel id when locating
the resource to delete.
🪄 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: f2fc4730-04e1-4d25-8fcb-5a23fcd9f075
📒 Files selected for processing (4)
holmes/plugins/toolsets/teams/teams.pyscripts/get_teams_token.pytests/llm/fixtures/test_ask_holmes/258_teams_create_channel/test_case.yamltests/llm/fixtures/test_ask_holmes/258_teams_create_channel/toolsets.yaml
✅ Files skipped from review due to trivial changes (1)
- tests/llm/fixtures/test_ask_holmes/258_teams_create_channel/toolsets.yaml
Comment on lines
+58
to
+82
| class TeamsToolset(Toolset): | ||
| config_classes: ClassVar[list[Type[TeamsConfig]]] = [TeamsConfig] | ||
|
|
||
| def __init__(self): | ||
| super().__init__( | ||
| name="teams", | ||
| description=( | ||
| "Microsoft Teams chat and channel operations via delegated Graph API: " | ||
| "search users, create group chats, post messages and updates, " | ||
| "list joined teams, create channels in teams, read chat history." | ||
| ), | ||
| icon_url="https://cdn.simpleicons.org/microsoftteams/6264A7", | ||
| docs_url="https://holmesgpt.dev/data-sources/builtin-toolsets/teams/", | ||
| prerequisites=[CallablePrerequisite(callable=self.prerequisites_callable)], | ||
| tools=[ | ||
| TeamsSearchUsers(self), | ||
| TeamsCreateChat(self), | ||
| TeamsSendChatMessage(self), | ||
| TeamsListChats(self), | ||
| TeamsGetChatMessages(self), | ||
| TeamsListTeams(self), | ||
| TeamsCreateChannel(self), | ||
| TeamsSendChannelMessage(self), | ||
| ], | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -HI -t f 'teams' docs README.md images/integration_logos 2>/dev/null
echo "---"
rg -n -i 'teams' README.md docs/walkthrough/why-holmesgpt.md docs/data-sources/builtin-toolsets/index.md 2>/dev/null | head -40Repository: HolmesGPT/holmesgpt
Length of output: 287
🏁 Script executed:
#!/bin/bash
# Check for teams documentation file specifically
find docs/data-sources/builtin-toolsets -type f -name "*teams*" 2>/dev/null
echo "---"
# Check if index.md in builtin-toolsets has been updated
echo "=== builtin-toolsets/index.md ==="
cat docs/data-sources/builtin-toolsets/index.md 2>/dev/null | head -100
echo "---"
# Check why-holmesgpt.md
echo "=== why-holmesgpt.md (grep teams) ==="
grep -n -i "teams" docs/walkthrough/why-holmesgpt.md 2>/dev/null || echo "No teams mention found"Repository: HolmesGPT/holmesgpt
Length of output: 4121
Add missing documentation for new Teams toolset.
The docs_url in the toolset points to /data-sources/builtin-toolsets/teams/ but this page does not exist. Per repo learnings, new toolsets require:
docs/data-sources/builtin-toolsets/teams.md— create this filedocs/data-sources/builtin-toolsets/index.md— add Teams entry (logo exists atimages/integration_logos/teams-icon.png)docs/walkthrough/why-holmesgpt.md— add Teams referenceREADME.md— update to reflect this new standalone integration
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@holmes/plugins/toolsets/teams/teams.py` around lines 58 - 82, The
TeamsToolset class sets docs_url to a page that doesn't exist; add the missing
documentation and index entries so the new Teams integration is discoverable.
Create a new teams.md docs page describing the Teams toolset and its
capabilities, add a Teams entry to the builtin-toolsets index (including the
teams icon), add a short reference to Teams in the why-holmesgpt walkthrough,
and update the project README to list the standalone Teams integration; ensure
the docs_url in TeamsToolset still points to the new teams docs and keep names
consistent with the TeamsToolset class and its tool names (e.g.,
TeamsSearchUsers, TeamsCreateChat, TeamsSendChatMessage).
Test discovery loads every fixture under tests/llm/fixtures/. When a
fixture references {{ env.VAR }} for a var the current shell hasn't set,
the previous implementation raised ValueError and blocked collection of
*all* tests — not just the fixture that needed the var.
Tolerate the missing-var case: leave the placeholder in place, log at
debug level, and let collection proceed. Tests that actually rely on the
unresolved placeholder will fail clearly at run time (Holmes sees the
literal placeholder and cannot complete the task) rather than masking
the real tests behind a collection error.
Signed-off-by: Claude <noreply@anthropic.com>
…ents, subject, importance)
The two send tools (chat + channel) now mirror the Graph chatMessage
resource, giving Holmes real control over formatting rather than always
sending plain text:
- content_type: 'text' | 'html' (default 'html'). Teams renders a subset
of HTML — headers, lists, <pre>, <code>, <br>, <a>, tables. Lets
Holmes post structured investigation updates instead of wall-of-text.
- attachments: list of Adaptive Card attachments. To send a card, put a
<attachment id="..."></attachment> placeholder in content and a
matching entry in attachments with contentType
'application/vnd.microsoft.card.adaptive' and content =
JSON-serialized card payload. Holmes composes the card schema at
runtime — v1.5 per adaptivecards.io.
- importance: 'normal' | 'high' | 'urgent'. 'urgent' triggers priority
notification + red banner.
- subject: channel messages only. One-line headline above the body;
useful for incident titles.
Shared _build_message_body helper validates enums, shapes the body,
handles attachments. Defaults to HTML content_type so Holmes can use
its native HTML fluency without opting in.
Signed-off-by: Claude <noreply@anthropic.com>
Same infra as 257 (group chat with TEAMS_TEST_MEMBER responders), but the user_prompt feeds Holmes a realistic raw-fact dump (Victoria Metrics outage reproduction from the screenshot) and says NOTHING about HTML, bullets, headers, adaptive cards, or any other formatting treatment. Purpose: verify that the richer send-message API (content_type, attachments, importance) is enough to get Holmes to format structured updates on its own without prompt engineering. Eval-level pass still just checks the right tools were called with the verification code — the visual quality of the formatting is a human-in-the-loop check in the Teams client after the run. Signed-off-by: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
holmes/plugins/toolsets/teams/teams.py (1)
140-149: Tighten the return type of_build_message_body.The signature declares
-> Any, but the function actually returnsUnion[Dict[str, Any], StructuredToolResult]. The callers (TeamsSendChatMessage._invoke,TeamsSendChannelMessage._invoke) doisinstance(body, StructuredToolResult)to distinguish the two — a precise annotation documents that contract and keeps mypy honest.Suggested change
-def _build_message_body( - params: Dict[str, Any], allow_subject: bool = False -) -> Any: +def _build_message_body( + params: Dict[str, Any], allow_subject: bool = False +) -> Union[Dict[str, Any], StructuredToolResult]:(and add
Unionto thetypingimport)As per coding guidelines, "Type hints are required throughout the codebase".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/plugins/toolsets/teams/teams.py` around lines 140 - 149, Update the _build_message_body signature to return the precise union type rather than Any: change its return annotation to Union[Dict[str, Any], StructuredToolResult] and add Union to the typing imports; keep the existing behavior (callers TeamsSendChatMessage._invoke and TeamsSendChannelMessage._invoke rely on isinstance(body, StructuredToolResult)) so no runtime changes are needed—just tighten the type hint to reflect the actual contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@holmes/plugins/toolsets/teams/teams.py`:
- Around line 616-629: The TeamsCreateChannel._invoke and
TeamsSendChannelMessage._invoke methods currently perform non-idempotent Graph
writes without checking user approval; modify both methods to call
self.requires_approval() and, if it returns True and context.user_approved is
falsy, short-circuit before making the _graph_request (e.g., return a
failed/denied StructuredToolResult or raise the same approval-denial behavior
used by the chat tools) so channel creation and channel-message sends cannot
proceed unless the user has approved the action.
- Around line 560-577: TeamsListTeams currently returns an unbounded collection;
update the class to accept a paged/filtered query by adding parameters (e.g.,
"limit" int default 20, max 50, and optional "name_filter" string) in the
parameters dict, then modify _invoke to map limit -> $top (clamped to 50) and if
name_filter is provided add $filter=startswith(displayName,'<value>') before
calling _graph_request, and update get_parameterized_one_liner to include the
name_filter text when present; reference the TeamsListTeams class, its _invoke
method, and get_parameterized_one_liner for locating the changes.
---
Nitpick comments:
In `@holmes/plugins/toolsets/teams/teams.py`:
- Around line 140-149: Update the _build_message_body signature to return the
precise union type rather than Any: change its return annotation to
Union[Dict[str, Any], StructuredToolResult] and add Union to the typing imports;
keep the existing behavior (callers TeamsSendChatMessage._invoke and
TeamsSendChannelMessage._invoke rely on isinstance(body, StructuredToolResult))
so no runtime changes are needed—just tighten the type hint to reflect the
actual contract.
🪄 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: 86d8ed1a-7ea5-4f1b-a4d0-ea69d31ba1da
📒 Files selected for processing (1)
holmes/plugins/toolsets/teams/teams.py
Comment on lines
+560
to
+577
| class TeamsListTeams(BaseTeamsTool): | ||
| def __init__(self, toolset: TeamsToolset): | ||
| super().__init__( | ||
| toolset=toolset, | ||
| name="teams_list_teams", | ||
| description=( | ||
| "List Microsoft Teams that the authenticated user has joined. " | ||
| "Returns id, displayName, and description for each team. " | ||
| "Use this to find a team id before calling teams_create_channel." | ||
| ), | ||
| parameters={}, | ||
| ) | ||
|
|
||
| def _invoke(self, params: dict, context: ToolInvokeContext) -> StructuredToolResult: | ||
| return self._graph_request("GET", "/me/joinedTeams", params_for_result=params) | ||
|
|
||
| def get_parameterized_one_liner(self, params: Dict) -> str: | ||
| return f"{toolset_name_for_one_liner(self._toolset.name)}: list joined teams" |
Contributor
There was a problem hiding this comment.
TeamsListTeams returns an unbounded collection — add $top (and optionally a name filter).
GET /me/joinedTeams is paged on the Graph side but this tool has no limit, no $top, and no filter — for users in many teams (common in large tenants) the full response will be dumped into the model context and risk token overflow. Mirror TeamsListChats: expose a limit parameter (default ~20, max 50) mapped to $top, and consider an optional name_filter using startswith(displayName, '…').
Based on learnings: "Never return unbounded data from APIs - always include filter parameters on tools that query collections to prevent token overflow".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@holmes/plugins/toolsets/teams/teams.py` around lines 560 - 577,
TeamsListTeams currently returns an unbounded collection; update the class to
accept a paged/filtered query by adding parameters (e.g., "limit" int default
20, max 50, and optional "name_filter" string) in the parameters dict, then
modify _invoke to map limit -> $top (clamped to 50) and if name_filter is
provided add $filter=startswith(displayName,'<value>') before calling
_graph_request, and update get_parameterized_one_liner to include the
name_filter text when present; reference the TeamsListTeams class, its _invoke
method, and get_parameterized_one_liner for locating the changes.
Comment on lines
+616
to
+629
| def _invoke(self, params: dict, context: ToolInvokeContext) -> StructuredToolResult: | ||
| body: Dict[str, Any] = { | ||
| "displayName": params["display_name"], | ||
| "membershipType": params.get("membership_type") or "standard", | ||
| } | ||
| if params.get("description"): | ||
| body["description"] = params["description"] | ||
|
|
||
| return self._graph_request( | ||
| "POST", | ||
| f"/teams/{params['team_id']}/channels", | ||
| params_for_result=params, | ||
| json_body=body, | ||
| ) |
Contributor
There was a problem hiding this comment.
Channel writes also bypass user approval — extend the gating to TeamsCreateChannel and TeamsSendChannelMessage.
The earlier approval-gating review called out teams_create_chat / teams_send_chat_message, but TeamsCreateChannel._invoke (here) and TeamsSendChannelMessage._invoke (lines 724-733) are the same class of non-idempotent delegated Graph writes — creating a permanent channel in a team and broadcasting a message to it. They must be subject to the same requires_approval() / context.user_approved check you add for the chat tools; otherwise the LLM can silently create channels and post to them on the user's behalf.
Sketch (apply to both channel tools)
def _invoke(self, params: dict, context: ToolInvokeContext) -> StructuredToolResult:
+ if not context.user_approved:
+ return StructuredToolResult(
+ status=StructuredToolResultStatus.APPROVAL_REQUIRED,
+ params=params,
+ )
body: Dict[str, Any] = {
"displayName": params["display_name"],
"membershipType": params.get("membership_type") or "standard",
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@holmes/plugins/toolsets/teams/teams.py` around lines 616 - 629, The
TeamsCreateChannel._invoke and TeamsSendChannelMessage._invoke methods currently
perform non-idempotent Graph writes without checking user approval; modify both
methods to call self.requires_approval() and, if it returns True and
context.user_approved is falsy, short-circuit before making the _graph_request
(e.g., return a failed/denied StructuredToolResult or raise the same
approval-denial behavior used by the chat tools) so channel creation and
channel-message sends cannot proceed unless the user has approved the action.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/llm/fixtures/test_ask_holmes/259_teams_formatted_status_update/test_case.yaml (1)
43-47:teams_search_userscall-count assertion may be too strict.Line 44 says "once per responder" to resolve each UPN in
TEAMS_TEST_MEMBER. If Holmes ever batches lookups, caches a previous result, or the model resolves multiple UPNs in a single search query, this expectation will fail even when the end state is correct. Consider loosening to "calledteams_search_usersto resolve each UPN in{{ env.TEAMS_TEST_MEMBER }}" without prescribing the exact call count.Proposed wording
- - "Must have called teams_search_users (once per responder) to resolve each of {{ env.TEAMS_TEST_MEMBER }} to a user id" + - "Must have called teams_search_users to resolve each UPN in {{ env.TEAMS_TEST_MEMBER }} to a user id"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/llm/fixtures/test_ask_holmes/259_teams_formatted_status_update/test_case.yaml` around lines 43 - 47, The assertion text in expected_output is too strict about call count for teams_search_users; change the line referencing teams_search_users so it asserts that teams_search_users was called to resolve each UPN in {{ env.TEAMS_TEST_MEMBER }} (or that all UPNs were resolved) without requiring "once per responder" or an exact call count; keep the other expectations about teams_create_chat (chat_type 'group', both resolved user ids, topic starting with 'holmes-eval-259-'), teams_send_chat_message containing 'HLMS259-V3R1FY', and reporting the chat topic and chat ID unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@tests/llm/fixtures/test_ask_holmes/259_teams_formatted_status_update/test_case.yaml`:
- Line 20: The test currently creates persistent group chats named with the
prefix "holmes-eval-259-" but lacks an after_test cleanup, so add an after_test
hook that safely deletes the created chat(s) matching the "holmes-eval-259-*"
suffix pattern (or, if Graph delete-chat semantics prevent safe per-test
deletion, add a clear header comment documenting intentional accumulation and
link to a periodic sweep script/cron job) and ensure the after_test explicitly
avoids touching any shared resources; update the test that constructs the
"holmes-eval-259-*" chat to record the created chat id so the after_test can
target only those IDs.
---
Nitpick comments:
In
`@tests/llm/fixtures/test_ask_holmes/259_teams_formatted_status_update/test_case.yaml`:
- Around line 43-47: The assertion text in expected_output is too strict about
call count for teams_search_users; change the line referencing
teams_search_users so it asserts that teams_search_users was called to resolve
each UPN in {{ env.TEAMS_TEST_MEMBER }} (or that all UPNs were resolved) without
requiring "once per responder" or an exact call count; keep the other
expectations about teams_create_chat (chat_type 'group', both resolved user ids,
topic starting with 'holmes-eval-259-'), teams_send_chat_message containing
'HLMS259-V3R1FY', and reporting the chat topic and chat ID unchanged.
🪄 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: eda1b81c-db7f-4888-84fa-1ebf8978a24d
📒 Files selected for processing (2)
tests/llm/fixtures/test_ask_holmes/259_teams_formatted_status_update/test_case.yamltests/llm/fixtures/test_ask_holmes/259_teams_formatted_status_update/toolsets.yaml
✅ Files skipped from review due to trivial changes (1)
- tests/llm/fixtures/test_ask_holmes/259_teams_formatted_status_update/toolsets.yaml
| # TEAMS_TEST_MEMBER - Comma-separated UPNs to include in the chat | ||
| # (natan@robusta.dev,arik@robusta.dev in demo). | ||
| # | ||
| # Reentrancy: each run creates a new chat with a Holmes-chosen suffix. |
Contributor
There was a problem hiding this comment.
Chats accumulate indefinitely — consider an after_test or documenting the cleanup story.
The header comment states each run creates a new chat with a unique suffix (reentrancy), but there is no after_test to delete the created chat, so every eval run leaves a permanent holmes-eval-259-* group chat in the tenant. Over time this clutters the test account and makes it harder to audit real chats. If Graph's delete-chat semantics make cleanup impractical, consider at least a comment here explaining the intentional accumulation, or a periodic sweep script. Note: after_test must still avoid deleting shared resources.
Also applies to: 57-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@tests/llm/fixtures/test_ask_holmes/259_teams_formatted_status_update/test_case.yaml`
at line 20, The test currently creates persistent group chats named with the
prefix "holmes-eval-259-" but lacks an after_test cleanup, so add an after_test
hook that safely deletes the created chat(s) matching the "holmes-eval-259-*"
suffix pattern (or, if Graph delete-chat semantics prevent safe per-test
deletion, add a clear header comment documenting intentional accumulation and
link to a periodic sweep script/cron job) and ensure the after_test explicitly
avoids touching any shared resources; update the test that constructs the
"holmes-eval-259-*" chat to record the created chat id so the after_test can
target only those IDs.
Previous description claimed "content_type='text' — raw text. Newlines
render as newlines." That's false for Teams. Graph's text content mode
HTML-escapes the body and collapses whitespace (including \n), so a
multi-line plain-text message renders as one run-on paragraph in the
Teams client.
Correct the description for both send_chat_message and
send_channel_message to state this plainly:
- text mode: use only for a single short line; newlines collapse
- html mode: default, required for any multi-line/formatted content;
line breaks require <br> or <p> (literal \n are ignored)
This is a description-accuracy fix, not prompt engineering — once the
tool tells the truth, Holmes will correctly pick html when it wants
section breaks and bullet lists.
Signed-off-by: Claude <noreply@anthropic.com>
Two Holmes-triggered bugs observed live:
1. Tool call failed with "'NoneType' object is not subscriptable" when
Holmes passed attachments=[None] (some LLMs emit [null] in place of
omitting an optional array). _build_message_body iterated and did
None["id"]. Now:
- Filter out None / empty / non-dict entries before iterating.
- Validate each remaining attachment has 'id' + 'content'; return a
structured error if not so Holmes sees a clear message to self-
correct rather than an opaque Python traceback.
2. Holmes chose content_type='text' but passed HTML-tag content (even
pre-escaping <tag> as <tag>), guaranteeing the message would
render as literal visible markup. Tighten the description to be
unambiguous: text mode is only for literal-plain-text single-line
messages; anything containing <tags> must use html; pre-escaping
your own markup is never correct.
Signed-off-by: Claude <noreply@anthropic.com>
This branch has not been deployed
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 by CodeRabbit
New Features
Tests
Chores