chore: [mirror of #9267] pass OAuthClientProvider to call_mcp_tool for automatic token refresh

[evan-onyx]

Automated mirror of PR #9267 so private CI can run.

• [Required] I have considered whether this PR needs to be cherry-picked to the latest beta branch.
• [Optional] Override Linear Check

[github-actions]

🖼️ Visual Regression Report

Project	Changed	Added	Removed	Unchanged	Report
admin	0	0	0	122	✅ No changes
exclusive	0	0	0	8	✅ No changes

[cubic-dev-ai]

No issues found across 3 files

Confidence score: 5/5

• Automated review surfaced no issues in the provided summaries.
• No files require special attention.

[greptile-apps]

Greptile Summary

This PR propagates user_id from construct_tools into MCPTool and uses it to build an OAuthClientProvider for MCPAuthenticationType.OAUTH servers on non-SSE transports, enabling the MCP SDK to automatically refresh expired tokens. It also adds "please reconnect to the server" as an auth-error indicator (matching the ValueError raised by redirect_handler when UNUSED_RETURN_PATH is used), and fixes the mock function signature in the test file to match call_mcp_tool's actual parameter names.

Key observations:

• The token-refresh flow integrates cleanly with the existing OnyxTokenStorage mechanism: set_tokens stores the Authorization header in connection_config, so has_auth_config remains True and the early-return guard is not incorrectly triggered.
• The SSE transport guard (auth_for_request = None for SSE in mcp_client.py) is correctly mirrored in mcp_tool.py with a warning log, keeping the two layers consistent.
• Deferred inline imports (make_oauth_provider, UNUSED_RETURN_PATH) inside run() are used to avoid a circular dependency; a brief inline comment explaining the cycle would aid future maintainers.
• The new OAUTH auth-provider branch (lines 205–232 of mcp_tool.py) has no dedicated test; only the pre-existing PT_OAUTH flow is covered.

Confidence Score: 4/5

• This PR is safe to merge; the token-refresh logic integrates correctly with existing storage and header-building mechanisms.
• The changes are small and well-scoped. The core token-refresh flow is consistent with how OnyxTokenStorage.set_tokens() already writes headers to connection_config, so the has_auth_config guard works correctly. The main gap is missing test coverage for the new OAUTH provider branch and a minor code-smell with deferred imports, neither of which block correctness.
• backend/onyx/tools/tool_implementations/mcp/mcp_tool.py — the new OAUTH auth-provider branch (lines 205–232) should be covered by tests before relying on it in production.

Important Files Changed

Filename	Overview
backend/onyx/tools/tool_implementations/mcp/mcp_tool.py	Adds user_id parameter and constructs an OAuthClientProvider for OAUTH-type MCP servers on non-SSE transports, enabling automatic token refresh; also adds "please reconnect to the server" as an auth-error indicator. Core logic is sound but the new OAUTH branch lacks test coverage.
backend/onyx/tools/tool_constructor.py	Passes user_id=str(user.id) to MCPTool; user is typed as User (non-nullable) at the call site, so this is safe. Minimal and correct change.
backend/tests/external_dependency_unit/tools/test_mcp_passthrough_oauth.py	Fixes mock signature: renames kwargs → arguments to match the real call_mcp_tool signature, and adds the new auth keyword parameter. No functional test logic changed; existing PT_OAUTH coverage is unaffected.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[MCPTool.run called] --> B[Build headers from connection_config]
    B --> C{has_auth_config?}
    C -- No --> D[Return auth-required error]
    C -- Yes --> E{auth_type == OAUTH?}
    E -- No --> F[call_mcp_tool with headers only]
    E -- Yes --> G{transport == SSE?}
    G -- Yes --> H[Log warning, skip auth provider]
    H --> F
    G -- No --> I[make_oauth_provider with user_id and connection_config]
    I --> J[call_mcp_tool with auth provider]
    J --> K{Token valid?}
    K -- Yes --> L[Return tool result]
    K -- Expired, refresh available --> M[SDK refreshes token, retries]
    M --> L
    K -- Re-auth needed --> N[ValueError caught as auth error]
    N --> O[Return reconnect message to user]

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: backend/onyx/tools/tool_implementations/mcp/mcp_tool.py
Line: 220-221

Comment:
**Deferred imports inside hot path**

`make_oauth_provider` and `UNUSED_RETURN_PATH` are imported inside `run()` on every OAUTH tool invocation. Python's module cache means this is safe (no repeated I/O), but deferred inline imports are a code smell that usually signals a circular-import problem. The comment in the code doesn't explain *why* the circular import exists; it only describes what the parameters are.

If the circular import can't be broken, at minimum a comment should explain the dependency cycle. If it *can* be broken (e.g., by moving `make_oauth_provider` to a lower-level module that `mcp_tool.py` doesn't transitively pull from), that would be preferred — the best-practices guide flags hidden import-time coupling as an issue to avoid.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: backend/onyx/tools/tool_implementations/mcp/mcp_tool.py
Line: 205-232

Comment:
**Missing test coverage for the new auth provider path**

The new OAuth-provider branch (lines 205–232) is the primary new behavior in this PR but has no dedicated test. The existing test class only covers the pass-through OAuth flow. The SSE warning path, token-refresh happy path, and the reconnect-error path are all untested.

Consider adding tests that verify:
- An OAuth-type server with a valid connection config causes the auth provider to be non-None
- The SSE transport path results in auth being skipped with a warning logged

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "fix test" | Re-trigger Greptile}

[greptile-apps]

Deferred imports inside hot path

make_oauth_provider and UNUSED_RETURN_PATH are imported inside run() on every OAUTH tool invocation. Python's module cache means this is safe (no repeated I/O), but deferred inline imports are a code smell that usually signals a circular-import problem. The comment in the code doesn't explain why the circular import exists; it only describes what the parameters are.

If the circular import can't be broken, at minimum a comment should explain the dependency cycle. If it can be broken (e.g., by moving make_oauth_provider to a lower-level module that mcp_tool.py doesn't transitively pull from), that would be preferred — the best-practices guide flags hidden import-time coupling as an issue to avoid.

Prompt To Fix With AI

This is a comment left during a code review.
Path: backend/onyx/tools/tool_implementations/mcp/mcp_tool.py
Line: 220-221

Comment:
**Deferred imports inside hot path**

`make_oauth_provider` and `UNUSED_RETURN_PATH` are imported inside `run()` on every OAUTH tool invocation. Python's module cache means this is safe (no repeated I/O), but deferred inline imports are a code smell that usually signals a circular-import problem. The comment in the code doesn't explain *why* the circular import exists; it only describes what the parameters are.

If the circular import can't be broken, at minimum a comment should explain the dependency cycle. If it *can* be broken (e.g., by moving `make_oauth_provider` to a lower-level module that `mcp_tool.py` doesn't transitively pull from), that would be preferred — the best-practices guide flags hidden import-time coupling as an issue to avoid.

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

Missing test coverage for the new auth provider path

The new OAuth-provider branch (lines 205–232) is the primary new behavior in this PR but has no dedicated test. The existing test class only covers the pass-through OAuth flow. The SSE warning path, token-refresh happy path, and the reconnect-error path are all untested.

Consider adding tests that verify:

• An OAuth-type server with a valid connection config causes the auth provider to be non-None
• The SSE transport path results in auth being skipped with a warning logged

Prompt To Fix With AI

This is a comment left during a code review.
Path: backend/onyx/tools/tool_implementations/mcp/mcp_tool.py
Line: 205-232

Comment:
**Missing test coverage for the new auth provider path**

The new OAuth-provider branch (lines 205–232) is the primary new behavior in this PR but has no dedicated test. The existing test class only covers the pass-through OAuth flow. The SSE warning path, token-refresh happy path, and the reconnect-error path are all untested.

Consider adding tests that verify:
- An OAuth-type server with a valid connection config causes the auth provider to be non-None
- The SSE transport path results in auth being skipped with a warning logged

How can I resolve this? If you propose a fix, please make it concise.