From e5dd64d2167d9d020d6b68ff67a2e5032aededbf Mon Sep 17 00:00:00 2001 From: Rob Lourens Date: Wed, 4 Feb 2026 15:10:04 -0800 Subject: [PATCH 1/2] Add common output hook types, reorganize a bit more --- .../common/extensionsApiProposals.ts | 2 +- .../workbench/api/browser/mainThreadHooks.ts | 6 +- .../workbench/api/common/extHost.api.impl.ts | 1 - .../workbench/api/common/extHost.protocol.ts | 4 +- .../api/common/extHostTypeConverters.ts | 10 +- src/vs/workbench/api/common/extHostTypes.ts | 9 - src/vs/workbench/api/node/extHostHooksNode.ts | 17 +- .../api/test/node/extHostHooks.test.ts | 18 +- .../tools/languageModelToolsService.ts | 3 +- .../chat/common/hooksExecutionService.ts | 183 +++++++++++++++--- .../tools/languageModelToolsService.test.ts | 20 +- .../test/common/hooksExecutionService.test.ts | 106 ++++++++-- src/vscode-dts/vscode.proposed.chatHooks.d.ts | 32 ++- 13 files changed, 307 insertions(+), 104 deletions(-) diff --git a/src/vs/platform/extensions/common/extensionsApiProposals.ts b/src/vs/platform/extensions/common/extensionsApiProposals.ts index 8b89e5b53a8ba..ab2b5407d60cf 100644 --- a/src/vs/platform/extensions/common/extensionsApiProposals.ts +++ b/src/vs/platform/extensions/common/extensionsApiProposals.ts @@ -45,7 +45,7 @@ const _allApiProposals = { }, chatHooks: { proposal: 'https://raw.githubusercontent.com/microsoft/vscode/main/src/vscode-dts/vscode.proposed.chatHooks.d.ts', - version: 1 + version: 2 }, chatOutputRenderer: { proposal: 'https://raw.githubusercontent.com/microsoft/vscode/main/src/vscode-dts/vscode.proposed.chatOutputRenderer.d.ts', diff --git a/src/vs/workbench/api/browser/mainThreadHooks.ts b/src/vs/workbench/api/browser/mainThreadHooks.ts index 0283383ac65e5..65e39b2fea493 100644 --- a/src/vs/workbench/api/browser/mainThreadHooks.ts +++ b/src/vs/workbench/api/browser/mainThreadHooks.ts @@ -7,7 +7,7 @@ import { URI, UriComponents } from '../../../base/common/uri.js'; import { Disposable } from '../../../base/common/lifecycle.js'; import { extHostNamedCustomer, IExtHostContext } from '../../services/extensions/common/extHostCustomers.js'; import { ExtHostContext, MainContext, MainThreadHooksShape } from '../common/extHost.protocol.js'; -import { HookResultKind, IHookResult, IHooksExecutionProxy, IHooksExecutionService } from '../../contrib/chat/common/hooksExecutionService.js'; +import { HookCommandResultKind, IHookCommandResult, IHookResult, IHooksExecutionProxy, IHooksExecutionService } from '../../contrib/chat/common/hooksExecutionService.js'; import { HookTypeValue, IHookCommand } from '../../contrib/chat/common/promptSyntax/hookSchema.js'; import { CancellationToken } from '../../../base/common/cancellation.js'; @@ -22,10 +22,10 @@ export class MainThreadHooks extends Disposable implements MainThreadHooksShape const extHostProxy = extHostContext.getProxy(ExtHostContext.ExtHostHooks); const proxy: IHooksExecutionProxy = { - runHookCommand: async (hookCommand: IHookCommand, input: unknown, token: CancellationToken): Promise => { + runHookCommand: async (hookCommand: IHookCommand, input: unknown, token: CancellationToken): Promise => { const result = await extHostProxy.$runHookCommand(hookCommand, input, token); return { - kind: result.kind as HookResultKind, + kind: result.kind as HookCommandResultKind, result: result.result }; } diff --git a/src/vs/workbench/api/common/extHost.api.impl.ts b/src/vs/workbench/api/common/extHost.api.impl.ts index 04e055c5b92c7..a193a61de27a9 100644 --- a/src/vs/workbench/api/common/extHost.api.impl.ts +++ b/src/vs/workbench/api/common/extHost.api.impl.ts @@ -2020,7 +2020,6 @@ export function createApiFactoryAndRegisterActors(accessor: ServicesAccessor): I McpToolAvailability: extHostTypes.McpToolAvailability, McpToolInvocationContentData: extHostTypes.McpToolInvocationContentData, SettingsSearchResultKind: extHostTypes.SettingsSearchResultKind, - ChatHookResultKind: extHostTypes.ChatHookResultKind, ChatTodoStatus: extHostTypes.ChatTodoStatus, }; }; diff --git a/src/vs/workbench/api/common/extHost.protocol.ts b/src/vs/workbench/api/common/extHost.protocol.ts index cb91ae6e736fe..3b5f26b330093 100644 --- a/src/vs/workbench/api/common/extHost.protocol.ts +++ b/src/vs/workbench/api/common/extHost.protocol.ts @@ -99,7 +99,7 @@ import { IExtHostDocumentSaveDelegate } from './extHostDocumentData.js'; import { TerminalShellExecutionCommandLineConfidence } from './extHostTypes.js'; import * as tasks from './shared/tasks.js'; import { PromptsType } from '../../contrib/chat/common/promptSyntax/promptTypes.js'; -import { IHookResult } from '../../contrib/chat/common/hooksExecutionService.js'; +import { IHookCommandResult, IHookResult } from '../../contrib/chat/common/hooksExecutionService.js'; import { IHookCommand } from '../../contrib/chat/common/promptSyntax/hookSchema.js'; export type IconPathDto = @@ -3202,7 +3202,7 @@ export interface IStartMcpOptions { export type IHookCommandDto = Dto; export interface ExtHostHooksShape { - $runHookCommand(hookCommand: IHookCommandDto, input: unknown, token: CancellationToken): Promise; + $runHookCommand(hookCommand: IHookCommandDto, input: unknown, token: CancellationToken): Promise; } export interface ExtHostMcpShape { diff --git a/src/vs/workbench/api/common/extHostTypeConverters.ts b/src/vs/workbench/api/common/extHostTypeConverters.ts index ba3b0487d381e..cd033f7c1c7c6 100644 --- a/src/vs/workbench/api/common/extHostTypeConverters.ts +++ b/src/vs/workbench/api/common/extHostTypeConverters.ts @@ -45,7 +45,7 @@ import { IChatAgentMarkdownContentWithVulnerability, IChatCodeCitation, IChatCom import { LocalChatSessionUri } from '../../contrib/chat/common/model/chatUri.js'; import { ChatRequestToolReferenceEntry, IChatRequestVariableEntry, isImageVariableEntry, isPromptFileVariableEntry, isPromptTextVariableEntry } from '../../contrib/chat/common/attachments/chatVariableEntries.js'; import { ChatAgentLocation } from '../../contrib/chat/common/constants.js'; -import { HookResultKind, IHookResult } from '../../contrib/chat/common/hooksExecutionService.js'; +import { IHookResult } from '../../contrib/chat/common/hooksExecutionService.js'; import { IToolInvocationContext, IToolResult, IToolResultInputOutputDetails, IToolResultOutputDetails, ToolDataSource, ToolInvocationPresentation } from '../../contrib/chat/common/tools/languageModelToolsService.js'; import * as chatProvider from '../../contrib/chat/common/languageModels.js'; import { IChatMessageDataPart, IChatResponseDataPart, IChatResponsePromptTsxPart, IChatResponseTextPart } from '../../contrib/chat/common/languageModels.js'; @@ -4016,10 +4016,10 @@ export namespace SourceControlInputBoxValidationType { export namespace ChatHookResult { export function to(result: IHookResult): vscode.ChatHookResult { return { - kind: result.kind === HookResultKind.Success - ? types.ChatHookResultKind.Success - : types.ChatHookResultKind.Error, - result: result.result + stopReason: result.stopReason, + messageForUser: result.messageForUser, + output: result.output, + success: result.success, }; } } diff --git a/src/vs/workbench/api/common/extHostTypes.ts b/src/vs/workbench/api/common/extHostTypes.ts index d927b08e96e98..a0a9ed3a0a5a1 100644 --- a/src/vs/workbench/api/common/extHostTypes.ts +++ b/src/vs/workbench/api/common/extHostTypes.ts @@ -3935,15 +3935,6 @@ export enum SettingsSearchResultKind { //#endregion -//#region Chat Hooks - -export enum ChatHookResultKind { - Success = 1, - Error = 2 -} - -//#endregion - //#region Speech export enum SpeechToTextStatus { diff --git a/src/vs/workbench/api/node/extHostHooksNode.ts b/src/vs/workbench/api/node/extHostHooksNode.ts index db4d738600af2..63d44c79fb3b0 100644 --- a/src/vs/workbench/api/node/extHostHooksNode.ts +++ b/src/vs/workbench/api/node/extHostHooksNode.ts @@ -16,7 +16,7 @@ import { isToolInvocationContext, IToolInvocationContext } from '../../contrib/c import { IHookCommandDto, MainContext, MainThreadHooksShape } from '../common/extHost.protocol.js'; import { IChatHookExecutionOptions, IExtHostHooks } from '../common/extHostHooks.js'; import { IExtHostRpcService } from '../common/extHostRpcService.js'; -import { HookResultKind, IHookResult } from '../../contrib/chat/common/hooksExecutionService.js'; +import { HookCommandResultKind, IHookCommandResult, IHookResult } from '../../contrib/chat/common/hooksExecutionService.js'; import * as typeConverters from '../common/extHostTypeConverters.js'; const SIGKILL_DELAY_MS = 5000; @@ -40,26 +40,23 @@ export class NodeExtHostHooks implements IExtHostHooks { const context = options.toolInvocationToken as IToolInvocationContext; const results = await this._mainThreadProxy.$executeHook(hookType, context.sessionResource, options.input, token ?? CancellationToken.None); - return results.map(r => typeConverters.ChatHookResult.to({ - kind: r.kind as HookResultKind, - result: r.result - })); + return results.map(r => typeConverters.ChatHookResult.to(r as IHookResult)); } - async $runHookCommand(hookCommand: IHookCommandDto, input: unknown, token: CancellationToken): Promise { + async $runHookCommand(hookCommand: IHookCommandDto, input: unknown, token: CancellationToken): Promise { this._logService.debug(`[ExtHostHooks] Running hook command: ${JSON.stringify(hookCommand)}`); try { return await this._executeCommand(hookCommand, input, token); } catch (err) { return { - kind: HookResultKind.Error, + kind: HookCommandResultKind.Error, result: err instanceof Error ? err.message : String(err) }; } } - private _executeCommand(hook: IHookCommandDto, input: unknown, token?: CancellationToken): Promise { + private _executeCommand(hook: IHookCommandDto, input: unknown, token?: CancellationToken): Promise { const home = homedir(); const cwdUri = hook.cwd ? URI.revive(hook.cwd) : undefined; const cwd = cwdUri ? cwdUri.fsPath : home; @@ -157,10 +154,10 @@ export class NodeExtHostHooks implements IExtHostHooks { } catch { // Keep as string if not valid JSON } - resolve({ kind: HookResultKind.Success, result }); + resolve({ kind: HookCommandResultKind.Success, result }); } else { // Error - resolve({ kind: HookResultKind.Error, result: stderrStr }); + resolve({ kind: HookCommandResultKind.Error, result: stderrStr }); } }); diff --git a/src/vs/workbench/api/test/node/extHostHooks.test.ts b/src/vs/workbench/api/test/node/extHostHooks.test.ts index 0ed5021c0795a..da9e090e9b7ba 100644 --- a/src/vs/workbench/api/test/node/extHostHooks.test.ts +++ b/src/vs/workbench/api/test/node/extHostHooks.test.ts @@ -9,7 +9,7 @@ import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../base/test/c import { NullLogService } from '../../../../platform/log/common/log.js'; import { NodeExtHostHooks } from '../../node/extHostHooksNode.js'; import { IHookCommandDto, MainThreadHooksShape } from '../../common/extHost.protocol.js'; -import { IHookResult, HookResultKind } from '../../../contrib/chat/common/hooksExecutionService.js'; +import { HookCommandResultKind, IHookResult } from '../../../contrib/chat/common/hooksExecutionService.js'; import { IExtHostRpcService } from '../../common/extHostRpcService.js'; import { CancellationToken } from '../../../../base/common/cancellation.js'; @@ -57,7 +57,7 @@ suite.skip('ExtHostHooks', () => { const hookCommand = createHookCommandDto('echo "hello world"'); const result = await hooksService.$runHookCommand(hookCommand, undefined, CancellationToken.None); - assert.strictEqual(result.kind, HookResultKind.Success); + assert.strictEqual(result.kind, HookCommandResultKind.Success); assert.strictEqual((result.result as string).trim(), 'hello world'); }); @@ -65,7 +65,7 @@ suite.skip('ExtHostHooks', () => { const hookCommand = createHookCommandDto('echo \'{"key": "value"}\''); const result = await hooksService.$runHookCommand(hookCommand, undefined, CancellationToken.None); - assert.strictEqual(result.kind, HookResultKind.Success); + assert.strictEqual(result.kind, HookCommandResultKind.Success); assert.deepStrictEqual(result.result, { key: 'value' }); }); @@ -73,14 +73,14 @@ suite.skip('ExtHostHooks', () => { const hookCommand = createHookCommandDto('exit 1'); const result = await hooksService.$runHookCommand(hookCommand, undefined, CancellationToken.None); - assert.strictEqual(result.kind, HookResultKind.Error); + assert.strictEqual(result.kind, HookCommandResultKind.Error); }); test('$runHookCommand captures stderr on failure', async () => { const hookCommand = createHookCommandDto('echo "error message" >&2 && exit 1'); const result = await hooksService.$runHookCommand(hookCommand, undefined, CancellationToken.None); - assert.strictEqual(result.kind, HookResultKind.Error); + assert.strictEqual(result.kind, HookCommandResultKind.Error); assert.strictEqual((result.result as string).trim(), 'error message'); }); @@ -89,7 +89,7 @@ suite.skip('ExtHostHooks', () => { const input = { tool: 'bash', args: { command: 'ls' } }; const result = await hooksService.$runHookCommand(hookCommand, input, CancellationToken.None); - assert.strictEqual(result.kind, HookResultKind.Success); + assert.strictEqual(result.kind, HookCommandResultKind.Success); assert.deepStrictEqual(result.result, input); }); @@ -97,14 +97,14 @@ suite.skip('ExtHostHooks', () => { const hookCommand = createHookCommandDto('/nonexistent/command/that/does/not/exist'); const result = await hooksService.$runHookCommand(hookCommand, undefined, CancellationToken.None); - assert.strictEqual(result.kind, HookResultKind.Error); + assert.strictEqual(result.kind, HookCommandResultKind.Error); }); test('$runHookCommand uses custom environment variables', async () => { const hookCommand = createHookCommandDto('echo $MY_VAR', { env: { MY_VAR: 'custom_value' } }); const result = await hooksService.$runHookCommand(hookCommand, undefined, CancellationToken.None); - assert.strictEqual(result.kind, HookResultKind.Success); + assert.strictEqual(result.kind, HookCommandResultKind.Success); assert.strictEqual((result.result as string).trim(), 'custom_value'); }); @@ -112,7 +112,7 @@ suite.skip('ExtHostHooks', () => { const hookCommand = createHookCommandDto('pwd', { cwd: URI.file('/tmp') }); const result = await hooksService.$runHookCommand(hookCommand, undefined, CancellationToken.None); - assert.strictEqual(result.kind, HookResultKind.Success); + assert.strictEqual(result.kind, HookCommandResultKind.Success); // The result should contain /tmp or /private/tmp (macOS symlink) assert.ok((result.result as string).includes('tmp')); }); diff --git a/src/vs/workbench/contrib/chat/browser/tools/languageModelToolsService.ts b/src/vs/workbench/contrib/chat/browser/tools/languageModelToolsService.ts index c221d7a54bd07..3ec56297eb715 100644 --- a/src/vs/workbench/contrib/chat/browser/tools/languageModelToolsService.ts +++ b/src/vs/workbench/contrib/chat/browser/tools/languageModelToolsService.ts @@ -48,6 +48,7 @@ import { CountTokensCallback, createToolSchemaUri, IBeginToolCallOptions, ILangu import { getToolConfirmationAlert } from '../accessibility/chatAccessibilityProvider.js'; import { URI } from '../../../../../base/common/uri.js'; import { chatSessionResourceToId } from '../../common/model/chatUri.js'; +import { HookType } from '../../common/promptSyntax/hookSchema.js'; const jsonSchemaRegistry = Registry.as(JSONContributionRegistry.Extensions.JSONContribution); @@ -384,7 +385,7 @@ export class LanguageModelToolsService extends Disposable implements ILanguageMo if (hookResult?.permissionDecision === 'deny') { const hookReason = hookResult.permissionDecisionReason ?? localize('hookDeniedNoReason', "Hook denied tool execution"); - const reason = localize('deniedByPreToolUseHook', "Denied by {0} hook: {1}", 'preToolUse', hookReason); + const reason = localize('deniedByPreToolUseHook', "Denied by {0} hook: {1}", HookType.PreToolUse, hookReason); this._logService.debug(`[LanguageModelToolsService#invokeTool] Tool ${dto.toolId} denied by preToolUse hook: ${hookReason}`); // Handle the tool invocation in cancelled state diff --git a/src/vs/workbench/contrib/chat/common/hooksExecutionService.ts b/src/vs/workbench/contrib/chat/common/hooksExecutionService.ts index 98e095d42b09d..d0e8a7effe201 100644 --- a/src/vs/workbench/contrib/chat/common/hooksExecutionService.ts +++ b/src/vs/workbench/contrib/chat/common/hooksExecutionService.ts @@ -33,6 +33,31 @@ export interface ICommonHookInput { //#endregion +//#region Common Hook Output + +/** + * Common output fields that can be present in any hook result. + * These fields control execution flow and user feedback. + */ +export interface ICommonHookOutput { + /** + * If set, stops processing entirely after this hook. + * The message is shown to the user but not to the agent. + */ + readonly stopReason?: string; + /** + * Message shown to the user. + */ + readonly systemMessage?: string; +} + +const commonHookOutputValidator = vObj({ + stopReason: vOptionalProp(vString()), + systemMessage: vOptionalProp(vString()), +}); + +//#endregion + //#region PreToolUse Hook Types /** @@ -73,16 +98,61 @@ const preToolUseOutputValidator = vObj({ //#endregion -export const enum HookResultKind { +export const enum HookCommandResultKind { Success = 1, Error = 2 } -export interface IHookResult { - readonly kind: HookResultKind; +//#region Hook Result Types + +/** + * Raw result from spawning a hook command. + * This is the low-level result before semantic processing. + */ +export interface IHookCommandResult { + readonly kind: HookCommandResultKind; + /** + * For success, this is stdout (parsed as JSON if valid, otherwise string). + * For errors, this is stderr. + */ readonly result: string | object; } +/** + * Semantic hook result with common fields extracted and defaults applied. + * This is what callers receive from executeHook. + */ +export interface IHookResult { + /** + * If set, the agent should stop processing entirely after this hook. + * The message is shown to the user but not to the agent. + */ + readonly stopReason?: string; + /** + * Message shown to the user. + */ + readonly messageForUser?: string; + /** + * The hook's output (hook-specific fields only). + * For errors, this is the error message string. + */ + readonly output: unknown; + /** + * Whether the hook command executed successfully (exit code 0). + */ + readonly success: boolean; +} + +/** + * Result from preToolUse hooks with permission decision fields. + */ +export interface IPreToolUseHookResult extends IHookResult { + readonly permissionDecision?: PreToolUsePermissionDecision; + readonly permissionDecisionReason?: string; +} + +//#endregion + export interface IHooksExecutionOptions { readonly input?: unknown; readonly token?: CancellationToken; @@ -93,7 +163,7 @@ export interface IHooksExecutionOptions { * MainThreadHooks implements this to forward calls to the extension host. */ export interface IHooksExecutionProxy { - runHookCommand(hookCommand: IHookCommand, input: unknown, token: CancellationToken): Promise; + runHookCommand(hookCommand: IHookCommand, input: unknown, token: CancellationToken): Promise; } export const IHooksExecutionService = createDecorator('hooksExecutionService'); @@ -124,9 +194,9 @@ export interface IHooksExecutionService { /** * Execute preToolUse hooks with typed input and validated output. * The execution service builds the full hook input from the caller input plus session context. - * Output is optional, but if provided, it must conform to the expected schema. + * Returns a combined result with common fields and permission decision. */ - executePreToolUseHook(sessionResource: URI, input: IPreToolUseCallerInput, token?: CancellationToken): Promise; + executePreToolUseHook(sessionResource: URI, input: IPreToolUseCallerInput, token?: CancellationToken): Promise; } export class HooksExecutionService implements IHooksExecutionService { @@ -198,18 +268,75 @@ export class HooksExecutionService implements IHooksExecutionService { const sw = StopWatch.create(); try { - const result = await this._proxy!.runHookCommand(hookCommand, fullInput, token); - this._logResult(requestId, hookType, result, Math.round(sw.elapsed())); + const commandResult = await this._proxy!.runHookCommand(hookCommand, fullInput, token); + const result = this._toInternalResult(commandResult); + this._logCommandResult(requestId, hookType, commandResult, Math.round(sw.elapsed())); return result; } catch (err) { const errMessage = err instanceof Error ? err.message : String(err); this._log(requestId, hookType, `Error in ${Math.round(sw.elapsed())}ms: ${errMessage}`); - return { kind: HookResultKind.Error, result: errMessage }; + return this._createErrorResult(errMessage); } } - private _logResult(requestId: number, hookType: HookTypeValue, result: IHookResult, elapsed: number): void { - const resultKindStr = result.kind === HookResultKind.Success ? 'Success' : 'Error'; + private _createErrorResult(errorMessage: string): IHookResult { + return { + output: errorMessage, + success: false, + }; + } + + private _toInternalResult(commandResult: IHookCommandResult): IHookResult { + if (commandResult.kind !== HookCommandResultKind.Success) { + return this._createErrorResult( + typeof commandResult.result === 'string' ? commandResult.result : JSON.stringify(commandResult.result) + ); + } + + // For string results, no common fields to extract + if (typeof commandResult.result !== 'object') { + return { + output: commandResult.result, + success: true, + }; + } + + // Extract and validate common fields + const validationResult = commonHookOutputValidator.validate(commandResult.result); + const commonFields = validationResult.error ? {} : validationResult.content; + + // Extract only known hook-specific fields for output + const resultObj = commandResult.result as Record; + const hookOutput = this._extractHookSpecificOutput(resultObj); + + return { + stopReason: commonFields.stopReason, + messageForUser: commonFields.systemMessage, + output: Object.keys(hookOutput).length > 0 ? hookOutput : undefined, + success: true, + }; + } + + /** + * Extract only known hook-specific output fields. + * This prevents unknown fields from being passed through. + */ + private _extractHookSpecificOutput(result: Record): Record { + const output: Record = {}; + + // PreToolUse hook fields + if (result.permissionDecision !== undefined) { + output.permissionDecision = result.permissionDecision; + } + if (result.permissionDecisionReason !== undefined) { + output.permissionDecisionReason = result.permissionDecisionReason; + } + + return output; + } + + private _logCommandResult(requestId: number, hookType: HookTypeValue, result: IHookCommandResult, elapsed: number): void { + const resultKindStr = result.kind === HookCommandResultKind.Success ? 'Success' : 'Error'; const resultStr = typeof result.result === 'string' ? result.result : JSON.stringify(result.result); const hasOutput = resultStr.length > 0 && resultStr !== '{}' && resultStr !== '[]'; if (hasOutput) { @@ -257,12 +384,18 @@ export class HooksExecutionService implements IHooksExecutionService { for (const hookCommand of hookCommands) { const result = await this._runSingleHook(requestId, hookType, hookCommand, sessionResource, options?.input, token); results.push(result); + + // If stopReason is set, stop processing remaining hooks + if (result.stopReason) { + this._log(requestId, hookType, `Stopping: ${result.stopReason}`); + break; + } } return results; } - async executePreToolUseHook(sessionResource: URI, input: IPreToolUseCallerInput, token?: CancellationToken): Promise { + async executePreToolUseHook(sessionResource: URI, input: IPreToolUseCallerInput, token?: CancellationToken): Promise { // Pass the caller input directly - common properties are added in _runSingleHook const results = await this.executeHook(HookType.PreToolUse, sessionResource, { input, @@ -270,19 +403,25 @@ export class HooksExecutionService implements IHooksExecutionService { }); // Collect all valid outputs - "any deny wins" for security - let lastAllowOutput: IPreToolUseHookOutput | undefined; + let lastAllowResult: IPreToolUseHookResult | undefined; for (const result of results) { - if (result.kind === HookResultKind.Success && typeof result.result === 'object') { - const validationResult = preToolUseOutputValidator.validate(result.result); + if (result.success && typeof result.output === 'object' && result.output !== null) { + const validationResult = preToolUseOutputValidator.validate(result.output); if (!validationResult.error) { - const output = validationResult.content; + const hookOutput = validationResult.content; + const preToolUseResult: IPreToolUseHookResult = { + ...result, + permissionDecision: hookOutput.permissionDecision, + permissionDecisionReason: hookOutput.permissionDecisionReason, + }; + // If any hook denies, return immediately with that denial - if (output.permissionDecision === 'deny') { - return output; + if (hookOutput.permissionDecision === 'deny') { + return preToolUseResult; } // Track the last allow in case we need to return it - if (output.permissionDecision === 'allow') { - lastAllowOutput = output; + if (hookOutput.permissionDecision === 'allow') { + lastAllowResult = preToolUseResult; } } else { // If validation fails, log a warning and continue to next result @@ -291,7 +430,7 @@ export class HooksExecutionService implements IHooksExecutionService { } } - // Return the last allow output, or undefined if no valid outputs - return lastAllowOutput; + // Return the last allow result, or undefined if no valid outputs + return lastAllowResult; } } diff --git a/src/vs/workbench/contrib/chat/test/browser/tools/languageModelToolsService.test.ts b/src/vs/workbench/contrib/chat/test/browser/tools/languageModelToolsService.test.ts index 407e1b76b6d04..d6353f0d48b27 100644 --- a/src/vs/workbench/contrib/chat/test/browser/tools/languageModelToolsService.test.ts +++ b/src/vs/workbench/contrib/chat/test/browser/tools/languageModelToolsService.test.ts @@ -32,7 +32,7 @@ import { ILanguageModelToolsConfirmationService } from '../../../common/tools/la import { MockLanguageModelToolsConfirmationService } from '../../common/tools/mockLanguageModelToolsConfirmationService.js'; import { runWithFakedTimers } from '../../../../../../base/test/common/timeTravelScheduler.js'; import { ILanguageModelChatMetadata } from '../../../common/languageModels.js'; -import { IHooksExecutionService, IPreToolUseCallerInput, IPreToolUseHookOutput, IHooksExecutionOptions, IHookResult, IHooksExecutionProxy } from '../../../common/hooksExecutionService.js'; +import { IHooksExecutionService, IPreToolUseCallerInput, IPreToolUseHookResult, IHooksExecutionOptions, IHookResult, IHooksExecutionProxy } from '../../../common/hooksExecutionService.js'; import { HookTypeValue, IChatRequestHooks } from '../../../common/promptSyntax/hookSchema.js'; import { IDisposable } from '../../../../../../base/common/lifecycle.js'; @@ -64,7 +64,7 @@ class TestTelemetryService implements Partial { class MockHooksExecutionService implements IHooksExecutionService { readonly _serviceBrand: undefined; - public preToolUseHookResult: IPreToolUseHookOutput | undefined = undefined; + public preToolUseHookResult: IPreToolUseHookResult | undefined = undefined; public lastPreToolUseInput: IPreToolUseCallerInput | undefined = undefined; setProxy(_proxy: IHooksExecutionProxy): void { } @@ -73,7 +73,7 @@ class MockHooksExecutionService implements IHooksExecutionService { executeHook(_hookType: HookTypeValue, _sessionResource: URI, _options?: IHooksExecutionOptions): Promise { return Promise.resolve([]); } - async executePreToolUseHook(_sessionResource: URI, input: IPreToolUseCallerInput, _token?: CancellationToken): Promise { + async executePreToolUseHook(_sessionResource: URI, input: IPreToolUseCallerInput, _token?: CancellationToken): Promise { this.lastPreToolUseInput = input; return this.preToolUseHookResult; } @@ -3732,6 +3732,9 @@ suite('LanguageModelToolsService', () => { test('when hook denies, tool returns error and creates cancelled invocation', async () => { mockHooksService.preToolUseHookResult = { + suppressOutput: false, + output: undefined, + success: true, permissionDecision: 'deny', permissionDecisionReason: 'Destructive operations require approval', }; @@ -3762,12 +3765,15 @@ suite('LanguageModelToolsService', () => { assert.strictEqual(state.type, IChatToolInvocation.StateKind.Cancelled); if (state.type === IChatToolInvocation.StateKind.Cancelled) { assert.strictEqual(state.reason, ToolConfirmKind.Denied); - assert.strictEqual(state.reasonMessage, 'Denied by preToolUse hook: Destructive operations require approval'); + assert.strictEqual(state.reasonMessage, 'Denied by PreToolUse hook: Destructive operations require approval'); } }); test('when hook allows, tool executes normally', async () => { mockHooksService.preToolUseHookResult = { + suppressOutput: false, + output: undefined, + success: true, permissionDecision: 'allow', }; @@ -3810,6 +3816,9 @@ suite('LanguageModelToolsService', () => { test('hook receives correct input parameters', async () => { mockHooksService.preToolUseHookResult = { + suppressOutput: false, + output: undefined, + success: true, permissionDecision: 'allow', }; @@ -3832,6 +3841,9 @@ suite('LanguageModelToolsService', () => { test('when hook denies, tool invoke is never called', async () => { mockHooksService.preToolUseHookResult = { + suppressOutput: false, + output: undefined, + success: true, permissionDecision: 'deny', permissionDecisionReason: 'Operation not allowed', }; diff --git a/src/vs/workbench/contrib/chat/test/common/hooksExecutionService.test.ts b/src/vs/workbench/contrib/chat/test/common/hooksExecutionService.test.ts index 09238ec7ae6f0..bcfea2828a1e7 100644 --- a/src/vs/workbench/contrib/chat/test/common/hooksExecutionService.test.ts +++ b/src/vs/workbench/contrib/chat/test/common/hooksExecutionService.test.ts @@ -8,7 +8,7 @@ import { CancellationToken, CancellationTokenSource } from '../../../../../base/ import { URI } from '../../../../../base/common/uri.js'; import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../base/test/common/utils.js'; import { NullLogService } from '../../../../../platform/log/common/log.js'; -import { HookResultKind, HooksExecutionService, IHookResult, IHooksExecutionProxy } from '../../common/hooksExecutionService.js'; +import { HookCommandResultKind, HooksExecutionService, IHookCommandResult, IHooksExecutionProxy } from '../../common/hooksExecutionService.js'; import { HookType, IHookCommand } from '../../common/promptSyntax/hookSchema.js'; import { IOutputChannel, IOutputService } from '../../../../services/output/common/output.js'; @@ -102,13 +102,11 @@ suite('HooksExecutionService', () => { assert.deepStrictEqual(results, []); }); - test('executes hook commands via proxy and returns results', async () => { - const proxyResults: IHookResult[] = []; - const proxy = createMockProxy((cmd) => { - const result: IHookResult = { kind: HookResultKind.Success, result: `executed: ${cmd.command}` }; - proxyResults.push(result); - return result; - }); + test('executes hook commands via proxy and returns semantic results', async () => { + const proxy = createMockProxy((cmd) => ({ + kind: HookCommandResultKind.Success, + result: `executed: ${cmd.command}` + })); service.setProxy(proxy); const hooks = { [HookType.PreToolUse]: [cmd('echo test')] }; @@ -117,15 +115,17 @@ suite('HooksExecutionService', () => { const results = await service.executeHook(HookType.PreToolUse, sessionUri, { input: 'test-input' }); assert.strictEqual(results.length, 1); - assert.strictEqual(results[0].kind, HookResultKind.Success); - assert.strictEqual(results[0].result, 'executed: echo test'); + assert.strictEqual(results[0].success, true); + assert.strictEqual(results[0].stopReason, undefined); + assert.strictEqual(results[0].suppressOutput, false); + assert.strictEqual(results[0].output, 'executed: echo test'); }); test('executes multiple hook commands in order', async () => { const executedCommands: string[] = []; const proxy = createMockProxy((cmd) => { executedCommands.push(cmd.command ?? ''); - return { kind: HookResultKind.Success, result: 'ok' }; + return { kind: HookCommandResultKind.Success, result: 'ok' }; }); service.setProxy(proxy); @@ -140,7 +140,7 @@ suite('HooksExecutionService', () => { assert.deepStrictEqual(executedCommands, ['cmd1', 'cmd2', 'cmd3']); }); - test('wraps proxy errors in HookResultKind.Error', async () => { + test('wraps proxy errors in error result', async () => { const proxy = createMockProxy(() => { throw new Error('proxy failed'); }); @@ -152,15 +152,18 @@ suite('HooksExecutionService', () => { const results = await service.executeHook(HookType.PreToolUse, sessionUri); assert.strictEqual(results.length, 1); - assert.strictEqual(results[0].kind, HookResultKind.Error); - assert.strictEqual(results[0].result, 'proxy failed'); + assert.strictEqual(results[0].success, false); + assert.strictEqual(results[0].output, 'proxy failed'); + // Error results still have default common fields + assert.strictEqual(results[0].stopReason, undefined); + assert.strictEqual(results[0].suppressOutput, false); }); test('passes cancellation token to proxy', async () => { let receivedToken: CancellationToken | undefined; const proxy = createMockProxy((_cmd, _input, token) => { receivedToken = token; - return { kind: HookResultKind.Success, result: 'ok' }; + return { kind: HookCommandResultKind.Success, result: 'ok' }; }); service.setProxy(proxy); @@ -177,7 +180,7 @@ suite('HooksExecutionService', () => { let receivedToken: CancellationToken | undefined; const proxy = createMockProxy((_cmd, _input, token) => { receivedToken = token; - return { kind: HookResultKind.Success, result: 'ok' }; + return { kind: HookCommandResultKind.Success, result: 'ok' }; }); service.setProxy(proxy); @@ -189,11 +192,76 @@ suite('HooksExecutionService', () => { assert.strictEqual(receivedToken, CancellationToken.None); }); + test('extracts common fields from successful result', async () => { + const proxy = createMockProxy(() => ({ + kind: HookCommandResultKind.Success, + result: { + stopReason: 'User requested stop', + suppressOutput: true, + systemMessage: 'Warning: hook triggered', + permissionDecision: 'allow' + } + })); + service.setProxy(proxy); + + const hooks = { [HookType.PreToolUse]: [cmd('echo test')] }; + store.add(service.registerHooks(sessionUri, hooks)); + + const results = await service.executeHook(HookType.PreToolUse, sessionUri); + + assert.strictEqual(results.length, 1); + assert.strictEqual(results[0].success, true); + assert.strictEqual(results[0].stopReason, 'User requested stop'); + assert.strictEqual(results[0].suppressOutput, true); + assert.strictEqual(results[0].systemMessage, 'Warning: hook triggered'); + // Hook-specific fields are in output + assert.deepStrictEqual(results[0].output, { permissionDecision: 'allow' }); + }); + + test('uses defaults when no common fields present', async () => { + const proxy = createMockProxy(() => ({ + kind: HookCommandResultKind.Success, + result: { permissionDecision: 'allow' } + })); + service.setProxy(proxy); + + const hooks = { [HookType.PreToolUse]: [cmd('echo test')] }; + store.add(service.registerHooks(sessionUri, hooks)); + + const results = await service.executeHook(HookType.PreToolUse, sessionUri); + + assert.strictEqual(results.length, 1); + assert.strictEqual(results[0].stopReason, undefined); + assert.strictEqual(results[0].suppressOutput, false); + assert.strictEqual(results[0].systemMessage, undefined); + assert.deepStrictEqual(results[0].output, { permissionDecision: 'allow' }); + }); + + test('handles error results from command', async () => { + const proxy = createMockProxy(() => ({ + kind: HookCommandResultKind.Error, + result: 'command failed with error' + })); + service.setProxy(proxy); + + const hooks = { [HookType.PreToolUse]: [cmd('echo test')] }; + store.add(service.registerHooks(sessionUri, hooks)); + + const results = await service.executeHook(HookType.PreToolUse, sessionUri); + + assert.strictEqual(results.length, 1); + assert.strictEqual(results[0].success, false); + assert.strictEqual(results[0].output, 'command failed with error'); + // Defaults are still applied + assert.strictEqual(results[0].stopReason, undefined); + assert.strictEqual(results[0].suppressOutput, false); + }); + test('passes input to proxy', async () => { let receivedInput: unknown; const proxy = createMockProxy((_cmd, input) => { receivedInput = input; - return { kind: HookResultKind.Success, result: 'ok' }; + return { kind: HookCommandResultKind.Success, result: 'ok' }; }); service.setProxy(proxy); @@ -214,13 +282,13 @@ suite('HooksExecutionService', () => { }); }); - function createMockProxy(handler?: (cmd: IHookCommand, input: unknown, token: CancellationToken) => IHookResult): IHooksExecutionProxy { + function createMockProxy(handler?: (cmd: IHookCommand, input: unknown, token: CancellationToken) => IHookCommandResult): IHooksExecutionProxy { return { runHookCommand: async (hookCommand, input, token) => { if (handler) { return handler(hookCommand, input, token); } - return { kind: HookResultKind.Success, result: 'mock result' }; + return { kind: HookCommandResultKind.Success, result: 'mock result' }; } }; } diff --git a/src/vscode-dts/vscode.proposed.chatHooks.d.ts b/src/vscode-dts/vscode.proposed.chatHooks.d.ts index ff3af7ddb35a3..a222ea6ef6931 100644 --- a/src/vscode-dts/vscode.proposed.chatHooks.d.ts +++ b/src/vscode-dts/vscode.proposed.chatHooks.d.ts @@ -3,7 +3,7 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ -// version: 1 +// version: 2 declare module 'vscode' { @@ -28,32 +28,28 @@ declare module 'vscode' { } /** - * The kind of result from a hook execution. + * Result of executing a hook command. + * Contains common flow control fields and the hook's output. */ - export enum ChatHookResultKind { + export interface ChatHookResult { /** - * Hook executed successfully (exit code 0). + * If set, the agent should stop processing entirely after this hook. + * The message is shown to the user but not to the agent. */ - Success = 1, + readonly stopReason?: string; /** - * Hook returned an error (any non-zero exit code). + * Message shown to the user. */ - Error = 2 - } - - /** - * Result of executing a hook command. - */ - export interface ChatHookResult { + readonly messageForUser?: string; /** - * The kind of result. + * The hook's output (hook-specific fields only). + * For errors, this is the error message string. */ - readonly kind: ChatHookResultKind; + readonly output: unknown; /** - * The result from the hook. For success, this is stdout parsed as JSON. - * For errors, this is stderr. + * Whether the hook command executed successfully (exit code 0). */ - readonly result: string | object; + readonly success: boolean; } export namespace chat { From 74db46d029dffccc6f7f641bbfa81e4f7bbf4bb8 Mon Sep 17 00:00:00 2001 From: Rob Lourens Date: Wed, 4 Feb 2026 15:12:04 -0800 Subject: [PATCH 2/2] test fixes --- .../browser/tools/languageModelToolsService.test.ts | 4 ---- .../chat/test/common/hooksExecutionService.test.ts | 10 ++-------- 2 files changed, 2 insertions(+), 12 deletions(-) diff --git a/src/vs/workbench/contrib/chat/test/browser/tools/languageModelToolsService.test.ts b/src/vs/workbench/contrib/chat/test/browser/tools/languageModelToolsService.test.ts index d6353f0d48b27..3f8da43b20065 100644 --- a/src/vs/workbench/contrib/chat/test/browser/tools/languageModelToolsService.test.ts +++ b/src/vs/workbench/contrib/chat/test/browser/tools/languageModelToolsService.test.ts @@ -3732,7 +3732,6 @@ suite('LanguageModelToolsService', () => { test('when hook denies, tool returns error and creates cancelled invocation', async () => { mockHooksService.preToolUseHookResult = { - suppressOutput: false, output: undefined, success: true, permissionDecision: 'deny', @@ -3771,7 +3770,6 @@ suite('LanguageModelToolsService', () => { test('when hook allows, tool executes normally', async () => { mockHooksService.preToolUseHookResult = { - suppressOutput: false, output: undefined, success: true, permissionDecision: 'allow', @@ -3816,7 +3814,6 @@ suite('LanguageModelToolsService', () => { test('hook receives correct input parameters', async () => { mockHooksService.preToolUseHookResult = { - suppressOutput: false, output: undefined, success: true, permissionDecision: 'allow', @@ -3841,7 +3838,6 @@ suite('LanguageModelToolsService', () => { test('when hook denies, tool invoke is never called', async () => { mockHooksService.preToolUseHookResult = { - suppressOutput: false, output: undefined, success: true, permissionDecision: 'deny', diff --git a/src/vs/workbench/contrib/chat/test/common/hooksExecutionService.test.ts b/src/vs/workbench/contrib/chat/test/common/hooksExecutionService.test.ts index bcfea2828a1e7..75a2bd0bf85fe 100644 --- a/src/vs/workbench/contrib/chat/test/common/hooksExecutionService.test.ts +++ b/src/vs/workbench/contrib/chat/test/common/hooksExecutionService.test.ts @@ -117,7 +117,6 @@ suite('HooksExecutionService', () => { assert.strictEqual(results.length, 1); assert.strictEqual(results[0].success, true); assert.strictEqual(results[0].stopReason, undefined); - assert.strictEqual(results[0].suppressOutput, false); assert.strictEqual(results[0].output, 'executed: echo test'); }); @@ -156,7 +155,6 @@ suite('HooksExecutionService', () => { assert.strictEqual(results[0].output, 'proxy failed'); // Error results still have default common fields assert.strictEqual(results[0].stopReason, undefined); - assert.strictEqual(results[0].suppressOutput, false); }); test('passes cancellation token to proxy', async () => { @@ -197,7 +195,6 @@ suite('HooksExecutionService', () => { kind: HookCommandResultKind.Success, result: { stopReason: 'User requested stop', - suppressOutput: true, systemMessage: 'Warning: hook triggered', permissionDecision: 'allow' } @@ -212,8 +209,7 @@ suite('HooksExecutionService', () => { assert.strictEqual(results.length, 1); assert.strictEqual(results[0].success, true); assert.strictEqual(results[0].stopReason, 'User requested stop'); - assert.strictEqual(results[0].suppressOutput, true); - assert.strictEqual(results[0].systemMessage, 'Warning: hook triggered'); + assert.strictEqual(results[0].messageForUser, 'Warning: hook triggered'); // Hook-specific fields are in output assert.deepStrictEqual(results[0].output, { permissionDecision: 'allow' }); }); @@ -232,8 +228,7 @@ suite('HooksExecutionService', () => { assert.strictEqual(results.length, 1); assert.strictEqual(results[0].stopReason, undefined); - assert.strictEqual(results[0].suppressOutput, false); - assert.strictEqual(results[0].systemMessage, undefined); + assert.strictEqual(results[0].messageForUser, undefined); assert.deepStrictEqual(results[0].output, { permissionDecision: 'allow' }); }); @@ -254,7 +249,6 @@ suite('HooksExecutionService', () => { assert.strictEqual(results[0].output, 'command failed with error'); // Defaults are still applied assert.strictEqual(results[0].stopReason, undefined); - assert.strictEqual(results[0].suppressOutput, false); }); test('passes input to proxy', async () => {