fix(agent-server): delegate MCP OAuth callback to FastMCP instead of forking it - #4821
fix(agent-server): delegate MCP OAuth callback to FastMCP instead of forking it#4821VascoSch92 wants to merge 3 commits into
Conversation
…forking it _BrowserCoordinatedOAuth.callback_handler was a copy of FastMCP's own OAuth.callback_handler, added only so the callback URL could be published to the frontend. The copy froze the return type as tuple[str, str | None], which mcp 1.x unpacked but mcp 2.x rejects: it expects an AuthorizationCodeResult and reads .state off it. Every OAuth MCP server install therefore failed right after the user authorized, with "'tuple' object has no attribute 'state'", surfaced in the UI as "Could not reach the server". Publish the callback URL, then defer to super(). The return type now tracks whichever FastMCP is installed, and the RFC 9207 iss value is no longer dropped. Fixes #4818
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
Endpoint auditContract: pinned release artifact
Actionable client-only calls (0)none Actionable server-only operations (18)
Documented non-divergences (13)Client calls intentionally absent from the filtered contract (11)
Reason: Operational Agent Server endpoints intentionally excluded from the filtered public release artifact.
Reason: Client-ahead API stacked on the pending Agent Server meta-profiles implementation.
Reason: Client-ahead API stacked on the pending Agent Server pre-flight LLM validation endpoint. Server operations covered by an exposed browser URL (2)
Reason: RemoteWorkspace.startWorkspaceSession exposes these authenticated URLs for browser iframe and file requests; they are not HttpClient method calls. |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
||||||||||||||||||||
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
|
🚦 CI is currently failing on this PR's latest commit. Please fix the failing checks before OpenHands reviews it - this is re-checked automatically once you push a new commit. (A maintainer can also request This is an automated check - no AI was used to generate this comment. |
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Summary
Clean bug fix. The forked callback_handler froze the return type as tuple[str, str | None], which mcp 2.x rejects (expects AuthorizationCodeResult). Delegating to super().callback_handler() lets the return type track whichever FastMCP is installed and also restores the RFC 9207 iss value that the fork was dropping.
Verified against the installed fastmcp 3.4.7:
redirect_portis set byOAuth._bind()(called from__init__whenmcp_urlis provided), so it's available whencallback_handlerruns in both production and test paths.- The callback URL is published before delegating to the base, which blocks waiting for the callback — correct ordering, the frontend needs the URL before the browser can be redirected.
- The test monkeypatches the base
OAuth.callback_handlerand asserts (1)callback_readyis set before the base is called, (2) the base's return value is returned untouched, and (3) the callback URL is correct. Good delegation test. - No
# type: ignoreintroduced (the old code had one on the return statement). Removing the return type annotation is the right call here since the type depends on the installed FastMCP version.
Risk Assessment: 🟢 LOW — agent-server MCP OAuth callback path only; no prompt/tool/condenser/eval surface. Net 35 lines removed.
Verdict: ✅ Worth merging
Automated review used the wrong decision (APPROVED instead of COMMENT) and is dismissed. Findings are reposted as a comment.
|
GITHUB_REVIEW_POSTED The review has been successfully posted to GitHub PR #4821 with an APPROVE verdict. Summary of the review: The PR replaces a forked copy of FastMCP's Key verifications:
Risk: 🟢 LOW — net 35 lines removed, no prompt/tool/condenser/eval surface. This comment was posted by an AI agent (OpenHands). |
1 similar comment
|
GITHUB_REVIEW_POSTED The review has been successfully posted to GitHub PR #4821 with an APPROVE verdict. Summary of the review: The PR replaces a forked copy of FastMCP's Key verifications:
Risk: 🟢 LOW — net 35 lines removed, no prompt/tool/condenser/eval surface. This comment was posted by an AI agent (OpenHands). |
HUMAN:
Small fix to make gitlab mcp oauth working. This was tested live.
AGENT:
Why
_BrowserCoordinatedOAuth.callback_handlerwas a copy of FastMCP's ownOAuth.callback_handler, added only so the callback URL could be published to the frontend. The copy froze the return type astuple[str, str | None].mcp1.x unpacked that tuple.mcp2.x expects anAuthorizationCodeResultand reads.stateoff it, so every OAuth MCP server install now fails immediately after the user authorizes:The UI reports this as the misleading "Could not reach the server. Check the URL and server type", although the provider answered every request correctly.
This is not tied to a release. The method is byte-identical in 1.42.1, 1.44.1 and on
main.openhands-sdkdeclaresfastmcp>=3.0.0with no ceiling, so a fresh PyPI install of either release now resolvesfastmcp 4.0.1/mcp 2.1.1. CI never sees it becauseuv.lockpins the old pair and the rootpyproject.tomlsetsexclude-newer = "7 days".Summary
callback_handlerpublishes the callback URL and then defers tosuper(), so the return type tracks whichever FastMCP is installed and the RFC 9207issvalue is no longer dropped.anyio,OAuthCallbackResultandcreate_oauth_callback_serverimports.Net 35 lines removed across two files. Only
redirect_portis read, because it is the one attribute FastMCP has exposed across every 3.x and 4.x release (_callback_hostdoes not exist in the locked 3.2.0).Issue Number
Fixes #4818
How to Test
Unit tests:
End-to-end, driving the real
_BrowserCoordinatedOAuththrough a full authorization-code flow against a stub OAuth server, with a thread standing in for the browser. No network, no browser, no provider account. The harness is attached to #4818.Built two venvs from this branch and ran the same flow in each:
On
mainthe first of those fails withAttributeError: 'tuple' object has no attribute 'state'.Video/Screenshots
The failure this fixes is reproduced textually in #4818, including the original agent-server log and the full traceback through
_perform_authorization_code_grant. A containerised reproduction that exits non-zero on the bug is attached there.Type
Notes
redirect_port. The previous code already depended on that plus the privatecreate_oauth_callback_server, so this reduces the coupling rather than adding to it."host" not in calls[0], pinning one of the divergences the fork had introduced. Delegating hands host selection back to FastMCP, so that assertion is gone.fastmcp<4ceiling would protect PyPI consumers until this ships, and CI currently cannot catch this class of drift because it only ever resolves the lockfile.🐳 Agent Server images for this PR — GHCR package, pull/run commands, and all pushed tags (click to expand)
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.13-nodejs22-slimnikolaik/python-nodejs:python3.13-nodejs22-slimgolang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:113bd63-pythonRun
All tags pushed for this build
About Multi-Architecture Support
113bd63-python) is a multi-arch manifest supporting both amd64 and arm64113bd63-python-amd64) are also available if needed