feat(providers): route openai and gemini protocols to sdk providers#925
feat(providers): route openai and gemini protocols to sdk providers#925mingmxren wants to merge 2 commits intosipeed:mainfrom
Conversation
nikolasdehor
left a comment
There was a problem hiding this comment.
Review: feat(providers): route openai and gemini protocols to sdk providers
Large PR (2385+/28-) with well-documented design docs. I focused my review on the factory routing, provider implementations, and critical paths.
Architecture
-
Clean separation -- The hybrid approach (SDK for OpenAI/Gemini, keep
openai_compatfor other compatible providers, keepantigravityuntouched) is the right strategy. It minimizes blast radius while improving protocol correctness. -
Design docs are thorough -- The three plan documents cover design rationale, implementation steps, and compatibility considerations well. Good to see explicit thought-signature compatibility analysis for Gemini.
Factory Routing (reviewed)
-
Factory changes look correct -- The
case "openai"routing toopenai_sdk.NewProviderfor API-key path, andcase "gemini", "google"routing togemini_sdk.NewProvider, match the design. Factory tests confirm the routing. -
Question: what happens with
openaiprotocol behind a proxy/gateway? Some users run OpenAI-compatible endpoints (like LiteLLM, vLLM) withprotocol: "openai". With this change, those would be routed to the SDK provider instead ofopenai_compat. Is that intentional? The SDK provider may not handle all the dialect variations thatopenai_compatsupports. If this is an issue, consider adding a config flag likeforce_compat: trueor routing based on whetherapiBasediffers fromapi.openai.com.
OpenAI SDK Provider
-
Message mapping -- The role mapping (system/user/assistant/tool) and tool-call parsing look correct from what I can see.
-
maxTokensFieldhandling -- Good that you support bothmax_tokensandmax_completion_tokensto handle API version differences.
Gemini SDK Provider
-
Thought-signature dual-write -- Writing to both
ExtraContent.Google.ThoughtSignatureandFunction.ThoughtSignaturefor backward compatibility is the right approach. The read-path priority (extra_content first, then function fallback) ensures old sessions remain usable. -
Session serialization compatibility -- The compatibility strategy (new writes both fields, old readers use function field) is sound.
Concerns
-
Test coverage -- 306 lines of OpenAI SDK tests and 404 lines of Gemini SDK tests is good for a new provider. However, I would want to see integration-level tests that verify the full factory -> provider -> response path with mock servers.
-
Dependency footprint -- Adding both
openai-go/v3andgoogle.golang.org/genaiincreases the dependency tree significantly (13 new go.mod entries, 71 new go.sum entries). This is the cost of using official SDKs, but worth noting for binary size and supply chain considerations. -
openai_compatcleanup -- The PR description says it removes "Gemini-specific host/prefix branching fromopenai_compat", but the diff shows only 18 deletions inopenai_compat/provider.go. Make sure the thought-signature response parsing inopenai_compatis preserved for non-Gemini OpenAI-compatible gateways that might relay Gemini responses.
Minor
- The PR is marked as both "Fully AI-generated (100%)" and "Mostly AI-generated (AI draft, Human verified)". These are mutually exclusive -- please clarify.
Overall a solid architectural refactor. The main risk is point #4 (proxy/gateway users with protocol: "openai" getting unexpected behavior changes).
|
@yinwm Would you like to have a review on this PR. It is an optimization mentioned in this comment which generally won't affect existing functionality. |
📝 Description
This PR restructures provider routing to use official SDKs where protocol-specific behavior is required, while keeping the external
Provider.Chatcontract stable.Main changes:
openai/*(API-key path) to a newopenai_sdkprovider based ongithub.zerozr99.workers.dev/openai/openai-go/v3.gemini/*andgoogle/*to a newgemini_sdkprovider based ongoogle.golang.org/genai.antigravity/*unchanged.openai_compatHTTP adapter.openai_compatrequest shaping, reducing API base coupling.Also included:
🗣️ Type of Change
🤖 AI Code Generation
🔗 Related Issue
N/A
📚 Technical Context (Skip for Docs)
openai_compatwas brittle behind API proxies/gateways.🧪 Test Environment
📸 Evidence (Optional)
Click to view Logs/Screenshots
☑️ Checklist