From cc8aeb72ca4f2e78295b0938c90a7b596d9f047a Mon Sep 17 00:00:00 2001 From: Paul Date: Sat, 7 Feb 2026 08:56:27 -0800 Subject: [PATCH] Add stopReason and warningMessage support for hooks (#3548) * wip * wip * cleanups * nit * fix test * update * update * updates * updates * fix * Update src/extension/intents/node/toolCallingLoop.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- extensions/copilot/package.json | 2 +- .../intents/node/hookResultProcessor.ts | 146 ++++ .../extension/intents/node/toolCallingLoop.ts | 136 ++-- .../test/node/hookResultProcessor.spec.ts | 716 ++++++++++++++++++ .../test/node/toolCallingLoopHooks.spec.ts | 74 +- .../node/defaultIntentRequestHandler.ts | 25 +- .../agent/summarizedConversationHistory.tsx | 2 +- .../extension/vscode.proposed.chatHooks.d.ts | 24 +- 8 files changed, 1038 insertions(+), 87 deletions(-) create mode 100644 extensions/copilot/src/extension/intents/node/hookResultProcessor.ts create mode 100644 extensions/copilot/src/extension/intents/test/node/hookResultProcessor.spec.ts diff --git a/extensions/copilot/package.json b/extensions/copilot/package.json index 86b5eab8e8e..91e06f0f447 100644 --- a/extensions/copilot/package.json +++ b/extensions/copilot/package.json @@ -90,7 +90,7 @@ "l10n": "./l10n", "enabledApiProposals": [ "agentSessionsWorkspace", - "chatHooks@2", + "chatHooks@3", "extensionsAny", "newSymbolNamesProvider", "interactive", diff --git a/extensions/copilot/src/extension/intents/node/hookResultProcessor.ts b/extensions/copilot/src/extension/intents/node/hookResultProcessor.ts new file mode 100644 index 00000000000..fa289b9539a --- /dev/null +++ b/extensions/copilot/src/extension/intents/node/hookResultProcessor.ts @@ -0,0 +1,146 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import * as l10n from '@vscode/l10n'; +import type { ChatResponseStream } from 'vscode'; +import { ILogService } from '../../../platform/log/common/logService'; +import { ChatHookType } from '../../../vscodeTypes'; + +/** + * Error thrown when a hook requests the agent to abort processing. + * The message should be shown to the user. + */ +export class HookAbortError extends Error { + constructor( + public readonly hookType: string, + public readonly stopReason: string + ) { + super(`Hook ${hookType} aborted: ${stopReason}`); + this.name = 'HookAbortError'; + } +} + +/** + * Type guard to check if an error is a HookAbortError. + */ +export function isHookAbortError(error: unknown): error is HookAbortError { + return error instanceof HookAbortError; +} + +/** + * A hook result from the chat hook service. + */ +export interface HookResult { + stopReason?: string; + resultKind: 'success' | 'error' | 'warning'; + warningMessage?: string; + output: unknown; +} + +/** + * Options for processing hook results. + */ +export interface ProcessHookResultsOptions { + /** The type of hook being processed */ + hookType: ChatHookType; + /** The hook results to process */ + results: readonly HookResult[]; + /** The output stream for displaying messages */ + outputStream: ChatResponseStream | undefined; + /** The log service for logging */ + logService: ILogService; + /** Callback for handling successful hook results. Called with the output for each success. */ + onSuccess: (output: unknown) => void; + /** + * When true, errors and stopReason are completely ignored (no throw, no warning, no hookProgress). + * Use for hooks like SessionStart/SubagentStart where blocking errors should be silently ignored. + */ + ignoreErrors?: boolean; + /** + * Callback for handling error results. When provided, errors are passed to this callback + * instead of being shown to the user. Use for Stop/SubagentStop hooks where errors + * should be collected as blocking reasons. + */ + onError?: (errorMessage: string) => void; +} + +/** + * Processes hook results, handling aborts, warnings, errors, and success cases. + * Warnings are aggregated and displayed together via hookProgress after processing all results. + * + * @param options The processing options + * @throws HookAbortError if any result contains a stopReason or an error result is encountered + */ +export function processHookResults(options: ProcessHookResultsOptions): void { + const { hookType, results, outputStream, logService, onSuccess, ignoreErrors, onError } = options; + + const warnings: string[] = []; + + for (const result of results) { + // Check for stopReason - abort immediately (unless ignoreErrors is set) + if (result.stopReason) { + if (ignoreErrors) { + logService.trace(`[ToolCallingLoop] ${hookType} hook stopReason ignored: ${result.stopReason}`); + continue; + } + logService.info(`[ToolCallingLoop] ${hookType} hook requested abort: ${result.stopReason}`); + outputStream?.hookProgress(hookType, result.stopReason); + throw new HookAbortError(hookType, result.stopReason); + } + + // Collect warnings + if (result.resultKind === 'warning' && result.warningMessage) { + logService.trace(`[ToolCallingLoop] ${hookType} hook warning: ${result.warningMessage}`); + warnings.push(result.warningMessage); + } + + // Handle success + if (result.resultKind === 'success') { + if (result.warningMessage) { + warnings.push(result.warningMessage); + } + onSuccess(result.output); + } + + // Handle error - abort unless ignoreErrors is set or onError is provided + if (result.resultKind === 'error') { + const errorMessage = typeof result.output === 'string' && result.output ? result.output : ''; + logService.error(`[ToolCallingLoop] ${hookType} hook error: ${errorMessage}`); + if (onError) { + // Pass error to callback (for Stop/SubagentStop to collect as blocking reason) + onError(errorMessage); + return; + } else if (ignoreErrors) { + // Completely ignore error - no throw, no hookProgress (silently continue) + continue; + } else { + outputStream?.hookProgress(hookType, formatHookErrorMessage(errorMessage)); + throw new HookAbortError(hookType, errorMessage); + } + } + } + + // Show aggregated warnings via hookProgress + if (warnings.length > 0 && outputStream) { + if (warnings.length === 1) { + outputStream.hookProgress(hookType, undefined, warnings[0]); + } else { + const formattedWarnings = warnings.map((w, i) => `${i + 1}. ${w}`).join('\n'); + outputStream.hookProgress(hookType, undefined, formattedWarnings); + } + } +} + +/** + * Formats a localized error message for a failed hook. + * @param errorMessage The error message from the hook + * @returns A localized error message string + */ +export function formatHookErrorMessage(errorMessage: string): string { + if (errorMessage) { + return l10n.t('A hook failed with a fatal error. Please check the Hooks output channel for more details. Error message: {0}', errorMessage); + } + return l10n.t('A hook failed with a fatal error. Please check the Hooks output channel for more details.'); +} diff --git a/extensions/copilot/src/extension/intents/node/toolCallingLoop.ts b/extensions/copilot/src/extension/intents/node/toolCallingLoop.ts index 3240944dbcc..97f11d597de 100644 --- a/extensions/copilot/src/extension/intents/node/toolCallingLoop.ts +++ b/extensions/copilot/src/extension/intents/node/toolCallingLoop.ts @@ -45,6 +45,7 @@ import { ToolFailureEncountered, ToolResultMetadata } from '../../prompts/node/p import { ToolName } from '../../tools/common/toolNames'; import { ToolCallCancelledError } from '../../tools/common/toolsService'; import { ReadFileParams } from '../../tools/node/readFileTool'; +import { isHookAbortError, processHookResults } from './hookResultProcessor'; export const enum ToolCallLimitBehavior { Confirm, @@ -95,6 +96,13 @@ export interface IToolCallingBuiltPromptEvent { export type ToolCallingLoopFetchOptions = Required> & Pick; +interface StartHookResult { + /** + * Additional context to add to the agent's context, if any. + */ + readonly additionalContext?: string; +} + interface StopHookResult { /** * Whether the agent should continue (not stop). @@ -243,39 +251,47 @@ export abstract class ToolCallingLoop { + protected async executeStopHook(input: StopHookInput, sessionId: string, outputStream: ChatResponseStream | undefined, token: CancellationToken): Promise { try { const results = await this._chatHookService.executeHook('Stop', { toolInvocationToken: this.options.request.toolInvocationToken, input: input }, sessionId, token); - // Collect all blocking reasons (deduplicated) const blockingReasons = new Set(); - for (const result of results) { - if (result.success === true) { - // Output may be a parsed object or a JSON string - const output = result.output; + processHookResults({ + hookType: 'Stop', + results, + outputStream, + logService: this._logService, + onSuccess: (output) => { if (typeof output === 'object' && output !== null) { const hookOutput = output as StopHookOutput; - this._logService.trace(`[DefaultToolCallingLoop] Checking hook output: decision=${hookOutput.decision}, reason=${hookOutput.reason}`); + this._logService.trace(`[ToolCallingLoop] Checking hook output: decision=${hookOutput.decision}, reason=${hookOutput.reason}`); if (hookOutput.decision === 'block' && hookOutput.reason) { - this._logService.trace(`[DefaultToolCallingLoop] Stop hook blocked: ${hookOutput.reason}`); + this._logService.trace(`[ToolCallingLoop] Stop hook blocked: ${hookOutput.reason}`); blockingReasons.add(hookOutput.reason); } } - } else if (result.success === false) { - const errorMessage = typeof result.output === 'string' ? result.output : 'Unknown error'; - this._logService.error(`[DefaultToolCallingLoop] Stop hook error: ${errorMessage}`); - } - } + }, + // Collect errors as blocking reasons (stderr from exit code != 0) + onError: (errorMessage) => { + if (errorMessage) { + this._logService.trace(`[ToolCallingLoop] Stop hook error collected as blocking reason: ${errorMessage}`); + blockingReasons.add(errorMessage); + } + }, + }); if (blockingReasons.size > 0) { return { shouldContinue: true, reasons: [...blockingReasons] }; } return { shouldContinue: false }; } catch (error) { - this._logService.error('[DefaultToolCallingLoop] Error executing Stop hook', error); + if (isHookAbortError(error)) { + throw error; + } + this._logService.error('[ToolCallingLoop] Error executing Stop hook', error); return { shouldContinue: false }; } } @@ -301,21 +317,24 @@ export abstract class ToolCallingLoop { + protected async executeSessionStartHook(input: SessionStartHookInput, sessionId: string, outputStream: ChatResponseStream | undefined, token: CancellationToken): Promise { try { const results = await this._chatHookService.executeHook('SessionStart', { toolInvocationToken: this.options.request.toolInvocationToken, input: input }, sessionId, token); - // Collect additionalContext from all successful hook results const additionalContexts: string[] = []; - for (const result of results) { - if (result.success === true) { - const output = result.output; + processHookResults({ + hookType: 'SessionStart', + results, + outputStream, + logService: this._logService, + onSuccess: (output) => { if (typeof output === 'object' && output !== null) { const hookOutput = output as SessionStartHookOutput; if (hookOutput.additionalContext) { @@ -323,16 +342,18 @@ export abstract class ToolCallingLoop 0 ? additionalContexts.join('\n') : undefined }; } catch (error) { + if (isHookAbortError(error)) { + throw error; + } this._logService.error('[ToolCallingLoop] Error executing SessionStart hook', error); return {}; } @@ -341,21 +362,24 @@ export abstract class ToolCallingLoop { + protected async executeSubagentStartHook(input: SubagentStartHookInput, sessionId: string, outputStream: ChatResponseStream | undefined, token: CancellationToken): Promise { try { const results = await this._chatHookService.executeHook('SubagentStart', { toolInvocationToken: this.options.request.toolInvocationToken, input: input }, sessionId, token); - // Collect additionalContext from all successful hook results const additionalContexts: string[] = []; - for (const result of results) { - if (result.success === true) { - const output = result.output; + processHookResults({ + hookType: 'SubagentStart', + results, + outputStream, + logService: this._logService, + onSuccess: (output) => { if (typeof output === 'object' && output !== null) { const hookOutput = output as SubagentStartHookOutput; if (hookOutput.additionalContext) { @@ -363,16 +387,18 @@ export abstract class ToolCallingLoop 0 ? additionalContexts.join('\n') : undefined }; } catch (error) { + if (isHookAbortError(error)) { + throw error; + } this._logService.error('[ToolCallingLoop] Error executing SubagentStart hook', error); return {}; } @@ -385,18 +411,20 @@ export abstract class ToolCallingLoop { + protected async executeSubagentStopHook(input: SubagentStopHookInput, sessionId: string, outputStream: ChatResponseStream | undefined, token: CancellationToken): Promise { try { const results = await this._chatHookService.executeHook('SubagentStop', { toolInvocationToken: this.options.request.toolInvocationToken, input: input }, sessionId, token); - // Collect all blocking reasons (deduplicated) const blockingReasons = new Set(); - for (const result of results) { - if (result.success === true) { - const output = result.output; + processHookResults({ + hookType: 'SubagentStop', + results, + outputStream, + logService: this._logService, + onSuccess: (output) => { if (typeof output === 'object' && output !== null) { const hookOutput = output as SubagentStopHookOutput; this._logService.trace(`[ToolCallingLoop] Checking SubagentStop hook output: decision=${hookOutput.decision}, reason=${hookOutput.reason}`); @@ -405,17 +433,24 @@ export abstract class ToolCallingLoop { + if (errorMessage) { + this._logService.trace(`[ToolCallingLoop] SubagentStop hook error collected as blocking reason: ${errorMessage}`); + blockingReasons.add(errorMessage); + } + }, + }); if (blockingReasons.size > 0) { return { shouldContinue: true, reasons: [...blockingReasons] }; } return { shouldContinue: false }; } catch (error) { + if (isHookAbortError(error)) { + throw error; + } this._logService.error('[ToolCallingLoop] Error executing SubagentStop hook', error); return { shouldContinue: false }; } @@ -452,8 +487,9 @@ export abstract class ToolCallingLoop { + public async runStartHooks(outputStream: ChatResponseStream | undefined, token: CancellationToken): Promise { const sessionId = this.options.conversation.sessionId; const hasHooks = this.options.request.hasHooksEnabled; @@ -462,7 +498,7 @@ export abstract class ToolCallingLoop = []; + + hookProgress(hookType: ChatHookType, stopReason?: string, systemMessage?: string): void { + this.hookProgressCalls.push({ hookType, stopReason, systemMessage }); + } +} + +describe('hookResultProcessor', () => { + let logService: TestLogService; + let mockStream: MockChatResponseStream; + + beforeEach(() => { + logService = new TestLogService(); + mockStream = new MockChatResponseStream(); + }); + + describe('HookAbortError', () => { + it('should create error with hookType and stopReason', () => { + const error = new HookAbortError('UserPromptSubmit', 'Build failed'); + expect(error.hookType).toBe('UserPromptSubmit'); + expect(error.stopReason).toBe('Build failed'); + expect(error.message).toBe('Hook UserPromptSubmit aborted: Build failed'); + expect(error.name).toBe('HookAbortError'); + }); + + it('should be identifiable via isHookAbortError', () => { + const hookError = new HookAbortError('Stop', 'reason'); + expect(isHookAbortError(hookError)).toBe(true); + expect(isHookAbortError(new Error('regular error'))).toBe(false); + expect(isHookAbortError(null)).toBe(false); + expect(isHookAbortError(undefined)).toBe(false); + }); + }); + + describe('stopReason handling for all hook types', () => { + const hookTypes: ChatHookType[] = [ + 'UserPromptSubmit', + 'SessionStart', + 'Stop', + 'SubagentStart', + 'SubagentStop', + ]; + + hookTypes.forEach((hookType) => { + describe(`${hookType} hook`, () => { + it('should throw HookAbortError when stopReason is present', () => { + const results: HookResult[] = [ + { + resultKind: 'success', + output: {}, + stopReason: 'Build failed, fix errors before continuing', + }, + ]; + + const onSuccess = vi.fn(); + const options: ProcessHookResultsOptions = { + hookType, + results, + outputStream: mockStream as unknown as ChatResponseStream, + logService, + onSuccess, + }; + + expect(() => processHookResults(options)).toThrow(HookAbortError); + expect(() => processHookResults(options)).toThrow( + `Hook ${hookType} aborted: Build failed, fix errors before continuing` + ); + // Verify hookProgress is called with the stopReason + expect(mockStream.hookProgressCalls.length).toBeGreaterThan(0); + expect(mockStream.hookProgressCalls[0].hookType).toBe(hookType); + expect(mockStream.hookProgressCalls[0].stopReason).toBe('Build failed, fix errors before continuing'); + }); + + it('should not call onSuccess when stopReason is present', () => { + const results: HookResult[] = [ + { + resultKind: 'success', + output: { someData: 'value' }, + stopReason: 'Processing blocked', + }, + ]; + + const onSuccess = vi.fn(); + const options: ProcessHookResultsOptions = { + hookType, + results, + outputStream: mockStream as unknown as ChatResponseStream, + logService, + onSuccess, + }; + + try { + processHookResults(options); + } catch { + // Expected to throw + } + + expect(onSuccess).not.toHaveBeenCalled(); + }); + + it('should throw HookAbortError immediately and stop processing remaining results', () => { + const results: HookResult[] = [ + { + resultKind: 'success', + output: { index: 0 }, + stopReason: 'First hook aborted', + }, + { + resultKind: 'success', + output: { index: 1 }, + }, + ]; + + const onSuccess = vi.fn(); + const options: ProcessHookResultsOptions = { + hookType, + results, + outputStream: mockStream as unknown as ChatResponseStream, + logService, + onSuccess, + }; + + expect(() => processHookResults(options)).toThrow('First hook aborted'); + expect(onSuccess).not.toHaveBeenCalled(); + // Verify hookProgress is called with the stopReason + expect(mockStream.hookProgressCalls).toHaveLength(1); + expect(mockStream.hookProgressCalls[0].hookType).toBe(hookType); + expect(mockStream.hookProgressCalls[0].stopReason).toBe('First hook aborted'); + }); + }); + }); + }); + + describe('UserPromptSubmit exit codes', () => { + // Exit code 0 - stdout shown to Claude (onSuccess called) + it('should call onSuccess with output on exit code 0 (success)', () => { + const results: HookResult[] = [ + { + resultKind: 'success', + output: 'Additional context for Claude', + }, + ]; + + const onSuccess = vi.fn(); + processHookResults({ + hookType: 'UserPromptSubmit', + results, + outputStream: mockStream as unknown as ChatResponseStream, + logService, + onSuccess, + }); + + expect(onSuccess).toHaveBeenCalledWith('Additional context for Claude'); + }); + + // Exit code 2 - block processing, erase original prompt, and show stderr to user only + it('should throw HookAbortError and push hookProgress on exit code 2 (error)', () => { + const results: HookResult[] = [ + { + resultKind: 'error', + output: 'Validation failed: missing required field', + }, + ]; + + const onSuccess = vi.fn(); + expect(() => + processHookResults({ + hookType: 'UserPromptSubmit', + results, + outputStream: mockStream as unknown as ChatResponseStream, + logService, + onSuccess, + }) + ).toThrow(HookAbortError); + + expect(onSuccess).not.toHaveBeenCalled(); + expect(mockStream.hookProgressCalls).toHaveLength(1); + expect(mockStream.hookProgressCalls[0].hookType).toBe('UserPromptSubmit'); + expect(mockStream.hookProgressCalls[0].stopReason).toContain('Validation failed: missing required field'); + }); + + // Other exit codes - show stderr to user only (warnings flow) + it('should show warning to user on other exit codes', () => { + const results: HookResult[] = [ + { + resultKind: 'warning', + warningMessage: 'Process exited with code 1: Some warning', + output: undefined, + }, + ]; + + const onSuccess = vi.fn(); + processHookResults({ + hookType: 'UserPromptSubmit', + results, + outputStream: mockStream as unknown as ChatResponseStream, + logService, + onSuccess, + }); + + expect(onSuccess).not.toHaveBeenCalled(); + expect(mockStream.hookProgressCalls).toHaveLength(1); + expect(mockStream.hookProgressCalls[0].hookType).toBe('UserPromptSubmit'); + expect(mockStream.hookProgressCalls[0].systemMessage).toBe('Process exited with code 1: Some warning'); + }); + + it('should aggregate multiple warnings', () => { + const results: HookResult[] = [ + { resultKind: 'warning', warningMessage: 'Warning 1', output: undefined }, + { resultKind: 'warning', warningMessage: 'Warning 2', output: undefined }, + ]; + + processHookResults({ + hookType: 'UserPromptSubmit', + results, + outputStream: mockStream as unknown as ChatResponseStream, + logService, + onSuccess: () => { }, + }); + + expect(mockStream.hookProgressCalls).toHaveLength(1); + expect(mockStream.hookProgressCalls[0].systemMessage).toContain('1. Warning 1'); + expect(mockStream.hookProgressCalls[0].systemMessage).toContain('2. Warning 2'); + }); + }); + + describe('SessionStart exit codes', () => { + // Exit code 0 - stdout shown to Claude + it('should call onSuccess with output on exit code 0 (success)', () => { + const results: HookResult[] = [ + { + resultKind: 'success', + output: { additionalContext: 'Session context data' }, + }, + ]; + + const onSuccess = vi.fn(); + processHookResults({ + hookType: 'SessionStart', + results, + outputStream: mockStream as unknown as ChatResponseStream, + logService, + onSuccess, + }); + + expect(onSuccess).toHaveBeenCalledWith({ additionalContext: 'Session context data' }); + }); + + // Blocking errors are silently ignored (ignoreErrors: true) - no throw, no hookProgress + it('should silently ignore errors when ignoreErrors is true (no throw, no hookProgress)', () => { + const results: HookResult[] = [ + { + resultKind: 'error', + output: 'Session hook error', + }, + ]; + + const onSuccess = vi.fn(); + expect(() => + processHookResults({ + hookType: 'SessionStart', + results, + outputStream: mockStream as unknown as ChatResponseStream, + logService, + onSuccess, + ignoreErrors: true, + }) + ).not.toThrow(); + + expect(onSuccess).not.toHaveBeenCalled(); + // hookProgress should NOT be called when ignoreErrors is true + expect(mockStream.hookProgressCalls).toHaveLength(0); + }); + + // stopReason (continue: false) is silently ignored (ignoreErrors: true) + it('should silently ignore stopReason when ignoreErrors is true', () => { + const results: HookResult[] = [ + { + resultKind: 'success', + output: { additionalContext: 'Some context' }, + stopReason: 'Build failed, should be ignored', + }, + ]; + + const onSuccess = vi.fn(); + expect(() => + processHookResults({ + hookType: 'SessionStart', + results, + outputStream: mockStream as unknown as ChatResponseStream, + logService, + onSuccess, + ignoreErrors: true, + }) + ).not.toThrow(); + + // stopReason means the result is ignored entirely, so onSuccess is NOT called + expect(onSuccess).not.toHaveBeenCalled(); + // hookProgress should NOT be called when ignoreErrors is true + expect(mockStream.hookProgressCalls).toHaveLength(0); + }); + + // Other exit codes - show stderr to user only (warnings) + it('should show warning to user on other exit codes', () => { + const results: HookResult[] = [ + { + resultKind: 'warning', + warningMessage: 'Session start warning', + output: undefined, + }, + ]; + + processHookResults({ + hookType: 'SessionStart', + results, + outputStream: mockStream as unknown as ChatResponseStream, + logService, + onSuccess: () => { }, + }); + + expect(mockStream.hookProgressCalls).toHaveLength(1); + expect(mockStream.hookProgressCalls[0].systemMessage).toBe('Session start warning'); + }); + }); + + describe('Stop exit codes', () => { + // Exit code 0 - stdout/stderr not shown (success silently processed) + it('should call onSuccess with output on exit code 0 (success)', () => { + const results: HookResult[] = [ + { + resultKind: 'success', + output: { decision: 'allow' }, + }, + ]; + + const onSuccess = vi.fn(); + processHookResults({ + hookType: 'Stop', + results, + outputStream: mockStream as unknown as ChatResponseStream, + logService, + onSuccess, + }); + + expect(onSuccess).toHaveBeenCalledWith({ decision: 'allow' }); + // No hookProgress for success + expect(mockStream.hookProgressCalls).toHaveLength(0); + }); + + // Exit code 2 - show stderr to model and continue conversation (onError callback) + it('should call onError callback on exit code 2 (error) instead of throwing', () => { + const results: HookResult[] = [ + { + resultKind: 'error', + output: 'Stop hook blocking reason', + }, + ]; + + const onSuccess = vi.fn(); + const onError = vi.fn(); + processHookResults({ + hookType: 'Stop', + results, + outputStream: mockStream as unknown as ChatResponseStream, + logService, + onSuccess, + onError, + }); + + expect(onSuccess).not.toHaveBeenCalled(); + expect(onError).toHaveBeenCalledWith('Stop hook blocking reason'); + // hookProgress should NOT be called when onError is provided + expect(mockStream.hookProgressCalls).toHaveLength(0); + }); + + // Other exit codes - show stderr to user only (warnings) + it('should show warning to user on other exit codes', () => { + const results: HookResult[] = [ + { + resultKind: 'warning', + warningMessage: 'Stop hook warning', + output: undefined, + }, + ]; + + processHookResults({ + hookType: 'Stop', + results, + outputStream: mockStream as unknown as ChatResponseStream, + logService, + onSuccess: () => { }, + }); + + expect(mockStream.hookProgressCalls).toHaveLength(1); + expect(mockStream.hookProgressCalls[0].systemMessage).toBe('Stop hook warning'); + }); + }); + + describe('SubagentStart exit codes', () => { + // Exit code 0 - stdout shown to subagent + it('should call onSuccess with output on exit code 0 (success)', () => { + const results: HookResult[] = [ + { + resultKind: 'success', + output: { additionalContext: 'Subagent context' }, + }, + ]; + + const onSuccess = vi.fn(); + processHookResults({ + hookType: 'SubagentStart', + results, + outputStream: mockStream as unknown as ChatResponseStream, + logService, + onSuccess, + }); + + expect(onSuccess).toHaveBeenCalledWith({ additionalContext: 'Subagent context' }); + }); + + // Blocking errors are silently ignored (ignoreErrors: true) - no throw, no hookProgress + it('should silently ignore errors when ignoreErrors is true (no throw, no hookProgress)', () => { + const results: HookResult[] = [ + { + resultKind: 'error', + output: 'Subagent start error', + }, + ]; + + const onSuccess = vi.fn(); + expect(() => + processHookResults({ + hookType: 'SubagentStart', + results, + outputStream: mockStream as unknown as ChatResponseStream, + logService, + onSuccess, + ignoreErrors: true, + }) + ).not.toThrow(); + + expect(onSuccess).not.toHaveBeenCalled(); + // hookProgress should NOT be called when ignoreErrors is true + expect(mockStream.hookProgressCalls).toHaveLength(0); + }); + + // stopReason (continue: false) is silently ignored (ignoreErrors: true) + it('should silently ignore stopReason when ignoreErrors is true', () => { + const results: HookResult[] = [ + { + resultKind: 'success', + output: { additionalContext: 'Subagent context' }, + stopReason: 'Blocking condition, should be ignored', + }, + ]; + + const onSuccess = vi.fn(); + expect(() => + processHookResults({ + hookType: 'SubagentStart', + results, + outputStream: mockStream as unknown as ChatResponseStream, + logService, + onSuccess, + ignoreErrors: true, + }) + ).not.toThrow(); + + // stopReason means the result is ignored entirely, so onSuccess is NOT called + expect(onSuccess).not.toHaveBeenCalled(); + // hookProgress should NOT be called when ignoreErrors is true + expect(mockStream.hookProgressCalls).toHaveLength(0); + }); + + // Other exit codes - show stderr to user only (warnings) + it('should show warning to user on other exit codes', () => { + const results: HookResult[] = [ + { + resultKind: 'warning', + warningMessage: 'Subagent start warning', + output: undefined, + }, + ]; + + processHookResults({ + hookType: 'SubagentStart', + results, + outputStream: mockStream as unknown as ChatResponseStream, + logService, + onSuccess: () => { }, + }); + + expect(mockStream.hookProgressCalls).toHaveLength(1); + expect(mockStream.hookProgressCalls[0].systemMessage).toBe('Subagent start warning'); + }); + }); + + describe('SubagentStop exit codes', () => { + // Exit code 0 - stdout/stderr not shown (success silently processed) + it('should call onSuccess with output on exit code 0 (success)', () => { + const results: HookResult[] = [ + { + resultKind: 'success', + output: { decision: 'allow' }, + }, + ]; + + const onSuccess = vi.fn(); + processHookResults({ + hookType: 'SubagentStop', + results, + outputStream: mockStream as unknown as ChatResponseStream, + logService, + onSuccess, + }); + + expect(onSuccess).toHaveBeenCalledWith({ decision: 'allow' }); + expect(mockStream.hookProgressCalls).toHaveLength(0); + }); + + // Exit code 2 - show stderr to subagent and continue having it run (onError callback) + it('should call onError callback on exit code 2 (error) instead of throwing', () => { + const results: HookResult[] = [ + { + resultKind: 'error', + output: 'Subagent stop blocking reason', + }, + ]; + + const onSuccess = vi.fn(); + const onError = vi.fn(); + processHookResults({ + hookType: 'SubagentStop', + results, + outputStream: mockStream as unknown as ChatResponseStream, + logService, + onSuccess, + onError, + }); + + expect(onSuccess).not.toHaveBeenCalled(); + expect(onError).toHaveBeenCalledWith('Subagent stop blocking reason'); + // hookProgress should NOT be called when onError is provided + expect(mockStream.hookProgressCalls).toHaveLength(0); + }); + + // Other exit codes - show stderr to user only (warnings) + it('should show warning to user on other exit codes', () => { + const results: HookResult[] = [ + { + resultKind: 'warning', + warningMessage: 'Subagent stop warning', + output: undefined, + }, + ]; + + processHookResults({ + hookType: 'SubagentStop', + results, + outputStream: mockStream as unknown as ChatResponseStream, + logService, + onSuccess: () => { }, + }); + + expect(mockStream.hookProgressCalls).toHaveLength(1); + expect(mockStream.hookProgressCalls[0].systemMessage).toBe('Subagent stop warning'); + }); + }); + + describe('formatHookErrorMessage', () => { + it('should format error message with details', () => { + const message = formatHookErrorMessage('Connection failed'); + expect(message).toContain('Connection failed'); + expect(message).toContain('hook failed'); + }); + + it('should format error message without details', () => { + const message = formatHookErrorMessage(''); + expect(message).toContain('hook failed'); + expect(message).not.toContain('Error message:'); + }); + }); + + describe('edge cases', () => { + it('should handle empty results array', () => { + const onSuccess = vi.fn(); + processHookResults({ + hookType: 'UserPromptSubmit', + results: [], + outputStream: mockStream as unknown as ChatResponseStream, + logService, + onSuccess, + }); + + expect(onSuccess).not.toHaveBeenCalled(); + expect(mockStream.hookProgressCalls).toHaveLength(0); + }); + + it('should handle undefined outputStream', () => { + const results: HookResult[] = [ + { resultKind: 'warning', warningMessage: 'Warning message', output: undefined }, + ]; + + // Should not throw when outputStream is undefined + expect(() => + processHookResults({ + hookType: 'UserPromptSubmit', + results, + outputStream: undefined, + logService, + onSuccess: () => { }, + }) + ).not.toThrow(); + }); + + it('should include warnings from success results', () => { + const results: HookResult[] = [ + { + resultKind: 'success', + output: 'some output', + warningMessage: 'Warning from success result', + }, + ]; + + const onSuccess = vi.fn(); + processHookResults({ + hookType: 'UserPromptSubmit', + results, + outputStream: mockStream as unknown as ChatResponseStream, + logService, + onSuccess, + }); + + expect(onSuccess).toHaveBeenCalledWith('some output'); + expect(mockStream.hookProgressCalls).toHaveLength(1); + expect(mockStream.hookProgressCalls[0].systemMessage).toBe('Warning from success result'); + }); + + it('should handle error result with empty output', () => { + const results: HookResult[] = [ + { + resultKind: 'error', + output: '', + }, + ]; + + expect(() => + processHookResults({ + hookType: 'UserPromptSubmit', + results, + outputStream: mockStream as unknown as ChatResponseStream, + logService, + onSuccess: () => { }, + }) + ).toThrow(HookAbortError); + + expect(mockStream.hookProgressCalls).toHaveLength(1); + }); + + it('should handle error result with non-string output', () => { + const results: HookResult[] = [ + { + resultKind: 'error', + output: { complex: 'object' }, + }, + ]; + + expect(() => + processHookResults({ + hookType: 'UserPromptSubmit', + results, + outputStream: mockStream as unknown as ChatResponseStream, + logService, + onSuccess: () => { }, + }) + ).toThrow(HookAbortError); + + // Empty error message when output is not a string + expect(mockStream.hookProgressCalls).toHaveLength(1); + }); + + it('should process multiple results in order', () => { + const results: HookResult[] = [ + { resultKind: 'success', output: 'first' }, + { resultKind: 'success', output: 'second' }, + { resultKind: 'success', output: 'third' }, + ]; + + const outputs: unknown[] = []; + processHookResults({ + hookType: 'UserPromptSubmit', + results, + outputStream: mockStream as unknown as ChatResponseStream, + logService, + onSuccess: (output) => outputs.push(output), + }); + + expect(outputs).toEqual(['first', 'second', 'third']); + }); + }); +}); diff --git a/extensions/copilot/src/extension/intents/test/node/toolCallingLoopHooks.spec.ts b/extensions/copilot/src/extension/intents/test/node/toolCallingLoopHooks.spec.ts index 3f55440e19f..63e1186ce9d 100644 --- a/extensions/copilot/src/extension/intents/test/node/toolCallingLoopHooks.spec.ts +++ b/extensions/copilot/src/extension/intents/test/node/toolCallingLoopHooks.spec.ts @@ -103,7 +103,7 @@ class TestToolCallingLoop extends ToolCallingLoop { // Expose the protected method for testing public async testRunStartHooks(token: CancellationToken): Promise { - await this.runStartHooks(token); + await this.runStartHooks(undefined, token); } // Expose additionalHookContext for verification @@ -252,7 +252,7 @@ describe('ToolCallingLoop SessionStart hook', () => { mockChatHookService.setHookResults('SessionStart', [ { - success: true, + resultKind: 'success', output: { additionalContext: 'Context from hook 1' }, }, ]); @@ -279,15 +279,15 @@ describe('ToolCallingLoop SessionStart hook', () => { mockChatHookService.setHookResults('SessionStart', [ { - success: true, + resultKind: 'success', output: { additionalContext: 'Context from hook 1' }, }, { - success: true, + resultKind: 'success', output: { additionalContext: 'Context from hook 2' }, }, { - success: true, + resultKind: 'success', output: { additionalContext: 'Context from hook 3' }, }, ]); @@ -314,15 +314,15 @@ describe('ToolCallingLoop SessionStart hook', () => { mockChatHookService.setHookResults('SessionStart', [ { - success: true, + resultKind: 'success', output: { additionalContext: 'Context from hook 1' }, }, { - success: true, + resultKind: 'success', output: {}, // No additionalContext }, { - success: true, + resultKind: 'success', output: { additionalContext: 'Context from hook 3' }, }, ]); @@ -343,21 +343,21 @@ describe('ToolCallingLoop SessionStart hook', () => { expect(additionalContext).toBe('Context from hook 1\nContext from hook 3'); }); - it('should ignore failed hook results', async () => { + it('should silently ignore failed hook results (blocking errors are ignored)', async () => { const conversation = createTestConversation(1); const request = createMockChatRequest(); mockChatHookService.setHookResults('SessionStart', [ { - success: true, + resultKind: 'success', output: { additionalContext: 'Context from hook 1' }, }, { - success: false, + resultKind: 'error', output: 'Hook error message', }, { - success: true, + resultKind: 'success', output: { additionalContext: 'Context from hook 3' }, }, ]); @@ -372,8 +372,48 @@ describe('ToolCallingLoop SessionStart hook', () => { ); disposables.add(loop); - await loop.testRunStartHooks(tokenSource.token); + // Should NOT throw - blocking errors are silently ignored for SessionStart + await expect(loop.testRunStartHooks(tokenSource.token)).resolves.not.toThrow(); + // Only non-error results should be processed + const additionalContext = loop.getAdditionalHookContext(); + expect(additionalContext).toBe('Context from hook 1\nContext from hook 3'); + }); + + it('should silently ignore stopReason (continue: false) from hook results', async () => { + const conversation = createTestConversation(1); + const request = createMockChatRequest(); + + mockChatHookService.setHookResults('SessionStart', [ + { + resultKind: 'success', + output: { additionalContext: 'Context from hook 1' }, + }, + { + resultKind: 'success', + output: { additionalContext: 'Context from hook 2' }, + stopReason: 'Build failed, should be ignored', + }, + { + resultKind: 'success', + output: { additionalContext: 'Context from hook 3' }, + }, + ]); + + const loop = instantiationService.createInstance( + TestToolCallingLoop, + { + conversation, + toolCallLimit: 10, + request, + } + ); + disposables.add(loop); + + // Should NOT throw - stopReason is silently ignored for SessionStart + await expect(loop.testRunStartHooks(tokenSource.token)).resolves.not.toThrow(); + + // Results with stopReason are skipped, only other results are processed const additionalContext = loop.getAdditionalHookContext(); expect(additionalContext).toBe('Context from hook 1\nContext from hook 3'); }); @@ -434,7 +474,7 @@ describe('ToolCallingLoop SessionStart hook', () => { mockChatHookService.setHookResults('SessionStart', [ { - success: true, + resultKind: 'success', output: { additionalContext: 'Custom context for prompt' }, }, ]); @@ -548,7 +588,7 @@ describe('ToolCallingLoop SubagentStart hook', () => { mockChatHookService.setHookResults('SubagentStart', [ { - success: true, + resultKind: 'success', output: { additionalContext: 'Subagent-specific context' }, }, ]); @@ -578,11 +618,11 @@ describe('ToolCallingLoop SubagentStart hook', () => { mockChatHookService.setHookResults('SubagentStart', [ { - success: true, + resultKind: 'success', output: { additionalContext: 'First subagent context' }, }, { - success: true, + resultKind: 'success', output: { additionalContext: 'Second subagent context' }, }, ]); diff --git a/extensions/copilot/src/extension/prompt/node/defaultIntentRequestHandler.ts b/extensions/copilot/src/extension/prompt/node/defaultIntentRequestHandler.ts index 8fbb6560330..f4130828471 100644 --- a/extensions/copilot/src/extension/prompt/node/defaultIntentRequestHandler.ts +++ b/extensions/copilot/src/extension/prompt/node/defaultIntentRequestHandler.ts @@ -37,6 +37,7 @@ import { IInstantiationService } from '../../../util/vs/platform/instantiation/c import { ChatResponseMarkdownPart, ChatResponseProgressPart, ChatResponseTextEditPart, LanguageModelToolResult2 } from '../../../vscodeTypes'; import { CodeBlocksMetadata, CodeBlockTrackingChatResponseStream } from '../../codeBlocks/node/codeBlockProcessor'; import { CopilotInteractiveEditorResponse, InteractionOutcomeComputer } from '../../inlineChat/node/promptCraftingTypes'; +import { isHookAbortError, processHookResults } from '../../intents/node/hookResultProcessor'; import { EmptyPromptError, IToolCallingBuiltPromptEvent, IToolCallingLoopOptions, IToolCallingResponseEvent, IToolCallLoopResult, ToolCallingLoop, ToolCallingLoopFetchOptions, ToolCallLimitBehavior } from '../../intents/node/toolCallingLoop'; import { UnknownIntent } from '../../intents/node/unknownIntent'; import { ResponseStreamWithLinkification } from '../../linkify/common/responseStreamWithLinkification'; @@ -169,6 +170,9 @@ export class DefaultIntentRequestHandler { return CanceledResult; } else if (err instanceof EmptyPromptError) { return {}; + } else if (isHookAbortError(err)) { + this._logService.info(`[DefaultIntentRequestHandler] Hook ${err.hookType} aborted: ${err.stopReason}`); + return {}; } this._logService.error(err); @@ -342,16 +346,17 @@ export class DefaultIntentRequestHandler { try { // Execute start hooks first (SessionStart/SubagentStart), then UserPromptSubmit - try { - await loop.runStartHooks(this.token); - } catch (error) { - this._logService.error('[DefaultIntentRequestHandler] Error executing start hooks', error); - } - try { - await this._chatHookService.executeHook('UserPromptSubmit', { toolInvocationToken: this.request.toolInvocationToken, input: { prompt: this.request.prompt } satisfies UserPromptSubmitHookInput }, this.conversation.sessionId); - } catch (error) { - this._logService.error('[DefaultIntentRequestHandler] Error executing UserPromptSubmit hook', error); - } + await loop.runStartHooks(this.stream, this.token); + + const userPromptSubmitResults = await this._chatHookService.executeHook('UserPromptSubmit', { toolInvocationToken: this.request.toolInvocationToken, input: { prompt: this.request.prompt } satisfies UserPromptSubmitHookInput }, this.conversation.sessionId, this.token); + processHookResults({ + hookType: 'UserPromptSubmit', + results: userPromptSubmitResults, + outputStream: this.stream, + logService: this._logService, + onSuccess: () => { /* UserPromptSubmit has no output to process */ }, + }); + const result = await loop.run(this.stream, this.token); if (!result.round.toolCalls.length || result.response.type !== ChatFetchResponseType.Success) { loop.telemetry.sendToolCallingTelemetry(result.toolCallRounds, result.availableTools, this.token.isCancellationRequested ? 'cancelled' : result.response.type); diff --git a/extensions/copilot/src/extension/prompts/node/agent/summarizedConversationHistory.tsx b/extensions/copilot/src/extension/prompts/node/agent/summarizedConversationHistory.tsx index 5e17e32258c..1ec7ffe2dbe 100644 --- a/extensions/copilot/src/extension/prompts/node/agent/summarizedConversationHistory.tsx +++ b/extensions/copilot/src/extension/prompts/node/agent/summarizedConversationHistory.tsx @@ -498,7 +498,7 @@ class ConversationHistorySummarizer { }, this.props.promptContext.conversation?.sessionId, this.token ?? CancellationToken.None); for (const result of results) { - if (result.success === false) { + if (result.resultKind === 'error') { const errorMessage = typeof result.output === 'string' ? result.output : 'Unknown error'; this.logService.error(`[ConversationHistorySummarizer] PreCompact hook error: ${errorMessage}`); } diff --git a/extensions/copilot/src/extension/vscode.proposed.chatHooks.d.ts b/extensions/copilot/src/extension/vscode.proposed.chatHooks.d.ts index a6bab344419..a479b9aa6bc 100644 --- a/extensions/copilot/src/extension/vscode.proposed.chatHooks.d.ts +++ b/extensions/copilot/src/extension/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: 2 +// version: 3 declare module 'vscode' { @@ -27,29 +27,37 @@ declare module 'vscode' { readonly toolInvocationToken: ChatParticipantToolToken; } + /** + * The kind of result from executing a hook command. + * - 'success': Hook executed successfully (exit code 0) + * - 'error': Blocking error shown to model (exit code 2) + * - 'warning': Non-blocking warning shown to user only (other exit codes) + */ + export type ChatHookResultKind = 'success' | 'error' | 'warning'; + /** * Result of executing a hook command. * Contains common flow control fields and the hook's output. */ export interface ChatHookResult { + /** + * The kind of result from executing the hook. + */ + readonly resultKind: ChatHookResultKind; /** * 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. + * Warning message shown to the user. */ - readonly messageForUser?: string; + readonly warningMessage?: 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; } export namespace chat { @@ -64,4 +72,4 @@ declare module 'vscode' { */ export function executeHook(hookType: ChatHookType, options: ChatHookExecutionOptions, token?: CancellationToken): Thenable; } -} \ No newline at end of file +}