Feat/google genai sdk as common provider across Google AI Studio and Google Cloud#65023
Feat/google genai sdk as common provider across Google AI Studio and Google Cloud#65023zeroasterisk wants to merge 8 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c0b825675
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Greptile SummaryThis PR integrates the
Confidence Score: 3/5Safe to merge for API-key users; the Vertex AI config path (vertexai.project/location in openclaw.json) is broken — the error message guides users to set it but the stream function ignores it. One P1 finding: the error message in google-genai-stream.ts references a config option (vertexai.project) that the code never reads, making the Vertex AI config path dysfunctional for users who follow the documented advice. Remaining findings are P2 style/cleanup. src/agents/google-genai-stream.ts — the vertexai config block needs to be plumbed through to this function or the error message must be corrected to env-only guidance. Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/agents/google-genai-stream.ts
Line: 43
Comment:
**`vertexai` config fields are silently ignored**
The error message tells users to set `models.providers.google-genai.vertexai.project` in `openclaw.json`, but the stream function never reads from that path — it only reads `GOOGLE_CLOUD_PROJECT` and `GOOGLE_CLOUD_LOCATION` from `env`. The `vertexai` block is defined in the Zod schema and `ModelProviderConfig` type but is never injected into this function, so any user who follows this advice will get no effect.
Either pass the resolved `vertexai` config down to this function (e.g., add `vertexai?: { project?: string; location?: string }` to the `model` parameter) and read from it with `env` fallback, or remove the misleading config reference from the error message.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/agents/google-genai-stream.ts
Line: 126
Comment:
**Use subsystem logger instead of `console.error`**
Other stream/transport handlers in this codebase use `createSubsystemLogger` for error reporting. Raw `console.error` bypasses structured logging, log-level filtering, and subsystem tagging.
(Import `createSubsystemLogger` from `"../logging/subsystem.js"` and replace the `console.error` call.)
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/agents/google-genai.live.test.ts
Line: 1-2
Comment:
**Global side-effect at module load time has no cleanup**
`(global as any).window = globalThis` runs unconditionally when this file is imported — even when `GEMINI_LIVE_TEST` is unset and every `describeLive` block is skipped. The CLAUDE.md guidelines require tests to clean up globals so `--isolate=false` stays green. Scope this inside a `beforeAll`/`afterAll` pair and restore the original value with `delete (global as any).window` in `afterAll`.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "docs: Add 404 troubleshooting to auth gu..." | Re-trigger Greptile |
|
Tip Next Step: Migration Path for Legacy Provider I have also prepared a follow-up branch feat/google-genai-migration (on my fork) that implements a runtime fallback. If a user still has This ensures zero breakage for existing users while guiding them to the new unified SDK. Let me know if you'd like me to open a follow-up PR with those changes after this one lands! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7f00e4e23
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const responseStream = await ai.models.generateContentStream({ | ||
| model: model.id, | ||
| contents: contents, | ||
| config: context.systemPrompt ? { systemInstruction: context.systemPrompt } : undefined, |
There was a problem hiding this comment.
Pass tool definitions to Google GenAI requests
In createGoogleGenAiStreamFnForModel, the generateContentStream payload is built from context.messages but never includes context.tools, so models on this provider cannot emit function calls for agent tools. This breaks tool-dependent runs (e.g., web/file/tool workflows) because the model is never told which functions are available, unlike the existing Google transport path that serializes tool declarations.
Useful? React with 👍 / 👎.
| if (authOverride === "google-genai-sdk") { | ||
| return resolveGoogleGenAiSdkAuthInfo(); |
There was a problem hiding this comment.
Guard new keyless auth mode in all runtime callers
resolveApiKeyForProvider now returns google-genai-sdk with no apiKey, but only the embedded-runner auth checks were updated to allow that. I checked src/agents/btw.ts and it still only exempts aws-sdk before calling requireApiKey, so /btw runs using models.providers.<id>.auth: "google-genai-sdk" will fail immediately with a missing API key error.
Useful? React with 👍 / 👎.
095138b to
b13b826
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b13b8264b1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const responseStream = await ai.models.generateContentStream({ | ||
| model: model.id, | ||
| contents: contents, | ||
| config: context.systemPrompt ? { systemInstruction: context.systemPrompt } : undefined, | ||
| }); |
There was a problem hiding this comment.
Forward generation controls to Google GenAI stream
createGoogleGenAiStreamFnForModel builds generateContentStream params with only systemInstruction and omits SimpleStreamOptions controls like maxTokens, temperature, and request cancellation. As a result, callers that set token limits or sampling settings get provider-default behavior instead, which can produce overlong/unexpected responses and higher token spend compared with other transports using the same run options.
Useful? React with 👍 / 👎.
| for await (const chunk of responseStream) { | ||
| const text = chunk.text; | ||
| if (text) { |
There was a problem hiding this comment.
Derive stop reason from Google stream finish metadata
The streaming loop consumes only chunk.text and never maps the model's finish reason, so output.stopReason remains the default "stop" for all non-error completions. When the provider ends on max-token or safety-related finish conditions, downstream logic will misclassify the result as a normal stop and may skip expected retry/handling paths.
Useful? React with 👍 / 👎.
|
Codex review: needs real behavior proof before merge. Workflow note: Future ClawSweeper reviews update this same comment in place. How this review workflow works
Summary Reproducibility: yes. for the review findings: source inspection of the PR head and current main shows stale imports, duplicate core provider routing, a direct SDK stream path, dropped request controls, and partial keyless auth plumbing. No high-confidence live reproduction exists for the claimed fix because no inspectable proof artifact is attached. PR rating Rank-up moves:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. Real behavior proof Risk before merge
Maintainer options:
Next step before merge Security Review findings
Review detailsBest possible solution: Keep Google AI Studio and Vertex behavior in the plugin-owned guarded Google transport unless maintainers explicitly approve a new provider surface, then require the chosen path to preserve auth, config, tools/options, cancellation, diagnostics, and redacted live proof. Do we have a high-confidence way to reproduce the issue? Yes for the review findings: source inspection of the PR head and current main shows stale imports, duplicate core provider routing, a direct SDK stream path, dropped request controls, and partial keyless auth plumbing. No high-confidence live reproduction exists for the claimed fix because no inspectable proof artifact is attached. Is this the best way to solve the issue? No. A parallel core direct-SDK provider is not the narrowest maintainable solution while current main keeps Google behavior in the bundled Google plugin's guarded transport path and newer Vertex PRs are already scoped to native ADC improvements. Label justifications:
Full review comments:
Overall correctness: patch is incorrect Security concerns:
What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against a13468320c63. |
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
|
I think mine is legacy and now deprecated by:
Thanks yall |
Summary
This PR integrates the new unified
@google/genaiSDK into OpenClaw, enabling support for both Google AI Studio (Gemini API) and Vertex AI (Google Cloud) via a single provider (google-genai).API_KEYand Google Cloud Vertex AI are confusing to users, and hard to configure - non-API_KEY auth needs to be standardized in several spots in the codebase.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Regression Test Plan (if applicable)
For bug fixes or regressions, name the smallest reliable test coverage that should catch this. Otherwise write
N/A.src/agents/google-genai.live.test.tsUser-visible / Behavior Changes
List user-visible changes (including defaults/config).
If none, write
None.google-genaithat can be used inopenclaw.json.google-genai-sdkauthentication mode."global"to better support modern model aliases (likegemini-flash-latest).Diagram (if applicable)
For UI changes or non-trivial logic flows, include a small ASCII diagram reviewers can scan quickly. Otherwise write
N/A.Security Impact (required)
No)No)Yes)No)No)Yes, explain risk + mitigation:Repro + Verification
Environment
gemini-flash-latestviagoogle-genaiGEMINI_LIVE_TEST=1,GEMINI_API_KEYorGOOGLE_APPLICATION_CREDENTIALS.Steps
pnpm exec vitest run src/agents/google-genai.live.test.tsExpected
Actual
gaxiosbug in Node ESM.Evidence
Attach at least one:
gaxiosdynamic import error before the polyfill fix).Human Verification (required)
What you personally verified (not just CI), and how:
globaland successfully resolves thegemini-flash-latestalias on Vertex AI.gcloud auth application-default logindirectly due to environment constraints on the test machine, but verified the identical ADC resolution path via a service account file.Review Conversations
Compatibility / Migration
Yes)Yes- adds new config options for the new provider).No)Risks and Mitigations
List only real risks for this PR. Add/remove entries as needed. If none, write
None.gaxioslibrary has a bug in Node ESM environments where it fails to dynamically importnode-fetchwhen checking forwindow.fetch.docs/google-genai-auth.mdexplaining the workaround (global.window = globalThis) and applied it in the test file.