Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
5746424
feat(core): enhance shell command validation and add core tools allow…
galdawave Apr 20, 2026
a56f3f6
Merge branch 'main' into galzahavi/fix/coretools
DavidAPierce Apr 21, 2026
8e4882e
Merge branch 'main' into galzahavi/fix/coretools
galdawave Apr 21, 2026
8c7b837
addressed the failure in shell-substitution.test.ts
DavidAPierce Apr 21, 2026
cfbdbe6
Merge branch 'main' into galzahavi/fix/coretools
DavidAPierce Apr 21, 2026
d0391b3
Merge branch 'main' into galzahavi/fix/coretools
DavidAPierce Apr 21, 2026
ffd9f9f
test(core): mock getShellConfiguration in shell substitution tests
DavidAPierce Apr 21, 2026
d115821
Merge branch 'main' into galzahavi/fix/coretools
DavidAPierce Apr 22, 2026
579ebe0
fix failing unit test for windows.
DavidAPierce Apr 22, 2026
c2984f9
Merge branch 'main' into galzahavi/fix/coretools
DavidAPierce Apr 22, 2026
add751e
Modifying strategy around planning modes and fixing ask handling
kschaab Apr 23, 2026
ad6b710
Merge branch 'main' into galzahavi/fix/coretools
kschaab Apr 23, 2026
cc6d7a4
Merge branch 'main' into galzahavi/fix/coretools
DavidAPierce Apr 23, 2026
5331853
Add a deny policy for tool calls looser than explicetly allowed tool …
DavidAPierce Apr 23, 2026
1734a03
set aggregateDecision to ASK_USER if any result dictates asking the u…
DavidAPierce Apr 23, 2026
7bb4fab
Merge branch 'main' into galzahavi/fix/coretools
DavidAPierce Apr 23, 2026
ad56ab5
respect confirmation required setting when set by users in updated po…
DavidAPierce Apr 23, 2026
a5bd5f4
Merge remote-tracking branch 'refs/remotes/origin/galzahavi/fix/coret…
DavidAPierce Apr 23, 2026
4c6ec40
regenerate settings schema and update configuration.md to include too…
DavidAPierce Apr 23, 2026
2e21f85
add trusted workspace setting to evals
DavidAPierce Apr 23, 2026
bf768cb
set GEMINI_CLI_TRUST_WORKSPACE in evals test helper and revert yml ch…
emilyhedlund Apr 23, 2026
173d4d6
disable the memoryV2 experiment in eval tests since they don't play n…
DavidAPierce Apr 23, 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
52 changes: 42 additions & 10 deletions packages/core/src/policy/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ export const ADMIN_POLICY_TIER = 5;
export const MCP_EXCLUDED_PRIORITY = USER_POLICY_TIER + 0.9;
export const EXCLUDE_TOOLS_FLAG_PRIORITY = USER_POLICY_TIER + 0.4;
export const ALLOWED_TOOLS_FLAG_PRIORITY = USER_POLICY_TIER + 0.3;
export const CORE_TOOLS_FLAG_PRIORITY = USER_POLICY_TIER + 0.25;
export const TRUSTED_MCP_SERVER_PRIORITY = USER_POLICY_TIER + 0.2;
export const ALLOWED_MCP_SERVER_PRIORITY = USER_POLICY_TIER + 0.1;

Expand Down Expand Up @@ -434,10 +435,12 @@ export async function createPolicyEngineConfig(
}
}

// Tools that are explicitly allowed in the settings.
// Priority: ALLOWED_TOOLS_FLAG_PRIORITY (user tier - explicit temporary allows)
if (settings.tools?.allowed) {
for (const tool of settings.tools.allowed) {
const mapToolsToRules = (
tools: string[],
priority: number,
source: string,
) => {
for (const tool of tools) {
// Check for legacy format: toolName(args)
const match = tool.match(/^([a-zA-Z0-9_-]+)\((.*)\)$/);
if (match) {
Expand All @@ -455,9 +458,9 @@ export async function createPolicyEngineConfig(
rules.push({
toolName,
decision: PolicyDecision.ALLOW,
priority: ALLOWED_TOOLS_FLAG_PRIORITY,
priority,
argsPattern: new RegExp(pattern),
source: 'Settings (Tools Allowed)',
source,
});
}
}
Expand All @@ -467,8 +470,8 @@ export async function createPolicyEngineConfig(
rules.push({
toolName,
decision: PolicyDecision.ALLOW,
priority: ALLOWED_TOOLS_FLAG_PRIORITY,
source: 'Settings (Tools Allowed)',
priority,
source,
});
}
} else {
Expand All @@ -479,11 +482,40 @@ export async function createPolicyEngineConfig(
rules.push({
toolName,
decision: PolicyDecision.ALLOW,
priority: ALLOWED_TOOLS_FLAG_PRIORITY,
source: 'Settings (Tools Allowed)',
priority,
source,
});
}
}
};

// Tools that are explicitly allowed in the settings.
// Priority: ALLOWED_TOOLS_FLAG_PRIORITY (user tier - explicit temporary allows)
if (settings.tools?.allowed) {
mapToolsToRules(
settings.tools.allowed,
ALLOWED_TOOLS_FLAG_PRIORITY,
'Settings (Tools Allowed)',
);
}

// Core tools that are restricted in the settings.
// Priority: CORE_TOOLS_FLAG_PRIORITY (user tier - core tool allowlist)
if (settings.tools?.core) {
mapToolsToRules(
settings.tools.core,
CORE_TOOLS_FLAG_PRIORITY,
'Settings (Core Tools)',
);

// If core tools are restricted, we should add a default DENY rule for everything else
// at a slightly lower priority than the explicit allows.
rules.push({
toolName: '*',
decision: PolicyDecision.DENY,
priority: CORE_TOOLS_FLAG_PRIORITY - 0.01,
source: 'Settings (Core Tools Allowlist Enforcement)',
});
Comment on lines +555 to +561

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

The implementation currently defaults to a DENY decision when tools.core is configured. Based on the repository rules, the default policy decision for tool calls should be ALLOW until an approval mechanism is in place, as a DENY default is considered too restrictive. Please update the decision to ALLOW and ensure the priority is correctly set to avoid unintentionally overriding other policies.

Suggested change
rules.push({
toolName: '*',
decision: PolicyDecision.DENY,
priority: CORE_TOOLS_FLAG_PRIORITY - 0.01,
source: 'Settings (Core Tools Allowlist Enforcement)',
});
rules.push({
toolName: '*',
decision: PolicyDecision.ALLOW,
priority: USER_POLICY_TIER + 0.05,
source: 'Settings (Core Tools Allowlist Enforcement)',
});
References
  1. The default policy decision for tool calls should be ALLOW until a mechanism for wiring up approvals is integrated, as a DENY default would be too restrictive without an approval flow.

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.

This comment seems to be ignoring the secure by default methodology being implemented when core tools are restricted. I don't think that this is a valid concern from the bot's part, but if you think this is correct, feel free to address the above comment.

}

// MCP servers that are trusted in the settings.
Expand Down
76 changes: 76 additions & 0 deletions packages/core/src/policy/core-tools-mapping.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
/**
* @license
* Copyright 2026 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/

import { describe, it, expect } from 'vitest';
import { createPolicyEngineConfig } from './config.js';
import { PolicyEngine } from './policy-engine.js';
import { PolicyDecision, ApprovalMode } from './types.js';

describe('PolicyEngine - Core Tools Mapping', () => {
it('should allow tools explicitly listed in settings.tools.core', async () => {
const settings = {
tools: {
core: ['run_shell_command(ls)', 'run_shell_command(git status)'],
},
};

const config = await createPolicyEngineConfig(
settings,
ApprovalMode.DEFAULT,
undefined,
true, // interactive
);

const engine = new PolicyEngine(config);

// Test simple tool name
const result1 = await engine.check(
{ name: 'run_shell_command', args: { command: 'ls' } },
undefined,
);
expect(result1.decision).toBe(PolicyDecision.ALLOW);

// Test tool name with args
const result2 = await engine.check(
{ name: 'run_shell_command', args: { command: 'git status' } },
undefined,
);
expect(result2.decision).toBe(PolicyDecision.ALLOW);

// Test tool not in core list
const result3 = await engine.check(
{ name: 'run_shell_command', args: { command: 'npm test' } },
undefined,
);
// Should be DENIED because of strict allowlist
expect(result3.decision).toBe(PolicyDecision.DENY);
});

it('should allow tools in tools.core even if they are restricted by default policies', async () => {
// By default run_shell_command is ASK_USER.
// Putting it in tools.core should make it ALLOW.
const settings = {
tools: {
core: ['run_shell_command'],
},
};

const config = await createPolicyEngineConfig(
settings,
ApprovalMode.DEFAULT,
undefined,
true,
);

const engine = new PolicyEngine(config);

const result = await engine.check(
{ name: 'run_shell_command', args: { command: 'any command' } },
undefined,
);
expect(result.decision).toBe(PolicyDecision.ALLOW);
});
});
36 changes: 34 additions & 2 deletions packages/core/src/policy/policy-engine.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,35 @@ vi.mock('../utils/shell-utils.js', async (importOriginal) => {
}
return [command];
}),
parseCommandDetails: vi.fn().mockImplementation((command: string) => {
// Basic mock implementation for PolicyEngine test needs
const commands = command.includes('&&')
? command.split('&&').map((c) => c.trim())
: [command.trim()];

// Detect $(...) or `...` and add as sub-commands for recursion tests
const subCommands = [...commands];
for (const cmd of commands) {
const subMatch = cmd.match(/\$\((.*)\)/) || cmd.match(/`(.*)`/);
if (subMatch?.[1]) {
subCommands.push(subMatch[1].trim());
}
}

return {
details: subCommands.map((c, i) => ({
name: c.split(' ')[0],
text: c,
startIndex: i === 0 ? 0 : -1, // Simple root indication
})),
hasError: false,
};
}),
stripShellWrapper: vi.fn().mockImplementation((command: string) => {
// Simple mock for stripping wrappers
const match = command.match(/^(?:bash|sh|zsh)\s+-c\s+["'](.*)["']$/i);
return match ? match[1] : command;
}),
hasRedirection: vi.fn().mockImplementation(
(command: string) =>
// Simple mock: true if '>' is present, unless it looks like "-> arrow"
Expand Down Expand Up @@ -1862,7 +1891,6 @@ describe('PolicyEngine', () => {
});

it('should return ASK_USER in non-YOLO mode if shell command parsing fails', async () => {
const { splitCommands } = await import('../utils/shell-utils.js');
const rules: PolicyRule[] = [
{
toolName: 'run_shell_command',
Expand All @@ -1877,7 +1905,11 @@ describe('PolicyEngine', () => {
});

// Simulate parsing failure
vi.mocked(splitCommands).mockReturnValueOnce([]);
const { parseCommandDetails } = await import('../utils/shell-utils.js');
vi.mocked(parseCommandDetails).mockReturnValueOnce({
details: [],
hasError: true,
});

const result = await engine.check(
{ name: 'run_shell_command', args: { command: 'complex command' } },
Expand Down
Loading
Loading