Enhance preToolUse hook and clean up code (microsoft#293265)

* Flesh out preToolUse hook

* Cleanup

* cleanups

* Cleanup

Author: roblourens

src/vs/workbench/api/common/extHostLanguageModelTools.ts

@@ -290,8 +290,12 @@ export class ExtHostLanguageModelTools implements ExtHostLanguageModelToolsShape
 			chatRequestId: context.chatRequestId,
 			chatSessionId: context.chatSessionId,
 			chatSessionResource: context.chatSessionResource,
-			chatInteractionId: context.chatInteractionId
+			chatInteractionId: context.chatInteractionId,
+			forceConfirmationReason: context.forceConfirmationReason
 		};
+		if (context.forceConfirmationReason) {
+			checkProposedApiEnabled(item.extension, 'chatParticipantPrivate');
+		}
 		if (item.tool.prepareInvocation) {
 			const result = await item.tool.prepareInvocation(options, token);
 			if (!result) {
@@ -309,7 +313,7 @@ export class ExtHostLanguageModelTools implements ExtHostLanguageModelToolsShape
 				} : undefined,
 				invocationMessage: typeConvert.MarkdownString.fromStrict(result.invocationMessage),
 				pastTenseMessage: typeConvert.MarkdownString.fromStrict(result.pastTenseMessage),
-				presentation: result.presentation as ToolInvocationPresentation | undefined
+				presentation: result.presentation as ToolInvocationPresentation | undefined,
 			};
 		}
 

src/vs/workbench/contrib/chat/browser/tools/languageModelToolsService.ts

@@ -34,7 +34,7 @@ import { Registry } from '../../../../../platform/registry/common/platform.js';
 import { IStorageService, StorageScope, StorageTarget } from '../../../../../platform/storage/common/storage.js';
 import { ITelemetryService } from '../../../../../platform/telemetry/common/telemetry.js';
 import { IExtensionService } from '../../../../services/extensions/common/extensions.js';
-import { IPreToolUseCallerInput } from '../../common/hooks/hooksTypes.js';
+import { IPreToolUseCallerInput, IPreToolUseHookResult } from '../../common/hooks/hooksTypes.js';
 import { IHooksExecutionService } from '../../common/hooks/hooksExecutionService.js';
 import { ChatContextKeys } from '../../common/actions/chatContextKeys.js';
 import { ChatRequestToolReferenceEntry, toToolSetVariableEntry, toToolVariableEntry } from '../../common/attachments/chatVariableEntries.js';
@@ -363,19 +363,21 @@ export class LanguageModelToolsService extends Disposable implements ILanguageMo
 
 	/**
 	 * Execute the preToolUse hook and handle denial.
-	 * Returns a tool result if the hook denied execution, or undefined to continue.
+	 * Returns an object containing:
+	 * - denialResult: A tool result if the hook denied execution (caller should return early)
+	 * - hookResult: The full hook result for use in auto-approval logic (allow/ask decisions)
 	 * @param pendingInvocation If there's an existing streaming invocation from beginToolCall, pass it here to cancel it instead of creating a new one.
 	 */
-	private async _executePreToolUseHookAndHandleDenial(
+	private async _executePreToolUseHook(
 		dto: IToolInvocation,
 		toolData: IToolData | undefined,
 		request: IChatRequestModel | undefined,
 		pendingInvocation: ChatToolInvocation | undefined,
 		token: CancellationToken
-	): Promise<IToolResult | undefined> {
+	): Promise<{ denialResult?: IToolResult; hookResult?: IPreToolUseHookResult }> {
 		// Skip hook if no session context or tool doesn't exist
 		if (!dto.context?.sessionResource || !toolData) {
-			return undefined;
+			return {};
 		}
 
 		const hookInput: IPreToolUseCallerInput = {
@@ -409,12 +411,15 @@ export class LanguageModelToolsService extends Disposable implements ILanguageMo
 
 			const denialMessage = localize('toolExecutionDenied', "Tool execution denied: {0}", hookReason);
 			return {
-				content: [{ kind: 'text', value: denialMessage }],
-				toolResultError: hookReason,
+				denialResult: {
+					content: [{ kind: 'text', value: denialMessage }],
+					toolResultError: hookReason,
+				},
+				hookResult,
 			};
 		}
 
-		return undefined;
+		return { hookResult };
 	}
 
 	async invokeTool(dto: IToolInvocation, countTokens: CountTokensCallback, token: CancellationToken): Promise<IToolResult> {
@@ -465,7 +470,7 @@ export class LanguageModelToolsService extends Disposable implements ILanguageMo
 		}
 
 		// Execute preToolUse hook - returns early if hook denies execution
-		const hookDenialResult = await this._executePreToolUseHookAndHandleDenial(dto, toolData, request, toolInvocation, token);
+		const { denialResult: hookDenialResult, hookResult: preToolUseHookResult } = await this._executePreToolUseHook(dto, toolData, request, toolInvocation, token);
 		if (hookDenialResult) {
 			// Clean up pending tool call if it exists
 			if (pendingToolCallKey) {
@@ -522,10 +527,11 @@ export class LanguageModelToolsService extends Disposable implements ILanguageMo
 				dto.userSelectedTools = request.userSelectedTools && { ...request.userSelectedTools };
 
 				prepareTimeWatch = StopWatch.create(true);
-				preparedInvocation = await this.prepareToolInvocation(tool, dto, token);
+				preparedInvocation = await this.prepareToolInvocationWithHookResult(tool, dto, preToolUseHookResult, token);
 				prepareTimeWatch.stop();
 
-				const autoConfirmed = await this.shouldAutoConfirm(tool.data.id, tool.data.runsInWorkspace, tool.data.source, dto.parameters, dto.context?.sessionResource);
+				const { autoConfirmed, preparedInvocation: updatedPreparedInvocation } = await this.resolveAutoConfirmFromHook(preToolUseHookResult, tool, dto, preparedInvocation, dto.context?.sessionResource);
+				preparedInvocation = updatedPreparedInvocation;
 
 
 				// Important: a tool invocation that will be autoconfirmed should never
@@ -570,9 +576,12 @@ export class LanguageModelToolsService extends Disposable implements ILanguageMo
 				}
 			} else {
 				prepareTimeWatch = StopWatch.create(true);
-				preparedInvocation = await this.prepareToolInvocation(tool, dto, token);
+				preparedInvocation = await this.prepareToolInvocationWithHookResult(tool, dto, preToolUseHookResult, token);
 				prepareTimeWatch.stop();
-				if (preparedInvocation?.confirmationMessages?.title && !(await this.shouldAutoConfirm(tool.data.id, tool.data.runsInWorkspace, tool.data.source, dto.parameters, undefined))) {
+
+				const { autoConfirmed: fallbackAutoConfirmed, preparedInvocation: updatedPreparedInvocation } = await this.resolveAutoConfirmFromHook(preToolUseHookResult, tool, dto, preparedInvocation, undefined);
+				preparedInvocation = updatedPreparedInvocation;
+				if (preparedInvocation?.confirmationMessages?.title && !fallbackAutoConfirmed) {
 					const result = await this._dialogService.confirm({ message: renderAsPlaintext(preparedInvocation.confirmationMessages.title), detail: renderAsPlaintext(preparedInvocation.confirmationMessages.message!) });
 					if (!result.confirmed) {
 						throw new CancellationError();
@@ -656,15 +665,77 @@ export class LanguageModelToolsService extends Disposable implements ILanguageMo
 		}
 	}
 
-	private async prepareToolInvocation(tool: IToolEntry, dto: IToolInvocation, token: CancellationToken): Promise<IPreparedToolInvocation | undefined> {
+	private async prepareToolInvocationWithHookResult(tool: IToolEntry, dto: IToolInvocation, hookResult: IPreToolUseHookResult | undefined, token: CancellationToken): Promise<IPreparedToolInvocation | undefined> {
+		let forceConfirmationReason: string | undefined;
+		if (hookResult?.permissionDecision === 'ask') {
+			const hookMessage = localize('preToolUseHookRequiredConfirmation', "{0} required confirmation", HookType.PreToolUse);
+			forceConfirmationReason = hookResult.permissionDecisionReason
+				? `${hookMessage}: ${hookResult.permissionDecisionReason}`
+				: hookMessage;
+		}
+		return this.prepareToolInvocation(tool, dto, forceConfirmationReason, token);
+	}
+
+	/**
+	 * Determines the auto-confirm decision based on a preToolUse hook result.
+	 * If the hook returned 'allow', auto-approves. If 'ask', forces confirmation
+	 * and ensures confirmation messages exist on `preparedInvocation`. Otherwise
+	 * falls back to normal auto-confirm logic.
+	 *
+	 * Returns the possibly-updated preparedInvocation along with the auto-confirm decision,
+	 * since when the hook returns 'ask' and preparedInvocation was undefined, we create one.
+	 */
+	private async resolveAutoConfirmFromHook(
+		hookResult: IPreToolUseHookResult | undefined,
+		tool: IToolEntry,
+		dto: IToolInvocation,
+		preparedInvocation: IPreparedToolInvocation | undefined,
+		sessionResource: URI | undefined,
+	): Promise<{ autoConfirmed: ConfirmedReason | undefined; preparedInvocation: IPreparedToolInvocation | undefined }> {
+		if (hookResult?.permissionDecision === 'allow') {
+			this._logService.debug(`[LanguageModelToolsService#invokeTool] Tool ${dto.toolId} auto-approved by preToolUse hook`);
+			return { autoConfirmed: { type: ToolConfirmKind.ConfirmationNotNeeded, reason: localize('hookAllowed', "Allowed by hook") }, preparedInvocation };
+		}
+
+		if (hookResult?.permissionDecision === 'ask') {
+			this._logService.debug(`[LanguageModelToolsService#invokeTool] Tool ${dto.toolId} requires confirmation (preToolUse hook returned 'ask')`);
+			// Ensure confirmation messages exist when hook requires confirmation
+			if (!preparedInvocation?.confirmationMessages?.title) {
+				if (!preparedInvocation) {
+					preparedInvocation = {};
+				}
+				const fullReferenceName = getToolFullReferenceName(tool.data);
+				const hookReason = hookResult.permissionDecisionReason;
+				const baseMessage = localize('hookRequiresConfirmation.message', "{0} hook confirmation required", HookType.PreToolUse);
+				preparedInvocation.confirmationMessages = {
+					...preparedInvocation.confirmationMessages,
+					title: localize('hookRequiresConfirmation.title', "Use the '{0}' tool?", fullReferenceName),
+					message: new MarkdownString(hookReason ? `${baseMessage}\n\n${hookReason}` : baseMessage),
+					allowAutoConfirm: false,
+				};
+				preparedInvocation.toolSpecificData = {
+					kind: 'input',
+					rawInput: dto.parameters,
+				};
+			}
+			return { autoConfirmed: undefined, preparedInvocation };
+		}
+
+		// No hook decision - use normal auto-confirm logic
+		const autoConfirmed = await this.shouldAutoConfirm(tool.data.id, tool.data.runsInWorkspace, tool.data.source, dto.parameters, sessionResource);
+		return { autoConfirmed, preparedInvocation };
+	}
+
+	private async prepareToolInvocation(tool: IToolEntry, dto: IToolInvocation, forceConfirmationReason: string | undefined, token: CancellationToken): Promise<IPreparedToolInvocation | undefined> {
 		let prepared: IPreparedToolInvocation | undefined;
 		if (tool.impl!.prepareToolInvocation) {
 			const preparePromise = tool.impl!.prepareToolInvocation({
 				parameters: dto.parameters,
 				chatRequestId: dto.chatRequestId,
 				chatSessionId: dto.context?.sessionId,
 				chatSessionResource: dto.context?.sessionResource,
-				chatInteractionId: dto.chatInteractionId
+				chatInteractionId: dto.chatInteractionId,
+				forceConfirmationReason: forceConfirmationReason
 			}, token);
 
 			const raceResult = await Promise.race([

src/vs/workbench/contrib/chat/common/hooks/hooksExecutionService.ts

@@ -3,28 +3,28 @@
  *  Licensed under the MIT License. See License.txt in the project root for license information.
  *--------------------------------------------------------------------------------------------*/
 
-import { createDecorator } from '../../../../../platform/instantiation/common/instantiation.js';
-import { URI } from '../../../../../base/common/uri.js';
-import { HookType, HookTypeValue, IChatRequestHooks, IHookCommand } from '../promptSyntax/hookSchema.js';
-import { IDisposable, toDisposable } from '../../../../../base/common/lifecycle.js';
-import { ILogService } from '../../../../../platform/log/common/log.js';
 import { CancellationToken } from '../../../../../base/common/cancellation.js';
+import { IDisposable, toDisposable } from '../../../../../base/common/lifecycle.js';
 import { StopWatch } from '../../../../../base/common/stopwatch.js';
-import { Extensions, IOutputChannelRegistry, IOutputService } from '../../../../services/output/common/output.js';
-import { Registry } from '../../../../../platform/registry/common/platform.js';
+import { URI } from '../../../../../base/common/uri.js';
 import { localize } from '../../../../../nls.js';
+import { createDecorator } from '../../../../../platform/instantiation/common/instantiation.js';
+import { ILogService } from '../../../../../platform/log/common/log.js';
+import { Registry } from '../../../../../platform/registry/common/platform.js';
+import { Extensions, IOutputChannelRegistry, IOutputService } from '../../../../services/output/common/output.js';
+import { HookType, HookTypeValue, IChatRequestHooks, IHookCommand } from '../promptSyntax/hookSchema.js';
 import {
 	HookCommandResultKind,
 	IHookCommandInput,
 	IHookCommandResult,
-	IPreToolUseCommandInput,
+	IPreToolUseCommandInput
 } from './hooksCommandTypes.js';
 import {
 	commonHookOutputValidator,
 	IHookResult,
 	IPreToolUseCallerInput,
 	IPreToolUseHookResult,
-	preToolUseOutputValidator,
+	preToolUseOutputValidator
 } from './hooksTypes.js';
 
 export const hooksOutputChannelId = 'hooksExecution';
@@ -296,7 +296,8 @@ export class HooksExecutionService implements IHooksExecutionService {
 			token: token ?? CancellationToken.None,
 		});
 
-		// Collect all valid outputs - "any deny wins" for security
+		// Collect all valid outputs - priority order: deny > ask > allow
+		let lastAskResult: IPreToolUseHookResult | undefined;
 		let lastAllowResult: IPreToolUseHookResult | undefined;
 		for (const result of results) {
 			if (result.success && typeof result.output === 'object' && result.output !== null) {
@@ -305,6 +306,12 @@ export class HooksExecutionService implements IHooksExecutionService {
 					// Extract from hookSpecificOutput wrapper
 					const hookSpecificOutput = validationResult.content.hookSpecificOutput;
 					if (hookSpecificOutput) {
+						// Validate hookEventName if present - must match the hook type
+						if (hookSpecificOutput.hookEventName !== undefined && hookSpecificOutput.hookEventName !== HookType.PreToolUse) {
+							this._logService.warn(`[HooksExecutionService] preToolUse hook returned invalid hookEventName '${hookSpecificOutput.hookEventName}', expected '${HookType.PreToolUse}'`);
+							continue;
+						}
+
 						const preToolUseResult: IPreToolUseHookResult = {
 							...result,
 							permissionDecision: hookSpecificOutput.permissionDecision,
@@ -316,6 +323,10 @@ export class HooksExecutionService implements IHooksExecutionService {
 						if (hookSpecificOutput.permissionDecision === 'deny') {
 							return preToolUseResult;
 						}
+						// Track 'ask' results (ask takes priority over allow)
+						if (hookSpecificOutput.permissionDecision === 'ask') {
+							lastAskResult = preToolUseResult;
+						}
 						// Track the last allow in case we need to return it
 						if (hookSpecificOutput.permissionDecision === 'allow') {
 							lastAllowResult = preToolUseResult;
@@ -328,7 +339,7 @@ export class HooksExecutionService implements IHooksExecutionService {
 			}
 		}
 
-		// Return the last allow result, or undefined if no valid outputs
-		return lastAllowResult;
+		// Return with priority: ask > allow > undefined
+		return lastAskResult ?? lastAllowResult;
 	}
 }

src/vs/workbench/contrib/chat/common/hooks/hooksTypes.ts

@@ -67,16 +67,19 @@ export interface IPreToolUseCallerInput {
 export const preToolUseOutputValidator = vObj({
 	hookSpecificOutput: vOptionalProp(vObj({
 		hookEventName: vOptionalProp(vString()),
-		permissionDecision: vEnum('allow', 'deny'),
+		permissionDecision: vEnum('allow', 'deny', 'ask'),
 		permissionDecisionReason: vOptionalProp(vString()),
 		additionalContext: vOptionalProp(vString()),
 	})),
 });
 
 /**
  * Valid permission decisions for preToolUse hooks.
+ * - 'allow': Auto-approve the tool execution (skip user confirmation)
+ * - 'deny': Deny the tool execution
+ * - 'ask': Always require user confirmation (never auto-approve)
  */
-export type PreToolUsePermissionDecision = 'allow' | 'deny';
+export type PreToolUsePermissionDecision = 'allow' | 'deny' | 'ask';
 
 /**
  * Result from preToolUse hooks with permission decision fields.

src/vs/workbench/contrib/chat/common/tools/languageModelToolsService.ts

@@ -203,6 +203,8 @@ export interface IToolInvocationPreparationContext {
 	chatSessionId?: string;
 	chatSessionResource: URI | undefined;
 	chatInteractionId?: string;
+	/** If set, tells the tool that it should include confirmmation messages. */
+	forceConfirmationReason?: string;
 }
 
 export type ToolInputOutputBase = {