diff --git a/packages/cli/src/config/config.test.ts b/packages/cli/src/config/config.test.ts index 5abf181e9..50f869e94 100644 --- a/packages/cli/src/config/config.test.ts +++ b/packages/cli/src/config/config.test.ts @@ -438,6 +438,40 @@ describe('loadCliConfig', () => { const config = await loadCliConfig(settings, [], 'test-session', argv); expect(config.getProxy()).toBe('http://localhost:7890'); }); + + it('should disable proxy when --proxy="" is explicitly provided', async () => { + // Set environment variable that would normally be used + vi.stubEnv('HTTPS_PROXY', 'http://localhost:7891'); + process.argv = ['node', 'script.js', '--proxy', '']; + const argv = await parseArguments({} as Settings); + const settings: Settings = {}; + const config = await loadCliConfig(settings, [], 'test-session', argv); + // Empty string should disable proxy (return undefined) + expect(config.getProxy()).toBeUndefined(); + }); + + it('should fall back to environment variables when --proxy is not provided', async () => { + vi.stubEnv('HTTPS_PROXY', 'http://env-proxy:8080'); + process.argv = ['node', 'script.js']; // No --proxy argument + const argv = await parseArguments({} as Settings); + const settings: Settings = {}; + const config = await loadCliConfig(settings, [], 'test-session', argv); + expect(config.getProxy()).toBe('http://env-proxy:8080'); + }); + + it('should respect proxy precedence: HTTPS_PROXY > https_proxy > HTTP_PROXY > http_proxy', async () => { + // Test HTTPS_PROXY takes precedence + vi.stubEnv('HTTPS_PROXY', 'http://https-upper:8080'); + vi.stubEnv('https_proxy', 'http://https-lower:8080'); + vi.stubEnv('HTTP_PROXY', 'http://http-upper:8080'); + vi.stubEnv('http_proxy', 'http://http-lower:8080'); + + process.argv = ['node', 'script.js']; + const argv = await parseArguments({} as Settings); + const settings: Settings = {}; + const config = await loadCliConfig(settings, [], 'test-session', argv); + expect(config.getProxy()).toBe('http://https-upper:8080'); + }); }); }); diff --git a/packages/cli/src/config/config.ts b/packages/cli/src/config/config.ts index da06e35c8..38ea2e942 100755 --- a/packages/cli/src/config/config.ts +++ b/packages/cli/src/config/config.ts @@ -614,12 +614,7 @@ export async function loadCliConfig( }, checkpointing: argv.checkpointing || settings.general?.checkpointing?.enabled, - proxy: - argv.proxy || - process.env['HTTPS_PROXY'] || - process.env['https_proxy'] || - process.env['HTTP_PROXY'] || - process.env['http_proxy'], + proxy: getProxyConfiguration(argv.proxy), cwd, fileDiscoveryService: fileService, bugCommand: settings.advanced?.bugCommand, @@ -738,3 +733,25 @@ function mergeExcludeTools( } return [...allExcludeTools]; } + +/** + * Determines the proxy configuration with proper handling of explicit empty string overrides. + * + * @param argvProxy - The proxy value from command line arguments + * @returns The proxy URL string or undefined if no proxy should be used + */ +function getProxyConfiguration(argvProxy: string | undefined): string | undefined { + // If --proxy was explicitly provided (even as empty string), respect that choice + if (argvProxy !== undefined) { + // Empty string means explicitly disable proxy + return argvProxy === '' ? undefined : argvProxy; + } + + // If --proxy was not provided, fall back to environment variables + return ( + process.env['HTTPS_PROXY'] || + process.env['https_proxy'] || + process.env['HTTP_PROXY'] || + process.env['http_proxy'] + ); +} diff --git a/packages/core/src/core/client.test.ts b/packages/core/src/core/client.test.ts index 11cc1c38f..5504e9ef2 100644 --- a/packages/core/src/core/client.test.ts +++ b/packages/core/src/core/client.test.ts @@ -2670,4 +2670,180 @@ ${JSON.stringify( await expect(client.reinitialize()).resolves.not.toThrow(); }); }); + + describe('Proxy Configuration', () => { + let originalEnvVars: Record; + + beforeEach(() => { + // Store original environment variables + originalEnvVars = { + HTTP_PROXY: process.env['HTTP_PROXY'], + http_proxy: process.env['http_proxy'], + HTTPS_PROXY: process.env['HTTPS_PROXY'], + https_proxy: process.env['https_proxy'], + NO_PROXY: process.env['NO_PROXY'], + no_proxy: process.env['no_proxy'], + }; + + // Clear all proxy environment variables for clean test state + delete process.env['HTTP_PROXY']; + delete process.env['http_proxy']; + delete process.env['HTTPS_PROXY']; + delete process.env['https_proxy']; + delete process.env['NO_PROXY']; + delete process.env['no_proxy']; + }); + + afterEach(() => { + // Restore original environment variables + Object.entries(originalEnvVars).forEach(([key, value]) => { + if (value === undefined) { + delete process.env[key]; + } else { + process.env[key] = value; + } + }); + }); + + it('should not setup proxy when no proxy is configured', () => { + // Arrange: No proxy in config, no environment variables + mockConfigObject.getProxy.mockReturnValue(undefined); + + // Act: Create a new client (which calls setupProxyConfiguration) + const testClient = new GeminiClient( + new Config({ sessionId: 'test-proxy-none' } as never), + ); + + // Assert: No proxy should be set up (we can't easily test setGlobalDispatcher, + // but we can verify the logic by checking environment variables weren't modified) + expect(process.env['HTTP_PROXY']).toBeUndefined(); + expect(process.env['HTTPS_PROXY']).toBeUndefined(); + }); + + it('should setup proxy when --proxy parameter is provided', () => { + // Arrange: Proxy from --proxy parameter, no environment variables + const proxyUrl = 'http://cli-proxy:8080'; + mockConfigObject.getProxy.mockReturnValue(proxyUrl); + + // Act: Create a new client + const testClient = new GeminiClient( + new Config({ sessionId: 'test-proxy-cli' } as never), + ); + + // Assert: Environment variables should be set for EnvHttpProxyAgent + expect(process.env['HTTP_PROXY']).toBe(proxyUrl); + expect(process.env['HTTPS_PROXY']).toBe(proxyUrl); + }); + + it('should not override existing environment variables when --proxy is provided', () => { + // Arrange: Both --proxy parameter and existing environment variables + const cliProxyUrl = 'http://cli-proxy:8080'; + const envProxyUrl = 'https://env-proxy:3128'; + + process.env['HTTPS_PROXY'] = envProxyUrl; + mockConfigObject.getProxy.mockReturnValue(cliProxyUrl); + + // Act: Create a new client + const testClient = new GeminiClient( + new Config({ sessionId: 'test-proxy-both' } as never), + ); + + // Assert: Existing environment variables should not be overridden + expect(process.env['HTTPS_PROXY']).toBe(envProxyUrl); + // HTTP_PROXY should not be set because HTTPS_PROXY is already set + expect(process.env['HTTP_PROXY']).toBeUndefined(); + }); + + it('should setup proxy when only environment variables are set', () => { + // Arrange: No --proxy parameter, but environment variables are set + const envProxyUrl = 'https://corporate-proxy:8080'; + process.env['HTTPS_PROXY'] = envProxyUrl; + mockConfigObject.getProxy.mockReturnValue(undefined); + + // Act: Create a new client + const testClient = new GeminiClient( + new Config({ sessionId: 'test-proxy-env' } as never), + ); + + // Assert: Environment variables should remain unchanged + expect(process.env['HTTPS_PROXY']).toBe(envProxyUrl); + }); + + it('should include NO_PROXY values in proxy configuration', () => { + // Arrange: Set up proxy and NO_PROXY environment variables + const proxyUrl = 'http://proxy:8080'; + const noProxyHosts = 'internal-llm.company.com,localhost'; + + process.env['HTTPS_PROXY'] = proxyUrl; + process.env['NO_PROXY'] = noProxyHosts; + mockConfigObject.getProxy.mockReturnValue(undefined); + + // Act: Create a new client (this calls setupProxyConfiguration) + const testClient = new GeminiClient( + new Config({ sessionId: 'test-no-proxy' } as never), + ); + + // Assert: NO_PROXY should still be set + expect(process.env['NO_PROXY']).toBe(noProxyHosts); + + // Note: We can't easily test the EnvHttpProxyAgent configuration directly, + // but the setupProxyConfiguration method should build a noProxy list that includes: + // - The existing NO_PROXY value + // - localhost, 127.0.0.1, ::1 (automatically added) + }); + + it('should handle mixed case environment variables', () => { + // Arrange: Use lowercase environment variables + const proxyUrl = 'http://lowercase-proxy:3128'; + process.env['http_proxy'] = proxyUrl; + process.env['no_proxy'] = 'example.com'; + mockConfigObject.getProxy.mockReturnValue(undefined); + + // Act: Create a new client + const testClient = new GeminiClient( + new Config({ sessionId: 'test-proxy-lowercase' } as never), + ); + + // Assert: Lowercase variables should be recognized + expect(process.env['http_proxy']).toBe(proxyUrl); + expect(process.env['no_proxy']).toBe('example.com'); + }); + + it('should work with --proxy=undefined (explicit disable)', () => { + // Arrange: --proxy explicitly set to undefined, but environment variables exist + const envProxyUrl = 'https://env-proxy:8080'; + process.env['HTTPS_PROXY'] = envProxyUrl; + mockConfigObject.getProxy.mockReturnValue(undefined); // This simulates --proxy="" + + // Act: Create a new client + const testClient = new GeminiClient( + new Config({ sessionId: 'test-proxy-disable' } as never), + ); + + // Assert: Environment variables should still be used + // (because --proxy="" disables config proxy but doesn't clear env vars) + expect(process.env['HTTPS_PROXY']).toBe(envProxyUrl); + }); + + it('should automatically add localhost to NO_PROXY list', () => { + // This test verifies the logic that automatically includes localhost variants + // in the NO_PROXY list to prevent issues with local services + + // Arrange: Set up proxy without any NO_PROXY + const proxyUrl = 'http://proxy:8080'; + mockConfigObject.getProxy.mockReturnValue(proxyUrl); + + // Act: Create a new client + const testClient = new GeminiClient( + new Config({ sessionId: 'test-auto-localhost' } as never), + ); + + // Assert: The setupProxyConfiguration method should have been called + // and would have built a noProxy list including localhost variants + // We can't directly test the EnvHttpProxyAgent configuration, + // but we can verify the environment was set up correctly + expect(process.env['HTTP_PROXY']).toBe(proxyUrl); + expect(process.env['HTTPS_PROXY']).toBe(proxyUrl); + }); + }); }); diff --git a/packages/core/src/core/client.ts b/packages/core/src/core/client.ts index 898cee3a0..aef260846 100644 --- a/packages/core/src/core/client.ts +++ b/packages/core/src/core/client.ts @@ -14,7 +14,7 @@ import type { Schema, Tool, } from '@google/genai'; -import { ProxyAgent, setGlobalDispatcher } from 'undici'; +import { EnvHttpProxyAgent, setGlobalDispatcher } from 'undici'; import type { UserTierId } from '../code_assist/types.js'; import type { Config } from '../config/config.js'; import { ApprovalMode } from '../config/config.js'; @@ -130,15 +130,57 @@ export class GeminiClient { private hasFailedCompressionAttempt = false; constructor(private readonly config: Config) { - if (config.getProxy()) { - setGlobalDispatcher(new ProxyAgent(config.getProxy() as string)); - } + this.setupProxyConfiguration(); this.embeddingModel = config.getEmbeddingModel(); this.loopDetector = new LoopDetectionService(config); this.lastPromptId = this.config.getSessionId(); } + /** + * Sets up proxy configuration with NO_PROXY support for LLM requests. + * Supports both --proxy parameter and standard proxy environment variables. + * Uses EnvHttpProxyAgent which respects NO_PROXY, unlike the basic ProxyAgent. + */ + private setupProxyConfiguration(): void { + const configProxy = this.config.getProxy(); // This includes --proxy parameter handling + + // Check if any proxy environment variables are already set + const hasProxyEnvVars = !!( + process.env['HTTP_PROXY'] || + process.env['http_proxy'] || + process.env['HTTPS_PROXY'] || + process.env['https_proxy'] + ); + + // If no proxy is configured via --proxy or environment variables, nothing to do + if (!configProxy && !hasProxyEnvVars) { + return; + } + + // If --proxy was specified but no environment variables are set, + // temporarily set environment variables for EnvHttpProxyAgent to use + if (configProxy && !hasProxyEnvVars) { + // Set both HTTP and HTTPS proxy environment variables + process.env['HTTPS_PROXY'] = configProxy; + process.env['HTTP_PROXY'] = configProxy; + } + + // Build NO_PROXY list including localhost and any existing NO_PROXY values + const existingNoProxy = process.env['NO_PROXY'] || process.env['no_proxy'] || ''; + const noProxyList = [existingNoProxy, 'localhost', '127.0.0.1', '::1'] + .filter(Boolean) + .join(','); + + // Create EnvHttpProxyAgent that respects NO_PROXY and reads proxy URLs from environment + const proxyAgent = new EnvHttpProxyAgent({ + // Respect NO_PROXY environment variable + noProxy: noProxyList, + }); + + setGlobalDispatcher(proxyAgent); + } + async initialize( contentGeneratorConfig: ContentGeneratorConfig, extraHistory?: Content[],