Fix: honor the credentialKey override in the OpenAPI tool credential cache slot (stacked on #864) - #953
Open
AmaadMartin wants to merge 3 commits into
Conversation
added 3 commits
August 11, 2026 12:20
…e slot ToolAuthHandler passed credentialKey only to the auth request slot, so a developer who set it could not choose the exchanged-credential cache slot. The store now takes the override on its constructor, which is the single site both the read path and the write path derive their key from.
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.
Please ensure you have read the contribution guide before creating a pull request.
Stacked on #864 (
fix/openapi-credential-key-identity). Review and merge that one first; this PR targets its branch.Link to Issue or Description of Change
N/A
Problem: The
credentialKeyoption reaches only the auth request slot (temp:<key>), never the exchanged-credential cache slot.ToolContextCredentialStorederives the cache key from the scheme and credential digests alone, so a developer who setscredentialKeycannot choose the cache slot, share one credential across tools, or keep two tools apart. The adk-python docstrings promise the option covers "interactive auth and credential caching", so the option does half of what it says.Solution: The store now takes the override on its constructor and returns it verbatim ahead of the digest. The constructor is the single site both the read path and the write path derive their key from, so the two cannot disagree. An absent or empty key still derives the scheme+credential digest key from #864.
Notes for the reviewer:
_get_credential_key_override()is read only by_build_auth_config(), andToolContextCredentialStore.get_credential_key()never sees it. adk-js fixes it first; adk-python follows separately.OpenAPIToolsethands onecredentialKeyto every tool it builds, socredentialKeyon a toolset means one exchanged-credential slot for all of them. That is the documented intent, and it is opt-in: the derived key still isolates per tool by default.credentialKey. The cost is one extra exchange, after which it caches at the new slot. There is deliberately no fallback read of the old key: Fix: key the OpenAPI tool credential cache on scheme and credential identity #864 dropped exactly that ambiguous second lookup one commit earlier.tool_auth_handler.ts(Fix: request user consent for authorization-code OpenAPI tools #862, Fix: refresh an expired stored OAuth2 credential in ToolAuthHandler #772, Fix: accept an AuthScheme at the OpenAPI tool entry points #774) plus Test: build the tool_auth_handler suite from real Contexts #845 and Fix: assert the OpenAPIToolset global auth override through the public API (stacked on #337) #645. None reads the override in the store. Fix: key the OpenAPI tool credential cache on scheme and credential identity #864 is the base this stacks on.Testing Plan
CI does not run on this PR.
validation.yaml,license-check.ymlandcross-language-integration.ymlall trigger onpull_request: branches: [main], and this PR's base isfix/openapi-credential-key-identity. Onlyauto-assignran. Everything below was run locally on the pushed commit1b76b82.Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.
Four cases added at the end of
core/test/tools/openapi_tool/tool_auth_handler_test.ts. No existing test was edited.Coverage of
tool_auth_handler.tsover the two suites that exercise it is 100% of statements, branches, functions and lines.Mutation proof. I deleted the
if (this.credentialKeyOverride) { return ...; }early return and re-ran the file. Three of the four new tests failed:The fourth case pins the empty-string fall-through, which that mutation does not change. I proved it separately by widening the guard to
if (this.credentialKeyOverride !== undefined):Manual End-to-End (E2E) Tests:
I built one
OpenAPIToolsetwith two operations and a toolset-widecredentialKey, seededtemp:shared_toolset_tokens, and ran both tools against a real local HTTP server that echoes its headers.Before the fix, the same run ends with
["temp:shared_toolset_tokens","apiKey_ab718306e1907ce2__existing_exchanged_credential"].No new integration test is needed.
tests/integration/tools/openapi_credential_cache_test.tsfrom #864 already drives realOpenAPIToolsettools over HTTP through the slot this change moves, and it stays green.Pre-submit:
The five
{type: 'apiKey'}errors intool_auth_handler_test.tsare on lines 80, 97, 139, 167 and 179, all in tests that predate this branch. I confirmed the count is identical with my test hunk stashed.Checklist
[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.