diff --git a/packages/cli/src/ui/components/messages/RedirectionConfirmation.test.tsx b/packages/cli/src/ui/components/messages/RedirectionConfirmation.test.tsx new file mode 100644 index 0000000000..d5b7f54f0e --- /dev/null +++ b/packages/cli/src/ui/components/messages/RedirectionConfirmation.test.tsx @@ -0,0 +1,54 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, vi, beforeAll } from 'vitest'; +import { ToolConfirmationMessage } from './ToolConfirmationMessage.js'; +import type { + ToolCallConfirmationDetails, + Config, +} from '@google/gemini-cli-core'; +import { initializeShellParsers } from '@google/gemini-cli-core'; +import { renderWithProviders } from '../../../test-utils/render.js'; + +describe('ToolConfirmationMessage Redirection', () => { + beforeAll(async () => { + await initializeShellParsers(); + }); + + const mockConfig = { + isTrustedFolder: () => true, + getIdeMode: () => false, + } as unknown as Config; + + it('should display redirection warning and tip for redirected commands', () => { + const confirmationDetails: ToolCallConfirmationDetails = { + type: 'exec', + title: 'Confirm Shell Command', + command: 'echo "hello" > test.txt', + rootCommand: 'echo, redirection (>)', + rootCommands: ['echo'], + onConfirm: vi.fn(), + }; + + const { lastFrame } = renderWithProviders( + , + ); + + const output = lastFrame(); + expect(output).toContain('echo "hello" > test.txt'); + expect(output).toContain( + 'Note: Command contains redirection which can be undesirable.', + ); + expect(output).toContain( + 'Tip: Toggle auto-edit (Shift+Tab) to allow redirection in the future.', + ); + }); +}); diff --git a/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx b/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx index 24dac94e67..468c8dc806 100644 --- a/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx +++ b/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx @@ -13,13 +13,23 @@ import type { ToolCallConfirmationDetails, Config, } from '@google/gemini-cli-core'; -import { IdeClient, ToolConfirmationOutcome } from '@google/gemini-cli-core'; +import { + IdeClient, + ToolConfirmationOutcome, + hasRedirection, +} from '@google/gemini-cli-core'; import type { RadioSelectItem } from '../shared/RadioButtonSelect.js'; import { RadioButtonSelect } from '../shared/RadioButtonSelect.js'; -import { MaxSizedBox } from '../shared/MaxSizedBox.js'; +import { MaxSizedBox, MINIMUM_MAX_HEIGHT } from '../shared/MaxSizedBox.js'; import { useKeypress } from '../../hooks/useKeypress.js'; import { theme } from '../../semantic-colors.js'; import { useSettings } from '../../contexts/SettingsContext.js'; +import { + REDIRECTION_WARNING_NOTE_LABEL, + REDIRECTION_WARNING_NOTE_TEXT, + REDIRECTION_WARNING_TIP_LABEL, + REDIRECTION_WARNING_TIP_TEXT, +} from '../../textConstants.js'; export interface ToolConfirmationMessageProps { confirmationDetails: ToolCallConfirmationDetails; @@ -270,30 +280,79 @@ export const ToolConfirmationMessage: React.FC< } } else if (confirmationDetails.type === 'exec') { const executionProps = confirmationDetails; + + const commandsToDisplay = + executionProps.commands && executionProps.commands.length > 1 + ? executionProps.commands + : [executionProps.command]; + const containsRedirection = commandsToDisplay.some((cmd) => + hasRedirection(cmd), + ); + let bodyContentHeight = availableBodyContentHeight(); + let warnings: React.ReactNode = null; + if (bodyContentHeight !== undefined) { bodyContentHeight -= 2; // Account for padding; } + if (containsRedirection) { + // Calculate lines needed for Note and Tip + const safeWidth = Math.max(terminalWidth, 1); + const noteLength = + REDIRECTION_WARNING_NOTE_LABEL.length + + REDIRECTION_WARNING_NOTE_TEXT.length; + const tipLength = + REDIRECTION_WARNING_TIP_LABEL.length + + REDIRECTION_WARNING_TIP_TEXT.length; + + const noteLines = Math.ceil(noteLength / safeWidth); + const tipLines = Math.ceil(tipLength / safeWidth); + const spacerLines = 1; + const warningHeight = noteLines + tipLines + spacerLines; + + if (bodyContentHeight !== undefined) { + bodyContentHeight = Math.max( + bodyContentHeight - warningHeight, + MINIMUM_MAX_HEIGHT, + ); + } + + warnings = ( + <> + + + + {REDIRECTION_WARNING_NOTE_LABEL} + {REDIRECTION_WARNING_NOTE_TEXT} + + + + + {REDIRECTION_WARNING_TIP_LABEL} + {REDIRECTION_WARNING_TIP_TEXT} + + + + ); + } + bodyContent = ( - - - {executionProps.commands && executionProps.commands.length > 1 ? ( - executionProps.commands.map((cmd, idx) => ( + + + + {commandsToDisplay.map((cmd, idx) => ( {cmd} - )) - ) : ( - - {executionProps.command} - - )} - - + ))} + + + {warnings} + ); } else if (confirmationDetails.type === 'info') { const infoProps = confirmationDetails; diff --git a/packages/cli/src/ui/textConstants.ts b/packages/cli/src/ui/textConstants.ts index 53236cfed6..a7ea77de79 100644 --- a/packages/cli/src/ui/textConstants.ts +++ b/packages/cli/src/ui/textConstants.ts @@ -11,3 +11,10 @@ export const SCREEN_READER_MODEL_PREFIX = 'Model: '; export const SCREEN_READER_LOADING = 'loading'; export const SCREEN_READER_RESPONDING = 'responding'; + +export const REDIRECTION_WARNING_NOTE_LABEL = 'Note: '; +export const REDIRECTION_WARNING_NOTE_TEXT = + 'Command contains redirection which can be undesirable.'; +export const REDIRECTION_WARNING_TIP_LABEL = 'Tip: '; // Padded to align with "Note: " +export const REDIRECTION_WARNING_TIP_TEXT = + 'Toggle auto-edit (Shift+Tab) to allow redirection in the future.'; diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index f73d4ad12a..585d7b9bf6 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -595,7 +595,6 @@ export class CoreToolScheduler { name: toolCall.request.name, args: toolCall.request.args, }; - const serverName = toolCall.tool instanceof DiscoveredMCPTool ? toolCall.tool.serverName diff --git a/packages/core/src/policy/config.test.ts b/packages/core/src/policy/config.test.ts index 608f1c51c7..b6dc71c71b 100644 --- a/packages/core/src/policy/config.test.ts +++ b/packages/core/src/policy/config.test.ts @@ -98,7 +98,7 @@ describe('createPolicyEngineConfig', () => { expect(config.rules).toEqual([]); vi.doUnmock('node:fs/promises'); - }); + }, 30000); it('should allow tools in tools.allowed', async () => { const { createPolicyEngineConfig } = await import('./config.js'); diff --git a/packages/core/src/policy/policy-engine.test.ts b/packages/core/src/policy/policy-engine.test.ts index c30681a429..a5df8e8167 100644 --- a/packages/core/src/policy/policy-engine.test.ts +++ b/packages/core/src/policy/policy-engine.test.ts @@ -1279,6 +1279,123 @@ describe('PolicyEngine', () => { expect(result.decision).toBe(PolicyDecision.ALLOW); }); + + it('should require confirmation for a compound command with redirection even if individual commands are allowed', async () => { + const rules: PolicyRule[] = [ + { + toolName: 'run_shell_command', + argsPattern: /"command":"mkdir\b/, + decision: PolicyDecision.ALLOW, + priority: 20, + }, + { + toolName: 'run_shell_command', + argsPattern: /"command":"echo\b/, + decision: PolicyDecision.ALLOW, + priority: 20, + }, + ]; + + engine = new PolicyEngine({ rules }); + + // The full command has redirection, even if the individual split commands do not. + // splitCommands will return ['mkdir -p "bar"', 'echo "hello"'] + // The redirection '> bar/test.md' is stripped by splitCommands. + const result = await engine.check( + { + name: 'run_shell_command', + args: { command: 'mkdir -p "bar" && echo "hello" > bar/test.md' }, + }, + undefined, + ); + + expect(result.decision).toBe(PolicyDecision.ASK_USER); + }); + + it('should report redirection when a sub-command specifically has redirection', async () => { + const rules: PolicyRule[] = [ + { + toolName: 'run_shell_command', + argsPattern: /"command":"mkdir\b/, + decision: PolicyDecision.ALLOW, + priority: 20, + }, + { + toolName: 'run_shell_command', + argsPattern: /"command":"echo\b/, + decision: PolicyDecision.ALLOW, + priority: 20, + }, + ]; + + engine = new PolicyEngine({ rules }); + + // In this case, we mock splitCommands to keep the redirection in the sub-command + vi.mocked(initializeShellParsers).mockResolvedValue(undefined); + const { splitCommands } = await import('../utils/shell-utils.js'); + vi.mocked(splitCommands).mockReturnValueOnce([ + 'mkdir bar', + 'echo hello > bar/test.md', + ]); + + const result = await engine.check( + { + name: 'run_shell_command', + args: { command: 'mkdir bar && echo hello > bar/test.md' }, + }, + undefined, + ); + + expect(result.decision).toBe(PolicyDecision.ASK_USER); + }); + + it('should allow redirected shell commands in AUTO_EDIT mode if individual commands are allowed', async () => { + const rules: PolicyRule[] = [ + { + toolName: 'run_shell_command', + argsPattern: /"command":"echo\b/, + decision: PolicyDecision.ALLOW, + priority: 20, + }, + ]; + + engine = new PolicyEngine({ rules }); + engine.setApprovalMode(ApprovalMode.AUTO_EDIT); + + const result = await engine.check( + { + name: 'run_shell_command', + args: { command: 'echo "hello" > test.txt' }, + }, + undefined, + ); + + expect(result.decision).toBe(PolicyDecision.ALLOW); + }); + + it('should allow compound commands with safe operators (&&, ||) if individual commands are allowed', async () => { + const rules: PolicyRule[] = [ + { + toolName: 'run_shell_command', + argsPattern: /"command":"echo\b/, + decision: PolicyDecision.ALLOW, + priority: 20, + }, + ]; + + engine = new PolicyEngine({ rules }); + + // "echo hello && echo world" should be allowed since both parts are ALLOW and no redirection is present. + const result = await engine.check( + { + name: 'run_shell_command', + args: { command: 'echo hello && echo world' }, + }, + undefined, + ); + + expect(result.decision).toBe(PolicyDecision.ALLOW); + }); }); describe('safety checker integration', () => { diff --git a/packages/core/src/policy/policy-engine.ts b/packages/core/src/policy/policy-engine.ts index 3394dc5b30..48feb537e6 100644 --- a/packages/core/src/policy/policy-engine.ts +++ b/packages/core/src/policy/policy-engine.ts @@ -14,6 +14,7 @@ import { type HookExecutionContext, getHookSource, ApprovalMode, + type CheckResult, } from './types.js'; import { stableStringify } from './stable-stringify.js'; import { debugLogger } from '../utils/debugLogger.js'; @@ -141,6 +142,18 @@ export class PolicyEngine { return this.approvalMode; } + private shouldDowngradeForRedirection( + command: string, + allowRedirection?: boolean, + ): boolean { + return ( + !allowRedirection && + hasRedirection(command) && + this.approvalMode !== ApprovalMode.AUTO_EDIT && + this.approvalMode !== ApprovalMode.YOLO + ); + } + /** * Check if a shell command is allowed. */ @@ -152,7 +165,7 @@ export class PolicyEngine { dir_path: string | undefined, allowRedirection?: boolean, rule?: PolicyRule, - ): Promise<{ decision: PolicyDecision; rule?: PolicyRule }> { + ): Promise { if (!command) { return { decision: this.applyNonInteractiveMode(ruleDecision), @@ -190,11 +203,20 @@ export class PolicyEngine { let aggregateDecision = PolicyDecision.ALLOW; let responsibleRule: PolicyRule | undefined; + // Check for redirection on the full command string + if (this.shouldDowngradeForRedirection(command, allowRedirection)) { + debugLogger.debug( + `[PolicyEngine.check] Downgrading ALLOW to ASK_USER for redirected command: ${command}`, + ); + aggregateDecision = PolicyDecision.ASK_USER; + responsibleRule = undefined; // Inherent policy + } + for (const rawSubCmd of subCommands) { const subCmd = rawSubCmd.trim(); // Prevent infinite recursion for the root command if (subCmd === command) { - if (!allowRedirection && hasRedirection(subCmd)) { + if (this.shouldDowngradeForRedirection(subCmd, allowRedirection)) { debugLogger.debug( `[PolicyEngine.check] Downgrading ALLOW to ASK_USER for redirected command: ${subCmd}`, ); @@ -224,7 +246,7 @@ export class PolicyEngine { // subResult.decision is already filtered through applyNonInteractiveMode by this.check() const subDecision = subResult.decision; - // If any part is DENIED, the whole command is DENIED + // If any part is DENIED, the whole command is DENY if (subDecision === PolicyDecision.DENY) { return { decision: PolicyDecision.DENY, @@ -243,8 +265,7 @@ export class PolicyEngine { // Check for redirection in allowed sub-commands if ( subDecision === PolicyDecision.ALLOW && - !allowRedirection && - hasRedirection(subCmd) + this.shouldDowngradeForRedirection(subCmd, allowRedirection) ) { debugLogger.debug( `[PolicyEngine.check] Downgrading ALLOW to ASK_USER for redirected command: ${subCmd}`, @@ -255,6 +276,7 @@ export class PolicyEngine { } } } + return { decision: this.applyNonInteractiveMode(aggregateDecision), // If we stayed at ALLOW, we return the original rule (if any). @@ -276,10 +298,7 @@ export class PolicyEngine { async check( toolCall: FunctionCall, serverName: string | undefined, - ): Promise<{ - decision: PolicyDecision; - rule?: PolicyRule; - }> { + ): Promise { let stringifiedArgs: string | undefined; // Compute stringified args once before the loop if ( @@ -299,7 +318,9 @@ export class PolicyEngine { let command: string | undefined; let shellDirPath: string | undefined; - if (toolCall.name && SHELL_TOOL_NAMES.includes(toolCall.name)) { + const toolName = toolCall.name; + + if (toolName && SHELL_TOOL_NAMES.includes(toolName)) { isShellCommand = true; const args = toolCall.args as { command?: string; dir_path?: string }; command = args?.command; @@ -330,9 +351,9 @@ export class PolicyEngine { `[PolicyEngine.check] MATCHED rule: toolName=${rule.toolName}, decision=${rule.decision}, priority=${rule.priority}, argsPattern=${rule.argsPattern?.source || 'none'}`, ); - if (isShellCommand) { + if (isShellCommand && toolName) { const shellResult = await this.checkShellCommand( - toolCall.name!, + toolName, command, rule.decision, serverName, @@ -345,11 +366,6 @@ export class PolicyEngine { matchedRule = shellResult.rule; break; } - // If no rule returned (e.g. downgraded to default ASK_USER due to redirection), - // we might still want to blame the matched rule? - // No, test says we should return undefined rule if implicit. - matchedRule = shellResult.rule; - break; } else { decision = this.applyNonInteractiveMode(rule.decision); matchedRule = rule; @@ -358,31 +374,27 @@ export class PolicyEngine { } } - if (!decision) { - // No matching rule found, use default decision + // Default if no rule matched + if (decision === undefined) { debugLogger.debug( `[PolicyEngine.check] NO MATCH - using default decision: ${this.defaultDecision}`, ); - decision = this.applyNonInteractiveMode(this.defaultDecision); - - // If it's a shell command and we fell back to default, we MUST still verify subcommands! - // This is critical for security: "git commit && git push" where "git push" is DENY but "git commit" has no rule. - if (isShellCommand && decision !== PolicyDecision.DENY) { + if (toolName && SHELL_TOOL_NAMES.includes(toolName)) { const shellResult = await this.checkShellCommand( - toolCall.name!, + toolName, command, - decision, // default decision + this.defaultDecision, serverName, shellDirPath, - false, // no rule, so no allowRedirection - undefined, // no rule ); decision = shellResult.decision; matchedRule = shellResult.rule; + } else { + decision = this.applyNonInteractiveMode(this.defaultDecision); } } - // If decision is not DENY, run safety checkers + // Safety checks if (decision !== PolicyDecision.DENY && this.checkerRunner) { for (const checkerRule of this.checkers) { if ( @@ -402,10 +414,9 @@ export class PolicyEngine { toolCall, checkerRule.checker, ); - if (result.decision === SafetyCheckDecision.DENY) { debugLogger.debug( - `[PolicyEngine.check] Safety checker denied: ${result.reason}`, + `[PolicyEngine.check] Safety checker '${checkerRule.checker.name}' denied execution: ${result.reason}`, ); return { decision: PolicyDecision.DENY, @@ -419,7 +430,8 @@ export class PolicyEngine { } } catch (error) { debugLogger.debug( - `[PolicyEngine.check] Safety checker failed: ${error}`, + `[PolicyEngine.check] Safety checker '${checkerRule.checker.name}' threw an error:`, + error, ); return { decision: PolicyDecision.DENY, diff --git a/packages/core/src/policy/types.ts b/packages/core/src/policy/types.ts index 21c7c7f190..9cec21cfcc 100644 --- a/packages/core/src/policy/types.ts +++ b/packages/core/src/policy/types.ts @@ -265,3 +265,8 @@ export interface PolicySettings { }; mcpServers?: Record; } + +export interface CheckResult { + decision: PolicyDecision; + rule?: PolicyRule; +} diff --git a/packages/core/src/tools/glob.test.ts b/packages/core/src/tools/glob.test.ts index 381a47dc12..d015c37e59 100644 --- a/packages/core/src/tools/glob.test.ts +++ b/packages/core/src/tools/glob.test.ts @@ -84,7 +84,7 @@ describe('GlobTool', () => { expect(result.llmContent).toContain(path.join(tempRootDir, 'fileA.txt')); expect(result.llmContent).toContain(path.join(tempRootDir, 'FileB.TXT')); expect(result.returnDisplay).toBe('Found 2 matching file(s)'); - }); + }, 30000); it('should find files case-sensitively when case_sensitive is true', async () => { const params: GlobToolParams = { pattern: '*.txt', case_sensitive: true }; @@ -95,16 +95,17 @@ describe('GlobTool', () => { expect(result.llmContent).not.toContain( path.join(tempRootDir, 'FileB.TXT'), ); - }); + }, 30000); it('should find files case-insensitively by default (pattern: *.TXT)', async () => { const params: GlobToolParams = { pattern: '*.TXT' }; const invocation = globTool.build(params); + const result = await invocation.execute(abortSignal); - expect(result.llmContent).toContain('Found 2 file(s)'); - expect(result.llmContent).toContain(path.join(tempRootDir, 'fileA.txt')); - expect(result.llmContent).toContain(path.join(tempRootDir, 'FileB.TXT')); - }); + + expect(result.llmContent).toContain('fileA.txt'); + expect(result.llmContent).toContain('FileB.TXT'); + }, 30000); it('should find files case-insensitively when case_sensitive is false (pattern: *.TXT)', async () => { const params: GlobToolParams = { @@ -116,7 +117,7 @@ describe('GlobTool', () => { expect(result.llmContent).toContain('Found 2 file(s)'); expect(result.llmContent).toContain(path.join(tempRootDir, 'fileA.txt')); expect(result.llmContent).toContain(path.join(tempRootDir, 'FileB.TXT')); - }); + }, 30000); it('should find files using a pattern that includes a subdirectory', async () => { const params: GlobToolParams = { pattern: 'sub/*.md' }; @@ -129,7 +130,7 @@ describe('GlobTool', () => { expect(result.llmContent).toContain( path.join(tempRootDir, 'sub', 'FileD.MD'), ); - }); + }, 30000); it('should find files in a specified relative path (relative to rootDir)', async () => { const params: GlobToolParams = { pattern: '*.md', dir_path: 'sub' }; @@ -142,7 +143,7 @@ describe('GlobTool', () => { expect(result.llmContent).toContain( path.join(tempRootDir, 'sub', 'FileD.MD'), ); - }); + }, 30000); it('should find files using a deep globstar pattern (e.g., **/*.log)', async () => { const params: GlobToolParams = { pattern: '**/*.log' }; @@ -152,7 +153,7 @@ describe('GlobTool', () => { expect(result.llmContent).toContain( path.join(tempRootDir, 'sub', 'deep', 'fileE.log'), ); - }); + }, 30000); it('should return "No files found" message when pattern matches nothing', async () => { const params: GlobToolParams = { pattern: '*.nonexistent' }; @@ -162,7 +163,7 @@ describe('GlobTool', () => { 'No files found matching pattern "*.nonexistent"', ); expect(result.returnDisplay).toBe('No files found'); - }); + }, 30000); it('should find files with special characters in the name', async () => { await fs.writeFile(path.join(tempRootDir, 'file[1].txt'), 'content'); @@ -173,7 +174,7 @@ describe('GlobTool', () => { expect(result.llmContent).toContain( path.join(tempRootDir, 'file[1].txt'), ); - }); + }, 30000); it('should find files with special characters like [] and () in the path', async () => { const filePath = path.join( @@ -190,7 +191,7 @@ describe('GlobTool', () => { const result = await invocation.execute(abortSignal); expect(result.llmContent).toContain('Found 1 file(s)'); expect(result.llmContent).toContain(filePath); - }); + }, 30000); it('should correctly sort files by modification time (newest first)', async () => { const params: GlobToolParams = { pattern: '*.sortme' }; @@ -216,7 +217,7 @@ describe('GlobTool', () => { expect(path.resolve(filesListed[1])).toBe( path.resolve(tempRootDir, 'older.sortme'), ); - }); + }, 30000); it('should return a PATH_NOT_IN_WORKSPACE error if path is outside workspace', async () => { // Bypassing validation to test execute method directly @@ -226,7 +227,7 @@ describe('GlobTool', () => { const result = await invocation.execute(abortSignal); expect(result.error?.type).toBe(ToolErrorType.PATH_NOT_IN_WORKSPACE); expect(result.returnDisplay).toBe('Path is not within workspace'); - }); + }, 30000); it('should return a GLOB_EXECUTION_ERROR on glob failure', async () => { vi.mocked(glob.glob).mockRejectedValue(new Error('Glob failed')); @@ -239,7 +240,7 @@ describe('GlobTool', () => { ); // Reset glob. vi.mocked(glob.glob).mockReset(); - }); + }, 30000); }); describe('validateToolParams', () => { diff --git a/packages/core/src/tools/grep.test.ts b/packages/core/src/tools/grep.test.ts index 131042fe3b..7c9f224feb 100644 --- a/packages/core/src/tools/grep.test.ts +++ b/packages/core/src/tools/grep.test.ts @@ -145,7 +145,7 @@ describe('GrepTool', () => { ); expect(result.llmContent).toContain('L1: another world in sub dir'); expect(result.returnDisplay).toBe('Found 3 matches'); - }); + }, 30000); it('should find matches in a specific path', async () => { const params: GrepToolParams = { pattern: 'world', dir_path: 'sub' }; @@ -157,7 +157,7 @@ describe('GrepTool', () => { expect(result.llmContent).toContain('File: fileC.txt'); // Path relative to 'sub' expect(result.llmContent).toContain('L1: another world in sub dir'); expect(result.returnDisplay).toBe('Found 1 match'); - }); + }, 30000); it('should find matches with an include glob', async () => { const params: GrepToolParams = { pattern: 'hello', include: '*.js' }; @@ -171,7 +171,7 @@ describe('GrepTool', () => { 'L2: function baz() { return "hello"; }', ); expect(result.returnDisplay).toBe('Found 1 match'); - }); + }, 30000); it('should find matches with an include glob and path', async () => { await fs.writeFile( @@ -191,7 +191,7 @@ describe('GrepTool', () => { expect(result.llmContent).toContain('File: another.js'); expect(result.llmContent).toContain('L1: const greeting = "hello";'); expect(result.returnDisplay).toBe('Found 1 match'); - }); + }, 30000); it('should return "No matches found" when pattern does not exist', async () => { const params: GrepToolParams = { pattern: 'nonexistentpattern' }; @@ -201,7 +201,7 @@ describe('GrepTool', () => { 'No matches found for pattern "nonexistentpattern" in the workspace directory.', ); expect(result.returnDisplay).toBe('No matches found'); - }); + }, 30000); it('should handle regex special characters correctly', async () => { const params: GrepToolParams = { pattern: 'foo.*bar' }; // Matches 'const foo = "bar";' @@ -212,7 +212,7 @@ describe('GrepTool', () => { ); expect(result.llmContent).toContain('File: fileB.js'); expect(result.llmContent).toContain('L1: const foo = "bar";'); - }); + }, 30000); it('should be case-insensitive by default (JS fallback)', async () => { const params: GrepToolParams = { pattern: 'HELLO' }; @@ -227,14 +227,14 @@ describe('GrepTool', () => { expect(result.llmContent).toContain( 'L2: function baz() { return "hello"; }', ); - }); + }, 30000); it('should throw an error if params are invalid', async () => { const params = { dir_path: '.' } as unknown as GrepToolParams; // Invalid: pattern missing expect(() => grepTool.build(params)).toThrow( /params must have required property 'pattern'/, ); - }); + }, 30000); it('should return a GREP_EXECUTION_ERROR on failure', async () => { vi.mocked(glob.globStream).mockRejectedValue(new Error('Glob failed')); @@ -243,7 +243,7 @@ describe('GrepTool', () => { const result = await invocation.execute(abortSignal); expect(result.error?.type).toBe(ToolErrorType.GREP_EXECUTION_ERROR); vi.mocked(glob.globStream).mockReset(); - }); + }, 30000); }); describe('multi-directory workspace', () => { diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts index ed2ff86c5f..9b05afec36 100644 --- a/packages/core/src/tools/shell.test.ts +++ b/packages/core/src/tools/shell.test.ts @@ -538,4 +538,56 @@ describe('ShellTool', () => { expect(shellTool.description).toMatchSnapshot(); }); }); + + describe('getConfirmationDetails', () => { + it('should annotate sub-commands with redirection correctly', async () => { + const shellTool = new ShellTool(mockConfig, createMockMessageBus()); + const command = 'mkdir -p baz && echo "hello" > baz/test.md && ls'; + const invocation = shellTool.build({ command }); + + // @ts-expect-error - getConfirmationDetails is protected + const details = await invocation.getConfirmationDetails( + new AbortController().signal, + ); + + expect(details).not.toBe(false); + if (details && details.type === 'exec') { + expect(details.rootCommand).toBe('mkdir, echo, redirection (>), ls'); + } + }); + + it('should annotate all redirected sub-commands', async () => { + const shellTool = new ShellTool(mockConfig, createMockMessageBus()); + const command = 'cat < input.txt && grep "foo" > output.txt'; + const invocation = shellTool.build({ command }); + + // @ts-expect-error - getConfirmationDetails is protected + const details = await invocation.getConfirmationDetails( + new AbortController().signal, + ); + + expect(details).not.toBe(false); + if (details && details.type === 'exec') { + expect(details.rootCommand).toBe( + 'cat, redirection (<), grep, redirection (>)', + ); + } + }); + + it('should annotate sub-commands with pipes correctly', async () => { + const shellTool = new ShellTool(mockConfig, createMockMessageBus()); + const command = 'ls | grep "baz"'; + const invocation = shellTool.build({ command }); + + // @ts-expect-error - getConfirmationDetails is protected + const details = await invocation.getConfirmationDetails( + new AbortController().signal, + ); + + expect(details).not.toBe(false); + if (details && details.type === 'exec') { + expect(details.rootCommand).toBe('ls, grep'); + } + }); + }); }); diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index 4fc8a64735..e5e375b9ef 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -38,6 +38,8 @@ import { getCommandRoots, initializeShellParsers, stripShellWrapper, + parseCommandDetails, + hasRedirection, } from '../utils/shell-utils.js'; import { SHELL_TOOL_NAME } from './tool-names.js'; import type { MessageBus } from '../confirmation-bus/message-bus.js'; @@ -101,17 +103,25 @@ export class ShellToolInvocation extends BaseToolInvocation< _abortSignal: AbortSignal, ): Promise { const command = stripShellWrapper(this.params.command); - let rootCommands = [...new Set(getCommandRoots(command))]; - // Fallback for UI display if parser fails or returns no commands (e.g. - // variable assignments only) - if (rootCommands.length === 0 && command.trim()) { + const parsed = parseCommandDetails(command); + let rootCommandDisplay = ''; + + if (!parsed || parsed.hasError || parsed.details.length === 0) { + // Fallback if parser fails const fallback = command.trim().split(/\s+/)[0]; - if (fallback) { - rootCommands = [fallback]; + rootCommandDisplay = fallback || 'shell command'; + if (hasRedirection(command)) { + rootCommandDisplay += ', redirection'; } + } else { + rootCommandDisplay = parsed.details + .map((detail) => detail.name) + .join(', '); } + const rootCommands = [...new Set(getCommandRoots(command))]; + // Rely entirely on PolicyEngine for interactive confirmation. // If we are here, it means PolicyEngine returned ASK_USER (or no message bus), // so we must provide confirmation details. @@ -119,7 +129,7 @@ export class ShellToolInvocation extends BaseToolInvocation< type: 'exec', title: 'Confirm Shell Command', command: this.params.command, - rootCommand: rootCommands.join(', '), + rootCommand: rootCommandDisplay, rootCommands, onConfirm: async (outcome: ToolConfirmationOutcome) => { await this.publishPolicyUpdate(outcome); @@ -306,7 +316,7 @@ export class ShellToolInvocation extends BaseToolInvocation< `Command: ${this.params.command}`, `Directory: ${this.params.dir_path || '(root)'}`, `Output: ${result.output || '(empty)'}`, - `Error: ${finalError}`, // Use the cleaned error string. + `Error: ${finalError}`, `Exit Code: ${result.exitCode ?? '(none)'}`, `Signal: ${result.signal ?? '(none)'}`, `Background PIDs: ${ diff --git a/packages/core/src/tools/tools.ts b/packages/core/src/tools/tools.ts index cca65ce404..6029ba6673 100644 --- a/packages/core/src/tools/tools.ts +++ b/packages/core/src/tools/tools.ts @@ -45,8 +45,10 @@ export interface ToolInvocation< toolLocations(): ToolLocation[]; /** - * Determines if the tool should prompt for confirmation before execution. - * @returns Confirmation details or false if no confirmation is needed. + * Checks if the tool call should be confirmed by the user before execution. + * + * @param abortSignal An AbortSignal that can be used to cancel the confirmation request. + * @returns A ToolCallConfirmationDetails object if confirmation is required, or false if not. */ shouldConfirmExecute( abortSignal: AbortSignal, @@ -143,7 +145,7 @@ export abstract class BaseToolInvocation< ) { if (this._toolName) { const options = this.getPolicyUpdateOptions(outcome); - await this.messageBus.publish({ + void this.messageBus.publish({ type: MessageBusType.UPDATE_POLICY, toolName: this._toolName, persist: outcome === ToolConfirmationOutcome.ProceedAlwaysAndSave, @@ -179,16 +181,21 @@ export abstract class BaseToolInvocation< protected getMessageBusDecision( abortSignal: AbortSignal, ): Promise<'ALLOW' | 'DENY' | 'ASK_USER'> { - if (!this.messageBus) { + if (!this.messageBus || !this._toolName) { // If there's no message bus, we can't make a decision, so we allow. // The legacy confirmation flow will still apply if the tool needs it. return Promise.resolve('ALLOW'); } const correlationId = randomUUID(); - const toolCall = { - name: this._toolName || this.constructor.name, - args: this.params as Record, + const request: ToolConfirmationRequest = { + type: MessageBusType.TOOL_CONFIRMATION_REQUEST, + correlationId, + toolCall: { + name: this._toolName, + args: this.params as Record, + }, + serverName: this._serverName, }; return new Promise<'ALLOW' | 'DENY' | 'ASK_USER'>((resolve) => { @@ -197,18 +204,19 @@ export abstract class BaseToolInvocation< return; } - let timeoutId: NodeJS.Timeout | undefined; + let timeoutId: NodeJS.Timeout | null = null; + let unsubscribe: (() => void) | null = null; const cleanup = () => { if (timeoutId) { clearTimeout(timeoutId); - timeoutId = undefined; + timeoutId = null; + } + if (unsubscribe) { + unsubscribe(); + unsubscribe = null; } abortSignal.removeEventListener('abort', abortHandler); - this.messageBus.unsubscribe( - MessageBusType.TOOL_CONFIRMATION_RESPONSE, - responseHandler, - ); }; const abortHandler = () => { @@ -245,17 +253,15 @@ export abstract class BaseToolInvocation< MessageBusType.TOOL_CONFIRMATION_RESPONSE, responseHandler, ); - - const request: ToolConfirmationRequest = { - type: MessageBusType.TOOL_CONFIRMATION_REQUEST, - toolCall, - correlationId, - serverName: this._serverName, + unsubscribe = () => { + this.messageBus?.unsubscribe( + MessageBusType.TOOL_CONFIRMATION_RESPONSE, + responseHandler, + ); }; try { - // eslint-disable-next-line @typescript-eslint/no-floating-promises - this.messageBus.publish(request); + void this.messageBus.publish(request); } catch (_error) { cleanup(); resolve('ALLOW'); diff --git a/packages/core/src/utils/shell-utils.test.ts b/packages/core/src/utils/shell-utils.test.ts index 745e85ca30..81b43abf50 100644 --- a/packages/core/src/utils/shell-utils.test.ts +++ b/packages/core/src/utils/shell-utils.test.ts @@ -18,6 +18,7 @@ import { getCommandRoots, getShellConfiguration, initializeShellParsers, + parseCommandDetails, stripShellWrapper, hasRedirection, resolveExecutable, @@ -168,6 +169,20 @@ describe('getCommandRoots', () => { expect(result).toEqual(['echo', 'cat']); }); + it('should correctly identify input redirection with explicit file descriptor', () => { + const result = parseCommandDetails('ls 2< input.txt'); + const redirection = result?.details.find((d) => + d.name.startsWith('redirection'), + ); + expect(redirection?.name).toBe('redirection (<)'); + }); + + it('should filter out all redirections from getCommandRoots', () => { + expect(getCommandRoots('cat < input.txt')).toEqual(['cat']); + expect(getCommandRoots('ls 2> error.log')).toEqual(['ls']); + expect(getCommandRoots('exec 3<&0')).toEqual(['exec']); + }); + it('should handle parser initialization failures gracefully', async () => { // Reset modules to clear singleton state vi.resetModules(); @@ -220,6 +235,11 @@ describe('hasRedirection', () => { expect(hasRedirection('cat < input')).toBe(true); }); + it('should detect redirection with explicit file descriptor', () => { + expect(hasRedirection('ls 2> error.log')).toBe(true); + expect(hasRedirection('exec 3<&0')).toBe(true); + }); + it('should detect append redirection', () => { expect(hasRedirection('echo hello >> world')).toBe(true); }); @@ -242,6 +262,11 @@ describe('hasRedirection', () => { // A pipe is a 'pipeline' node. expect(hasRedirection('echo hello | cat')).toBe(false); }); + + it('should return false when redirection characters are inside quotes in bash', () => { + mockPlatform.mockReturnValue('linux'); + expect(hasRedirection('echo "a > b"')).toBe(false); + }); }); describeWindowsOnly('PowerShell integration', () => { diff --git a/packages/core/src/utils/shell-utils.ts b/packages/core/src/utils/shell-utils.ts index 230870415c..6610a5d45c 100644 --- a/packages/core/src/utils/shell-utils.ts +++ b/packages/core/src/utils/shell-utils.ts @@ -141,6 +141,7 @@ export async function initializeShellParsers(): Promise { export interface ParsedCommandDetail { name: string; text: string; + startIndex: number; } interface CommandParseResult { @@ -194,6 +195,13 @@ foreach ($commandAst in $commandAsts) { 'utf16le', ).toString('base64'); +const REDIRECTION_NAMES = new Set([ + 'redirection (<)', + 'redirection (>)', + 'heredoc (<<)', + 'herestring (<<<)', +]); + function createParser(): Parser | null { if (!bashLanguage) { if (treeSitterInitializationError) { @@ -278,6 +286,24 @@ function extractNameFromNode(node: Node): string | null { } return normalizeCommandName(firstChild.text); } + case 'file_redirect': { + // The first child might be a file descriptor (e.g., '2>'). + // We iterate to find the actual operator token. + for (let i = 0; i < node.childCount; i++) { + const child = node.child(i); + if (child && child.text.includes('<')) { + return 'redirection (<)'; + } + if (child && child.text.includes('>')) { + return 'redirection (>)'; + } + } + return 'redirection (>)'; + } + case 'heredoc_redirect': + return 'heredoc (<<)'; + case 'herestring_redirect': + return 'herestring (<<<)'; default: return null; } @@ -293,43 +319,19 @@ function collectCommandDetails( while (stack.length > 0) { const current = stack.pop()!; - let name: string | null = null; - let ignoreChildId: number | undefined; - - if (current.type === 'redirected_statement') { - const body = current.childForFieldName('body'); - if (body) { - const bodyName = extractNameFromNode(body); - if (bodyName) { - name = bodyName; - ignoreChildId = body.id; - - // If we ignore the body node (because we used it to name the redirected_statement), - // we must still traverse its children to find nested commands (e.g. command substitution). - for (let i = body.namedChildCount - 1; i >= 0; i -= 1) { - const grandChild = body.namedChild(i); - if (grandChild) { - stack.push(grandChild); - } - } - } - } - } - - if (!name) { - name = extractNameFromNode(current); - } - + const name = extractNameFromNode(current); if (name) { details.push({ name, text: source.slice(current.startIndex, current.endIndex).trim(), + startIndex: current.startIndex, }); } - for (let i = current.namedChildCount - 1; i >= 0; i -= 1) { - const child = current.namedChild(i); - if (child && child.id !== ignoreChildId) { + // Traverse all children to find all sub-components (commands, redirections, etc.) + for (let i = current.childCount - 1; i >= 0; i -= 1) { + const child = current.child(i); + if (child) { stack.push(child); } } @@ -424,7 +426,7 @@ function parseBashCommandDetails(command: string): CommandParseResult | null { } } return { - details, + details: details.sort((a, b) => a.startIndex - b.startIndex), hasError, }; } @@ -499,6 +501,7 @@ function parsePowerShellCommandDetails( return { name, text, + startIndex: 0, }; }) .filter((detail): detail is ParsedCommandDetail => detail !== null); @@ -610,6 +613,12 @@ export function escapeShellArg(arg: string, shell: ShellType): string { */ export function hasRedirection(command: string): boolean { const fallbackCheck = () => /[><]/.test(command); + + // If there are no redirection characters at all, we can skip parsing. + if (!fallbackCheck()) { + return false; + } + const configuration = getShellConfiguration(); if (configuration.shell === 'powershell') { @@ -684,7 +693,10 @@ export function getCommandRoots(command: string): string[] { return []; } - return parsed.details.map((detail) => detail.name).filter(Boolean); + return parsed.details + .map((detail) => detail.name) + .filter((name) => !REDIRECTION_NAMES.has(name)) + .filter(Boolean); } export function stripShellWrapper(command: string): string { diff --git a/packages/core/vitest.config.ts b/packages/core/vitest.config.ts index b8027f6512..cda8a07d0e 100644 --- a/packages/core/vitest.config.ts +++ b/packages/core/vitest.config.ts @@ -10,6 +10,7 @@ export default defineConfig({ test: { reporters: ['default', 'junit'], timeout: 30000, + hookTimeout: 30000, silent: true, setupFiles: ['./test-setup.ts'], outputFile: {