Skip to content

Commit 2020647

Browse files
stephentoubCopilot
andcommitted
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 2020647

33 files changed

Lines changed: 989 additions & 62 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>
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/client_options_e2e_test.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -322,10 +322,10 @@ func assertArgValue(t *testing.T, args []string, name, expected string) {
322322

323323
// capturedCli mirrors the JSON file written by the fake stdio CLI script.
324324
type capturedCli struct {
325-
Args []string `json:"args"`
326-
WorkingDirectory string `json:"cwd"`
327-
Requests []capturedRequest `json:"requests"`
328-
Env map[string]string `json:"env"`
325+
Args []string `json:"args"`
326+
WorkingDirectory string `json:"cwd"`
327+
Requests []capturedRequest `json:"requests"`
328+
Env map[string]string `json:"env"`
329329
}
330330

331331
type capturedRequest struct {

go/internal/e2e/mcp_and_agents_e2e_test.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -157,11 +157,11 @@ func TestMCPServersE2E(t *testing.T) {
157157

158158
mcpServers := map[string]copilot.MCPServerConfig{
159159
"env-echo": copilot.MCPStdioServerConfig{
160-
Command: "node",
161-
Args: []string{mcpServerPath},
162-
Tools: &[]string{"*"},
163-
Env: map[string]string{"TEST_SECRET": "hunter2"},
164-
Cwd: mcpServerDir,
160+
Command: "node",
161+
Args: []string{mcpServerPath},
162+
Tools: &[]string{"*"},
163+
Env: map[string]string{"TEST_SECRET": "hunter2"},
164+
WorkingDirectory: mcpServerDir,
165165
},
166166
}
167167

0 commit comments

Comments
 (0)