From 181abde2ffa8f1f68b6c540a5600346116cb5145 Mon Sep 17 00:00:00 2001 From: Marat Boshernitsan Date: Thu, 12 Jun 2025 17:17:29 -0700 Subject: [PATCH] Reduce coupling between core and cli packages (#961) Co-authored-by: Marat Boshernitsan --- package-lock.json | 2 +- .../cli/src/config/config.integration.test.ts | 9 ++ packages/cli/src/config/config.ts | 49 ++++++++++- packages/cli/src/ui/App.test.tsx | 1 + packages/cli/vitest.config.ts | 2 + packages/core/package.json | 2 +- packages/core/src/config/config.test.ts | 1 + packages/core/src/config/config.ts | 56 ++++-------- packages/core/src/core/client.test.ts | 70 +++++++++++++++ packages/core/src/core/client.ts | 17 ++-- packages/core/src/core/geminiChat.test.ts | 69 ++++++++++++++- packages/core/src/core/geminiRequest.test.ts | 85 +++++++++++++++++++ packages/core/src/tools/tool-registry.test.ts | 1 + packages/core/vitest.config.ts | 2 + 14 files changed, 310 insertions(+), 56 deletions(-) create mode 100644 packages/core/src/core/geminiRequest.test.ts diff --git a/package-lock.json b/package-lock.json index 3031663e..845de105 100644 --- a/package-lock.json +++ b/package-lock.json @@ -10814,7 +10814,7 @@ "ignore": "^7.0.0", "open": "^10.1.2", "shell-quote": "^1.8.2", - "simple-git": "^3.27.0", + "simple-git": "^3.28.0", "strip-ansi": "^7.1.0", "undici": "^7.10.0" }, diff --git a/packages/cli/src/config/config.integration.test.ts b/packages/cli/src/config/config.integration.test.ts index 2cbe8c66..99745d29 100644 --- a/packages/cli/src/config/config.integration.test.ts +++ b/packages/cli/src/config/config.integration.test.ts @@ -53,6 +53,7 @@ describe('Configuration Integration Tests', () => { describe('File Filtering Configuration', () => { it('should load default file filtering settings', async () => { const configParams: ConfigParameters = { + cwd: '/tmp', contentGeneratorConfig: TEST_CONTENT_GENERATOR_CONFIG, embeddingModel: 'test-embedding-model', sandbox: false, @@ -70,6 +71,7 @@ describe('Configuration Integration Tests', () => { it('should load custom file filtering settings from configuration', async () => { const configParams: ConfigParameters = { + cwd: '/tmp', contentGeneratorConfig: TEST_CONTENT_GENERATOR_CONFIG, embeddingModel: 'test-embedding-model', sandbox: false, @@ -87,6 +89,7 @@ describe('Configuration Integration Tests', () => { it('should merge user and workspace file filtering settings', async () => { const configParams: ConfigParameters = { + cwd: '/tmp', contentGeneratorConfig: TEST_CONTENT_GENERATOR_CONFIG, embeddingModel: 'test-embedding-model', sandbox: false, @@ -106,6 +109,7 @@ describe('Configuration Integration Tests', () => { describe('Configuration Integration', () => { it('should handle partial configuration objects gracefully', async () => { const configParams: ConfigParameters = { + cwd: '/tmp', contentGeneratorConfig: TEST_CONTENT_GENERATOR_CONFIG, embeddingModel: 'test-embedding-model', sandbox: false, @@ -126,6 +130,7 @@ describe('Configuration Integration Tests', () => { it('should handle empty configuration objects gracefully', async () => { const configParams: ConfigParameters = { + cwd: '/tmp', contentGeneratorConfig: TEST_CONTENT_GENERATOR_CONFIG, embeddingModel: 'test-embedding-model', sandbox: false, @@ -144,6 +149,7 @@ describe('Configuration Integration Tests', () => { it('should handle missing configuration sections gracefully', async () => { const configParams: ConfigParameters = { + cwd: '/tmp', contentGeneratorConfig: TEST_CONTENT_GENERATOR_CONFIG, embeddingModel: 'test-embedding-model', sandbox: false, @@ -163,6 +169,7 @@ describe('Configuration Integration Tests', () => { describe('Real-world Configuration Scenarios', () => { it('should handle a security-focused configuration', async () => { const configParams: ConfigParameters = { + cwd: '/tmp', contentGeneratorConfig: TEST_CONTENT_GENERATOR_CONFIG, embeddingModel: 'test-embedding-model', sandbox: false, @@ -180,6 +187,7 @@ describe('Configuration Integration Tests', () => { it('should handle a development-focused configuration', async () => { const configParams: ConfigParameters = { + cwd: '/tmp', contentGeneratorConfig: TEST_CONTENT_GENERATOR_CONFIG, embeddingModel: 'test-embedding-model', sandbox: false, @@ -196,6 +204,7 @@ describe('Configuration Integration Tests', () => { it('should handle a CI/CD environment configuration', async () => { const configParams: ConfigParameters = { + cwd: '/tmp', contentGeneratorConfig: TEST_CONTENT_GENERATOR_CONFIG, embeddingModel: 'test-embedding-model', sandbox: false, diff --git a/packages/cli/src/config/config.ts b/packages/cli/src/config/config.ts index 2f989883..3ee03c82 100644 --- a/packages/cli/src/config/config.ts +++ b/packages/cli/src/config/config.ts @@ -9,16 +9,20 @@ import { hideBin } from 'yargs/helpers'; import process from 'node:process'; import { Config, - loadEnvironment, loadServerHierarchicalMemory, setGeminiMdFilename as setServerGeminiMdFilename, getCurrentGeminiMdFilename, ApprovalMode, ContentGeneratorConfig, + GEMINI_CONFIG_DIR as GEMINI_DIR, } from '@gemini-cli/core'; import { Settings } from './settings.js'; import { getEffectiveModel } from '../utils/modelCheck.js'; import { ExtensionConfig } from './extension.js'; +import * as dotenv from 'dotenv'; +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import * as os from 'node:os'; // Simple console logger for now - replace with actual logger if available const logger = { @@ -196,6 +200,13 @@ export async function loadCliConfig( fileFilteringAllowBuildArtifacts: settings.fileFiltering?.allowBuildArtifacts, checkpoint: argv.checkpoint, + proxy: + process.env.HTTPS_PROXY || + process.env.https_proxy || + process.env.HTTP_PROXY || + process.env.http_proxy, + cwd: process.cwd(), + telemetryOtlpEndpoint: process.env.OTEL_EXPORTER_OTLP_ENDPOINT, }); } @@ -259,3 +270,39 @@ async function createContentGeneratorConfig( return config; } + +function findEnvFile(startDir: string): string | null { + let currentDir = path.resolve(startDir); + while (true) { + // prefer gemini-specific .env under GEMINI_DIR + const geminiEnvPath = path.join(currentDir, GEMINI_DIR, '.env'); + if (fs.existsSync(geminiEnvPath)) { + return geminiEnvPath; + } + const envPath = path.join(currentDir, '.env'); + if (fs.existsSync(envPath)) { + return envPath; + } + const parentDir = path.dirname(currentDir); + if (parentDir === currentDir || !parentDir) { + // check .env under home as fallback, again preferring gemini-specific .env + const homeGeminiEnvPath = path.join(os.homedir(), GEMINI_DIR, '.env'); + if (fs.existsSync(homeGeminiEnvPath)) { + return homeGeminiEnvPath; + } + const homeEnvPath = path.join(os.homedir(), '.env'); + if (fs.existsSync(homeEnvPath)) { + return homeEnvPath; + } + return null; + } + currentDir = parentDir; + } +} + +export function loadEnvironment(): void { + const envFilePath = findEnvFile(process.cwd()); + if (envFilePath) { + dotenv.config({ path: envFilePath }); + } +} diff --git a/packages/cli/src/ui/App.test.tsx b/packages/cli/src/ui/App.test.tsx index bfd2efaf..b8959bfb 100644 --- a/packages/cli/src/ui/App.test.tsx +++ b/packages/cli/src/ui/App.test.tsx @@ -194,6 +194,7 @@ describe('App UI', () => { geminiMdFileCount: 0, showMemoryUsage: false, sessionId: 'test-session-id', + cwd: '/tmp', // Provide other required fields for ConfigParameters if necessary }) as unknown as MockServerConfig; diff --git a/packages/cli/vitest.config.ts b/packages/cli/vitest.config.ts index a7548310..fd8a1124 100644 --- a/packages/cli/vitest.config.ts +++ b/packages/cli/vitest.config.ts @@ -16,8 +16,10 @@ export default defineConfig({ junit: 'junit.xml', }, coverage: { + enabled: true, provider: 'v8', reportsDirectory: './coverage', + include: ['src/**/*'], reporter: [ ['text', { file: 'full-text-summary.txt' }], 'html', diff --git a/packages/core/package.json b/packages/core/package.json index 9dfd6c5e..32f1462a 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -37,7 +37,7 @@ "ignore": "^7.0.0", "open": "^10.1.2", "shell-quote": "^1.8.2", - "simple-git": "^3.27.0", + "simple-git": "^3.28.0", "strip-ansi": "^7.1.0", "undici": "^7.10.0" }, diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index 2827f581..71af832b 100644 --- a/packages/core/src/config/config.test.ts +++ b/packages/core/src/config/config.test.ts @@ -51,6 +51,7 @@ describe('Server Config (config.ts)', () => { const EMBEDDING_MODEL = 'gemini-embedding'; const SESSION_ID = 'test-session-id'; const baseParams: ConfigParameters = { + cwd: '/tmp', contentGeneratorConfig: { apiKey: API_KEY, model: MODEL, diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index b94a88a4..3bb9815f 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -4,11 +4,8 @@ * SPDX-License-Identifier: Apache-2.0 */ -import * as dotenv from 'dotenv'; -import * as fs from 'node:fs'; import * as path from 'node:path'; import process from 'node:process'; -import * as os from 'node:os'; import { ContentGeneratorConfig } from '../core/contentGenerator.js'; import { ToolRegistry } from '../tools/tool-registry.js'; import { LSTool } from '../tools/ls.js'; @@ -79,9 +76,12 @@ export interface ConfigParameters { accessibility?: AccessibilitySettings; telemetry?: boolean; telemetryLogUserPromptsEnabled?: boolean; + telemetryOtlpEndpoint?: string; fileFilteringRespectGitIgnore?: boolean; fileFilteringAllowBuildArtifacts?: boolean; checkpoint?: boolean; + proxy?: string; + cwd: string; } export class Config { @@ -115,6 +115,8 @@ export class Config { private fileDiscoveryService: FileDiscoveryService | null = null; private gitService: GitService | undefined = undefined; private readonly checkpoint: boolean; + private readonly proxy: string | undefined; + private readonly cwd: string; constructor(params: ConfigParameters) { this.sessionId = params.sessionId; @@ -140,12 +142,14 @@ export class Config { this.telemetryLogUserPromptsEnabled = params.telemetryLogUserPromptsEnabled ?? true; this.telemetryOtlpEndpoint = - process.env.OTEL_EXPORTER_OTLP_ENDPOINT ?? 'http://localhost:4317'; + params.telemetryOtlpEndpoint ?? 'http://localhost:4317'; this.fileFilteringRespectGitIgnore = params.fileFilteringRespectGitIgnore ?? true; this.fileFilteringAllowBuildArtifacts = params.fileFilteringAllowBuildArtifacts ?? false; this.checkpoint = params.checkpoint ?? false; + this.proxy = params.proxy; + this.cwd = params.cwd ?? process.cwd(); if (params.contextFileName) { setGeminiMdFilename(params.contextFileName); @@ -297,6 +301,14 @@ export class Config { return this.checkpoint; } + getProxy(): string | undefined { + return this.proxy; + } + + getWorkingDir(): string { + return this.cwd; + } + async getFileService(): Promise { if (!this.fileDiscoveryService) { this.fileDiscoveryService = new FileDiscoveryService(this.targetDir); @@ -317,42 +329,6 @@ export class Config { } } -function findEnvFile(startDir: string): string | null { - let currentDir = path.resolve(startDir); - while (true) { - // prefer gemini-specific .env under GEMINI_DIR - const geminiEnvPath = path.join(currentDir, GEMINI_DIR, '.env'); - if (fs.existsSync(geminiEnvPath)) { - return geminiEnvPath; - } - const envPath = path.join(currentDir, '.env'); - if (fs.existsSync(envPath)) { - return envPath; - } - const parentDir = path.dirname(currentDir); - if (parentDir === currentDir || !parentDir) { - // check .env under home as fallback, again preferring gemini-specific .env - const homeGeminiEnvPath = path.join(os.homedir(), GEMINI_DIR, '.env'); - if (fs.existsSync(homeGeminiEnvPath)) { - return homeGeminiEnvPath; - } - const homeEnvPath = path.join(os.homedir(), '.env'); - if (fs.existsSync(homeEnvPath)) { - return homeEnvPath; - } - return null; - } - currentDir = parentDir; - } -} - -export function loadEnvironment(): void { - const envFilePath = findEnvFile(process.cwd()); - if (envFilePath) { - dotenv.config({ path: envFilePath }); - } -} - export function createToolRegistry(config: Config): Promise { const registry = new ToolRegistry(config); const targetDir = config.getTargetDir(); diff --git a/packages/core/src/core/client.test.ts b/packages/core/src/core/client.test.ts index 0362f72a..d227e57b 100644 --- a/packages/core/src/core/client.test.ts +++ b/packages/core/src/core/client.test.ts @@ -17,6 +17,7 @@ import { ContentGenerator } from './contentGenerator.js'; import { GeminiChat } from './geminiChat.js'; import { Config } from '../config/config.js'; import { Turn } from './turn.js'; +import { getCoreSystemPrompt } from './prompts.js'; // --- Mocks --- const mockChatCreateFn = vi.fn(); @@ -54,6 +55,11 @@ vi.mock('../utils/generateContentResponseUtilities', () => ({ result.candidates?.[0]?.content?.parts?.map((part) => part.text).join('') || undefined, })); +vi.mock('../telemetry/index.js', () => ({ + logApiRequest: vi.fn(), + logApiResponse: vi.fn(), + logApiError: vi.fn(), +})); describe('Gemini Client (client.ts)', () => { let client: GeminiClient; @@ -109,6 +115,8 @@ describe('Gemini Client (client.ts)', () => { getUserMemory: vi.fn().mockReturnValue(''), getFullContext: vi.fn().mockReturnValue(false), getSessionId: vi.fn().mockReturnValue('test-session-id'), + getProxy: vi.fn().mockReturnValue(undefined), + getWorkingDir: vi.fn().mockReturnValue('/test/dir'), }; // eslint-disable-next-line @typescript-eslint/no-explicit-any return mock as any; @@ -239,6 +247,68 @@ describe('Gemini Client (client.ts)', () => { }); }); + describe('generateContent', () => { + it('should call generateContent with the correct parameters', async () => { + const contents = [{ role: 'user', parts: [{ text: 'hello' }] }]; + const generationConfig = { temperature: 0.5 }; + const abortSignal = new AbortController().signal; + + // Mock countTokens + const mockGenerator: Partial = { + countTokens: vi.fn().mockResolvedValue({ totalTokens: 1 }), + generateContent: mockGenerateContentFn, + }; + client['contentGenerator'] = Promise.resolve( + mockGenerator as ContentGenerator, + ); + + await client.generateContent(contents, generationConfig, abortSignal); + + expect(mockGenerateContentFn).toHaveBeenCalledWith({ + model: 'test-model', + config: { + abortSignal, + systemInstruction: getCoreSystemPrompt(''), + temperature: 0.5, + topP: 1, + }, + contents, + }); + }); + }); + + describe('generateJson', () => { + it('should call generateContent with the correct parameters', async () => { + const contents = [{ role: 'user', parts: [{ text: 'hello' }] }]; + const schema = { type: 'string' }; + const abortSignal = new AbortController().signal; + + // Mock countTokens + const mockGenerator: Partial = { + countTokens: vi.fn().mockResolvedValue({ totalTokens: 1 }), + generateContent: mockGenerateContentFn, + }; + client['contentGenerator'] = Promise.resolve( + mockGenerator as ContentGenerator, + ); + + await client.generateJson(contents, schema, abortSignal); + + expect(mockGenerateContentFn).toHaveBeenCalledWith({ + model: 'gemini-2.0-flash', + config: { + abortSignal, + systemInstruction: getCoreSystemPrompt(''), + temperature: 0, + topP: 1, + responseSchema: schema, + responseMimeType: 'application/json', + }, + contents, + }); + }); + }); + describe('addHistory', () => { it('should call chat.addHistory with the provided content', async () => { const mockChat = { diff --git a/packages/core/src/core/client.ts b/packages/core/src/core/client.ts index 94cdf0e5..83c322b0 100644 --- a/packages/core/src/core/client.ts +++ b/packages/core/src/core/client.ts @@ -14,7 +14,6 @@ import { Tool, GenerateContentResponse, } from '@google/genai'; -import process from 'node:process'; import { getFolderStructure } from '../utils/getFolderStructure.js'; import { Turn, ServerGeminiStreamEvent, GeminiEventType } from './turn.js'; import { Config } from '../config/config.js'; @@ -33,16 +32,6 @@ import { } from './contentGenerator.js'; import { ProxyAgent, setGlobalDispatcher } from 'undici'; -const proxy = - process.env.HTTPS_PROXY || - process.env.https_proxy || - process.env.HTTP_PROXY || - process.env.http_proxy; - -if (proxy) { - setGlobalDispatcher(new ProxyAgent(proxy)); -} - export class GeminiClient { private chat: Promise; private contentGenerator: Promise; @@ -55,6 +44,10 @@ export class GeminiClient { private readonly MAX_TURNS = 100; constructor(private config: Config) { + if (config.getProxy()) { + setGlobalDispatcher(new ProxyAgent(config.getProxy() as string)); + } + this.contentGenerator = createContentGenerator( this.config.getContentGeneratorConfig(), ); @@ -83,7 +76,7 @@ export class GeminiClient { } private async getEnvironment(): Promise { - const cwd = process.cwd(); + const cwd = this.config.getWorkingDir(); const today = new Date().toLocaleDateString(undefined, { weekday: 'long', year: 'numeric', diff --git a/packages/core/src/core/geminiChat.test.ts b/packages/core/src/core/geminiChat.test.ts index 03e933cc..24a7279d 100644 --- a/packages/core/src/core/geminiChat.test.ts +++ b/packages/core/src/core/geminiChat.test.ts @@ -5,7 +5,13 @@ */ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; -import { Content, Models, GenerateContentConfig, Part } from '@google/genai'; +import { + Content, + Models, + GenerateContentConfig, + Part, + GenerateContentResponse, +} from '@google/genai'; import { GeminiChat } from './geminiChat.js'; import { Config } from '../config/config.js'; @@ -20,6 +26,7 @@ const mockModelsModule = { const mockConfig = { getSessionId: () => 'test-session-id', + getTelemetryLogUserPromptsEnabled: () => true, } as unknown as Config; describe('GeminiChat', () => { @@ -37,6 +44,66 @@ describe('GeminiChat', () => { vi.restoreAllMocks(); }); + describe('sendMessage', () => { + it('should call generateContent with the correct parameters', async () => { + const response = { + candidates: [ + { + content: { + parts: [{ text: 'response' }], + role: 'model', + }, + finishReason: 'STOP', + index: 0, + safetyRatings: [], + }, + ], + text: () => 'response', + } as unknown as GenerateContentResponse; + vi.mocked(mockModelsModule.generateContent).mockResolvedValue(response); + + await chat.sendMessage({ message: 'hello' }); + + expect(mockModelsModule.generateContent).toHaveBeenCalledWith({ + model: 'gemini-pro', + contents: [{ role: 'user', parts: [{ text: 'hello' }] }], + config: {}, + }); + }); + }); + + describe('sendMessageStream', () => { + it('should call generateContentStream with the correct parameters', async () => { + const response = (async function* () { + yield { + candidates: [ + { + content: { + parts: [{ text: 'response' }], + role: 'model', + }, + finishReason: 'STOP', + index: 0, + safetyRatings: [], + }, + ], + text: () => 'response', + } as unknown as GenerateContentResponse; + })(); + vi.mocked(mockModelsModule.generateContentStream).mockResolvedValue( + response, + ); + + await chat.sendMessageStream({ message: 'hello' }); + + expect(mockModelsModule.generateContentStream).toHaveBeenCalledWith({ + model: 'gemini-pro', + contents: [{ role: 'user', parts: [{ text: 'hello' }] }], + config: {}, + }); + }); + }); + describe('recordHistory', () => { const userInput: Content = { role: 'user', diff --git a/packages/core/src/core/geminiRequest.test.ts b/packages/core/src/core/geminiRequest.test.ts new file mode 100644 index 00000000..fd298cb6 --- /dev/null +++ b/packages/core/src/core/geminiRequest.test.ts @@ -0,0 +1,85 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect } from 'vitest'; +import { partListUnionToString } from './geminiRequest.js'; +import { type Part } from '@google/genai'; + +describe('partListUnionToString', () => { + it('should return the string value if the input is a string', () => { + const result = partListUnionToString('hello'); + expect(result).toBe('hello'); + }); + + it('should return a concatenated string if the input is an array of strings', () => { + const result = partListUnionToString(['hello', ' ', 'world']); + expect(result).toBe('hello world'); + }); + + it('should handle videoMetadata', () => { + const part: Part = { videoMetadata: {} }; + const result = partListUnionToString(part); + expect(result).toBe('[Video Metadata]'); + }); + + it('should handle thought', () => { + const part: Part = { thought: true }; + const result = partListUnionToString(part); + expect(result).toBe('[Thought: true]'); + }); + + it('should handle codeExecutionResult', () => { + const part: Part = { codeExecutionResult: {} }; + const result = partListUnionToString(part); + expect(result).toBe('[Code Execution Result]'); + }); + + it('should handle executableCode', () => { + const part: Part = { executableCode: {} }; + const result = partListUnionToString(part); + expect(result).toBe('[Executable Code]'); + }); + + it('should handle fileData', () => { + const part: Part = { + fileData: { mimeType: 'text/plain', fileUri: 'file.txt' }, + }; + const result = partListUnionToString(part); + expect(result).toBe('[File Data]'); + }); + + it('should handle functionCall', () => { + const part: Part = { functionCall: { name: 'myFunction' } }; + const result = partListUnionToString(part); + expect(result).toBe('[Function Call: myFunction]'); + }); + + it('should handle functionResponse', () => { + const part: Part = { + functionResponse: { name: 'myFunction', response: {} }, + }; + const result = partListUnionToString(part); + expect(result).toBe('[Function Response: myFunction]'); + }); + + it('should handle inlineData', () => { + const part: Part = { inlineData: { mimeType: 'image/png', data: '...' } }; + const result = partListUnionToString(part); + expect(result).toBe(''); + }); + + it('should handle text', () => { + const part: Part = { text: 'hello' }; + const result = partListUnionToString(part); + expect(result).toBe('hello'); + }); + + it('should return an empty string for an unknown part type', () => { + const part: Part = {}; + const result = partListUnionToString(part); + expect(result).toBe(''); + }); +}); diff --git a/packages/core/src/tools/tool-registry.test.ts b/packages/core/src/tools/tool-registry.test.ts index f4cedcd4..5837fd76 100644 --- a/packages/core/src/tools/tool-registry.test.ts +++ b/packages/core/src/tools/tool-registry.test.ts @@ -124,6 +124,7 @@ class MockTool extends BaseTool<{ param: string }, ToolResult> { } const baseConfigParams: ConfigParameters = { + cwd: '/tmp', contentGeneratorConfig: { model: 'test-model', apiKey: 'test-api-key', diff --git a/packages/core/vitest.config.ts b/packages/core/vitest.config.ts index 17defb41..f5b2650b 100644 --- a/packages/core/vitest.config.ts +++ b/packages/core/vitest.config.ts @@ -13,8 +13,10 @@ export default defineConfig({ junit: 'junit.xml', }, coverage: { + enabled: true, provider: 'v8', reportsDirectory: './coverage', + include: ['src/**/*'], reporter: [ ['text', { file: 'full-text-summary.txt' }], 'html',