[ROB-2694] Oauth

[Avi-Robusta]

Summary by CodeRabbit

• New Features

  • Full OAuth support for MCP servers: PKCE/browser CLI flows, authorization-code callback, per-user/conversation token caching, background refresh, dynamic tool loading after auth, and token persistence options (memory/disk/DB).

• Bug Fixes

  • Improved tool lookup, registration and replacement behavior; clearer tool-change logging.

• Documentation

  • Added OAuth setup guide with CLI and Helm examples and browser-accessibility notes.

• Tests

  • Extensive unit/integration tests and Kubernetes fixtures for MCP OAuth flows.

• Chores

  • Added a manual pytest marker and CI-aware skipping of manual tests.

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Adds end-to-end OAuth for MCP toolsets: discovery, PKCE/authorization-code exchange, per-conversation/user token caching (memory, disk, DB), background refresh, dynamic tool loading post-auth, server callback endpoint, DAL wiring, docs, many tests, and a pytest hook to skip manual tests by default.

Changes

Cohort / File(s)	Summary
Test collection & markers conftest.py, pyproject.toml	Added pytest_collection_modifyitems hook to auto-skip manual tests unless included; declared manual marker in pytest config.
Docs docs/data-sources/remote-mcp-servers.md	New “OAuth Authentication” section with required fields, examples, and browser warning.
Core models & config holmes/core/models.py, holmes/config.py, holmes/core/tools.py	Added OAuth callback models; ToolApprovalDecision.decision field; ChatRequestBaseModel.user_id; ToolsetDBModel.meta; Config.create_toolcalling_llm gains optional tool_executor and sets OAuth DAL when creating executor.
OAuth helpers holmes/core/oauth_utils.py	New module: token-exchange, config lookup, callback processing, and related exceptions.
DAL (Supabase) holmes/core/supabase_dal.py	Added OAUTH_TOKENS_TABLE and methods get_oauth_token, upsert_oauth_token (DB gated by enabled, user_id required for upsert).
Token storage primitives holmes/plugins/toolsets/mcp/oauth_token_store.py	New OAuthTokenCache (thread-safe monotonic TTL cache) and DiskTokenStore (JSON-backed disk persistence with expiry validation and safe fallback).
Token manager & refresh holmes/plugins/toolsets/mcp/oauth_token_manager.py	New OAuthTokenManager: multi-tier retrieval (cache → DB → disk), encryption with signing key, background refresh daemon, public token APIs, and helpers for conversation/user keys.
MCP toolset OAuth integration holmes/plugins/toolsets/mcp/toolset_mcp.py	Added MCPOAuthConfig and oauth field, OAuth discovery, CLI/frontend PKCE flows, placeholder <toolset>_connect tool, header injection to include Bearer token, set_oauth_dal, and exchange_code_for_token entrypoint; prereq flow changed for OAuth-enabled servers.
ToolExecutor enhancements holmes/core/tools_utils/tool_executor.py	Tracks _toolset_names, registers OAuth-provided tools dynamically, name prefix-stripping fallback, and new with_replaced_tools() to clone/replace toolset tools per-executor.
Tool-calling LLM holmes/core/tool_calling_llm.py	Process OAuth decision payloads before executing approved tools, log decision counts, refine decision variable names, and change tool-refetch behavior in streaming flow.
Server & API server.py	Added POST /api/oauth/callback route with mapped error codes; propagate chat_request.user_id into request_context; preload per-user OAuth MCP tools and pass per-request tool_executor into LLM creation.
Sync helpers holmes/utils/holmes_sync_toolsets.py	Persist toolset meta (including parsed oauth_config) and added get_config_meta_for_toolset.
Tests & fixtures tests/test_mcp_oauth.py, tests/llm/fixtures/test_ask_holmes/254_mcp_oauth/*	Large test suite and Kubernetes/Keycloak + MCP fixtures for end-to-end OAuth flows, plus many unit tests covering cache, disk, DB, encryption, discovery, PKCE, and CLI/frontend exchanges.
Misc small conftest.py (tests hook)	Added pytest collection hook to skip manual tests unless selected via -m manual.

Sequence Diagram(s)

sequenceDiagram
    rect rgba(150,200,250,0.5)
    participant User as Browser (User)
    participant Holmes as Holmes Server
    participant TokenMgr as OAuth Token Manager
    participant DB as Supabase DB
    participant OAuth as OAuth Provider
    participant MCP as MCP Server
    end

    User->>Holmes: Trigger tool requiring OAuth (includes user_id)
    Holmes->>TokenMgr: preload_oauth_mcp_tools(user_id, convo)
    TokenMgr->>TokenMgr: check cache/disk/DB for token
    alt token missing
        TokenMgr->>DB: get_oauth_token(...)
        DB-->>TokenMgr: encrypted token (if any)
        TokenMgr->>TokenMgr: decrypt & cache
    end
    alt user needs auth
        Holmes->>User: present "connect" placeholder / PKCE link
        User->>OAuth: Authorize (browser)
        OAuth-->>Holmes: Auth code -> POST /api/oauth/callback
        Holmes->>TokenMgr: process_oauth_callback(code)
        TokenMgr->>OAuth: POST token_url (code exchange)
        OAuth-->>TokenMgr: access_token/refresh_token
        TokenMgr->>DB: upsert_oauth_token(encrypted)
    end
    Holmes->>MCP: Call tool with Authorization: Bearer <access_token>
    MCP-->>Holmes: Tool response

Loading

sequenceDiagram
    rect rgba(220,180,180,0.5)
    participant TokenMgr as OAuth Token Manager
    participant BG as Background Refresh Thread
    participant OAuth as OAuth Provider
    participant DB as Supabase DB
    end

    TokenMgr->>BG: schedule refresh (expires_in - safety_buffer)
    Note over BG: wakes periodically (~60s)
    BG->>BG: check due refreshes
    alt refresh due
        BG->>OAuth: POST token_url (refresh_token grant)
        alt success
            OAuth-->>BG: new access_token
            BG->>TokenMgr: update cache
            BG->>DB: upsert_oauth_token(encrypted)
        else failure
            BG->>BG: log failure, possibly retry/remove schedule
        end
    end

Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~110 minutes

Possibly related PRs

• feat: support passing token to mcp in header #1397: overlaps MCP header/token handling and threading request_context through server → tool executor paths.
• ROB-1288 toolsset mcp #419: prior MCP toolset implementation that this change extends with OAuth/token management and hooks.
• ROB-1933: issues with SaaS approval flow #1037: overlapping changes to approval/decision handling in holmes/core/tool_calling_llm.py.

Suggested labels

evals-tag-mcp

Suggested reviewers

• moshemorad
• aantn
• arikalon1

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 40.79% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title '[ROB-2694] Oauth' is vague and generic—it only states the issue ticket and feature name without describing the main change or its purpose.	Replace with a more descriptive title that summarizes the primary change, such as 'Add OAuth token management and integration for MCP servers' or 'Implement OAuth authorization-code flow for MCP toolsets'.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ 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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[netlify]

✅ Deploy Preview for holmes-docs ready!

Name	Link
🔨 Latest commit	a0f0eb4
🔍 Latest deploy log	https://app.netlify.com/projects/holmes-docs/deploys/69ea51306782c400089eb007
😎 Deploy Preview	https://deploy-preview-1900--holmes-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[coderabbitai]

Actionable comments posted: 17

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

holmes/core/tool_calling_llm.py (1)

250-306: ⚠️ Potential issue | 🔴 Critical

The approved OAuth retry is broken before it ever sees the token.

ToolApprovalDecision no longer has an encrypted_token field, so the log at Line 250 and the branch at Line 294 will raise AttributeError as soon as approvals are processed. Even after that is fixed, the only supported OAuth payload now lives in decision.decision, but this code handles it after _invoke_llm_tool_call(), so the approved retry runs once without the exchanged token.

💡 Suggested direction

         logging.warning("OAuth flow: received %d tool decisions: %s",
             len(tool_decisions),
-            [(d.tool_call_id, d.approved, bool(d.encrypted_token)) for d in tool_decisions])
+            [(d.tool_call_id, d.approved, bool(d.decision)) for d in tool_decisions])

         ...

             if decision and decision.approved:
-                # Exchange OAuth auth code for token if present (from frontend browser OAuth flow).
-                # The OAuth payload is passed via the encrypted_token field or save_prefixes
-                # with a "__oauth_token__:" marker.
-                oauth_payload = None
-                if decision.encrypted_token:
-                    oauth_payload = decision.encrypted_token
-                    decision.encrypted_token = None
-                elif decision.save_prefixes:
-                    for prefix in list(decision.save_prefixes):
-                        if prefix.startswith("__oauth_token__:"):
-                            oauth_payload = prefix[len("__oauth_token__:"):]
-                            decision.save_prefixes.remove(prefix)
-                            break
-
-                if oauth_payload:
-                    logging.warning("OAuth: received auth code for tool_call_id=%s, exchanging for token", tool_call.id)
-                    exchange_code_for_token(tool_call.id, oauth_payload, request_context)
+                if decision.decision:
+                    _try_process_oauth_decision(decision.decision)

                 tool_result = self._invoke_llm_tool_call(
                     tool_to_call=tool_call,
                     previous_tool_calls=[],
                     trace_span=trace_span,
                     tool_number=None,
                     user_approved=True,
                     session_approved_prefixes=session_prefixes,
                     request_context=request_context,
                     enable_tool_approval=True,
                 )

-            # If the decision contains OAuth callback data, process token exchange
-            if decision and decision.approved and decision.decision:
-                _try_process_oauth_decision(decision.decision)

Also applies to: 338-340

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@holmes/core/tool_calling_llm.py` around lines 250 - 306, The code reads
decision.encrypted_token (which no longer exists) and logs it, causing
AttributeError; change logging and token-extraction to use the current payload
location (decision.decision or entries in decision.save_prefixes with
"__oauth_token__:"), avoid accessing encrypted_token, and extract the OAuth
payload before any retry/invocation so the exchanged token is available;
specifically update the handling in the loop over pending_tool_calls
(ToolCallWithDecision, tool_call, decision) to (1) stop referencing
decision.encrypted_token in the logging and extraction, (2) check
decision.decision first for an OAuth payload and fallback to scanning
decision.save_prefixes for "__oauth_token__:", remove that prefix when used,
call exchange_code_for_token(tool_call.id, payload, request_context) immediately
before invoking the tool retry path (so the token is present for
_invoke_llm_tool_call), and audit any other references to encrypted_token in
this function (including the other occurrence noted) to apply the same fix.

🧹 Nitpick comments (1)

tests/llm/fixtures/test_ask_holmes/254_mcp_oauth/mcp_oauth_server.py (1)

164-169: Bring the new fixture back in line with the repo’s Python conventions.

KeycloakTokenVerifier.__init__ introduces untyped parameters, and add_wellknown_routes() adds function-local imports. Please type the constructor inputs (str) and move the Starlette imports to module scope.

As per coding guidelines, "Type hints are required throughout the codebase" and "ALWAYS place Python imports at the top of the file, not inside functions or methods".

Also applies to: 293-295

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/llm/fixtures/test_ask_holmes/254_mcp_oauth/mcp_oauth_server.py` around
lines 164 - 169, KeycloakTokenVerifier.__init__ currently has untyped parameters
and add_wellknown_routes() contains function-local Starlette imports; update the
constructor signature for KeycloakTokenVerifier to annotate all parameters as
str (e.g., introspection_endpoint: str, server_url: str, client_id: str,
client_secret: str) and ensure any other constructors in this file follow the
same typing (also apply to the other constructor around the indicated area).
Move all Starlette imports used in add_wellknown_routes() (e.g., Starlette,
Route, JSONResponse, PlainTextResponse or whatever is imported there) to the
module top-level instead of importing inside the function, keeping their
original names so callers in add_wellknown_routes() continue to work. Ensure
imports remain at top of file and that type hints are added consistently across
similar fixtures.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/data-sources/remote-mcp-servers.md`:
- Around line 351-430: Update the OAuth docs to scope token lifetime and storage
by deployment mode: replace the blanket statements "access token is cached for
the conversation" and "The access token never leaves the cluster" with explicit
notes that in CLI mode tokens may be persisted locally (e.g., in the user's CLI
config or OS credential store / ~/.holmes/config.yaml) and in server mode tokens
may be stored in the configured DAL-backed credential storage, while still
ensuring that the authorization code is exchanged server-side at `token_url` and
that `authorization_url` remains browser-accessible; reference the
`authorization_url`, `token_url`, `client_id`, and `scopes` fields when
describing these mode-specific storage behaviors.

In `@holmes/config.py`:
- Around line 312-317: The cached-executor early return prevents
set_oauth_dal(dal) from being called on subsequent create_tool_executor() calls,
so call set_oauth_dal(dal) before returning the cached executor: inside
create_tool_executor(), invoke set_oauth_dal(dal) (or set it only when dal is
not None, if desired) immediately before the if self._server_tool_executor:
return self._server_tool_executor branch so the OAuth DAL is refreshed on every
call even when reusing the cached _server_tool_executor.

In `@holmes/core/oauth_utils.py`:
- Around line 41-54: Wrap the network and JSON decode failures during token
exchange so they raise OAuthTokenExchangeError instead of bubbling out: catch
httpx.RequestError (including timeouts) around the httpx.post call that assigns
resp in oauth_utils (resp = httpx.post(...)) and re-raise as
OAuthTokenExchangeError(502, "<short detail including exception message>");
likewise wrap resp.json() (token_data = resp.json()) in a try/except catching
json.JSONDecodeError (or ValueError) and raise OAuthTokenExchangeError(502,
"Invalid JSON response: <exception message>") if decoding fails; keep the
existing checks for resp.status_code and missing "access_token" and include the
original exception messages in the error details for debugging.

In `@holmes/core/supabase_dal.py`:
- Around line 975-989: The get_oauth_token method drops the user filter when
user_id is None, causing it to return other users' tokens because
upsert_oauth_token stores unscoped rows with user_id=""; fix get_oauth_token to
always include a user_id equality filter: when user_id is provided use
.eq("user_id", user_id), otherwise use .eq("user_id", "") so only unscoped
tokens are returned. Update the query-building logic in get_oauth_token
(referencing OAUTH_TOKENS_TABLE and get_oauth_token) to apply that explicit
equality instead of omitting the filter.
- Around line 1025-1035: delete_oauth_token currently treats user_id=None as “no
filter” and will delete all users' tokens for the provider; to match
get_oauth_token's normalization, normalize the user_id before building the query
(e.g., if user_id is None set user_id = ""), then always add the .eq("user_id",
user_id) filter so the delete targets the global token when user_id is omitted
and does not unscoped-delete every user's record; update the delete_oauth_token
implementation to use this normalized user_id and the query-building logic
around it.

In `@holmes/core/tools_utils/tool_executor.py`:
- Around line 146-178: with_replaced_tools currently only updates executor
registries but leaves the original Toolset.tools list unchanged, causing later
syncs to re-register placeholders; when iterating replacements, create or clone
the matching Toolset object from new.enabled_toolsets (replace the entry in
new.enabled_toolsets) and set its .tools to the replacement list (apply the same
icon_url fallback as you do when populating new.tools_by_name), and also ensure
new._tool_to_toolset entries point to that cloned/updated Toolset so the
executor maps and the Toolset.tools stay in sync.

In `@holmes/plugins/toolsets/mcp/oauth_token_manager.py`:
- Around line 135-145: The DB-loaded token code reuses the original expires_in
instead of the DB's absolute token_expiry; change the logic in the block that
calls _load_from_db to compute remaining_ttl = int(db_token["token_expiry"] -
now) (use time.time() or datetime.utcnow() as appropriate), clamp it to a
non-negative value (e.g., max(0, remaining_ttl)), pass that remaining_ttl to
self._cache.set as expires_in and to self._schedule_background_refresh (instead
of db_token.get("expires_in", 300)), and keep refresh_token/refresh_expires_in
unchanged; if remaining_ttl is zero consider scheduling an immediate refresh via
the same _schedule_background_refresh call.

In `@holmes/plugins/toolsets/mcp/oauth_token_store.py`:
- Around line 96-124: The on-disk token file can be created with permissive
umask; update the token store so the file at self._path is created/chmod'ed with
0o600 to prevent group/world reads — e.g. in __init__ or in set() before/after
writing: ensure directory exists as now, then when creating/writing (in set)
either create via os.open(self._path, os.O_WRONLY|os.O_CREAT|os.O_TRUNC, 0o600)
or call os.chmod(self._path, 0o600) immediately after json.dump; apply the same
secure-perm logic when initializing an empty file in __init__ (use self._path,
_load, and _enabled to locate behavior) so mcp_tokens.json is always only
user-readable/writable.

In `@holmes/plugins/toolsets/mcp/toolset_mcp.py`:
- Around line 600-617: _inject_oauth_token is using
_token_manager.get_access_token without the disk-key that CLI tokens are
persisted under, so persisted tokens (stored under str(toolset._mcp_config.url))
are missed; update the call in _inject_oauth_token (involving RemoteMCPToolset
and MCPConfig) to pass the disk key derived from the MCP URL (e.g.,
str(toolset._mcp_config.url)) to _token_manager.get_access_token instead of
relying on OAuthTokenManager._default_disk_key(), so the persisted CLI token is
found when the in-memory cache is cold.
- Around line 845-858: with_replaced_tools is mutating shared RemoteMCPToolset
and tool_executor state (self.toolset.tools and
tool_executor.tools_by_name/_tool_to_toolset), leaking one user's discovered
tools to others; instead, create a new RemoteMCPToolset (or shallow-copy the
toolset object) and assign its .tools to real_tools locally, and replace the
executor mappings by copying the dicts (e.g. new_tools_by_name =
dict(tool_executor.tools_by_name) and new_tool_to_toolset =
dict(tool_executor._tool_to_toolset)) then update those copies with the new tool
entries for self.name and attach them only to the per-request executor instance
returned by with_replaced_tools; reference with_replaced_tools,
RemoteMCPToolset/self.toolset, tool_executor.tools_by_name,
tool_executor._tool_to_toolset, self.name, and real_tools when making these
non-mutating replacements.

In `@holmes/utils/holmes_sync_toolsets.py`:
- Around line 96-103: The code is leaking an internal DCR URL by serializing
MCPOAuthConfig.registration_endpoint into res["oauth"]; remove
registration_endpoint from the user-facing payload (the dict assigned to
res["oauth"]) so that oauth_config.registration_endpoint is not included in
DB/API/UI toolset metadata, but continue to keep
MCPOAuthConfig.registration_endpoint available for internal use where needed (do
not change its visibility on the model). Update the block that builds
res["oauth"] (the code assigning authorization_url, token_url, client_id,
scopes) to omit registration_endpoint and add a unit/integration test asserting
that registration_endpoint is not present in the serialized toolset metadata.
- Around line 50-53: The code calls get_config_meta_for_toolset(toolset) before
possibly populating toolset.installation_instructions via
get_config_schema_for_toolset(toolset), so meta is computed from an incomplete
toolset (causing RemoteMCPToolset.oauth_config to be lost); fix by deferring or
recomputing meta after ensuring installation_instructions is set — move the
get_config_meta_for_toolset(toolset) call to after the block that fills
toolset.installation_instructions (or call it again after
get_config_schema_for_toolset) so meta reflects the generated schema and
oauth_config is persisted.

In `@server.py`:
- Around line 307-323: process_oauth_callback is storing tokens without the
per-user scope, causing authenticated users not to see their tokens later;
obtain the current request user id from the request context
(request_context["user_id"]) before calling _get_toolset_oauth_config or after,
and pass that user id into token_manager.store_token so the token is stored
under the same user-scoped key used by /api/chat; update the call to
token_manager.store_token(oauth, token_data, user_id) or the equivalent
parameter your OAuthTokenManager API expects to ensure user-scoped storage.

In `@tests/llm/fixtures/test_ask_holmes/254_mcp_oauth/keycloak.yaml`:
- Around line 7-109: The manifest seeds sensitive credentials inline in
mcp-oauth-realm.json (client "mcp-server" secret "mcp-server-secret", user
"test-user" password "test-password", and bootstrap admin password referenced in
the Deployment env) — move these values into a Kubernetes Secret and update
references: create a Secret containing keys for mcp-server-secret,
test-user-password, and bootstrap-admin-password; modify the realm import
payload (mcp-oauth-realm.json) to reference the client secret and user
credential values from that Secret at runtime (or inject them into the realm
import container via envFrom/env), and update the Deployment to pull bootstrap
admin password and any other env vars from the same Secret instead of
hardcoding; ensure clientId "mcp-server", user "test-user", and the Deployment
env names are the ones you update.

In `@tests/llm/fixtures/test_ask_holmes/254_mcp_oauth/mcp-oauth-server.yaml`:
- Around line 351-357: The init container step (initContainers -> name:
install-deps) currently installs floating package versions; update the pip
install command in that container (the command list under install-deps) to pin
exact versions to match the repo lock: replace "mcp[cli]>=1.25.0" and the
unpinned httpx, uvicorn, pydantic entries with pinned versions
"mcp[cli]==1.25.0", "httpx==0.27.2", "uvicorn==0.40.0", and "pydantic==2.12.5"
so the eval fixture uses the same resolved dependencies as the repository.

In `@tests/llm/fixtures/test_ask_holmes/254_mcp_oauth/test_case.yaml`:
- Around line 113-116: The test hook hardcodes the token path to
~/.holmes/auth/mcp_tokens.json which breaks when HOLMES_CONFIGPATH_DIR overrides
Holmes' config dir; change the script to compute the token directory from the
HOLMES_CONFIGPATH_DIR environment variable (falling back to "$HOME/.holmes" if
unset) and write to "${CONFIG_DIR}/auth/mcp_tokens.json" instead of
"~/.holmes/..."; apply the same change to the other occurrences mentioned (lines
around the cleanup at 138-140) so the DiskTokenStore and test hooks operate on
the same config directory.
- Around line 1-9: The test currently hardcodes the verification code
"CF-EVAL-4x7k9m" in expected_output; instead generate a unique verification code
in the before_test setup and reference that generated value in expected_output.
Update the test fixture to create a seeded/unique verification string during
before_test (e.g., via a helper that returns a code) and ensure the
expected_output asserts against that runtime value rather than the literal
"CF-EVAL-4x7k9m"; keep the worker name checkout-api-handler and the log error
assertion (ERR-9872-TIMEOUT) intact so the LLM searches the listing for the
generated code and the checkout-api-handler logs.

---

Outside diff comments:
In `@holmes/core/tool_calling_llm.py`:
- Around line 250-306: The code reads decision.encrypted_token (which no longer
exists) and logs it, causing AttributeError; change logging and token-extraction
to use the current payload location (decision.decision or entries in
decision.save_prefixes with "__oauth_token__:"), avoid accessing
encrypted_token, and extract the OAuth payload before any retry/invocation so
the exchanged token is available; specifically update the handling in the loop
over pending_tool_calls (ToolCallWithDecision, tool_call, decision) to (1) stop
referencing decision.encrypted_token in the logging and extraction, (2) check
decision.decision first for an OAuth payload and fallback to scanning
decision.save_prefixes for "__oauth_token__:", remove that prefix when used,
call exchange_code_for_token(tool_call.id, payload, request_context) immediately
before invoking the tool retry path (so the token is present for
_invoke_llm_tool_call), and audit any other references to encrypted_token in
this function (including the other occurrence noted) to apply the same fix.

---

Nitpick comments:
In `@tests/llm/fixtures/test_ask_holmes/254_mcp_oauth/mcp_oauth_server.py`:
- Around line 164-169: KeycloakTokenVerifier.__init__ currently has untyped
parameters and add_wellknown_routes() contains function-local Starlette imports;
update the constructor signature for KeycloakTokenVerifier to annotate all
parameters as str (e.g., introspection_endpoint: str, server_url: str,
client_id: str, client_secret: str) and ensure any other constructors in this
file follow the same typing (also apply to the other constructor around the
indicated area). Move all Starlette imports used in add_wellknown_routes()
(e.g., Starlette, Route, JSONResponse, PlainTextResponse or whatever is imported
there) to the module top-level instead of importing inside the function, keeping
their original names so callers in add_wellknown_routes() continue to work.
Ensure imports remain at top of file and that type hints are added consistently
across similar fixtures.

🪄 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: 59ce5ef8-07f5-4c8f-bc47-aefdf6ba47a8

📥 Commits

Reviewing files that changed from the base of the PR and between 4e48278 and 68b2747.

📒 Files selected for processing (22)

• conftest.py
• docs/data-sources/remote-mcp-servers.md
• holmes/config.py
• holmes/core/models.py
• holmes/core/oauth_utils.py
• holmes/core/supabase_dal.py
• holmes/core/tool_calling_llm.py
• holmes/core/tools.py
• holmes/core/tools_utils/tool_executor.py
• holmes/plugins/toolsets/mcp/oauth_token_manager.py
• holmes/plugins/toolsets/mcp/oauth_token_store.py
• holmes/plugins/toolsets/mcp/toolset_mcp.py
• holmes/utils/holmes_sync_toolsets.py
• pyproject.toml
• server.py
• tests/llm/fixtures/test_ask_holmes/254_mcp_oauth/keycloak-realm.json
• tests/llm/fixtures/test_ask_holmes/254_mcp_oauth/keycloak.yaml
• tests/llm/fixtures/test_ask_holmes/254_mcp_oauth/mcp-oauth-server.yaml
• tests/llm/fixtures/test_ask_holmes/254_mcp_oauth/mcp_oauth_server.py
• tests/llm/fixtures/test_ask_holmes/254_mcp_oauth/test_case.yaml
• tests/llm/fixtures/test_ask_holmes/254_mcp_oauth/toolsets.yaml
• tests/test_mcp_oauth.py

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

holmes/core/tool_calling_llm.py (1)

150-152: Move the server import out of the helper.

This lazy import makes the dependency harder to follow and breaks the repo's Python import rule. If this was added to dodge a cycle, extract process_oauth_callback into a dependency-neutral module instead of importing it inside _try_process_oauth_decision().

As per coding guidelines, ALWAYS place Python imports at the top of the file, not inside functions or methods.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@holmes/core/tool_calling_llm.py` around lines 150 - 152, The import of
process_oauth_callback from server is performed lazily inside
_try_process_oauth_decision(), violating the top-level import rule; remove the
in-function import and either (A) move "from server import
process_oauth_callback" to the top of holmes/core/tool_calling_llm.py, or (B) if
that creates an import cycle, extract process_oauth_callback into a new
dependency-neutral module (e.g., holmes/core/oauth_utils) and import it at the
top, then update _try_process_oauth_decision() to call process_oauth_callback
without performing any local imports.

🤖 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/core/tool_calling_llm.py`:
- Around line 1095-1102: The current refresh only compares lengths, which misses
changes in tool identities/schemas; modify the refresh logic around
self._get_tools() and the local variable tools so it compares the actual tool
definitions (e.g., sets of tool names and/or signatures/schema hashes) rather
than len(new_tools) != len(tools). Call self._get_tools() into new_tools,
compute a deterministic representation for each tool (name plus
signature/schema/version or a hash), compare that set/list to the representation
of tools, and if they differ log the change and replace tools = new_tools so the
LLM sees updated tool names/schemas.
- Around line 293-297: The OAuth exchange result from
_try_process_oauth_decision must be checked and, on failure, the approved tool
call must be aborted; change the code inside the if tool_decision and
tool_decision.approved block to capture the return value of
_try_process_oauth_decision(tool_decision.decision), treat a falsy/failed return
(or an exception) as an OAuth failure, log an error, and skip/invalidate the
approved tool path so the approved tool is not invoked; if needed, update
_try_process_oauth_decision to return a boolean success flag or propagate an
exception so the caller can decide to abort the tool execution.

---

Nitpick comments:
In `@holmes/core/tool_calling_llm.py`:
- Around line 150-152: The import of process_oauth_callback from server is
performed lazily inside _try_process_oauth_decision(), violating the top-level
import rule; remove the in-function import and either (A) move "from server
import process_oauth_callback" to the top of holmes/core/tool_calling_llm.py, or
(B) if that creates an import cycle, extract process_oauth_callback into a new
dependency-neutral module (e.g., holmes/core/oauth_utils) and import it at the
top, then update _try_process_oauth_decision() to call process_oauth_callback
without performing any local imports.

🪄 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: 0e35b03d-408c-43a6-95a0-65a20f3eb0e2

📥 Commits

Reviewing files that changed from the base of the PR and between 68b2747 and 722a213.

📒 Files selected for processing (1)

• holmes/core/tool_calling_llm.py

[coderabbitai]

Actionable comments posted: 3

♻️ Duplicate comments (6)

holmes/core/oauth_utils.py (2)

104-126: ⚠️ Potential issue | 🟠 Major

Store callback tokens with the same request scope used for lookup.

store_token() is called without any request_context, so the callback writes the token under the unscoped cache/DB key (__no_user__ / user_id=None). Later /api/chat lookups use user-scoped keys and DB queries, so the user who just authenticated will not see this token. Thread request context or user identity through this helper and pass it to store_token().

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@holmes/core/oauth_utils.py` around lines 104 - 126, process_oauth_callback
currently calls mgr.store_token(oauth, token_data) without the request scope,
which writes tokens to the unscoped cache/DB key; instead, thread the same
request user/context used in lookups into the store call: retrieve the request
context/user identity from the OAuthCallbackRequest (or from
get_toolset_oauth_config if it returns a context), and pass it as the
request_context (or appropriate parameter name expected by mgr.store_token) so
tokens are stored under the user-scoped key; update the call site in
process_oauth_callback to pass that context to mgr.store_token and ensure
get_toolset_oauth_config and/or token_manager usage exposes the needed context
if not already available.

---

43-56: ⚠️ Potential issue | 🟠 Major

Normalize transport and JSON failures into OAuthTokenExchangeError.

Timeouts, DNS errors, and invalid JSON currently escape as generic exceptions, so /api/oauth/callback returns 500 instead of the intended 502 path for upstream IdP failures.

Suggested fix

-    resp = httpx.post(
-        token_url,
-        data=data,
-        headers={"Content-Type": "application/x-www-form-urlencoded"},
-        timeout=30,
-    )
+    try:
+        resp = httpx.post(
+            token_url,
+            data=data,
+            headers={"Content-Type": "application/x-www-form-urlencoded"},
+            timeout=30,
+        )
+    except httpx.RequestError as err:
+        raise OAuthTokenExchangeError(502, f"Token endpoint request failed: {err}") from err
@@
-    token_data = resp.json()
+    try:
+        token_data = resp.json()
+    except ValueError as err:
+        raise OAuthTokenExchangeError(502, f"Invalid JSON response: {err}") from err

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@holmes/core/oauth_utils.py` around lines 43 - 56, Wrap the external HTTP call
and JSON parsing around the httpx.post and resp.json() (the block creating resp
from token_url and token_data = resp.json()) in a try/except that catches
httpx.RequestError (and related httpx exceptions like TimeoutException) and JSON
parsing errors (ValueError/JSONDecodeError) and re-raise them as
OAuthTokenExchangeError with a 502 status and the original error
message/details; preserve the existing behavior for non-200 responses and
missing "access_token" but ensure all transport/JSON failures produce
OAuthTokenExchangeError so the /api/oauth/callback path treats IdP failures as
upstream errors.

holmes/plugins/toolsets/mcp/toolset_mcp.py (2)

571-582: ⚠️ Potential issue | 🟠 Major

Pass the MCP URL disk key when resolving persisted CLI tokens.

CLI tokens are stored under str(toolset._mcp_config.url), but this lookup falls back to the default auth-url-based disk key. After a cold start, _inject_oauth_token() misses the persisted token and sends unauthenticated requests even though the CLI login already succeeded.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@holmes/plugins/toolsets/mcp/toolset_mcp.py` around lines 571 - 582,
_inject_oauth_token currently calls _token_manager.get_access_token without
supplying the MCP-specific disk key, so persisted CLI tokens saved under
str(toolset._mcp_config.url) are missed; update the call to
_token_manager.get_access_token(oauth_config, request_context,
disk_key=str(toolset._mcp_config.url)) (or the equivalent parameter name used by
_token_manager.get_access_token) so the token lookup uses the
toolset._mcp_config.url as the auth disk key and returns the persisted token
after cold start.

---

813-826: ⚠️ Potential issue | 🟠 Major

Don’t mutate the shared toolset/executor during OAuth connect.

This updates self.toolset.tools and the live executor registries in place. Those objects are shared across requests, so one user’s discovered tool list can leak into other sessions. Build a per-request replacement toolset/executor instead of mutating the shared ones.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@holmes/plugins/toolsets/mcp/toolset_mcp.py` around lines 813 - 826, The code
mutates shared state (self.toolset.tools and tool_executor registries) during
OAuth connect which can leak tools across requests; instead create per-request
replacements: construct a new Toolset instance or shallow-copy of self.toolset
and set its tools to real_tools (do not assign to self.toolset.tools), and
create or clone a per-request tool_executor (copying necessary attributes but
with fresh tools_by_name and _tool_to_toolset dicts), then register each tool
into those per-request dicts mapping tool.name -> tool and tool.name ->
new_toolset (use symbols self.toolset, real_tools, self.name, tool_executor,
tools_by_name, _tool_to_toolset to locate spots to change). Finally ensure the
rest of the request uses these new per-request toolset and executor instead of
mutating the shared ones.

holmes/plugins/toolsets/mcp/oauth_token_manager.py (1)

127-138: ⚠️ Potential issue | 🟠 Major

Use the DB row’s absolute expiry when rehydrating the cache.

This still re-caches a DB-loaded token with its original expires_in, not the remaining TTL. A token written 25 minutes ago with expires_in=1800 gets another fresh 30 minutes here, so Holmes can return an already-expired access token until the next refresh path runs. _load_from_db() needs to surface the stored absolute expiry and this branch should cache/schedule from the remaining lifetime instead.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@holmes/plugins/toolsets/mcp/oauth_token_manager.py` around lines 127 - 138,
The DB-loaded token branch currently reuses the stored expires_in rather than
the remaining TTL; update _load_from_db to return the token's absolute expiry
(e.g., "expires_at" timestamp) or otherwise expose the stored expiry, then in
the branch that handles db_token compute remaining_ttl = max(0, expires_at -
now); if remaining_ttl <= 0 treat the token as expired (don't cache or trigger
refresh) otherwise call self._cache.set(..., expires_in=remaining_ttl,
refresh_token=..., refresh_expires_in=...) and call
self._schedule_background_refresh(cache_key, oauth_config, remaining_ttl,
user_id). Ensure you reference db_token, _load_from_db, _cache.set and
_schedule_background_refresh when making these changes.

holmes/core/tools_utils/tool_executor.py (1)

152-184: ⚠️ Potential issue | 🟠 Major

Keep the cloned Toolset in sync with the registry replacements.

with_replaced_tools() only rewrites tools_by_name and _tool_to_toolset. new.enabled_toolsets still holds the original placeholder tools, so any later _sync_dynamic_tools() miss can re-register the placeholder you just removed. Clone/update the matching Toolset and replace its .tools list as part of this path.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@holmes/core/tools_utils/tool_executor.py` around lines 152 - 184,
with_replaced_tools currently updates tools_by_name and _tool_to_toolset but
leaves the Toolset objects in new.enabled_toolsets unchanged, so later
_sync_dynamic_tools can re-register the original placeholders; fix by finding
the matching Toolset in new.enabled_toolsets (same loop that finds `toolset`)
and replace or update its Toolset.tools list to the replacement `new_tools`
(applying the same icon_url fallback logic used when inserting into
tools_by_name), ensuring new._tool_to_toolset and new.tools_by_name remain
consistent with the updated Toolset so _sync_dynamic_tools won't restore
placeholders.

🧹 Nitpick comments (1)

tests/test_mcp_oauth.py (1)

1426-1459: Cover the placeholder-resurrection case in these replacement tests.

These assertions only verify tools_by_name. The current bug is that with_replaced_tools() can leave Toolset.tools stale, so a later lookup miss triggers _sync_dynamic_tools() and re-registers the placeholder. Please also assert that the cloned toolset’s .tools list was replaced and that the old placeholder stays gone after a miss.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_mcp_oauth.py` around lines 1426 - 1459, The tests must cover the
placeholder-resurrection bug by asserting that with_replaced_tools actually
replaces the Toolset.tools list (not just tools_by_name) and that a subsequent
lookup miss does not re-register the placeholder; update the two tests to (1)
inspect the returned augmented toolset object from
executor.with_replaced_tools("my-mcp") and assert augmented_toolset.tools
contains the new real tool objects and does not contain the original
placeholder, and (2) force a lookup miss that triggers
ToolExecutor._sync_dynamic_tools (e.g., call whatever lookup method triggers a
miss or remove the tool from tools_by_name) and then assert the placeholder is
still absent from both augmented.tools and augmented.tools_by_name; reference
the methods/classes ToolExecutor.with_replaced_tools, Toolset.tools,
ToolExecutor._sync_dynamic_tools, and tools_by_name when making the assertions
so the test proves the placeholder isn’t resurrected.

🤖 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/mcp/oauth_token_manager.py`:
- Around line 199-203: The info logs in OAuthTokenManager that currently print
raw cache_key (e.g., the logger.info call after
self._schedule_background_refresh and other logger lines referencing
cache_key/user_id) leak request-scoped identifiers; update those log statements
to emit a redacted or hashed representation of cache_key/user_id (compute a
stable hash or redact sensitive parts before logging) and apply the same change
to every other occurrence in this class where cache_key or user_id is logged so
no raw identifiers appear in application logs; keep the log text and variables
otherwise unchanged (e.g., still log expires_in and has_refresh) while replacing
cache_key/user_id with the redacted/hash value.
- Around line 480-484: The existing logic sets expiry from refresh_expires_in
before expires_in which records the refresh-token lifetime as the record's
token_expiry; change the assignment so token_data["expires_in"] (the
access-token lifetime) is used to compute expiry first and only fall back to
token_data["refresh_expires_in"] when expires_in is missing, and if you need to
persist refresh lifetime store it separately (e.g., as refresh_expiry or
refresh_token_expiry) rather than overriding token_expiry; update the code
around expiry/token_data handling in oauth_token_manager.py accordingly.

In `@holmes/plugins/toolsets/mcp/toolset_mcp.py`:
- Around line 372-389: The callback handler currently accepts any returned code
without verifying the OAuth "state"; update CallbackHandler.do_GET to validate
the returned params.get("state", [None])[0] against the original generated state
(e.g., the outer-scope variable named state or expected_state) and only set
result["code"] when they match; if missing or mismatched, set
result["error"]="invalid_state" (and error_description accordingly), send a 400
response and do not proceed with exchanging the code; ensure
callback_event.set() is still called so the flow unblocks.

---

Duplicate comments:
In `@holmes/core/oauth_utils.py`:
- Around line 104-126: process_oauth_callback currently calls
mgr.store_token(oauth, token_data) without the request scope, which writes
tokens to the unscoped cache/DB key; instead, thread the same request
user/context used in lookups into the store call: retrieve the request
context/user identity from the OAuthCallbackRequest (or from
get_toolset_oauth_config if it returns a context), and pass it as the
request_context (or appropriate parameter name expected by mgr.store_token) so
tokens are stored under the user-scoped key; update the call site in
process_oauth_callback to pass that context to mgr.store_token and ensure
get_toolset_oauth_config and/or token_manager usage exposes the needed context
if not already available.
- Around line 43-56: Wrap the external HTTP call and JSON parsing around the
httpx.post and resp.json() (the block creating resp from token_url and
token_data = resp.json()) in a try/except that catches httpx.RequestError (and
related httpx exceptions like TimeoutException) and JSON parsing errors
(ValueError/JSONDecodeError) and re-raise them as OAuthTokenExchangeError with a
502 status and the original error message/details; preserve the existing
behavior for non-200 responses and missing "access_token" but ensure all
transport/JSON failures produce OAuthTokenExchangeError so the
/api/oauth/callback path treats IdP failures as upstream errors.

In `@holmes/core/tools_utils/tool_executor.py`:
- Around line 152-184: with_replaced_tools currently updates tools_by_name and
_tool_to_toolset but leaves the Toolset objects in new.enabled_toolsets
unchanged, so later _sync_dynamic_tools can re-register the original
placeholders; fix by finding the matching Toolset in new.enabled_toolsets (same
loop that finds `toolset`) and replace or update its Toolset.tools list to the
replacement `new_tools` (applying the same icon_url fallback logic used when
inserting into tools_by_name), ensuring new._tool_to_toolset and
new.tools_by_name remain consistent with the updated Toolset so
_sync_dynamic_tools won't restore placeholders.

In `@holmes/plugins/toolsets/mcp/oauth_token_manager.py`:
- Around line 127-138: The DB-loaded token branch currently reuses the stored
expires_in rather than the remaining TTL; update _load_from_db to return the
token's absolute expiry (e.g., "expires_at" timestamp) or otherwise expose the
stored expiry, then in the branch that handles db_token compute remaining_ttl =
max(0, expires_at - now); if remaining_ttl <= 0 treat the token as expired
(don't cache or trigger refresh) otherwise call self._cache.set(...,
expires_in=remaining_ttl, refresh_token=..., refresh_expires_in=...) and call
self._schedule_background_refresh(cache_key, oauth_config, remaining_ttl,
user_id). Ensure you reference db_token, _load_from_db, _cache.set and
_schedule_background_refresh when making these changes.

In `@holmes/plugins/toolsets/mcp/toolset_mcp.py`:
- Around line 571-582: _inject_oauth_token currently calls
_token_manager.get_access_token without supplying the MCP-specific disk key, so
persisted CLI tokens saved under str(toolset._mcp_config.url) are missed; update
the call to _token_manager.get_access_token(oauth_config, request_context,
disk_key=str(toolset._mcp_config.url)) (or the equivalent parameter name used by
_token_manager.get_access_token) so the token lookup uses the
toolset._mcp_config.url as the auth disk key and returns the persisted token
after cold start.
- Around line 813-826: The code mutates shared state (self.toolset.tools and
tool_executor registries) during OAuth connect which can leak tools across
requests; instead create per-request replacements: construct a new Toolset
instance or shallow-copy of self.toolset and set its tools to real_tools (do not
assign to self.toolset.tools), and create or clone a per-request tool_executor
(copying necessary attributes but with fresh tools_by_name and _tool_to_toolset
dicts), then register each tool into those per-request dicts mapping tool.name
-> tool and tool.name -> new_toolset (use symbols self.toolset, real_tools,
self.name, tool_executor, tools_by_name, _tool_to_toolset to locate spots to
change). Finally ensure the rest of the request uses these new per-request
toolset and executor instead of mutating the shared ones.

---

Nitpick comments:
In `@tests/test_mcp_oauth.py`:
- Around line 1426-1459: The tests must cover the placeholder-resurrection bug
by asserting that with_replaced_tools actually replaces the Toolset.tools list
(not just tools_by_name) and that a subsequent lookup miss does not re-register
the placeholder; update the two tests to (1) inspect the returned augmented
toolset object from executor.with_replaced_tools("my-mcp") and assert
augmented_toolset.tools contains the new real tool objects and does not contain
the original placeholder, and (2) force a lookup miss that triggers
ToolExecutor._sync_dynamic_tools (e.g., call whatever lookup method triggers a
miss or remove the tool from tools_by_name) and then assert the placeholder is
still absent from both augmented.tools and augmented.tools_by_name; reference
the methods/classes ToolExecutor.with_replaced_tools, Toolset.tools,
ToolExecutor._sync_dynamic_tools, and tools_by_name when making the assertions
so the test proves the placeholder isn’t resurrected.

🪄 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: 14b86e99-9aab-45e2-ab02-3a88e8310fb6

📥 Commits

Reviewing files that changed from the base of the PR and between 722a213 and 9e85d83.

📒 Files selected for processing (8)

• holmes/core/models.py
• holmes/core/oauth_utils.py
• holmes/core/tool_calling_llm.py
• holmes/core/tools_utils/tool_executor.py
• holmes/plugins/toolsets/mcp/oauth_token_manager.py
• holmes/plugins/toolsets/mcp/toolset_mcp.py
• server.py
• tests/test_mcp_oauth.py

🚧 Files skipped from review as they are similar to previous changes (1)

• holmes/core/tool_calling_llm.py

[coderabbitai]

Actionable comments posted: 3

♻️ Duplicate comments (6)

holmes/plugins/toolsets/mcp/toolset_mcp.py (3)

569-586: ⚠️ Potential issue | 🟠 Major

Pass the MCP URL disk key when resolving persisted CLI tokens here.

CLI auth stores tokens under str(self.toolset._mcp_config.url) in Lines 736-739, but _inject_oauth_token() falls back to the manager default key. Once the in-memory cache is cold, persisted CLI tokens will be missed and authenticated requests regress to 401s.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@holmes/plugins/toolsets/mcp/toolset_mcp.py` around lines 569 - 586,
_inject_oauth_token currently calls
_token_manager.get_access_token(oauth_config, request_context) which uses the
manager default disk key and therefore misses CLI-persisted tokens stored under
the MCP URL; update the call in _inject_oauth_token to pass the MCP URL disk key
(e.g. use str(toolset._mcp_config.url)) so get_access_token resolves persisted
CLI tokens for this toolset, ensuring you still reference toolset._mcp_config
and MCPConfig to locate the url when building that disk key.

---

372-389: ⚠️ Potential issue | 🔴 Critical

Validate the OAuth state before accepting the callback.

The CLI flow generates state, but CallbackHandler.do_GET() accepts any returned code without comparing it. That drops the CSRF/session-binding check for this OAuth flow.

Suggested direction

         def do_GET(self):
             parsed = urlparse(self.path)
             params = parse_qs(parsed.query)
-            if "code" in params:
+            returned_state = params.get("state", [None])[0]
+            if returned_state != state:
+                result["error"] = "invalid_state"
+                result["error_description"] = "OAuth state mismatch"
+                self.send_response(400)
+                self.send_header("Content-Type", "text/html")
+                self.end_headers()
+                self.wfile.write(b"<h1>Error: invalid_state</h1>")
+            elif "code" in params:
                 result["code"] = params["code"][0]
                 self.send_response(200)
                 self.send_header("Content-Type", "text/html")
                 self.end_headers()
                 self.wfile.write(b"<h1>Authenticated! You can close this tab.</h1>")
             else:
                 result["error"] = params.get("error", ["unknown"])[0]
                 result["error_description"] = params.get("error_description", [""])[0]
                 self.send_response(400)
                 self.send_header("Content-Type", "text/html")
                 self.end_headers()
                 self.wfile.write(f"<h1>Error: {result['error']}</h1>".encode())
             callback_event.set()

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@holmes/plugins/toolsets/mcp/toolset_mcp.py` around lines 372 - 389,
CallbackHandler.do_GET currently accepts any returned code without verifying the
OAuth `state`; update do_GET to read the incoming "state" query param and
compare it to the expected state value generated by the CLI (the
outer-scope/state variable used when starting the server). If the states match,
proceed to set result["code"] and respond 200 as before; if they do not match or
"state" is missing, set result["error"]="invalid_state" (and optionally
result["error_description"]) and respond 400 without storing result["code"]; in
all cases ensure callback_event.set() is still called. Locate the
CallbackHandler class and do_GET method to add the state check and use the
existing result and callback_event symbols.

---

811-824: ⚠️ Potential issue | 🟠 Major

Avoid mutating the shared toolset/executor during OAuth connect.

This branch rewrites self.toolset.tools and tool_executor.tools_by_name/_tool_to_toolset in place. Because those objects are shared, one user's discovered tool list can leak into other requests.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@holmes/plugins/toolsets/mcp/toolset_mcp.py` around lines 811 - 824, The code
mutates shared state (self.toolset.tools and
tool_executor.tools_by_name/_tool_to_toolset) which can leak tools between
requests; instead create request-scoped copies and register those without
altering globals: clone the tools list (e.g., new_tools = list(real_tools)) and
assign it to a request-local toolset/variable rather than self.toolset.tools,
and when registering with the executor avoid mutating its global maps—either
build a temporary mapping for this request or clone the executor's maps, update
the clones with entries for each tool.name (using self.name and real_tools to
locate items), and use those clones for the current operation (or restore
originals after use) so shared objects are never overwritten in place.

holmes/core/tool_calling_llm.py (2)

1092-1099: ⚠️ Potential issue | 🟠 Major

Refresh tools on definition changes, not just count changes.

OAuth discovery can swap placeholder tools for real tools without changing the total count. With len(new_tools) != len(tools), the next LLM turn can keep stale tool names/schema.

Suggested direction

                 if tools is not None:
                     new_tools = self._get_tools()
-                    if len(new_tools) != len(tools):
+                    if json.dumps(new_tools, sort_keys=True) != json.dumps(
+                        tools, sort_keys=True
+                    ):
                         logging.warning(
                             f"Tool list changed - refreshing ({len(tools)} -> {len(new_tools)} tools)"
                         )
                         tools = new_tools

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@holmes/core/tool_calling_llm.py` around lines 1092 - 1099, The refresh logic
in the tool fetching block only checks len(new_tools) != len(tools) so
replacements (e.g., OAuth discovery swapping placeholders for real tools) with
the same count won't trigger a refresh; update the check in the method
containing self._get_tools() to do a content-level comparison between tools and
new_tools (for example compare tool IDs/names and signatures/metadata or perform
a shallow deep-equality on each tool object) instead of only comparing lengths,
and if any difference is found (name/schema/description/params changed) assign
tools = new_tools and emit the warning; reference the existing variables tools
and new_tools and the helper self._get_tools() when making the change.

---

143-157: ⚠️ Potential issue | 🟠 Major

Abort the approved tool call when the OAuth exchange fails.

_try_process_oauth_decision() only logs failures, and _execute_tool_decisions() immediately invokes the approved tool anyway. That turns a clear auth failure into a second downstream tool call against an unauthenticated session.

Suggested direction

-def _try_process_oauth_decision(
+def _try_process_oauth_decision(
     tool_call_id: str,
     decision_data: Dict[str, Any],
     request_context: Optional[Dict[str, Any]],
-) -> None:
+) -> Optional[str]:
@@
     try:
         payload_json = json.dumps(decision_data)
         exchange_code_for_token(tool_call_id, payload_json, request_context)
+        return None
     except Exception as e:
         logging.error(f"Failed to process OAuth decision: {e}", exc_info=True)
+        return str(e)
@@
                 if tool_decision.decision:
-                    _try_process_oauth_decision(tool_call.id, tool_decision.decision, request_context)
+                    oauth_error = _try_process_oauth_decision(
+                        tool_call.id, tool_decision.decision, request_context
+                    )
+                    if oauth_error:
+                        tool_result = ToolCallResult(
+                            tool_call_id=tool_call.id,
+                            tool_name=tool_call.function.name,
+                            description=tool_call.function.name,
+                            result=StructuredToolResult(
+                                status=StructuredToolResultStatus.ERROR,
+                                error=oauth_error,
+                            ),
+                        )
+                        continue

Also applies to: 290-304

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@holmes/core/tool_calling_llm.py` around lines 143 - 157, The OAuth exchange
failure in _try_process_oauth_decision currently only logs the error so
execution continues; change it to abort the approved tool call by propagating
the failure: catch exceptions from exchange_code_for_token, log with exc_info,
then either raise a specific exception (e.g., OAuthExchangeError) or re-raise
the caught exception so callers like _execute_tool_decisions see the failure and
can stop invoking the approved tool; ensure the chosen exception type is
imported/defined and update any caller logic to handle/propagate that exception
to abort the tool execution.

holmes/core/oauth_utils.py (1)

43-56: ⚠️ Potential issue | 🟠 Major

Wrap transport and JSON failures in OAuthTokenExchangeError.

httpx.post() timeouts/request errors and resp.json() decode failures currently bubble out as generic exceptions, so callers miss the intended token-exchange error path and return a 500 instead of a handled upstream failure.

Suggested direction

-    resp = httpx.post(
-        token_url,
-        data=data,
-        headers={"Content-Type": "application/x-www-form-urlencoded"},
-        timeout=30,
-    )
+    try:
+        resp = httpx.post(
+            token_url,
+            data=data,
+            headers={"Content-Type": "application/x-www-form-urlencoded"},
+            timeout=30,
+        )
+    except httpx.RequestError as e:
+        raise OAuthTokenExchangeError(502, f"Token endpoint request failed: {e}") from e
@@
-    token_data = resp.json()
+    try:
+        token_data = resp.json()
+    except ValueError as e:
+        raise OAuthTokenExchangeError(502, f"Invalid JSON response: {e}") from e

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@holmes/core/oauth_utils.py` around lines 43 - 56, The code calls
httpx.post(...) and resp.json() directly which lets httpx.RequestError/Timeout
and JSON decode exceptions escape; wrap the httpx.post call in a try/except
catching httpx.RequestError (and httpx.TimeoutException if desired) and re-raise
OAuthTokenExchangeError with a useful message and a non-200 pseudo-status (e.g.
0 or the underlying exception details), and likewise wrap resp.json() in
try/except catching JSONDecodeError/ValueError to raise OAuthTokenExchangeError
with the response text/snippet and parsing error; update the logic around resp,
resp.status_code, and token_data so all transport and JSON failures are
translated into OAuthTokenExchangeError (referencing httpx.post, resp,
resp.json(), and OAuthTokenExchangeError).

🤖 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/mcp/toolset_mcp.py`:
- Around line 1376-1385: The code currently only logs when both
oauth_config.client_id and registration_endpoint are missing, but still reports
discovery success; change the branch in the if not oauth_config.client_id block
so that when registration_endpoint is also falsy you log an error (including
self.name and that both client_id and registration_endpoint are missing) and
fail discovery immediately (e.g., raise a specific exception or return a failure
result) instead of continuing; ensure the code path that emits the approval
payload honors this failure so no successful discovery/approval is reported when
neither oauth_config.client_id nor registration_endpoint is available.
- Line 391: The method log_message currently uses a parameter named format which
shadows the built-in and lacks type hints; rename that parameter to
format_string (or similar) and add type annotations to the signature (e.g.,
format_string: str, *args: Any) and a return type of None; also ensure Any is
imported from typing at top of the module if not already. Update any internal
references to the old parameter name inside log_message to the new name and run
linters to verify Ruff A002 and typing rules are satisfied.
- Around line 1247-1255: The health-check fallback currently uses a
non-idempotent POST to probe reachability; update
_check_oauth_server_reachable() to use a read-only method (HEAD or GET) instead
of httpx.post for the root probe so startup checks cannot cause side-effects.
Replace the httpx.post(url, ...) call with a httpx.head(url, timeout=...,
verify=self._mcp_config.verify_ssl) (or httpx.get if HEAD is unsupported by some
endpoints), preserving the same timeout and verify parameters and handling
status_code checks the same way.

---

Duplicate comments:
In `@holmes/core/oauth_utils.py`:
- Around line 43-56: The code calls httpx.post(...) and resp.json() directly
which lets httpx.RequestError/Timeout and JSON decode exceptions escape; wrap
the httpx.post call in a try/except catching httpx.RequestError (and
httpx.TimeoutException if desired) and re-raise OAuthTokenExchangeError with a
useful message and a non-200 pseudo-status (e.g. 0 or the underlying exception
details), and likewise wrap resp.json() in try/except catching
JSONDecodeError/ValueError to raise OAuthTokenExchangeError with the response
text/snippet and parsing error; update the logic around resp, resp.status_code,
and token_data so all transport and JSON failures are translated into
OAuthTokenExchangeError (referencing httpx.post, resp, resp.json(), and
OAuthTokenExchangeError).

In `@holmes/core/tool_calling_llm.py`:
- Around line 1092-1099: The refresh logic in the tool fetching block only
checks len(new_tools) != len(tools) so replacements (e.g., OAuth discovery
swapping placeholders for real tools) with the same count won't trigger a
refresh; update the check in the method containing self._get_tools() to do a
content-level comparison between tools and new_tools (for example compare tool
IDs/names and signatures/metadata or perform a shallow deep-equality on each
tool object) instead of only comparing lengths, and if any difference is found
(name/schema/description/params changed) assign tools = new_tools and emit the
warning; reference the existing variables tools and new_tools and the helper
self._get_tools() when making the change.
- Around line 143-157: The OAuth exchange failure in _try_process_oauth_decision
currently only logs the error so execution continues; change it to abort the
approved tool call by propagating the failure: catch exceptions from
exchange_code_for_token, log with exc_info, then either raise a specific
exception (e.g., OAuthExchangeError) or re-raise the caught exception so callers
like _execute_tool_decisions see the failure and can stop invoking the approved
tool; ensure the chosen exception type is imported/defined and update any caller
logic to handle/propagate that exception to abort the tool execution.

In `@holmes/plugins/toolsets/mcp/toolset_mcp.py`:
- Around line 569-586: _inject_oauth_token currently calls
_token_manager.get_access_token(oauth_config, request_context) which uses the
manager default disk key and therefore misses CLI-persisted tokens stored under
the MCP URL; update the call in _inject_oauth_token to pass the MCP URL disk key
(e.g. use str(toolset._mcp_config.url)) so get_access_token resolves persisted
CLI tokens for this toolset, ensuring you still reference toolset._mcp_config
and MCPConfig to locate the url when building that disk key.
- Around line 372-389: CallbackHandler.do_GET currently accepts any returned
code without verifying the OAuth `state`; update do_GET to read the incoming
"state" query param and compare it to the expected state value generated by the
CLI (the outer-scope/state variable used when starting the server). If the
states match, proceed to set result["code"] and respond 200 as before; if they
do not match or "state" is missing, set result["error"]="invalid_state" (and
optionally result["error_description"]) and respond 400 without storing
result["code"]; in all cases ensure callback_event.set() is still called. Locate
the CallbackHandler class and do_GET method to add the state check and use the
existing result and callback_event symbols.
- Around line 811-824: The code mutates shared state (self.toolset.tools and
tool_executor.tools_by_name/_tool_to_toolset) which can leak tools between
requests; instead create request-scoped copies and register those without
altering globals: clone the tools list (e.g., new_tools = list(real_tools)) and
assign it to a request-local toolset/variable rather than self.toolset.tools,
and when registering with the executor avoid mutating its global maps—either
build a temporary mapping for this request or clone the executor's maps, update
the clones with entries for each tool.name (using self.name and real_tools to
locate items), and use those clones for the current operation (or restore
originals after use) so shared objects are never overwritten in place.

🪄 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: 039ae2b4-45ce-4f7b-a626-28f3285994a3

📥 Commits

Reviewing files that changed from the base of the PR and between 9e85d83 and 4898a60.

📒 Files selected for processing (5)

• holmes/core/models.py
• holmes/core/oauth_utils.py
• holmes/core/supabase_dal.py
• holmes/core/tool_calling_llm.py
• holmes/plugins/toolsets/mcp/toolset_mcp.py

🚧 Files skipped from review as they are similar to previous changes (1)

• holmes/core/supabase_dal.py

[claude]

🟡 Line 37 imports OAuthTokenExchangeError from holmes.core.oauth_config, but the name is never referenced anywhere else in the file — _try_process_oauth_decision catches generic Exception rather than this specific type. The other two imports on that line (_get_exchange_manager, parse_oauth_decision) are used, so trim the import to from holmes.core.oauth_config import _get_exchange_manager, parse_oauth_decision. Note: the sibling from json import tool on line 3 is also dead (every tool reference in the file is a local variable that shadows the import) — same file, worth cleaning up together.

Extended reasoning...

What the bug is\n\nLine 37 of holmes/core/tool_calling_llm.py reads:\n\npython\nfrom holmes.core.oauth_config import OAuthTokenExchangeError, _get_exchange_manager, parse_oauth_decision\n\n\nOf the three names imported, OAuthTokenExchangeError is never referenced. The other two are used inside _try_process_oauth_decision and _execute_tool_decisions.\n\n### Step-by-step proof\n\n1. grep -n 'OAuthTokenExchangeError' holmes/core/tool_calling_llm.py → exactly one hit, line 37 (the import itself).\n2. The nearby function _try_process_oauth_decision (line 159) is the natural candidate for catching this exception, but it uses a blanket except Exception as e: rather than the specific type:\n python\n def _try_process_oauth_decision(tool_call_id, oauth_code, request_context) -> bool:\n try:\n _get_exchange_manager().complete_exchange(tool_call_id, oauth_code, request_context)\n return True\n except Exception as e:\n logging.error(f"Failed to process OAuth decision: {e}", exc_info=True)\n return False\n \n3. There is no type hint, no isinstance check, and no raise of OAuthTokenExchangeError anywhere else in the file. The name is purely dead.\n\n### Impact\n\nZero runtime impact. Pure code-hygiene: misleads readers into thinking the file handles this specific exception type, and will trip linters configured for unused-import detection (F401). Severity is nit.\n\n### Companion issue\n\nLine 3 of the same file has from json import tool, which imports the stdlib json.tool CLI pretty-printer. Every subsequent tool identifier in the file is a local variable (e.g., line 669 tool = self.tool_executor.get_tool_by_name(...)) that shadows the import. That is also dead. An earlier CodeRabbit review already flagged it, so bundling the two unused-import fixes in the same cleanup avoids a second round-trip.\n\n### How to fix\n\ndiff\n-from json import tool\n ...\n-from holmes.core.oauth_config import OAuthTokenExchangeError, _get_exchange_manager, parse_oauth_decision\n+from holmes.core.oauth_config import _get_exchange_manager, parse_oauth_decision\n

[claude]

🟡 The return type annotation on require_user_id (oauth_token_manager.py:228) says -> str, but line 238 returns None when DalTokenStore is active and no user_id is in the request context (see commit b4f5e46). The docstring is also stale — it still says "raises ValueError if user_id is missing in Server mode" even though the code now returns None. No runtime crash today (callers either use the return as a dict key via dict.get(...), which tolerates None, or pass it into functions like store_user_tools/upsert_oauth_token that already short-circuit on falsy input), but the annotation hides the None case from mypy/pyright and misleads future readers. Update the annotation to Optional[str] and fix the docstring to match the new return-None contract.

Extended reasoning...

What the bug is

At holmes/plugins/toolsets/mcp/oauth_token_manager.py:228-239, OAuthTokenManager.require_user_id is declared:

def require_user_id(self, request_context: Optional[Dict[str, Any]]) -> str:
    """Return a user_id, using DEFAULT_CLI_USER in CLI mode or raising in server mode.

    CLI mode (DiskTokenStore / no store): returns DEFAULT_CLI_USER.
    Server mode (DalTokenStore): raises ValueError if user_id is missing.
    """
    user_id = _get_user_id(request_context)
    if user_id:
        return user_id
    if isinstance(self._store, DalTokenStore):
        return None  # <-- violates `-> str` annotation
    return DEFAULT_CLI_USER

Commit b4f5e46 (require_user_id returns None in server mode instead of raising) intentionally changed line 238 from raise ValueError(...) to return None, but neither the signature nor the docstring was updated. The annotation now lies about the contract, and the docstring contradicts the code.

How it manifests

Static type checkers (mypy, pyright) trust the -> str annotation and will not flag callers that assume a non-None result. Human readers of the docstring will expect a ValueError that will never arrive.

The specific code paths that use the return value

• oauth_tool_connector.py:153, 193, 203 — key = user_id or _get_token_manager().require_user_id(None), then self._user_tools.get(key) / self._user_tool_to_toolset.get(key, {}).get(tool_name). dict.get(None) works fine as a lookup and simply returns None/no-match, so no crash.
• oauth_tool_connector.py:75 — user_id = _get_token_manager().require_user_id(request_context) is passed to load_tools_for_user(user_id, ...) which does user_id[:6] if user_id else user_id (None-safe) and to store_user_tools(user_id, ...) which uses user_id only as a dict key.
• tool_calling_llm.py:697 — effective_user = _get_token_manager().require_user_id(request_context) then store_user_tools(effective_user, toolset_name, ...). store_user_tools annotates user_id: str but operates on it purely as a dict key, so passing None does not crash.
• supabase_dal.upsert_oauth_token at supabase_dal.py:1001-1003 already short-circuits on if not user_id: logging.warning(...); return False, so any DB write path with None user_id is a silent no-op rather than a crash.

Why existing code does not prevent it

There is no runtime assertion that the return is non-None, and the annotation is the only line-of-defense for type-checkers. The existing tolerant-of-None downstream code masks the annotation lie at runtime.

Impact

No runtime incident today — this is a documentation/type-safety defect. Severity is nit. But the lie will bite when a future caller adds a .startswith("user-") or similar str operation on the result, or when someone refactors store_user_tools to enforce its user_id: str annotation.

How to fix

One-line annotation fix plus docstring rewrite:

def require_user_id(self, request_context: Optional[Dict[str, Any]]) -> Optional[str]:
    """Return a user_id, or None when none can be determined.

    - CLI mode (DiskTokenStore / no store): returns DEFAULT_CLI_USER as fallback.
    - Server mode (DalTokenStore): returns None when no user_id is present in the
      request context (callers must handle None — typically as a no-op for lookups
      and an early return for writes).
    """

Step-by-step proof

1. Call manager.require_user_id({}) in server mode (self._store is a DalTokenStore).
2. Line 234: _get_user_id({}) returns None.
3. Line 235: if user_id: → False.
4. Line 237: isinstance(self._store, DalTokenStore) → True.
5. Line 238: return None.
6. Annotation -> str is violated. Any caller that a type-checker treats as returning str will not be flagged on this path.

[claude]

🟡 The variable user_id = _get_user_id(context.request_context) on line 303 is assigned but never used anywhere else in requires_approval. The subsequent CLI branching uses context.request_context is None directly and passes context.request_context to downstream calls — user_id is dead code. Either delete the assignment or reference it in the CLI-mode log message for traceability.

Extended reasoning...

What the bug is

At holmes/plugins/toolsets/mcp/toolset_mcp.py:303, inside RemoteMCPTool.requires_approval, the line user_id = _get_user_id(context.request_context) binds a local variable that is never read in the remainder of the function. The only references to user_id in the whole file are the import on line 47 and this assignment on line 303 — grep confirms no other occurrences.

Why the assignment is dead

After line 303, the function does the following:

• Line 306: is_cli = context.request_context is None — checks the context directly, not user_id.
• Line 317: cli_oauth_flow(oauth_endpoints, self.toolset.name) — does not receive user_id.
• Line 319–322: _get_token_manager().store_token(oauth_config, token_data, context.request_context, ...) — passes context.request_context (the manager internally re-extracts user_id via its own helpers), not the local user_id.
• Lines 332–336: register_pending(tool_call_id=context.tool_call_id, code_verifier=code_verifier, oauth_config=oauth_config) — no user_id argument.
• Lines 338–348 and 350–353: the OAuth metadata dict and ApprovalRequirement return value contain no user_id.

The variable is therefore pure dead code. Python does not raise or warn on unused locals, and Ruff's F841 would catch it if enabled on this path.

Why existing code does not prevent it

There is no linter gate that flags unused locals in this file. The import of _get_user_id is valid (used at import time), so nothing in the codebase signals that the binding is orphaned.

Impact

Zero runtime impact — it's purely cosmetic. The logic is correct: the CLI vs. frontend branching uses context.request_context is None, which is the right check (DEFAULT_CLI_USER is the fallback user_id in CLI mode, so user_id would always be non-None and couldn't discriminate CLI from server anyway).

How to fix

Two reasonable options:

1. Delete the assignment. Smallest diff, removes dead code.
2. Use it for log traceability. The CLI-mode and OAuth-failure log lines in this function (OAuth MCP %s: CLI mode, running browser OAuth flow, OAuth MCP %s: CLI auth successful, OAuth MCP %s: CLI OAuth flow failed) would be more useful if they included user_id — this is likely the original intent of the binding.

Step-by-step proof

1. grep -n user_id holmes/plugins/toolsets/mcp/toolset_mcp.py returns exactly two lines: the import on line 47 and the assignment on line 303.
2. Inspect the function body from line 303 onwards (lines 303–353). Search for any expression that reads user_id. There are none — the next references to the identifier user_id in the file do not exist.
3. The function returns at one of three points (lines 300, 324, 350) without ever reading user_id.
4. Therefore removing line 303 produces no behavioural change.

[claude]

🟡 OAuthToolConnector.process_oauth_decision (lines 42-80) and its helper _try_exchange (lines 82-94) are defined but never called anywhere in the codebase. The live OAuth decision path at tool_calling_llm.py:313 uses the module-level helper _try_process_oauth_decision (line 159) which invokes _get_exchange_manager().complete_exchange() directly, bypassing this method entirely. The class docstring claims ToolCallingLLM delegates OAuth decisions to it without needing OAuth-specific imports or logic, but the actual delegation never happens — tool_calling_llm.py still imports _get_exchange_manager, parse_oauth_decision, and OAuthTokenExchangeError directly. Either wire _execute_tool_decisions to call self.tool_executor.oauth_connector.process_oauth_decision, or delete both methods.

Extended reasoning...

What the bug is\n\nOAuthToolConnector.process_oauth_decision at holmes/core/tools_utils/oauth_tool_connector.py:42-80 is defined as the "single entry point" for tool-approval OAuth decisions. Its docstring explicitly advertises this role (line 30-31: "ToolCallingLLM delegates OAuth decisions to it without needing OAuth-specific imports or logic"). But nothing ever calls it, and the logic it contains (parse → _try_exchange → load_tools_for_user) is exactly duplicated inline in tool_calling_llm._execute_tool_decisions at lines 311-315.\n\n## The specific code path that triggers it\n\nThe live OAuth decision flow in _execute_tool_decisions (tool_calling_llm.py:307-315):\n\npython\noauth_code = parse_oauth_decision(tool_decision.decision)\nuser_id = (request_context or {}).get("user_id")\nif oauth_code and user_id:\n oauth_success = _try_process_oauth_decision(tool_call.id, oauth_code, request_context)\n toolset = self.tool_executor._tool_to_toolset.get(tool_call.function.name) if oauth_success else None\n if oauth_success and toolset:\n self.tool_executor.oauth_connector.load_tools_for_user(user_id, toolset, request_context)\n\n\nwhere _try_process_oauth_decision (tool_calling_llm.py:159) is a module-level function — different from OAuthToolConnector.process_oauth_decision despite the similar name — that calls _get_exchange_manager().complete_exchange() directly. So the module-level helper handles the token exchange, and then the caller manually invokes oauth_connector.load_tools_for_user to do the second half of what process_oauth_decision would have done.\n\n## Why existing code does not prevent it\n\nprocess_oauth_decision has no call site in the codebase. _try_exchange is transitively dead — its only caller is process_oauth_decision at line 70. The class docstring contradicts reality: tool_calling_llm.py still carries OAuth-specific imports (_get_exchange_manager, parse_oauth_decision, OAuthTokenExchangeError) and OAuth-specific logic at lines 307-315, so the "no OAuth imports or logic" contract the connector advertises is violated by the very caller it was built to serve.\n\n## Impact\n\nZero runtime impact — this is pure dead code. The OAuth flow works correctly via the inline duplicate at lines 311-315. Severity is nit: no correctness or performance concern. The cost is purely for the reader: a future developer will look at OAuthToolConnector and reasonably assume its public process_oauth_decision method is the way to wire up new OAuth paths, then spend time discovering that nothing calls it and the inline path does the real work. The duplicated logic also creates drift risk — e.g. commit history already shows OAuth decision logic being edited in _execute_tool_decisions without corresponding changes to process_oauth_decision.\n\n## How to fix it\n\nTwo acceptable options:\n\nOption A (delete): Remove process_oauth_decision (42-80) and _try_exchange (82-94). Also strip the dangling docstring reference at line 109 that mentions this method as a caller of load_tools_for_user.\n\nOption B (wire up): Replace lines 311-315 in tool_calling_llm.py with a single call to self.tool_executor.oauth_connector.process_oauth_decision(tool_call.id, tool_decision.decision, request_context, toolset=self.tool_executor._tool_to_toolset.get(tool_call.function.name)). Then drop the now-unused _try_process_oauth_decision, _get_exchange_manager, parse_oauth_decision imports from tool_calling_llm.py. This honors the docstring's claim.\n\nOption A is smaller; Option B fulfills the stated design intent.\n\n## Step-by-step proof\n\n1. grep -n process_oauth_decision . across the repo returns exactly 3 hits:\n - holmes/core/tools_utils/oauth_tool_connector.py:42 — the definition\n - holmes/core/tools_utils/oauth_tool_connector.py:109 — a docstring inside load_tools_for_user that lists process_oauth_decision as a caller (stale — it is not a caller since nothing invokes it)\n - The other two matches in tool_calling_llm.py are for _try_process_oauth_decision, a differently-named module-level helper that happens to share the substring\n2. grep -n _try_exchange . returns 2 hits, both inside oauth_tool_connector.py (lines 70, 83) — the call from process_oauth_decision and the definition. Confirms _try_exchange is transitively dead.\n3. grep -n "oauth_connector.process_oauth_decision" . returns zero matches — no caller anywhere invokes the method via self.tool_executor.oauth_connector, executor.oauth_connector, or any other path.\n4. Reading _execute_tool_decisions at tool_calling_llm.py:307-315 confirms the inline duplicate: parse_oauth_decision → _try_process_oauth_decision → oauth_connector.load_tools_for_user, which mirrors the three steps inside OAuthToolConnector.process_oauth_decision (parse → _try_exchange → load_tools_for_user).\n\nThe method was clearly an intended refactoring target — it was added as the clean delegation point, but the inline call was never migrated to use it.