feat(go-adk): enhance model config and provider support#1650
feat(go-adk): enhance model config and provider support#1650EItanya merged 12 commits intokagent-dev:mainfrom
Conversation
acbc7c3 to
30d8e60
Compare
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR improves parity between Python and Go runtimes by expanding LLM/embedding provider support (Ollama, Bedrock, Gemini/Vertex), adding shared transport features (TLS + API key passthrough + headers), and reducing unnecessary tokens by stripping synthetic approval tool messages before model calls.
Changes:
- Introduces provider-specific clients for Ollama and Bedrock (Go) and refactors Python embeddings into a standalone client.
- Adds shared HTTP transport configuration in Go (TLS, headers, API key passthrough) and wires passthrough token extraction from A2A requests.
- Sanitizes Bedrock tool IDs and refactors tool schema/response conversions to be more robust across providers.
Reviewed changes
Copilot reviewed 28 out of 29 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| python/packages/kagent-adk/tests/unittests/test_embedding.py | Updates unit tests to target the new standalone embedding client and adds normalization tests. |
| python/packages/kagent-adk/src/kagent/adk/types.py | Adjusts OpenAI/Azure import paths to model-specific modules. |
| python/packages/kagent-adk/src/kagent/adk/models/_embedding.py | Adds standalone KAgentEmbedding client with multi-provider support (OpenAI, Azure, Ollama, Gemini/Vertex, Bedrock). |
| python/packages/kagent-adk/src/kagent/adk/models/_bedrock.py | Adds Bedrock toolUseId sanitization for Bedrock Converse message conversion. |
| python/packages/kagent-adk/src/kagent/adk/models/init.py | Exports KAgentEmbedding from models package. |
| python/packages/kagent-adk/src/kagent/adk/_memory_service.py | Refactors memory service to use KAgentEmbedding instead of embedding logic inline. |
| go/go.sum | Updates/extends Go dependencies (AWS Bedrock runtime SDK, Ollama SDK, x/exp, etc.). |
| go/go.mod | Adds/updates required dependencies for Bedrock runtime + Ollama SDK and shared utilities. |
| go/adk/pkg/session/session_test.go | Removes tests for confirmation role normalization (function removed). |
| go/adk/pkg/session/session.go | Removes confirmation role normalization from session persistence/retrieval. |
| go/adk/pkg/models/tls_test.go | Adds tests for TLS transport + HTTP client behavior (CA file, skip verify, headers, passthrough auth). |
| go/adk/pkg/models/tls.go | Adds TLS transport builder supporting custom CA and skip-verify behavior. |
| go/adk/pkg/models/openai_adk_test.go | Renames function response extraction helper referenced by tests. |
| go/adk/pkg/models/openai_adk.go | Uses shared helpers for function response extraction and schema conversion. |
| go/adk/pkg/models/openai.go | Refactors OpenAI/Azure model constructors to use shared TransportConfig and passthrough flow. |
| go/adk/pkg/models/ollama_test.go | Adds tests for Ollama option conversion and config defaults. |
| go/adk/pkg/models/ollama_adk.go | Adds native Ollama SDK model implementation incl. tool conversion and streaming/non-streaming support. |
| go/adk/pkg/models/ollama.go | Adds Ollama model constructor using native SDK and shared HTTP transport. |
| go/adk/pkg/models/bedrock_test.go | Adds tests for Bedrock conversion utilities, stop reasons, schema conversion, tool ID sanitization, streaming args parsing. |
| go/adk/pkg/models/bedrock.go | Adds Bedrock Converse API model implementation and conversion utilities. |
| go/adk/pkg/models/base.go | Introduces shared TransportConfig, BuildHTTPClient, passthrough/header transports, and schema/response helpers. |
| go/adk/pkg/models/anthropic_adk.go | Switches to shared function response/schema helpers. |
| go/adk/pkg/models/anthropic.go | Refactors Anthropic constructors to use shared TransportConfig + passthrough. |
| go/adk/pkg/embedding/embedding.go | Expands Go embedding client to support Ollama/Gemini/Bedrock and OpenAI fallback. |
| go/adk/pkg/agent/testdata/config_ollama.json | Updates Ollama test config to use OpenAI-compatible endpoint for mock testing. |
| go/adk/pkg/agent/createllm_test.go | Updates Ollama test to match OpenAI-compatible mock behavior. |
| go/adk/pkg/agent/approval.go | Adds a before-model callback to strip synthetic approval tool parts from model requests. |
| go/adk/pkg/agent/agent.go | Wires before-model callback, moves models to shared transport config, and switches Bedrock to Converse API model. |
| go/adk/pkg/a2a/executor.go | Extracts bearer token from inbound A2A request metadata and injects into context for passthrough. |
Comments suppressed due to low confidence (1)
go/adk/pkg/models/tls.go:1
- The non-*http.Transport branch does not apply tlsConfig at all (RoundTrip ignores tlsConfig), so callers passing a custom RoundTripper silently get no TLS behavior changes. A tangible fix is to return an error when base is not *http.Transport (so behavior is explicit), or redesign the API to accept/construct an *http.Transport only. If you want to keep supporting custom RoundTrippers, the wrapper needs to actually enforce TLS (which generally requires owning the dial/TLS handshake layer).
package models
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
EItanya
left a comment
There was a problem hiding this comment.
A couple of structural things about the clients, but mostly looks good!
There was a problem hiding this comment.
Why are these subfields on a Client and not their own structs?
There was a problem hiding this comment.
Agreed, I've split them to providers
| sessionID := reqCtx.ContextID | ||
|
|
||
| // Extract Bearer token from incoming request for API key passthrough | ||
| if callCtx, ok := a2asrv.CallContextFrom(ctx); ok { |
There was a problem hiding this comment.
Not sure if this would be better, but couldn't we just turn this into a function and reuse that for passthrough rather than adding a new context variable?
There was a problem hiding this comment.
We could, but I think context variable keeps models decoupled from the a2asrv package (A2A server), similar to how Python uses session state to pass the key rather than having models read the A2A request directly.
An alternative works as well though, if this is what you're suggesting?
func openAIPassthroughOpts(ctx context.Context, m *OpenAIModel) []option.RequestOption {
// Read key from call context directly, avoid context var, we can make this a function
callCtx, ok := a2asrv.CallContextFrom(ctx)
if !ok { return nil }
vals, _ := callCtx.RequestMeta().Get("authorization")
// parse Bearer token...
} | func (t *tlsTransport) RoundTrip(req *http.Request) (*http.Response, error) { | ||
| // If the request's TLS config needs to be modified, we would need to | ||
| // create a new client for each request, which is inefficient. | ||
| // Instead, we rely on the base transport having the TLS config set. | ||
| // This wrapper is primarily for when base is not an *http.Transport. | ||
| return t.base.RoundTrip(req) | ||
| } |
There was a problem hiding this comment.
I don't quite understand this function
There was a problem hiding this comment.
Removed it, I don't think it's necessary
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class KAgentEmbedding: |
There was a problem hiding this comment.
Same comment about packing clients vs creating some sort of interface or using multiple structs
There was a problem hiding this comment.
For python I don't think it's too necessary, since each provider interface would just be a single method to generate embeddings so practically there would be no difference (this is because we don't need client caching for Python since asyncio is single threaded)
169a5bc to
0de2b54
Compare
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
… support ollama Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
5a5de6c to
6a2674f
Compare
) ## Motivation This will allow most users to directly switch from `runtime: python` to `runtime: go` without needing to worry about existing LLM provider configs since everything will be supported on the Go side, facilitating adoption of the new go runtime ## Summary Closes most of the gap between python and go identified in kagent-dev#1643 - TLS and api key passthrough for LLM provider - Support Ollama and Bedrock using client SDK as we've done earlier in kagent-dev#1540 - Use Bedrock client instead of messages API for Anthropic on Bedrock to support all bedrock runtime models - Tightens tool config conversion for Anthropic + Bedrock and fixes issues like kagent-dev#1645, kagent-dev#1683 - Sanitize ToolName for bedrock LLMs kagent-dev#1473, see [bedrock API docs](https://docs.aws.amazon.com/bedrock/latest/APIReference/API_runtime_ToolSpecification.html) - Refactor embedding models in python to be separate from memory service - Strip approval confirmation synthetic tool calls from LLM requests, these messages are persisted in task / events store and used by ADK internally but sending them to the model will be wasting tokens and confuse the model. If the session is long and has many HITL events, these internal tool messages will be consuming many unnecessary token! ## Testing Plan - [x] All new unit tests in go adk passes, all old unit tests in python passes - [x] Test for no regression with OpenAI and Gemini models in go runtime - [x] Validate with a wide range of use cases such as: builtin tools (ask user, save memory), ADK built-in tools (load memory) MCP tools, Remote A2A (subagent) tools, HITL tools (approvals) - [x] Bedrock LLM and embedding model in Go runtime - [x] OpenAI API key passthrough with A2A `--token` option - [x] Ollama LLM and embedding in Go runtime (local models, Gemma 4 + embedding Gemma) - [x] Ollama with TLS (local https server with self-signed certs) --------- Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Motivation
This will allow most users to directly switch from
runtime: pythontoruntime: gowithout needing to worry about existing LLM provider configs since everything will be supported on the Go side, facilitating adoption of the new go runtimeSummary
Closes most of the gap between python and go identified in #1643
Testing Plan
--tokenoption