Fix: accept an AuthScheme at the OpenAPI tool entry points - #774
Open
AmaadMartin wants to merge 3 commits into
Open
Fix: accept an AuthScheme at the OpenAPI tool entry points#774AmaadMartin wants to merge 3 commits into
AmaadMartin wants to merge 3 commits into
Conversation
added 3 commits
August 7, 2026 06:13
The credential machinery under the OpenAPI tool layer is typed against AuthScheme, which adds OpenIdConnectWithConfig to SecuritySchemeObject. The tool layer still declared the narrower SecuritySchemeObject, so a caller could not pass an OIDC-with-config literal that the exchangers read at runtime. Widen the ten input positions to AuthScheme. Output positions keep SecuritySchemeObject so callers lose no precision.
Each case passes an inline OpenID Connect literal that carries endpoint config. An inline literal is what the excess-property check rejects, so reverting a signature makes tsc report TS2353 on these lines. A value first assigned to a typed const compiles either way and proves nothing. The cases observe the scheme through the public auth path -- the ToolAuthHandler.fromToolContext call and the configureAuthScheme override -- rather than reading the private authScheme field. The tool_auth_handler case also pins the credential cache key, which must stay openIdConnect_existing_exchanged_credential.
An in-body mockRestore() never runs when an assertion above it throws, which is the case that matters. The spy then leaks into every later test in the file. Move the restore into afterEach, matching the hook rest_api_tool_test.ts already uses.
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.
Link to Issue or Description of Change
Link to an existing issue (if applicable):
N/A
Or, if no issue exists, describe the change:
Problem: The credential machinery under the OpenAPI tool layer is typed against
AuthScheme, which isOpenAPIV3.SecuritySchemeObject | OpenIdConnectWithConfig. The OpenAPI tool entry points still declared the narrowerSecuritySchemeObject.getTokenEndpointanddetermineGrantTyperead theOpenIdConnectWithConfigendpoint fields at runtime, so the feature works, but a caller cannot pass the scheme inline.new OpenAPIToolset({specStr, authScheme: {type: 'openIdConnect', authorizationEndpoint: ..., tokenEndpoint: ...}})fails to compile withTS2353.Solution: I widened the ten positions where a caller supplies a scheme to
AuthScheme. This matchesadk-python, where each of these is alreadyOptional[AuthScheme]. Every change is a parameter or field widening, so no existing call site breaks and no runtime behaviour changes. The diff is types and imports only.Three sites keep
SecuritySchemeObjecton purpose, because they are output positions where widening costs callers precision and unlocks no new call:openapi_spec_parser.ts:228— thespec.components?.securitySchemescast. These values come from anOpenAPIV3.Document, which cannot express anOpenIdConnectWithConfig.auth_helpers.ts:64,75— thecreateApiKeySchemeandcreateBearerSchemereturn types. Their results stay assignable to every widened parameter by subtyping.Out of scope, and deliberately not done:
adk-python'sdict_to_auth_schemenormalisation, anyCustomAuthScheme, and any change to how the credential cache key is derived.Collision check: I listed all 400 open and closed pull requests on the fork and read the file lists of the nine that touch
openapi_toolor auth. None changes an auth-scheme type, so I branched frommainrather than stacking. The nearest neighbours are #772 (tool_auth_handler.ts), #605 (openapi_toolset.ts), #750, #748, #742 and #645 (the same test files). Any conflict with them is textual.Note on #645: that pull request replaces the private
authSchemefield reads inopenapi_toolset_test.tswith public-API assertions. My new cases therefore observe the scheme through the public path — theToolAuthHandler.fromToolContextcall and theconfigureAuthSchemeoverride — instead of adding more private field reads.Testing Plan
Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.
Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.
I added 7 cases across 5 files. I added new
it(...)blocks and edited no existing test.Read this before the rest:
tscis the only guard, and CI does not run it.This change is types-only. TypeScript erases types, so no runtime test can tell the code before this change from the code after it. All 7 new cases pass on
mainunchanged. The regression guard isnpm run ts:checkand nothing else.That guard is not enforced by CI today. I checked:
.github/workflows/validation.yamlrunsbuild,test:coverage,lint,format:checkanddocs:check. It does not runts:check.core/tsconfig.jsonsets"include": ["src/**/*"], sonpm run buildnever typecheckscore/test.vitest.config.tssets notypecheckoption.typedoc.jsonexcludes**/*_test.ts, sodocs:checkdoes not see these files either.So if someone narrows one of these signatures again, CI stays green. A reviewer should treat the
ts:checkoutput below as the evidence, not the green checkmarks on this PR.I did not wire
ts:checkinto CI here. It is not green today: it reports 291 pre-existing errors acrosscore/testandtests/, and several open pull requests are already working through them. Turning it on is a separate job, much larger than this change.Every new case uses an inline object literal at the call site. This matters.
OpenIdConnectWithConfig extends OpenAPIV3.OpenIdSecurityScheme, so a value already typed as either one is structurally assignable toSecuritySchemeObjectand compiles against the old narrow signature too. Only a fresh object literal trips the excess-property check. A test that assigns to a typedconstfirst would pass before and after this change and would prove nothing.Proof the compile-time signal works. I reverted the five signatures to
OpenAPIV3.SecuritySchemeObject, rebuilt, and rannpm run ts:check. Each of the five files reports the error, 8 in total:Proof the new cases are not vacuous. These mutations do not distinguish this PR's before and after — nothing can, as explained above. They show the assertions pin real behaviour rather than passing regardless. I mutated the exact line each case pins and confirmed a failure:
getCredentialKeyreturns a hard-coded'default'expected undefined to be 'exchanged-token'fromToolContextpassesundefinedforauthSchemeconfigureAuthSchemebecomes a no-opcreateRestApiTooldropsparsed.authSchemeOpenAPIToolsetdrops theconfigureAuthSchemeoverrideapplyCredentialwrites a different default headerParsedOperation.authSchemegets no new test. It is an output position, so no call site can demonstrate it. Its proof is that the existing assertions on it still compile and pass unmodified.Coverage. Type annotations are erased at compile time, so this change adds no executable lines. Coverage does not regress.
No suppressions. I added no
@ts-expect-error,@ts-ignore,eslint-disable,as anyoras never.npm run ts:checkreports 291 errors on this branch and 291 onmain, and the per-file counts are identical, so this change introduces none.Manual End-to-End (E2E) Tests:
To reproduce the original failure, check out
mainand compile this:npm run ts:checkrejects it onmainand accepts it on this branch.core/test/tools/openapi_tool/openapi_toolset_integration_test.tsruns that exact flow through the public@google/adkentry point, building the toolset from a spec string and asserting the override reaches every tool.Commands run on the pushed commit:
tests/e2e/tools/rest_api_tool_auth_e2e_test.tsis unmodified and is the regression check that behaviour did not change. It does not run in this environment: it needs a real model. It fails withAPI key must be provided via constructor or GOOGLE_GENAI_API_KEY or GEMINI_API_KEY environment variableon this branch and identically on the base commit9360bf24, so the failure is environmental and unrelated.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.