refactor(core): Improve environment variable handling in shell execution

integration-tests/run_shell_command.test.ts

@@ -515,40 +515,6 @@ describe('run_shell_command', () => {
     );
   });
 
-  it('should propagate environment variables to the child process', async () => {
-    const rig = new TestRig();
-    await rig.setup('should propagate environment variables', {
-      settings: { tools: { core: ['run_shell_command'] } },
-    });
-
-    const varName = 'GEMINI_CLI_TEST_VAR';
-    const varValue = `test-value-${Math.random().toString(36).substring(7)}`;
-    process.env[varName] = varValue;
-
-    try {
-      const prompt = `Use echo to learn the value of the environment variable named ${varName} and tell me what it is.`;
-      const result = await rig.run(prompt);
-
-      const foundToolCall = await rig.waitForToolCall('run_shell_command');
-
-      if (!foundToolCall || !result.includes(varValue)) {
-        printDebugInfo(rig, result, {
-          'Found tool call': foundToolCall,
-          'Contains varValue': result.includes(varValue),
-        });
-      }
-
-      expect(
-        foundToolCall,
-        'Expected to find a run_shell_command tool call',
-      ).toBeTruthy();
-      validateModelOutput(result, varValue, 'Env var propagation test');
-      expect(result).toContain(varValue);
-    } finally {
-      delete process.env[varName];
-    }
-  });
-
   it.skip('should run a platform-specific file listing command', async () => {
     const rig = new TestRig();
     await rig.setup('should run platform-specific file listing');

packages/core/src/services/shellExecutionService.test.ts

@@ -4,7 +4,15 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-import { vi, describe, it, expect, beforeEach, type Mock } from 'vitest';
+import {
+  vi,
+  describe,
+  it,
+  expect,
+  beforeEach,
+  afterEach,
+  type Mock,
+} from 'vitest';
 import EventEmitter from 'node:events';
 import type { Readable } from 'node:stream';
 import { type ChildProcess } from 'node:child_process';
@@ -1284,3 +1292,165 @@ describe('ShellExecutionService execution method selection', () => {
     expect(result.executionMethod).toBe('child_process');
   });
 });
+
+describe('ShellExecutionService environment variables', () => {
+  let mockPtyProcess: EventEmitter & {
+    pid: number;
+    kill: Mock;
+    onData: Mock;
+    onExit: Mock;
+    write: Mock;
+    resize: Mock;
+  };
+  let mockChildProcess: EventEmitter & Partial<ChildProcess>;
+
+  beforeEach(() => {
+    vi.clearAllMocks();
+    vi.resetModules(); // Reset modules to ensure process.env changes are fresh
+
+    // Mock for pty
+    mockPtyProcess = new EventEmitter() as EventEmitter & {
+      pid: number;
+      kill: Mock;
+      onData: Mock;
+      onExit: Mock;
+      write: Mock;
+      resize: Mock;
+    };
+    mockPtyProcess.pid = 12345;
+    mockPtyProcess.kill = vi.fn();
+    mockPtyProcess.onData = vi.fn();
+    mockPtyProcess.onExit = vi.fn();
+    mockPtyProcess.write = vi.fn();
+    mockPtyProcess.resize = vi.fn();
+
+    mockPtySpawn.mockReturnValue(mockPtyProcess);
+    mockGetPty.mockResolvedValue({
+      module: { spawn: mockPtySpawn },
+      name: 'mock-pty',
+    });
+
+    // Mock for child_process
+    mockChildProcess = new EventEmitter() as EventEmitter &
+      Partial<ChildProcess>;
+    mockChildProcess.stdout = new EventEmitter() as Readable;
+    mockChildProcess.stderr = new EventEmitter() as Readable;
+    mockChildProcess.kill = vi.fn();
+    Object.defineProperty(mockChildProcess, 'pid', {
+      value: 54321,
+      configurable: true,
+    });
+    mockCpSpawn.mockReturnValue(mockChildProcess);
+
+    // Default exit behavior for mocks
+    mockPtyProcess.onExit.mockImplementationOnce(({ exitCode, signal }) => {
+      // Small delay to allow async ops to complete
+      setTimeout(() => mockPtyProcess.emit('exit', { exitCode, signal }), 0);
+    });
+    mockChildProcess.on('exit', (code, signal) => {
+      // Small delay to allow async ops to complete
+      setTimeout(() => mockChildProcess.emit('close', code, signal), 0);
+    });
+  });
+
+  afterEach(() => {
+    // Clean up process.env after each test
+    vi.unstubAllEnvs();
+  });
+
+  it('should not include process.env when GITHUB_SHA is set (pty and child_process)', async () => {
+    vi.stubEnv('GITHUB_SHA', 'test-sha');
+    vi.stubEnv('MY_TEST_VAR', 'test-value');
+    vi.resetModules();
+    const { ShellExecutionService } = await import(
+      './shellExecutionService.js'
+    );
+
+    // Test pty path
+    await ShellExecutionService.execute(
+      'test-pty-command',
+      '/',
+      vi.fn(),
+      new AbortController().signal,
+      true,
+      shellExecutionConfig,
+    );
+    expect(mockPtySpawn).toHaveBeenCalled();
+    const ptyEnv = mockPtySpawn.mock.calls[0][2].env;
+    expect(ptyEnv).not.toHaveProperty('MY_TEST_VAR');
+    expect(ptyEnv).toHaveProperty('GEMINI_CLI', '1');
+    expect(ptyEnv).toHaveProperty('TERM', 'xterm-256color');
+
+    // Ensure pty process exits
+    mockPtyProcess.onExit.mock.calls[0][0]({ exitCode: 0, signal: null });
+    await new Promise(process.nextTick);
+
+    // Test child_process path (forcing fallback by making pty unavailable)
+    mockGetPty.mockResolvedValue(null);
+    await ShellExecutionService.execute(
+      'test-cp-command',
+      '/',
+      vi.fn(),
+      new AbortController().signal,
+      true, // Still tries pty, but it will fall back
+      shellExecutionConfig,
+    );
+    expect(mockCpSpawn).toHaveBeenCalled();
+    const cpEnv = mockCpSpawn.mock.calls[0][2].env;
+    expect(cpEnv).not.toHaveProperty('MY_TEST_VAR');
+    expect(cpEnv).toHaveProperty('GEMINI_CLI', '1');
+    expect(cpEnv).toHaveProperty('TERM', 'xterm-256color');
+
+    // Ensure child_process exits
+    mockChildProcess.emit('exit', 0, null);
+    mockChildProcess.emit('close', 0, null);
+    await new Promise(process.nextTick);
+  });
+  it('should include process.env when GITHUB_SHA is not set (pty and child_process)', async () => {
+    vi.stubEnv('MY_TEST_VAR', 'test-value');
+    vi.stubEnv('GITHUB_SHA', '');
+    vi.stubEnv('SURFACE', '');
+    vi.resetModules();
+    const { ShellExecutionService } = await import(
+      './shellExecutionService.js'
+    );
+
+    // Test pty path
+    await ShellExecutionService.execute(
+      'test-pty-command-no-github',
+      '/',
+      vi.fn(),
+      new AbortController().signal,
+      true,
+      shellExecutionConfig,
+    );
+    expect(mockPtySpawn).toHaveBeenCalled();
+    const ptyEnv = mockPtySpawn.mock.calls[0][2].env;
+    expect(ptyEnv).toHaveProperty('MY_TEST_VAR', 'test-value');
+    expect(ptyEnv).toHaveProperty('GEMINI_CLI', '1');
+
+    // Ensure pty process exits
+    mockPtyProcess.onExit.mock.calls[0][0]({ exitCode: 0, signal: null });
+    await new Promise(process.nextTick);
+
+    // Test child_process path (forcing fallback by making pty unavailable)
+    mockGetPty.mockResolvedValue(null);
+    await ShellExecutionService.execute(
+      'test-cp-command-no-github',
+      '/',
+      vi.fn(),
+      new AbortController().signal,
+      true, // Still tries pty, but it will fall back
+      shellExecutionConfig,
+    );
+    expect(mockCpSpawn).toHaveBeenCalled();
+    const cpEnv = mockCpSpawn.mock.calls[0][2].env;
+    expect(cpEnv).toHaveProperty('MY_TEST_VAR', 'test-value');
+    expect(cpEnv).toHaveProperty('GEMINI_CLI', '1');
+
+    // Ensure child_process exits
+    mockChildProcess.emit('exit', 0, null);
+    mockChildProcess.emit('close', 0, null);
+    await new Promise(process.nextTick);
+  });
+});

packages/core/src/services/shellExecutionService.ts

@@ -148,6 +148,21 @@ const getFullBufferText = (terminal: pkg.Terminal): string => {
   return lines.join('\n');
 };
 
+function getSanitizedEnv(): NodeJS.ProcessEnv {
+  const env: NodeJS.ProcessEnv =
+    process.env['GITHUB_SHA'] || process.env['SURFACE'] === 'Github'
+      ? {}
+      : { ...process.env };
+
+  for (const key in process.env) {
+    if (key.startsWith('GEMINI_CLI_TEST')) {
+      env[key] = process.env[key];
+    }
+  }
+
+  return env;
+}
+
 /**
  * A centralized service for executing shell commands with robust process
  * management, cross-platform compatibility, and streaming output capabilities.
@@ -249,7 +264,7 @@ export class ShellExecutionService {
         shell: false,
         detached: !isWindows,
         env: {
-          ...process.env,
+          ...getSanitizedEnv(),
           GEMINI_CLI: '1',
           TERM: 'xterm-256color',
           PAGER: 'cat',
@@ -463,7 +478,7 @@ export class ShellExecutionService {
         cols,
         rows,
         env: {
-          ...process.env,
+          ...getSanitizedEnv(),
           GEMINI_CLI: '1',
           TERM: 'xterm-256color',
           PAGER: shellExecutionConfig.pager ?? 'cat',