From 451edb3ea68d412a6f818efd52f6904e35c24a62 Mon Sep 17 00:00:00 2001 From: Emily Hedlund Date: Thu, 9 Apr 2026 15:04:16 -0700 Subject: [PATCH 1/4] fix(sandbox): centralize async git worktree resolution and enforce read-only security (#25040) --- .../sandbox/linux/bwrapArgsBuilder.test.ts | 57 ++++++++ .../src/sandbox/linux/bwrapArgsBuilder.ts | 24 ++-- .../sandbox/macos/seatbeltArgsBuilder.test.ts | 57 ++++++++ .../src/sandbox/macos/seatbeltArgsBuilder.ts | 40 ++++-- .../core/src/sandbox/utils/fsUtils.test.ts | 130 +++++++++++++----- packages/core/src/sandbox/utils/fsUtils.ts | 43 ++---- .../windows/WindowsSandboxManager.test.ts | 59 +++++++- .../sandbox/windows/WindowsSandboxManager.ts | 10 +- .../sandboxManager.integration.test.ts | 96 +++++++++++++ .../core/src/services/sandboxManager.test.ts | 114 ++------------- packages/core/src/services/sandboxManager.ts | 37 +++-- 11 files changed, 459 insertions(+), 208 deletions(-) diff --git a/packages/core/src/sandbox/linux/bwrapArgsBuilder.test.ts b/packages/core/src/sandbox/linux/bwrapArgsBuilder.test.ts index 24612bbb09..b9584062bc 100644 --- a/packages/core/src/sandbox/linux/bwrapArgsBuilder.test.ts +++ b/packages/core/src/sandbox/linux/bwrapArgsBuilder.test.ts @@ -339,4 +339,61 @@ describe.skipIf(os.platform() === 'win32')('buildBwrapArgs', () => { const envIndex = args.indexOf(`${includeDir}/.env`); expect(args[envIndex - 2]).toBe('--bind'); }); + + it('binds git worktree directories if present', async () => { + const worktreeGitDir = '/path/to/worktree/.git'; + const mainGitDir = '/path/to/main/.git'; + + const args = await buildBwrapArgs({ + ...defaultOptions, + resolvedPaths: createResolvedPaths({ + gitWorktree: { + worktreeGitDir, + mainGitDir, + }, + }), + }); + + expect(args).toContain(worktreeGitDir); + expect(args).toContain(mainGitDir); + expect(args[args.indexOf(worktreeGitDir) - 1]).toBe('--ro-bind-try'); + expect(args[args.indexOf(mainGitDir) - 1]).toBe('--ro-bind-try'); + }); + + it('enforces read-only binding for git worktrees even if workspaceWrite is true', async () => { + const worktreeGitDir = '/path/to/worktree/.git'; + + const args = await buildBwrapArgs({ + ...defaultOptions, + workspaceWrite: true, + resolvedPaths: createResolvedPaths({ + gitWorktree: { + worktreeGitDir, + }, + }), + }); + + expect(args[args.indexOf(worktreeGitDir) - 1]).toBe('--ro-bind-try'); + }); + + it('git worktree read-only bindings should override previous policyWrite bindings', async () => { + const worktreeGitDir = '/custom/worktree/.git'; + + const args = await buildBwrapArgs({ + ...defaultOptions, + resolvedPaths: createResolvedPaths({ + policyWrite: ['/custom/worktree'], + gitWorktree: { + worktreeGitDir, + }, + }), + }); + + const writeBindIndex = args.indexOf('/custom/worktree'); + const worktreeBindIndex = args.lastIndexOf(worktreeGitDir); + + expect(writeBindIndex).toBeGreaterThan(-1); + expect(worktreeBindIndex).toBeGreaterThan(-1); + expect(worktreeBindIndex).toBeGreaterThan(writeBindIndex); + }); }); diff --git a/packages/core/src/sandbox/linux/bwrapArgsBuilder.ts b/packages/core/src/sandbox/linux/bwrapArgsBuilder.ts index d7172b648e..d7fec044e3 100644 --- a/packages/core/src/sandbox/linux/bwrapArgsBuilder.ts +++ b/packages/core/src/sandbox/linux/bwrapArgsBuilder.ts @@ -11,7 +11,7 @@ import { getSecretFileFindArgs, type ResolvedSandboxPaths, } from '../../services/sandboxManager.js'; -import { resolveGitWorktreePaths, isErrnoException } from '../utils/fsUtils.js'; +import { isErrnoException } from '../utils/fsUtils.js'; import { spawnAsync } from '../../utils/shell-utils.js'; import { debugLogger } from '../../utils/debugLogger.js'; @@ -70,16 +70,6 @@ export async function buildBwrapArgs( bwrapArgs.push(bindFlag, workspace.resolved, workspace.resolved); } - const { worktreeGitDir, mainGitDir } = resolveGitWorktreePaths( - workspace.resolved, - ); - if (worktreeGitDir) { - bwrapArgs.push(bindFlag, worktreeGitDir, worktreeGitDir); - } - if (mainGitDir) { - bwrapArgs.push(bindFlag, mainGitDir, mainGitDir); - } - for (const includeDir of resolvedPaths.globalIncludes) { bwrapArgs.push('--ro-bind-try', includeDir, includeDir); } @@ -113,6 +103,18 @@ export async function buildBwrapArgs( } } + // Grant read-only access to git worktrees/submodules. We do this last in order to + // ensure that these rules aren't overwritten by broader write policies. + if (resolvedPaths.gitWorktree) { + const { worktreeGitDir, mainGitDir } = resolvedPaths.gitWorktree; + if (worktreeGitDir) { + bwrapArgs.push('--ro-bind-try', worktreeGitDir, worktreeGitDir); + } + if (mainGitDir) { + bwrapArgs.push('--ro-bind-try', mainGitDir, mainGitDir); + } + } + for (const p of resolvedPaths.forbidden) { if (!fs.existsSync(p)) continue; try { diff --git a/packages/core/src/sandbox/macos/seatbeltArgsBuilder.test.ts b/packages/core/src/sandbox/macos/seatbeltArgsBuilder.test.ts index 19ba8303ae..e8801b055b 100644 --- a/packages/core/src/sandbox/macos/seatbeltArgsBuilder.test.ts +++ b/packages/core/src/sandbox/macos/seatbeltArgsBuilder.test.ts @@ -142,5 +142,62 @@ describe.skipIf(os.platform() === 'win32')('seatbeltArgsBuilder', () => { expect(denyIndex).toBeGreaterThan(allowIndex); }); }); + + describe('git worktree paths', () => { + it('enforces read-only binding for git worktrees even if workspaceWrite is true', () => { + const worktreeGitDir = '/path/to/worktree/.git'; + const mainGitDir = '/path/to/main/.git'; + + const profile = buildSeatbeltProfile({ + resolvedPaths: { + ...defaultResolvedPaths, + gitWorktree: { + worktreeGitDir, + mainGitDir, + }, + }, + workspaceWrite: true, + }); + + // Should grant read access + expect(profile).toContain( + `(allow file-read* (subpath "${worktreeGitDir}"))`, + ); + expect(profile).toContain( + `(allow file-read* (subpath "${mainGitDir}"))`, + ); + + // Should NOT grant write access + expect(profile).not.toContain( + `(allow file-read* file-write* (subpath "${worktreeGitDir}"))`, + ); + expect(profile).not.toContain( + `(allow file-read* file-write* (subpath "${mainGitDir}"))`, + ); + }); + + it('git worktree read-only rules should override previous policyAllowed write paths', () => { + const worktreeGitDir = '/custom/worktree/.git'; + const profile = buildSeatbeltProfile({ + resolvedPaths: { + ...defaultResolvedPaths, + policyAllowed: ['/custom/worktree'], + gitWorktree: { + worktreeGitDir, + }, + }, + }); + + const allowString = `(allow file-read* file-write* (subpath "/custom/worktree"))`; + const denyString = `(deny file-write* (subpath "${worktreeGitDir}"))`; + + expect(profile).toContain(allowString); + expect(profile).toContain(denyString); + + const allowIndex = profile.indexOf(allowString); + const denyIndex = profile.indexOf(denyString); + expect(denyIndex).toBeGreaterThan(allowIndex); + }); + }); }); }); diff --git a/packages/core/src/sandbox/macos/seatbeltArgsBuilder.ts b/packages/core/src/sandbox/macos/seatbeltArgsBuilder.ts index 967cd8f183..abbf1a6d92 100644 --- a/packages/core/src/sandbox/macos/seatbeltArgsBuilder.ts +++ b/packages/core/src/sandbox/macos/seatbeltArgsBuilder.ts @@ -16,7 +16,7 @@ import { SECRET_FILES, type ResolvedSandboxPaths, } from '../../services/sandboxManager.js'; -import { tryRealpath, resolveGitWorktreePaths } from '../utils/fsUtils.js'; +import { resolveToRealPath } from '../../utils/paths.js'; /** * Options for building macOS Seatbelt profile. @@ -52,21 +52,21 @@ export function buildSeatbeltProfile(options: SeatbeltArgsOptions): string { profile += `(allow file-write* (subpath "${escapeSchemeString(resolvedPaths.workspace.resolved)}"))\n`; } - const tmpPath = tryRealpath(os.tmpdir()); + const tmpPath = resolveToRealPath(os.tmpdir()); profile += `(allow file-read* file-write* (subpath "${escapeSchemeString(tmpPath)}"))\n`; - // Auto-detect and support git worktrees by granting read and write access to the underlying git directory - const { worktreeGitDir, mainGitDir } = resolveGitWorktreePaths( - resolvedPaths.workspace.resolved, - ); - if (worktreeGitDir) { - profile += `(allow file-read* file-write* (subpath "${escapeSchemeString(worktreeGitDir)}"))\n`; - } - if (mainGitDir) { - profile += `(allow file-read* file-write* (subpath "${escapeSchemeString(mainGitDir)}"))\n`; + // Support git worktrees/submodules; read-only to prevent malicious hook/config modification (RCE). + if (resolvedPaths.gitWorktree) { + const { worktreeGitDir, mainGitDir } = resolvedPaths.gitWorktree; + if (worktreeGitDir) { + profile += `(allow file-read* (subpath "${escapeSchemeString(worktreeGitDir)}"))\n`; + } + if (mainGitDir) { + profile += `(allow file-read* (subpath "${escapeSchemeString(mainGitDir)}"))\n`; + } } - const nodeRootPath = tryRealpath( + const nodeRootPath = resolveToRealPath( path.dirname(path.dirname(process.execPath)), ); profile += `(allow file-read* (subpath "${escapeSchemeString(nodeRootPath)}"))\n`; @@ -79,7 +79,7 @@ export function buildSeatbeltProfile(options: SeatbeltArgsOptions): string { for (const p of paths) { if (!p.trim()) continue; try { - let resolved = tryRealpath(p); + let resolved = resolveToRealPath(p); // If this is a 'bin' directory (like /usr/local/bin or homebrew/bin), // also grant read access to its parent directory so that symlinked @@ -148,7 +148,7 @@ export function buildSeatbeltProfile(options: SeatbeltArgsOptions): string { resolvedPaths.workspace.resolved, GOVERNANCE_FILES[i].path, ); - const realGovernanceFile = tryRealpath(governanceFile); + const realGovernanceFile = resolveToRealPath(governanceFile); // Determine if it should be treated as a directory (subpath) or a file (literal). // .git is generally a directory, while ignore files are literals. @@ -170,6 +170,18 @@ export function buildSeatbeltProfile(options: SeatbeltArgsOptions): string { } } + // Grant read-only access to git worktrees/submodules. We do this last in order to + // ensure that these rules aren't overwritten by broader write policies. + if (resolvedPaths.gitWorktree) { + const { worktreeGitDir, mainGitDir } = resolvedPaths.gitWorktree; + if (worktreeGitDir) { + profile += `(deny file-write* (subpath "${escapeSchemeString(worktreeGitDir)}"))\n`; + } + if (mainGitDir) { + profile += `(deny file-write* (subpath "${escapeSchemeString(mainGitDir)}"))\n`; + } + } + // Add explicit deny rules for secret files (.env, .env.*) in the workspace and allowed paths. // We use regex rules to avoid expensive file discovery scans. // Anchoring to workspace/allowed paths to avoid over-blocking. diff --git a/packages/core/src/sandbox/utils/fsUtils.test.ts b/packages/core/src/sandbox/utils/fsUtils.test.ts index 9439050680..460fb9d26b 100644 --- a/packages/core/src/sandbox/utils/fsUtils.test.ts +++ b/packages/core/src/sandbox/utils/fsUtils.test.ts @@ -4,49 +4,117 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect, beforeAll, afterAll } from 'vitest'; -import fs from 'node:fs'; +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import fsPromises from 'node:fs/promises'; import path from 'node:path'; -import os from 'node:os'; -import { tryRealpath } from './fsUtils.js'; +import { resolveGitWorktreePaths } from './fsUtils.js'; + +vi.mock('node:fs/promises', async () => { + const actual = + await vi.importActual( + 'node:fs/promises', + ); + return { + ...actual, + default: { + ...actual, + lstat: vi.fn(), + readFile: vi.fn(), + }, + lstat: vi.fn(), + readFile: vi.fn(), + }; +}); + +vi.mock('../../utils/paths.js', async () => { + const actual = await vi.importActual( + '../../utils/paths.js', + ); + return { + ...actual, + resolveToRealPath: vi.fn((p) => p), + }; +}); describe('fsUtils', () => { - let tempDir: string; - let realTempDir: string; + describe('resolveGitWorktreePaths', () => { + const workspace = path.resolve('/workspace'); + const gitPath = path.join(workspace, '.git'); - beforeAll(() => { - tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'fs-utils-test-')); - realTempDir = fs.realpathSync(tempDir); - }); + beforeEach(() => { + vi.clearAllMocks(); + }); - afterAll(() => { - fs.rmSync(tempDir, { recursive: true, force: true }); - }); - - describe('tryRealpath', () => { - it('should throw error for paths with null bytes', () => { - expect(() => tryRealpath(path.join(tempDir, 'foo\0bar'))).toThrow( - 'Invalid path', + it('should return empty if .git does not exist', async () => { + vi.mocked(fsPromises.lstat).mockRejectedValue( + Object.assign(new Error('ENOENT'), { code: 'ENOENT' }) as never, ); + const result = await resolveGitWorktreePaths(workspace); + expect(result).toEqual({}); }); - it('should resolve existing paths', () => { - const resolved = tryRealpath(tempDir); - expect(resolved).toBe(realTempDir); + it('should return empty if .git is a directory', async () => { + vi.mocked(fsPromises.lstat).mockResolvedValue({ + isFile: () => false, + } as never); + const result = await resolveGitWorktreePaths(workspace); + expect(result).toEqual({}); }); - it('should handle non-existent paths by resolving parent', () => { - const nonExistentPath = path.join(tempDir, 'non-existent-file-12345'); - const expected = path.join(realTempDir, 'non-existent-file-12345'); - const resolved = tryRealpath(nonExistentPath); - expect(resolved).toBe(expected); + it('should resolve worktree paths from .git file', async () => { + const mainGitDir = path.resolve('/project/.git'); + const worktreeGitDir = path.join(mainGitDir, 'worktrees', 'feature'); + + vi.mocked(fsPromises.lstat).mockResolvedValue({ + isFile: () => true, + } as never); + vi.mocked(fsPromises.readFile).mockImplementation(((p: string) => { + if (p === gitPath) return Promise.resolve(`gitdir: ${worktreeGitDir}`); + if (p === path.join(worktreeGitDir, 'gitdir')) + return Promise.resolve(gitPath); + return Promise.reject(new Error('ENOENT')); + }) as never); + + const result = await resolveGitWorktreePaths(workspace); + expect(result).toEqual({ + worktreeGitDir, + mainGitDir, + }); }); - it('should handle nested non-existent paths', () => { - const nonExistentPath = path.join(tempDir, 'dir1', 'dir2', 'file'); - const expected = path.join(realTempDir, 'dir1', 'dir2', 'file'); - const resolved = tryRealpath(nonExistentPath); - expect(resolved).toBe(expected); + it('should reject worktree if backlink is missing or invalid', async () => { + const worktreeGitDir = path.resolve('/git/worktrees/feature'); + + vi.mocked(fsPromises.lstat).mockResolvedValue({ + isFile: () => true, + } as never); + vi.mocked(fsPromises.readFile).mockImplementation(((p: string) => { + if (p === gitPath) return Promise.resolve(`gitdir: ${worktreeGitDir}`); + return Promise.reject(new Error('ENOENT')); + }) as never); + + const result = await resolveGitWorktreePaths(workspace); + expect(result).toEqual({}); + }); + + it('should support submodules via config check', async () => { + const submoduleGitDir = path.resolve('/project/.git/modules/sub'); + + vi.mocked(fsPromises.lstat).mockResolvedValue({ + isFile: () => true, + } as never); + vi.mocked(fsPromises.readFile).mockImplementation(((p: string) => { + if (p === gitPath) return Promise.resolve(`gitdir: ${submoduleGitDir}`); + if (p === path.join(submoduleGitDir, 'config')) + return Promise.resolve(`[core]\n\tworktree = ${workspace}`); + return Promise.reject(new Error('ENOENT')); + }) as never); + + const result = await resolveGitWorktreePaths(workspace); + expect(result).toEqual({ + worktreeGitDir: submoduleGitDir, + mainGitDir: path.resolve('/project/.git'), + }); }); }); }); diff --git a/packages/core/src/sandbox/utils/fsUtils.ts b/packages/core/src/sandbox/utils/fsUtils.ts index 2e3eda1342..c9729caf26 100644 --- a/packages/core/src/sandbox/utils/fsUtils.ts +++ b/packages/core/src/sandbox/utils/fsUtils.ts @@ -4,68 +4,55 @@ * SPDX-License-Identifier: Apache-2.0 */ -import fs from 'node:fs'; +import fs from 'node:fs/promises'; import path from 'node:path'; -import { assertValidPathString } from '../../utils/paths.js'; +import { resolveToRealPath } from '../../utils/paths.js'; export function isErrnoException(e: unknown): e is NodeJS.ErrnoException { return e instanceof Error && 'code' in e; } -export function tryRealpath(p: string): string { - assertValidPathString(p); - try { - return fs.realpathSync(p); - } catch (e) { - if (isErrnoException(e) && e.code === 'ENOENT') { - const parentDir = path.dirname(p); - if (parentDir === p) { - return p; - } - return path.join(tryRealpath(parentDir), path.basename(p)); - } - throw e; - } -} - -export function resolveGitWorktreePaths(workspacePath: string): { +export async function resolveGitWorktreePaths(workspacePath: string): Promise<{ worktreeGitDir?: string; mainGitDir?: string; -} { +}> { try { const gitPath = path.join(workspacePath, '.git'); - const gitStat = fs.lstatSync(gitPath); + const gitStat = await fs.lstat(gitPath); if (gitStat.isFile()) { - const gitContent = fs.readFileSync(gitPath, 'utf8'); + const gitContent = await fs.readFile(gitPath, 'utf8'); const match = gitContent.match(/^gitdir:\s+(.+)$/m); if (match && match[1]) { let worktreeGitDir = match[1].trim(); if (!path.isAbsolute(worktreeGitDir)) { worktreeGitDir = path.resolve(workspacePath, worktreeGitDir); } - const resolvedWorktreeGitDir = tryRealpath(worktreeGitDir); + const resolvedWorktreeGitDir = resolveToRealPath(worktreeGitDir); // Security check: Verify the bidirectional link to prevent sandbox escape let isValid = false; try { const backlinkPath = path.join(resolvedWorktreeGitDir, 'gitdir'); - const backlink = fs.readFileSync(backlinkPath, 'utf8').trim(); + const backlink = (await fs.readFile(backlinkPath, 'utf8')).trim(); // The backlink must resolve to the workspace's .git file - if (tryRealpath(backlink) === tryRealpath(gitPath)) { + if (resolveToRealPath(backlink) === resolveToRealPath(gitPath)) { isValid = true; } } catch { // Fallback for submodules: check core.worktree in config try { const configPath = path.join(resolvedWorktreeGitDir, 'config'); - const config = fs.readFileSync(configPath, 'utf8'); + const config = await fs.readFile(configPath, 'utf8'); const match = config.match(/^\s*worktree\s*=\s*(.+)$/m); if (match && match[1]) { const worktreePath = path.resolve( resolvedWorktreeGitDir, match[1].trim(), ); - if (tryRealpath(worktreePath) === tryRealpath(workspacePath)) { + if ( + resolveToRealPath(worktreePath) === + resolveToRealPath(workspacePath) + ) { isValid = true; } } @@ -78,7 +65,7 @@ export function resolveGitWorktreePaths(workspacePath: string): { return {}; // Reject: valid worktrees/submodules must have a readable backlink } - const mainGitDir = tryRealpath( + const mainGitDir = resolveToRealPath( path.dirname(path.dirname(resolvedWorktreeGitDir)), ); return { diff --git a/packages/core/src/sandbox/windows/WindowsSandboxManager.test.ts b/packages/core/src/sandbox/windows/WindowsSandboxManager.test.ts index 40902b9121..b504d92f72 100644 --- a/packages/core/src/sandbox/windows/WindowsSandboxManager.test.ts +++ b/packages/core/src/sandbox/windows/WindowsSandboxManager.test.ts @@ -10,6 +10,7 @@ import os from 'node:os'; import path from 'node:path'; import { WindowsSandboxManager } from './WindowsSandboxManager.js'; import * as sandboxManager from '../../services/sandboxManager.js'; +import * as paths from '../../utils/paths.js'; import type { SandboxRequest } from '../../services/sandboxManager.js'; import { spawnAsync } from '../../utils/shell-utils.js'; import type { SandboxPolicyManager } from '../../policy/sandboxPolicyManager.js'; @@ -44,9 +45,7 @@ describe('WindowsSandboxManager', () => { beforeEach(() => { vi.spyOn(os, 'platform').mockReturnValue('win32'); - vi.spyOn(sandboxManager, 'tryRealpath').mockImplementation(async (p) => - p.toString(), - ); + vi.spyOn(paths, 'resolveToRealPath').mockImplementation((p) => p); // Mock existsSync to skip the csc.exe auto-compilation of helper during unit tests. const originalExistsSync = fs.existsSync; @@ -299,6 +298,60 @@ describe('WindowsSandboxManager', () => { } }); + it('should NOT grant Low Integrity access to git worktree paths (enforce read-only)', async () => { + const worktreeGitDir = createTempDir('worktree-git'); + const mainGitDir = createTempDir('main-git'); + + try { + vi.spyOn(sandboxManager, 'resolveSandboxPaths').mockResolvedValue({ + workspace: { original: testCwd, resolved: testCwd }, + forbidden: [], + globalIncludes: [], + policyAllowed: [], + policyRead: [], + policyWrite: [], + gitWorktree: { + worktreeGitDir, + mainGitDir, + }, + }); + + const req: SandboxRequest = { + command: 'test', + args: [], + cwd: testCwd, + env: {}, + }; + + await manager.prepareCommand(req); + + const icaclsArgs = vi + .mocked(spawnAsync) + .mock.calls.filter((c) => c[0] === 'icacls') + .map((c) => c[1]); + + // Verify that no icacls grants were issued for the git directories + expect(icaclsArgs).not.toContainEqual([ + worktreeGitDir, + '/grant', + '*S-1-16-4096:(OI)(CI)(M)', + '/setintegritylevel', + '(OI)(CI)Low', + ]); + + expect(icaclsArgs).not.toContainEqual([ + mainGitDir, + '/grant', + '*S-1-16-4096:(OI)(CI)(M)', + '/setintegritylevel', + '(OI)(CI)Low', + ]); + } finally { + fs.rmSync(worktreeGitDir, { recursive: true, force: true }); + fs.rmSync(mainGitDir, { recursive: true, force: true }); + } + }); + it('should grant Low Integrity access to additional write paths', async () => { const extraWritePath = createTempDir('extra-write'); try { diff --git a/packages/core/src/sandbox/windows/WindowsSandboxManager.ts b/packages/core/src/sandbox/windows/WindowsSandboxManager.ts index 86d1eda641..2cf736f865 100644 --- a/packages/core/src/sandbox/windows/WindowsSandboxManager.ts +++ b/packages/core/src/sandbox/windows/WindowsSandboxManager.ts @@ -299,7 +299,9 @@ export class WindowsSandboxManager implements SandboxManager { ) : false; - if (!isReadonlyMode || isApproved) { + const workspaceWrite = !isReadonlyMode || isApproved || isYolo; + + if (workspaceWrite) { await this.grantLowIntegrityAccess(resolvedPaths.workspace.resolved); writableRoots.push(resolvedPaths.workspace.resolved); } @@ -345,6 +347,12 @@ export class WindowsSandboxManager implements SandboxManager { } } + // Support git worktrees/submodules; read-only to prevent malicious hook/config modification (RCE). + // Read access is inherited; skip grantLowIntegrityAccess to ensure write protection. + if (resolvedPaths.gitWorktree) { + // No-op for read access. + } + // 2. Collect secret files and apply protective ACLs // On Windows, we explicitly deny access to secret files for Low Integrity // processes to ensure they cannot be read or written. diff --git a/packages/core/src/services/sandboxManager.integration.test.ts b/packages/core/src/services/sandboxManager.integration.test.ts index 1461b6d606..65adeaacbb 100644 --- a/packages/core/src/services/sandboxManager.integration.test.ts +++ b/packages/core/src/services/sandboxManager.integration.test.ts @@ -507,6 +507,102 @@ describe('SandboxManager Integration', () => { }); }); + describe('Git Worktree Support', () => { + it('allows access to git common directory in a worktree', async () => { + const mainRepo = createTempDir('main-repo-'); + const worktreeDir = createTempDir('worktree-'); + + const mainGitDir = path.join(mainRepo, '.git'); + fs.mkdirSync(mainGitDir, { recursive: true }); + fs.writeFileSync( + path.join(mainGitDir, 'config'), + '[core]\n\trepositoryformatversion = 0\n', + ); + + const worktreeGitDir = path.join( + mainGitDir, + 'worktrees', + 'test-worktree', + ); + fs.mkdirSync(worktreeGitDir, { recursive: true }); + + // Create the .git file in the worktree directory pointing to the worktree git dir + fs.writeFileSync( + path.join(worktreeDir, '.git'), + `gitdir: ${worktreeGitDir}\n`, + ); + + // Create the backlink from worktree git dir to the worktree's .git file + const backlinkPath = path.join(worktreeGitDir, 'gitdir'); + fs.writeFileSync(backlinkPath, path.join(worktreeDir, '.git')); + + // Create a file in the worktree git dir that we want to access + const secretFile = path.join(worktreeGitDir, 'secret.txt'); + fs.writeFileSync(secretFile, 'git-secret'); + + const osManager = createSandboxManager( + { enabled: true }, + { workspace: worktreeDir }, + ); + + const { command, args } = Platform.cat(secretFile); + const sandboxed = await osManager.prepareCommand({ + command, + args, + cwd: worktreeDir, + env: process.env, + }); + + const result = await runCommand(sandboxed); + assertResult(result, sandboxed, 'success'); + expect(result.stdout.trim()).toBe('git-secret'); + }); + + it('blocks write access to git common directory in a worktree', async () => { + const mainRepo = createTempDir('main-repo-'); + const worktreeDir = createTempDir('worktree-'); + + const mainGitDir = path.join(mainRepo, '.git'); + fs.mkdirSync(mainGitDir, { recursive: true }); + + const worktreeGitDir = path.join( + mainGitDir, + 'worktrees', + 'test-worktree', + ); + fs.mkdirSync(worktreeGitDir, { recursive: true }); + + fs.writeFileSync( + path.join(worktreeDir, '.git'), + `gitdir: ${worktreeGitDir}\n`, + ); + fs.writeFileSync( + path.join(worktreeGitDir, 'gitdir'), + path.join(worktreeDir, '.git'), + ); + + const targetFile = path.join(worktreeGitDir, 'secret.txt'); + + const osManager = createSandboxManager( + { enabled: true }, + // Use YOLO mode to ensure the workspace is fully writable, but git worktrees should still be read-only + { workspace: worktreeDir, modeConfig: { yolo: true } }, + ); + + const { command, args } = Platform.touch(targetFile); + const sandboxed = await osManager.prepareCommand({ + command, + args, + cwd: worktreeDir, + env: process.env, + }); + + const result = await runCommand(sandboxed); + assertResult(result, sandboxed, 'failure'); + expect(fs.existsSync(targetFile)).toBe(false); + }); + }); + describe('Network Access', () => { let server: http.Server; let url: string; diff --git a/packages/core/src/services/sandboxManager.test.ts b/packages/core/src/services/sandboxManager.test.ts index 134ef167bd..7ff8525f77 100644 --- a/packages/core/src/services/sandboxManager.test.ts +++ b/packages/core/src/services/sandboxManager.test.ts @@ -13,7 +13,6 @@ import { sanitizePaths, findSecretFiles, isSecretFile, - tryRealpath, resolveSandboxPaths, getPathIdentity, type SandboxRequest, @@ -36,10 +35,25 @@ vi.mock('node:fs/promises', async () => { readdir: vi.fn(), realpath: vi.fn(), stat: vi.fn(), + lstat: vi.fn(), + readFile: vi.fn(), }, readdir: vi.fn(), realpath: vi.fn(), stat: vi.fn(), + lstat: vi.fn(), + readFile: vi.fn(), + }; +}); + +vi.mock('../utils/paths.js', async () => { + const actual = + await vi.importActual( + '../utils/paths.js', + ); + return { + ...actual, + resolveToRealPath: vi.fn((p) => p), }; }); @@ -279,104 +293,6 @@ describe('SandboxManager', () => { }); }); - describe('tryRealpath', () => { - beforeEach(() => { - vi.clearAllMocks(); - }); - - it('should return the realpath if the file exists', async () => { - const realPath = path.resolve('/real/path/to/file.txt'); - const symlinkPath = path.resolve('/some/symlink/to/file.txt'); - vi.mocked(fsPromises.realpath).mockResolvedValue(realPath as never); - const result = await tryRealpath(symlinkPath); - expect(result).toBe(realPath); - expect(fsPromises.realpath).toHaveBeenCalledWith(symlinkPath); - }); - - it('should fallback to parent directory if file does not exist (ENOENT)', async () => { - const nonexistent = path.resolve('/workspace/nonexistent.txt'); - const workspace = path.resolve('/workspace'); - const realWorkspace = path.resolve('/real/workspace'); - - vi.mocked(fsPromises.realpath).mockImplementation(((p: string) => { - if (p === nonexistent) { - return Promise.reject( - Object.assign(new Error('ENOENT: no such file or directory'), { - code: 'ENOENT', - }), - ); - } - if (p === workspace) { - return Promise.resolve(realWorkspace); - } - return Promise.reject(new Error(`Unexpected path: ${p}`)); - }) as never); - - const result = await tryRealpath(nonexistent); - - // It should combine the real path of the parent with the original basename - expect(result).toBe(path.join(realWorkspace, 'nonexistent.txt')); - }); - - it('should recursively fallback up the directory tree on multiple ENOENT errors', async () => { - const missingFile = path.resolve( - '/workspace/missing_dir/missing_file.txt', - ); - const missingDir = path.resolve('/workspace/missing_dir'); - const workspace = path.resolve('/workspace'); - const realWorkspace = path.resolve('/real/workspace'); - - vi.mocked(fsPromises.realpath).mockImplementation(((p: string) => { - if (p === missingFile) { - return Promise.reject( - Object.assign(new Error('ENOENT'), { code: 'ENOENT' }), - ); - } - if (p === missingDir) { - return Promise.reject( - Object.assign(new Error('ENOENT'), { code: 'ENOENT' }), - ); - } - if (p === workspace) { - return Promise.resolve(realWorkspace); - } - return Promise.reject(new Error(`Unexpected path: ${p}`)); - }) as never); - - const result = await tryRealpath(missingFile); - - // It should resolve '/workspace' to '/real/workspace' and append the missing parts - expect(result).toBe( - path.join(realWorkspace, 'missing_dir', 'missing_file.txt'), - ); - }); - - it('should return the path unchanged if it reaches the root directory and it still does not exist', async () => { - const rootPath = path.resolve('/'); - vi.mocked(fsPromises.realpath).mockImplementation(() => - Promise.reject(Object.assign(new Error('ENOENT'), { code: 'ENOENT' })), - ); - - const result = await tryRealpath(rootPath); - expect(result).toBe(rootPath); - }); - - it('should throw an error if realpath fails with a non-ENOENT error (e.g. EACCES)', async () => { - const secretFile = path.resolve('/secret/file.txt'); - vi.mocked(fsPromises.realpath).mockImplementation(() => - Promise.reject( - Object.assign(new Error('EACCES: permission denied'), { - code: 'EACCES', - }), - ), - ); - - await expect(tryRealpath(secretFile)).rejects.toThrow( - 'EACCES: permission denied', - ); - }); - }); - describe('NoopSandboxManager', () => { const sandboxManager = new NoopSandboxManager(); diff --git a/packages/core/src/services/sandboxManager.ts b/packages/core/src/services/sandboxManager.ts index f7f2944fe7..0191207b16 100644 --- a/packages/core/src/services/sandboxManager.ts +++ b/packages/core/src/services/sandboxManager.ts @@ -15,7 +15,6 @@ import { isKnownSafeCommand as isWindowsSafeCommand, isDangerousCommand as isWindowsDangerousCommand, } from '../sandbox/windows/commandSafety.js'; -import { isNodeError } from '../utils/errors.js'; import { sanitizeEnvironment, getSecureSanitizationConfig, @@ -24,6 +23,7 @@ import { import type { ShellExecutionResult } from './shellExecutionService.js'; import type { SandboxPolicyManager } from '../policy/sandboxPolicyManager.js'; import { resolveToRealPath } from '../utils/paths.js'; +import { resolveGitWorktreePaths } from '../sandbox/utils/fsUtils.js'; /** * A structured result of fully resolved sandbox paths. @@ -48,6 +48,13 @@ export interface ResolvedSandboxPaths { policyRead: string[]; /** Paths granted temporary write access by the current command's dynamic permissions. */ policyWrite: string[]; + /** Auto-detected paths for git worktrees/submodules. */ + gitWorktree?: { + /** The actual .git directory for this worktree. */ + worktreeGitDir: string; + /** The main repository's .git directory (if applicable). */ + mainGitDir?: string; + }; } export interface SandboxPermissions { @@ -392,6 +399,12 @@ export async function resolveSandboxPaths( ); const forbiddenIdentities = new Set(forbidden.map(getPathIdentity)); + const { worktreeGitDir, mainGitDir } = + await resolveGitWorktreePaths(resolvedWorkspace); + const gitWorktree = worktreeGitDir + ? { gitWorktree: { worktreeGitDir, mainGitDir } } + : undefined; + /** * Filters out any paths that are explicitly forbidden or match the workspace root (original or resolved). */ @@ -413,8 +426,10 @@ export async function resolveSandboxPaths( policyAllowed: filter(policyAllowed), policyRead: filter(policyRead), policyWrite: filter(policyWrite), + ...gitWorktree, }; } + /** * Sanitizes an array of paths by deduplicating them and ensuring they are absolute. * Always returns an array (empty if input is null/undefined). @@ -451,24 +466,4 @@ export function getPathIdentity(p: string): string { return isCaseInsensitive ? norm.toLowerCase() : norm; } -/** - * Resolves symlinks for a given path to prevent sandbox escapes. - * If a file does not exist (ENOENT), it recursively resolves the parent directory. - * Other errors (e.g. EACCES) are re-thrown. - */ -export async function tryRealpath(p: string): Promise { - try { - return await fs.realpath(p); - } catch (e) { - if (isNodeError(e) && e.code === 'ENOENT') { - const parentDir = path.dirname(p); - if (parentDir === p) { - return p; - } - return path.join(await tryRealpath(parentDir), path.basename(p)); - } - throw e; - } -} - export { createSandboxManager } from './sandboxManagerFactory.js'; From de628b04fc6012b2c78c37b7390127e79f8d31d0 Mon Sep 17 00:00:00 2001 From: Sri Pasumarthi <111310667+sripasg@users.noreply.github.com> Date: Thu, 9 Apr 2026 15:23:00 -0700 Subject: [PATCH 2/4] feat(test): add high-volume shell test and refine perf harness (#24983) --- .../core/src/telemetry/activity-monitor.ts | 1 + .../core/src/telemetry/event-loop-monitor.ts | 99 +++++++++++++++ packages/core/src/telemetry/index.ts | 7 ++ packages/core/src/telemetry/metrics.ts | 28 +++++ packages/core/src/telemetry/sdk.ts | 28 ++++- packages/test-utils/src/perf-test-harness.ts | 27 ++-- perf-tests/baselines.json | 28 +++-- perf-tests/perf-usage.test.ts | 118 +++++++++++++++++- perf-tests/perf.high-volume.responses | 3 + 9 files changed, 312 insertions(+), 27 deletions(-) create mode 100644 packages/core/src/telemetry/event-loop-monitor.ts create mode 100644 perf-tests/perf.high-volume.responses diff --git a/packages/core/src/telemetry/activity-monitor.ts b/packages/core/src/telemetry/activity-monitor.ts index 15b96cb1e3..255fb39e5f 100644 --- a/packages/core/src/telemetry/activity-monitor.ts +++ b/packages/core/src/telemetry/activity-monitor.ts @@ -50,6 +50,7 @@ export const DEFAULT_ACTIVITY_CONFIG: ActivityMonitorConfig = { ActivityType.USER_INPUT_START, ActivityType.MESSAGE_ADDED, ActivityType.TOOL_CALL_SCHEDULED, + ActivityType.TOOL_CALL_COMPLETED, ActivityType.STREAM_START, ], }; diff --git a/packages/core/src/telemetry/event-loop-monitor.ts b/packages/core/src/telemetry/event-loop-monitor.ts new file mode 100644 index 0000000000..d56179d0da --- /dev/null +++ b/packages/core/src/telemetry/event-loop-monitor.ts @@ -0,0 +1,99 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import process from 'node:process'; +import { monitorEventLoopDelay, type IntervalHistogram } from 'node:perf_hooks'; +import type { Config } from '../config/config.js'; +import { + recordEventLoopDelay, + isPerformanceMonitoringActive, +} from './metrics.js'; + +export class EventLoopMonitor { + private eventLoopHistogram: IntervalHistogram | null = null; + private intervalId: NodeJS.Timeout | null = null; + private isRunning = false; + + start(config: Config, intervalMs: number = 10000): void { + const isEnabled = + process.env['GEMINI_EVENT_LOOP_MONITOR_ENABLED'] === 'true'; + if (!isEnabled || !isPerformanceMonitoringActive() || this.isRunning) { + return; + } + + this.isRunning = true; + this.eventLoopHistogram = monitorEventLoopDelay({ resolution: 10 }); + this.eventLoopHistogram.enable(); + + this.intervalId = setInterval(() => { + this.takeSnapshot(config); + }, intervalMs).unref(); + } + + stop(): void { + if (!this.isRunning) { + return; + } + + if (this.intervalId) { + clearInterval(this.intervalId); + this.intervalId = null; + } + + if (this.eventLoopHistogram) { + this.eventLoopHistogram.disable(); + this.eventLoopHistogram = null; + } + + this.isRunning = false; + } + + private takeSnapshot(config: Config): void { + if (!this.eventLoopHistogram) { + return; + } + + const p50 = this.eventLoopHistogram.percentile(50) / 1e6; + const p95 = this.eventLoopHistogram.percentile(95) / 1e6; + const max = this.eventLoopHistogram.max / 1e6; + + recordEventLoopDelay(config, p50, { + percentile: 'p50', + component: 'event_loop_monitor', + }); + recordEventLoopDelay(config, p95, { + percentile: 'p95', + component: 'event_loop_monitor', + }); + recordEventLoopDelay(config, max, { + percentile: 'max', + component: 'event_loop_monitor', + }); + } +} + +let globalEventLoopMonitor: EventLoopMonitor | null = null; + +export function startGlobalEventLoopMonitoring( + config: Config, + intervalMs?: number, +): void { + if (!globalEventLoopMonitor) { + globalEventLoopMonitor = new EventLoopMonitor(); + } + globalEventLoopMonitor.start(config, intervalMs); +} + +export function stopGlobalEventLoopMonitoring(): void { + if (globalEventLoopMonitor) { + globalEventLoopMonitor.stop(); + globalEventLoopMonitor = null; + } +} + +export function getEventLoopMonitor(): EventLoopMonitor | null { + return globalEventLoopMonitor; +} diff --git a/packages/core/src/telemetry/index.ts b/packages/core/src/telemetry/index.ts index ea65941e06..d3cc033341 100644 --- a/packages/core/src/telemetry/index.ts +++ b/packages/core/src/telemetry/index.ts @@ -93,6 +93,12 @@ export { stopGlobalMemoryMonitoring, } from './memory-monitor.js'; export type { MemorySnapshot, ProcessMetrics } from './memory-monitor.js'; +export { + EventLoopMonitor, + startGlobalEventLoopMonitoring, + stopGlobalEventLoopMonitoring, + getEventLoopMonitor, +} from './event-loop-monitor.js'; export { HighWaterMarkTracker } from './high-water-mark-tracker.js'; export { RateLimiter } from './rate-limiter.js'; export { ActivityType } from './activity-types.js'; @@ -133,6 +139,7 @@ export { recordStartupPerformance, recordMemoryUsage, recordCpuUsage, + recordEventLoopDelay, recordToolQueueDepth, recordToolExecutionBreakdown, recordTokenEfficiency, diff --git a/packages/core/src/telemetry/metrics.ts b/packages/core/src/telemetry/metrics.ts index 422f0222a5..377479c1e4 100644 --- a/packages/core/src/telemetry/metrics.ts +++ b/packages/core/src/telemetry/metrics.ts @@ -88,6 +88,7 @@ const GEN_AI_CLIENT_OPERATION_DURATION = 'gen_ai.client.operation.duration'; const STARTUP_TIME = 'gemini_cli.startup.duration'; const MEMORY_USAGE = 'gemini_cli.memory.usage'; const CPU_USAGE = 'gemini_cli.cpu.usage'; +const EVENT_LOOP_DELAY = 'gemini_cli.event_loop.delay'; const TOOL_QUEUE_DEPTH = 'gemini_cli.tool.queue.depth'; const TOOL_EXECUTION_BREAKDOWN = 'gemini_cli.tool.execution.breakdown'; const TOKEN_EFFICIENCY = 'gemini_cli.token.efficiency'; @@ -608,6 +609,17 @@ const PERFORMANCE_HISTOGRAM_DEFINITIONS = { component?: string; }, }, + [EVENT_LOOP_DELAY]: { + description: 'Event loop delay in milliseconds.', + unit: 'ms', + valueType: ValueType.DOUBLE, + assign: (h: Histogram) => (eventLoopDelayHistogram = h), + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion + attributes: {} as { + percentile: string; + component?: string; + }, + }, [TOOL_QUEUE_DEPTH]: { description: 'Number of tools in execution queue.', unit: 'count', @@ -806,6 +818,7 @@ let genAiClientOperationDurationHistogram: Histogram | undefined; let startupTimeHistogram: Histogram | undefined; let memoryUsageGauge: Histogram | undefined; // Using Histogram until ObservableGauge is available let cpuUsageGauge: Histogram | undefined; +let eventLoopDelayHistogram: Histogram | undefined; let toolQueueDepthGauge: Histogram | undefined; let toolExecutionBreakdownHistogram: Histogram | undefined; let tokenEfficiencyHistogram: Histogram | undefined; @@ -1339,6 +1352,21 @@ export function recordCpuUsage( cpuUsageGauge.record(percentage, metricAttributes); } +export function recordEventLoopDelay( + config: Config, + delayMs: number, + attributes: MetricDefinitions[typeof EVENT_LOOP_DELAY]['attributes'], +): void { + if (!eventLoopDelayHistogram || !isPerformanceMonitoringEnabled) return; + + const metricAttributes: Attributes = { + ...baseMetricDefinition.getCommonAttributes(config), + ...attributes, + }; + + eventLoopDelayHistogram.record(delayMs, metricAttributes); +} + export function recordToolQueueDepth(config: Config, queueDepth: number): void { if (!toolQueueDepthGauge || !isPerformanceMonitoringEnabled) return; diff --git a/packages/core/src/telemetry/sdk.ts b/packages/core/src/telemetry/sdk.ts index bafa540790..ac90bf86ad 100644 --- a/packages/core/src/telemetry/sdk.ts +++ b/packages/core/src/telemetry/sdk.ts @@ -52,6 +52,11 @@ import { } from './gcp-exporters.js'; import { TelemetryTarget } from './index.js'; import { debugLogger } from '../utils/debugLogger.js'; +import { + startGlobalMemoryMonitoring, + getMemoryMonitor, +} from './memory-monitor.js'; +import { startGlobalEventLoopMonitoring } from './event-loop-monitor.js'; import { authEvents } from '../code_assist/oauth2.js'; import { coreEvents, CoreEvent } from '../utils/events.js'; import { @@ -91,6 +96,7 @@ diag.setLogger(new DiagLoggerAdapter(), DiagLogLevel.INFO); let sdk: NodeSDK | undefined; let spanProcessor: BatchSpanProcessor | undefined; let logRecordProcessor: BatchLogRecordProcessor | undefined; +let metricReader: PeriodicExportingMetricReader | undefined; let telemetryInitialized = false; let callbackRegistered = false; let authListener: ((newCredentials: JWTInput) => Promise) | undefined = @@ -258,7 +264,6 @@ export async function initializeTelemetry( | GcpLogExporter | FileLogExporter | ConsoleLogRecordExporter; - let metricReader: PeriodicExportingMetricReader; if (useDirectGcpExport) { debugLogger.log( @@ -346,6 +351,26 @@ export async function initializeTelemetry( } activeTelemetryEmail = credentials?.client_email; initializeMetrics(config); + + // Start memory monitoring if interval is specified via environment variable + const monitorInterval = process.env['GEMINI_MEMORY_MONITOR_INTERVAL']; + debugLogger.log( + `[TELEMETRY] GEMINI_MEMORY_MONITOR_INTERVAL: ${monitorInterval}`, + ); + if (monitorInterval) { + const intervalMs = parseInt(monitorInterval, 10); + if (!isNaN(intervalMs) && intervalMs > 0) { + startGlobalMemoryMonitoring(config, intervalMs); + startGlobalEventLoopMonitoring(config, intervalMs); + // Disable enhanced monitoring (rate limiting/high water mark) in tests + // to ensure we get regular snapshots regardless of growth. + const monitor = getMemoryMonitor(); + if (monitor) { + monitor.setEnhancedMonitoring(false); + } + } + } + telemetryInitialized = true; void flushTelemetryBuffer(); } catch (error) { @@ -378,6 +403,7 @@ export async function flushTelemetry(config: Config): Promise { await Promise.all([ spanProcessor.forceFlush(), logRecordProcessor.forceFlush(), + metricReader ? metricReader.forceFlush() : Promise.resolve(), ]); if (config.getDebugMode()) { debugLogger.log('OpenTelemetry SDK flushed successfully.'); diff --git a/packages/test-utils/src/perf-test-harness.ts b/packages/test-utils/src/perf-test-harness.ts index c4625077be..2f376f58b6 100644 --- a/packages/test-utils/src/perf-test-harness.ts +++ b/packages/test-utils/src/perf-test-harness.ts @@ -23,7 +23,6 @@ type PlotFn = (series: number[], config?: PlotConfig) => string; export interface PerfBaseline { wallClockMs: number; cpuTotalUs: number; - eventLoopDelayP99Ms: number; timestamp: string; } @@ -48,8 +47,10 @@ export interface PerfSnapshot { cpuTotalUs: number; eventLoopDelayP50Ms: number; eventLoopDelayP95Ms: number; - eventLoopDelayP99Ms: number; eventLoopDelayMaxMs: number; + childEventLoopDelayP50Ms?: number; + childEventLoopDelayP95Ms?: number; + childEventLoopDelayMaxMs?: number; } /** @@ -159,7 +160,6 @@ export class PerfTestHarness { cpuTotalUs: cpuDelta.user + cpuDelta.system, eventLoopDelayP50Ms: 0, eventLoopDelayP95Ms: 0, - eventLoopDelayP99Ms: 0, eventLoopDelayMaxMs: 0, }; } @@ -196,7 +196,6 @@ export class PerfTestHarness { // Convert from nanoseconds to milliseconds snapshot.eventLoopDelayP50Ms = histogram.percentile(50) / 1e6; snapshot.eventLoopDelayP95Ms = histogram.percentile(95) / 1e6; - snapshot.eventLoopDelayP99Ms = histogram.percentile(99) / 1e6; snapshot.eventLoopDelayMaxMs = histogram.max / 1e6; return snapshot; @@ -305,7 +304,6 @@ export class PerfTestHarness { ` Baseline: ${result.baseline.wallClockMs.toFixed(1)} ms wall-clock\n` + ` Delta: ${deltaPercent.toFixed(1)}% (tolerance: ${tolerance}%)\n` + ` CPU total: ${formatUs(result.median.cpuTotalUs)}\n` + - ` EL p99: ${result.median.eventLoopDelayP99Ms.toFixed(1)} ms\n` + ` Samples: ${result.samples.length} (${result.filteredSamples.length} after IQR filter)`, ); } @@ -316,8 +314,7 @@ export class PerfTestHarness { ` Measured: ${formatUs(result.median.cpuTotalUs)}\n` + ` Baseline: ${formatUs(result.baseline.cpuTotalUs)}\n` + ` Delta: ${result.cpuDeltaPercent.toFixed(1)}% (tolerance: ${cpuTolerance}%)\n` + - ` Wall-clock: ${result.median.wallClockMs.toFixed(1)} ms\n` + - ` EL p99: ${result.median.eventLoopDelayP99Ms.toFixed(1)} ms`, + ` Wall-clock: ${result.median.wallClockMs.toFixed(1)} ms`, ); } } @@ -329,7 +326,6 @@ export class PerfTestHarness { updatePerfBaseline(this.baselinesPath, result.scenarioName, { wallClockMs: result.median.wallClockMs, cpuTotalUs: result.median.cpuTotalUs, - eventLoopDelayP99Ms: result.median.eventLoopDelayP99Ms, }); // Reload baselines after update this.baselines = loadPerfBaselines(this.baselinesPath); @@ -375,9 +371,18 @@ export class PerfTestHarness { ` CPU: ${cpuMs} (user: ${formatUs(result.median.cpuUserUs)}, system: ${formatUs(result.median.cpuSystemUs)})`, ); - if (result.median.eventLoopDelayP99Ms > 0) { + if (result.median.eventLoopDelayMaxMs > 0) { lines.push( - ` Event loop: p50=${result.median.eventLoopDelayP50Ms.toFixed(1)}ms p95=${result.median.eventLoopDelayP95Ms.toFixed(1)}ms p99=${result.median.eventLoopDelayP99Ms.toFixed(1)}ms max=${result.median.eventLoopDelayMaxMs.toFixed(1)}ms`, + ` Event loop (runner): p50=${result.median.eventLoopDelayP50Ms.toFixed(1)}ms p95=${result.median.eventLoopDelayP95Ms.toFixed(1)}ms max=${result.median.eventLoopDelayMaxMs.toFixed(1)}ms`, + ); + } + + if ( + result.median.childEventLoopDelayMaxMs !== undefined && + result.median.childEventLoopDelayMaxMs > 0 + ) { + lines.push( + ` Event loop (CLI): p50=${result.median.childEventLoopDelayP50Ms!.toFixed(1)}ms p95=${result.median.childEventLoopDelayP95Ms!.toFixed(1)}ms max=${result.median.childEventLoopDelayMaxMs!.toFixed(1)}ms`, ); } @@ -517,14 +522,12 @@ export function updatePerfBaseline( measured: { wallClockMs: number; cpuTotalUs: number; - eventLoopDelayP99Ms: number; }, ): void { const baselines = loadPerfBaselines(path); baselines.scenarios[scenarioName] = { wallClockMs: measured.wallClockMs, cpuTotalUs: measured.cpuTotalUs, - eventLoopDelayP99Ms: measured.eventLoopDelayP99Ms, timestamp: new Date().toISOString(), }; savePerfBaselines(path, baselines); diff --git a/perf-tests/baselines.json b/perf-tests/baselines.json index a6bad73574..1dd52a5213 100644 --- a/perf-tests/baselines.json +++ b/perf-tests/baselines.json @@ -1,24 +1,26 @@ { "version": 1, - "updatedAt": "2026-04-08T18:51:29.839Z", + "updatedAt": "2026-04-09T02:30:22.000Z", "scenarios": { "cold-startup-time": { - "wallClockMs": 1333.4230420000004, - "cpuTotalUs": 1711, - "eventLoopDelayP99Ms": 0, - "timestamp": "2026-04-08T18:50:58.124Z" + "wallClockMs": 927.553249999999, + "cpuTotalUs": 1470, + "timestamp": "2026-04-08T22:27:54.871Z" }, "idle-cpu-usage": { - "wallClockMs": 5001.926125, - "cpuTotalUs": 128518, - "eventLoopDelayP99Ms": 12.705791, - "timestamp": "2026-04-08T18:51:23.938Z" + "wallClockMs": 5000.460750000002, + "cpuTotalUs": 12157, + "timestamp": "2026-04-08T22:28:19.098Z" }, "skill-loading-time": { - "wallClockMs": 1372.4463749999995, - "cpuTotalUs": 1550, - "eventLoopDelayP99Ms": 0, - "timestamp": "2026-04-08T18:51:29.839Z" + "wallClockMs": 930.0920409999962, + "cpuTotalUs": 1323, + "timestamp": "2026-04-08T22:28:23.290Z" + }, + "high-volume-shell-output": { + "wallClockMs": 1119.9, + "cpuTotalUs": 2100, + "timestamp": "2026-04-09T02:30:22.000Z" } } } diff --git a/perf-tests/perf-usage.test.ts b/perf-tests/perf-usage.test.ts index 3f92cd9f91..1a361eda5d 100644 --- a/perf-tests/perf-usage.test.ts +++ b/perf-tests/perf-usage.test.ts @@ -8,6 +8,7 @@ import { describe, it, beforeAll, afterAll } from 'vitest'; import { TestRig, PerfTestHarness } from '@google/gemini-cli-test-utils'; import { join, dirname } from 'node:path'; import { fileURLToPath } from 'node:url'; +import { existsSync, readFileSync } from 'node:fs'; const __dirname = dirname(fileURLToPath(import.meta.url)); const BASELINES_PATH = join(__dirname, 'baselines.json'); @@ -33,7 +34,7 @@ describe('CPU Performance Tests', () => { afterAll(async () => { // Generate the summary report after all tests await harness.generateReport(); - }); + }, 30000); it('cold-startup-time: startup completes within baseline', async () => { const result = await harness.runScenario('cold-startup-time', async () => { @@ -150,4 +151,119 @@ describe('CPU Performance Tests', () => { harness.assertWithinBaseline(result); } }); + + it('high-volume-shell-output: handles large output efficiently', async () => { + const result = await harness.runScenario( + 'high-volume-shell-output', + async () => { + const rig = new TestRig(); + try { + rig.setup('perf-high-volume-output', { + fakeResponsesPath: join(__dirname, 'perf.high-volume.responses'), + }); + + const snapshot = await harness.measureWithEventLoop( + 'high-volume-output', + async () => { + const runResult = await rig.run({ + args: ['Generate 1M lines of output'], + timeout: 120000, + env: { + GEMINI_API_KEY: 'fake-perf-test-key', + GEMINI_TELEMETRY_ENABLED: 'true', + GEMINI_MEMORY_MONITOR_INTERVAL: '500', + GEMINI_EVENT_LOOP_MONITOR_ENABLED: 'true', + DEBUG: 'true', + }, + }); + console.log(` Child Process Output:`, runResult); + }, + ); + + // Query CLI's own performance metrics from telemetry logs + await rig.waitForTelemetryReady(); + + // Debug: Read and log the telemetry file content + try { + const logFilePath = join(rig.homeDir!, 'telemetry.log'); + if (existsSync(logFilePath)) { + const content = readFileSync(logFilePath, 'utf-8'); + console.log(` Telemetry Log Content:\n`, content); + } else { + console.log(` Telemetry log file not found at: ${logFilePath}`); + } + } catch (e) { + console.error(` Failed to read telemetry log:`, e); + } + + const memoryMetric = rig.readMetric('memory.usage'); + const cpuMetric = rig.readMetric('cpu.usage'); + const toolLatencyMetric = rig.readMetric('tool.call.latency'); + const eventLoopMetric = rig.readMetric('event_loop.delay'); + + if (memoryMetric) { + console.log( + ` CLI Memory Metric found:`, + JSON.stringify(memoryMetric), + ); + } + if (cpuMetric) { + console.log(` CLI CPU Metric found:`, JSON.stringify(cpuMetric)); + } + if (toolLatencyMetric) { + console.log( + ` CLI Tool Latency Metric found:`, + JSON.stringify(toolLatencyMetric), + ); + } + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const logs = (rig as any)._readAndParseTelemetryLog(); + console.log(` Total telemetry log entries: ${logs.length}`); + for (const logData of logs) { + if (logData.scopeMetrics) { + for (const scopeMetric of logData.scopeMetrics) { + for (const metric of scopeMetric.metrics) { + if (metric.descriptor.name.includes('event_loop')) { + console.log( + ` Found event_loop metric in log:`, + metric.descriptor.name, + ); + } + } + } + } + } + + if (eventLoopMetric) { + console.log( + ` CLI Event Loop Metric found:`, + JSON.stringify(eventLoopMetric), + ); + + const findValue = (percentile: string) => { + const dp = eventLoopMetric.dataPoints.find( + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (p: any) => p.attributes.percentile === percentile, + ); + return dp ? dp.value.min : undefined; + }; + + snapshot.childEventLoopDelayP50Ms = findValue('p50'); + snapshot.childEventLoopDelayP95Ms = findValue('p95'); + snapshot.childEventLoopDelayMaxMs = findValue('max'); + } + + return snapshot; + } finally { + await rig.cleanup(); + } + }, + ); + + if (UPDATE_BASELINES) { + harness.updateScenarioBaseline(result); + } else { + harness.assertWithinBaseline(result); + } + }); }); diff --git a/perf-tests/perf.high-volume.responses b/perf-tests/perf.high-volume.responses new file mode 100644 index 0000000000..74f5972db9 --- /dev/null +++ b/perf-tests/perf.high-volume.responses @@ -0,0 +1,3 @@ +{"method":"generateContent","response":{"candidates":[{"content":{"parts":[{"text":"0"}],"role":"model"},"finishReason":"STOP","index":0}]}} +{"method":"generateContentStream","response":[{"candidates":[{"content":{"parts":[{"functionCall":{"name":"run_shell_command","args":{"command":"yes | head -n 1000000"}}}],"role":"model"},"finishReason":"STOP","index":0}]}]} +{"method":"generateContentStream","response":[{"candidates":[{"content":{"parts":[{"text":"I have generated 1M lines of output."}],"role":"model"},"finishReason":"STOP","index":0}]}]} From 55db77bb9120994d2ed0b6994d15307d959023da Mon Sep 17 00:00:00 2001 From: Tommaso Sciortino Date: Thu, 9 Apr 2026 22:25:23 +0000 Subject: [PATCH 3/4] fix(core): silently handle EPERM when listing dir structure (#25066) --- packages/core/src/utils/getFolderStructure.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/core/src/utils/getFolderStructure.ts b/packages/core/src/utils/getFolderStructure.ts index 5a2f99d729..5e7adc9d5b 100644 --- a/packages/core/src/utils/getFolderStructure.ts +++ b/packages/core/src/utils/getFolderStructure.ts @@ -113,7 +113,9 @@ async function readFullStructure( } catch (error: unknown) { if ( isNodeError(error) && - (error.code === 'EACCES' || error.code === 'ENOENT') + (error.code === 'EACCES' || + error.code === 'ENOENT' || + error.code === 'EPERM') ) { debugLogger.warn( `Warning: Could not read directory ${currentPath}: ${error.message}`, @@ -121,7 +123,7 @@ async function readFullStructure( if (currentPath === rootPath && error.code === 'ENOENT') { return null; // Root directory itself not found } - // For other EACCES/ENOENT on subdirectories, just skip them. + // For other EACCES/ENOENT/EPERM on subdirectories, just skip them. continue; } throw error; From 96cc8a0daddd1e66597ff862be20434a5d6a1b1b Mon Sep 17 00:00:00 2001 From: gemini-cli-robot Date: Thu, 9 Apr 2026 16:30:26 -0700 Subject: [PATCH 4/4] Changelog for v0.37.1 (#25055) Co-authored-by: gemini-cli-robot <224641728+gemini-cli-robot@users.noreply.github.com> --- docs/changelogs/latest.md | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/docs/changelogs/latest.md b/docs/changelogs/latest.md index f57ea4b56d..3184abf79d 100644 --- a/docs/changelogs/latest.md +++ b/docs/changelogs/latest.md @@ -1,6 +1,6 @@ -# Latest stable release: v0.37.0 +# Latest stable release: v0.37.1 -Released: April 08, 2026 +Released: April 09, 2026 For most users, our latest stable release is the recommended release. Install the latest stable version with: @@ -26,6 +26,12 @@ npm install -g @google/gemini-cli ## What's Changed +- fix(acp): handle all InvalidStreamError types gracefully in prompt + [#24540](https://github.com/google-gemini/gemini-cli/pull/24540) +- feat(acp): add support for /about command + [#24649](https://github.com/google-gemini/gemini-cli/pull/24649) +- feat(acp): add /help command + [#24839](https://github.com/google-gemini/gemini-cli/pull/24839) - feat(evals): centralize test agents into test-utils for reuse by @Samee24 in [#23616](https://github.com/google-gemini/gemini-cli/pull/23616) - revert: chore(config): disable agents by default by @abhipatel12 in @@ -416,4 +422,4 @@ npm install -g @google/gemini-cli [#24842](https://github.com/google-gemini/gemini-cli/pull/24842) **Full Changelog**: -https://github.com/google-gemini/gemini-cli/compare/v0.36.0...v0.37.0 +https://github.com/google-gemini/gemini-cli/compare/v0.36.0...v0.37.1