MediaMetz/gemini-cli
1
0
Fork 0
mirror of https://github.com/google-gemini/gemini-cli.git synced 2026-05-12 21:03:05 -07:00
Code Issues Packages Projects Releases Wiki Activity

fix(review): address review findings with proper type guards and clean formatting

Browse Source
This commit is contained in:
Spencer
2026-03-04 04:36:54 +00:00
parent cf08dcf6ac
commit 9888f8afa7
18 changed files with 199 additions and 333 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/cli/settings.md
+1
View File
@@ -125,6 +125,7 @@ they appear in the UI.
| ------------------------------------- | ----------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | ------- |
| Disable YOLO Mode | `security.disableYoloMode` | Disable YOLO mode, even if enabled by a flag. | `false` |
| Allow Permanent Tool Approval | `security.enablePermanentToolApproval` | Enable the "Allow for all future sessions" option in tool confirmation dialogs. | `false` |
| Auto-add to Policy by Default | `security.autoAddToPolicyByDefault` | When enabled, the "Allow for all future sessions" option becomes the default choice for low-risk tools in trusted workspaces. | `true` |
| Blocks extensions from Git | `security.blockGitExtensions` | Blocks installing and loading extensions from Git. | `false` |
| Extension Source Regex Allowlist | `security.allowedExtensions` | List of Regex patterns for allowed extensions. If nonempty, only extensions that match the patterns in this list are allowed. Overrides the blockGitExtensions setting. | `[]` |
| Folder Trust | `security.folderTrust.enabled` | Setting to track whether Folder trust is enabled. | `true` |
packages/cli/src/ui/components/messages/ToolConfirmationMessage.test.tsx
+3 -35
View File
@@ -411,7 +411,7 @@ describe('ToolConfirmationMessage', () => {
unmount();
});
it('should show "Allow for all future sessions" when setting is true', async () => {
it('should show "Allow for all future sessions" when trusted', async () => {
const mockConfig = {
isTrustedFolder: () => true,
getIdeMode: () => false,
@@ -434,41 +434,9 @@ describe('ToolConfirmationMessage', () => {
);
await waitUntilReady();
expect(lastFrame()).toContain('Allow for all future sessions');
unmount();
});
it('should default to "Allow for all future sessions" when autoAddToPolicyByDefault is true', async () => {
const mockConfig = {
isTrustedFolder: () => true,
getIdeMode: () => false,
} as unknown as Config;
const { lastFrame, waitUntilReady, unmount } = renderWithProviders(
<ToolConfirmationMessage
callId="test-call-id"
confirmationDetails={editConfirmationDetails}
config={mockConfig}
getPreferredEditor={vi.fn()}
availableTerminalHeight={30}
terminalWidth={80}
/>,
{
settings: createMockSettings({
security: {
enablePermanentToolApproval: true,
autoAddToPolicyByDefault: true,
},
}),
},
);
await waitUntilReady();
const output = lastFrame();
// In Ink, the selected item is usually highlighted with a cursor or different color.
// We can't easily check colors in text output, but we can verify it's NOT the first option
// if we could see the selection indicator.
// Instead, we'll verify the snapshot which should show the selection.
expect(output).toContain('future sessions');
// Verify it is the default selection (matching the indicator in the snapshot)
expect(output).toMatchSnapshot();
unmount();
});
packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx
+8 -11
View File
@@ -245,7 +245,7 @@ export const ToolConfirmationMessage: React.FC<
});
if (allowPermanentApproval) {
options.push({
label: 'Allow for all future sessions',
label: `Allow for this file in all future sessions`,
value: ToolConfirmationOutcome.ProceedAlwaysAndSave,
key: 'Allow for all future sessions',
});
@@ -281,7 +281,7 @@ export const ToolConfirmationMessage: React.FC<
});
if (allowPermanentApproval) {
options.push({
label: `Allow for all future sessions`,
label: `Allow this command for all future sessions`,
value: ToolConfirmationOutcome.ProceedAlwaysAndSave,
key: `Allow for all future sessions`,
});
@@ -401,19 +401,16 @@ export const ToolConfirmationMessage: React.FC<
const options = getOptions();
let initialIndex = 0;
if (
settings.merged.security.autoAddToPolicyByDefault &&
isTrustedFolder &&
allowPermanentApproval
) {
if (isTrustedFolder && allowPermanentApproval) {
const isSafeToPersist =
confirmationDetails.type === 'info' ||
confirmationDetails.type === 'edit' ||
(confirmationDetails.type === 'exec' &&
confirmationDetails.rootCommand) ||
confirmationDetails.type === 'mcp';
if (isSafeToPersist) {
if (
isSafeToPersist &&
settings.merged.security.autoAddToPolicyByDefault
) {
const alwaysAndSaveIndex = options.findIndex(
(o) => o.value === ToolConfirmationOutcome.ProceedAlwaysAndSave,
);
@@ -671,9 +668,9 @@ export const ToolConfirmationMessage: React.FC<
mcpToolDetailsText,
expandDetailsHintKey,
getPreferredEditor,
settings.merged.security.autoAddToPolicyByDefault,
isTrustedFolder,
allowPermanentApproval,
settings.merged.security.autoAddToPolicyByDefault,
]);
packages/cli/src/ui/components/messages/__snapshots__/ToolConfirmationMessage.test.tsx.snap
+2 -2
View File
@@ -1,6 +1,6 @@
// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html
exports[`ToolConfirmationMessage > enablePermanentToolApproval setting > should default to "Allow for all future sessions" when autoAddToPolicyByDefault is true 1`] = `
exports[`ToolConfirmationMessage > enablePermanentToolApproval setting > should show "Allow for all future sessions" when trusted 1`] = `
"╭──────────────────────────────────────────────────────────────────────────────╮
│ │
│ No changes detected. │
@@ -10,7 +10,7 @@ Apply this change?
1. Allow once
2. Allow for this session
● 3. Allow for all future sessions
● 3. Allow for this file in all future sessions
4. Modify with external editor
5. No, suggest changes (esc)
"
packages/core/src/agents/browser/mcpToolWrapper.ts
+2 -2
View File
@@ -70,7 +70,7 @@ class McpToolInvocation extends BaseToolInvocation<
};
}
protected override getPolicyUpdateOptions(
override getPolicyUpdateOptions(
_outcome: ToolConfirmationOutcome,
): PolicyUpdateOptions | undefined {
return {
@@ -177,7 +177,7 @@ class TypeTextInvocation extends BaseToolInvocation<
};
}
protected override getPolicyUpdateOptions(
override getPolicyUpdateOptions(
_outcome: ToolConfirmationOutcome,
): PolicyUpdateOptions | undefined {
return {
packages/core/src/policy/config.ts
+11 -14
View File
@@ -19,7 +19,12 @@ import {
} from './types.js';
import type { PolicyEngine } from './policy-engine.js';
import { loadPoliciesFromToml, type PolicyFileError } from './toml-loader.js';
import { buildArgsPatterns, isSafeRegExp } from './utils.js';
import {
buildArgsPatterns,
isSafeRegExp,
ALWAYS_ALLOW_PRIORITY,
getAlwaysAllowPriorityFraction,
} from './utils.js';
import toml from '@iarna/toml';
import {
MessageBusType,
@@ -47,12 +52,6 @@ export const USER_POLICY_TIER = 4;
export const ADMIN_POLICY_TIER = 5;
// Specific priority offsets and derived priorities for dynamic/settings rules.
// These are added to the tier base (e.g., USER_POLICY_TIER).
// Workspace tier (3) + high priority (950/1000) = ALWAYS_ALLOW_PRIORITY
// This ensures user "always allow" selections are high priority
// within the workspace tier but still lose to user/admin policies.
export const ALWAYS_ALLOW_PRIORITY = WORKSPACE_POLICY_TIER + 0.95;
export const MCP_EXCLUDED_PRIORITY = USER_POLICY_TIER + 0.9;
export const EXCLUDE_TOOLS_FLAG_PRIORITY = USER_POLICY_TIER + 0.4;
@@ -563,21 +562,19 @@ export function createPolicyUpdater(
}
// Create new rule object
const newRule: TomlRule = {};
const newRule: TomlRule = {
decision: 'allow',
priority: getAlwaysAllowPriorityFraction(),
};
if (message.mcpName) {
newRule.mcpName = message.mcpName;
// Extract simple tool name
const simpleToolName = toolName.startsWith(`${message.mcpName}__`)
newRule.toolName = toolName.startsWith(`${message.mcpName}__`)
? toolName.slice(message.mcpName.length + 2)
: toolName;
newRule.toolName = simpleToolName;
newRule.decision = 'allow';
newRule.priority = 200;
} else {
newRule.toolName = toolName;
newRule.decision = 'allow';
newRule.priority = 100;
}
if (message.commandPrefix) {
packages/core/src/policy/persistence.test.ts
+86 -151
View File
@@ -4,25 +4,20 @@
* SPDX-License-Identifier: Apache-2.0
*/
import {
describe,
it,
expect,
vi,
beforeEach,
afterEach,
type Mock,
} from 'vitest';
import * as fs from 'node:fs/promises';
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
import * as path from 'node:path';
import { createPolicyUpdater, ALWAYS_ALLOW_PRIORITY } from './config.js';
import { createPolicyUpdater } from './config.js';
import { ALWAYS_ALLOW_PRIORITY } from './utils.js';
import { PolicyEngine } from './policy-engine.js';
import { MessageBus } from '../confirmation-bus/message-bus.js';
import { MessageBusType } from '../confirmation-bus/types.js';
import { Storage, AUTO_SAVED_POLICY_FILENAME } from '../config/storage.js';
import { ApprovalMode } from './types.js';
import { vol, fs as memfs } from 'memfs';
// Use memfs for all fs operations in this test
vi.mock('node:fs/promises', () => import('memfs').then((m) => m.fs.promises));
vi.mock('node:fs/promises');
vi.mock('../config/storage.js');
describe('createPolicyUpdater', () => {
@@ -31,6 +26,7 @@ describe('createPolicyUpdater', () => {
let mockStorage: Storage;
beforeEach(() => {
vol.reset();
policyEngine = new PolicyEngine({
rules: [],
checkers: [],
@@ -48,185 +44,141 @@ describe('createPolicyUpdater', () => {
it('should persist policy when persist flag is true', async () => {
createPolicyUpdater(policyEngine, messageBus, mockStorage);
const userPoliciesDir = '/mock/user/.gemini/policies';
const policyFile = path.join(userPoliciesDir, AUTO_SAVED_POLICY_FILENAME);
const policyFile = '/mock/user/.gemini/policies/auto-saved.toml';
vi.spyOn(mockStorage, 'getAutoSavedPolicyPath').mockReturnValue(policyFile);
(fs.mkdir as unknown as Mock).mockResolvedValue(undefined);
(fs.readFile as unknown as Mock).mockRejectedValue(
new Error('File not found'),
); // Simulate new file
const mockFileHandle = {
writeFile: vi.fn().mockResolvedValue(undefined),
close: vi.fn().mockResolvedValue(undefined),
};
(fs.open as unknown as Mock).mockResolvedValue(mockFileHandle);
(fs.rename as unknown as Mock).mockResolvedValue(undefined);
const toolName = 'test_tool';
await messageBus.publish({
type: MessageBusType.UPDATE_POLICY,
toolName,
toolName: 'test_tool',
persist: true,
});
// Wait for async operations (microtasks)
await new Promise((resolve) => setTimeout(resolve, 0));
// Policy updater handles persistence asynchronously
await new Promise((resolve) => setTimeout(resolve, 100));
expect(fs.mkdir).toHaveBeenCalledWith(userPoliciesDir, {
recursive: true,
});
const fileExists = memfs.existsSync(policyFile);
expect(fileExists).toBe(true);
expect(fs.open).toHaveBeenCalledWith(expect.stringMatching(/\.tmp$/), 'wx');
// Check written content
const expectedContent = expect.stringContaining(`toolName = "test_tool"`);
expect(mockFileHandle.writeFile).toHaveBeenCalledWith(
expectedContent,
'utf-8',
);
expect(fs.rename).toHaveBeenCalledWith(
expect.stringMatching(/\.tmp$/),
policyFile,
const content = memfs.readFileSync(policyFile, 'utf-8') as string;
expect(content).toContain('toolName = "test_tool"');
expect(content).toContain('decision = "allow"');
const expectedPriority = Math.round(
(ALWAYS_ALLOW_PRIORITY - Math.floor(ALWAYS_ALLOW_PRIORITY)) * 1000,
);
expect(content).toContain(`priority = ${expectedPriority}`);
});
it('should not persist policy when persist flag is false or undefined', async () => {
createPolicyUpdater(policyEngine, messageBus, mockStorage);
const policyFile = '/mock/user/.gemini/policies/auto-saved.toml';
vi.spyOn(mockStorage, 'getAutoSavedPolicyPath').mockReturnValue(policyFile);
await messageBus.publish({
type: MessageBusType.UPDATE_POLICY,
toolName: 'test_tool',
});
await new Promise((resolve) => setTimeout(resolve, 0));
await new Promise((resolve) => setTimeout(resolve, 100));
expect(fs.writeFile).not.toHaveBeenCalled();
expect(fs.rename).not.toHaveBeenCalled();
expect(memfs.existsSync(policyFile)).toBe(false);
});
it('should persist policy with commandPrefix when provided', async () => {
it('should append to existing policy file', async () => {
createPolicyUpdater(policyEngine, messageBus, mockStorage);
const userPoliciesDir = '/mock/user/.gemini/policies';
const policyFile = path.join(userPoliciesDir, AUTO_SAVED_POLICY_FILENAME);
const policyFile = '/mock/user/.gemini/policies/auto-saved.toml';
vi.spyOn(mockStorage, 'getAutoSavedPolicyPath').mockReturnValue(policyFile);
(fs.mkdir as unknown as Mock).mockResolvedValue(undefined);
(fs.readFile as unknown as Mock).mockRejectedValue(
new Error('File not found'),
);
const mockFileHandle = {
writeFile: vi.fn().mockResolvedValue(undefined),
close: vi.fn().mockResolvedValue(undefined),
};
(fs.open as unknown as Mock).mockResolvedValue(mockFileHandle);
(fs.rename as unknown as Mock).mockResolvedValue(undefined);
const toolName = 'run_shell_command';
const commandPrefix = 'git status';
const existingContent =
'[[rule]]\ntoolName = "existing_tool"\ndecision = "allow"\n';
const dir = path.dirname(policyFile);
memfs.mkdirSync(dir, { recursive: true });
memfs.writeFileSync(policyFile, existingContent);
await messageBus.publish({
type: MessageBusType.UPDATE_POLICY,
toolName,
toolName: 'new_tool',
persist: true,
commandPrefix,
});
await new Promise((resolve) => setTimeout(resolve, 0));
await new Promise((resolve) => setTimeout(resolve, 100));
// In-memory rule check (unchanged)
const rules = policyEngine.getRules();
const addedRule = rules.find((r) => r.toolName === toolName);
expect(addedRule).toBeDefined();
expect(addedRule?.priority).toBe(ALWAYS_ALLOW_PRIORITY);
expect(addedRule?.argsPattern).toEqual(
new RegExp(`"command":"git\\ status(?:[\\s"]|\\\\")`),
);
// Verify file written
expect(fs.open).toHaveBeenCalledWith(expect.stringMatching(/\.tmp$/), 'wx');
expect(mockFileHandle.writeFile).toHaveBeenCalledWith(
expect.stringContaining(`commandPrefix = "git status"`),
'utf-8',
);
const content = memfs.readFileSync(policyFile, 'utf-8') as string;
expect(content).toContain('toolName = "existing_tool"');
expect(content).toContain('toolName = "new_tool"');
});
it('should persist policy with mcpName and toolName when provided', async () => {
it('should handle toml with multiple rules correctly', async () => {
createPolicyUpdater(policyEngine, messageBus, mockStorage);
const userPoliciesDir = '/mock/user/.gemini/policies';
const policyFile = path.join(userPoliciesDir, AUTO_SAVED_POLICY_FILENAME);
const policyFile = '/mock/user/.gemini/policies/auto-saved.toml';
vi.spyOn(mockStorage, 'getAutoSavedPolicyPath').mockReturnValue(policyFile);
(fs.mkdir as unknown as Mock).mockResolvedValue(undefined);
(fs.readFile as unknown as Mock).mockRejectedValue(
new Error('File not found'),
);
const mockFileHandle = {
writeFile: vi.fn().mockResolvedValue(undefined),
close: vi.fn().mockResolvedValue(undefined),
};
(fs.open as unknown as Mock).mockResolvedValue(mockFileHandle);
(fs.rename as unknown as Mock).mockResolvedValue(undefined);
const existingContent = `
[[rule]]
toolName = "tool1"
decision = "allow"
const mcpName = 'my-jira-server';
const simpleToolName = 'search';
const toolName = `${mcpName}__${simpleToolName}`;
[[rule]]
toolName = "tool2"
decision = "deny"
`;
const dir = path.dirname(policyFile);
memfs.mkdirSync(dir, { recursive: true });
memfs.writeFileSync(policyFile, existingContent);
await messageBus.publish({
type: MessageBusType.UPDATE_POLICY,
toolName,
toolName: 'tool3',
persist: true,
mcpName,
});
await new Promise((resolve) => setTimeout(resolve, 0));
await new Promise((resolve) => setTimeout(resolve, 100));
// Verify file written
expect(fs.open).toHaveBeenCalledWith(expect.stringMatching(/\.tmp$/), 'wx');
const writeCall = mockFileHandle.writeFile.mock.calls[0];
const writtenContent = writeCall[0] as string;
expect(writtenContent).toContain(`mcpName = "${mcpName}"`);
expect(writtenContent).toContain(`toolName = "${simpleToolName}"`);
expect(writtenContent).toContain('priority = 200');
const content = memfs.readFileSync(policyFile, 'utf-8') as string;
expect(content).toContain('toolName = "tool1"');
expect(content).toContain('toolName = "tool2"');
expect(content).toContain('toolName = "tool3"');
});
it('should escape special characters in toolName and mcpName', async () => {
it('should include argsPattern if provided', async () => {
createPolicyUpdater(policyEngine, messageBus, mockStorage);
const userPoliciesDir = '/mock/user/.gemini/policies';
const policyFile = path.join(userPoliciesDir, AUTO_SAVED_POLICY_FILENAME);
const policyFile = '/mock/user/.gemini/policies/auto-saved.toml';
vi.spyOn(mockStorage, 'getAutoSavedPolicyPath').mockReturnValue(policyFile);
(fs.mkdir as unknown as Mock).mockResolvedValue(undefined);
(fs.readFile as unknown as Mock).mockRejectedValue(
new Error('File not found'),
);
const mockFileHandle = {
writeFile: vi.fn().mockResolvedValue(undefined),
close: vi.fn().mockResolvedValue(undefined),
};
(fs.open as unknown as Mock).mockResolvedValue(mockFileHandle);
(fs.rename as unknown as Mock).mockResolvedValue(undefined);
const mcpName = 'my"jira"server';
const toolName = `my"jira"server__search"tool"`;
await messageBus.publish({
type: MessageBusType.UPDATE_POLICY,
toolName,
toolName: 'test_tool',
persist: true,
mcpName,
argsPattern: '^foo.*$',
});
await new Promise((resolve) => setTimeout(resolve, 0));
await new Promise((resolve) => setTimeout(resolve, 100));
expect(fs.open).toHaveBeenCalledWith(expect.stringMatching(/\.tmp$/), 'wx');
const writeCall = mockFileHandle.writeFile.mock.calls[0];
const writtenContent = writeCall[0] as string;
const content = memfs.readFileSync(policyFile, 'utf-8') as string;
expect(content).toContain('argsPattern = "^foo.*$"');
});
// Verify escaping - should be valid TOML
it('should include mcpName if provided', async () => {
createPolicyUpdater(policyEngine, messageBus, mockStorage);
const policyFile = '/mock/user/.gemini/policies/auto-saved.toml';
vi.spyOn(mockStorage, 'getAutoSavedPolicyPath').mockReturnValue(policyFile);
await messageBus.publish({
type: MessageBusType.UPDATE_POLICY,
toolName: 'search"tool"',
persist: true,
mcpName: 'my"jira"server',
});
await new Promise((resolve) => setTimeout(resolve, 100));
const writtenContent = memfs.readFileSync(policyFile, 'utf-8') as string;
// Verify escaping - should be valid TOML and contain the values
// Note: @iarna/toml optimizes for shortest representation, so it may use single quotes 'foo"bar'
// instead of "foo\"bar\"" if there are no single quotes in the string.
try {
@@ -253,18 +205,6 @@ describe('createPolicyUpdater', () => {
vi.spyOn(mockStorage, 'getWorkspaceAutoSavedPolicyPath').mockReturnValue(
policyFile,
);
(fs.mkdir as unknown as Mock).mockResolvedValue(undefined);
(fs.readFile as unknown as Mock).mockRejectedValue(
new Error('File not found'),
);
(fs.copyFile as unknown as Mock).mockResolvedValue(undefined);
const mockFileHandle = {
writeFile: vi.fn().mockResolvedValue(undefined),
close: vi.fn().mockResolvedValue(undefined),
};
(fs.open as unknown as Mock).mockResolvedValue(mockFileHandle);
(fs.rename as unknown as Mock).mockResolvedValue(undefined);
await messageBus.publish({
type: MessageBusType.UPDATE_POLICY,
@@ -273,15 +213,10 @@ describe('createPolicyUpdater', () => {
persistScope: 'workspace',
});
await new Promise((resolve) => setTimeout(resolve, 0));
await new Promise((resolve) => setTimeout(resolve, 100));
expect(mockStorage.getWorkspaceAutoSavedPolicyPath).toHaveBeenCalled();
expect(fs.mkdir).toHaveBeenCalledWith(workspacePoliciesDir, {
recursive: true,
});
expect(fs.rename).toHaveBeenCalledWith(
expect.stringMatching(/\.tmp$/),
policyFile,
);
expect(memfs.existsSync(policyFile)).toBe(true);
const content = memfs.readFileSync(policyFile, 'utf-8') as string;
expect(content).toContain('toolName = "test_tool"');
});
});
packages/core/src/policy/policy-updater.test.ts
+2 -1
View File
@@ -6,7 +6,8 @@
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
import * as fs from 'node:fs/promises';
import { createPolicyUpdater, ALWAYS_ALLOW_PRIORITY } from './config.js';
import { createPolicyUpdater } from './config.js';
import { ALWAYS_ALLOW_PRIORITY } from './utils.js';
import { PolicyEngine } from './policy-engine.js';
import { MessageBus } from '../confirmation-bus/message-bus.js';
import { MessageBusType } from '../confirmation-bus/types.js';
packages/core/src/policy/utils.ts
+26
View File
@@ -4,6 +4,32 @@
* SPDX-License-Identifier: Apache-2.0
*/
/**
* Priority used for user-defined "Always allow" rules.
* This is above extension rules but below user-defined TOML rules.
*/
export const ALWAYS_ALLOW_PRIORITY = 3.95;
/**
* Calculates a unique priority within the ALWAYS_ALLOW_PRIORITY tier.
* It uses the fractional part as a base and adds a small offset.
*/
export function getAlwaysAllowPriority(offset: number): number {
const base = Math.floor(ALWAYS_ALLOW_PRIORITY);
const fraction = ALWAYS_ALLOW_PRIORITY - base;
// Use a precision of 3 decimal places for the offset
return base + fraction + offset / 1000;
}
/**
* Returns the fractional priority of ALWAYS_ALLOW_PRIORITY scaled to 1000.
*/
export function getAlwaysAllowPriorityFraction(): number {
return Math.round(
(ALWAYS_ALLOW_PRIORITY - Math.floor(ALWAYS_ALLOW_PRIORITY)) * 1000,
);
}
/**
* Escapes a string for use in a regular expression.
*/
packages/core/src/scheduler/policy.test.ts
+7 -4
View File
@@ -559,12 +559,15 @@ describe('policy.ts', () => {
publish: vi.fn(),
} as unknown as Mocked<MessageBus>;
const tool = { name: 'write_file' } as AnyDeclarativeTool;
const details = {
const details: SerializableConfirmationDetails = {
type: 'edit',
filePath: 'src/foo.ts',
title: 'Edit',
onConfirm: vi.fn(),
} as unknown as SerializableConfirmationDetails;
filePath: 'src/foo.ts',
fileName: 'foo.ts',
fileDiff: '--- foo.ts\n+++ foo.ts\n@@ -1 +1 @@\n-old\n+new',
originalContent: 'old',
newContent: 'new',
};
await updatePolicy(
tool,
packages/core/src/scheduler/policy.ts
+1 -14
View File
@@ -178,21 +178,8 @@ async function handleStandardPolicyUpdate(
outcome === ToolConfirmationOutcome.ProceedAlways ||
outcome === ToolConfirmationOutcome.ProceedAlwaysAndSave
) {
interface ToolInvocationWithOptions {
getPolicyUpdateOptions(
outcome: ToolConfirmationOutcome,
): PolicyUpdateOptions | undefined;
}
/* eslint-disable @typescript-eslint/no-unsafe-type-assertion */
const options: PolicyUpdateOptions =
typeof (toolInvocation as unknown as ToolInvocationWithOptions)
?.getPolicyUpdateOptions === 'function'
? (
toolInvocation as unknown as ToolInvocationWithOptions
).getPolicyUpdateOptions(outcome) || {}
: {};
/* eslint-enable @typescript-eslint/no-unsafe-type-assertion */
toolInvocation?.getPolicyUpdateOptions?.(outcome) || {};
if (!options.commandPrefix && confirmationDetails?.type === 'exec') {
options.commandPrefix = confirmationDetails.rootCommands;
packages/core/src/tools/edit.ts
+31 -36
View File
@@ -46,7 +46,6 @@ import {
logEditCorrectionEvent,
} from '../telemetry/loggers.js';
import { correctPath } from '../utils/pathCorrector.js';
import {
EDIT_TOOL_NAME,
READ_FILE_TOOL_NAME,
@@ -444,6 +443,8 @@ class EditToolInvocation
extends BaseToolInvocation<EditToolParams, ToolResult>
implements ToolInvocation<EditToolParams, ToolResult>
{
private readonly resolvedPath: string;
constructor(
private readonly config: Config,
params: EditToolParams,
@@ -452,13 +453,17 @@ class EditToolInvocation
displayName?: string,
) {
super(params, messageBus, toolName, displayName);
this.resolvedPath = path.resolve(
this.config.getTargetDir(),
this.params.file_path,
);
}
override toolLocations(): ToolLocation[] {
return [{ path: this.params.file_path }];
return [{ path: this.resolvedPath }];
}
protected override getPolicyUpdateOptions(
override getPolicyUpdateOptions(
_outcome: ToolConfirmationOutcome,
): PolicyUpdateOptions | undefined {
return {
@@ -481,7 +486,7 @@ class EditToolInvocation
const initialContentHash = hashContent(currentContent);
const onDiskContent = await this.config
.getFileSystemService()
.readTextFile(params.file_path);
.readTextFile(this.resolvedPath);
const onDiskContentHash = hashContent(onDiskContent.replace(/\r\n/g, '\n'));
if (initialContentHash !== onDiskContentHash) {
@@ -592,7 +597,7 @@ class EditToolInvocation
try {
currentContent = await this.config
.getFileSystemService()
.readTextFile(params.file_path);
.readTextFile(this.resolvedPath);
originalLineEnding = detectLineEnding(currentContent);
currentContent = currentContent.replace(/\r\n/g, '\n');
fileExists = true;
@@ -625,7 +630,7 @@ class EditToolInvocation
isNewFile: false,
error: {
display: `File not found. Cannot apply edit. Use an empty old_string to create a new file.`,
raw: `File not found: ${params.file_path}`,
raw: `File not found: ${this.resolvedPath}`,
type: ToolErrorType.FILE_NOT_FOUND,
},
originalLineEnding,
@@ -640,7 +645,7 @@ class EditToolInvocation
isNewFile: false,
error: {
display: `Failed to read content of file.`,
raw: `Failed to read content of existing file: ${params.file_path}`,
raw: `Failed to read content of existing file: ${this.resolvedPath}`,
type: ToolErrorType.READ_CONTENT_FAILURE,
},
originalLineEnding,
@@ -655,7 +660,7 @@ class EditToolInvocation
isNewFile: false,
error: {
display: `Failed to edit. Attempted to create a file that already exists.`,
raw: `File already exists, cannot create: ${params.file_path}`,
raw: `File already exists, cannot create: ${this.resolvedPath}`,
type: ToolErrorType.ATTEMPT_TO_CREATE_EXISTING_FILE,
},
originalLineEnding,
@@ -737,7 +742,7 @@ class EditToolInvocation
return false;
}
const fileName = path.basename(this.params.file_path);
const fileName = path.basename(this.resolvedPath);
const fileDiff = Diff.createPatch(
fileName,
editData.currentContent ?? '',
@@ -749,14 +754,14 @@ class EditToolInvocation
const ideClient = await IdeClient.getInstance();
const ideConfirmation =
this.config.getIdeMode() && ideClient.isDiffingEnabled()
? ideClient.openDiff(this.params.file_path, editData.newContent)
? ideClient.openDiff(this.resolvedPath, editData.newContent)
: undefined;
const confirmationDetails: ToolEditConfirmationDetails = {
type: 'edit',
title: `Confirm Edit: ${shortenPath(makeRelative(this.params.file_path, this.config.getTargetDir()))}`,
title: `Confirm Edit: ${shortenPath(makeRelative(this.resolvedPath, this.config.getTargetDir()))}`,
fileName,
filePath: this.params.file_path,
filePath: this.resolvedPath,
fileDiff,
originalContent: editData.currentContent,
newContent: editData.newContent,
@@ -781,7 +786,7 @@ class EditToolInvocation
getDescription(): string {
const relativePath = makeRelative(
this.params.file_path,
this.resolvedPath,
this.config.getTargetDir(),
);
if (this.params.old_string === '') {
@@ -807,11 +812,7 @@ class EditToolInvocation
* @returns Result of the edit operation
*/
async execute(signal: AbortSignal): Promise<ToolResult> {
const resolvedPath = path.resolve(
this.config.getTargetDir(),
this.params.file_path,
);
const validationError = this.config.validatePathAccess(resolvedPath);
const validationError = this.config.validatePathAccess(this.resolvedPath);
if (validationError) {
return {
llmContent: validationError,
@@ -853,7 +854,7 @@ class EditToolInvocation
}
try {
await this.ensureParentDirectoriesExistAsync(this.params.file_path);
await this.ensureParentDirectoriesExistAsync(this.resolvedPath);
let finalContent = editData.newContent;
// Restore original line endings if they were CRLF, or use OS default for new files
@@ -866,15 +867,15 @@ class EditToolInvocation
}
await this.config
.getFileSystemService()
.writeTextFile(this.params.file_path, finalContent);
.writeTextFile(this.resolvedPath, finalContent);
let displayResult: ToolResultDisplay;
if (editData.isNewFile) {
displayResult = `Created ${shortenPath(makeRelative(this.params.file_path, this.config.getTargetDir()))}`;
displayResult = `Created ${shortenPath(makeRelative(this.resolvedPath, this.config.getTargetDir()))}`;
} else {
// Generate diff for display, even though core logic doesn't technically need it
// The CLI wrapper will use this part of the ToolResult
const fileName = path.basename(this.params.file_path);
const fileName = path.basename(this.resolvedPath);
const fileDiff = Diff.createPatch(
fileName,
editData.currentContent ?? '', // Should not be null here if not isNewFile
@@ -893,7 +894,7 @@ class EditToolInvocation
displayResult = {
fileDiff,
fileName,
filePath: this.params.file_path,
filePath: this.resolvedPath,
originalContent: editData.currentContent,
newContent: editData.newContent,
diffStat,
@@ -903,8 +904,8 @@ class EditToolInvocation
const llmSuccessMessageParts = [
editData.isNewFile
? `Created new file: ${this.params.file_path} with provided content.`
: `Successfully modified file: ${this.params.file_path} (${editData.occurrences} replacements).`,
? `Created new file: ${this.resolvedPath} with provided content.`
: `Successfully modified file: ${this.resolvedPath} (${editData.occurrences} replacements).`,
];
// Return a diff of the file before and after the write so that the agent
@@ -995,16 +996,10 @@ export class EditTool
return "The 'file_path' parameter must be non-empty.";
}
let filePath = params.file_path;
if (!path.isAbsolute(filePath)) {
// Attempt to auto-correct to an absolute path
const result = correctPath(filePath, this.config);
if (!result.success) {
return result.error;
}
filePath = result.correctedPath;
}
params.file_path = filePath;
const resolvedPath = path.resolve(
this.config.getTargetDir(),
params.file_path,
);
const newPlaceholders = detectOmissionPlaceholders(params.new_string);
if (newPlaceholders.length > 0) {
@@ -1019,7 +1014,7 @@ export class EditTool
}
}
return this.config.validatePathAccess(params.file_path);
return this.config.validatePathAccess(resolvedPath);
}
protected createInvocation(
packages/core/src/tools/mcp-tool.ts
+1 -1
View File
@@ -184,7 +184,7 @@ export class DiscoveredMCPToolInvocation extends BaseToolInvocation<
);
}
protected override getPolicyUpdateOptions(
override getPolicyUpdateOptions(
_outcome: ToolConfirmationOutcome,
): PolicyUpdateOptions | undefined {
return { mcpName: this.serverName };
packages/core/src/tools/shell.ts
+1 -1
View File
@@ -90,7 +90,7 @@ export class ShellToolInvocation extends BaseToolInvocation<
return description;
}
protected override getPolicyUpdateOptions(
override getPolicyUpdateOptions(
outcome: ToolConfirmationOutcome,
): PolicyUpdateOptions | undefined {
if (
packages/core/src/tools/tools.ts
+9 -1
View File
@@ -68,6 +68,14 @@ export interface ToolInvocation<
updateOutput?: (output: ToolLiveOutput) => void,
shellExecutionConfig?: ShellExecutionConfig,
): Promise<TResult>;
/**
* Returns tool-specific options for policy updates.
* This is used by the scheduler to narrow policy rules when a tool is approved.
*/
getPolicyUpdateOptions?(
outcome: ToolConfirmationOutcome,
): PolicyUpdateOptions | undefined;
}
/**
@@ -131,7 +139,7 @@ export abstract class BaseToolInvocation<
* Subclasses can override this to provide additional options like
* commandPrefix (for shell) or mcpName (for MCP tools).
*/
protected getPolicyUpdateOptions(
getPolicyUpdateOptions(
_outcome: ToolConfirmationOutcome,
): PolicyUpdateOptions | undefined {
return undefined;
packages/core/src/tools/write-file.ts
+1 -1
View File
@@ -166,7 +166,7 @@ class WriteFileToolInvocation extends BaseToolInvocation<
return [{ path: this.resolvedPath }];
}
protected override getPolicyUpdateOptions(
override getPolicyUpdateOptions(
_outcome: ToolConfirmationOutcome,
): PolicyUpdateOptions | undefined {
return {
review_findings.md
-59
View File
@@ -1,59 +0,0 @@
# Review Findings - PR #20361
## Summary
The PR implements "auto-add to policy by default" with workspace-first
persistence and rule narrowing for edit tools. The core logic is sound, but
there are several violations of the "Strict Development Rules".
## Actionable Findings
### 1. Type Safety (STRICT TYPING Rule)
- **`packages/core/src/scheduler/policy.test.ts`**: Still uses `any` for
`details` in 'should narrow edit tools with argsPattern' test (Line 512).
- **`packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx`**: The
`initialIndex` calculation logic uses `confirmationDetails` which is complex.
Ensure no `any` is leaked here.
### 2. React Best Practices (packages/cli)
- **Dependency Management**: In `ToolConfirmationMessage.tsx`, the `useMemo`
block for `question`, `bodyContent`, etc. (Lines 418-444) includes many new
dependencies. Ensure `initialIndex` is calculated correctly and doesn't
trigger unnecessary re-renders.
- **Reducers**: The `initialIndex` is derived state. While `useMemo` is
acceptable here, verify if this state should be part of a larger reducer if
the confirmation UI becomes more complex.
### 3. Core Logic Placement
- **Inconsistency**: Narrowing for edit tools is implemented in both
`scheduler/policy.ts` and individual tools (`write-file.ts`, `edit.ts`).
- _Recommendation_: Centralize the narrowing logic in the tools via
`getPolicyUpdateOptions` and ensure `scheduler/policy.ts` purely respects
what the tool provides, rather than duplicating the
`buildFilePathArgsPattern` call.
### 4. Testing Guidelines
- **Snapshot Clarity**: The new snapshot for `ToolConfirmationMessage` includes
a large block of text. Ensure the snapshot specifically highlights the change
in the selected radio button (the `●` indicator).
- **Mocking**: In `persistence.test.ts`, ensure `vi.restoreAllMocks()` or
`vi.clearAllMocks()` is consistently used to avoid pollution between the new
workspace persistence tests and existing ones.
### 5. Settings & Documentation
- **RequiresRestart**: The `autoAddToPolicyByDefault` setting has
`requiresRestart: false`. Verify if the `ToolConfirmationMessage` correctly
picks up setting changes without a restart (it should, as it uses the
`settings` hook).
- **Documentation**: Ensure this new setting is added to
`docs/get-started/configuration.md` as per the general principles.
## Directive
Fix all findings above, prioritizing strict typing and removal of duplicate
narrowing logic.
schemas/settings.schema.json
+7
View File
@@ -1461,6 +1461,13 @@
"default": false,
"type": "boolean"
},
"autoAddToPolicyByDefault": {
"title": "Auto-add to Policy by Default",
"description": "When enabled, the \"Allow for all future sessions\" option becomes the default choice for low-risk tools in trusted workspaces.",
"markdownDescription": "When enabled, the \"Allow for all future sessions\" option becomes the default choice for low-risk tools in trusted workspaces.\n\n- Category: `Security`\n- Requires restart: `no`\n- Default: `true`",
"default": true,
"type": "boolean"
},
"blockGitExtensions": {
"title": "Blocks extensions from Git",
"description": "Blocks installing and loading extensions from Git.",