Skip to content

Commit 895ff7d

Browse files
stephentoubCopilot
stephentoub
and
Copilot
committed
Fix code review feedback and add .NET E2E test infrastructure
- Add OnPreMcpToolCall to hasHooks checks in .NET Client.cs and Go client.go - Add SerializeHookOutput helper for source-gen serialization - Add .NET PreMcpToolCallHookE2ETests (3 tests: set, replace, remove meta) - Add MCP meta-echo test server and snapshot YAML files - Fix Go mcp_and_agents_e2e_test.go (Cwd -> WorkingDirectory) - Remove stale dead_code lint expectation in Rust support.rs - Add serialization unit tests for hook output Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent d0e2621 commit 895ff7d

12 files changed

Lines changed: 413 additions & 5 deletions

‎dotnet/src/Client.cs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -528,6 +528,7 @@ public async Task<CopilotSession> CreateSessionAsync(SessionConfig config, Cance
528528

529529
var hasHooks = config.Hooks != null && (
530530
config.Hooks.OnPreToolUse != null ||
531+
config.Hooks.OnPreMcpToolCall != null ||
531532
config.Hooks.OnPostToolUse != null ||
532533
config.Hooks.OnUserPromptSubmitted != null ||
533534
config.Hooks.OnSessionStart != null ||
@@ -688,6 +689,7 @@ public async Task<CopilotSession> ResumeSessionAsync(string sessionId, ResumeSes
688689

689690
var hasHooks = config.Hooks != null && (
690691
config.Hooks.OnPreToolUse != null ||
692+
config.Hooks.OnPreMcpToolCall != null ||
691693
config.Hooks.OnPostToolUse != null ||
692694
config.Hooks.OnUserPromptSubmitted != null ||
693695
config.Hooks.OnSessionStart != null ||

‎dotnet/src/Session.cs‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1246,9 +1246,9 @@ internal void RegisterHooks(SessionHooks hooks)
12461246
invocation)
12471247
: null,
12481248
"preMcpToolCall" => hooks.OnPreMcpToolCall != null
1249-
? await hooks.OnPreMcpToolCall(
1249+
? SerializeHookOutput(await hooks.OnPreMcpToolCall(
12501250
JsonSerializer.Deserialize(input.GetRawText(), SessionJsonContext.Default.PreMcpToolCallHookInput)!,
1251-
invocation)
1251+
invocation))
12521252
: null,
12531253
"postToolUse" => hooks.OnPostToolUse != null
12541254
? await hooks.OnPostToolUse(
@@ -1288,6 +1288,14 @@ internal void RegisterHooks(SessionHooks hooks)
12881288
}
12891289
}
12901290

1291+
/// <summary>
1292+
/// Pre-serializes a hook output to JsonElement so that the <c>object?</c> typed
1293+
/// <see cref="CopilotClient.HooksInvokeResponse.Output"/> property writes the
1294+
/// correct JSON without relying on polymorphic type resolution.
1295+
/// </summary>
1296+
private static JsonElement? SerializeHookOutput(PreMcpToolCallHookOutput? output) =>
1297+
output is null ? null : JsonSerializer.SerializeToElement(output, SessionJsonContext.Default.PreMcpToolCallHookOutput);
1298+
12911299
/// <summary>
12921300
/// Registers transform callbacks for system message sections.
12931301
/// </summary>

‎dotnet/test/E2E/PreMcpToolCallHookE2ETests.cs‎

Lines changed: 164 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,164 @@
1+
/*---------------------------------------------------------------------------------------------
2+
* Copyright (c) Microsoft Corporation. All rights reserved.
3+
*--------------------------------------------------------------------------------------------*/
4+
5+
using System.Text.Json;
6+
using Xunit;
7+
using Xunit.Abstractions;
8+
9+
namespace GitHub.Copilot.Test.E2E;
10+
11+
/// <summary>
12+
/// E2E tests for the preMcpToolCall hook, verifying meta manipulation scenarios:
13+
/// setting meta, replacing meta, and removing meta.
14+
/// </summary>
15+
public class PreMcpToolCallHookE2ETests(E2ETestFixture fixture, ITestOutputHelper output)
16+
: E2ETestBase(fixture, "pre_mcp_tool_call_hook", output)
17+
{
18+
private static string FindTestHarnessDir()
19+
{
20+
var dir = new DirectoryInfo(AppContext.BaseDirectory);
21+
while (dir != null)
22+
{
23+
var candidate = Path.Combine(dir.FullName, "test", "harness", "test-mcp-meta-echo-server.mjs");
24+
if (File.Exists(candidate))
25+
return Path.GetDirectoryName(candidate)!;
26+
dir = dir.Parent;
27+
}
28+
throw new InvalidOperationException("Could not find test/harness/test-mcp-meta-echo-server.mjs");
29+
}
30+
31+
private static Dictionary<string, McpServerConfig> CreateMetaEchoMcpConfig(string testHarnessDir) => new()
32+
{
33+
["meta-echo"] = new McpStdioServerConfig
34+
{
35+
Command = "node",
36+
Args = [Path.Combine(testHarnessDir, "test-mcp-meta-echo-server.mjs")],
37+
WorkingDirectory = testHarnessDir,
38+
Tools = ["*"]
39+
}
40+
};
41+
42+
[Fact]
43+
public async Task Should_Set_Meta_Via_PreMcpToolCall_Hook()
44+
{
45+
var testHarnessDir = FindTestHarnessDir();
46+
var hookInputs = new List<PreMcpToolCallHookInput>();
47+
48+
var session = await CreateSessionAsync(new SessionConfig
49+
{
50+
McpServers = CreateMetaEchoMcpConfig(testHarnessDir),
51+
Hooks = new SessionHooks
52+
{
53+
OnPreMcpToolCall = (input, invocation) =>
54+
{
55+
hookInputs.Add(input);
56+
using var doc = JsonDocument.Parse("""{"injected":"by-hook","source":"test"}""");
57+
return Task.FromResult<PreMcpToolCallHookOutput?>(new PreMcpToolCallHookOutput
58+
{
59+
MetaToUse = doc.RootElement.Clone()
60+
});
61+
},
62+
},
63+
OnPermissionRequest = PermissionHandler.ApproveAll,
64+
});
65+
66+
var message = await session.SendAndWaitAsync(new MessageOptions
67+
{
68+
Prompt = "Use the meta-echo/echo_meta tool with value 'test-set'. Reply with just the raw tool result."
69+
});
70+
71+
Assert.NotNull(message);
72+
Assert.Contains("injected", message!.Data.Content);
73+
Assert.Contains("by-hook", message.Data.Content);
74+
75+
Assert.NotEmpty(hookInputs);
76+
Assert.Equal("meta-echo", hookInputs[0].ServerName);
77+
Assert.Equal("echo_meta", hookInputs[0].ToolName);
78+
Assert.False(string.IsNullOrEmpty(hookInputs[0].WorkingDirectory));
79+
Assert.True(hookInputs[0].Timestamp > DateTimeOffset.UnixEpoch);
80+
81+
await session.DisposeAsync();
82+
}
83+
84+
[Fact]
85+
public async Task Should_Replace_Meta_Via_PreMcpToolCall_Hook()
86+
{
87+
var testHarnessDir = FindTestHarnessDir();
88+
var hookInputs = new List<PreMcpToolCallHookInput>();
89+
90+
var session = await CreateSessionAsync(new SessionConfig
91+
{
92+
McpServers = CreateMetaEchoMcpConfig(testHarnessDir),
93+
Hooks = new SessionHooks
94+
{
95+
OnPreMcpToolCall = (input, invocation) =>
96+
{
97+
hookInputs.Add(input);
98+
// Completely replace: ignore input.Meta entirely
99+
using var doc = JsonDocument.Parse("""{"completely":"replaced"}""");
100+
return Task.FromResult<PreMcpToolCallHookOutput?>(new PreMcpToolCallHookOutput
101+
{
102+
MetaToUse = doc.RootElement.Clone()
103+
});
104+
},
105+
},
106+
OnPermissionRequest = PermissionHandler.ApproveAll,
107+
});
108+
109+
var message = await session.SendAndWaitAsync(new MessageOptions
110+
{
111+
Prompt = "Use the meta-echo/echo_meta tool with value 'test-replace'. Reply with just the raw tool result."
112+
});
113+
114+
Assert.NotNull(message);
115+
Assert.Contains("completely", message!.Data.Content);
116+
Assert.Contains("replaced", message.Data.Content);
117+
118+
Assert.NotEmpty(hookInputs);
119+
Assert.Equal("meta-echo", hookInputs[0].ServerName);
120+
Assert.Equal("echo_meta", hookInputs[0].ToolName);
121+
122+
await session.DisposeAsync();
123+
}
124+
125+
[Fact]
126+
public async Task Should_Remove_Meta_Via_PreMcpToolCall_Hook()
127+
{
128+
var testHarnessDir = FindTestHarnessDir();
129+
var hookInputs = new List<PreMcpToolCallHookInput>();
130+
131+
var session = await CreateSessionAsync(new SessionConfig
132+
{
133+
McpServers = CreateMetaEchoMcpConfig(testHarnessDir),
134+
Hooks = new SessionHooks
135+
{
136+
OnPreMcpToolCall = (input, invocation) =>
137+
{
138+
hookInputs.Add(input);
139+
// Return output with null MetaToUse to signal removal
140+
return Task.FromResult<PreMcpToolCallHookOutput?>(new PreMcpToolCallHookOutput
141+
{
142+
MetaToUse = null
143+
});
144+
},
145+
},
146+
OnPermissionRequest = PermissionHandler.ApproveAll,
147+
});
148+
149+
var message = await session.SendAndWaitAsync(new MessageOptions
150+
{
151+
Prompt = "Use the meta-echo/echo_meta tool with value 'test-remove'. Reply with just the raw tool result."
152+
});
153+
154+
Assert.NotNull(message);
155+
Assert.Contains("\"meta\":null", message!.Data.Content);
156+
Assert.Contains("test-remove", message.Data.Content);
157+
158+
Assert.NotEmpty(hookInputs);
159+
Assert.Equal("meta-echo", hookInputs[0].ServerName);
160+
Assert.Equal("echo_meta", hookInputs[0].ToolName);
161+
162+
await session.DisposeAsync();
163+
}
164+
}

‎dotnet/test/Unit/SerializationTests.cs‎

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -306,6 +306,75 @@ public void PermissionDecision_SerializesBaseDiscriminator_WithSdkOptions()
306306
Assert.Equal("approve-once", document.RootElement.GetProperty("kind").GetString());
307307
}
308308

309+
[Fact]
310+
public void HooksInvokeResponse_SerializesPreMcpToolCallHookOutput_WithMetaToUse()
311+
{
312+
var options = GetSerializerOptions();
313+
314+
// Create the PreMcpToolCallHookOutput with meta
315+
using var doc = JsonDocument.Parse("""{"injected":"by-hook","source":"test"}""");
316+
var meta = doc.RootElement.Clone();
317+
var hookOutput = new PreMcpToolCallHookOutput { MetaToUse = meta };
318+
319+
// Create the HooksInvokeResponse using reflection (it's internal)
320+
var responseType = GetNestedType(typeof(CopilotClient), "HooksInvokeResponse");
321+
var response = CreateInternalRequest(responseType, ("Output", hookOutput));
322+
323+
// Serialize using the exact same path as SendResultResponseAsync
324+
var typeInfo = options.GetTypeInfo(response.GetType());
325+
var json = JsonSerializer.SerializeToElement(response, typeInfo);
326+
327+
// The JSON should be {"output":{"metaToUse":{"injected":"by-hook","source":"test"}}}
328+
Assert.True(json.TryGetProperty("output", out var outputProp), $"Expected 'output' property. Got: {json}");
329+
Assert.True(outputProp.TryGetProperty("metaToUse", out var metaToUseProp), $"Expected 'metaToUse' property. Got: {outputProp}");
330+
Assert.Equal("by-hook", metaToUseProp.GetProperty("injected").GetString());
331+
Assert.Equal("test", metaToUseProp.GetProperty("source").GetString());
332+
}
333+
334+
[Fact]
335+
public void HooksInvokeResponse_SerializesPreMcpToolCallHookOutput_WithNullMetaToUse()
336+
{
337+
var options = GetSerializerOptions();
338+
339+
// Create the PreMcpToolCallHookOutput with null meta (remove meta)
340+
var hookOutput = new PreMcpToolCallHookOutput { MetaToUse = null };
341+
342+
// Create the HooksInvokeResponse using reflection (it's internal)
343+
var responseType = GetNestedType(typeof(CopilotClient), "HooksInvokeResponse");
344+
var response = CreateInternalRequest(responseType, ("Output", hookOutput));
345+
346+
// Serialize
347+
var typeInfo = options.GetTypeInfo(response.GetType());
348+
var json = JsonSerializer.SerializeToElement(response, typeInfo);
349+
350+
// Should be {"output":{"metaToUse":null}}
351+
Assert.True(json.TryGetProperty("output", out var outputProp), $"Expected 'output' property. Got: {json}");
352+
Assert.True(outputProp.TryGetProperty("metaToUse", out var metaToUseProp), $"Expected 'metaToUse' property. Got: {outputProp}");
353+
Assert.Equal(JsonValueKind.Null, metaToUseProp.ValueKind);
354+
}
355+
356+
[Fact]
357+
public void HooksInvokeResponse_SerializesNullOutput_AsEmptyOrNoOutputProperty()
358+
{
359+
var options = GetSerializerOptions();
360+
361+
// Create the HooksInvokeResponse with null Output (preserve meta)
362+
var responseType = GetNestedType(typeof(CopilotClient), "HooksInvokeResponse");
363+
var response = CreateInternalRequest(responseType, ("Output", (object?)null));
364+
365+
// Serialize
366+
var typeInfo = options.GetTypeInfo(response.GetType());
367+
var json = JsonSerializer.SerializeToElement(response, typeInfo);
368+
369+
// With WhenWritingNull, output property should be omitted when null
370+
// OR if present, should be null
371+
if (json.TryGetProperty("output", out var outputProp))
372+
{
373+
Assert.Equal(JsonValueKind.Null, outputProp.ValueKind);
374+
}
375+
// else: property omitted, which is fine (runtime treats undefined output as no-op)
376+
}
377+
309378
private static JsonSerializerOptions GetSerializerOptions()
310379
{
311380
var prop = typeof(CopilotClient)
@@ -324,6 +393,37 @@ private static Type GetNestedType(Type containingType, string name)
324393
return type!;
325394
}
326395

396+
[Fact]
397+
public void HooksInvokeResponse_SerializesBoxedJsonElement_AsOutput()
398+
{
399+
// This tests the EXACT path used by SerializeHookOutput:
400+
// PreMcpToolCallHookOutput -> serialize to JsonElement -> box as object? in HooksInvokeResponse.Output
401+
var options = GetSerializerOptions();
402+
403+
using var metaDoc = JsonDocument.Parse("""{"injected":"by-hook","source":"test"}""");
404+
var hookOutput = new PreMcpToolCallHookOutput
405+
{
406+
MetaToUse = metaDoc.RootElement.Clone()
407+
};
408+
// SerializeHookOutput returns a JsonElement (value type)
409+
var hookTypeInfo = options.GetTypeInfo(typeof(PreMcpToolCallHookOutput));
410+
JsonElement serializedOutput = JsonSerializer.SerializeToElement(hookOutput, hookTypeInfo);
411+
412+
// HooksInvokeResponse stores this as object? (boxed JsonElement)
413+
var responseType = GetNestedType(typeof(CopilotClient), "HooksInvokeResponse");
414+
var response = CreateInternalRequest(responseType, ("Output", (object)serializedOutput));
415+
416+
// Serialize via GetTypeInfo(response.GetType()) — same as SendResultResponseAsync
417+
var typeInfo = options.GetTypeInfo(response.GetType());
418+
var json = JsonSerializer.SerializeToElement(response, typeInfo);
419+
420+
// Expected: {"output":{"metaToUse":{"injected":"by-hook","source":"test"}}}
421+
Assert.True(json.TryGetProperty("output", out var outputProp), $"Expected 'output'. Got: {json}");
422+
Assert.True(outputProp.TryGetProperty("metaToUse", out var metaToUseProp), $"Expected 'metaToUse' in output. Got: {outputProp}");
423+
Assert.Equal("by-hook", metaToUseProp.GetProperty("injected").GetString());
424+
Assert.Equal("test", metaToUseProp.GetProperty("source").GetString());
425+
}
426+
327427
private static object CreateInternalRequest(Type type, params (string Name, object? Value)[] properties)
328428
{
329429
#if NET8_0_OR_GREATER

‎go/client.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -658,6 +658,7 @@ func (c *Client) CreateSession(ctx context.Context, config *SessionConfig) (*Ses
658658
req.RequestUserInput = Bool(true)
659659
}
660660
if config.Hooks != nil && (config.Hooks.OnPreToolUse != nil ||
661+
config.Hooks.OnPreMcpToolCall != nil ||
661662
config.Hooks.OnPostToolUse != nil ||
662663
config.Hooks.OnUserPromptSubmitted != nil ||
663664
config.Hooks.OnSessionStart != nil ||
@@ -810,6 +811,7 @@ func (c *Client) ResumeSessionWithOptions(ctx context.Context, sessionID string,
810811
req.RequestUserInput = Bool(true)
811812
}
812813
if config.Hooks != nil && (config.Hooks.OnPreToolUse != nil ||
814+
config.Hooks.OnPreMcpToolCall != nil ||
813815
config.Hooks.OnPostToolUse != nil ||
814816
config.Hooks.OnUserPromptSubmitted != nil ||
815817
config.Hooks.OnSessionStart != nil ||

‎go/internal/e2e/mcp_and_agents_e2e_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -161,7 +161,7 @@ func TestMCPServersE2E(t *testing.T) {
161161
Args: []string{mcpServerPath},
162162
Tools: &[]string{"*"},
163163
Env: map[string]string{"TEST_SECRET": "hunter2"},
164-
Cwd: mcpServerDir,
164+
WorkingDirectory: mcpServerDir,
165165
},
166166
}
167167

‎rust/tests/e2e/support.rs‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,6 @@ impl E2eContext {
7878
Ok(ctx)
7979
}
8080

81-
#[expect(dead_code, reason = "used by follow-on E2E ports")]
8281
pub fn repo_root(&self) -> &Path {
8382
&self.repo_root
8483
}

‎test/harness/replayingCapiProxy.ts‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
* Copyright (c) Microsoft Corporation. All rights reserved.
33
*--------------------------------------------------------------------------------------------*/
44

5-
import { existsSync } from "fs";
5+
import { existsSync, appendFileSync } from "fs";
66
import { mkdir, readFile, writeFile } from "fs/promises";
77
import type {
88
ChatCompletion,
@@ -1215,14 +1215,21 @@ function findAssistantIndexAfterPrefix(
12151215
requestMessages: NormalizedMessage[],
12161216
savedMessages: NormalizedMessage[],
12171217
): number | undefined {
1218+
const logFile = process.env.PROXY_DEBUG_LOG;
1219+
const log = (msg: string) => { if (logFile) try { appendFileSync(logFile, msg + "\n"); } catch {} };
1220+
12181221
if (requestMessages.length >= savedMessages.length) {
1222+
log(`prefix check failed: request.length=${requestMessages.length} >= saved.length=${savedMessages.length}`);
12191223
return undefined;
12201224
}
12211225

12221226
for (let i = 0; i < requestMessages.length; i++) {
12231227
const reqMsg = JSON.stringify(requestMessages[i]);
12241228
const savedMsg = JSON.stringify(savedMessages[i]);
12251229
if (reqMsg !== savedMsg) {
1230+
log(`mismatch at index ${i}:`);
1231+
log(` REQ: ${reqMsg.substring(0, 1000)}`);
1232+
log(` SAVED: ${savedMsg.substring(0, 1000)}`);
12261233
return undefined;
12271234
}
12281235
}
@@ -1233,9 +1240,11 @@ function findAssistantIndexAfterPrefix(
12331240
nextIndex < savedMessages.length &&
12341241
savedMessages[nextIndex].role === "assistant"
12351242
) {
1243+
log(`MATCH found at index ${nextIndex}`);
12361244
return nextIndex;
12371245
}
12381246

1247+
log(`no assistant at nextIndex=${nextIndex}, saved.length=${savedMessages.length}`);
12391248
return undefined;
12401249
}
12411250

0 commit comments

Comments
 (0)