From 460c81a7b64d2e7ec7aac73646fc087a4634424d Mon Sep 17 00:00:00 2001 From: LaZzyMan Date: Thu, 26 Mar 2026 17:54:38 +0800 Subject: [PATCH 1/5] fix(tools): fix glob ignore base path and add dedup for grep/ripgrep - glob: compute relativePaths relative to projectRoot (config.getTargetDir()) instead of searchDir so that FileDiscoveryService evaluates .gitignore / .qwenignore rules against the correct paths when path option is used or when searching across multiple workspace directories - grep: deduplicate rawMatches by filePath:lineNumber key when searching multiple workspace directories to prevent duplicate results from overlapping search roots (e.g. parent dir + child dir in workspace) - ripGrep: deduplicate output lines by filepath:linenum prefix when searching multiple workspace paths to handle the same edge case - tests: add regression tests covering - glob ignores files matching .gitignore/.qwenignore when path option points to a subdirectory - grep deduplication with parent+child overlapping workspace dirs - ripgrep deduplication when raw output contains duplicate lines --- packages/core/src/tools/glob.test.ts | 38 +++++++++++++++++++++++++ packages/core/src/tools/glob.ts | 12 ++++---- packages/core/src/tools/grep.test.ts | 27 ++++++++++++++++++ packages/core/src/tools/grep.ts | 14 ++++++++- packages/core/src/tools/ripGrep.test.ts | 30 +++++++++++++++++++ packages/core/src/tools/ripGrep.ts | 22 +++++++++++++- 6 files changed, 136 insertions(+), 7 deletions(-) diff --git a/packages/core/src/tools/glob.test.ts b/packages/core/src/tools/glob.test.ts index 24be79d2ef..67769a6e98 100644 --- a/packages/core/src/tools/glob.test.ts +++ b/packages/core/src/tools/glob.test.ts @@ -491,6 +491,44 @@ describe('GlobTool', () => { expect(result.llmContent).toContain('Found 3 file(s)'); // fileA.txt, FileB.TXT, b.notignored.txt expect(result.llmContent).not.toContain('a.qwenignored.txt'); }); + + it('should respect .gitignore when searching a subdirectory (path option)', async () => { + // This tests the regression fix: relativePaths must be computed relative + // to projectRoot, not to searchDir, so that gitignore rules rooted at + // projectRoot are evaluated against the correct paths. + await fs.writeFile(path.join(tempRootDir, '.gitignore'), '*.secret'); + await fs.writeFile(path.join(tempRootDir, 'sub', 'visible.txt'), 'ok'); + await fs.writeFile( + path.join(tempRootDir, 'sub', 'hidden.secret'), + 'should be ignored', + ); + + const subDirTool = new GlobTool(mockConfig); + const params: GlobToolParams = { pattern: '*', path: 'sub' }; + const invocation = subDirTool.build(params); + const result = await invocation.execute(abortSignal); + + expect(result.llmContent).toContain('visible.txt'); + expect(result.llmContent).not.toContain('hidden.secret'); + }); + + it('should respect .qwenignore when searching a subdirectory (path option)', async () => { + await fs.writeFile(path.join(tempRootDir, '.qwenignore'), '*.secret'); + await fs.writeFile(path.join(tempRootDir, 'sub', 'visible.txt'), 'ok'); + await fs.writeFile( + path.join(tempRootDir, 'sub', 'hidden.secret'), + 'should be ignored', + ); + + // Recreate to pick up .qwenignore + const subDirTool = new GlobTool(mockConfig); + const params: GlobToolParams = { pattern: '*', path: 'sub' }; + const invocation = subDirTool.build(params); + const result = await invocation.execute(abortSignal); + + expect(result.llmContent).toContain('visible.txt'); + expect(result.llmContent).not.toContain('hidden.secret'); + }); }); describe('file count truncation', () => { diff --git a/packages/core/src/tools/glob.ts b/packages/core/src/tools/glob.ts index 44edae4e64..ab6b6d80a1 100644 --- a/packages/core/src/tools/glob.ts +++ b/packages/core/src/tools/glob.ts @@ -144,11 +144,13 @@ class GlobToolInvocation extends BaseToolInvocation< signal, })) as GlobPath[]; - // Filter using paths relative to the search directory so that - // .gitignore / .qwenignore patterns match correctly regardless of - // which workspace directory the file belongs to. + // Filter using paths relative to the project root (the base that + // FileDiscoveryService uses for .gitignore / .qwenignore evaluation). + // Using searchDir-relative paths would cause ignore rules to be + // evaluated against incorrect paths when searchDir != projectRoot. + const projectRoot = this.config.getTargetDir(); const relativePaths = entries.map((p) => - path.relative(searchDir, p.fullpath()), + path.relative(projectRoot, p.fullpath()), ); const { filteredPaths } = this.fileService.filterFilesWithReport( @@ -163,7 +165,7 @@ class GlobToolInvocation extends BaseToolInvocation< const filteredAbsolutePaths = new Set( filteredPaths.map((p) => - normalizePathForComparison(path.resolve(searchDir, p)), + normalizePathForComparison(path.resolve(projectRoot, p)), ), ); diff --git a/packages/core/src/tools/grep.test.ts b/packages/core/src/tools/grep.test.ts index 868cecd784..4af6d8d7ce 100644 --- a/packages/core/src/tools/grep.test.ts +++ b/packages/core/src/tools/grep.test.ts @@ -399,6 +399,33 @@ describe('GrepTool', () => { await fs.rm(secondDir, { recursive: true, force: true }); }); + + it('should deduplicate matches from overlapping workspace directories', async () => { + // This tests the fix: when workspace dirs overlap (parent + child), + // the same file should appear only once in the results. + const subDir = path.join(tempRootDir, 'sub'); + + const multiDirConfig = { + getTargetDir: () => tempRootDir, + getWorkspaceContext: () => + createMockWorkspaceContext(tempRootDir, [subDir]), + getFileExclusions: () => ({ + getGlobExcludes: () => [], + }), + getTruncateToolOutputThreshold: () => 25000, + getTruncateToolOutputLines: () => 1000, + } as unknown as Config; + + const multiDirGrepTool = new GrepTool(multiDirConfig); + // 'sub dir' exists only in sub/fileC.txt — a file that lives under both + // tempRootDir and subDir, so without deduplication it would appear twice. + const params: GrepToolParams = { pattern: 'sub dir' }; + const invocation = multiDirGrepTool.build(params); + const result = await invocation.execute(abortSignal); + + // sub/fileC.txt (or its absolute path equivalent) should appear only once + expect(result.llmContent).toContain('Found 1 match'); + }); }); describe('getDescription', () => { diff --git a/packages/core/src/tools/grep.ts b/packages/core/src/tools/grep.ts index 6e16348d95..4f927a1675 100644 --- a/packages/core/src/tools/grep.ts +++ b/packages/core/src/tools/grep.ts @@ -121,7 +121,7 @@ class GrepToolInvocation extends BaseToolInvocation< } // Perform grep search across all directories - const rawMatches: GrepMatch[] = []; + let rawMatches: GrepMatch[] = []; for (const searchDir of searchDirs) { const matches = await this.performGrepSearch({ pattern: this.params.pattern, @@ -142,6 +142,18 @@ class GrepToolInvocation extends BaseToolInvocation< rawMatches.push(...matches); } + // Deduplicate matches that might appear from overlapping workspace + // directories (e.g. parent + child both in workspace dirs). + if (searchDirs.length > 1) { + const seen = new Set(); + rawMatches = rawMatches.filter((match) => { + const key = `${match.filePath}:${match.lineNumber}`; + if (seen.has(key)) return false; + seen.add(key); + return true; + }); + } + const filterDescription = this.params.glob ? ` (filter: "${this.params.glob}")` : ''; diff --git a/packages/core/src/tools/ripGrep.test.ts b/packages/core/src/tools/ripGrep.test.ts index 5edbc680a7..118ed97916 100644 --- a/packages/core/src/tools/ripGrep.test.ts +++ b/packages/core/src/tools/ripGrep.test.ts @@ -544,6 +544,36 @@ describe('RipGrepTool', () => { await fs.rm(secondDir, { recursive: true, force: true }); }); + + it('should deduplicate matches from overlapping workspace directories', async () => { + // This tests the fix: when ripgrep receives overlapping search paths + // (e.g. /parent and /parent/sub), it may report the same file twice. + // The deduplication layer must remove duplicates. + const subDir = path.join(tempRootDir, 'sub'); + + const multiDirConfig = { + ...mockConfig, + getWorkspaceContext: () => + createMockWorkspaceContext(tempRootDir, [subDir]), + } as unknown as Config; + + const multiDirGrepTool = new RipGrepTool(multiDirConfig); + + // Simulate ripgrep returning the same file:line twice (once from each search root) + const dupLine = `${path.join(subDir, 'fileC.txt')}:1:hello world`; + (runRipgrep as Mock).mockResolvedValue({ + stdout: `${dupLine}${EOL}${dupLine}${EOL}`, + truncated: false, + error: undefined, + }); + + const params: RipGrepToolParams = { pattern: 'hello' }; + const invocation = multiDirGrepTool.build(params); + const result = await invocation.execute(abortSignal); + + // Despite two identical lines in the raw output, only 1 match should be reported. + expect(result.llmContent).toContain('Found 1 match'); + }); }); describe('abort signal handling', () => { diff --git a/packages/core/src/tools/ripGrep.ts b/packages/core/src/tools/ripGrep.ts index 19a98af800..a023d9d61a 100644 --- a/packages/core/src/tools/ripGrep.ts +++ b/packages/core/src/tools/ripGrep.ts @@ -106,7 +106,27 @@ class GrepToolInvocation extends BaseToolInvocation< } // Split into lines and count total matches - const allLines = rawOutput.split('\n').filter((line) => line.trim()); + let allLines = rawOutput.split('\n').filter((line) => line.trim()); + + // Deduplicate lines from potentially overlapping workspace directories. + // ripgrep reports the same file twice when given paths like /a and /a/sub. + if (searchPaths.length > 1) { + const seen = new Set(); + allLines = allLines.filter((line) => { + // ripgrep output format: filepath:linenum:content + const firstColon = line.indexOf(':'); + if (firstColon !== -1) { + const secondColon = line.indexOf(':', firstColon + 1); + if (secondColon !== -1) { + const key = line.substring(0, secondColon); + if (seen.has(key)) return false; + seen.add(key); + } + } + return true; + }); + } + const totalMatches = allLines.length; const matchTerm = totalMatches === 1 ? 'match' : 'matches'; From dd518de5b0abde1b87b84afa5a1adaf3f9c59f13 Mon Sep 17 00:00:00 2001 From: LaZzyMan Date: Thu, 26 Mar 2026 23:25:04 +0800 Subject: [PATCH 2/5] fix(acp): align permission flow across clients --- packages/cli/src/acp-integration/acpAgent.ts | 24 +- .../acp-integration/session/Session.test.ts | 148 ++++- .../src/acp-integration/session/Session.ts | 455 +++++++++------- .../session/SubAgentTracker.test.ts | 56 +- .../session/SubAgentTracker.ts | 150 +---- .../session/permissionUtils.test.ts | 54 ++ .../session/permissionUtils.ts | 208 +++++++ packages/cli/src/config/config.ts | 32 +- packages/cli/src/gemini.tsx | 1 - .../prompt-processors/shellProcessor.ts | 4 +- .../messages/ToolConfirmationMessage.tsx | 7 +- packages/core/src/config/config.ts | 40 +- packages/core/src/core/coreToolScheduler.ts | 407 ++++++-------- packages/core/src/core/permission-helpers.ts | 207 +++++++ packages/core/src/index.ts | 2 + .../permissions/permission-manager.test.ts | 511 ++++++++++-------- .../src/permissions/permission-manager.ts | 165 +++++- packages/core/src/tools/edit.test.ts | 43 ++ packages/core/src/tools/edit.ts | 6 +- packages/core/src/tools/shell.test.ts | 84 ++- packages/core/src/tools/shell.ts | 29 +- packages/core/src/tools/write-file.test.ts | 30 + packages/core/src/tools/write-file.ts | 6 +- packages/core/src/utils/shell-utils.test.ts | 255 ++++----- packages/core/src/utils/shell-utils.ts | 17 +- .../src/webview/providers/WebViewProvider.ts | 19 - 26 files changed, 1884 insertions(+), 1076 deletions(-) create mode 100644 packages/cli/src/acp-integration/session/permissionUtils.test.ts create mode 100644 packages/cli/src/acp-integration/session/permissionUtils.ts create mode 100644 packages/core/src/core/permission-helpers.ts diff --git a/packages/cli/src/acp-integration/acpAgent.ts b/packages/cli/src/acp-integration/acpAgent.ts index dbdab8de45..b78a8a0d60 100644 --- a/packages/cli/src/acp-integration/acpAgent.ts +++ b/packages/cli/src/acp-integration/acpAgent.ts @@ -57,7 +57,7 @@ import { buildAuthMethods } from './authMethods.js'; import { AcpFileSystemService } from './service/filesystem.js'; import { Readable, Writable } from 'node:stream'; import type { LoadedSettings } from '../config/settings.js'; -import { SettingScope } from '../config/settings.js'; +import { loadSettings, SettingScope } from '../config/settings.js'; import type { ApprovalModeValue } from './session/types.js'; import { z } from 'zod'; import type { CliArgs } from '../config/config.js'; @@ -223,30 +223,18 @@ class QwenAgent implements Agent { return sessionService.sessionExists(params.sessionId); }, ); - if (!exists) { - throw RequestError.invalidParams( - undefined, - `Session not found for id: ${params.sessionId}`, - ); - } const config = await this.newSessionConfig( params.cwd, params.mcpServers, params.sessionId, + exists, ); await this.ensureAuthenticated(config); this.setupFileSystem(config); const sessionData = config.getResumedSessionData(); - if (!sessionData) { - throw RequestError.internalError( - undefined, - `Failed to load session data for id: ${params.sessionId}`, - ); - } - - await this.createAndStoreSession(config, sessionData.conversation); + await this.createAndStoreSession(config, sessionData?.conversation); const modesData = this.buildModesData(config); const availableModels = this.buildAvailableModels(config); @@ -380,7 +368,9 @@ class QwenAgent implements Agent { cwd: string, mcpServers: McpServer[], sessionId?: string, + resume?: boolean, ): Promise { + this.settings = loadSettings(cwd); const mergedMcpServers = { ...this.settings.merged.mcpServers }; for (const server of mcpServers) { @@ -402,11 +392,11 @@ class QwenAgent implements Agent { const settings = { ...this.settings.merged, mcpServers: mergedMcpServers }; const argvForSession = { ...this.argv, - resume: sessionId, + ...(resume ? { resume: sessionId } : { sessionId }), continue: false, }; - const config = await loadCliConfig(settings, argvForSession, cwd); + const config = await loadCliConfig(settings, argvForSession, cwd, []); await config.initialize(); return config; } diff --git a/packages/cli/src/acp-integration/session/Session.test.ts b/packages/cli/src/acp-integration/session/Session.test.ts index 9715d765c5..330506770a 100644 --- a/packages/cli/src/acp-integration/session/Session.test.ts +++ b/packages/cli/src/acp-integration/session/Session.test.ts @@ -34,6 +34,7 @@ describe('Session', () => { let currentAuthType: AuthType; let switchModelSpy: ReturnType; let getAvailableCommandsSpy: ReturnType; + let mockToolRegistry: { getTool: ReturnType }; beforeEach(() => { currentModel = 'qwen3-code-plus'; @@ -50,7 +51,7 @@ describe('Session', () => { addHistory: vi.fn(), } as unknown as GeminiChat; - const toolRegistry = { getTool: vi.fn() }; + mockToolRegistry = { getTool: vi.fn() }; const fileService = { shouldGitIgnoreFile: vi.fn().mockReturnValue(false) }; mockConfig = { @@ -65,8 +66,9 @@ describe('Session', () => { getChatRecordingService: vi.fn().mockReturnValue({ recordUserMessage: vi.fn(), recordUiTelemetryEvent: vi.fn(), + recordToolResult: vi.fn(), }), - getToolRegistry: vi.fn().mockReturnValue(toolRegistry), + getToolRegistry: vi.fn().mockReturnValue(mockToolRegistry), getFileService: vi.fn().mockReturnValue(fileService), getFileFilteringRespectGitIgnore: vi.fn().mockReturnValue(true), getEnableRecursiveFileSearch: vi.fn().mockReturnValue(false), @@ -275,5 +277,147 @@ describe('Session', () => { expect.any(Function), ); }); + + it('hides allow-always options when confirmation already forbids them', async () => { + const executeSpy = vi.fn().mockResolvedValue({ + llmContent: 'ok', + returnDisplay: 'ok', + }); + const onConfirmSpy = vi.fn().mockResolvedValue(undefined); + const invocation = { + params: { path: '/tmp/file.txt' }, + getDefaultPermission: vi.fn().mockResolvedValue('ask'), + getConfirmationDetails: vi.fn().mockResolvedValue({ + type: 'info', + title: 'Need permission', + prompt: 'Allow?', + hideAlwaysAllow: true, + onConfirm: onConfirmSpy, + }), + getDescription: vi.fn().mockReturnValue('Inspect file'), + toolLocations: vi.fn().mockReturnValue([]), + execute: executeSpy, + }; + const tool = { + name: 'read_file', + kind: core.Kind.Read, + build: vi.fn().mockReturnValue(invocation), + }; + + mockToolRegistry.getTool.mockReturnValue(tool); + mockConfig.getApprovalMode = vi + .fn() + .mockReturnValue(ApprovalMode.DEFAULT); + mockConfig.getPermissionManager = vi.fn().mockReturnValue(null); + mockChat.sendMessageStream = vi.fn().mockResolvedValue( + (async function* () { + yield { + type: core.StreamEventType.CHUNK, + value: { + functionCalls: [ + { + id: 'call-1', + name: 'read_file', + args: { path: '/tmp/file.txt' }, + }, + ], + }, + }; + })(), + ); + + await session.prompt({ + sessionId: 'test-session-id', + prompt: [{ type: 'text', text: 'run tool' }], + }); + + expect(mockClient.requestPermission).toHaveBeenCalledWith( + expect.objectContaining({ + options: [ + expect.objectContaining({ kind: 'allow_once' }), + expect.objectContaining({ kind: 'reject_once' }), + ], + }), + ); + const options = (mockClient.requestPermission as ReturnType) + .mock.calls[0][0].options as Array<{ kind: string }>; + expect(options.some((option) => option.kind === 'allow_always')).toBe( + false, + ); + }); + + it('respects permission-request hook allow decisions without opening ACP permission dialog', async () => { + const hookSpy = vi + .spyOn(core, 'firePermissionRequestHook') + .mockResolvedValue({ + hasDecision: true, + shouldAllow: true, + updatedInput: { path: '/tmp/updated.txt' }, + denyMessage: undefined, + }); + const executeSpy = vi.fn().mockResolvedValue({ + llmContent: 'ok', + returnDisplay: 'ok', + }); + const onConfirmSpy = vi.fn().mockResolvedValue(undefined); + const invocation = { + params: { path: '/tmp/original.txt' }, + getDefaultPermission: vi.fn().mockResolvedValue('ask'), + getConfirmationDetails: vi.fn().mockResolvedValue({ + type: 'info', + title: 'Need permission', + prompt: 'Allow?', + onConfirm: onConfirmSpy, + }), + getDescription: vi.fn().mockReturnValue('Inspect file'), + toolLocations: vi.fn().mockReturnValue([]), + execute: executeSpy, + }; + const tool = { + name: 'read_file', + kind: core.Kind.Read, + build: vi.fn().mockReturnValue(invocation), + }; + + mockToolRegistry.getTool.mockReturnValue(tool); + mockConfig.getApprovalMode = vi + .fn() + .mockReturnValue(ApprovalMode.DEFAULT); + mockConfig.getPermissionManager = vi.fn().mockReturnValue(null); + mockConfig.getEnableHooks = vi.fn().mockReturnValue(true); + mockConfig.getMessageBus = vi.fn().mockReturnValue({}); + mockChat.sendMessageStream = vi.fn().mockResolvedValue( + (async function* () { + yield { + type: core.StreamEventType.CHUNK, + value: { + functionCalls: [ + { + id: 'call-2', + name: 'read_file', + args: { path: '/tmp/original.txt' }, + }, + ], + }, + }; + })(), + ); + + try { + await session.prompt({ + sessionId: 'test-session-id', + prompt: [{ type: 'text', text: 'run tool' }], + }); + } finally { + hookSpy.mockRestore(); + } + + expect(mockClient.requestPermission).not.toHaveBeenCalled(); + expect(onConfirmSpy).toHaveBeenCalledWith( + core.ToolConfirmationOutcome.ProceedOnce, + ); + expect(invocation.params).toEqual({ path: '/tmp/updated.txt' }); + expect(executeSpy).toHaveBeenCalled(); + }); }); }); diff --git a/packages/cli/src/acp-integration/session/Session.ts b/packages/cli/src/acp-integration/session/Session.ts index 45b8375696..b89044be44 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -36,6 +36,13 @@ import { readManyFiles, Storage, ToolNames, + buildPermissionCheckContext, + evaluatePermissionRules, + fireNotificationHook, + firePermissionRequestHook, + injectPermissionRulesIfMissing, + NotificationType, + persistPermissionOutcome, } from '@qwen-code/qwen-code-core'; import { RequestError } from '@agentclientprotocol/sdk'; @@ -43,7 +50,6 @@ import type { AvailableCommand, ContentBlock, EmbeddedResourceResource, - PermissionOption, PromptRequest, PromptResponse, RequestPermissionRequest, @@ -54,7 +60,6 @@ import type { SetSessionModeResponse, SetSessionModelRequest, SetSessionModelResponse, - ToolCallContent, AgentSideConnection, } from '@agentclientprotocol/sdk'; import type { LoadedSettings } from '../../config/settings.js'; @@ -79,6 +84,10 @@ import { ToolCallEmitter } from './emitters/ToolCallEmitter.js'; import { PlanEmitter } from './emitters/PlanEmitter.js'; import { MessageEmitter } from './emitters/MessageEmitter.js'; import { SubAgentTracker } from './SubAgentTracker.js'; +import { + buildPermissionRequestContent, + toPermissionOptions, +} from './permissionUtils.js'; const debugLogger = createDebugLogger('SESSION'); @@ -487,13 +496,34 @@ export class Session implements SessionContext { await this.sendUpdate(update); } + private async resolveIdeDiffForOutcome( + confirmationDetails: ToolCallConfirmationDetails, + outcome: ToolConfirmationOutcome, + ): Promise { + if ( + confirmationDetails.type !== 'edit' || + !confirmationDetails.ideConfirmation + ) { + return; + } + + const { IdeClient } = await import('@qwen-code/qwen-code-core'); + const ideClient = await IdeClient.getInstance(); + const cliOutcome = + outcome === ToolConfirmationOutcome.Cancel ? 'rejected' : 'accepted'; + await ideClient.resolveDiffFromCli( + confirmationDetails.filePath, + cliOutcome as 'accepted' | 'rejected', + ); + } + private async runTool( abortSignal: AbortSignal, promptId: string, fc: FunctionCall, ): Promise { const callId = fc.id ?? `${fc.name}-${Date.now()}`; - const args = (fc.args ?? {}) as Record; + let args = (fc.args ?? {}) as Record; const startTime = Date.now(); @@ -526,19 +556,49 @@ export class Session implements SessionContext { ]; }; + const earlyErrorResponse = async ( + error: Error, + toolName = fc.name ?? 'unknown_tool', + ) => { + if (toolName !== TodoWriteTool.Name) { + await this.toolCallEmitter.emitError(callId, toolName, error); + } + + const errorParts = errorResponse(error); + this.config.getChatRecordingService()?.recordToolResult(errorParts, { + callId, + status: 'error', + resultDisplay: undefined, + error, + errorType: undefined, + }); + return errorParts; + }; + if (!fc.name) { - return errorResponse(new Error('Missing function name')); + return earlyErrorResponse(new Error('Missing function name')); } const toolRegistry = this.config.getToolRegistry(); const tool = toolRegistry.getTool(fc.name as string); if (!tool) { - return errorResponse( + return earlyErrorResponse( new Error(`Tool "${fc.name}" not found in registry.`), ); } + // ---- L1: Tool enablement check ---- + const pm = this.config.getPermissionManager?.(); + if (pm && !pm.isToolEnabled(fc.name as string)) { + return earlyErrorResponse( + new Error( + `Qwen Code requires permission to use "${fc.name}", but that permission was declined.`, + ), + fc.name, + ); + } + // Detect TodoWriteTool early - route to plan updates instead of tool_call events const isTodoWriteTool = tool.name === TodoWriteTool.Name; const isAgentTool = tool.name === AgentTool.Name; @@ -577,127 +637,238 @@ export class Session implements SessionContext { ); } - // Use the new permission flow: getDefaultPermission + getConfirmationDetails - // ask_user_question must always go through confirmation even in YOLO mode - // so the user always has a chance to respond to questions. + // L3→L4→L5 Permission Flow (aligned with coreToolScheduler) + // + // L3: Tool's intrinsic default permission + // L4: PermissionManager rule override + // L5: ApprovalMode override (YOLO / AUTO_EDIT / PLAN) + // + // AUTO_EDIT auto-approval is handled HERE, same as coreToolScheduler. + // The VS Code extension is just a UI layer for requestPermission. const isAskUserQuestionTool = fc.name === ToolNames.ASK_USER_QUESTION; + + // ---- L3: Tool's default permission ---- + // In YOLO mode, force 'allow' for everything except ask_user_question. const defaultPermission = this.config.getApprovalMode() !== ApprovalMode.YOLO || isAskUserQuestionTool ? await invocation.getDefaultPermission() : 'allow'; - const needsConfirmation = defaultPermission === 'ask'; + // ---- L4: PermissionManager override (if relevant rules exist) ---- + const toolParams = invocation.params as Record; + const pmCtx = buildPermissionCheckContext( + fc.name, + toolParams, + this.config.getTargetDir?.() ?? '', + ); + const { finalPermission, pmForcedAsk } = await evaluatePermissionRules( + pm, + defaultPermission, + pmCtx, + ); + + const needsConfirmation = finalPermission === 'ask'; + + // ---- L5: ApprovalMode overrides ---- + const approvalMode = this.config.getApprovalMode(); + const isPlanMode = approvalMode === ApprovalMode.PLAN; - // Check for plan mode enforcement - block non-read-only tools - // but allow ask_user_question so users can answer clarification questions - const isPlanMode = this.config.getApprovalMode() === ApprovalMode.PLAN; + // PLAN mode: block non-read-only tools if ( isPlanMode && !isExitPlanModeTool && !isAskUserQuestionTool && needsConfirmation ) { - // In plan mode, block any tool that requires confirmation (write operations) - return errorResponse( + return earlyErrorResponse( new Error( `Plan mode is active. The tool "${fc.name}" cannot be executed because it modifies the system. ` + 'Please use the exit_plan_mode tool to present your plan and exit plan mode before making changes.', ), + fc.name, ); } - if (defaultPermission === 'deny') { - return errorResponse( + if (finalPermission === 'deny') { + return earlyErrorResponse( new Error( - `Tool "${fc.name}" is denied: command substitution is not allowed for security reasons.`, + defaultPermission === 'deny' + ? `Tool "${fc.name}" is denied: command substitution is not allowed for security reasons.` + : `Tool "${fc.name}" is denied by permission rules.`, ), + fc.name, ); } + let didRequestPermission = false; + if (needsConfirmation) { const confirmationDetails = await invocation.getConfirmationDetails(abortSignal); - const content: ToolCallContent[] = []; - - if (confirmationDetails.type === 'edit') { - content.push({ - type: 'diff', - path: confirmationDetails.fileName, - oldText: confirmationDetails.originalContent, - newText: confirmationDetails.newContent, - }); - } - // Add plan content for exit_plan_mode - if (confirmationDetails.type === 'plan') { - content.push({ - type: 'content', - content: { - type: 'text', - text: confirmationDetails.plan, - }, - }); - } + // Centralised rule injection (for display and persistence) + injectPermissionRulesIfMissing(confirmationDetails, pmCtx); - // Map tool kind, using switch_mode for exit_plan_mode per ACP spec - const mappedKind = this.toolCallEmitter.mapToolKind(tool.kind, fc.name); + const messageBus = this.config.getMessageBus?.(); + const hooksEnabled = this.config.getEnableHooks?.() ?? false; + let hookHandled = false; - const params: RequestPermissionRequest = { - sessionId: this.sessionId, - options: toPermissionOptions(confirmationDetails), - toolCall: { - toolCallId: callId, - status: 'pending', - title: invocation.getDescription(), - content, - locations: invocation.toolLocations(), - kind: mappedKind, - rawInput: args, - }, - }; + if (hooksEnabled && messageBus) { + const hookResult = await firePermissionRequestHook( + messageBus, + fc.name, + args, + String(approvalMode), + ); - const output = (await this.client.requestPermission( - params, - )) as RequestPermissionResponse & { - answers?: Record; - }; - const outcome = - output.outcome.outcome === 'cancelled' - ? ToolConfirmationOutcome.Cancel - : z - .nativeEnum(ToolConfirmationOutcome) - .parse(output.outcome.optionId); - - await confirmationDetails.onConfirm(outcome, { - answers: output.answers, - }); + if (hookResult.hasDecision) { + hookHandled = true; + if (hookResult.shouldAllow) { + if (hookResult.updatedInput) { + args = hookResult.updatedInput; + invocation.params = + hookResult.updatedInput as typeof invocation.params; + } - // After exit_plan_mode confirmation, send current_mode_update notification - if (isExitPlanModeTool && outcome !== ToolConfirmationOutcome.Cancel) { - await this.sendCurrentModeUpdateNotification(outcome); + await this.resolveIdeDiffForOutcome( + confirmationDetails, + ToolConfirmationOutcome.ProceedOnce, + ); + await confirmationDetails.onConfirm( + ToolConfirmationOutcome.ProceedOnce, + ); + } else { + return earlyErrorResponse( + new Error( + hookResult.denyMessage || + `Permission denied by hook for "${fc.name}"`, + ), + fc.name, + ); + } + } } - switch (outcome) { - case ToolConfirmationOutcome.Cancel: - return errorResponse( - new Error(`Tool "${fc.name}" was canceled by the user.`), + // AUTO_EDIT mode: auto-approve edit and info tools + // (same as coreToolScheduler L5 — NOT delegated to the extension) + if ( + approvalMode === ApprovalMode.AUTO_EDIT && + (confirmationDetails.type === 'edit' || + confirmationDetails.type === 'info') + ) { + // Auto-approve, skip requestPermission. + // didRequestPermission stays false → emitStart below. + } else if (!hookHandled) { + // Show permission dialog via ACP requestPermission + didRequestPermission = true; + const content = buildPermissionRequestContent(confirmationDetails); + + // Map tool kind, using switch_mode for exit_plan_mode per ACP spec + const mappedKind = this.toolCallEmitter.mapToolKind( + tool.kind, + fc.name, + ); + + if (hooksEnabled && messageBus) { + void fireNotificationHook( + messageBus, + `Qwen Code needs your permission to use ${fc.name}`, + NotificationType.PermissionPrompt, + 'Permission needed', ); - case ToolConfirmationOutcome.ProceedOnce: - case ToolConfirmationOutcome.ProceedAlways: - case ToolConfirmationOutcome.ProceedAlwaysProject: - case ToolConfirmationOutcome.ProceedAlwaysUser: - case ToolConfirmationOutcome.ProceedAlwaysServer: - case ToolConfirmationOutcome.ProceedAlwaysTool: - case ToolConfirmationOutcome.ModifyWithEditor: - break; - default: { - const resultOutcome: never = outcome; - throw new Error(`Unexpected: ${resultOutcome}`); + } + + const params: RequestPermissionRequest = { + sessionId: this.sessionId, + options: toPermissionOptions(confirmationDetails, pmForcedAsk), + toolCall: { + toolCallId: callId, + status: 'pending', + title: invocation.getDescription(), + content, + locations: invocation.toolLocations(), + kind: mappedKind, + rawInput: args, + }, + }; + + const output = (await this.client.requestPermission( + params, + )) as RequestPermissionResponse & { + answers?: Record; + }; + const outcome = + output.outcome.outcome === 'cancelled' + ? ToolConfirmationOutcome.Cancel + : z + .nativeEnum(ToolConfirmationOutcome) + .parse(output.outcome.optionId); + + await this.resolveIdeDiffForOutcome(confirmationDetails, outcome); + + await confirmationDetails.onConfirm(outcome, { + answers: output.answers, + }); + + // Persist permission rules when user explicitly chose "Always Allow". + // This branch is only reached for tools that went through + // requestPermission (user saw dialog and made a choice). + // AUTO_EDIT auto-approved tools never reach here. + if ( + outcome === ToolConfirmationOutcome.ProceedAlways || + outcome === ToolConfirmationOutcome.ProceedAlwaysProject || + outcome === ToolConfirmationOutcome.ProceedAlwaysUser + ) { + await persistPermissionOutcome( + outcome, + confirmationDetails, + this.config.getOnPersistPermissionRule?.(), + this.config.getPermissionManager?.(), + { answers: output.answers }, + ); + } + + // After exit_plan_mode confirmation, send current_mode_update + if ( + isExitPlanModeTool && + outcome !== ToolConfirmationOutcome.Cancel + ) { + await this.sendCurrentModeUpdateNotification(outcome); + } + + // After edit tool ProceedAlways, notify the client about mode change + if ( + confirmationDetails.type === 'edit' && + outcome === ToolConfirmationOutcome.ProceedAlways + ) { + await this.sendCurrentModeUpdateNotification(outcome); + } + + switch (outcome) { + case ToolConfirmationOutcome.Cancel: + return errorResponse( + new Error(`Tool "${fc.name}" was canceled by the user.`), + ); + case ToolConfirmationOutcome.ProceedOnce: + case ToolConfirmationOutcome.ProceedAlways: + case ToolConfirmationOutcome.ProceedAlwaysProject: + case ToolConfirmationOutcome.ProceedAlwaysUser: + case ToolConfirmationOutcome.ProceedAlwaysServer: + case ToolConfirmationOutcome.ProceedAlwaysTool: + case ToolConfirmationOutcome.ModifyWithEditor: + break; + default: { + const resultOutcome: never = outcome; + throw new Error(`Unexpected: ${resultOutcome}`); + } } } - } else if (!isTodoWriteTool) { - // Skip tool_call event for TodoWriteTool - use ToolCallEmitter + } + + if (!didRequestPermission && !isTodoWriteTool) { + // Auto-approved (L3 allow / L4 PM allow / L5 YOLO|AUTO_EDIT) + // → emit tool_call start notification const startParams: ToolCallStartParams = { callId, toolName: fc.name, @@ -1041,113 +1212,3 @@ export class Session implements SessionContext { } } } - -// ============================================================================ -// Helper functions -// ============================================================================ - -const basicPermissionOptions = [ - { - optionId: ToolConfirmationOutcome.ProceedOnce, - name: 'Allow', - kind: 'allow_once', - }, - { - optionId: ToolConfirmationOutcome.Cancel, - name: 'Reject', - kind: 'reject_once', - }, -] as const; - -function toPermissionOptions( - confirmation: ToolCallConfirmationDetails, -): PermissionOption[] { - switch (confirmation.type) { - case 'edit': - return [ - { - optionId: ToolConfirmationOutcome.ProceedAlways, - name: 'Allow All Edits', - kind: 'allow_always', - }, - ...basicPermissionOptions, - ]; - case 'exec': - return [ - { - optionId: ToolConfirmationOutcome.ProceedAlwaysProject, - name: `Always Allow in project: ${confirmation.rootCommand}`, - kind: 'allow_always', - }, - { - optionId: ToolConfirmationOutcome.ProceedAlwaysUser, - name: `Always Allow for user: ${confirmation.rootCommand}`, - kind: 'allow_always', - }, - ...basicPermissionOptions, - ]; - case 'mcp': - return [ - { - optionId: ToolConfirmationOutcome.ProceedAlwaysProject, - name: `Always Allow in project: ${confirmation.toolName}`, - kind: 'allow_always', - }, - { - optionId: ToolConfirmationOutcome.ProceedAlwaysUser, - name: `Always Allow for user: ${confirmation.toolName}`, - kind: 'allow_always', - }, - ...basicPermissionOptions, - ]; - case 'info': - return [ - { - optionId: ToolConfirmationOutcome.ProceedAlwaysProject, - name: `Always Allow in project`, - kind: 'allow_always', - }, - { - optionId: ToolConfirmationOutcome.ProceedAlwaysUser, - name: `Always Allow for user`, - kind: 'allow_always', - }, - ...basicPermissionOptions, - ]; - case 'plan': - return [ - { - optionId: ToolConfirmationOutcome.ProceedAlways, - name: `Yes, and auto-accept edits`, - kind: 'allow_always', - }, - { - optionId: ToolConfirmationOutcome.ProceedOnce, - name: `Yes, and manually approve edits`, - kind: 'allow_once', - }, - { - optionId: ToolConfirmationOutcome.Cancel, - name: `No, keep planning (esc)`, - kind: 'reject_once', - }, - ]; - case 'ask_user_question': - return [ - { - optionId: ToolConfirmationOutcome.ProceedOnce, - name: 'Submit', - kind: 'allow_once', - }, - { - optionId: ToolConfirmationOutcome.Cancel, - name: 'Cancel', - kind: 'reject_once', - }, - ]; - default: { - const unreachable: never = confirmation; - throw new Error(`Unexpected: ${unreachable}`); - } - } -} diff --git a/packages/cli/src/acp-integration/session/SubAgentTracker.test.ts b/packages/cli/src/acp-integration/session/SubAgentTracker.test.ts index 0be126ff43..7e65d95eb5 100644 --- a/packages/cli/src/acp-integration/session/SubAgentTracker.test.ts +++ b/packages/cli/src/acp-integration/session/SubAgentTracker.test.ts @@ -488,6 +488,9 @@ describe('SubAgentTracker', () => { await vi.waitFor(() => { expect(respondSpy).toHaveBeenCalledWith( ToolConfirmationOutcome.ProceedOnce, + { + answers: undefined, + }, ); }); }); @@ -528,7 +531,58 @@ describe('SubAgentTracker', () => { eventEmitter.emit(AgentEventType.TOOL_WAITING_APPROVAL, event); await vi.waitFor(() => { - expect(respondSpy).toHaveBeenCalledWith(ToolConfirmationOutcome.Cancel); + expect(respondSpy).toHaveBeenCalledWith( + ToolConfirmationOutcome.Cancel, + { + answers: undefined, + }, + ); + }); + }); + + it('should forward answers payload from ACP permission responses', async () => { + requestPermissionSpy.mockResolvedValue({ + outcome: { + outcome: 'selected', + optionId: ToolConfirmationOutcome.ProceedOnce, + }, + answers: { + answer: 'yes', + }, + }); + tracker.setup(eventEmitter, abortController.signal); + + const respondSpy = vi.fn().mockResolvedValue(undefined); + const confirmationDetails = { + type: 'ask_user_question', + title: 'Question', + questions: [ + { + question: 'Continue?', + header: 'Question', + options: [], + multiSelect: false, + }, + ], + } as unknown as AgentApprovalRequestEvent['confirmationDetails']; + const event = createApprovalEvent({ + name: 'ask_user_question', + callId: 'call-ask', + confirmationDetails, + respond: respondSpy, + }); + + eventEmitter.emit(AgentEventType.TOOL_WAITING_APPROVAL, event); + + await vi.waitFor(() => { + expect(respondSpy).toHaveBeenCalledWith( + ToolConfirmationOutcome.ProceedOnce, + { + answers: { + answer: 'yes', + }, + }, + ); }); }); }); diff --git a/packages/cli/src/acp-integration/session/SubAgentTracker.ts b/packages/cli/src/acp-integration/session/SubAgentTracker.ts index 5536390bc0..133339fad6 100644 --- a/packages/cli/src/acp-integration/session/SubAgentTracker.ts +++ b/packages/cli/src/acp-integration/session/SubAgentTracker.ts @@ -26,44 +26,15 @@ import { ToolCallEmitter } from './emitters/ToolCallEmitter.js'; import { MessageEmitter } from './emitters/MessageEmitter.js'; import type { AgentSideConnection, - PermissionOption, RequestPermissionRequest, - ToolCallContent, } from '@agentclientprotocol/sdk'; +import { + buildPermissionRequestContent, + toPermissionOptions, +} from './permissionUtils.js'; const debugLogger = createDebugLogger('ACP_SUBAGENT_TRACKER'); -/** - * Permission option kind type matching ACP schema. - */ -type PermissionKind = - | 'allow_once' - | 'reject_once' - | 'allow_always' - | 'reject_always'; - -/** - * Configuration for permission options displayed to users. - */ -interface PermissionOptionConfig { - optionId: ToolConfirmationOutcome; - name: string; - kind: PermissionKind; -} - -const basicPermissionOptions: readonly PermissionOptionConfig[] = [ - { - optionId: ToolConfirmationOutcome.ProceedOnce, - name: 'Allow', - kind: 'allow_once', - }, - { - optionId: ToolConfirmationOutcome.Cancel, - name: 'Reject', - kind: 'reject_once', - }, -] as const; - /** * Tracks and emits events for sub-agent tool calls within AgentTool execution. * @@ -219,23 +190,6 @@ export class SubAgentTracker { if (abortSignal.aborted) return; const state = this.toolStates.get(event.callId); - const content: ToolCallContent[] = []; - - // Handle edit confirmation type - show diff - if (event.confirmationDetails.type === 'edit') { - const editDetails = event.confirmationDetails as unknown as { - type: 'edit'; - fileName: string; - originalContent: string | null; - newContent: string; - }; - content.push({ - type: 'diff', - path: editDetails.fileName, - oldText: editDetails.originalContent ?? '', - newText: editDetails.newContent, - }); - } // Build permission request const fullConfirmationDetails = { @@ -250,12 +204,12 @@ export class SubAgentTracker { const params: RequestPermissionRequest = { sessionId: this.ctx.sessionId, - options: this.toPermissionOptions(fullConfirmationDetails), + options: toPermissionOptions(fullConfirmationDetails), toolCall: { toolCallId: event.callId, status: 'pending', title, - content, + content: buildPermissionRequestContent(fullConfirmationDetails), locations, kind, rawInput: state?.args, @@ -273,7 +227,9 @@ export class SubAgentTracker { .parse(output.outcome.optionId); // Respond to subagent with the outcome - await event.respond(outcome); + await event.respond(outcome, { + answers: 'answers' in output ? output.answers : undefined, + }); } catch (error) { // If permission request fails, cancel the tool call debugLogger.error( @@ -323,92 +279,4 @@ export class SubAgentTracker { ); }; } - - /** - * Converts confirmation details to permission options for the client. - */ - private toPermissionOptions( - confirmation: ToolCallConfirmationDetails, - ): PermissionOption[] { - const hideAlwaysAllow = - 'hideAlwaysAllow' in confirmation && confirmation.hideAlwaysAllow; - switch (confirmation.type) { - case 'edit': - return [ - { - optionId: ToolConfirmationOutcome.ProceedAlways, - name: 'Allow All Edits', - kind: 'allow_always', - }, - ...basicPermissionOptions, - ]; - case 'exec': - return [ - ...(hideAlwaysAllow - ? [] - : [ - { - optionId: ToolConfirmationOutcome.ProceedAlwaysProject, - name: `Always Allow in project: ${(confirmation as { rootCommand?: string }).rootCommand ?? 'command'}`, - kind: 'allow_always' as const, - }, - { - optionId: ToolConfirmationOutcome.ProceedAlwaysUser, - name: `Always Allow for user: ${(confirmation as { rootCommand?: string }).rootCommand ?? 'command'}`, - kind: 'allow_always' as const, - }, - ]), - ...basicPermissionOptions, - ]; - case 'mcp': - return [ - ...(hideAlwaysAllow - ? [] - : [ - { - optionId: ToolConfirmationOutcome.ProceedAlwaysProject, - name: `Always Allow in project: ${(confirmation as { toolName?: string }).toolName ?? 'tool'}`, - kind: 'allow_always' as const, - }, - { - optionId: ToolConfirmationOutcome.ProceedAlwaysUser, - name: `Always Allow for user: ${(confirmation as { toolName?: string }).toolName ?? 'tool'}`, - kind: 'allow_always' as const, - }, - ]), - ...basicPermissionOptions, - ]; - case 'info': - return [ - ...(hideAlwaysAllow - ? [] - : [ - { - optionId: ToolConfirmationOutcome.ProceedAlwaysProject, - name: 'Always Allow in project', - kind: 'allow_always' as const, - }, - { - optionId: ToolConfirmationOutcome.ProceedAlwaysUser, - name: 'Always Allow for user', - kind: 'allow_always' as const, - }, - ]), - ...basicPermissionOptions, - ]; - case 'plan': - return [ - { - optionId: ToolConfirmationOutcome.ProceedAlways, - name: 'Always Allow Plans', - kind: 'allow_always', - }, - ...basicPermissionOptions, - ]; - default: { - // Fallback for unknown types - return [...basicPermissionOptions]; - } - } - } } diff --git a/packages/cli/src/acp-integration/session/permissionUtils.test.ts b/packages/cli/src/acp-integration/session/permissionUtils.test.ts new file mode 100644 index 0000000000..743049f2e6 --- /dev/null +++ b/packages/cli/src/acp-integration/session/permissionUtils.test.ts @@ -0,0 +1,54 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, expect, it } from 'vitest'; +import { ToolConfirmationOutcome } from '@qwen-code/qwen-code-core'; +import { toPermissionOptions } from './permissionUtils.js'; + +describe('permissionUtils', () => { + describe('toPermissionOptions', () => { + it('uses permissionRules for exec always-allow labels when available', () => { + const options = toPermissionOptions({ + type: 'exec', + title: 'Confirm Shell Command', + command: 'git add package.json', + rootCommand: 'git', + permissionRules: ['Bash(git add *)'], + onConfirm: async () => undefined, + }); + + expect(options).toContainEqual( + expect.objectContaining({ + optionId: ToolConfirmationOutcome.ProceedAlwaysProject, + name: 'Always Allow in project: git add *', + }), + ); + expect(options).toContainEqual( + expect.objectContaining({ + optionId: ToolConfirmationOutcome.ProceedAlwaysUser, + name: 'Always Allow for user: git add *', + }), + ); + }); + + it('falls back to rootCommand when exec permissionRules are unavailable', () => { + const options = toPermissionOptions({ + type: 'exec', + title: 'Confirm Shell Command', + command: 'git add package.json', + rootCommand: 'git', + onConfirm: async () => undefined, + }); + + expect(options).toContainEqual( + expect.objectContaining({ + optionId: ToolConfirmationOutcome.ProceedAlwaysProject, + name: 'Always Allow in project: git', + }), + ); + }); + }); +}); diff --git a/packages/cli/src/acp-integration/session/permissionUtils.ts b/packages/cli/src/acp-integration/session/permissionUtils.ts new file mode 100644 index 0000000000..fbbc0ef4c6 --- /dev/null +++ b/packages/cli/src/acp-integration/session/permissionUtils.ts @@ -0,0 +1,208 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import type { ToolCallConfirmationDetails } from '@qwen-code/qwen-code-core'; +import { ToolConfirmationOutcome } from '@qwen-code/qwen-code-core'; +import type { + PermissionOption, + ToolCallContent, +} from '@agentclientprotocol/sdk'; + +const basicPermissionOptions = [ + { + optionId: ToolConfirmationOutcome.ProceedOnce, + name: 'Allow', + kind: 'allow_once', + }, + { + optionId: ToolConfirmationOutcome.Cancel, + name: 'Reject', + kind: 'reject_once', + }, +] as const satisfies readonly PermissionOption[]; + +function supportsHideAlwaysAllow( + confirmation: ToolCallConfirmationDetails, +): confirmation is Exclude< + ToolCallConfirmationDetails, + { type: 'ask_user_question' } +> { + return confirmation.type !== 'ask_user_question'; +} + +function filterAlwaysAllowOptions( + confirmation: ToolCallConfirmationDetails, + options: PermissionOption[], + forceHideAlwaysAllow = false, +): PermissionOption[] { + const hideAlwaysAllow = + forceHideAlwaysAllow || + (supportsHideAlwaysAllow(confirmation) && + confirmation.hideAlwaysAllow === true); + return hideAlwaysAllow + ? options.filter((option) => option.kind !== 'allow_always') + : options; +} + +function formatExecPermissionScopeLabel( + confirmation: Extract, +): string { + const permissionRules = confirmation.permissionRules ?? []; + const bashRules = permissionRules + .map((rule) => { + const match = /^Bash\((.*)\)$/.exec(rule.trim()); + return match?.[1]?.trim() || undefined; + }) + .filter((rule): rule is string => Boolean(rule)); + + const uniqueRules = [...new Set(bashRules)]; + if (uniqueRules.length === 1) { + return uniqueRules[0]; + } + if (uniqueRules.length > 1) { + return uniqueRules.join(', '); + } + return confirmation.rootCommand; +} + +export function buildPermissionRequestContent( + confirmation: ToolCallConfirmationDetails, +): ToolCallContent[] { + const content: ToolCallContent[] = []; + + if (confirmation.type === 'edit') { + content.push({ + type: 'diff', + path: confirmation.fileName, + oldText: confirmation.originalContent ?? '', + newText: confirmation.newContent, + }); + } + + if (confirmation.type === 'plan') { + content.push({ + type: 'content', + content: { + type: 'text', + text: confirmation.plan, + }, + }); + } + + return content; +} + +export function toPermissionOptions( + confirmation: ToolCallConfirmationDetails, + forceHideAlwaysAllow = false, +): PermissionOption[] { + switch (confirmation.type) { + case 'edit': + return filterAlwaysAllowOptions( + confirmation, + [ + { + optionId: ToolConfirmationOutcome.ProceedAlways, + name: 'Allow All Edits', + kind: 'allow_always', + }, + ...basicPermissionOptions, + ], + forceHideAlwaysAllow, + ); + case 'exec': { + const label = formatExecPermissionScopeLabel(confirmation); + return filterAlwaysAllowOptions( + confirmation, + [ + { + optionId: ToolConfirmationOutcome.ProceedAlwaysProject, + name: `Always Allow in project: ${label}`, + kind: 'allow_always', + }, + { + optionId: ToolConfirmationOutcome.ProceedAlwaysUser, + name: `Always Allow for user: ${label}`, + kind: 'allow_always', + }, + ...basicPermissionOptions, + ], + forceHideAlwaysAllow, + ); + } + case 'mcp': + return filterAlwaysAllowOptions( + confirmation, + [ + { + optionId: ToolConfirmationOutcome.ProceedAlwaysProject, + name: `Always Allow in project: ${confirmation.toolName}`, + kind: 'allow_always', + }, + { + optionId: ToolConfirmationOutcome.ProceedAlwaysUser, + name: `Always Allow for user: ${confirmation.toolName}`, + kind: 'allow_always', + }, + ...basicPermissionOptions, + ], + forceHideAlwaysAllow, + ); + case 'info': + return filterAlwaysAllowOptions( + confirmation, + [ + { + optionId: ToolConfirmationOutcome.ProceedAlwaysProject, + name: 'Always Allow in project', + kind: 'allow_always', + }, + { + optionId: ToolConfirmationOutcome.ProceedAlwaysUser, + name: 'Always Allow for user', + kind: 'allow_always', + }, + ...basicPermissionOptions, + ], + forceHideAlwaysAllow, + ); + case 'plan': + return [ + { + optionId: ToolConfirmationOutcome.ProceedAlways, + name: 'Yes, and auto-accept edits', + kind: 'allow_always', + }, + { + optionId: ToolConfirmationOutcome.ProceedOnce, + name: 'Yes, and manually approve edits', + kind: 'allow_once', + }, + { + optionId: ToolConfirmationOutcome.Cancel, + name: 'No, keep planning (esc)', + kind: 'reject_once', + }, + ]; + case 'ask_user_question': + return [ + { + optionId: ToolConfirmationOutcome.ProceedOnce, + name: 'Submit', + kind: 'allow_once', + }, + { + optionId: ToolConfirmationOutcome.Cancel, + name: 'Cancel', + kind: 'reject_once', + }, + ]; + default: { + const unreachable: never = confirmation; + throw new Error(`Unexpected: ${unreachable}`); + } + } +} diff --git a/packages/cli/src/config/config.ts b/packages/cli/src/config/config.ts index 78ef3dde03..fcc33f76ad 100755 --- a/packages/cli/src/config/config.ts +++ b/packages/cli/src/config/config.ts @@ -33,8 +33,8 @@ import { } from '@qwen-code/qwen-code-core'; import { extensionsCommand } from '../commands/extensions.js'; import { hooksCommand } from '../commands/hooks.js'; -import type { Settings, LoadedSettings } from './settings.js'; -import { SettingScope } from './settings.js'; +import type { Settings } from './settings.js'; +import { loadSettings, SettingScope } from './settings.js'; import { authCommand } from '../commands/auth.js'; import { resolveCliGenerationConfig, @@ -704,7 +704,6 @@ export async function loadCliConfig( argv: CliArgs, cwd: string = process.cwd(), overrideExtensions?: string[], - loadedSettings?: LoadedSettings, ): Promise { const debugMode = isDebugMode(argv); @@ -1042,20 +1041,19 @@ export async function loadCliConfig( deny: mergedDeny.length > 0 ? mergedDeny : undefined, }, // Permission rule persistence callback (writes to settings files). - onPersistPermissionRule: loadedSettings - ? async (scope, ruleType, rule) => { - const settingScope = - scope === 'project' ? SettingScope.Workspace : SettingScope.User; - const key = `permissions.${ruleType}`; - const currentRules: string[] = - loadedSettings.forScope(settingScope).settings.permissions?.[ - ruleType - ] ?? []; - if (!currentRules.includes(rule)) { - loadedSettings.setValue(settingScope, key, [...currentRules, rule]); - } - } - : undefined, + onPersistPermissionRule: async (scope, ruleType, rule) => { + const currentSettings = loadSettings(cwd); + const settingScope = + scope === 'project' ? SettingScope.Workspace : SettingScope.User; + const key = `permissions.${ruleType}`; + const currentRules: string[] = + currentSettings.forScope(settingScope).settings.permissions?.[ + ruleType + ] ?? []; + if (!currentRules.includes(rule)) { + currentSettings.setValue(settingScope, key, [...currentRules, rule]); + } + }, toolDiscoveryCommand: settings.tools?.discoveryCommand, toolCallCommand: settings.tools?.callCommand, mcpServerCommand: settings.mcp?.serverCommand, diff --git a/packages/cli/src/gemini.tsx b/packages/cli/src/gemini.tsx index b28ed2591e..aebb67993b 100644 --- a/packages/cli/src/gemini.tsx +++ b/packages/cli/src/gemini.tsx @@ -358,7 +358,6 @@ export async function main() { argv, process.cwd(), argv.extensions, - settings, ); // Register cleanup for MCP clients as early as possible diff --git a/packages/cli/src/services/prompt-processors/shellProcessor.ts b/packages/cli/src/services/prompt-processors/shellProcessor.ts index f499c27134..679e1d0c6e 100644 --- a/packages/cli/src/services/prompt-processors/shellProcessor.ts +++ b/packages/cli/src/services/prompt-processors/shellProcessor.ts @@ -133,12 +133,12 @@ export class ShellProcessor implements IPromptProcessor { // Security check on the final, escaped command string. const { allAllowed, disallowedCommands, blockReason, isHardDenial } = - checkCommandPermissions(command, config, sessionShellAllowlist); + await checkCommandPermissions(command, config, sessionShellAllowlist); // Determine if this command is explicitly auto-approved via PermissionManager const pm = config.getPermissionManager?.(); const isAllowedBySettings = pm - ? pm.isCommandAllowed(command) === 'allow' + ? (await pm.isCommandAllowed(command)) === 'allow' : false; if (!allAllowed) { diff --git a/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx b/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx index 3946b0b05d..be2720b75a 100644 --- a/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx +++ b/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx @@ -74,6 +74,12 @@ export const ToolConfirmationMessage: React.FC< }, [config]); const handleConfirm = async (outcome: ToolConfirmationOutcome) => { + // Call onConfirm before resolving the IDE diff so that the CLI outcome + // (e.g. ProceedAlways) is processed first. resolveDiffFromCli would + // otherwise trigger the scheduler's ideConfirmation .then() handler + // with ProceedOnce, racing with the intended CLI outcome. + onConfirm(outcome); + if (confirmationDetails.type === 'edit') { if (config.getIdeMode() && isDiffingEnabled) { const cliOutcome = @@ -84,7 +90,6 @@ export const ToolConfirmationMessage: React.FC< ); } } - onConfirm(outcome); }; const isTrustedFolder = config.isTrustedFolder(); diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index a69e4d29b0..dc743d9b98 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -2111,7 +2111,7 @@ export class Config { // Helper to create & register core tools that are enabled // eslint-disable-next-line @typescript-eslint/no-explicit-any - const registerCoreTool = (ToolClass: any, ...args: unknown[]) => { + const registerCoreTool = async (ToolClass: any, ...args: unknown[]) => { const toolName = ToolClass?.Name as ToolName | undefined; const className = ToolClass?.name ?? 'UnknownTool'; @@ -2127,7 +2127,7 @@ export class Config { // PermissionManager handles both the coreTools allowlist (registry-level) // and deny rules (runtime-level) in a single check. const pmEnabled = this.permissionManager - ? this.permissionManager.isToolEnabled(toolName) + ? await this.permissionManager.isToolEnabled(toolName) : true; // Should never reach here after initialize(), but safe default. if (pmEnabled) { @@ -2143,10 +2143,10 @@ export class Config { } }; - registerCoreTool(AgentTool, this); - registerCoreTool(SkillTool, this); - registerCoreTool(LSTool, this); - registerCoreTool(ReadFileTool, this); + await registerCoreTool(AgentTool, this); + await registerCoreTool(SkillTool, this); + await registerCoreTool(LSTool, this); + await registerCoreTool(ReadFileTool, this); if (this.getUseRipgrep()) { let useRipgrep = false; @@ -2157,7 +2157,7 @@ export class Config { errorString = getErrorMessage(error); } if (useRipgrep) { - registerCoreTool(RipGrepTool, this); + await registerCoreTool(RipGrepTool, this); } else { // Log for telemetry logRipgrepFallback( @@ -2168,30 +2168,30 @@ export class Config { errorString || 'ripgrep is not available', ), ); - registerCoreTool(GrepTool, this); + await registerCoreTool(GrepTool, this); } } else { - registerCoreTool(GrepTool, this); + await registerCoreTool(GrepTool, this); } - registerCoreTool(GlobTool, this); - registerCoreTool(EditTool, this); - registerCoreTool(WriteFileTool, this); - registerCoreTool(ShellTool, this); - registerCoreTool(MemoryTool); - registerCoreTool(TodoWriteTool, this); - registerCoreTool(AskUserQuestionTool, this); - !this.sdkMode && registerCoreTool(ExitPlanModeTool, this); - registerCoreTool(WebFetchTool, this); + await registerCoreTool(GlobTool, this); + await registerCoreTool(EditTool, this); + await registerCoreTool(WriteFileTool, this); + await registerCoreTool(ShellTool, this); + await registerCoreTool(MemoryTool); + await registerCoreTool(TodoWriteTool, this); + await registerCoreTool(AskUserQuestionTool, this); + !this.sdkMode && (await registerCoreTool(ExitPlanModeTool, this)); + await registerCoreTool(WebFetchTool, this); // Conditionally register web search tool if web search provider is configured // buildWebSearchConfig ensures qwen-oauth users get dashscope provider, so // if tool is registered, config must exist if (this.getWebSearchConfig()) { - registerCoreTool(WebSearchTool, this); + await registerCoreTool(WebSearchTool, this); } if (this.isLspEnabled() && this.getLspClient()) { // Register the unified LSP tool - registerCoreTool(LspTool, this); + await registerCoreTool(LspTool, this); } if (!options?.skipDiscovery) { diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index 097120d08c..c20c917614 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -49,7 +49,12 @@ import type { PartListUnion, } from '@google/genai'; import { ToolNames } from '../tools/tool-names.js'; -import { buildPermissionRules } from '../permissions/rule-parser.js'; +import { + buildPermissionCheckContext, + evaluatePermissionRules, + injectPermissionRulesIfMissing, + persistPermissionOutcome, +} from './permission-helpers.js'; import { getResponseTextFromParts } from '../utils/generateContentResponseUtilities.js'; import type { ModifyContext } from '../tools/modifiable-tool.js'; import { @@ -57,7 +62,6 @@ import { modifyWithEditor, } from '../tools/modifiable-tool.js'; import * as Diff from 'diff'; -import * as path from 'node:path'; import levenshtein from 'fast-levenshtein'; import { getPlanModeSystemReminder } from './prompts.js'; import { ShellToolInvocation } from '../tools/shell.js'; @@ -695,116 +699,119 @@ export class CoreToolScheduler { } const requestsToProcess = Array.isArray(request) ? request : [request]; - const newToolCalls: ToolCall[] = requestsToProcess.map( - (reqInfo): ToolCall => { - // Check if the tool is excluded due to permissions/environment restrictions - // This check should happen before registry lookup to provide a clear permission error - const pm = this.config.getPermissionManager?.(); - if (pm && !pm.isToolEnabled(reqInfo.name)) { - const permissionErrorMessage = `Qwen Code requires permission to use "${reqInfo.name}", but that permission was declined.`; - return { - status: 'error', - request: reqInfo, - response: createErrorResponse( - reqInfo, - new Error(permissionErrorMessage), - ToolErrorType.EXECUTION_DENIED, - ), - durationMs: 0, - }; - } + const newToolCalls: ToolCall[] = []; + for (const reqInfo of requestsToProcess) { + // Check if the tool is excluded due to permissions/environment restrictions + // This check should happen before registry lookup to provide a clear permission error + const pm = this.config.getPermissionManager?.(); + if (pm && !(await pm.isToolEnabled(reqInfo.name))) { + const permissionErrorMessage = `Qwen Code requires permission to use "${reqInfo.name}", but that permission was declined.`; + newToolCalls.push({ + status: 'error', + request: reqInfo, + response: createErrorResponse( + reqInfo, + new Error(permissionErrorMessage), + ToolErrorType.EXECUTION_DENIED, + ), + durationMs: 0, + }); + continue; + } - // Legacy fallback: check getPermissionsDeny() when PM is not available - if (!pm) { - const excludeTools = - this.config.getPermissionsDeny?.() ?? undefined; - if (excludeTools && excludeTools.length > 0) { - const normalizedToolName = reqInfo.name.toLowerCase().trim(); - const excludedMatch = excludeTools.find( - (excludedTool) => - excludedTool.toLowerCase().trim() === normalizedToolName, - ); - if (excludedMatch) { - const permissionErrorMessage = `Qwen Code requires permission to use ${excludedMatch}, but that permission was declined.`; - return { - status: 'error', - request: reqInfo, - response: createErrorResponse( - reqInfo, - new Error(permissionErrorMessage), - ToolErrorType.EXECUTION_DENIED, - ), - durationMs: 0, - }; - } + // Legacy fallback: check getPermissionsDeny() when PM is not available + if (!pm) { + const excludeTools = this.config.getPermissionsDeny?.() ?? undefined; + if (excludeTools && excludeTools.length > 0) { + const normalizedToolName = reqInfo.name.toLowerCase().trim(); + const excludedMatch = excludeTools.find( + (excludedTool) => + excludedTool.toLowerCase().trim() === normalizedToolName, + ); + if (excludedMatch) { + const permissionErrorMessage = `Qwen Code requires permission to use ${excludedMatch}, but that permission was declined.`; + newToolCalls.push({ + status: 'error', + request: reqInfo, + response: createErrorResponse( + reqInfo, + new Error(permissionErrorMessage), + ToolErrorType.EXECUTION_DENIED, + ), + durationMs: 0, + }); + continue; } } + } - const toolInstance = this.toolRegistry.getTool(reqInfo.name); - if (!toolInstance) { - // Tool is not in registry and not excluded - likely hallucinated or typo - const errorMessage = this.getToolNotFoundMessage(reqInfo.name); - return { - status: 'error', - request: reqInfo, - response: createErrorResponse( - reqInfo, - new Error(errorMessage), - ToolErrorType.TOOL_NOT_REGISTERED, - ), - durationMs: 0, - }; - } - - const invocationOrError = this.buildInvocation( - toolInstance, - reqInfo.args, - ); - if (invocationOrError instanceof Error) { - const error = reqInfo.wasOutputTruncated - ? new Error( - `${invocationOrError.message} ${TRUNCATION_PARAM_GUIDANCE}`, - ) - : invocationOrError; - return { - status: 'error', - request: reqInfo, - tool: toolInstance, - response: createErrorResponse( - reqInfo, - error, - ToolErrorType.INVALID_TOOL_PARAMS, - ), - durationMs: 0, - }; - } + const toolInstance = this.toolRegistry.getTool(reqInfo.name); + if (!toolInstance) { + // Tool is not in registry and not excluded - likely hallucinated or typo + const errorMessage = this.getToolNotFoundMessage(reqInfo.name); + newToolCalls.push({ + status: 'error', + request: reqInfo, + response: createErrorResponse( + reqInfo, + new Error(errorMessage), + ToolErrorType.TOOL_NOT_REGISTERED, + ), + durationMs: 0, + }); + continue; + } - // Reject file-modifying calls when truncated to prevent - // writing incomplete content. - if (reqInfo.wasOutputTruncated && toolInstance.kind === Kind.Edit) { - const truncationError = new Error(TRUNCATION_EDIT_REJECTION); - return { - status: 'error', - request: reqInfo, - tool: toolInstance, - response: createErrorResponse( - reqInfo, - truncationError, - ToolErrorType.OUTPUT_TRUNCATED, - ), - durationMs: 0, - }; - } + const invocationOrError = this.buildInvocation( + toolInstance, + reqInfo.args, + ); + if (invocationOrError instanceof Error) { + const error = reqInfo.wasOutputTruncated + ? new Error( + `${invocationOrError.message} ${TRUNCATION_PARAM_GUIDANCE}`, + ) + : invocationOrError; + newToolCalls.push({ + status: 'error', + request: reqInfo, + tool: toolInstance, + response: createErrorResponse( + reqInfo, + error, + ToolErrorType.INVALID_TOOL_PARAMS, + ), + durationMs: 0, + }); + continue; + } - return { - status: 'validating', + // Reject file-modifying calls when truncated to prevent + // writing incomplete content. + if (reqInfo.wasOutputTruncated && toolInstance.kind === Kind.Edit) { + const truncationError = new Error(TRUNCATION_EDIT_REJECTION); + newToolCalls.push({ + status: 'error', request: reqInfo, tool: toolInstance, - invocation: invocationOrError, - startTime: Date.now(), - }; - }, - ); + response: createErrorResponse( + reqInfo, + truncationError, + ToolErrorType.OUTPUT_TRUNCATED, + ), + durationMs: 0, + }); + continue; + } + + newToolCalls.push({ + status: 'validating', + request: reqInfo, + tool: toolInstance, + invocation: invocationOrError, + startTime: Date.now(), + }); + } this.toolCalls = this.toolCalls.concat(newToolCalls); this.notifyToolCallsUpdate(); @@ -836,66 +843,14 @@ export class CoreToolScheduler { // ---- L4: PermissionManager override (if relevant rules exist) ---- const pm = this.config.getPermissionManager?.(); - let finalPermission = defaultPermission; - let pmForcedAsk = false; - - // Build invocation context from tool params. - // This is used both by the PM evaluation below and later by - // centralized permission-rule generation (Always Allow). const toolParams = invocation.params as Record; - const shellCommand = - 'command' in toolParams ? String(toolParams['command']) : undefined; - // Extract file path — tools use 'file_path' or 'path' - // (LS / grep / glob). - let invocationFilePath = - typeof toolParams['file_path'] === 'string' - ? toolParams['file_path'] - : undefined; - if ( - invocationFilePath === undefined && - typeof toolParams['path'] === 'string' - ) { - // LS uses absolute paths; grep/glob may be relative to targetDir. - invocationFilePath = path.isAbsolute(toolParams['path']) - ? toolParams['path'] - : path.resolve(this.config.getTargetDir(), toolParams['path']); - } - let invocationDomain: string | undefined; - if (typeof toolParams['url'] === 'string') { - try { - invocationDomain = new URL(toolParams['url']).hostname; - } catch { - // malformed URL — leave domain undefined - } - } - // Generic specifier for literal matching (Skill name, Task subagent type, etc.) - const literalSpecifier = - typeof toolParams['skill'] === 'string' - ? toolParams['skill'] - : typeof toolParams['subagent_type'] === 'string' - ? toolParams['subagent_type'] - : undefined; - const pmCtx = { - toolName: reqInfo.name, - command: shellCommand, - filePath: invocationFilePath, - domain: invocationDomain, - specifier: literalSpecifier, - }; - - if (pm && defaultPermission !== 'deny') { - if (pm.hasRelevantRules(pmCtx)) { - const pmDecision = pm.evaluate(pmCtx); - if (pmDecision !== 'default') { - finalPermission = pmDecision; - // If PM explicitly forces 'ask', adding allow rules won't help - // because ask has higher priority. Hide "Always allow" options. - if (pmDecision === 'ask') { - pmForcedAsk = true; - } - } - } - } + const pmCtx = buildPermissionCheckContext( + reqInfo.name, + toolParams, + this.config.getTargetDir?.() ?? '', + ); + const { finalPermission, pmForcedAsk } = + await evaluatePermissionRules(pm, defaultPermission, pmCtx); // ---- L5: Final decision based on permission + ApprovalMode ---- const approvalMode = this.config.getApprovalMode(); @@ -965,19 +920,7 @@ export class CoreToolScheduler { await invocation.getConfirmationDetails(signal); // ── Centralised rule injection ────────────────────────────────── - // If the tool did not provide its own permissionRules (e.g. Shell - // and WebFetch already do), generate minimum-scope rules from - // the invocation context so that "Always Allow" persists a - // properly scoped rule rather than nothing. - // Only exec/mcp/info types support the permissionRules field. - if ( - (confirmationDetails.type === 'exec' || - confirmationDetails.type === 'mcp' || - confirmationDetails.type === 'info') && - !confirmationDetails.permissionRules - ) { - confirmationDetails.permissionRules = buildPermissionRules(pmCtx); - } + injectPermissionRulesIfMissing(confirmationDetails, pmCtx); // AUTO_EDIT mode: auto-approve edit-like and info tools if ( @@ -1021,6 +964,17 @@ export class CoreToolScheduler { confirmationDetails.ideConfirmation ) { confirmationDetails.ideConfirmation.then((resolution) => { + // Guard: skip if the tool was already handled (e.g. by CLI + // confirmation). Without this check, resolveDiffFromCli + // triggers this handler AND the CLI's onConfirm, causing a + // race where ProceedOnce overwrites ProceedAlways. + const still = this.toolCalls.find( + (c) => + c.request.callId === reqInfo.callId && + c.status === 'awaiting_approval', + ); + if (!still) return; + if (resolution.status === 'accepted') { this.handleConfirmationResponse( reqInfo.callId, @@ -1185,6 +1139,11 @@ export class CoreToolScheduler { (c) => c.request.callId === callId && c.status === 'awaiting_approval', ); + // Guard: if the tool is no longer awaiting approval (already handled by + // another confirmation path, e.g. IDE vs CLI race), skip to avoid double + // processing and potential re-execution. + if (!toolCall) return; + await originalOnConfirm(outcome, payload); if ( @@ -1193,37 +1152,13 @@ export class CoreToolScheduler { outcome === ToolConfirmationOutcome.ProceedAlwaysUser ) { // Persist permission rules for Project/User scope outcomes - if ( - outcome === ToolConfirmationOutcome.ProceedAlwaysProject || - outcome === ToolConfirmationOutcome.ProceedAlwaysUser - ) { - const scope = - outcome === ToolConfirmationOutcome.ProceedAlwaysProject - ? 'project' - : 'user'; - // Read permissionRules from the stored confirmation details first, - // falling back to payload for backward compatibility. - const details = (toolCall as WaitingToolCall | undefined) - ?.confirmationDetails; - const detailsRules = (details as Record | undefined)?.[ - 'permissionRules' - ] as string[] | undefined; - const payloadRules = payload?.permissionRules; - const rules = payloadRules ?? detailsRules ?? []; - const persistFn = this.config.getOnPersistPermissionRule?.(); - const pm = this.config.getPermissionManager?.(); - if (rules.length > 0) { - for (const rule of rules) { - // 1. Persist to disk (settings.json) - if (persistFn) { - await persistFn(scope, 'allow', rule); - } - // 2. Immediately update in-memory PermissionManager so the - // new rule takes effect without restart. - pm?.addPersistentRule(rule, 'allow'); - } - } - } + await persistPermissionOutcome( + outcome, + (toolCall as WaitingToolCall).confirmationDetails, + this.config.getOnPersistPermissionRule?.(), + this.config.getPermissionManager?.(), + payload, + ); await this.autoApproveCompatiblePendingTools(signal, callId); } @@ -1726,50 +1661,20 @@ export class CoreToolScheduler { // Re-run L3→L4 to see if the tool can now be auto-approved const defaultPermission = await pendingTool.invocation.getDefaultPermission(); - let finalPermission = defaultPermission; - - // L4: PM override - const pm = this.config.getPermissionManager?.(); - if (pm && defaultPermission !== 'deny') { - const params = pendingTool.invocation.params as Record< - string, - unknown - >; - const shellCommand = - 'command' in params ? String(params['command']) : undefined; - const filePath = - typeof params['file_path'] === 'string' - ? params['file_path'] - : undefined; - let domain: string | undefined; - if (typeof params['url'] === 'string') { - try { - domain = new URL(params['url']).hostname; - } catch { - // malformed URL - } - } - // Generic specifier for literal matching (Skill name, Task subagent type, etc.) - const literalSpecifier = - typeof params['skill'] === 'string' - ? params['skill'] - : typeof params['subagent_type'] === 'string' - ? params['subagent_type'] - : undefined; - const pmCtx = { - toolName: pendingTool.request.name, - command: shellCommand, - filePath, - domain, - specifier: literalSpecifier, - }; - if (pm.hasRelevantRules(pmCtx)) { - const pmDecision = pm.evaluate(pmCtx); - if (pmDecision !== 'default') { - finalPermission = pmDecision; - } - } - } + const toolParams = pendingTool.invocation.params as Record< + string, + unknown + >; + const pmCtx = buildPermissionCheckContext( + pendingTool.request.name, + toolParams, + this.config.getTargetDir?.() ?? '', + ); + const { finalPermission } = await evaluatePermissionRules( + this.config.getPermissionManager?.(), + defaultPermission, + pmCtx, + ); if (finalPermission === 'allow') { this.setToolCallOutcome( diff --git a/packages/core/src/core/permission-helpers.ts b/packages/core/src/core/permission-helpers.ts new file mode 100644 index 0000000000..82d960bd6c --- /dev/null +++ b/packages/core/src/core/permission-helpers.ts @@ -0,0 +1,207 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * Shared permission-evaluation and persistence helpers. + * + * These are used by both `coreToolScheduler` (CLI mode) and the ACP + * `Session` (VS Code / webui mode) so that the L3→L4→L5 permission flow + * and the "Always Allow" persistence logic stay in sync. + */ + +import * as path from 'node:path'; +import type { PermissionCheckContext } from '../permissions/types.js'; +import type { PermissionManager } from '../permissions/permission-manager.js'; +import type { + ToolCallConfirmationDetails, + ToolConfirmationPayload, +} from '../tools/tools.js'; +import { ToolConfirmationOutcome } from '../tools/tools.js'; +import { buildPermissionRules } from '../permissions/rule-parser.js'; + +// ───────────────────────────────────────────────────────────────────────────── +// Context building +// ───────────────────────────────────────────────────────────────────────────── + +/** + * Build a {@link PermissionCheckContext} from raw tool invocation parameters. + * + * Extracts `command`, `filePath`, `domain`, and `specifier` fields from the + * tool's params, resolving relative paths against `targetDir`. + */ +export function buildPermissionCheckContext( + toolName: string, + toolParams: Record, + targetDir: string, +): PermissionCheckContext { + const command = + 'command' in toolParams ? String(toolParams['command']) : undefined; + + // Extract file path — tools use 'file_path' or 'path' (LS / grep / glob). + let filePath = + typeof toolParams['file_path'] === 'string' + ? toolParams['file_path'] + : undefined; + if (filePath === undefined && typeof toolParams['path'] === 'string') { + // LS uses absolute paths; grep/glob may be relative to targetDir. + filePath = path.isAbsolute(toolParams['path']) + ? toolParams['path'] + : path.resolve(targetDir, toolParams['path']); + } + + let domain: string | undefined; + if (typeof toolParams['url'] === 'string') { + try { + domain = new URL(toolParams['url']).hostname; + } catch { + // malformed URL — leave domain undefined + } + } + + // Generic specifier for literal matching (Skill name, Task subagent type, etc.) + const specifier = + typeof toolParams['skill'] === 'string' + ? toolParams['skill'] + : typeof toolParams['subagent_type'] === 'string' + ? toolParams['subagent_type'] + : undefined; + + return { toolName, command, filePath, domain, specifier }; +} + +// ───────────────────────────────────────────────────────────────────────────── +// PM evaluation +// ───────────────────────────────────────────────────────────────────────────── + +/** Result of {@link evaluatePermissionRules}. */ +export interface PermissionEvalResult { + /** The final permission after PM override. */ + finalPermission: string; + /** + * `true` when PM explicitly forces `'ask'`. In that case "Always Allow" + * buttons should be hidden because allow rules can never override the + * higher-priority ask rule. + */ + pmForcedAsk: boolean; +} + +/** + * L4 — evaluate {@link PermissionManager} rules against the given context. + * + * Returns the final permission decision and whether PM forced 'ask'. + * When `defaultPermission` is already `'deny'`, PM evaluation is skipped. + */ +export async function evaluatePermissionRules( + pm: PermissionManager | null | undefined, + defaultPermission: string, + pmCtx: PermissionCheckContext, +): Promise { + let finalPermission = defaultPermission; + let pmForcedAsk = false; + + if (pm && defaultPermission !== 'deny') { + if (pm.hasRelevantRules(pmCtx)) { + const pmDecision = await pm.evaluate(pmCtx); + if (pmDecision !== 'default') { + finalPermission = pmDecision; + // If PM explicitly forces 'ask', adding allow rules won't help + // because ask has higher priority. Hide "Always allow" options. + if (pmDecision === 'ask' && pm.hasMatchingAskRule(pmCtx)) { + pmForcedAsk = true; + } + } + } + } + + return { finalPermission, pmForcedAsk }; +} + +// ───────────────────────────────────────────────────────────────────────────── +// Centralised rule injection +// ───────────────────────────────────────────────────────────────────────────── + +/** + * Inject centralized permission rules into confirmation details when the tool + * doesn't provide its own. This ensures "Always Allow" persists a properly + * scoped rule rather than nothing. + * + * Only `exec` / `mcp` / `info` types support the `permissionRules` field. + * Mutates `confirmationDetails` in place. + */ +export function injectPermissionRulesIfMissing( + confirmationDetails: ToolCallConfirmationDetails, + pmCtx: PermissionCheckContext, +): void { + if ( + (confirmationDetails.type === 'exec' || + confirmationDetails.type === 'mcp' || + confirmationDetails.type === 'info') && + !confirmationDetails.permissionRules + ) { + confirmationDetails.permissionRules = buildPermissionRules(pmCtx); + } +} + +// ───────────────────────────────────────────────────────────────────────────── +// Permission persistence +// ───────────────────────────────────────────────────────────────────────────── + +/** + * Persist permission rules for `ProceedAlwaysProject` / `ProceedAlwaysUser` + * outcomes. + * + * Reads rules from `confirmationDetails.permissionRules` (set by the tool or + * by {@link injectPermissionRulesIfMissing}), falling back to + * `payload.permissionRules` for backward compatibility. + * + * Writes to disk via `persistFn` and updates the in-memory + * {@link PermissionManager}. No-op for other outcomes. + */ +export async function persistPermissionOutcome( + outcome: ToolConfirmationOutcome, + confirmationDetails: ToolCallConfirmationDetails, + persistFn: + | (( + scope: 'project' | 'user', + ruleType: 'allow' | 'ask' | 'deny', + rule: string, + ) => Promise) + | undefined, + pm: PermissionManager | null | undefined, + payload?: ToolConfirmationPayload, +): Promise { + if ( + outcome !== ToolConfirmationOutcome.ProceedAlwaysProject && + outcome !== ToolConfirmationOutcome.ProceedAlwaysUser + ) { + return; + } + + const scope = + outcome === ToolConfirmationOutcome.ProceedAlwaysProject + ? 'project' + : 'user'; + + // Read permissionRules from the stored confirmation details first, + // falling back to payload for backward compatibility. + const detailsRules = ( + confirmationDetails as unknown as Record + )?.['permissionRules'] as string[] | undefined; + const payloadRules = payload?.permissionRules; + const rules = payloadRules ?? detailsRules ?? []; + + if (rules.length > 0) { + for (const rule of rules) { + // 1. Persist to disk (settings.json) + if (persistFn) { + await persistFn(scope, 'allow', rule); + } + // 2. Immediately update in-memory PermissionManager so the + // new rule takes effect without restart. + pm?.addPersistentRule(rule, 'allow'); + } + } +} diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 8a498b9128..66359a8652 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -55,6 +55,7 @@ export * from './output/types.js'; export * from './core/client.js'; export * from './core/contentGenerator.js'; export * from './core/coreToolScheduler.js'; +export * from './core/permission-helpers.js'; export * from './core/geminiChat.js'; export * from './core/geminiRequest.js'; export * from './core/logger.js'; @@ -259,5 +260,6 @@ export type { HookRegistryEntry } from './hooks/index.js'; // Export hook triggers for notification hooks export { fireNotificationHook, + firePermissionRequestHook, type NotificationHookResult, } from './core/toolHookTriggers.js'; diff --git a/packages/core/src/permissions/permission-manager.test.ts b/packages/core/src/permissions/permission-manager.test.ts index d15f36b258..627230a8ee 100644 --- a/packages/core/src/permissions/permission-manager.test.ts +++ b/packages/core/src/permissions/permission-manager.test.ts @@ -27,12 +27,12 @@ import type { PermissionManagerConfig } from './permission-manager.js'; // ─── resolveToolName ───────────────────────────────────────────────────────── describe('resolveToolName', () => { - it('resolves canonical names', () => { + it('resolves canonical names', async () => { expect(resolveToolName('run_shell_command')).toBe('run_shell_command'); expect(resolveToolName('read_file')).toBe('read_file'); }); - it('resolves display-name aliases', () => { + it('resolves display-name aliases', async () => { expect(resolveToolName('Shell')).toBe('run_shell_command'); expect(resolveToolName('ShellTool')).toBe('run_shell_command'); expect(resolveToolName('Bash')).toBe('run_shell_command'); @@ -42,25 +42,25 @@ describe('resolveToolName', () => { expect(resolveToolName('WriteFileTool')).toBe('write_file'); }); - it('resolves "Read" and "Edit" meta-categories', () => { + it('resolves "Read" and "Edit" meta-categories', async () => { expect(resolveToolName('Read')).toBe('read_file'); expect(resolveToolName('Edit')).toBe('edit'); expect(resolveToolName('Write')).toBe('write_file'); }); - it('resolves Agent category', () => { + it('resolves Agent category', async () => { expect(resolveToolName('Agent')).toBe('agent'); expect(resolveToolName('agent')).toBe('agent'); expect(resolveToolName('AgentTool')).toBe('agent'); }); - it('resolves legacy task aliases to agent', () => { + it('resolves legacy task aliases to agent', async () => { expect(resolveToolName('task')).toBe('agent'); expect(resolveToolName('Task')).toBe('agent'); expect(resolveToolName('TaskTool')).toBe('agent'); }); - it('returns unknown names unchanged', () => { + it('returns unknown names unchanged', async () => { expect(resolveToolName('my_mcp_tool')).toBe('my_mcp_tool'); expect(resolveToolName('mcp__server__tool')).toBe('mcp__server__tool'); }); @@ -69,11 +69,11 @@ describe('resolveToolName', () => { // ─── getSpecifierKind ──────────────────────────────────────────────────────── describe('getSpecifierKind', () => { - it('returns "command" for shell tools', () => { + it('returns "command" for shell tools', async () => { expect(getSpecifierKind('run_shell_command')).toBe('command'); }); - it('returns "path" for file read/edit tools', () => { + it('returns "path" for file read/edit tools', async () => { expect(getSpecifierKind('read_file')).toBe('path'); expect(getSpecifierKind('edit')).toBe('path'); expect(getSpecifierKind('write_file')).toBe('path'); @@ -82,11 +82,11 @@ describe('getSpecifierKind', () => { expect(getSpecifierKind('list_directory')).toBe('path'); }); - it('returns "domain" for web fetch tools', () => { + it('returns "domain" for web fetch tools', async () => { expect(getSpecifierKind('web_fetch')).toBe('domain'); }); - it('returns "literal" for other tools', () => { + it('returns "literal" for other tools', async () => { expect(getSpecifierKind('Agent')).toBe('literal'); expect(getSpecifierKind('task')).toBe('literal'); expect(getSpecifierKind('mcp__server')).toBe('literal'); @@ -96,22 +96,22 @@ describe('getSpecifierKind', () => { // ─── toolMatchesRuleToolName ───────────────────────────────────────────────── describe('toolMatchesRuleToolName', () => { - it('exact match', () => { + it('exact match', async () => { expect(toolMatchesRuleToolName('read_file', 'read_file')).toBe(true); expect(toolMatchesRuleToolName('edit', 'edit')).toBe(true); }); - it('"Read" (read_file) covers grep_search, glob, list_directory', () => { + it('"Read" (read_file) covers grep_search, glob, list_directory', async () => { expect(toolMatchesRuleToolName('read_file', 'grep_search')).toBe(true); expect(toolMatchesRuleToolName('read_file', 'glob')).toBe(true); expect(toolMatchesRuleToolName('read_file', 'list_directory')).toBe(true); }); - it('"Edit" (edit) covers write_file', () => { + it('"Edit" (edit) covers write_file', async () => { expect(toolMatchesRuleToolName('edit', 'write_file')).toBe(true); }); - it('does not cross categories', () => { + it('does not cross categories', async () => { expect(toolMatchesRuleToolName('read_file', 'edit')).toBe(false); expect(toolMatchesRuleToolName('edit', 'read_file')).toBe(false); expect(toolMatchesRuleToolName('read_file', 'run_shell_command')).toBe( @@ -123,7 +123,7 @@ describe('toolMatchesRuleToolName', () => { // ─── parseRule ─────────────────────────────────────────────────────────────── describe('parseRule', () => { - it('parses a simple tool name', () => { + it('parses a simple tool name', async () => { const r = parseRule('ShellTool'); expect(r.raw).toBe('ShellTool'); expect(r.toolName).toBe('run_shell_command'); @@ -131,59 +131,59 @@ describe('parseRule', () => { expect(r.specifierKind).toBeUndefined(); }); - it('parses Bash alias (Claude Code compat)', () => { + it('parses Bash alias (Claude Code compat)', async () => { const r = parseRule('Bash'); expect(r.toolName).toBe('run_shell_command'); }); - it('parses a shell tool with a specifier', () => { + it('parses a shell tool with a specifier', async () => { const r = parseRule('Bash(git *)'); expect(r.toolName).toBe('run_shell_command'); expect(r.specifier).toBe('git *'); expect(r.specifierKind).toBe('command'); }); - it('parses Read with path specifier', () => { + it('parses Read with path specifier', async () => { const r = parseRule('Read(./secrets/**)'); expect(r.toolName).toBe('read_file'); expect(r.specifier).toBe('./secrets/**'); expect(r.specifierKind).toBe('path'); }); - it('parses Edit with path specifier', () => { + it('parses Edit with path specifier', async () => { const r = parseRule('Edit(/src/**/*.ts)'); expect(r.toolName).toBe('edit'); expect(r.specifier).toBe('/src/**/*.ts'); expect(r.specifierKind).toBe('path'); }); - it('parses WebFetch with domain specifier', () => { + it('parses WebFetch with domain specifier', async () => { const r = parseRule('WebFetch(domain:example.com)'); expect(r.toolName).toBe('web_fetch'); expect(r.specifier).toBe('domain:example.com'); expect(r.specifierKind).toBe('domain'); }); - it('parses Agent with literal specifier', () => { + it('parses Agent with literal specifier', async () => { const r = parseRule('Agent(Explore)'); expect(r.toolName).toBe('agent'); expect(r.specifier).toBe('Explore'); expect(r.specifierKind).toBe('literal'); }); - it('handles unknown tools without specifier', () => { + it('handles unknown tools without specifier', async () => { const r = parseRule('mcp__my_server__my_tool'); expect(r.toolName).toBe('mcp__my_server__my_tool'); expect(r.specifier).toBeUndefined(); }); - it('handles legacy :* suffix (deprecated)', () => { + it('handles legacy :* suffix (deprecated)', async () => { const r = parseRule('Bash(git:*)'); expect(r.toolName).toBe('run_shell_command'); expect(r.specifier).toBe('git *'); }); - it('handles malformed pattern (no closing paren)', () => { + it('handles malformed pattern (no closing paren)', async () => { const r = parseRule('Bash(git status'); expect(r.specifier).toBeUndefined(); }); @@ -192,7 +192,7 @@ describe('parseRule', () => { // ─── parseRules ────────────────────────────────────────────────────────────── describe('parseRules', () => { - it('filters empty strings', () => { + it('filters empty strings', async () => { const rules = parseRules(['ShellTool', '', ' ', 'ReadFileTool']); expect(rules).toHaveLength(2); }); @@ -203,48 +203,48 @@ describe('parseRules', () => { describe('matchesCommandPattern', () => { // Basic prefix matching (no wildcards) describe('prefix matching without glob', () => { - it('exact match', () => { + it('exact match', async () => { expect(matchesCommandPattern('git', 'git')).toBe(true); }); - it('prefix + space', () => { + it('prefix + space', async () => { expect(matchesCommandPattern('git', 'git status')).toBe(true); expect(matchesCommandPattern('git commit', 'git commit -m "test"')).toBe( true, ); }); - it('does not match as substring', () => { + it('does not match as substring', async () => { expect(matchesCommandPattern('git', 'gitcommit')).toBe(false); }); }); // Wildcard at tail describe('wildcard at tail', () => { - it('matches any arguments', () => { + it('matches any arguments', async () => { expect(matchesCommandPattern('git *', 'git status')).toBe(true); expect(matchesCommandPattern('git *', 'git commit -m "test"')).toBe(true); expect(matchesCommandPattern('npm run *', 'npm run build')).toBe(true); }); - it('space-star requires word boundary (ls * does not match lsof)', () => { + it('space-star requires word boundary (ls * does not match lsof)', async () => { expect(matchesCommandPattern('ls *', 'ls -la')).toBe(true); expect(matchesCommandPattern('ls *', 'lsof')).toBe(false); }); - it('no-space-star allows prefix matching (ls* matches lsof)', () => { + it('no-space-star allows prefix matching (ls* matches lsof)', async () => { expect(matchesCommandPattern('ls*', 'ls -la')).toBe(true); expect(matchesCommandPattern('ls*', 'lsof')).toBe(true); }); - it('does not match different command', () => { + it('does not match different command', async () => { expect(matchesCommandPattern('git *', 'echo hello')).toBe(false); }); }); // Wildcard at head describe('wildcard at head', () => { - it('matches any command ending with pattern', () => { + it('matches any command ending with pattern', async () => { expect(matchesCommandPattern('* --version', 'node --version')).toBe(true); expect(matchesCommandPattern('* --version', 'npm --version')).toBe(true); expect(matchesCommandPattern('* --help *', 'npm --help install')).toBe( @@ -252,21 +252,21 @@ describe('matchesCommandPattern', () => { ); }); - it('does not match non-matching suffix', () => { + it('does not match non-matching suffix', async () => { expect(matchesCommandPattern('* --version', 'node --help')).toBe(false); }); }); // Wildcard in middle describe('wildcard in middle', () => { - it('matches middle segments', () => { + it('matches middle segments', async () => { expect(matchesCommandPattern('git * main', 'git checkout main')).toBe( true, ); expect(matchesCommandPattern('git * main', 'git merge main')).toBe(true); }); - it('does not match different suffix', () => { + it('does not match different suffix', async () => { expect(matchesCommandPattern('git * main', 'git checkout dev')).toBe( false, ); @@ -275,19 +275,19 @@ describe('matchesCommandPattern', () => { // Word boundary rule: space before * matters describe('word boundary rule (space before *)', () => { - it('Bash(ls *): matches "ls -la" but NOT "lsof"', () => { + it('Bash(ls *): matches "ls -la" but NOT "lsof"', async () => { expect(matchesCommandPattern('ls *', 'ls -la')).toBe(true); expect(matchesCommandPattern('ls *', 'ls')).toBe(true); // "ls" alone expect(matchesCommandPattern('ls *', 'lsof')).toBe(false); }); - it('Bash(ls*): matches both "ls -la" and "lsof"', () => { + it('Bash(ls*): matches both "ls -la" and "lsof"', async () => { expect(matchesCommandPattern('ls*', 'ls -la')).toBe(true); expect(matchesCommandPattern('ls*', 'lsof')).toBe(true); expect(matchesCommandPattern('ls*', 'ls')).toBe(true); }); - it('Bash(npm *): matches "npm run" but NOT "npmx"', () => { + it('Bash(npm *): matches "npm run" but NOT "npmx"', async () => { expect(matchesCommandPattern('npm *', 'npm run build')).toBe(true); expect(matchesCommandPattern('npm *', 'npmx install')).toBe(false); }); @@ -306,13 +306,13 @@ describe('matchesCommandPattern', () => { // These tests verify that matchesCommandPattern works correctly on // individual simple commands (the sub-commands after splitting). describe('simple command matching (no operators)', () => { - it('matches when no operators are present', () => { + it('matches when no operators are present', async () => { expect( matchesCommandPattern('git *', 'git commit -m "hello world"'), ).toBe(true); }); - it('operators inside quotes are not boundaries for splitCompoundCommand', () => { + it('operators inside quotes are not boundaries for splitCompoundCommand', async () => { // "echo 'a && b'" → the && is inside quotes, not an operator expect(matchesCommandPattern('echo *', "echo 'a && b'")).toBe(true); }); @@ -320,24 +320,24 @@ describe('matchesCommandPattern', () => { // Special: lone * matches any command describe('lone wildcard', () => { - it('* matches any single command', () => { + it('* matches any single command', async () => { expect(matchesCommandPattern('*', 'anything here')).toBe(true); }); }); // Exact command match with specifier describe('exact command specifier', () => { - it('Bash(npm run build) matches exact command', () => { + it('Bash(npm run build) matches exact command', async () => { expect(matchesCommandPattern('npm run build', 'npm run build')).toBe( true, ); }); - it('Bash(npm run build) also matches with trailing args (prefix)', () => { + it('Bash(npm run build) also matches with trailing args (prefix)', async () => { expect( matchesCommandPattern('npm run build', 'npm run build --verbose'), ).toBe(true); }); - it('Bash(npm run build) does not match different command', () => { + it('Bash(npm run build) does not match different command', async () => { expect(matchesCommandPattern('npm run build', 'npm run test')).toBe( false, ); @@ -348,59 +348,59 @@ describe('matchesCommandPattern', () => { // ─── splitCompoundCommand ──────────────────────────────────────────────────── describe('splitCompoundCommand', () => { - it('simple command returns single-element array', () => { + it('simple command returns single-element array', async () => { expect(splitCompoundCommand('git status')).toEqual(['git status']); }); - it('splits on &&', () => { + it('splits on &&', async () => { expect(splitCompoundCommand('git status && rm -rf /')).toEqual([ 'git status', 'rm -rf /', ]); }); - it('splits on ||', () => { + it('splits on ||', async () => { expect(splitCompoundCommand('git push || echo failed')).toEqual([ 'git push', 'echo failed', ]); }); - it('splits on ;', () => { + it('splits on ;', async () => { expect(splitCompoundCommand('echo hello; echo world')).toEqual([ 'echo hello', 'echo world', ]); }); - it('splits on |', () => { + it('splits on |', async () => { expect(splitCompoundCommand('git log | grep fix')).toEqual([ 'git log', 'grep fix', ]); }); - it('handles three-part compound', () => { + it('handles three-part compound', async () => { expect(splitCompoundCommand('a && b && c')).toEqual(['a', 'b', 'c']); }); - it('handles mixed operators', () => { + it('handles mixed operators', async () => { expect(splitCompoundCommand('a && b | c; d')).toEqual(['a', 'b', 'c', 'd']); }); - it('does not split on operators inside single quotes', () => { + it('does not split on operators inside single quotes', async () => { expect(splitCompoundCommand("echo 'a && b'")).toEqual(["echo 'a && b'"]); }); - it('does not split on operators inside double quotes', () => { + it('does not split on operators inside double quotes', async () => { expect(splitCompoundCommand('echo "a && b"')).toEqual(['echo "a && b"']); }); - it('handles escaped characters', () => { + it('handles escaped characters', async () => { expect(splitCompoundCommand('echo a \\&& b')).toEqual(['echo a \\&& b']); }); - it('trims whitespace around sub-commands', () => { + it('trims whitespace around sub-commands', async () => { expect(splitCompoundCommand(' git status && rm -rf / ')).toEqual([ 'git status', 'rm -rf /', @@ -414,13 +414,13 @@ describe('resolvePathPattern', () => { const projectRoot = '/project'; const cwd = '/project/subdir'; - it('// prefix → absolute from filesystem root', () => { + it('// prefix → absolute from filesystem root', async () => { expect( resolvePathPattern('//Users/alice/secrets/**', projectRoot, cwd), ).toBe('/Users/alice/secrets/**'); }); - it('~/ prefix → relative to home directory', () => { + it('~/ prefix → relative to home directory', async () => { const result = resolvePathPattern('~/Documents/*.pdf', projectRoot, cwd); expect(result).toContain('Documents/*.pdf'); // On POSIX systems the home dir starts with '/'; on Windows it may look like @@ -430,25 +430,25 @@ describe('resolvePathPattern', () => { expect(result.startsWith(normalizedHome)).toBe(true); }); - it('/ prefix → relative to project root (NOT absolute)', () => { + it('/ prefix → relative to project root (NOT absolute)', async () => { expect(resolvePathPattern('/src/**/*.ts', projectRoot, cwd)).toBe( '/project/src/**/*.ts', ); }); - it('./ prefix → relative to cwd', () => { + it('./ prefix → relative to cwd', async () => { expect(resolvePathPattern('./secrets/**', projectRoot, cwd)).toBe( '/project/subdir/secrets/**', ); }); - it('no prefix → relative to cwd', () => { + it('no prefix → relative to cwd', async () => { expect(resolvePathPattern('*.env', projectRoot, cwd)).toBe( '/project/subdir/*.env', ); }); - it('/Users/alice/file is relative to project root, NOT absolute', () => { + it('/Users/alice/file is relative to project root, NOT absolute', async () => { // This is a gotcha from the Claude Code docs expect(resolvePathPattern('/Users/alice/file', projectRoot, cwd)).toBe( '/project/Users/alice/file', @@ -462,7 +462,7 @@ describe('matchesPathPattern', () => { const projectRoot = '/project'; const cwd = '/project'; - it('matches dotfiles (e.g. .env)', () => { + it('matches dotfiles (e.g. .env)', async () => { expect(matchesPathPattern('.env', '/project/.env', projectRoot, cwd)).toBe( true, ); @@ -471,7 +471,7 @@ describe('matchesPathPattern', () => { ); }); - it('** matches recursively across directories', () => { + it('** matches recursively across directories', async () => { expect( matchesPathPattern( './secrets/**', @@ -482,7 +482,7 @@ describe('matchesPathPattern', () => { ).toBe(true); }); - it('* matches single directory only', () => { + it('* matches single directory only', async () => { expect( matchesPathPattern( '/src/*.ts', @@ -501,7 +501,7 @@ describe('matchesPathPattern', () => { ).toBe(false); }); - it('/docs/** matches under project root docs', () => { + it('/docs/** matches under project root docs', async () => { expect( matchesPathPattern( '/docs/**', @@ -520,7 +520,7 @@ describe('matchesPathPattern', () => { ).toBe(false); }); - it('//tmp/scratch.txt matches absolute path', () => { + it('//tmp/scratch.txt matches absolute path', async () => { expect( matchesPathPattern( '//tmp/scratch.txt', @@ -531,7 +531,7 @@ describe('matchesPathPattern', () => { ).toBe(true); }); - it('does not match unrelated paths', () => { + it('does not match unrelated paths', async () => { expect( matchesPathPattern( './secrets/**', @@ -546,13 +546,13 @@ describe('matchesPathPattern', () => { // ─── matchesDomainPattern ──────────────────────────────────────────────────── describe('matchesDomainPattern', () => { - it('matches exact domain', () => { + it('matches exact domain', async () => { expect(matchesDomainPattern('domain:example.com', 'example.com')).toBe( true, ); }); - it('matches subdomain', () => { + it('matches subdomain', async () => { expect(matchesDomainPattern('domain:example.com', 'sub.example.com')).toBe( true, ); @@ -561,19 +561,19 @@ describe('matchesDomainPattern', () => { ).toBe(true); }); - it('does not match different domain', () => { + it('does not match different domain', async () => { expect(matchesDomainPattern('domain:example.com', 'notexample.com')).toBe( false, ); }); - it('is case-insensitive', () => { + it('is case-insensitive', async () => { expect(matchesDomainPattern('domain:Example.COM', 'example.com')).toBe( true, ); }); - it('handles missing prefix', () => { + it('handles missing prefix', async () => { expect(matchesDomainPattern('example.com', 'example.com')).toBe(true); }); }); @@ -582,26 +582,26 @@ describe('matchesDomainPattern', () => { describe('matchesRule', () => { // Basic tool name matching - it('simple tool-name rule matches any invocation', () => { + it('simple tool-name rule matches any invocation', async () => { const rule = parseRule('ShellTool'); expect(matchesRule(rule, 'run_shell_command')).toBe(true); expect(matchesRule(rule, 'run_shell_command', 'git status')).toBe(true); }); - it('does not match a different tool', () => { + it('does not match a different tool', async () => { const rule = parseRule('ShellTool'); expect(matchesRule(rule, 'read_file')).toBe(false); }); // Shell command specifier - it('specifier rule requires a command for shell tools', () => { + it('specifier rule requires a command for shell tools', async () => { const rule = parseRule('Bash(git *)'); expect(matchesRule(rule, 'run_shell_command')).toBe(false); // no command expect(matchesRule(rule, 'run_shell_command', 'git status')).toBe(true); expect(matchesRule(rule, 'run_shell_command', 'echo hello')).toBe(false); }); - it('matchesRule checks individual simple commands (compound splitting is at PM level)', () => { + it('matchesRule checks individual simple commands (compound splitting is at PM level)', async () => { const rule = parseRule('Bash(git *)'); // matchesRule receives a simple command (already split by PM) expect(matchesRule(rule, 'run_shell_command', 'git status')).toBe(true); @@ -609,7 +609,7 @@ describe('matchesRule', () => { }); // Meta-category matching: Read - it('Read rule matches grep_search, glob, list_directory', () => { + it('Read rule matches grep_search, glob, list_directory', async () => { const rule = parseRule('Read'); expect(matchesRule(rule, 'read_file')).toBe(true); expect(matchesRule(rule, 'grep_search')).toBe(true); @@ -619,7 +619,7 @@ describe('matchesRule', () => { }); // Meta-category matching: Edit - it('Edit rule matches edit and write_file', () => { + it('Edit rule matches edit and write_file', async () => { const rule = parseRule('Edit'); expect(matchesRule(rule, 'edit')).toBe(true); expect(matchesRule(rule, 'write_file')).toBe(true); @@ -627,7 +627,7 @@ describe('matchesRule', () => { }); // File path matching - it('Read with path specifier requires filePath', () => { + it('Read with path specifier requires filePath', async () => { const rule = parseRule('Read(.env)'); const pathCtx = { projectRoot: '/project', cwd: '/project' }; // No filePath → no match @@ -655,7 +655,7 @@ describe('matchesRule', () => { ).toBe(false); }); - it('Edit path specifier matches write_file too', () => { + it('Edit path specifier matches write_file too', async () => { const rule = parseRule('Edit(/src/**/*.ts)'); const pathCtx = { projectRoot: '/project', cwd: '/project' }; expect( @@ -681,7 +681,7 @@ describe('matchesRule', () => { }); // WebFetch domain matching - it('WebFetch domain specifier', () => { + it('WebFetch domain specifier', async () => { const rule = parseRule('WebFetch(domain:example.com)'); expect( matchesRule(rule, 'web_fetch', undefined, undefined, 'example.com'), @@ -697,7 +697,7 @@ describe('matchesRule', () => { }); // Agent literal matching - it('Agent literal specifier', () => { + it('Agent literal specifier', async () => { const rule = parseRule('Agent(Explore)'); // Agent is an alias for 'task'; specifier matches via the specifier field expect( @@ -726,26 +726,26 @@ describe('matchesRule', () => { }); // MCP tool matching - it('MCP tool exact match', () => { + it('MCP tool exact match', async () => { const rule = parseRule('mcp__puppeteer__puppeteer_navigate'); expect(matchesRule(rule, 'mcp__puppeteer__puppeteer_navigate')).toBe(true); expect(matchesRule(rule, 'mcp__puppeteer__puppeteer_click')).toBe(false); }); - it('MCP server-level match (2-part pattern)', () => { + it('MCP server-level match (2-part pattern)', async () => { const rule = parseRule('mcp__puppeteer'); expect(matchesRule(rule, 'mcp__puppeteer__puppeteer_navigate')).toBe(true); expect(matchesRule(rule, 'mcp__puppeteer__puppeteer_click')).toBe(true); expect(matchesRule(rule, 'mcp__other__tool')).toBe(false); }); - it('MCP wildcard match', () => { + it('MCP wildcard match', async () => { const rule = parseRule('mcp__puppeteer__*'); expect(matchesRule(rule, 'mcp__puppeteer__puppeteer_navigate')).toBe(true); expect(matchesRule(rule, 'mcp__other__tool')).toBe(false); }); - it('MCP intra-segment wildcard match (e.g. mcp__chrome__use_*)', () => { + it('MCP intra-segment wildcard match (e.g. mcp__chrome__use_*)', async () => { const rule = parseRule('mcp__chrome__use_*'); expect(matchesRule(rule, 'mcp__chrome__use_browser')).toBe(true); expect(matchesRule(rule, 'mcp__chrome__use_context')).toBe(true); @@ -791,25 +791,25 @@ describe('PermissionManager', () => { pm.initialize(); }); - it('returns deny for a denied tool', () => { - expect(pm.evaluate({ toolName: 'run_shell_command' })).toBe('deny'); + it('returns deny for a denied tool', async () => { + expect(await pm.evaluate({ toolName: 'run_shell_command' })).toBe('deny'); }); - it('returns ask for an ask-rule tool', () => { - expect(pm.evaluate({ toolName: 'write_file' })).toBe('ask'); + it('returns ask for an ask-rule tool', async () => { + expect(await pm.evaluate({ toolName: 'write_file' })).toBe('ask'); }); - it('returns allow for an allow-rule tool', () => { - expect(pm.evaluate({ toolName: 'read_file' })).toBe('allow'); + it('returns allow for an allow-rule tool', async () => { + expect(await pm.evaluate({ toolName: 'read_file' })).toBe('allow'); }); - it('returns default for unmatched tool', () => { + it('returns default for unmatched tool', async () => { // Note: 'glob' is covered by ReadFileTool via Read meta-category, // so use a tool not in any rule or meta-category - expect(pm.evaluate({ toolName: 'agent' })).toBe('default'); + expect(await pm.evaluate({ toolName: 'agent' })).toBe('default'); }); - it('deny takes precedence over ask and allow', () => { + it('deny takes precedence over ask and allow', async () => { const pm2 = new PermissionManager( makeConfig({ permissionsAllow: ['run_shell_command'], @@ -818,10 +818,12 @@ describe('PermissionManager', () => { }), ); pm2.initialize(); - expect(pm2.evaluate({ toolName: 'run_shell_command' })).toBe('deny'); + expect(await pm2.evaluate({ toolName: 'run_shell_command' })).toBe( + 'deny', + ); }); - it('ask takes precedence over allow', () => { + it('ask takes precedence over allow', async () => { const pm2 = new PermissionManager( makeConfig({ permissionsAllow: ['write_file'], @@ -829,7 +831,7 @@ describe('PermissionManager', () => { }), ); pm2.initialize(); - expect(pm2.evaluate({ toolName: 'write_file' })).toBe('ask'); + expect(await pm2.evaluate({ toolName: 'write_file' })).toBe('ask'); }); }); @@ -844,33 +846,51 @@ describe('PermissionManager', () => { pm.initialize(); }); - it('allows a matching allowed command', () => { + it('allows a matching allowed command', async () => { expect( - pm.evaluate({ toolName: 'run_shell_command', command: 'git status' }), + await pm.evaluate({ + toolName: 'run_shell_command', + command: 'git status', + }), ).toBe('allow'); }); - it('denies a matching denied command', () => { + it('denies a matching denied command', async () => { expect( - pm.evaluate({ toolName: 'run_shell_command', command: 'rm -rf /' }), + await pm.evaluate({ + toolName: 'run_shell_command', + command: 'rm -rf /', + }), ).toBe('deny'); }); - it('returns default for an unmatched command', () => { + it('resolves default to allow for readonly commands, ask for others', async () => { + // 'echo' is a readonly command, so it resolves to 'allow' expect( - pm.evaluate({ toolName: 'run_shell_command', command: 'echo hello' }), - ).toBe('default'); + await pm.evaluate({ + toolName: 'run_shell_command', + command: 'echo hello', + }), + ).toBe('allow'); + // 'npm install' is not readonly, so it resolves to 'ask' + expect( + await pm.evaluate({ + toolName: 'run_shell_command', + command: 'npm install', + }), + ).toBe('ask'); }); - it('isCommandAllowed delegates to evaluate', () => { - expect(pm.isCommandAllowed('git commit')).toBe('allow'); - expect(pm.isCommandAllowed('rm -rf /')).toBe('deny'); - expect(pm.isCommandAllowed('ls')).toBe('default'); + it('isCommandAllowed delegates to evaluate', async () => { + expect(await pm.isCommandAllowed('git commit')).toBe('allow'); + expect(await pm.isCommandAllowed('rm -rf /')).toBe('deny'); + // 'ls' is readonly, resolves to 'allow' when no rule matches + expect(await pm.isCommandAllowed('ls')).toBe('allow'); }); }); describe('compound command evaluation', () => { - it('all sub-commands allowed → allow', () => { + it('all sub-commands allowed → allow', async () => { pm = new PermissionManager( makeConfig({ permissionsAllow: ['Bash(safe-cmd *)', 'Bash(one-cmd *)'], @@ -878,29 +898,30 @@ describe('PermissionManager', () => { ); pm.initialize(); expect( - pm.evaluate({ + await pm.evaluate({ toolName: 'run_shell_command', command: 'safe-cmd arg1 && one-cmd arg2', }), ).toBe('allow'); }); - it('one sub-command unmatched → default (most restrictive)', () => { + it('one sub-command unmatched (non-readonly) → ask (resolved from default)', async () => { pm = new PermissionManager( makeConfig({ permissionsAllow: ['Bash(safe-cmd *)'], }), ); pm.initialize(); + // 'two-cmd' is unknown/non-readonly, so its default permission is 'ask' expect( - pm.evaluate({ + await pm.evaluate({ toolName: 'run_shell_command', command: 'safe-cmd && two-cmd', }), - ).toBe('default'); + ).toBe('ask'); }); - it('one sub-command denied → deny', () => { + it('one sub-command denied → deny', async () => { pm = new PermissionManager( makeConfig({ permissionsAllow: ['Bash(safe-cmd *)'], @@ -909,14 +930,14 @@ describe('PermissionManager', () => { ); pm.initialize(); expect( - pm.evaluate({ + await pm.evaluate({ toolName: 'run_shell_command', command: 'safe-cmd && evil-cmd rm-all', }), ).toBe('deny'); }); - it('one sub-command ask + one allow → ask', () => { + it('one sub-command ask + one allow → ask', async () => { pm = new PermissionManager( makeConfig({ permissionsAllow: ['Bash(git *)'], @@ -925,14 +946,14 @@ describe('PermissionManager', () => { ); pm.initialize(); expect( - pm.evaluate({ + await pm.evaluate({ toolName: 'run_shell_command', command: 'git status && npm publish', }), ).toBe('ask'); }); - it('pipe compound: all matched → allow', () => { + it('pipe compound: all matched → allow', async () => { pm = new PermissionManager( makeConfig({ permissionsAllow: ['Bash(git *)', 'Bash(grep *)'], @@ -940,29 +961,30 @@ describe('PermissionManager', () => { ); pm.initialize(); expect( - pm.evaluate({ + await pm.evaluate({ toolName: 'run_shell_command', command: 'git log | grep fix', }), ).toBe('allow'); }); - it('pipe compound: second unmatched → default', () => { + it('pipe compound: second unmatched but readonly → allow (resolved from default)', async () => { pm = new PermissionManager( makeConfig({ permissionsAllow: ['Bash(git *)'], }), ); pm.initialize(); + // 'grep' is a readonly command, so its default permission is 'allow' expect( - pm.evaluate({ + await pm.evaluate({ toolName: 'run_shell_command', command: 'git log | grep fix', }), - ).toBe('default'); + ).toBe('allow'); }); - it('semicolon compound: deny in second → deny', () => { + it('semicolon compound: deny in second → deny', async () => { pm = new PermissionManager( makeConfig({ permissionsAllow: ['Bash(echo *)'], @@ -971,14 +993,14 @@ describe('PermissionManager', () => { ); pm.initialize(); expect( - pm.evaluate({ + await pm.evaluate({ toolName: 'run_shell_command', command: 'echo hello; rm -rf /', }), ).toBe('deny'); }); - it('|| compound: all allowed → allow', () => { + it('|| compound: all allowed → allow', async () => { pm = new PermissionManager( makeConfig({ permissionsAllow: ['Bash(git *)', 'Bash(echo *)'], @@ -986,14 +1008,14 @@ describe('PermissionManager', () => { ); pm.initialize(); expect( - pm.evaluate({ + await pm.evaluate({ toolName: 'run_shell_command', command: 'git push || echo failed', }), ).toBe('allow'); }); - it('operators inside quotes: treated as single command', () => { + it('operators inside quotes: treated as single command', async () => { pm = new PermissionManager( makeConfig({ permissionsAllow: ['Bash(echo *)'], @@ -1001,14 +1023,14 @@ describe('PermissionManager', () => { ); pm.initialize(); expect( - pm.evaluate({ + await pm.evaluate({ toolName: 'run_shell_command', command: "echo 'a && b'", }), ).toBe('allow'); }); - it('three-part compound: all must pass', () => { + it('three-part compound: all must pass', async () => { pm = new PermissionManager( makeConfig({ permissionsAllow: ['Bash(git *)', 'Bash(npm *)', 'Bash(echo *)'], @@ -1016,29 +1038,30 @@ describe('PermissionManager', () => { ); pm.initialize(); expect( - pm.evaluate({ + await pm.evaluate({ toolName: 'run_shell_command', command: 'git add . && npm test && echo done', }), ).toBe('allow'); }); - it('three-part compound: one unmatched → default', () => { + it('three-part compound: one unmatched (non-readonly) → ask (resolved from default)', async () => { pm = new PermissionManager( makeConfig({ permissionsAllow: ['Bash(git *)', 'Bash(echo *)'], }), ); pm.initialize(); + // 'npm test' is not readonly, so its default permission is 'ask' expect( - pm.evaluate({ + await pm.evaluate({ toolName: 'run_shell_command', command: 'git add . && npm test && echo done', }), - ).toBe('default'); + ).toBe('ask'); }); - it('isCommandAllowed also handles compound commands', () => { + it('isCommandAllowed also handles compound commands', async () => { pm = new PermissionManager( makeConfig({ permissionsAllow: ['Bash(safe-cmd *)', 'Bash(one-cmd *)'], @@ -1046,9 +1069,16 @@ describe('PermissionManager', () => { }), ); pm.initialize(); - expect(pm.isCommandAllowed('safe-cmd a && one-cmd b')).toBe('allow'); - expect(pm.isCommandAllowed('safe-cmd a && unknown-cmd')).toBe('default'); - expect(pm.isCommandAllowed('safe-cmd a && evil-cmd b')).toBe('deny'); + expect(await pm.isCommandAllowed('safe-cmd a && one-cmd b')).toBe( + 'allow', + ); + // 'unknown-cmd' is not readonly, resolves to 'ask' + expect(await pm.isCommandAllowed('safe-cmd a && unknown-cmd')).toBe( + 'ask', + ); + expect(await pm.isCommandAllowed('safe-cmd a && evil-cmd b')).toBe( + 'deny', + ); }); }); @@ -1065,39 +1095,42 @@ describe('PermissionManager', () => { pm.initialize(); }); - it('denies reading a denied file', () => { + it('denies reading a denied file', async () => { expect( - pm.evaluate({ toolName: 'read_file', filePath: '/project/.env' }), + await pm.evaluate({ toolName: 'read_file', filePath: '/project/.env' }), ).toBe('deny'); }); - it('denies editing in a denied directory', () => { + it('denies editing in a denied directory', async () => { expect( - pm.evaluate({ + await pm.evaluate({ toolName: 'edit', filePath: '/project/src/generated/code.ts', }), ).toBe('deny'); }); - it('allows reading in an allowed directory', () => { + it('allows reading in an allowed directory', async () => { expect( - pm.evaluate({ + await pm.evaluate({ toolName: 'read_file', filePath: '/project/docs/readme.md', }), ).toBe('allow'); }); - it('Read deny applies to grep_search too (meta-category)', () => { + it('Read deny applies to grep_search too (meta-category)', async () => { expect( - pm.evaluate({ toolName: 'grep_search', filePath: '/project/.env' }), + await pm.evaluate({ + toolName: 'grep_search', + filePath: '/project/.env', + }), ).toBe('deny'); }); - it('returns default for unmatched path', () => { + it('returns default for unmatched path', async () => { expect( - pm.evaluate({ + await pm.evaluate({ toolName: 'read_file', filePath: '/project/src/index.ts', }), @@ -1116,89 +1149,89 @@ describe('PermissionManager', () => { pm.initialize(); }); - it('allows fetch to allowed domain', () => { - expect(pm.evaluate({ toolName: 'web_fetch', domain: 'github.com' })).toBe( - 'allow', - ); + it('allows fetch to allowed domain', async () => { + expect( + await pm.evaluate({ toolName: 'web_fetch', domain: 'github.com' }), + ).toBe('allow'); }); - it('allows fetch to subdomain of allowed domain', () => { + it('allows fetch to subdomain of allowed domain', async () => { expect( - pm.evaluate({ toolName: 'web_fetch', domain: 'api.github.com' }), + await pm.evaluate({ toolName: 'web_fetch', domain: 'api.github.com' }), ).toBe('allow'); }); - it('denies fetch to denied domain', () => { - expect(pm.evaluate({ toolName: 'web_fetch', domain: 'evil.com' })).toBe( - 'deny', - ); + it('denies fetch to denied domain', async () => { + expect( + await pm.evaluate({ toolName: 'web_fetch', domain: 'evil.com' }), + ).toBe('deny'); }); - it('returns default for unmatched domain', () => { + it('returns default for unmatched domain', async () => { expect( - pm.evaluate({ toolName: 'web_fetch', domain: 'example.com' }), + await pm.evaluate({ toolName: 'web_fetch', domain: 'example.com' }), ).toBe('default'); }); }); describe('isToolEnabled', () => { - it('returns false for deny-ruled tools', () => { + it('returns false for deny-ruled tools', async () => { pm = new PermissionManager( makeConfig({ permissionsDeny: ['ShellTool'] }), ); pm.initialize(); - expect(pm.isToolEnabled('run_shell_command')).toBe(false); + expect(await pm.isToolEnabled('run_shell_command')).toBe(false); }); - it('returns true for tools with only specifier deny rules', () => { + it('returns true for tools with only specifier deny rules', async () => { pm = new PermissionManager( makeConfig({ permissionsDeny: ['Bash(rm *)'] }), ); pm.initialize(); - expect(pm.isToolEnabled('run_shell_command')).toBe(true); + expect(await pm.isToolEnabled('run_shell_command')).toBe(true); }); - it('excludeTools passed via permissionsDeny disables the tool', () => { + it('excludeTools passed via permissionsDeny disables the tool', async () => { pm = new PermissionManager( makeConfig({ permissionsDeny: ['run_shell_command'] }), ); pm.initialize(); - expect(pm.isToolEnabled('run_shell_command')).toBe(false); + expect(await pm.isToolEnabled('run_shell_command')).toBe(false); }); - it('coreTools allowlist: listed tool is enabled', () => { + it('coreTools allowlist: listed tool is enabled', async () => { pm = new PermissionManager( makeConfig({ coreTools: ['read_file', 'Bash'] }), ); pm.initialize(); - expect(pm.isToolEnabled('read_file')).toBe(true); - expect(pm.isToolEnabled('run_shell_command')).toBe(true); // Bash resolves to run_shell_command + expect(await pm.isToolEnabled('read_file')).toBe(true); + expect(await pm.isToolEnabled('run_shell_command')).toBe(true); // Bash resolves to run_shell_command }); - it('coreTools allowlist: unlisted tool is disabled', () => { + it('coreTools allowlist: unlisted tool is disabled', async () => { pm = new PermissionManager(makeConfig({ coreTools: ['read_file'] })); pm.initialize(); - expect(pm.isToolEnabled('read_file')).toBe(true); - expect(pm.isToolEnabled('run_shell_command')).toBe(false); - expect(pm.isToolEnabled('edit')).toBe(false); + expect(await pm.isToolEnabled('read_file')).toBe(true); + expect(await pm.isToolEnabled('run_shell_command')).toBe(false); + expect(await pm.isToolEnabled('edit')).toBe(false); }); - it('coreTools with specifier: tool-level check strips specifier', () => { + it('coreTools with specifier: tool-level check strips specifier', async () => { // "Bash(ls -l)" should register run_shell_command (specifier only affects runtime) pm = new PermissionManager(makeConfig({ coreTools: ['Bash(ls -l)'] })); pm.initialize(); - expect(pm.isToolEnabled('run_shell_command')).toBe(true); - expect(pm.isToolEnabled('read_file')).toBe(false); + expect(await pm.isToolEnabled('run_shell_command')).toBe(true); + expect(await pm.isToolEnabled('read_file')).toBe(false); }); - it('empty coreTools: all tools enabled (no whitelist restriction)', () => { + it('empty coreTools: all tools enabled (no whitelist restriction)', async () => { pm = new PermissionManager(makeConfig({ coreTools: [] })); pm.initialize(); - expect(pm.isToolEnabled('read_file')).toBe(true); - expect(pm.isToolEnabled('run_shell_command')).toBe(true); + expect(await pm.isToolEnabled('read_file')).toBe(true); + expect(await pm.isToolEnabled('run_shell_command')).toBe(true); }); - it('coreTools allowlist + deny rule: deny takes precedence for listed tools', () => { + it('coreTools allowlist + deny rule: deny takes precedence for listed tools', async () => { pm = new PermissionManager( makeConfig({ coreTools: ['read_file', 'Bash'], @@ -1206,19 +1239,19 @@ describe('PermissionManager', () => { }), ); pm.initialize(); - expect(pm.isToolEnabled('read_file')).toBe(true); - expect(pm.isToolEnabled('run_shell_command')).toBe(false); // in list but denied + expect(await pm.isToolEnabled('read_file')).toBe(true); + expect(await pm.isToolEnabled('run_shell_command')).toBe(false); // in list but denied }); - it('permissionsAllow alone does NOT restrict unlisted tools (not a whitelist)', () => { + it('permissionsAllow alone does NOT restrict unlisted tools (not a whitelist)', async () => { // This verifies the previous incorrect behavior is gone: permissionsAllow // only means "auto-approve", it does NOT block unlisted tools. pm = new PermissionManager( makeConfig({ permissionsAllow: ['read_file'] }), ); pm.initialize(); - expect(pm.isToolEnabled('read_file')).toBe(true); - expect(pm.isToolEnabled('run_shell_command')).toBe(true); // not denied, just unreviewed + expect(await pm.isToolEnabled('read_file')).toBe(true); + expect(await pm.isToolEnabled('run_shell_command')).toBe(true); // not denied, just unreviewed }); }); @@ -1228,38 +1261,48 @@ describe('PermissionManager', () => { pm.initialize(); }); - it('addSessionAllowRule enables auto-approval for that pattern', () => { + it('addSessionAllowRule enables auto-approval for that pattern', async () => { + // Use 'git commit' which is not readonly, so it resolves to 'ask' by default expect( - pm.evaluate({ toolName: 'run_shell_command', command: 'git status' }), - ).toBe('default'); + await pm.evaluate({ + toolName: 'run_shell_command', + command: 'git commit', + }), + ).toBe('ask'); pm.addSessionAllowRule('Bash(git *)'); expect( - pm.evaluate({ toolName: 'run_shell_command', command: 'git status' }), + await pm.evaluate({ + toolName: 'run_shell_command', + command: 'git commit', + }), ).toBe('allow'); }); - it('session deny rules override allow rules', () => { + it('session deny rules override allow rules', async () => { pm.addSessionAllowRule('run_shell_command'); pm.addSessionDenyRule('run_shell_command'); - expect(pm.evaluate({ toolName: 'run_shell_command' })).toBe('deny'); + expect(await pm.evaluate({ toolName: 'run_shell_command' })).toBe('deny'); }); }); describe('allowedTools via permissionsAllow', () => { - it('allow rule auto-approves matching tools/commands', () => { + it('allow rule auto-approves matching tools/commands', async () => { pm = new PermissionManager( makeConfig({ permissionsAllow: ['ReadFileTool', 'Bash(git *)'] }), ); pm.initialize(); - expect(pm.evaluate({ toolName: 'read_file' })).toBe('allow'); + expect(await pm.evaluate({ toolName: 'read_file' })).toBe('allow'); expect( - pm.evaluate({ toolName: 'run_shell_command', command: 'git status' }), + await pm.evaluate({ + toolName: 'run_shell_command', + command: 'git status', + }), ).toBe('allow'); }); }); describe('listRules', () => { - it('returns all rules with type and scope', () => { + it('returns all rules with type and scope', async () => { pm = new PermissionManager( makeConfig({ permissionsAllow: ['ReadFileTool'], @@ -1277,37 +1320,67 @@ describe('PermissionManager', () => { expect(sessionAllow?.rule.toolName).toBe('run_shell_command'); }); }); + + describe('hasMatchingAskRule', () => { + it('returns false when shell ask comes only from default permission fallback', async () => { + pm = new PermissionManager( + makeConfig({ permissionsAllow: ['Bash(git add *)'] }), + ); + pm.initialize(); + + expect( + pm.hasMatchingAskRule({ + toolName: 'run_shell_command', + command: 'git add file && git commit -m "msg"', + }), + ).toBe(false); + }); + + it('returns true when an explicit ask rule matches a shell sub-command', async () => { + pm = new PermissionManager( + makeConfig({ permissionsAsk: ['Bash(git commit *)'] }), + ); + pm.initialize(); + + expect( + pm.hasMatchingAskRule({ + toolName: 'run_shell_command', + command: 'git add file && git commit -m "msg"', + }), + ).toBe(true); + }); + }); }); // ─── getRuleDisplayName ────────────────────────────────────────────────────── describe('getRuleDisplayName', () => { - it('maps read tools to "Read" meta-category', () => { + it('maps read tools to "Read" meta-category', async () => { expect(getRuleDisplayName('read_file')).toBe('Read'); expect(getRuleDisplayName('grep_search')).toBe('Read'); expect(getRuleDisplayName('glob')).toBe('Read'); expect(getRuleDisplayName('list_directory')).toBe('Read'); }); - it('maps edit tools to "Edit" meta-category', () => { + it('maps edit tools to "Edit" meta-category', async () => { expect(getRuleDisplayName('edit')).toBe('Edit'); expect(getRuleDisplayName('write_file')).toBe('Edit'); }); - it('maps shell to "Bash"', () => { + it('maps shell to "Bash"', async () => { expect(getRuleDisplayName('run_shell_command')).toBe('Bash'); }); - it('maps web_fetch to "WebFetch"', () => { + it('maps web_fetch to "WebFetch"', async () => { expect(getRuleDisplayName('web_fetch')).toBe('WebFetch'); }); - it('maps agent to "Agent" and skill to "Skill"', () => { + it('maps agent to "Agent" and skill to "Skill"', async () => { expect(getRuleDisplayName('agent')).toBe('Agent'); expect(getRuleDisplayName('skill')).toBe('Skill'); }); - it('returns the canonical name for unknown tools (e.g. MCP)', () => { + it('returns the canonical name for unknown tools (e.g. MCP)', async () => { expect(getRuleDisplayName('mcp__server__tool')).toBe('mcp__server__tool'); }); }); @@ -1316,7 +1389,7 @@ describe('getRuleDisplayName', () => { describe('buildPermissionRules', () => { describe('path-based tools (Read/Edit)', () => { - it('generates Read rule scoped to parent directory for read_file', () => { + it('generates Read rule scoped to parent directory for read_file', async () => { const rules = buildPermissionRules({ toolName: 'read_file', filePath: '/Users/alice/.secrets', @@ -1325,7 +1398,7 @@ describe('buildPermissionRules', () => { expect(rules).toEqual(['Read(//Users/alice/**)']); }); - it('generates Read rule with directory as-is for grep_search', () => { + it('generates Read rule with directory as-is for grep_search', async () => { const rules = buildPermissionRules({ toolName: 'grep_search', filePath: '/external/dir', @@ -1334,7 +1407,7 @@ describe('buildPermissionRules', () => { expect(rules).toEqual(['Read(//external/dir/**)']); }); - it('generates Read rule with directory as-is for glob', () => { + it('generates Read rule with directory as-is for glob', async () => { const rules = buildPermissionRules({ toolName: 'glob', filePath: '/tmp/data', @@ -1342,7 +1415,7 @@ describe('buildPermissionRules', () => { expect(rules).toEqual(['Read(//tmp/data/**)']); }); - it('generates Read rule with directory as-is for list_directory', () => { + it('generates Read rule with directory as-is for list_directory', async () => { const rules = buildPermissionRules({ toolName: 'list_directory', filePath: '/home/user/docs', @@ -1350,7 +1423,7 @@ describe('buildPermissionRules', () => { expect(rules).toEqual(['Read(//home/user/docs/**)']); }); - it('generates Edit rule scoped to parent directory for edit', () => { + it('generates Edit rule scoped to parent directory for edit', async () => { const rules = buildPermissionRules({ toolName: 'edit', filePath: '/external/file.ts', @@ -1359,7 +1432,7 @@ describe('buildPermissionRules', () => { expect(rules).toEqual(['Edit(//external/**)']); }); - it('generates Edit rule scoped to parent directory for write_file', () => { + it('generates Edit rule scoped to parent directory for write_file', async () => { const rules = buildPermissionRules({ toolName: 'write_file', filePath: '/tmp/output.txt', @@ -1367,14 +1440,14 @@ describe('buildPermissionRules', () => { expect(rules).toEqual(['Edit(//tmp/**)']); }); - it('falls back to bare display name when no filePath', () => { + it('falls back to bare display name when no filePath', async () => { const rules = buildPermissionRules({ toolName: 'read_file' }); expect(rules).toEqual(['Read']); }); }); describe('generated rules round-trip through parseRule and matchesRule', () => { - it('Read rule for external file covers the containing directory', () => { + it('Read rule for external file covers the containing directory', async () => { const rules = buildPermissionRules({ toolName: 'read_file', filePath: '/Users/alice/.secrets', @@ -1424,7 +1497,7 @@ describe('buildPermissionRules', () => { ).toBe(false); }); - it('Read rule also matches other read-family tools on the same path', () => { + it('Read rule also matches other read-family tools on the same path', async () => { const rules = buildPermissionRules({ toolName: 'grep_search', filePath: '/external/dir', @@ -1458,7 +1531,7 @@ describe('buildPermissionRules', () => { }); describe('domain-based tools', () => { - it('generates WebFetch rule with domain specifier', () => { + it('generates WebFetch rule with domain specifier', async () => { const rules = buildPermissionRules({ toolName: 'web_fetch', domain: 'example.com', @@ -1466,14 +1539,14 @@ describe('buildPermissionRules', () => { expect(rules).toEqual(['WebFetch(example.com)']); }); - it('falls back to bare display name when no domain', () => { + it('falls back to bare display name when no domain', async () => { const rules = buildPermissionRules({ toolName: 'web_fetch' }); expect(rules).toEqual(['WebFetch']); }); }); describe('command-based tools', () => { - it('generates Bash rule with command specifier', () => { + it('generates Bash rule with command specifier', async () => { const rules = buildPermissionRules({ toolName: 'run_shell_command', command: 'git status', @@ -1481,14 +1554,14 @@ describe('buildPermissionRules', () => { expect(rules).toEqual(['Bash(git status)']); }); - it('falls back to bare display name when no command', () => { + it('falls back to bare display name when no command', async () => { const rules = buildPermissionRules({ toolName: 'run_shell_command' }); expect(rules).toEqual(['Bash']); }); }); describe('literal-specifier tools', () => { - it('generates Skill rule with specifier', () => { + it('generates Skill rule with specifier', async () => { const rules = buildPermissionRules({ toolName: 'skill', specifier: 'Explore', @@ -1496,7 +1569,7 @@ describe('buildPermissionRules', () => { expect(rules).toEqual(['Skill(Explore)']); }); - it('generates Agent rule with specifier', () => { + it('generates Agent rule with specifier', async () => { const rules = buildPermissionRules({ toolName: 'agent', specifier: 'research', @@ -1504,14 +1577,14 @@ describe('buildPermissionRules', () => { expect(rules).toEqual(['Agent(research)']); }); - it('falls back to bare display name when no specifier', () => { + it('falls back to bare display name when no specifier', async () => { const rules = buildPermissionRules({ toolName: 'skill' }); expect(rules).toEqual(['Skill']); }); }); describe('unknown / MCP tools', () => { - it('uses the canonical name as display for MCP tools', () => { + it('uses the canonical name as display for MCP tools', async () => { const rules = buildPermissionRules({ toolName: 'mcp__puppeteer__navigate', }); diff --git a/packages/core/src/permissions/permission-manager.ts b/packages/core/src/permissions/permission-manager.ts index 06f0548b03..109d4339fe 100644 --- a/packages/core/src/permissions/permission-manager.ts +++ b/packages/core/src/permissions/permission-manager.ts @@ -14,6 +14,8 @@ import { import type { PathMatchContext } from './rule-parser.js'; import { extractShellOperations } from './shell-semantics.js'; import type { ShellOperation } from './shell-semantics.js'; +import { isShellCommandReadOnlyAST } from '../utils/shellAstParser.js'; +import { detectCommandSubstitution } from '../utils/shell-utils.js'; import type { PermissionCheckContext, PermissionDecision, @@ -153,12 +155,12 @@ export class PermissionManager { * @param ctx - The context containing the tool name and optional command. * @returns A PermissionDecision indicating how to handle this tool call. */ - evaluate(ctx: PermissionCheckContext): PermissionDecision { - const { command } = ctx; + async evaluate(ctx: PermissionCheckContext): Promise { + const { command, toolName } = ctx; // For shell commands, split compound commands and evaluate each // sub-command independently, then return the most restrictive result. - // Priority order (most to least restrictive): deny > ask > default > allow + // Priority order (most to least restrictive): deny > ask > allow if (command !== undefined) { const subCommands = splitCompoundCommand(command); if (subCommands.length > 1) { @@ -166,7 +168,20 @@ export class PermissionManager { } } - return this.evaluateSingle(ctx); + const decision = this.evaluateSingle(ctx); + + // For shell commands, resolve 'default' to actual permission using AST analysis + // This ensures 'default' is never returned for shell commands - they always get + // a concrete permission (deny/ask/allow) based on the command's readonly status. + if ( + decision === 'default' && + toolName === 'run_shell_command' && + command !== undefined + ) { + return this.resolveDefaultPermission(command); + } + + return decision; } /** @@ -295,32 +310,46 @@ export class PermissionManager { * Evaluate a compound command by splitting it into sub-commands, * evaluating each independently, and returning the most restrictive result. * - * Restriction order: deny > ask > default > allow + * Restriction order: deny > ask > allow * - * Example: with rules `allow: [safe-cmd *, one-cmd *]` - * - "safe-cmd && one-cmd" → both allow → allow - * - "safe-cmd && two-cmd" → allow + default → default - * - "safe-cmd && evil-cmd" (deny: [evil-cmd]) → allow + deny → deny + * When a sub-command returns 'default' (no rule matches), it is resolved to + * the actual default permission using AST analysis: + * - Command substitution detected → 'deny' + * - Read-only command (cd, ls, git status, etc.) → 'allow' + * - Otherwise → 'ask' + * + * Example: with rules `allow: [git checkout *]` + * - "cd /path && git checkout -b feature" → allow (cd) + allow (rule) → allow + * - "rm /path && git checkout -b feature" → ask (rm) + allow (rule) → ask + * - "evil-cmd && git checkout" (deny: [evil-cmd]) → deny + allow → deny */ - private evaluateCompoundCommand( + private async evaluateCompoundCommand( ctx: PermissionCheckContext, subCommands: string[], - ): PermissionDecision { - const PRIORITY: Record = { + ): Promise { + // Type for resolved decisions (excludes 'default' since it's resolved) + type ResolvedDecision = 'allow' | 'ask' | 'deny'; + const PRIORITY: Record = { deny: 3, ask: 2, - default: 1, allow: 0, }; - let mostRestrictive: PermissionDecision = 'allow'; + let mostRestrictive: ResolvedDecision = 'allow'; for (const subCmd of subCommands) { const subCtx: PermissionCheckContext = { ...ctx, command: subCmd, }; - const decision = this.evaluateSingle(subCtx); + const rawDecision = this.evaluateSingle(subCtx); + + // Resolve 'default' to actual permission using AST analysis + // (same logic as ShellToolInvocation.getDefaultPermission) + const decision: ResolvedDecision = + rawDecision === 'default' + ? await this.resolveDefaultPermission(subCmd) + : (rawDecision as ResolvedDecision); if (PRIORITY[decision] > PRIORITY[mostRestrictive]) { mostRestrictive = decision; @@ -335,6 +364,34 @@ export class PermissionManager { return mostRestrictive; } + /** + * Resolve 'default' permission to actual permission using AST analysis. + * This mirrors the logic in ShellToolInvocation.getDefaultPermission(). + * + * @param command - The shell command to analyze. + * @returns 'deny' for command substitution, 'allow' for read-only, 'ask' otherwise. + */ + private async resolveDefaultPermission( + command: string, + ): Promise<'allow' | 'ask' | 'deny'> { + // Security: command substitution ($(), ``, <(), >()) → deny + if (detectCommandSubstitution(command)) { + return 'deny'; + } + + // AST-based read-only detection + try { + const isReadOnly = await isShellCommandReadOnlyAST(command); + if (isReadOnly) { + return 'allow'; + } + } catch { + // AST check failed, fall back to 'ask' + } + + return 'ask'; + } + // --------------------------------------------------------------------------- // Registry-level helper // --------------------------------------------------------------------------- @@ -347,7 +404,7 @@ export class PermissionManager { * `"Bash(rm -rf *)"` do NOT remove the tool from the registry – they only * deny specific invocations at runtime. */ - isToolEnabled(toolName: string): boolean { + async isToolEnabled(toolName: string): Promise { const canonicalName = resolveToolName(toolName); // If a coreTools allowlist is active, only explicitly listed tools are @@ -361,7 +418,7 @@ export class PermissionManager { // evaluate({ toolName }) without a command will only match rules that have // no specifier, which is the correct registry-level check. - const decision = this.evaluate({ toolName: canonicalName }); + const decision = await this.evaluate({ toolName: canonicalName }); return decision !== 'deny'; } @@ -375,7 +432,7 @@ export class PermissionManager { * @param command - The shell command to evaluate. * @returns The PermissionDecision for this command. */ - isCommandAllowed(command: string): PermissionDecision { + async isCommandAllowed(command: string): Promise { return this.evaluate({ toolName: 'run_shell_command', command, @@ -410,6 +467,15 @@ export class PermissionManager { hasRelevantRules(ctx: PermissionCheckContext): boolean { const { toolName, command, filePath, domain, specifier } = ctx; + if (ctx.toolName === 'run_shell_command' && command !== undefined) { + const subCommands = splitCompoundCommand(command); + if (subCommands.length > 1) { + return subCommands.some((subCmd) => + this.hasRelevantRules({ ...ctx, command: subCmd }), + ); + } + } + const pathCtx: PathMatchContext | undefined = this.config.getProjectRoot && this.config.getCwd ? { @@ -465,6 +531,69 @@ export class PermissionManager { return false; } + /** + * Returns true when the invocation is matched by an explicit `ask` rule. + * + * This is intentionally narrower than `evaluate(ctx) === 'ask'`. Shell + * commands can resolve to `ask` simply because they are non-read-only and no + * explicit allow/deny rule matched. That fallback should still allow users to + * create new allow rules, so callers must only hide "Always allow" when a + * real ask rule matched. + */ + hasMatchingAskRule(ctx: PermissionCheckContext): boolean { + const { toolName, command, filePath, domain, specifier } = ctx; + + if (ctx.toolName === 'run_shell_command' && command !== undefined) { + const subCommands = splitCompoundCommand(command); + if (subCommands.length > 1) { + return subCommands.some((subCmd) => + this.hasMatchingAskRule({ ...ctx, command: subCmd }), + ); + } + } + + const pathCtx: PathMatchContext | undefined = + this.config.getProjectRoot && this.config.getCwd + ? { + projectRoot: this.config.getProjectRoot(), + cwd: this.config.getCwd(), + } + : undefined; + + const matchArgs = [ + toolName, + command, + filePath, + domain, + pathCtx, + specifier, + ] as const; + + const askRules = [...this.sessionRules.ask, ...this.persistentRules.ask]; + + if (askRules.some((rule) => matchesRule(rule, ...matchArgs))) { + return true; + } + + if (ctx.toolName === 'run_shell_command' && ctx.command !== undefined) { + const cwd = pathCtx?.cwd ?? process.cwd(); + const ops = extractShellOperations(ctx.command, cwd); + return ops.some((op) => { + const opMatchArgs = [ + op.virtualTool, + undefined, + op.filePath, + op.domain, + pathCtx, + undefined, + ] as const; + return askRules.some((rule) => matchesRule(rule, ...opMatchArgs)); + }); + } + + return false; + } + // --------------------------------------------------------------------------- // Session rule management // --------------------------------------------------------------------------- diff --git a/packages/core/src/tools/edit.test.ts b/packages/core/src/tools/edit.test.ts index c675203856..71ff122094 100644 --- a/packages/core/src/tools/edit.test.ts +++ b/packages/core/src/tools/edit.test.ts @@ -922,5 +922,48 @@ describe('EditTool', () => { expect(params.old_string).toBe(initialContent); expect(params.new_string).toBe(modifiedContent); }); + + it('should not call ideClient.openDiff in AUTO_EDIT mode', async () => { + const initialContent = 'some old content here'; + fs.writeFileSync(filePath, initialContent); + const params: EditToolParams = { + file_path: filePath, + old_string: 'old', + new_string: 'new', + }; + (mockConfig.getApprovalMode as Mock).mockReturnValueOnce( + ApprovalMode.AUTO_EDIT, + ); + + const invocation = tool.build(params); + const confirmation = await invocation.getConfirmationDetails( + new AbortController().signal, + ); + + expect(ideClient.openDiff).not.toHaveBeenCalled(); + expect(confirmation).toBeDefined(); + expect((confirmation as any).ideConfirmation).toBeUndefined(); + }); + + it('should not call ideClient.openDiff in YOLO mode', async () => { + const initialContent = 'some old content here'; + fs.writeFileSync(filePath, initialContent); + const params: EditToolParams = { + file_path: filePath, + old_string: 'old', + new_string: 'new', + }; + (mockConfig.getApprovalMode as Mock).mockReturnValueOnce( + ApprovalMode.YOLO, + ); + + const invocation = tool.build(params); + const confirmation = await invocation.getConfirmationDetails( + new AbortController().signal, + ); + + expect(ideClient.openDiff).not.toHaveBeenCalled(); + expect((confirmation as any).ideConfirmation).toBeUndefined(); + }); }); }); diff --git a/packages/core/src/tools/edit.ts b/packages/core/src/tools/edit.ts index e5b1480b93..d8126bf3fa 100644 --- a/packages/core/src/tools/edit.ts +++ b/packages/core/src/tools/edit.ts @@ -297,9 +297,13 @@ class EditToolInvocation implements ToolInvocation { 'Proposed', DEFAULT_DIFF_OPTIONS, ); + const approvalMode = this.config.getApprovalMode(); const ideClient = await IdeClient.getInstance(); const ideConfirmation = - this.config.getIdeMode() && ideClient.isDiffingEnabled() + this.config.getIdeMode() && + ideClient.isDiffingEnabled() && + approvalMode !== ApprovalMode.AUTO_EDIT && + approvalMode !== ApprovalMode.YOLO ? ideClient.openDiff(this.params.file_path, editData.newContent) : undefined; diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts index 73047dbea3..a8748e3757 100644 --- a/packages/core/src/tools/shell.test.ts +++ b/packages/core/src/tools/shell.test.ts @@ -37,6 +37,7 @@ import * as crypto from 'node:crypto'; import { ToolErrorType } from './tool-error.js'; import { OUTPUT_UPDATE_INTERVAL_MS } from './shell.js'; import { createMockWorkspaceContext } from '../test-utils/mockWorkspaceContext.js'; +import { PermissionManager } from '../permissions/permission-manager.js'; describe('ShellTool', () => { let shellTool: ShellTool; @@ -49,6 +50,8 @@ describe('ShellTool', () => { mockConfig = { getCoreTools: vi.fn().mockReturnValue([]), + getPermissionsAllow: vi.fn().mockReturnValue([]), + getPermissionsAsk: vi.fn().mockReturnValue([]), getPermissionsDeny: vi.fn().mockReturnValue([]), getDebugMode: vi.fn().mockReturnValue(false), getTargetDir: vi.fn().mockReturnValue('/test/dir'), @@ -61,6 +64,7 @@ describe('ShellTool', () => { }, getTruncateToolOutputThreshold: vi.fn().mockReturnValue(0), getTruncateToolOutputLines: vi.fn().mockReturnValue(0), + getPermissionManager: vi.fn().mockReturnValue(undefined), getGeminiClient: vi.fn(), getGitCoAuthor: vi.fn().mockReturnValue({ enabled: true, @@ -91,21 +95,21 @@ describe('ShellTool', () => { }); describe('isCommandAllowed', () => { - it('should allow a command if no restrictions are provided', () => { + it('should allow a command if no restrictions are provided', async () => { (mockConfig.getCoreTools as Mock).mockReturnValue(undefined); (mockConfig.getPermissionsDeny as Mock).mockReturnValue(undefined); - expect(isCommandAllowed('ls -l', mockConfig).allowed).toBe(true); + expect((await isCommandAllowed('ls -l', mockConfig)).allowed).toBe(true); }); - it('should block a command with command substitution using $()', () => { - expect(isCommandAllowed('echo $(rm -rf /)', mockConfig).allowed).toBe( - false, - ); + it('should block a command with command substitution using $()', async () => { + expect( + (await isCommandAllowed('echo $(rm -rf /)', mockConfig)).allowed, + ).toBe(false); }); }); describe('build', () => { - it('should return an invocation for a valid command', () => { + it('should return an invocation for a valid command', async () => { const invocation = shellTool.build({ command: 'ls -l', is_background: false, @@ -113,13 +117,13 @@ describe('ShellTool', () => { expect(invocation).toBeDefined(); }); - it('should throw an error for an empty command', () => { + it('should throw an error for an empty command', async () => { expect(() => shellTool.build({ command: ' ', is_background: false }), ).toThrow('Command cannot be empty.'); }); - it('should throw an error for a relative directory path', () => { + it('should throw an error for a relative directory path', async () => { expect(() => shellTool.build({ command: 'ls', @@ -129,7 +133,7 @@ describe('ShellTool', () => { ).toThrow('Directory must be an absolute path.'); }); - it('should throw an error for a directory outside the workspace', () => { + it('should throw an error for a directory outside the workspace', async () => { (mockConfig.getWorkspaceContext as Mock).mockReturnValue( createMockWorkspaceContext('/test/dir', ['/another/workspace']), ); @@ -144,7 +148,7 @@ describe('ShellTool', () => { ); }); - it('should throw an error for a directory within the user skills directory', () => { + it('should throw an error for a directory within the user skills directory', async () => { expect(() => shellTool.build({ command: 'ls', @@ -156,7 +160,7 @@ describe('ShellTool', () => { ); }); - it('should throw an error for the user skills directory itself', () => { + it('should throw an error for the user skills directory itself', async () => { expect(() => shellTool.build({ command: 'ls', @@ -168,7 +172,7 @@ describe('ShellTool', () => { ); }); - it('should resolve directory path before checking user skills directory', () => { + it('should resolve directory path before checking user skills directory', async () => { expect(() => shellTool.build({ command: 'ls', @@ -180,7 +184,7 @@ describe('ShellTool', () => { ); }); - it('should return an invocation for a valid absolute directory path', () => { + it('should return an invocation for a valid absolute directory path', async () => { (mockConfig.getWorkspaceContext as Mock).mockReturnValue( createMockWorkspaceContext('/test/dir', ['/another/workspace']), ); @@ -192,7 +196,7 @@ describe('ShellTool', () => { expect(invocation).toBeDefined(); }); - it('should include background indicator in description when is_background is true', () => { + it('should include background indicator in description when is_background is true', async () => { const invocation = shellTool.build({ command: 'npm start', is_background: true, @@ -200,7 +204,7 @@ describe('ShellTool', () => { expect(invocation.getDescription()).toContain('[background]'); }); - it('should not include background indicator in description when is_background is false', () => { + it('should not include background indicator in description when is_background is false', async () => { const invocation = shellTool.build({ command: 'npm test', is_background: false, @@ -209,7 +213,7 @@ describe('ShellTool', () => { }); describe('is_background parameter coercion', () => { - it('should accept string "true" as boolean true', () => { + it('should accept string "true" as boolean true', async () => { const invocation = shellTool.build({ command: 'npm run dev', is_background: 'true' as unknown as boolean, @@ -218,7 +222,7 @@ describe('ShellTool', () => { expect(invocation.getDescription()).toContain('[background]'); }); - it('should accept string "false" as boolean false', () => { + it('should accept string "false" as boolean false', async () => { const invocation = shellTool.build({ command: 'npm run build', is_background: 'false' as unknown as boolean, @@ -227,7 +231,7 @@ describe('ShellTool', () => { expect(invocation.getDescription()).not.toContain('[background]'); }); - it('should accept string "True" as boolean true', () => { + it('should accept string "True" as boolean true', async () => { const invocation = shellTool.build({ command: 'npm run dev', is_background: 'True' as unknown as boolean, @@ -236,7 +240,7 @@ describe('ShellTool', () => { expect(invocation.getDescription()).toContain('[background]'); }); - it('should accept string "False" as boolean false', () => { + it('should accept string "False" as boolean false', async () => { const invocation = shellTool.build({ command: 'npm run build', is_background: 'False' as unknown as boolean, @@ -459,13 +463,13 @@ describe('ShellTool', () => { expect(result.error?.message).toBe('command failed'); }); - it('should throw an error for invalid parameters', () => { + it('should throw an error for invalid parameters', async () => { expect(() => shellTool.build({ command: '', is_background: false }), ).toThrow('Command cannot be empty.'); }); - it('should throw an error for invalid directory', () => { + it('should throw an error for invalid directory', async () => { expect(() => shellTool.build({ command: 'ls', @@ -967,7 +971,31 @@ describe('ShellTool', () => { expect(details.permissionRules).toEqual(['Bash(npm run *)']); }); - it('should throw an error if validation fails', () => { + it('should exclude already-allowed sub-commands from confirmation details in compound commands', async () => { + const pm = new PermissionManager({ + getPermissionsAllow: () => ['Bash(git add *)'], + getPermissionsAsk: () => [], + getPermissionsDeny: () => [], + getProjectRoot: () => '/test/dir', + getCwd: () => '/test/dir', + }); + pm.initialize(); + (mockConfig.getPermissionManager as Mock).mockReturnValue(pm); + + const invocation = shellTool.build({ + command: 'git add /tmp/file && git commit -m "msg"', + is_background: false, + }); + + const details = (await invocation.getConfirmationDetails( + new AbortController().signal, + )) as { rootCommand: string; permissionRules: string[] }; + + expect(details.rootCommand).toBe('git'); + expect(details.permissionRules).toEqual(['Bash(git commit *)']); + }); + + it('should throw an error if validation fails', async () => { expect(() => shellTool.build({ command: '', is_background: false }), ).toThrow(); @@ -975,13 +1003,13 @@ describe('ShellTool', () => { }); describe('getDescription', () => { - it('should return the windows description when on windows', () => { + it('should return the windows description when on windows', async () => { vi.mocked(os.platform).mockReturnValue('win32'); const shellTool = new ShellTool(mockConfig); expect(shellTool.description).toMatchSnapshot(); }); - it('should return the non-windows description when not on windows', () => { + it('should return the non-windows description when not on windows', async () => { vi.mocked(os.platform).mockReturnValue('linux'); const shellTool = new ShellTool(mockConfig); expect(shellTool.description).toMatchSnapshot(); @@ -1026,7 +1054,7 @@ describe('ShellTool', () => { }); describe('timeout parameter', () => { - it('should validate timeout parameter correctly', () => { + it('should validate timeout parameter correctly', async () => { // Valid timeout expect(() => { shellTool.build({ @@ -1091,7 +1119,7 @@ describe('ShellTool', () => { }).toThrow('params/timeout must be number'); }); - it('should include timeout in description for foreground commands', () => { + it('should include timeout in description for foreground commands', async () => { const invocation = shellTool.build({ command: 'npm test', is_background: false, @@ -1101,7 +1129,7 @@ describe('ShellTool', () => { expect(invocation.getDescription()).toBe('npm test [timeout: 30000ms]'); }); - it('should not include timeout in description for background commands', () => { + it('should not include timeout in description for background commands', async () => { const invocation = shellTool.build({ command: 'npm start', is_background: true, diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index 3d38eaf4b8..c73ef1d9a4 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -127,25 +127,40 @@ export class ShellToolInvocation extends BaseToolInvocation< _abortSignal: AbortSignal, ): Promise { const command = stripShellWrapper(this.params.command); + const pm = this.config.getPermissionManager?.(); // Split compound command and filter out already-allowed (read-only) sub-commands const subCommands = splitCommands(command); - const nonReadOnlySubCommands: string[] = []; + const confirmableSubCommands: string[] = []; for (const sub of subCommands) { + let isReadOnly = false; try { - const isReadOnly = await isShellCommandReadOnlyAST(sub); - if (!isReadOnly) { - nonReadOnlySubCommands.push(sub); - } + isReadOnly = await isShellCommandReadOnlyAST(sub); } catch { - nonReadOnlySubCommands.push(sub); // conservative: include if check fails + // conservative: treat unknown commands as requiring confirmation } + + if (isReadOnly) { + continue; + } + + if (pm) { + try { + if ((await pm.isCommandAllowed(sub)) === 'allow') { + continue; + } + } catch (e) { + debugLogger.warn('PermissionManager command check failed:', e); + } + } + + confirmableSubCommands.push(sub); } // Fallback to all sub-commands if everything was filtered out (shouldn't // normally happen since getDefaultPermission already returned 'ask'). const effectiveSubCommands = - nonReadOnlySubCommands.length > 0 ? nonReadOnlySubCommands : subCommands; + confirmableSubCommands.length > 0 ? confirmableSubCommands : subCommands; const rootCommands = [ ...new Set( diff --git a/packages/core/src/tools/write-file.test.ts b/packages/core/src/tools/write-file.test.ts index f4808cdc09..cecfc81f6b 100644 --- a/packages/core/src/tools/write-file.test.ts +++ b/packages/core/src/tools/write-file.test.ts @@ -321,6 +321,36 @@ describe('WriteFileTool', () => { expect(mockIdeClient.openDiff).not.toHaveBeenCalled(); }); + it('should not call openDiff in AUTO_EDIT mode', async () => { + mockConfigInternal.getApprovalMode.mockReturnValue( + ApprovalMode.AUTO_EDIT, + ); + const filePath = path.join(rootDir, 'ide_auto_edit_file.txt'); + const params = { file_path: filePath, content: 'test' }; + const invocation = tool.build(params); + + const confirmation = (await invocation.getConfirmationDetails( + abortSignal, + )) as ToolEditConfirmationDetails; + + expect(mockIdeClient.openDiff).not.toHaveBeenCalled(); + expect(confirmation.ideConfirmation).toBeUndefined(); + }); + + it('should not call openDiff in YOLO mode', async () => { + mockConfigInternal.getApprovalMode.mockReturnValue(ApprovalMode.YOLO); + const filePath = path.join(rootDir, 'ide_yolo_file.txt'); + const params = { file_path: filePath, content: 'test' }; + const invocation = tool.build(params); + + const confirmation = (await invocation.getConfirmationDetails( + abortSignal, + )) as ToolEditConfirmationDetails; + + expect(mockIdeClient.openDiff).not.toHaveBeenCalled(); + expect(confirmation.ideConfirmation).toBeUndefined(); + }); + it('should update params.content with IDE content when onConfirm is called', async () => { const filePath = path.join(rootDir, 'ide_onconfirm_file.txt'); const params = { file_path: filePath, content: 'original-content' }; diff --git a/packages/core/src/tools/write-file.ts b/packages/core/src/tools/write-file.ts index 1f1a30cddb..d056dfd2c0 100644 --- a/packages/core/src/tools/write-file.ts +++ b/packages/core/src/tools/write-file.ts @@ -138,9 +138,13 @@ class WriteFileToolInvocation extends BaseToolInvocation< DEFAULT_DIFF_OPTIONS, ); + const approvalMode = this.config.getApprovalMode(); const ideClient = await IdeClient.getInstance(); const ideConfirmation = - this.config.getIdeMode() && ideClient.isDiffingEnabled() + this.config.getIdeMode() && + ideClient.isDiffingEnabled() && + approvalMode !== ApprovalMode.AUTO_EDIT && + approvalMode !== ApprovalMode.YOLO ? ideClient.openDiff(this.params.file_path, this.params.content) : undefined; diff --git a/packages/core/src/utils/shell-utils.test.ts b/packages/core/src/utils/shell-utils.test.ts index 7485384f85..183e4f5398 100644 --- a/packages/core/src/utils/shell-utils.test.ts +++ b/packages/core/src/utils/shell-utils.test.ts @@ -55,82 +55,82 @@ afterEach(() => { }); describe('isCommandAllowed', () => { - it('should allow a command if no restrictions are provided', () => { - const result = isCommandAllowed('ls -l', config); + it('should allow a command if no restrictions are provided', async () => { + const result = await isCommandAllowed('ls -l', config); expect(result.allowed).toBe(true); }); - it('should allow a command if it is in the global allowlist', () => { + it('should allow a command if it is in the global allowlist', async () => { config.getCoreTools = () => ['ShellTool(ls)']; - const result = isCommandAllowed('ls -l', config); + const result = await isCommandAllowed('ls -l', config); expect(result.allowed).toBe(true); }); - it('should block a command if it is not in a strict global allowlist', () => { + it('should block a command if it is not in a strict global allowlist', async () => { config.getCoreTools = () => ['ShellTool(ls -l)']; - const result = isCommandAllowed('rm -rf /', config); + const result = await isCommandAllowed('rm -rf /', config); expect(result.allowed).toBe(false); expect(result.reason).toBe( `Command(s) not in the allowed commands list. Disallowed commands: "rm -rf /"`, ); }); - it('should block a command if it is in the blocked list', () => { + it('should block a command if it is in the blocked list', async () => { config.getPermissionsDeny = () => ['ShellTool(rm -rf /)']; - const result = isCommandAllowed('rm -rf /', config); + const result = await isCommandAllowed('rm -rf /', config); expect(result.allowed).toBe(false); expect(result.reason).toBe( `Command 'rm -rf /' is blocked by configuration`, ); }); - it('should prioritize the blocklist over the allowlist', () => { + it('should prioritize the blocklist over the allowlist', async () => { config.getCoreTools = () => ['ShellTool(rm -rf /)']; config.getPermissionsDeny = () => ['ShellTool(rm -rf /)']; - const result = isCommandAllowed('rm -rf /', config); + const result = await isCommandAllowed('rm -rf /', config); expect(result.allowed).toBe(false); expect(result.reason).toBe( `Command 'rm -rf /' is blocked by configuration`, ); }); - it('should allow any command when a wildcard is in coreTools', () => { + it('should allow any command when a wildcard is in coreTools', async () => { config.getCoreTools = () => ['ShellTool']; - const result = isCommandAllowed('any random command', config); + const result = await isCommandAllowed('any random command', config); expect(result.allowed).toBe(true); }); - it('should block any command when a wildcard is in excludeTools', () => { + it('should block any command when a wildcard is in excludeTools', async () => { config.getPermissionsDeny = () => ['run_shell_command']; - const result = isCommandAllowed('any random command', config); + const result = await isCommandAllowed('any random command', config); expect(result.allowed).toBe(false); expect(result.reason).toBe( 'Shell tool is globally disabled in configuration', ); }); - it('should block a command on the blocklist even with a wildcard allow', () => { + it('should block a command on the blocklist even with a wildcard allow', async () => { config.getCoreTools = () => ['ShellTool']; config.getPermissionsDeny = () => ['ShellTool(rm -rf /)']; - const result = isCommandAllowed('rm -rf /', config); + const result = await isCommandAllowed('rm -rf /', config); expect(result.allowed).toBe(false); expect(result.reason).toBe( `Command 'rm -rf /' is blocked by configuration`, ); }); - it('should allow a chained command if all parts are on the global allowlist', () => { + it('should allow a chained command if all parts are on the global allowlist', async () => { config.getCoreTools = () => [ 'run_shell_command(echo)', 'run_shell_command(ls)', ]; - const result = isCommandAllowed('echo "hello" && ls -l', config); + const result = await isCommandAllowed('echo "hello" && ls -l', config); expect(result.allowed).toBe(true); }); - it('should block a chained command if any part is blocked', () => { + it('should block a chained command if any part is blocked', async () => { config.getPermissionsDeny = () => ['run_shell_command(rm)']; - const result = isCommandAllowed('echo "hello" && rm -rf /', config); + const result = await isCommandAllowed('echo "hello" && rm -rf /', config); expect(result.allowed).toBe(false); expect(result.reason).toBe( `Command 'rm -rf /' is blocked by configuration`, @@ -138,20 +138,20 @@ describe('isCommandAllowed', () => { }); describe('command substitution', () => { - it('should block command substitution using `$(...)`', () => { - const result = isCommandAllowed('echo $(rm -rf /)', config); + it('should block command substitution using `$(...)`', async () => { + const result = await isCommandAllowed('echo $(rm -rf /)', config); expect(result.allowed).toBe(false); expect(result.reason).toContain('Command substitution'); }); - it('should block command substitution using `<(...)`', () => { - const result = isCommandAllowed('diff <(ls) <(ls -a)', config); + it('should block command substitution using `<(...)`', async () => { + const result = await isCommandAllowed('diff <(ls) <(ls -a)', config); expect(result.allowed).toBe(false); expect(result.reason).toContain('Command substitution'); }); - it('should block command substitution using `>(...)`', () => { - const result = isCommandAllowed( + it('should block command substitution using `>(...)`', async () => { + const result = await isCommandAllowed( 'echo "Log message" > >(tee log.txt)', config, ); @@ -159,20 +159,20 @@ describe('isCommandAllowed', () => { expect(result.reason).toContain('Command substitution'); }); - it('should block command substitution using backticks', () => { - const result = isCommandAllowed('echo `rm -rf /`', config); + it('should block command substitution using backticks', async () => { + const result = await isCommandAllowed('echo `rm -rf /`', config); expect(result.allowed).toBe(false); expect(result.reason).toContain('Command substitution'); }); - it('should allow substitution-like patterns inside single quotes', () => { + it('should allow substitution-like patterns inside single quotes', async () => { config.getCoreTools = () => ['ShellTool(echo)']; - const result = isCommandAllowed("echo '$(pwd)'", config); + const result = await isCommandAllowed("echo '$(pwd)'", config); expect(result.allowed).toBe(true); }); describe('heredocs', () => { - it('should allow substitution-like content in a quoted heredoc delimiter', () => { + it('should allow substitution-like content in a quoted heredoc delimiter', async () => { const cmd = [ "cat <<'EOF' > user_session.md", '```', @@ -182,55 +182,55 @@ describe('isCommandAllowed', () => { 'EOF', ].join('\n'); - const result = isCommandAllowed(cmd, config); + const result = await isCommandAllowed(cmd, config); expect(result.allowed).toBe(true); }); - it('should block command substitution in an unquoted heredoc body', () => { + it('should block command substitution in an unquoted heredoc body', async () => { const cmd = [ 'cat < user_session.md', "'$(rm -rf /)'", 'EOF', ].join('\n'); - const result = isCommandAllowed(cmd, config); + const result = await isCommandAllowed(cmd, config); expect(result.allowed).toBe(false); expect(result.reason).toContain('Command substitution'); }); - it('should block backtick command substitution in an unquoted heredoc body', () => { + it('should block backtick command substitution in an unquoted heredoc body', async () => { const cmd = ['cat < user_session.md', '`rm -rf /`', 'EOF'].join( '\n', ); - const result = isCommandAllowed(cmd, config); + const result = await isCommandAllowed(cmd, config); expect(result.allowed).toBe(false); expect(result.reason).toContain('Command substitution'); }); - it('should allow escaped command substitution in an unquoted heredoc body', () => { + it('should allow escaped command substitution in an unquoted heredoc body', async () => { const cmd = [ 'cat < user_session.md', '\\$(rm -rf /)', 'EOF', ].join('\n'); - const result = isCommandAllowed(cmd, config); + const result = await isCommandAllowed(cmd, config); expect(result.allowed).toBe(true); }); - it('should support tab-stripping heredocs (<<-)', () => { + it('should support tab-stripping heredocs (<<-)', async () => { const cmd = [ "cat <<-'EOF' > user_session.md", '\t$(rm -rf /)', '\tEOF', ].join('\n'); - const result = isCommandAllowed(cmd, config); + const result = await isCommandAllowed(cmd, config); expect(result.allowed).toBe(true); }); - it('should block command substitution split by line continuation in an unquoted heredoc body', () => { + it('should block command substitution split by line continuation in an unquoted heredoc body', async () => { const cmd = [ 'cat < user_session.md', '$\\', @@ -238,12 +238,12 @@ describe('isCommandAllowed', () => { 'EOF', ].join('\n'); - const result = isCommandAllowed(cmd, config); + const result = await isCommandAllowed(cmd, config); expect(result.allowed).toBe(false); expect(result.reason).toContain('Command substitution'); }); - it('should allow escaped command substitution split by line continuation in an unquoted heredoc body', () => { + it('should allow escaped command substitution split by line continuation in an unquoted heredoc body', async () => { const cmd = [ 'cat < user_session.md', '\\$\\', @@ -251,36 +251,39 @@ describe('isCommandAllowed', () => { 'EOF', ].join('\n'); - const result = isCommandAllowed(cmd, config); + const result = await isCommandAllowed(cmd, config); expect(result.allowed).toBe(true); }); }); describe('comments', () => { - it('should ignore heredoc operators inside comments', () => { + it('should ignore heredoc operators inside comments', async () => { const cmd = ["# Fake heredoc <<'EOF'", '$(rm -rf /)', 'EOF'].join('\n'); - const result = isCommandAllowed(cmd, config); + const result = await isCommandAllowed(cmd, config); expect(result.allowed).toBe(false); expect(result.reason).toContain('Command substitution'); }); - it('should allow command substitution patterns inside full-line comments', () => { + it('should allow command substitution patterns inside full-line comments', async () => { const cmd = ['# Note: $(rm -rf /) is dangerous', 'echo hello'].join( '\n', ); - const result = isCommandAllowed(cmd, config); + const result = await isCommandAllowed(cmd, config); expect(result.allowed).toBe(true); }); - it('should allow command substitution patterns inside inline comments', () => { - const result = isCommandAllowed('echo hello # $(rm -rf /)', config); + it('should allow command substitution patterns inside inline comments', async () => { + const result = await isCommandAllowed( + 'echo hello # $(rm -rf /)', + config, + ); expect(result.allowed).toBe(true); }); - it('should not treat # inside a word as a comment starter', () => { - const result = isCommandAllowed('echo foo#$(rm -rf /)', config); + it('should not treat # inside a word as a comment starter', async () => { + const result = await isCommandAllowed('echo foo#$(rm -rf /)', config); expect(result.allowed).toBe(false); expect(result.reason).toContain('Command substitution'); }); @@ -290,17 +293,17 @@ describe('isCommandAllowed', () => { describe('checkCommandPermissions', () => { describe('in "Default Allow" mode (no sessionAllowlist)', () => { - it('should return a detailed success object for an allowed command', () => { - const result = checkCommandPermissions('ls -l', config); + it('should return a detailed success object for an allowed command', async () => { + const result = await checkCommandPermissions('ls -l', config); expect(result).toEqual({ allAllowed: true, disallowedCommands: [], }); }); - it('should return a detailed failure object for a blocked command', () => { + it('should return a detailed failure object for a blocked command', async () => { config.getPermissionsDeny = () => ['ShellTool(rm)']; - const result = checkCommandPermissions('rm -rf /', config); + const result = await checkCommandPermissions('rm -rf /', config); expect(result).toEqual({ allAllowed: false, disallowedCommands: ['rm -rf /'], @@ -309,9 +312,9 @@ describe('checkCommandPermissions', () => { }); }); - it('should return a detailed failure object for a command not on a strict allowlist', () => { + it('should return a detailed failure object for a command not on a strict allowlist', async () => { config.getCoreTools = () => ['ShellTool(ls)']; - const result = checkCommandPermissions('git status && ls', config); + const result = await checkCommandPermissions('git status && ls', config); expect(result).toEqual({ allAllowed: false, disallowedCommands: ['git status'], @@ -322,8 +325,8 @@ describe('checkCommandPermissions', () => { }); describe('in "Default Deny" mode (with sessionAllowlist)', () => { - it('should allow a command on the sessionAllowlist', () => { - const result = checkCommandPermissions( + it('should allow a command on the sessionAllowlist', async () => { + const result = await checkCommandPermissions( 'ls -l', config, new Set(['ls -l']), @@ -331,8 +334,8 @@ describe('checkCommandPermissions', () => { expect(result.allAllowed).toBe(true); }); - it('should block a command not on the sessionAllowlist or global allowlist', () => { - const result = checkCommandPermissions( + it('should block a command not on the sessionAllowlist or global allowlist', async () => { + const result = await checkCommandPermissions( 'rm -rf /', config, new Set(['ls -l']), @@ -344,9 +347,9 @@ describe('checkCommandPermissions', () => { expect(result.disallowedCommands).toEqual(['rm -rf /']); }); - it('should allow a command on the global allowlist even if not on the session allowlist', () => { + it('should allow a command on the global allowlist even if not on the session allowlist', async () => { config.getCoreTools = () => ['ShellTool(git status)']; - const result = checkCommandPermissions( + const result = await checkCommandPermissions( 'git status', config, new Set(['ls -l']), @@ -354,9 +357,9 @@ describe('checkCommandPermissions', () => { expect(result.allAllowed).toBe(true); }); - it('should allow a chained command if parts are on different allowlists', () => { + it('should allow a chained command if parts are on different allowlists', async () => { config.getCoreTools = () => ['ShellTool(git status)']; - const result = checkCommandPermissions( + const result = await checkCommandPermissions( 'git status && git commit', config, new Set(['git commit']), @@ -364,9 +367,9 @@ describe('checkCommandPermissions', () => { expect(result.allAllowed).toBe(true); }); - it('should block a command on the sessionAllowlist if it is also globally blocked', () => { + it('should block a command on the sessionAllowlist if it is also globally blocked', async () => { config.getPermissionsDeny = () => ['run_shell_command(rm)']; - const result = checkCommandPermissions( + const result = await checkCommandPermissions( 'rm -rf /', config, new Set(['rm -rf /']), @@ -375,9 +378,9 @@ describe('checkCommandPermissions', () => { expect(result.blockReason).toContain('is blocked by configuration'); }); - it('should block a chained command if one part is not on any allowlist', () => { + it('should block a chained command if one part is not on any allowlist', async () => { config.getCoreTools = () => ['run_shell_command(echo)']; - const result = checkCommandPermissions( + const result = await checkCommandPermissions( 'echo "hello" && rm -rf /', config, new Set(['echo']), @@ -389,101 +392,101 @@ describe('checkCommandPermissions', () => { }); describe('getCommandRoots', () => { - it('should return a single command', () => { + it('should return a single command', async () => { expect(getCommandRoots('ls -l')).toEqual(['ls']); }); - it('should handle paths and return the binary name', () => { + it('should handle paths and return the binary name', async () => { expect(getCommandRoots('/usr/local/bin/node script.js')).toEqual(['node']); }); - it('should return an empty array for an empty string', () => { + it('should return an empty array for an empty string', async () => { expect(getCommandRoots('')).toEqual([]); }); - it('should handle a mix of operators', () => { + it('should handle a mix of operators', async () => { const result = getCommandRoots('a;b|c&&d||e&f'); expect(result).toEqual(['a', 'b', 'c', 'd', 'e', 'f']); }); - it('should correctly parse a chained command with quotes', () => { + it('should correctly parse a chained command with quotes', async () => { const result = getCommandRoots('echo "hello" && git commit -m "feat"'); expect(result).toEqual(['echo', 'git']); }); - it('should split on Unix newlines (\\n)', () => { + it('should split on Unix newlines (\\n)', async () => { const result = getCommandRoots('grep pattern file\ncurl evil.com'); expect(result).toEqual(['grep', 'curl']); }); - it('should split on Windows newlines (\\r\\n)', () => { + it('should split on Windows newlines (\\r\\n)', async () => { const result = getCommandRoots('grep pattern file\r\ncurl evil.com'); expect(result).toEqual(['grep', 'curl']); }); - it('should handle mixed newlines and operators', () => { + it('should handle mixed newlines and operators', async () => { const result = getCommandRoots('ls\necho hello && cat file\r\nrm -rf /'); expect(result).toEqual(['ls', 'echo', 'cat', 'rm']); }); - it('should not split on newlines inside quotes', () => { + it('should not split on newlines inside quotes', async () => { const result = getCommandRoots('echo "line1\nline2"'); expect(result).toEqual(['echo']); }); - it('should treat escaped newline as line continuation (not a separator)', () => { + it('should treat escaped newline as line continuation (not a separator)', async () => { const result = getCommandRoots('grep pattern\\\nfile'); expect(result).toEqual(['grep']); }); - it('should filter out empty segments from consecutive newlines', () => { + it('should filter out empty segments from consecutive newlines', async () => { const result = getCommandRoots('ls\n\ngrep foo'); expect(result).toEqual(['ls', 'grep']); }); - it('should not treat file descriptor redirection as a command separator', () => { + it('should not treat file descriptor redirection as a command separator', async () => { const result = getCommandRoots('npm run build 2>&1 | head -100'); expect(result).toEqual(['npm', 'head']); }); - it('should not treat >| redirection as a pipeline separator', () => { + it('should not treat >| redirection as a pipeline separator', async () => { const result = getCommandRoots('echo hello >| out.txt'); expect(result).toEqual(['echo']); }); }); describe('stripShellWrapper', () => { - it('should strip sh -c with quotes', () => { + it('should strip sh -c with quotes', async () => { expect(stripShellWrapper('sh -c "ls -l"')).toEqual('ls -l'); }); - it('should strip bash -c with extra whitespace', () => { + it('should strip bash -c with extra whitespace', async () => { expect(stripShellWrapper(' bash -c "ls -l" ')).toEqual('ls -l'); }); - it('should strip zsh -c without quotes', () => { + it('should strip zsh -c without quotes', async () => { expect(stripShellWrapper('zsh -c ls -l')).toEqual('ls -l'); }); - it('should strip cmd.exe /c', () => { + it('should strip cmd.exe /c', async () => { expect(stripShellWrapper('cmd.exe /c "dir"')).toEqual('dir'); }); - it('should not strip anything if no wrapper is present', () => { + it('should not strip anything if no wrapper is present', async () => { expect(stripShellWrapper('ls -l')).toEqual('ls -l'); }); }); describe('escapeShellArg', () => { describe('POSIX (bash)', () => { - it('should use shell-quote for escaping', () => { + it('should use shell-quote for escaping', async () => { mockQuote.mockReturnValueOnce("'escaped value'"); const result = escapeShellArg('raw value', 'bash'); expect(mockQuote).toHaveBeenCalledWith(['raw value']); expect(result).toBe("'escaped value'"); }); - it('should handle empty strings', () => { + it('should handle empty strings', async () => { const result = escapeShellArg('', 'bash'); expect(result).toBe(''); expect(mockQuote).not.toHaveBeenCalled(); @@ -492,39 +495,39 @@ describe('escapeShellArg', () => { describe('Windows', () => { describe('when shell is cmd.exe', () => { - it('should wrap simple arguments in double quotes', () => { + it('should wrap simple arguments in double quotes', async () => { const result = escapeShellArg('search term', 'cmd'); expect(result).toBe('"search term"'); }); - it('should escape internal double quotes by doubling them', () => { + it('should escape internal double quotes by doubling them', async () => { const result = escapeShellArg('He said "Hello"', 'cmd'); expect(result).toBe('"He said ""Hello"""'); }); - it('should handle empty strings', () => { + it('should handle empty strings', async () => { const result = escapeShellArg('', 'cmd'); expect(result).toBe(''); }); }); describe('when shell is PowerShell', () => { - it('should wrap simple arguments in single quotes', () => { + it('should wrap simple arguments in single quotes', async () => { const result = escapeShellArg('search term', 'powershell'); expect(result).toBe("'search term'"); }); - it('should escape internal single quotes by doubling them', () => { + it('should escape internal single quotes by doubling them', async () => { const result = escapeShellArg("It's a test", 'powershell'); expect(result).toBe("'It''s a test'"); }); - it('should handle double quotes without escaping them', () => { + it('should handle double quotes without escaping them', async () => { const result = escapeShellArg('He said "Hello"', 'powershell'); expect(result).toBe('\'He said "Hello"\''); }); - it('should handle empty strings', () => { + it('should handle empty strings', async () => { const result = escapeShellArg('', 'powershell'); expect(result).toBe(''); }); @@ -539,7 +542,7 @@ describe('getShellConfiguration', () => { process.env = originalEnv; }); - it('should return bash configuration on Linux', () => { + it('should return bash configuration on Linux', async () => { mockPlatform.mockReturnValue('linux'); const config = getShellConfiguration(); expect(config.executable).toBe('bash'); @@ -547,7 +550,7 @@ describe('getShellConfiguration', () => { expect(config.shell).toBe('bash'); }); - it('should return bash configuration on macOS (darwin)', () => { + it('should return bash configuration on macOS (darwin)', async () => { mockPlatform.mockReturnValue('darwin'); const config = getShellConfiguration(); expect(config.executable).toBe('bash'); @@ -560,7 +563,7 @@ describe('getShellConfiguration', () => { mockPlatform.mockReturnValue('win32'); }); - it('should return cmd.exe configuration by default', () => { + it('should return cmd.exe configuration by default', async () => { delete process.env['ComSpec']; const config = getShellConfiguration(); expect(config.executable).toBe('cmd.exe'); @@ -568,7 +571,7 @@ describe('getShellConfiguration', () => { expect(config.shell).toBe('cmd'); }); - it('should respect ComSpec for cmd.exe', () => { + it('should respect ComSpec for cmd.exe', async () => { const cmdPath = 'C:\\WINDOWS\\system32\\cmd.exe'; process.env['ComSpec'] = cmdPath; const config = getShellConfiguration(); @@ -577,7 +580,7 @@ describe('getShellConfiguration', () => { expect(config.shell).toBe('cmd'); }); - it('should return PowerShell configuration if ComSpec points to powershell.exe', () => { + it('should return PowerShell configuration if ComSpec points to powershell.exe', async () => { const psPath = 'C:\\WINDOWS\\System32\\WindowsPowerShell\\v1.0\\powershell.exe'; process.env['ComSpec'] = psPath; @@ -587,7 +590,7 @@ describe('getShellConfiguration', () => { expect(config.shell).toBe('powershell'); }); - it('should return PowerShell configuration if ComSpec points to pwsh.exe', () => { + it('should return PowerShell configuration if ComSpec points to pwsh.exe', async () => { const pwshPath = 'C:\\Program Files\\PowerShell\\7\\pwsh.exe'; process.env['ComSpec'] = pwshPath; const config = getShellConfiguration(); @@ -596,7 +599,7 @@ describe('getShellConfiguration', () => { expect(config.shell).toBe('powershell'); }); - it('should be case-insensitive when checking ComSpec', () => { + it('should be case-insensitive when checking ComSpec', async () => { process.env['ComSpec'] = 'C:\\Path\\To\\POWERSHELL.EXE'; const config = getShellConfiguration(); expect(config.executable).toBe('C:\\Path\\To\\POWERSHELL.EXE'); @@ -607,12 +610,12 @@ describe('getShellConfiguration', () => { }); describe('isCommandNeedPermission', () => { - it('returns false for read-only commands', () => { + it('returns false for read-only commands', async () => { const result = isCommandNeedsPermission('ls'); expect(result.requiresPermission).toBe(false); }); - it('returns true for mutating commands with reason', () => { + it('returns true for mutating commands with reason', async () => { const result = isCommandNeedsPermission('rm -rf temp'); expect(result.requiresPermission).toBe(true); expect(result.reason).toContain('requires permission to execute'); @@ -621,13 +624,13 @@ describe('isCommandNeedPermission', () => { describe('checkArgumentSafety', () => { describe('command substitution patterns', () => { - it('should detect $() command substitution', () => { + it('should detect $() command substitution', async () => { const result = checkArgumentSafety('$(whoami)'); expect(result.isSafe).toBe(false); expect(result.dangerousPatterns).toContain('$() command substitution'); }); - it('should detect backtick command substitution', () => { + it('should detect backtick command substitution', async () => { const result = checkArgumentSafety('`whoami`'); expect(result.isSafe).toBe(false); expect(result.dangerousPatterns).toContain( @@ -635,13 +638,13 @@ describe('checkArgumentSafety', () => { ); }); - it('should detect <() process substitution', () => { + it('should detect <() process substitution', async () => { const result = checkArgumentSafety('<(cat file)'); expect(result.isSafe).toBe(false); expect(result.dangerousPatterns).toContain('<() process substitution'); }); - it('should detect >() process substitution', () => { + it('should detect >() process substitution', async () => { const result = checkArgumentSafety('>(tee file)'); expect(result.isSafe).toBe(false); expect(result.dangerousPatterns).toContain('>() process substitution'); @@ -649,25 +652,25 @@ describe('checkArgumentSafety', () => { }); describe('command separators', () => { - it('should detect semicolon separator', () => { + it('should detect semicolon separator', async () => { const result = checkArgumentSafety('arg1; rm -rf /'); expect(result.isSafe).toBe(false); expect(result.dangerousPatterns).toContain('; command separator'); }); - it('should detect pipe', () => { + it('should detect pipe', async () => { const result = checkArgumentSafety('arg1 | cat file'); expect(result.isSafe).toBe(false); expect(result.dangerousPatterns).toContain('| pipe'); }); - it('should detect && operator', () => { + it('should detect && operator', async () => { const result = checkArgumentSafety('arg1 && ls'); expect(result.isSafe).toBe(false); expect(result.dangerousPatterns).toContain('&& AND operator'); }); - it('should detect || operator', () => { + it('should detect || operator', async () => { const result = checkArgumentSafety('arg1 || ls'); expect(result.isSafe).toBe(false); expect(result.dangerousPatterns).toContain('|| OR operator'); @@ -675,7 +678,7 @@ describe('checkArgumentSafety', () => { }); describe('background execution', () => { - it('should detect background operator', () => { + it('should detect background operator', async () => { const result = checkArgumentSafety('arg1 & ls'); expect(result.isSafe).toBe(false); expect(result.dangerousPatterns).toContain('& background operator'); @@ -683,19 +686,19 @@ describe('checkArgumentSafety', () => { }); describe('input/output redirection', () => { - it('should detect output redirection', () => { + it('should detect output redirection', async () => { const result = checkArgumentSafety('arg1 > file'); expect(result.isSafe).toBe(false); expect(result.dangerousPatterns).toContain('> output redirection'); }); - it('should detect input redirection', () => { + it('should detect input redirection', async () => { const result = checkArgumentSafety('arg1 < file'); expect(result.isSafe).toBe(false); expect(result.dangerousPatterns).toContain('< input redirection'); }); - it('should detect append redirection', () => { + it('should detect append redirection', async () => { const result = checkArgumentSafety('arg1 >> file'); expect(result.isSafe).toBe(false); expect(result.dangerousPatterns).toContain('> output redirection'); @@ -703,45 +706,45 @@ describe('checkArgumentSafety', () => { }); describe('safe inputs', () => { - it('should accept simple arguments', () => { + it('should accept simple arguments', async () => { const result = checkArgumentSafety('arg1 arg2'); expect(result.isSafe).toBe(true); expect(result.dangerousPatterns).toHaveLength(0); }); - it('should accept arguments with numbers', () => { + it('should accept arguments with numbers', async () => { const result = checkArgumentSafety('file123.txt'); expect(result.isSafe).toBe(true); }); - it('should accept arguments with hyphens', () => { + it('should accept arguments with hyphens', async () => { const result = checkArgumentSafety('--flag=value'); expect(result.isSafe).toBe(true); }); - it('should accept arguments with underscores', () => { + it('should accept arguments with underscores', async () => { const result = checkArgumentSafety('my_file_name'); expect(result.isSafe).toBe(true); }); - it('should accept arguments with dots', () => { + it('should accept arguments with dots', async () => { const result = checkArgumentSafety('path/to/file.txt'); expect(result.isSafe).toBe(true); }); - it('should accept empty string', () => { + it('should accept empty string', async () => { const result = checkArgumentSafety(''); expect(result.isSafe).toBe(true); }); - it('should accept arguments with spaces (quoted)', () => { + it('should accept arguments with spaces (quoted)', async () => { const result = checkArgumentSafety('hello world'); expect(result.isSafe).toBe(true); }); }); describe('multiple dangerous patterns', () => { - it('should detect multiple dangerous patterns', () => { + it('should detect multiple dangerous patterns', async () => { const result = checkArgumentSafety('$(whoami); rm -rf / &'); expect(result.isSafe).toBe(false); expect(result.dangerousPatterns).toContain('$() command substitution'); diff --git a/packages/core/src/utils/shell-utils.ts b/packages/core/src/utils/shell-utils.ts index c30e554939..b44fb9b2a3 100644 --- a/packages/core/src/utils/shell-utils.ts +++ b/packages/core/src/utils/shell-utils.ts @@ -652,16 +652,16 @@ export function detectCommandSubstitution(command: string): boolean { * presence activates "Default Deny" mode. * @returns An object detailing which commands are not allowed. */ -export function checkCommandPermissions( +export async function checkCommandPermissions( command: string, config: Config, sessionAllowlist?: Set, -): { +): Promise<{ allAllowed: boolean; disallowedCommands: string[]; blockReason?: string; isHardDenial?: boolean; -} { +}> { // Disallow command substitution for security. if (detectCommandSubstitution(command)) { return { @@ -699,7 +699,7 @@ export function checkCommandPermissions( if (isSessionAllowed) continue; } - const decision = pm.isCommandAllowed(cmd); + const decision = await pm.isCommandAllowed(cmd); if (decision === 'deny') { return { @@ -976,12 +976,15 @@ export function isCommandAvailable(command: string): { return { available: path !== null, error }; } -export function isCommandAllowed( +export async function isCommandAllowed( command: string, config: Config, -): { allowed: boolean; reason?: string } { +): Promise<{ allowed: boolean; reason?: string }> { // By not providing a sessionAllowlist, we invoke "default allow" behavior. - const { allAllowed, blockReason } = checkCommandPermissions(command, config); + const { allAllowed, blockReason } = await checkCommandPermissions( + command, + config, + ); if (allAllowed) { return { allowed: true }; } diff --git a/packages/vscode-ide-companion/src/webview/providers/WebViewProvider.ts b/packages/vscode-ide-companion/src/webview/providers/WebViewProvider.ts index e5e69e66ac..1820259177 100644 --- a/packages/vscode-ide-companion/src/webview/providers/WebViewProvider.ts +++ b/packages/vscode-ide-companion/src/webview/providers/WebViewProvider.ts @@ -256,25 +256,6 @@ export class WebViewProvider { this.agentManager.onPermissionRequest( async (request: RequestPermissionRequest) => { - // Auto-approve in auto/yolo mode (no UI, no diff) - if (this.isAutoMode()) { - const options = request.options || []; - const pick = (substr: string) => - options.find((o) => - (o.optionId || '').toLowerCase().includes(substr), - )?.optionId; - const pickByKind = (k: string) => - options.find((o) => (o.kind || '').toLowerCase().includes(k)) - ?.optionId; - const optionId = - pick('allow_once') || - pickByKind('allow') || - pick('proceed') || - options[0]?.optionId || - 'allow_once'; - return optionId; - } - // Send permission request to WebView this.sendMessageToWebView({ type: 'permissionRequest', From cb97bf6068b473a4360d71b879bc4d42e96232ea Mon Sep 17 00:00:00 2001 From: LaZzyMan Date: Fri, 27 Mar 2026 10:37:20 +0800 Subject: [PATCH 3/5] fix(acp): add missing await for async isToolEnabled in Session.runTool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PermissionManager.isToolEnabled was changed to async in this PR but the call site in Session.runTool was not updated, causing the Promise to be evaluated as a truthy value and the L1 tool-enablement check to always pass — effectively disabling permission denial in the ACP session path. --- packages/cli/src/acp-integration/session/Session.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/cli/src/acp-integration/session/Session.ts b/packages/cli/src/acp-integration/session/Session.ts index b89044be44..fd009dddf2 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -590,7 +590,7 @@ export class Session implements SessionContext { // ---- L1: Tool enablement check ---- const pm = this.config.getPermissionManager?.(); - if (pm && !pm.isToolEnabled(fc.name as string)) { + if (pm && !(await pm.isToolEnabled(fc.name as string))) { return earlyErrorResponse( new Error( `Qwen Code requires permission to use "${fc.name}", but that permission was declined.`, From 07273015fd2e3da1dcaee5ea620090750f1b3d7d Mon Sep 17 00:00:00 2001 From: LaZzyMan Date: Fri, 27 Mar 2026 10:46:35 +0800 Subject: [PATCH 4/5] test(acp): add coverage for L1 isToolEnabled deny in Session.runTool Verify that when PermissionManager.isToolEnabled resolves to false the tool is never executed and no permission dialog is opened. This guards against the async-await regression fixed in the previous commit. --- .../acp-integration/session/Session.test.ts | 57 +++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/packages/cli/src/acp-integration/session/Session.test.ts b/packages/cli/src/acp-integration/session/Session.test.ts index 330506770a..4b52293213 100644 --- a/packages/cli/src/acp-integration/session/Session.test.ts +++ b/packages/cli/src/acp-integration/session/Session.test.ts @@ -346,6 +346,63 @@ describe('Session', () => { ); }); + it('returns permission error for disabled tools (L1 isToolEnabled check)', async () => { + const executeSpy = vi.fn(); + const invocation = { + params: { path: '/tmp/file.txt' }, + getDefaultPermission: vi.fn().mockResolvedValue('ask'), + getConfirmationDetails: vi.fn().mockResolvedValue({ + type: 'info', + title: 'Need permission', + prompt: 'Allow?', + onConfirm: vi.fn(), + }), + getDescription: vi.fn().mockReturnValue('Write file'), + toolLocations: vi.fn().mockReturnValue([]), + execute: executeSpy, + }; + const tool = { + name: 'write_file', + kind: core.Kind.Edit, + build: vi.fn().mockReturnValue(invocation), + }; + + mockToolRegistry.getTool.mockReturnValue(tool); + mockConfig.getApprovalMode = vi + .fn() + .mockReturnValue(ApprovalMode.DEFAULT); + // Mock a PermissionManager that denies the tool + mockConfig.getPermissionManager = vi.fn().mockReturnValue({ + isToolEnabled: vi.fn().mockResolvedValue(false), + }); + mockChat.sendMessageStream = vi.fn().mockResolvedValue( + (async function* () { + yield { + type: core.StreamEventType.CHUNK, + value: { + functionCalls: [ + { + id: 'call-denied', + name: 'write_file', + args: { path: '/tmp/file.txt' }, + }, + ], + }, + }; + })(), + ); + + await session.prompt({ + sessionId: 'test-session-id', + prompt: [{ type: 'text', text: 'write something' }], + }); + + // Tool should NOT have been executed + expect(executeSpy).not.toHaveBeenCalled(); + // No permission dialog should have been opened + expect(mockClient.requestPermission).not.toHaveBeenCalled(); + }); + it('respects permission-request hook allow decisions without opening ACP permission dialog', async () => { const hookSpy = vi .spyOn(core, 'firePermissionRequestHook') From cec0074b2d9455fe7cd25b26999f4a72680a0f09 Mon Sep 17 00:00:00 2001 From: LaZzyMan Date: Fri, 27 Mar 2026 10:58:00 +0800 Subject: [PATCH 5/5] fix(acp): prefer filePath over fileName in buildPermissionRequestContent When building diff content for edit-type permission requests, use confirmation.filePath (full path) when available, falling back to confirmation.fileName. This aligns with the test expectation and ensures SubAgentTracker sends the correct file path in ACP permission dialogs. --- packages/cli/src/acp-integration/session/permissionUtils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/cli/src/acp-integration/session/permissionUtils.ts b/packages/cli/src/acp-integration/session/permissionUtils.ts index fbbc0ef4c6..06434b4a04 100644 --- a/packages/cli/src/acp-integration/session/permissionUtils.ts +++ b/packages/cli/src/acp-integration/session/permissionUtils.ts @@ -76,7 +76,7 @@ export function buildPermissionRequestContent( if (confirmation.type === 'edit') { content.push({ type: 'diff', - path: confirmation.fileName, + path: confirmation.filePath ?? confirmation.fileName, oldText: confirmation.originalContent ?? '', newText: confirmation.newContent, });