feat(openapitoolset): add OpenAPI specification toolset #451
Conversation
- Add AuthConfig, AuthCredential, AuthScheme types - Add AuthHandler for OAuth2 flow orchestration - Add CredentialManager and CredentialService for token management - Add OAuth2 token exchange helpers - Add unit tests for auth package
Summary of ChangesHello @cpunion, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the system's ability to interact with external APIs by introducing a dedicated toolset for OpenAPI specifications. It allows developers to define API interactions using standard OpenAPI documents, which are then automatically converted into executable tools. This streamlines the process of integrating new services and ensures consistent API consumption patterns, complete with flexible authentication mechanisms. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new toolset for generating tools from OpenAPI specifications. The implementation covers parsing the spec, creating RestApiTool instances, and handling various authentication schemes. The code is well-structured and includes a good set of unit tests. I've identified a few areas for improvement, including a correctness issue in parameter parsing, a missing feature in authentication handling, and a misleading function signature. Overall, this is a great addition.
| var params []Parameter | ||
|
|
||
| // Parse path-level parameters | ||
| if pathParams, ok := pathItem["parameters"].([]any); ok { | ||
| params = append(params, parseParameterList(pathParams)...) | ||
| } | ||
|
|
||
| // Parse operation-level parameters (override path-level) | ||
| if opParams, ok := op["parameters"].([]any); ok { | ||
| params = append(params, parseParameterList(opParams)...) | ||
| } |
There was a problem hiding this comment.
The current implementation appends both path-level and operation-level parameters. According to the OpenAPI specification, parameters defined at the operation level should override any parameters with the same name and location (in) defined at the path level. Simply appending them can lead to duplicate parameters and incorrect API requests.
To fix this, you can use a map to collect parameters, which will naturally handle the overriding, and then convert the map back to a slice.
paramMap := make(map[string]Parameter)
// Parse path-level parameters
if pathParams, ok := pathItem["parameters"].([]any); ok {
for _, p := range parseParameterList(pathParams) {
paramMap[p.Name+":"+p.In] = p
}
}
// Parse operation-level parameters (overrides path-level)
if opParams, ok := op["parameters"].([]any); ok {
for _, p := range parseParameterList(opParams) {
paramMap[p.Name+":"+p.In] = p
}
}
params := make([]Parameter, 0, len(paramMap))
for _, p := range paramMap {
params = append(params, p)
}| parsed, err := parseOperation(path, method, op, baseURL, pathItemMap) | ||
| if err != nil { | ||
| continue // Skip invalid operations | ||
| } |
There was a problem hiding this comment.
The parseOperation function is declared to return an error, but it never actually returns one. This makes the error handling here redundant and the function's signature misleading. Since the parsing logic is very lenient and designed to skip invalid parts rather than fail, it would be clearer to remove the error return type from parseOperation altogether.
This would also require changing the signature of parseOperation on line 121 and its return statement on line 156.
| parsed, err := parseOperation(path, method, op, baseURL, pathItemMap) | |
| if err != nil { | |
| continue // Skip invalid operations | |
| } | |
| parsed := parseOperation(path, method, op, baseURL, pathItemMap) |
| case auth.AuthCredentialTypeAPIKey: | ||
| if t.authCredential.APIKey != "" && t.authScheme != nil { | ||
| if apiKeyScheme, ok := t.authScheme.(*auth.APIKeyScheme); ok { | ||
| switch apiKeyScheme.In { | ||
| case auth.APIKeyInHeader: | ||
| headers[apiKeyScheme.Name] = t.authCredential.APIKey | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation for API key authentication only supports placing the key in the request header. The OpenAPI specification also allows API keys to be passed in query parameters (in: query). This functionality is missing.
To add support for this, you would need to:
- Update
buildURLto check if the auth scheme is an API key in a query parameter, and if so, add it to the query values. - Ensure this function,
getAuthHeaders, only handles thein: headercase for API keys.
cpunion
force-pushed
the
feature/openapi-toolset
branch
from
9f2e0c9 to
22ebdb7
Compare
Add support for tools to request authentication credentials during execution: - Add RequestedAuthConfigs field to EventActions for storing auth requests - Implement GenerateAuthEvent to convert auth requests to adk_request_credential function calls - Add RequestAuthConfig method to ToolContext for tools to request authentication - Add AuthConfig field to Tool interface for tools to declare auth requirements - Integrate auth event generation into the LLM base flow Also fix test comparisons to ignore RequestedAuthConfigs field initialization and add UTC timezone setup in database tests for consistent timestamp formatting.
cpunion
force-pushed
the
feature/openapi-toolset
branch
from
22ebdb7 to
4a3116a
Compare
cpunion
force-pushed
the
feature/openapi-toolset
branch
from
4a3116a to
d6d4100
Compare
Add a new toolset that automatically generates tools from OpenAPI/Swagger specifications, enabling agents to interact with REST APIs. Key components: - OpenAPI spec parser supporting v3.0/v3.1 with JSON Schema conversion - RestApiTool for executing HTTP requests with path/query/body parameters - Toolset for loading specs from file/URL and generating tools - OAuth2 authentication support with credential request flow - Example demonstrating GitHub API integration with device auth flow
cpunion
force-pushed
the
feature/openapi-toolset
branch
from
d6d4100 to
3d1bdc7
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-structured feature: an OpenAPI toolset with comprehensive authentication support, including OAuth2. The new auth package is robust, with a clear separation of concerns for schemes, credentials, and lifecycle management. The use of registries for exchangers and refreshers is a good design choice for extensibility. The OpenAPI parser and the RestApiTool provide a powerful way to generate tools from specifications. I've identified a critical issue in the example's custom OAuth2 code exchange logic that should be refactored to use the standard library for better security and simplicity. Additionally, there are a couple of medium-severity issues related to parsing and key generation that could be improved for robustness and clarity. Overall, this is an excellent addition to the ADK.
1 participant
Base on:
#455 feat(auth): add OAuth2/OIDC authentication package
#457 Feature/auth flow