Repository navigation
Conversation
Somay-kousis
approved these changes
Oct 22, 2026
Somay-kousis
left a comment
Contributor
There was a problem hiding this comment.
Ran this locally. No blockers found.
What I ran (PR head d1712a3, merge base 1daa0ec)
- New test
test_run_async_impl_empty_oauth_does_not_fall_through_to_http: passes on head. With onlymcp_tool.pyreverted to the merge base it fails withExpected: create_session(headers={'Authorization': 'Bearer provider-token'}),Actual: create_session(headers={'Authorization': 'Bearer stale-http-token'}). tests/unittests/tools/mcp_tool: 442 passed on head.- My own comparison of
McpTool._get_headersagainstbuild_auth_headersacross 23 credential shapes: OAuth2 in {absent,access_token=None,access_token="", token set} crossed with HTTP in {absent, bearer, empty bearer, basic, bearer plusadditional_headers, custom scheme}. Head: 0 mismatches. Merge base: 8 mismatches, all of them OAuth2 with a None or empty token plus any usable HTTP credential, where invocation sent the HTTP header and discovery sent none.
Checked, no issues
src/google/adk/tools/mcp_tool/mcp_tool.py:862-869now has the same branch shape assrc/google/adk/auth/_auth_headers.py(if credential.oauth2:then the token check inside).- Existing callers: the fall-through only came in with #7590 (dc78d0a). No tag in my clone contains that commit, so no released version relied on falling back to the HTTP field. Before #7590 this branch sent
Bearer Noneinstead. - With a valid OAuth2 token, OAuth2 still wins over any HTTP field, same as before.
Nit
- The test covers
access_token=None(tests/unittests/tools/mcp_tool/test_mcp_tool.py:1952) but notaccess_token="". Both go down the same path today (my comparison above covers it), so a parametrize over[None, ""]would just pin it.
Not checked
- End to end against a real MCP server or the session pool.
- The full
tests/unittests/toolsandtests/unittests/authsuites (the PR reports 3556 passed there).
McpTool._get_headers used `if credential.oauth2 and credential.oauth2.access_token`, so an OAuth2 credential with an empty token fell through to the `elif credential.http` branch and emitted an Authorization header from an unrelated HTTP field on the same object. The discovery helper build_auth_headers keeps the credential on the OAuth2 branch and emits no header for an empty token. Restore the nested structure here so both paths resolve the same credential to the same header and therefore the same pooled MCP session. Fixes google#7646
CorgiBoyG
force-pushed
the
fix/mcp-oauth-branch-consistency
branch
from
October 22, 2026 06:06
d1712a3 to
14f104b
Compare
Contributor
Author
|
Thanks for the thorough review and the independent 23-shape comparison, @Somay-kousis — much appreciated. I took the nit: the regression test is now parametrized over Re-verified on the updated commit:
The earlier triage check failure was an infra-side |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Link to Issue or Description of Change
Problem:
McpTool._get_headers()usesif credential.oauth2 and credential.oauth2.access_token:. When anAuthCredentialcarries an OAuth2 object with an empty/Noneaccess_tokenand an HTTP credential on the same object, that condition is falsy, so control falls through toelif credential.http:and emits anAuthorizationheader from the HTTP field.The canonical discovery helper
build_auth_headers()(used byMcpToolset) keeps the credential on the OAuth2 branch and emits no header for an empty token. The two paths therefore resolve the same credential to differentAuthorizationheaders. Both feed_merge_auth_and_provider_headers, and the MCP session pool key is derived from the merged headers, so discovery and invocation can bind to different pooled sessions — the path-consistency concern behind #7548 / #7590.This mixed shape is reachable: a custom auth provider may return such a credential (the non-consent path in
CredentialManagerreturns it without clearing fields). It is a narrow edge, not the default OAuth2/HTTP flow.Solution:
Restore the nested structure so an OAuth2 credential stays on its own branch and an empty token simply emits no header, mirroring
build_auth_headers():The change is limited to the OAuth2 guard plus one regression test for the mixed-credential case.
Testing Plan
Unit Tests:
The new test fails on current main with:
After the fix:
I also compared the invocation path (
McpTool._get_headers) against the discovery path (build_auth_headers) across 15 OAuth2/HTTP credential combinations: 0 mismatches after the fix (4 before).Static checks:
The full repository test suite was not run.
Manual End-to-End (E2E) Tests:
Not run. The regression test asserts the final headers passed to
create_session().Checklist