agentHost: fix PostToolUse hook additionalContext not injected into same-turn request (#311984)

Fix PostToolUse hook additionalContext injection into same-turn model request

onPostToolUse was discarding the output of executeHookCommand instead of
parsing and returning it, unlike onPreToolUse which correctly returned the
parsed result. This caused additionalContext from PostToolUse hooks to only
appear in subsequent turns via history replay, not the same turn.

Also extracted a runHookCommands() helper to eliminate the copy-paste logic
between onPreToolUse and onPostToolUse.

Fixes #311138

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: digitarald

src/vs/platform/agentHost/node/copilot/copilotPluginConverters.ts

@@ -171,6 +171,37 @@ function executeHookCommand(hook: IParsedHookCommand, stdin?: string): Promise<s
 	});
 }
 
+/**
+ * Runs a list of hook commands sequentially, passing `input` as JSON stdin.
+ * Returns the parsed output of the first command that emits a valid JSON object,
+ * or `undefined` if no command produces parseable JSON output.
+ * Command failures are swallowed — hooks are non-fatal.
+ */
+async function runHookCommands(commands: readonly IParsedHookCommand[] | undefined, input: unknown): Promise<object | undefined> {
+	if (!commands) {
+		return undefined;
+	}
+	const stdin = JSON.stringify(input);
+	for (const cmd of commands) {
+		try {
+			const output = await executeHookCommand(cmd, stdin);
+			if (output.trim()) {
+				try {
+					const parsed = JSON.parse(output);
+					if (parsed && typeof parsed === 'object') {
+						return parsed;
+					}
+				} catch {
+					// Non-JSON output is fine — no modification
+				}
+			}
+		} catch {
+			// Hook failures are non-fatal
+		}
+	}
+	return undefined;
+}
+
 /**
  * Mapping from canonical hook type identifiers to SDK SessionHooks handler keys.
  */
@@ -218,26 +249,7 @@ export function toSdkHooks(
 	if (preToolCommands?.length || editTrackingHooks) {
 		hooks.onPreToolUse = async (input: { toolName: string; toolArgs: unknown }) => {
 			await editTrackingHooks?.onPreToolUse(input);
-			if (preToolCommands) {
-				const stdin = JSON.stringify(input);
-				for (const cmd of preToolCommands) {
-					try {
-						const output = await executeHookCommand(cmd, stdin);
-						if (output.trim()) {
-							try {
-								const parsed = JSON.parse(output);
-								if (parsed && typeof parsed === 'object') {
-									return parsed;
-								}
-							} catch {
-								// Non-JSON output is fine — no modification
-							}
-						}
-					} catch {
-						// Hook failures are non-fatal
-					}
-				}
-			}
+			return runHookCommands(preToolCommands, input);
 		};
 	}
 
@@ -246,16 +258,7 @@ export function toSdkHooks(
 	if (postToolCommands?.length || editTrackingHooks) {
 		hooks.onPostToolUse = async (input: { toolName: string; toolArgs: unknown }) => {
 			await editTrackingHooks?.onPostToolUse(input);
-			if (postToolCommands) {
-				const stdin = JSON.stringify(input);
-				for (const cmd of postToolCommands) {
-					try {
-						await executeHookCommand(cmd, stdin);
-					} catch {
-						// Hook failures are non-fatal
-					}
-				}
-			}
+			return runHookCommands(postToolCommands, input);
 		};
 	}
 

src/vs/platform/agentHost/test/node/copilotPluginConverters.test.ts

@@ -4,6 +4,8 @@
  *--------------------------------------------------------------------------------------------*/
 
 import assert from 'assert';
+import { writeFileSync, unlinkSync } from 'fs';
+import { fileURLToPath } from 'url';
 import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../base/test/common/utils.js';
 import { DisposableStore } from '../../../../base/common/lifecycle.js';
 import { Schemas } from '../../../../base/common/network.js';
@@ -13,8 +15,8 @@ import { FileService } from '../../../files/common/fileService.js';
 import { InMemoryFileSystemProvider } from '../../../files/common/inMemoryFilesystemProvider.js';
 import { NullLogService } from '../../../log/common/log.js';
 import { McpServerType } from '../../../mcp/common/mcpPlatformTypes.js';
-import { toSdkMcpServers, toSdkCustomAgents, toSdkSkillDirectories, parsedPluginsEqual } from '../../node/copilot/copilotPluginConverters.js';
-import type { IMcpServerDefinition, INamedPluginResource, IParsedPlugin } from '../../../agentPlugins/common/pluginParsers.js';
+import { toSdkMcpServers, toSdkCustomAgents, toSdkSkillDirectories, parsedPluginsEqual, toSdkHooks } from '../../node/copilot/copilotPluginConverters.js';
+import type { IMcpServerDefinition, INamedPluginResource, IParsedHookGroup, IParsedPlugin } from '../../../agentPlugins/common/pluginParsers.js';
 
 suite('copilotPluginConverters', () => {
 
@@ -187,6 +189,111 @@ suite('copilotPluginConverters', () => {
 		});
 	});
 
+	// ---- toSdkHooks -------------------------------------------------------
+
+	suite('toSdkHooks', () => {
+
+		function makeHookGroup(type: string, command: string): IParsedHookGroup {
+			return {
+				type,
+				commands: [{ command }],
+				uri: URI.file('/plugin/hooks.json'),
+				originalId: type,
+			};
+		}
+
+		/**
+		 * Writes a temp JS script that outputs JSON to stdout and returns
+		 * a `node <path>` command. Works on both bash (/bin/sh -c) and
+		 * cmd.exe without any shell-quoting issues.
+		 * The script is written alongside the compiled test file which is
+		 * guaranteed to exist, be writable, and have no spaces in CI.
+		 */
+		function echoJsonCmd(value: object): { command: string; cleanup: () => void } {
+			const json = JSON.stringify(value);
+			// fileURLToPath(new URL('.', import.meta.url)) is the Node ESM equivalent
+			// of __dirname and works on Node 12+, unlike import.meta.dirname (Node 21.2+).
+			const dir = fileURLToPath(new URL('.', import.meta.url)).replace(/[\\/]$/, '');
+			const filePath = `${dir}/vscode-test-hook-${Date.now()}.js`;
+			writeFileSync(filePath, `process.stdout.write(${JSON.stringify(json)});\n`);
+			// Do NOT quote the path: cmd.exe /c "node path" strips the outer quotes,
+			// leaving "node path" without inner quoting which cmd.exe handles cleanly.
+			const command = `node ${filePath}`;
+			return { command, cleanup: () => { try { unlinkSync(filePath); } catch { /* ignore */ } } };
+		}
+
+		test('onPostToolUse returns parsed JSON output as hook result', async () => {
+			const expectedOutput = { additionalContext: 'Before presenting the plan, run review-plan skill' };
+			const { command, cleanup } = echoJsonCmd(expectedOutput);
+			try {
+				const hookGroup = makeHookGroup('PostToolUse', command);
+				const hooks = toSdkHooks([hookGroup]);
+				const toolResult = { textResultForLlm: 'ok', resultType: 'success' as const };
+				const result = await hooks.onPostToolUse!({ toolName: 'memory', toolArgs: {}, toolResult, timestamp: 0, cwd: '/' }, { sessionId: 'test' });
+				assert.deepStrictEqual(result, expectedOutput);
+			} finally {
+				cleanup();
+			}
+		});
+
+		test('onPostToolUse returns undefined when output is non-JSON', async () => {
+			// Use a script file so there are no cmd.exe quoting issues on Windows.
+			const dir = fileURLToPath(new URL('.', import.meta.url)).replace(/[\\/]$/, '');
+			const filePath = `${dir}/vscode-test-hook-nonjson-${Date.now()}.js`;
+			writeFileSync(filePath, `process.stdout.write('not-json');\n`);
+			try {
+				const hookGroup = makeHookGroup('PostToolUse', `node ${filePath}`);
+				const hooks = toSdkHooks([hookGroup]);
+				const toolResult = { textResultForLlm: 'ok', resultType: 'success' as const };
+				const result = await hooks.onPostToolUse!({ toolName: 'memory', toolArgs: {}, toolResult, timestamp: 0, cwd: '/' }, { sessionId: 'test' });
+				assert.strictEqual(result, undefined);
+			} finally {
+				try { unlinkSync(filePath); } catch { /* ignore */ }
+			}
+		});
+
+		test('onPostToolUse returns undefined when command fails', async () => {
+			const dir = fileURLToPath(new URL('.', import.meta.url)).replace(/[\\/]$/, '');
+			const filePath = `${dir}/vscode-test-hook-fail-${Date.now()}.js`;
+			writeFileSync(filePath, `process.exit(1);\n`);
+			try {
+				const hookGroup = makeHookGroup('PostToolUse', `node ${filePath}`);
+				const hooks = toSdkHooks([hookGroup]);
+				const toolResult = { textResultForLlm: 'ok', resultType: 'success' as const };
+				const result = await hooks.onPostToolUse!({ toolName: 'memory', toolArgs: {}, toolResult, timestamp: 0, cwd: '/' }, { sessionId: 'test' });
+				assert.strictEqual(result, undefined);
+			} finally {
+				try { unlinkSync(filePath); } catch { /* ignore */ }
+			}
+		});
+
+		test('onPostToolUse returns undefined when no commands', async () => {
+			const hooks = toSdkHooks([]);
+			assert.strictEqual(hooks.onPostToolUse, undefined);
+		});
+
+		test('onPostToolUse calls editTrackingHooks and returns command output', async () => {
+			const expectedOutput = { additionalContext: 'context from hook' };
+			const { command, cleanup } = echoJsonCmd(expectedOutput);
+			try {
+				const hookGroup = makeHookGroup('PostToolUse', command);
+				let trackingInput: unknown;
+				const editTrackingHooks = {
+					onPreToolUse: async () => { },
+					onPostToolUse: async (input: unknown) => { trackingInput = input; },
+				};
+				const hooks = toSdkHooks([hookGroup], editTrackingHooks);
+				const toolResult = { textResultForLlm: 'ok', resultType: 'success' as const };
+				const callInput = { toolName: 'memory', toolArgs: {}, toolResult, timestamp: 0, cwd: '/' };
+				const result = await hooks.onPostToolUse!(callInput, { sessionId: 'test' });
+				assert.deepStrictEqual(result, expectedOutput);
+				assert.deepStrictEqual(trackingInput, callInput);
+			} finally {
+				cleanup();
+			}
+		});
+	});
+
 	// ---- parsedPluginsEqual ---------------------------------------------
 
 	suite('parsedPluginsEqual', () => {