diff --git a/docs/cli/settings.md b/docs/cli/settings.md index 5565a5e1f6..cc04992acf 100644 --- a/docs/cli/settings.md +++ b/docs/cli/settings.md @@ -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` | diff --git a/packages/cli/src/ui/components/messages/ToolConfirmationMessage.test.tsx b/packages/cli/src/ui/components/messages/ToolConfirmationMessage.test.tsx index eacf34cece..ec623f69a4 100644 --- a/packages/cli/src/ui/components/messages/ToolConfirmationMessage.test.tsx +++ b/packages/cli/src/ui/components/messages/ToolConfirmationMessage.test.tsx @@ -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( - , - { - 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(); }); diff --git a/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx b/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx index 7284d2dd19..c63f2937cc 100644 --- a/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx +++ b/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx @@ -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, ]); diff --git a/packages/cli/src/ui/components/messages/__snapshots__/ToolConfirmationMessage.test.tsx.snap b/packages/cli/src/ui/components/messages/__snapshots__/ToolConfirmationMessage.test.tsx.snap index d79403adad..591a49dd01 100644 --- a/packages/cli/src/ui/components/messages/__snapshots__/ToolConfirmationMessage.test.tsx.snap +++ b/packages/cli/src/ui/components/messages/__snapshots__/ToolConfirmationMessage.test.tsx.snap @@ -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) " diff --git a/packages/core/src/agents/browser/mcpToolWrapper.ts b/packages/core/src/agents/browser/mcpToolWrapper.ts index 96b6aa9b68..f27d3462e6 100644 --- a/packages/core/src/agents/browser/mcpToolWrapper.ts +++ b/packages/core/src/agents/browser/mcpToolWrapper.ts @@ -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 { diff --git a/packages/core/src/policy/config.ts b/packages/core/src/policy/config.ts index b210e3f5b8..83340c4261 100644 --- a/packages/core/src/policy/config.ts +++ b/packages/core/src/policy/config.ts @@ -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) { diff --git a/packages/core/src/policy/persistence.test.ts b/packages/core/src/policy/persistence.test.ts index def24899cd..8b3667c5a2 100644 --- a/packages/core/src/policy/persistence.test.ts +++ b/packages/core/src/policy/persistence.test.ts @@ -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"'); }); }); diff --git a/packages/core/src/policy/policy-updater.test.ts b/packages/core/src/policy/policy-updater.test.ts index 3037667949..fdc65a4542 100644 --- a/packages/core/src/policy/policy-updater.test.ts +++ b/packages/core/src/policy/policy-updater.test.ts @@ -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'; diff --git a/packages/core/src/policy/utils.ts b/packages/core/src/policy/utils.ts index cfe57f18ff..6dd4f19031 100644 --- a/packages/core/src/policy/utils.ts +++ b/packages/core/src/policy/utils.ts @@ -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. */ diff --git a/packages/core/src/scheduler/policy.test.ts b/packages/core/src/scheduler/policy.test.ts index 7cfc505b72..af5883bd0d 100644 --- a/packages/core/src/scheduler/policy.test.ts +++ b/packages/core/src/scheduler/policy.test.ts @@ -559,12 +559,15 @@ describe('policy.ts', () => { publish: vi.fn(), } as unknown as Mocked; 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, diff --git a/packages/core/src/scheduler/policy.ts b/packages/core/src/scheduler/policy.ts index 4ac56f2d82..59459b6214 100644 --- a/packages/core/src/scheduler/policy.ts +++ b/packages/core/src/scheduler/policy.ts @@ -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; diff --git a/packages/core/src/tools/edit.ts b/packages/core/src/tools/edit.ts index 3e64944ff1..75c0709e72 100644 --- a/packages/core/src/tools/edit.ts +++ b/packages/core/src/tools/edit.ts @@ -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 implements ToolInvocation { + 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 { - 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( diff --git a/packages/core/src/tools/mcp-tool.ts b/packages/core/src/tools/mcp-tool.ts index f67d1f9bea..523eac62ad 100644 --- a/packages/core/src/tools/mcp-tool.ts +++ b/packages/core/src/tools/mcp-tool.ts @@ -184,7 +184,7 @@ export class DiscoveredMCPToolInvocation extends BaseToolInvocation< ); } - protected override getPolicyUpdateOptions( + override getPolicyUpdateOptions( _outcome: ToolConfirmationOutcome, ): PolicyUpdateOptions | undefined { return { mcpName: this.serverName }; diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index 4ea83b0af4..a1bef189b5 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -90,7 +90,7 @@ export class ShellToolInvocation extends BaseToolInvocation< return description; } - protected override getPolicyUpdateOptions( + override getPolicyUpdateOptions( outcome: ToolConfirmationOutcome, ): PolicyUpdateOptions | undefined { if ( diff --git a/packages/core/src/tools/tools.ts b/packages/core/src/tools/tools.ts index 79bb599ff8..828461ea65 100644 --- a/packages/core/src/tools/tools.ts +++ b/packages/core/src/tools/tools.ts @@ -68,6 +68,14 @@ export interface ToolInvocation< updateOutput?: (output: ToolLiveOutput) => void, shellExecutionConfig?: ShellExecutionConfig, ): Promise; + + /** + * 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; diff --git a/packages/core/src/tools/write-file.ts b/packages/core/src/tools/write-file.ts index 95e5393916..4c0a533689 100644 --- a/packages/core/src/tools/write-file.ts +++ b/packages/core/src/tools/write-file.ts @@ -166,7 +166,7 @@ class WriteFileToolInvocation extends BaseToolInvocation< return [{ path: this.resolvedPath }]; } - protected override getPolicyUpdateOptions( + override getPolicyUpdateOptions( _outcome: ToolConfirmationOutcome, ): PolicyUpdateOptions | undefined { return { diff --git a/review_findings.md b/review_findings.md deleted file mode 100644 index 53a840ef9b..0000000000 --- a/review_findings.md +++ /dev/null @@ -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. diff --git a/schemas/settings.schema.json b/schemas/settings.schema.json index 280ad18db5..3f4211f627 100644 --- a/schemas/settings.schema.json +++ b/schemas/settings.schema.json @@ -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.",