more consistent confirmations, TODO to improve write confirmations, drop "description" from execution confirmation, add confirmation to new (still dummy) shell tool (#176)

This commit is contained in:
Olcan 2025-04-25 14:05:58 -07:00 committed by GitHub
parent 1a64268bb0
commit 7087c0508e
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 51 additions and 21 deletions

View File

@ -60,11 +60,11 @@ export const ToolConfirmationMessage: React.FC<
question = `Apply this change?`;
options.push(
{
label: 'Yes',
label: 'Yes, allow once',
value: ToolConfirmationOutcome.ProceedOnce,
},
{
label: 'Yes (always allow)',
label: 'Yes, allow always', // TODO: this is extreme w/o being qualified by file or directory
value: ToolConfirmationOutcome.ProceedAlways,
},
{ label: 'No (esc)', value: ToolConfirmationOutcome.Cancel },
@ -73,29 +73,22 @@ export const ToolConfirmationMessage: React.FC<
const executionProps =
confirmationDetails as ToolExecuteConfirmationDetails;
// For execution, we still need context display and description
const commandDisplay = (
<Text color={Colors.AccentCyan}>{executionProps.command}</Text>
);
// Combine command and description into bodyContent for layout consistency
bodyContent = (
<Box flexDirection="column">
<Box paddingX={1} marginLeft={1}>
{commandDisplay}
<Text color={Colors.AccentCyan}>{executionProps.command}</Text>
</Box>
</Box>
);
question = `Allow execution?`;
const alwaysLabel = `Yes (always allow '${executionProps.rootCommand}' commands)`;
options.push(
{
label: 'Yes',
label: 'Yes, allow once',
value: ToolConfirmationOutcome.ProceedOnce,
},
{
label: alwaysLabel,
label: `Yes, allow always for ${executionProps.rootCommand} ...`,
value: ToolConfirmationOutcome.ProceedAlways,
},
{ label: 'No (esc)', value: ToolConfirmationOutcome.Cancel },

View File

@ -147,7 +147,7 @@ function createToolRegistry(config: Config): ToolRegistry {
// use ShellTool (next-gen TerminalTool) if environment variable is set
if (process.env.SHELL_TOOL) {
tools.push(new ShellTool(targetDir, config));
tools.push(new ShellTool(config));
} else {
tools.push(new TerminalTool(targetDir, config));
}

View File

@ -4,10 +4,15 @@
* SPDX-License-Identifier: Apache-2.0
*/
import path from 'path';
import fs from 'fs';
import { Config } from '../config/config.js';
import { BaseTool, ToolResult } from './tools.js';
import {
BaseTool,
ToolResult,
ToolCallConfirmationDetails,
ToolExecuteConfirmationDetails,
ToolConfirmationOutcome,
} from './tools.js';
import toolParameterSchema from './shell.json' with { type: 'json' };
export interface ShellToolParams {
@ -17,10 +22,11 @@ export interface ShellToolParams {
export class ShellTool extends BaseTool<ShellToolParams, ToolResult> {
static Name: string = 'execute_bash_command';
private readonly rootDirectory: string;
private readonly config: Config;
private cwd: string;
private whitelist: Set<string> = new Set();
constructor(rootDirectory: string, config: Config) {
constructor(config: Config) {
const toolDisplayName = 'Shell';
const descriptionUrl = new URL('shell.md', import.meta.url);
const toolDescription = fs.readFileSync(descriptionUrl, 'utf-8');
@ -31,7 +37,41 @@ export class ShellTool extends BaseTool<ShellToolParams, ToolResult> {
toolParameterSchema,
);
this.config = config;
this.rootDirectory = path.resolve(rootDirectory);
this.cwd = config.getTargetDir();
}
getDescription(params: ShellToolParams): string {
return params.description || `Execute \`${params.command}\` in ${this.cwd}`;
}
validateToolParams(_params: ShellToolParams): string | null {
// TODO: validate the command here
return null;
}
async shouldConfirmExecute(
params: ShellToolParams,
): Promise<ToolCallConfirmationDetails | false> {
const rootCommand =
params.command
.trim()
.split(/[\s;&&|]+/)[0]
?.split(/[/\\]/)
.pop() || 'unknown';
if (this.whitelist.has(rootCommand)) {
return false;
}
const confirmationDetails: ToolExecuteConfirmationDetails = {
title: 'Confirm Shell Command',
command: params.command,
rootCommand,
onConfirm: async (outcome: ToolConfirmationOutcome) => {
if (outcome === ToolConfirmationOutcome.ProceedAlways) {
this.whitelist.add(rootCommand);
}
},
};
return confirmationDetails;
}
async execute(_params: ShellToolParams): Promise<ToolResult> {

View File

@ -253,12 +253,10 @@ Use this tool for running build steps (\`npm install\`, \`make\`), linters (\`es
if (this.shouldAlwaysExecuteCommands.get(rootCommand)) {
return false;
}
const description = this.getDescription(params);
const confirmationDetails: ToolExecuteConfirmationDetails = {
title: 'Confirm Shell Command',
command: params.command,
rootCommand,
description: `Execute in '${this.currentCwd}':\n${description}`,
onConfirm: async (outcome: ToolConfirmationOutcome) => {
if (outcome === ToolConfirmationOutcome.ProceedAlways) {
this.shouldAlwaysExecuteCommands.set(rootCommand, true);

View File

@ -181,7 +181,6 @@ export interface ToolExecuteConfirmationDetails
extends ToolCallConfirmationDetails {
command: string;
rootCommand: string;
description: string;
}
export enum ToolConfirmationOutcome {