fix(gemini): externalize @google/genai for Electron and add relay HTT…#219
fix(gemini): externalize @google/genai for Electron and add relay HTT…#219nxsre wants to merge 3 commits into
Conversation
…P probes Bundle @google/genai outside the pi-ai chunk so models.get works in diagnostics and runtime. Add HTTP fallbacks for one-api Gemini relays and ship genai deps in the installer.
There was a problem hiding this comment.
Review mode: initial
Findings
-
[MAJOR]
isGeminiSdkProbeUnavailableErrorregex is too broad:/reading ['"]get['"]|reading get/imay match unrelated error messages (e.g.,Something went wrong while reading getter). This could cause false positives where non-SDK-init errors trigger the HTTP fallback unnecessarily, potentially masking real network/auth issues. Narrow the pattern to require'models.get'or'client.models.get'context, or match the exact error string from the SDK (Cannot read properties of undefined (reading 'get')).
src/main/config/gemini-relay-probe.ts:10
Suggested fix:export function isGeminiSdkProbeUnavailableError(message: string): boolean { return /models\.get|client\.models\.get|cannot read properties of undefined \(reading 'get'\)/i.test(message); }
-
[MAJOR]
fetchGeminiRelayModelMetadatathrows an error with astatusproperty attached, but the type assertionas Error & { status?: number }is not checked. Callers (api-diagnostics.tsline ~413) catch withhttpErrtyped asunknownand pass togetApiErrorInfo, which relies onerr.status. If the error is not the constructed one (e.g., anAbortError),statuswill be undefined, potentially causinggetApiErrorInfoto misreport. Ensure the thrown error always carries astatusor handle undefined status gracefully.
src/main/config/gemini-relay-probe.ts:110-113
Suggested fix:if (!response.ok) { const body = await response.text(); const err = new Error(body || `HTTP ${response.status}`); (err as any).status = response.status; throw err; }
(or use a custom error class with a typed
statusfield). -
[MINOR] Missing unit tests for the critical functions in
gemini-relay-probe.ts(probeGeminiRelayGenerateContent,fetchGeminiRelayModelMetadata,isGeminiSdkProbeUnavailableError). The integration code inapi-diagnostics.tsandclaude-sdk-one-shot.tsrelies on these functions; without tests, regressions are harder to catch. Add tests mockingfetchand verifying fallback behavior, error mapping, and correct URL construction.
src/tests/config/gemini-relay-probe.test.ts(new) -
[MINOR] In
api-diagnostics.ts, the fallback instepAuthfor Gemini auth (lines ~388-417) usesDate.now() - startto updatestep.latencyMson success, butstartis not defined in the provided patch. The variablestartis declared earlier (likely in the surrounding function). For consistency, verifystartcaptures the correct timestamp before the auth step, not before the outer diagnostic start. Ifstartis not re-initialized per step, the latency may include time from earlier steps (DNS/TCP/TLS). Review of fullapi-diagnostics.tsneeded.
src/main/config/api-diagnostics.ts:399 -
[NIT] The
geminiRelayMetadataBaseUrlfunction always appends/v1betaif the base does not end with/v1betaor/gemini. For custom base URLs that already contain a path (e.g.,http://proxy.example.com/gemini/v1beta), an extra/v1betamay be appended, resulting in a malformed URL. Document this expectation or handle the case where the base already includesv1betain a sub-path.
src/main/config/gemini-relay-probe.ts:34-42 -
[NIT] The test for
google-genai-loadermockscreateRequireat module level, but the mock throws for unexpected require calls. This is brittle if the loader module ever imports other dependencies viacreateRequire. Consider usingvi.mock('node:module', ...)with a spy that returns a function lazily, or usevi.importActualto preserve real behavior for non-@google/genairequires.
src/tests/config/google-genai-loader.test.ts:12-18
Questions
- Is the
startvariable instepAuthintended to measure only the auth step, or the cumulative time including prior steps? If the latter, usingDate.now() - startforstep.latencyMsafter fallback success may over-report. Please confirm.
Testing
- Added tests:
src/tests/config/google-genai-loader.test.ts(1 test, verifies loader returns mock and creates client) - Missing tests: No tests for
gemini-relay-probe.ts(4 functions), no integration tests for the fallback logic inapi-diagnostics.tsorclaude-sdk-one-shot.ts. - Recommendation: Add unit tests for
isGeminiSdkProbeUnavailableError,fetchGeminiRelayModelMetadata,probeGeminiRelayGenerateContent(mock globalfetch), and a test for the diagnostics fallback flow.
Open Cowork Bot
There was a problem hiding this comment.
Review mode: follow-up after new commits
What changed since last review
- Removed unused import
probeGeminiRelayGenerateContentfromsrc/main/config/api-diagnostics.ts. This resolves the earlier minor issue about an unused import. ✅
Remaining Findings
-
[MAJOR]
isGeminiSdkProbeUnavailableErrorregex is too broad:/reading ['"]get['"]|reading get/imay match many unrelated error strings (e.g., "Error reading getter"). This can cause false-positive HTTP fallback attempts, masking real SDK initialization failures. Narrow the pattern to require'models.get'or'client.models.get'context, or match the exact known error message.
src/main/config/gemini-relay-probe.ts:10
Suggested fix:export function isGeminiSdkProbeUnavailableError(message: string): boolean { return /models\.get|client\.models\.get|cannot read properties of undefined \(reading 'get'\)/i.test(message); }
-
[MAJOR] In
fetchGeminiRelayModelMetadata(line 110-113), the thrown error is typed asError & { status?: number }via assertion, but thestatusproperty is not guaranteed (e.g.,AbortErrorfromfetchtimeout won't havestatus). DownstreamgetApiErrorInforelies onerr.statusand may misreport. Ensure the error always carries astatusor handle undefined gracefully.
src/main/config/gemini-relay-probe.ts:110-113
Suggested fix:if (!response.ok) { const body = await response.text(); const err = new Error(body || `HTTP ${response.status}`); (err as any).status = response.status; // or use a custom error class throw err; }
-
[MINOR] Missing unit tests for
gemini-relay-probe.ts:probeGeminiRelayGenerateContent,fetchGeminiRelayModelMetadata,isGeminiSdkProbeUnavailableError, and error mapping functions. These are critical for fallback behavior; without tests, regressions are harder to catch.
src/tests/config/gemini-relay-probe.test.ts(new) -
[NIT]
geminiRelayMetadataBaseUrlalways appends/v1betawhen the base does not end with/v1betaor/gemini. Custom base URLs that already contain a path like/custom/pathwill get an extra/v1beta, potentially forming a malformed URL. Either document this assumption or handle path segments more carefully.
src/main/config/gemini-relay-probe.ts:34-42 -
[NIT] The test mock for
createRequireingoogle-genai-loader.test.tsthrows on unexpected require calls, making it brittle if the loader ever needs to require other modules. Consider usingvi.mockwith a spy that delegates to the real implementation for non-@google/genaicalls.
src/tests/config/google-genai-loader.test.ts:12-18
Testing
- Existing test:
src/tests/config/google-genai-loader.test.ts(1 test, passes). - Missing tests for
gemini-relay-probe.tsas noted above. Recommended: unit tests mockingfetchand verifying URL construction + error mapping.
Open Cowork Bot
There was a problem hiding this comment.
Review mode: follow-up after new commits
Findings
-
MAJOR Previous Gemini fix findings remain unaddressed. The issues identified in the initial review have not been fixed in the new commits:
isGeminiSdkProbeUnavailableErrorregex is too broad (src/main/config/gemini-relay-probe.ts:10). Narrow to requireclient.models.getcontext or exact known error message.fetchGeminiRelayModelMetadataerror handling (src/main/config/gemini-relay-probe.ts:110-113):statusproperty not guaranteed on abort errors. Ensure it's always set or handle undefined.- Missing unit tests for
gemini-relay-probe.ts. geminiRelayMetadataBaseUrlalways appends/v1beta(src/main/config/gemini-relay-probe.ts:34-42), potentially malforming custom paths.- The test mock in
google-genai-loader.test.tsis brittle (src/tests/config/google-genai-loader.test.ts:12-18).
-
MAJOR New file
agent-runner-loop-guard.tsintroduces a complex loop detection mechanism without any unit tests. This is a significant new feature with zero test coverage. Additionally, the streak flags (streakWarnIssued,streakHaltIssued,streakAbortIssued) are not reset when a new hash streak begins, which could cause incorrect suppression of warnings in subsequent streaks. Add tests insrc/tests/claude/agent-runner-loop-guard.test.tsand reset streak flags whencurrentHashchanges inpushHash:} else { this.currentHash = hash; this.currentStreak = 1; this.streakWarnIssued = false; this.streakHaltIssued = false; this.streakAbortIssued = false; }
-
MINOR Version bump from 3.3.0 to 3.3.1 is included without justification. The PR description only references the Gemini fix, but the new loop guard feature is a significant addition that should be reflected in the description (and might warrant a minor version bump). Update the PR description to describe all changes.
-
MINOR The PR mixes two unrelated concerns: the Gemini relay fix (original changes) and the loop guard feature (new
agent-runner-loop-guard.ts, version bump, patch updates). Consider separating into two focused PRs for clarity and easier review. -
NIT In
agent-runner-loop-guard.ts, the comment says "MD5 over stable keys" but the implementation uses MD5 over JSON-stringified sorted key fields. Update the comment for accuracy.
Summary
The PR includes two major categories of changes: (1) the original Gemini relay fix (externalizing @google/genai, adding HTTP fallback probes) and (2) an unrelated new loop guard feature (agent-runner-loop-guard.ts) plus infrastructure updates. The Gemini fix issues from the previous review remain entirely unaddressed. The loop guard is a new, untested feature with a potential bug in streak flag management. The PR mixes concerns and lacks tests for the new module.
Testing
- Gemini fix: Missing tests for
gemini-relay-probe.ts. The existing test forgoogle-genai-loader.test.tsis brittle. - Loop guard: No tests added. Recommended: comprehensive unit tests for
stableToolKey,messageCallsHash, andLoopGuardclass covering streak, frequency, and edge cases. - Not run (automation)
Open Cowork Bot
fix(gemini): externalize @google/genai and add relay HTTP probes
Bundle
@google/genaioutside the pi-ai chunk somodels.getworks in diagnostics and runtime. Add HTTP fallbacks for one-api Gemini relays and ship genai deps in the installer.Summary
Fixes broken Custom + Gemini + custom Base URL connectivity checks in the Electron main process (e.g. one-api at
http://127.0.0.1:13000/gemini).Problem: Auth and model diagnostic steps fail with
Cannot read properties of undefined (reading 'get')within ~7ms. The same proxy works via gemini-cli and curl; the failure is a bundled SDK initialization issue, not a network timeout.Root cause: Vite bundles
@google/genaiinto the same mega-chunk aspi-ai. The resultingGoogleGenAIinstance does not initializeclient.modelscorrectly, somodels.get()andcompleteSimpleboth fail.This PR:
@google/genaias external in the main-process Vite build and loads the Node package at runtime viacreateRequire(google-genai-loader.ts).gemini-relay-probe.ts):…/v1beta/models/{model}(auth / metadata)…/models/{model}:generateContent(matches pi-ai whenapiVersionis cleared for custombaseUrl)@google/genaiand transitive dependencies in the installer (electron-builder.yml).Recommended proxy settings (one-api):
http://127.0.0.1:13000/geminisk-…token (not a Google AI Studio key)gemini-2.5-flash,gemini-3.1-pro-preview(must be allowed on the token)Type of change
fix)feat)refactor/perf)docs)test)build/ci)Checklist
feat:,fix:, etc.)npm run testpasses locallynpm run lintpasses locallyen+zh) — N/A (no new UI copy)Testing
Unit tests
npm run test -- --run src/tests/config/google-genai-loader.test.ts src/tests/config/api-diagnostics.test.tsBuild verification
dist-electron/main/google-*.jsusesrequire("@google/genai")(external, not inlined).api-diagnostics-*.jsloads genai viacreateRequire("@google/genai").Manual (one-api relay)
http://127.0.0.1:3000/geminisk-…tokengemini-2.5-flashorgemini-3.1-pro-previewFallback paths
reading 'get', auth should fall back to HTTP GET; model probe / test connection should fall back to HTTPgenerateContentand succeed.Regression (optional)
Screenshots / recordings (if applicable)
N/A — main-process diagnostics and connection testing only; no UI layout changes.
Example logs before fix: