From 2384eb67273a46d77e3af0cb100ecfdaa6a9c57d Mon Sep 17 00:00:00 2001 From: Coco Sheng Date: Wed, 6 May 2026 10:37:56 -0400 Subject: [PATCH 1/9] fix(core): preserve system PATH in Git environment to fix ENOENT (#25034) --- packages/core/src/services/gitService.test.ts | 51 +++++++++++++++++++ packages/core/src/services/gitService.ts | 1 + 2 files changed, 52 insertions(+) diff --git a/packages/core/src/services/gitService.test.ts b/packages/core/src/services/gitService.test.ts index f5213ac6ea4..f7000019a3a 100644 --- a/packages/core/src/services/gitService.test.ts +++ b/packages/core/src/services/gitService.test.ts @@ -304,6 +304,57 @@ describe('GitService', () => { ); expect(systemConfigContent).toBe(''); }); + + it('should preserve system PATH and other env vars in the Git environment', async () => { + const customPath = '/custom/bin'; + vi.stubEnv('PATH', customPath); + vi.stubEnv('OTHER_VAR', 'other-value'); + + try { + hoistedMockCheckIsRepo.mockResolvedValue(false); + const service = new GitService(projectRoot, storage); + await service.setupShadowGitRepository(); + + expect(hoistedMockEnv).toHaveBeenCalledWith( + expect.objectContaining({ + PATH: customPath, + OTHER_VAR: 'other-value', + GIT_CONFIG_GLOBAL: expect.any(String), + GIT_AUTHOR_NAME: SHADOW_REPO_AUTHOR_NAME, + }), + ); + } finally { + vi.unstubAllEnvs(); + } + }); + + it('should override GIT_CONFIG environment variables from process.env', async () => { + vi.stubEnv('GIT_CONFIG_GLOBAL', '/user/global/config'); + vi.stubEnv('GIT_CONFIG_SYSTEM', '/user/system/config'); + + try { + hoistedMockCheckIsRepo.mockResolvedValue(false); + const service = new GitService(projectRoot, storage); + await service.setupShadowGitRepository(); + + const expectedConfigPath = path.join(repoDir, '.gitconfig'); + const expectedSystemPath = path.join(repoDir, '.gitconfig_system_empty'); + + expect(hoistedMockEnv).toHaveBeenCalledWith( + expect.objectContaining({ + GIT_CONFIG_GLOBAL: expectedConfigPath, + GIT_CONFIG_SYSTEM: expectedSystemPath, + }), + ); + + // Ensure it's not using the values from stubbed process.env + const callArgs = hoistedMockEnv.mock.calls[0][0]; + expect(callArgs.GIT_CONFIG_GLOBAL).not.toBe('/user/global/config'); + expect(callArgs.GIT_CONFIG_SYSTEM).not.toBe('/user/system/config'); + } finally { + vi.unstubAllEnvs(); + } + }); }); describe('createFileSnapshot', () => { diff --git a/packages/core/src/services/gitService.ts b/packages/core/src/services/gitService.ts index f923dc61648..2f92fa2ec43 100644 --- a/packages/core/src/services/gitService.ts +++ b/packages/core/src/services/gitService.ts @@ -58,6 +58,7 @@ export class GitService { const gitConfigPath = path.join(repoDir, '.gitconfig'); const systemConfigPath = path.join(repoDir, '.gitconfig_system_empty'); return { + ...process.env, // Prevent git from using the user's global git config. GIT_CONFIG_GLOBAL: gitConfigPath, GIT_CONFIG_SYSTEM: systemConfigPath, From d91a3b58f35fb987de6ddeaa64ada705c1333ce5 Mon Sep 17 00:00:00 2001 From: Coco Sheng Date: Wed, 6 May 2026 10:53:56 -0400 Subject: [PATCH 2/9] chore: fix formatting in gitService.test.ts --- packages/core/src/services/gitService.test.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/core/src/services/gitService.test.ts b/packages/core/src/services/gitService.test.ts index f7000019a3a..29090053898 100644 --- a/packages/core/src/services/gitService.test.ts +++ b/packages/core/src/services/gitService.test.ts @@ -338,7 +338,10 @@ describe('GitService', () => { await service.setupShadowGitRepository(); const expectedConfigPath = path.join(repoDir, '.gitconfig'); - const expectedSystemPath = path.join(repoDir, '.gitconfig_system_empty'); + const expectedSystemPath = path.join( + repoDir, + '.gitconfig_system_empty', + ); expect(hoistedMockEnv).toHaveBeenCalledWith( expect.objectContaining({ From dac11730a92ac65f74bb287acab506a9f4aeea7d Mon Sep 17 00:00:00 2001 From: Coco Sheng Date: Wed, 6 May 2026 12:19:13 -0400 Subject: [PATCH 3/9] refactor(core): sanitize git environment and align tests with style guide --- packages/core/src/services/gitService.test.ts | 49 +++++++++++++------ packages/core/src/services/gitService.ts | 2 +- 2 files changed, 34 insertions(+), 17 deletions(-) diff --git a/packages/core/src/services/gitService.test.ts b/packages/core/src/services/gitService.test.ts index 29090053898..d1138891900 100644 --- a/packages/core/src/services/gitService.test.ts +++ b/packages/core/src/services/gitService.test.ts @@ -305,35 +305,54 @@ describe('GitService', () => { expect(systemConfigContent).toBe(''); }); - it('should preserve system PATH and other env vars in the Git environment', async () => { + describe('environment variable preservation', () => { const customPath = '/custom/bin'; - vi.stubEnv('PATH', customPath); - vi.stubEnv('OTHER_VAR', 'other-value'); + const otherVar = 'other-value'; - try { + beforeEach(() => { + vi.stubEnv('PATH', customPath); + vi.stubEnv('OTHER_VAR', otherVar); hoistedMockCheckIsRepo.mockResolvedValue(false); + }); + + afterEach(() => { + vi.unstubAllEnvs(); + }); + + it('should preserve system PATH in the Git environment', async () => { const service = new GitService(projectRoot, storage); await service.setupShadowGitRepository(); expect(hoistedMockEnv).toHaveBeenCalledWith( expect.objectContaining({ PATH: customPath, - OTHER_VAR: 'other-value', GIT_CONFIG_GLOBAL: expect.any(String), GIT_AUTHOR_NAME: SHADOW_REPO_AUTHOR_NAME, }), ); - } finally { - vi.unstubAllEnvs(); - } - }); + }); - it('should override GIT_CONFIG environment variables from process.env', async () => { - vi.stubEnv('GIT_CONFIG_GLOBAL', '/user/global/config'); - vi.stubEnv('GIT_CONFIG_SYSTEM', '/user/system/config'); + it('should NOT include unrelated environment variables in the Git environment', async () => { + const service = new GitService(projectRoot, storage); + await service.setupShadowGitRepository(); - try { + const callArgs = hoistedMockEnv.mock.calls[0][0]; + expect(callArgs.OTHER_VAR).toBeUndefined(); + }); + }); + + describe('GIT_CONFIG isolation', () => { + beforeEach(() => { + vi.stubEnv('GIT_CONFIG_GLOBAL', '/user/global/config'); + vi.stubEnv('GIT_CONFIG_SYSTEM', '/user/system/config'); hoistedMockCheckIsRepo.mockResolvedValue(false); + }); + + afterEach(() => { + vi.unstubAllEnvs(); + }); + + it('should override GIT_CONFIG environment variables from process.env', async () => { const service = new GitService(projectRoot, storage); await service.setupShadowGitRepository(); @@ -354,9 +373,7 @@ describe('GitService', () => { const callArgs = hoistedMockEnv.mock.calls[0][0]; expect(callArgs.GIT_CONFIG_GLOBAL).not.toBe('/user/global/config'); expect(callArgs.GIT_CONFIG_SYSTEM).not.toBe('/user/system/config'); - } finally { - vi.unstubAllEnvs(); - } + }); }); }); diff --git a/packages/core/src/services/gitService.ts b/packages/core/src/services/gitService.ts index 2f92fa2ec43..db18deedd10 100644 --- a/packages/core/src/services/gitService.ts +++ b/packages/core/src/services/gitService.ts @@ -58,7 +58,7 @@ export class GitService { const gitConfigPath = path.join(repoDir, '.gitconfig'); const systemConfigPath = path.join(repoDir, '.gitconfig_system_empty'); return { - ...process.env, + PATH: process.env.PATH, // Prevent git from using the user's global git config. GIT_CONFIG_GLOBAL: gitConfigPath, GIT_CONFIG_SYSTEM: systemConfigPath, From a6c677b46f4ab5894d2e96490c33b35e303a41a3 Mon Sep 17 00:00:00 2001 From: Coco Sheng Date: Wed, 6 May 2026 12:24:02 -0400 Subject: [PATCH 4/9] fix(core): use index access for process.env['PATH'] to satisfy TS4111 --- packages/core/src/services/gitService.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/services/gitService.ts b/packages/core/src/services/gitService.ts index db18deedd10..ec4dcc31c64 100644 --- a/packages/core/src/services/gitService.ts +++ b/packages/core/src/services/gitService.ts @@ -58,7 +58,7 @@ export class GitService { const gitConfigPath = path.join(repoDir, '.gitconfig'); const systemConfigPath = path.join(repoDir, '.gitconfig_system_empty'); return { - PATH: process.env.PATH, + PATH: process.env['PATH'], // Prevent git from using the user's global git config. GIT_CONFIG_GLOBAL: gitConfigPath, GIT_CONFIG_SYSTEM: systemConfigPath, From dd2e2ccfedcfb1dfb1c002bd9b569ea802b13204 Mon Sep 17 00:00:00 2001 From: Coco Sheng Date: Wed, 6 May 2026 12:37:01 -0400 Subject: [PATCH 5/9] refactor(core): use sanitized environment for shadow git repository --- packages/core/src/services/gitService.test.ts | 64 +++++++++++++++++-- packages/core/src/services/gitService.ts | 11 +++- 2 files changed, 69 insertions(+), 6 deletions(-) diff --git a/packages/core/src/services/gitService.test.ts b/packages/core/src/services/gitService.test.ts index d1138891900..6ec4c3ab99e 100644 --- a/packages/core/src/services/gitService.test.ts +++ b/packages/core/src/services/gitService.test.ts @@ -307,11 +307,14 @@ describe('GitService', () => { describe('environment variable preservation', () => { const customPath = '/custom/bin'; - const otherVar = 'other-value'; + const safeHome = '/home/user'; + const sensitiveKey = 'sk-123456789'; beforeEach(() => { vi.stubEnv('PATH', customPath); - vi.stubEnv('OTHER_VAR', otherVar); + vi.stubEnv('HOME', safeHome); + vi.stubEnv('API_KEY', sensitiveKey); + vi.stubEnv('UNRELATED_VAR', 'some-value'); hoistedMockCheckIsRepo.mockResolvedValue(false); }); @@ -332,12 +335,31 @@ describe('GitService', () => { ); }); - it('should NOT include unrelated environment variables in the Git environment', async () => { + it('should preserve safe environment variables like HOME', async () => { + const service = new GitService(projectRoot, storage); + await service.setupShadowGitRepository(); + + expect(hoistedMockEnv).toHaveBeenCalledWith( + expect.objectContaining({ + HOME: safeHome, + }), + ); + }); + + it('should NOT include sensitive environment variables like API_KEY', async () => { const service = new GitService(projectRoot, storage); await service.setupShadowGitRepository(); const callArgs = hoistedMockEnv.mock.calls[0][0]; - expect(callArgs.OTHER_VAR).toBeUndefined(); + expect(callArgs.API_KEY).toBeUndefined(); + }); + + it('should preserve unrelated environment variables (non-strict mode)', async () => { + const service = new GitService(projectRoot, storage); + await service.setupShadowGitRepository(); + + const callArgs = hoistedMockEnv.mock.calls[0][0]; + expect(callArgs.UNRELATED_VAR).toBe('some-value'); }); }); @@ -375,6 +397,40 @@ describe('GitService', () => { expect(callArgs.GIT_CONFIG_SYSTEM).not.toBe('/user/system/config'); }); }); + + describe('shadowGitRepository prioritization', () => { + beforeEach(() => { + vi.stubEnv('GIT_DIR', '/user/fake/.git'); + vi.stubEnv('GIT_WORK_TREE', '/user/fake/worktree'); + hoistedMockCheckIsRepo.mockResolvedValue(true); + }); + + afterEach(() => { + vi.unstubAllEnvs(); + }); + + it('should prioritize internal GIT_DIR and GIT_WORK_TREE over process.env', async () => { + const service = new GitService(projectRoot, storage); + // Trigger a call to shadowGitRepository (e.g., via getCurrentCommitHash) + hoistedMockRaw.mockResolvedValue('hash'); + await service.getCurrentCommitHash(); + + const expectedRepoDir = storage.getHistoryDir(); + const expectedGitDir = path.join(expectedRepoDir, '.git'); + + expect(hoistedMockEnv).toHaveBeenCalledWith( + expect.objectContaining({ + GIT_DIR: expectedGitDir, + GIT_WORK_TREE: projectRoot, + }), + ); + + // Ensure user env was overridden + const callArgs = hoistedMockEnv.mock.calls[0][0]; + expect(callArgs.GIT_DIR).not.toBe('/user/fake/.git'); + expect(callArgs.GIT_WORK_TREE).not.toBe('/user/fake/worktree'); + }); + }); }); describe('createFileSnapshot', () => { diff --git a/packages/core/src/services/gitService.ts b/packages/core/src/services/gitService.ts index ec4dcc31c64..3a174a02c63 100644 --- a/packages/core/src/services/gitService.ts +++ b/packages/core/src/services/gitService.ts @@ -11,6 +11,10 @@ import { spawnAsync } from '../utils/shell-utils.js'; import { simpleGit, CheckRepoActions, type SimpleGit } from 'simple-git'; import type { Storage } from '../config/storage.js'; import { debugLogger } from '../utils/debugLogger.js'; +import { + sanitizeEnvironment, + getSecureSanitizationConfig, +} from './environmentSanitization.js'; export const SHADOW_REPO_AUTHOR_NAME = 'Gemini CLI'; export const SHADOW_REPO_AUTHOR_EMAIL = 'gemini-cli@google.com'; @@ -58,7 +62,10 @@ export class GitService { const gitConfigPath = path.join(repoDir, '.gitconfig'); const systemConfigPath = path.join(repoDir, '.gitconfig_system_empty'); return { - PATH: process.env['PATH'], + ...sanitizeEnvironment( + process.env, + getSecureSanitizationConfig({ enableEnvironmentVariableRedaction: true }), + ), // Prevent git from using the user's global git config. GIT_CONFIG_GLOBAL: gitConfigPath, GIT_CONFIG_SYSTEM: systemConfigPath, @@ -127,9 +134,9 @@ export class GitService { private get shadowGitRepository(): SimpleGit { const repoDir = this.getHistoryDir(); return simpleGit(this.projectRoot).env({ + ...this.getShadowRepoEnv(repoDir), GIT_DIR: path.join(repoDir, '.git'), GIT_WORK_TREE: this.projectRoot, - ...this.getShadowRepoEnv(repoDir), }); } From 1b59b201fc08c51cd815abf2b3683e6f8d86457e Mon Sep 17 00:00:00 2001 From: Coco Sheng Date: Wed, 6 May 2026 12:45:13 -0400 Subject: [PATCH 6/9] refactor(core): harden git isolation and make environment tests robust --- packages/core/src/services/gitService.test.ts | 15 ++++++++++++++- packages/core/src/services/gitService.ts | 3 +++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/packages/core/src/services/gitService.test.ts b/packages/core/src/services/gitService.test.ts index 6ec4c3ab99e..cc58d398938 100644 --- a/packages/core/src/services/gitService.test.ts +++ b/packages/core/src/services/gitService.test.ts @@ -315,6 +315,10 @@ describe('GitService', () => { vi.stubEnv('HOME', safeHome); vi.stubEnv('API_KEY', sensitiveKey); vi.stubEnv('UNRELATED_VAR', 'some-value'); + // Explicitly unset strict mode triggers to ensure predictable test behavior + // across local and CI environments. + vi.stubEnv('GITHUB_SHA', ''); + vi.stubEnv('SURFACE', ''); hoistedMockCheckIsRepo.mockResolvedValue(false); }); @@ -354,13 +358,22 @@ describe('GitService', () => { expect(callArgs.API_KEY).toBeUndefined(); }); - it('should preserve unrelated environment variables (non-strict mode)', async () => { + it('should preserve unrelated environment variables in non-strict mode', async () => { const service = new GitService(projectRoot, storage); await service.setupShadowGitRepository(); const callArgs = hoistedMockEnv.mock.calls[0][0]; expect(callArgs.UNRELATED_VAR).toBe('some-value'); }); + + it('should explicitly unset GIT_DIR and GIT_WORK_TREE to maintain isolation', async () => { + const service = new GitService(projectRoot, storage); + await service.setupShadowGitRepository(); + + const callArgs = hoistedMockEnv.mock.calls[0][0]; + expect(callArgs.GIT_DIR).toBeUndefined(); + expect(callArgs.GIT_WORK_TREE).toBeUndefined(); + }); }); describe('GIT_CONFIG isolation', () => { diff --git a/packages/core/src/services/gitService.ts b/packages/core/src/services/gitService.ts index 3a174a02c63..c78f7a9b7a2 100644 --- a/packages/core/src/services/gitService.ts +++ b/packages/core/src/services/gitService.ts @@ -69,6 +69,9 @@ export class GitService { // Prevent git from using the user's global git config. GIT_CONFIG_GLOBAL: gitConfigPath, GIT_CONFIG_SYSTEM: systemConfigPath, + // Ensure we don't inherit isolation-breaking variables from the user environment. + GIT_DIR: undefined, + GIT_WORK_TREE: undefined, // Explicitly provide identity to prevent "Author identity unknown" errors // inside sandboxed environments like Docker where the gitconfig might not // be picked up properly. From c81a64d8403075cad16775b5c42bdc7f86ed4550 Mon Sep 17 00:00:00 2001 From: Coco Sheng Date: Wed, 6 May 2026 12:45:49 -0400 Subject: [PATCH 7/9] chore: fix formatting in gitService.ts --- packages/core/src/services/gitService.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/core/src/services/gitService.ts b/packages/core/src/services/gitService.ts index c78f7a9b7a2..c32a06130c8 100644 --- a/packages/core/src/services/gitService.ts +++ b/packages/core/src/services/gitService.ts @@ -64,7 +64,9 @@ export class GitService { return { ...sanitizeEnvironment( process.env, - getSecureSanitizationConfig({ enableEnvironmentVariableRedaction: true }), + getSecureSanitizationConfig({ + enableEnvironmentVariableRedaction: true, + }), ), // Prevent git from using the user's global git config. GIT_CONFIG_GLOBAL: gitConfigPath, From db0b613ce5d5c112bbcc82bb18320883bc037541 Mon Sep 17 00:00:00 2001 From: Coco Sheng Date: Wed, 6 May 2026 13:53:49 -0400 Subject: [PATCH 8/9] test: improve reliability of background tools integration test on Windows --- .../core/src/tools/shellBackgroundTools.integration.test.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/core/src/tools/shellBackgroundTools.integration.test.ts b/packages/core/src/tools/shellBackgroundTools.integration.test.ts index ab96df73831..99b233a1987 100644 --- a/packages/core/src/tools/shellBackgroundTools.integration.test.ts +++ b/packages/core/src/tools/shellBackgroundTools.integration.test.ts @@ -53,7 +53,7 @@ describe('Background Tools Integration', () => { const scriptPath = path.join(tempRootDir, 'log.js'); fs.writeFileSync( scriptPath, - "setInterval(() => console.log('Log line'), 100);", + "console.log('Log line'); setInterval(() => console.log('Log line'), 100);", ); // Using 'node' directly avoids cross-platform shell quoting issues with absolute paths. @@ -101,7 +101,8 @@ describe('Background Tools Integration', () => { ); // 4. Give it time to write output to interval - await new Promise((resolve) => setTimeout(resolve, 2000)); + // 5 seconds is safer for slow CI environments like Windows + await new Promise((resolve) => setTimeout(resolve, 5000)); // 5. Model decides to read logs const readInvocation = readTool.build({ pid, lines: 2 }); From fa86222e729914fe7b699920b1885dfba5b25552 Mon Sep 17 00:00:00 2001 From: Coco Sheng Date: Wed, 6 May 2026 15:11:33 -0400 Subject: [PATCH 9/9] Revert "test: improve reliability of background tools integration test on Windows" This reverts commit db0b613ce5d5c112bbcc82bb18320883bc037541. --- .../core/src/tools/shellBackgroundTools.integration.test.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/core/src/tools/shellBackgroundTools.integration.test.ts b/packages/core/src/tools/shellBackgroundTools.integration.test.ts index 99b233a1987..ab96df73831 100644 --- a/packages/core/src/tools/shellBackgroundTools.integration.test.ts +++ b/packages/core/src/tools/shellBackgroundTools.integration.test.ts @@ -53,7 +53,7 @@ describe('Background Tools Integration', () => { const scriptPath = path.join(tempRootDir, 'log.js'); fs.writeFileSync( scriptPath, - "console.log('Log line'); setInterval(() => console.log('Log line'), 100);", + "setInterval(() => console.log('Log line'), 100);", ); // Using 'node' directly avoids cross-platform shell quoting issues with absolute paths. @@ -101,8 +101,7 @@ describe('Background Tools Integration', () => { ); // 4. Give it time to write output to interval - // 5 seconds is safer for slow CI environments like Windows - await new Promise((resolve) => setTimeout(resolve, 5000)); + await new Promise((resolve) => setTimeout(resolve, 2000)); // 5. Model decides to read logs const readInvocation = readTool.build({ pid, lines: 2 });