Skip to content

test(core): skip environment-dependent tests with a reason instead of failing (#28830) #28832

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Chirag6722 wants to merge 2 commits into from
Closed

test(core): skip environment-dependent tests with a reason instead of failing (#28830) #28832

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ccc3b18
test(core): skip environment-dependent tests with a reason instead of…
Chirag6722 Aug 16, 2026
ddd6829
fix(test-utils): strip surrounding quotes from PATH entries in hasPow…
Chirag6722 Aug 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 31 additions & 24 deletions packages/core/src/safety/built-in.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import * as path from 'node:path';
import { AllowedPathChecker } from './built-in.js';
import { SafetyCheckDecision, type SafetyCheckInput } from './protocol.js';
import type { FunctionCall } from '@google/genai';
import { canCreateSymlinks } from '../test-utils/environment-capabilities.js';

describe('AllowedPathChecker', () => {
let checker: AllowedPathChecker;
Expand Down Expand Up @@ -116,35 +117,41 @@ describe('AllowedPathChecker', () => {
expect(result.decision).toBe(SafetyCheckDecision.ALLOW);
});

it('should deny access if path contains a symlink pointing outside allowed directories', async () => {
const symlinkPath = path.join(mockCwd, 'symlink');
const targetPath = path.join(testRootDir, 'etc', 'passwd');
await fs.mkdir(path.dirname(targetPath), { recursive: true });
await fs.writeFile(targetPath, 'secret');
it.skipIf(!canCreateSymlinks())(
'should deny access if path contains a symlink pointing outside allowed directories',
async () => {
const symlinkPath = path.join(mockCwd, 'symlink');
const targetPath = path.join(testRootDir, 'etc', 'passwd');
await fs.mkdir(path.dirname(targetPath), { recursive: true });
await fs.writeFile(targetPath, 'secret');

// Create symlink: mockCwd/symlink -> targetPath
await fs.symlink(targetPath, symlinkPath);
// Create symlink: mockCwd/symlink -> targetPath
await fs.symlink(targetPath, symlinkPath);

const input = createInput({ path: symlinkPath });
const result = await checker.check(input);
expect(result.decision).toBe(SafetyCheckDecision.DENY);
expect(result.reason).toContain(
'outside of the allowed workspace directories',
);
});
const input = createInput({ path: symlinkPath });
const result = await checker.check(input);
expect(result.decision).toBe(SafetyCheckDecision.DENY);
expect(result.reason).toContain(
'outside of the allowed workspace directories',
);
},
);

it('should allow access if path contains a symlink pointing INSIDE allowed directories', async () => {
const symlinkPath = path.join(mockCwd, 'symlink-inside');
const realFilePath = path.join(mockCwd, 'real-file');
await fs.writeFile(realFilePath, 'real content');
it.skipIf(!canCreateSymlinks())(
'should allow access if path contains a symlink pointing INSIDE allowed directories',
async () => {
const symlinkPath = path.join(mockCwd, 'symlink-inside');
const realFilePath = path.join(mockCwd, 'real-file');
await fs.writeFile(realFilePath, 'real content');

// Create symlink: mockCwd/symlink-inside -> mockCwd/real-file
await fs.symlink(realFilePath, symlinkPath);
// Create symlink: mockCwd/symlink-inside -> mockCwd/real-file
await fs.symlink(realFilePath, symlinkPath);

const input = createInput({ path: symlinkPath });
const result = await checker.check(input);
expect(result.decision).toBe(SafetyCheckDecision.ALLOW);
});
const input = createInput({ path: symlinkPath });
const result = await checker.check(input);
expect(result.decision).toBe(SafetyCheckDecision.ALLOW);
},
);

it('should check explicitly included arguments', async () => {
const outsidePath = path.join(testRootDir, 'etc', 'passwd');
Expand Down
76 changes: 40 additions & 36 deletions packages/core/src/services/sandboxManager.integration.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import os from 'node:os';
import fs from 'node:fs';
import path from 'node:path';
import http from 'node:http';
import { canCreateSymlinks } from '../test-utils/environment-capabilities.js';

/**
* Cross-platform command wrappers using Node.js inline scripts.
Expand Down Expand Up @@ -691,48 +692,51 @@ describe('SandboxManager Integration', () => {
expect(fs.existsSync(nonExistentFile)).toBe(false);
});

it('restricts symlinks to forbidden targets', async () => {
const tempWorkspace = createTempDir('workspace-');
const targetFile = path.join(tempWorkspace, 'target.txt');
const symlinkFile = path.join(tempWorkspace, 'link.txt');
it.skipIf(!canCreateSymlinks())(
'restricts symlinks to forbidden targets',
async () => {
const tempWorkspace = createTempDir('workspace-');
const targetFile = path.join(tempWorkspace, 'target.txt');
const symlinkFile = path.join(tempWorkspace, 'link.txt');

fs.writeFileSync(targetFile, 'secret data');
fs.symlinkSync(targetFile, symlinkFile);
fs.writeFileSync(targetFile, 'secret data');
fs.symlinkSync(targetFile, symlinkFile);

const osManager = createSandboxManager(
{ enabled: true },
{
workspace: tempWorkspace,
forbiddenPaths: async () => [symlinkFile],
},
);
const osManager = createSandboxManager(
{ enabled: true },
{
workspace: tempWorkspace,
forbiddenPaths: async () => [symlinkFile],
},
);

// Attempt to write to the target file directly
const { command: cmdTarget, args: argsTarget } =
Platform.touch(targetFile);
const commandTarget = await osManager.prepareCommand({
command: cmdTarget,
args: argsTarget,
cwd: tempWorkspace,
env: process.env,
});
// Attempt to write to the target file directly
const { command: cmdTarget, args: argsTarget } =
Platform.touch(targetFile);
const commandTarget = await osManager.prepareCommand({
command: cmdTarget,
args: argsTarget,
cwd: tempWorkspace,
env: process.env,
});

const resultTarget = await runCommand(commandTarget);
assertResult(resultTarget, commandTarget, 'failure');
const resultTarget = await runCommand(commandTarget);
assertResult(resultTarget, commandTarget, 'failure');

// Attempt to write via the symlink
const { command: cmdLink, args: argsLink } =
Platform.touch(symlinkFile);
const commandLink = await osManager.prepareCommand({
command: cmdLink,
args: argsLink,
cwd: tempWorkspace,
env: process.env,
});
// Attempt to write via the symlink
const { command: cmdLink, args: argsLink } =
Platform.touch(symlinkFile);
const commandLink = await osManager.prepareCommand({
command: cmdLink,
args: argsLink,
cwd: tempWorkspace,
env: process.env,
});

const resultLink = await runCommand(commandLink);
assertResult(resultLink, commandLink, 'failure');
});
const resultLink = await runCommand(commandLink);
assertResult(resultLink, commandLink, 'failure');
},
);
});

describe('Governance Files', () => {
Expand Down
7 changes: 6 additions & 1 deletion packages/core/src/services/shellExecutionService.windows.integration.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { describe, it, expect } from 'vitest';
import os from 'node:os';
import { ShellExecutionService } from './shellExecutionService.js';
import { NoopSandboxManager } from './sandboxManager.js';
import { hasPowerShell7 } from '../test-utils/environment-capabilities.js';

const isWindows = os.platform() === 'win32';

Expand All @@ -21,8 +22,12 @@ const isWindows = os.platform() === 'win32';
* These tests exercise the full pipeline end-to-end. They pass when
* gemini-cli selects pwsh.exe from PATH; they fail when the pipeline
* routes through Windows PowerShell 5.1.
*
* That condition is the guard below rather than only a note here: on a
* default Windows install pwsh.exe is absent, so these would otherwise
* fail for a reason unrelated to the change under test.
*/
describe.skipIf(!isWindows)(
describe.skipIf(!isWindows || !hasPowerShell7())(
'ShellExecutionService Windows quoting (real shell)',
() => {
const baseConfig = {
Expand Down
87 changes: 87 additions & 0 deletions packages/core/src/test-utils/environment-capabilities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
/**
* @license
* Copyright 2025 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/

import * as fs from 'node:fs';
import * as os from 'node:os';
import * as path from 'node:path';

/**
* Capability probes for tests whose outcome depends on the host rather than on
* the code under test.
*
* These exist so such a test can be skipped with a reason instead of failing.
* A suite that is red for environmental reasons teaches contributors to ignore
* it, which costs more than the coverage the test provides.
*
* Prefer a capability probe to a `process.platform` check. Skipping every
* Windows host would also skip the contributors most likely to be changing
* Windows-specific behavior, whose machines can usually run these tests.
*/

let cachedCanCreateSymlinks: boolean | undefined;

/**
* Whether this host can create symbolic links.
*
* On Windows `fs.symlinkSync` needs Developer Mode or an elevated shell and
* otherwise throws `EPERM`, so a default developer machine cannot build a
* symlink fixture. This creates one real link rather than inferring from the
* platform, and caches the answer because it cannot change within a run.
*/
export function canCreateSymlinks(): boolean {
if (cachedCanCreateSymlinks !== undefined) {
return cachedCanCreateSymlinks;
}

let dir: string | undefined;
try {
dir = fs.mkdtempSync(path.join(os.tmpdir(), 'symlink-probe-'));
const target = path.join(dir, 'target');
fs.writeFileSync(target, '');
fs.symlinkSync(target, path.join(dir, 'link'));
cachedCanCreateSymlinks = true;
} catch {
cachedCanCreateSymlinks = false;
} finally {
if (dir !== undefined) {
try {
fs.rmSync(dir, { recursive: true, force: true });
} catch {
// The probe must never fail a suite it is only there to describe.
}
}
}

return cachedCanCreateSymlinks;
}

/**
* Whether PowerShell 7+ (`pwsh`) is resolvable on PATH.
*
* The Windows shell-quoting pipeline behaves differently when it falls back to
* Windows PowerShell 5.1, which is what a default Windows install provides.
*/
export function hasPowerShell7(): boolean {
if (os.platform() !== 'win32') {
return false;
}

return (process.env['PATH'] ?? '').split(path.delimiter).some((entry) => {
// Windows PATH entries containing spaces are sometimes stored quoted.
// `path.join` would keep the quote inside the path, so the lookup would
// miss a pwsh that is actually installed — and a false negative here
// skips the tests on precisely the hosts that can run them.
const unquoted = entry.replace(/^"|"$/g, '');
if (unquoted === '') {
return false;
}
try {
return fs.existsSync(path.join(unquoted, 'pwsh.exe'));
} catch {
return false;
}
});
Comment on lines +72 to +86

@gemini-code-assist gemini-code-assist Bot Aug 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

On Windows, entries in the PATH environment variable can sometimes be wrapped in double quotes (e.g., if they contain spaces). When path.join is called with a quoted entry, it produces an invalid path (e.g., "C:\\Program Files\\PowerShell\\7"\\pwsh.exe), causing fs.existsSync to return false even if PowerShell 7 is installed. Sanitizing the path entry by removing surrounding quotes ensures robust capability detection.

  return (process.env['PATH'] ?? '').split(path.delimiter).some((entry) => {
    const cleanEntry = entry.replace(/^"|"$/g, '');
    if (cleanEntry === '') {
      return false;
    }
    try {
      return fs.existsSync(path.join(cleanEntry, 'pwsh.exe'));
    } catch {
      return false;
    }
  });

}
1 change: 1 addition & 0 deletions packages/core/src/test-utils/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,5 @@
* SPDX-License-Identifier: Apache-2.0
*/

export * from './environment-capabilities.js';
export * from './mock-tool.js';
99 changes: 53 additions & 46 deletions packages/core/src/tools/at-reference-resolution.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import { StandardFileSystemService } from '../services/fileSystemService.js';
import { createMockWorkspaceContext } from '../test-utils/mockWorkspaceContext.js';
import { createMockMessageBus } from '../test-utils/mock-message-bus.js';
import { isSubpath } from '../utils/paths.js';
import { canCreateSymlinks } from '../test-utils/environment-capabilities.js';

vi.mock('../telemetry/loggers.js', () => ({
logFileOperation: vi.fn(),
Expand Down Expand Up @@ -401,53 +402,59 @@ describe('Consolidated At-Reference Path Resolution Tests (b-495551283)', () =>
).rejects.toThrow('Path not in workspace');
});

it('getCorrectedFileContent handles symlink loops gracefully', async () => {
const symlinkPath1 = path.join(tempRootDir, 'symlink1');
const symlinkPath2 = path.join(tempRootDir, 'symlink2');
await fsp.symlink(symlinkPath2, symlinkPath1);
await fsp.symlink(symlinkPath1, symlinkPath2);

const result = await getCorrectedFileContent(
mockConfigInstance,
'symlink1',
'content',
abortSignal,
);

// The utility should fail gracefully with a resolution error
expect(result.error).toBeDefined();
expect(result.error?.message).toContain('Failed to resolve path');
});

it('EditTool.getModifyContext handles symlink loops gracefully by throwing a descriptive error', async () => {
const symlinkPath1 = path.join(tempRootDir, 'symlink1');
const symlinkPath2 = path.join(tempRootDir, 'symlink2');
await fsp.symlink(symlinkPath2, symlinkPath1);
await fsp.symlink(symlinkPath1, symlinkPath2);

const editTool = new EditTool(mockConfigInstance, createMockMessageBus());
const modifyContext = editTool.getModifyContext(abortSignal);

// The getCurrentContent method should throw a path resolution error
await expect(
modifyContext.getCurrentContent({
file_path: 'symlink1',
instruction: 'read file',
old_string: '',
new_string: '',
}),
).rejects.toThrow('Failed to resolve path');
it.skipIf(!canCreateSymlinks())(
'getCorrectedFileContent handles symlink loops gracefully',
async () => {
const symlinkPath1 = path.join(tempRootDir, 'symlink1');
const symlinkPath2 = path.join(tempRootDir, 'symlink2');
await fsp.symlink(symlinkPath2, symlinkPath1);
await fsp.symlink(symlinkPath1, symlinkPath2);

const result = await getCorrectedFileContent(
mockConfigInstance,
'symlink1',
'content',
abortSignal,
);

// The getProposedContent method should throw a path resolution error
await expect(
modifyContext.getProposedContent({
file_path: 'symlink1',
instruction: 'read file',
old_string: '',
new_string: '',
}),
).rejects.toThrow('Failed to resolve path');
});
// The utility should fail gracefully with a resolution error
expect(result.error).toBeDefined();
expect(result.error?.message).toContain('Failed to resolve path');
},
);

it.skipIf(!canCreateSymlinks())(
'EditTool.getModifyContext handles symlink loops gracefully by throwing a descriptive error',
async () => {
const symlinkPath1 = path.join(tempRootDir, 'symlink1');
const symlinkPath2 = path.join(tempRootDir, 'symlink2');
await fsp.symlink(symlinkPath2, symlinkPath1);
await fsp.symlink(symlinkPath1, symlinkPath2);

const editTool = new EditTool(mockConfigInstance, createMockMessageBus());
const modifyContext = editTool.getModifyContext(abortSignal);

// The getCurrentContent method should throw a path resolution error
await expect(
modifyContext.getCurrentContent({
file_path: 'symlink1',
instruction: 'read file',
old_string: '',
new_string: '',
}),
).rejects.toThrow('Failed to resolve path');

// The getProposedContent method should throw a path resolution error
await expect(
modifyContext.getProposedContent({
file_path: 'symlink1',
instruction: 'read file',
old_string: '',
new_string: '',
}),
).rejects.toThrow('Failed to resolve path');
},
);

it('getCorrectedFileContent successfully resolves paths in Plan Mode', async () => {
const plansDir = path.join(tempRootDir, '.plans');
Expand Down
Loading