From ad0e3de43acb842dd01aa341859661b606cfc38f Mon Sep 17 00:00:00 2001 From: Shangxin Date: Wed, 23 Sep 2026 13:51:53 +0000 Subject: [PATCH 1/2] fix(acp): route derived declared roots through the McpServer converter The v1-only sse write gate rides on the base McpServer [JsonConverter], but AcpJsonContext declares a [JsonSerializable] root per derived transport. A declared derived root resolves its own contract without the base converter, so serializing SseMcpServer (or any sibling) directly wrote {"name",...,"url"} with no "type" discriminator and no version gate, in both v1 and v2; a derived-root payload could not round-trip through the v2 read path, which rejects a type-less object. Repeat the converter on each derived record so every declared root routes through the same polymorphic writer, and widen CanConvert to the whole McpServer family because the generated context validates a root's converter with an exact-type check. Red-first evidence: the new declared-root test reproduced the bypass (assert-throws saw no exception) before the converter attribute landed. Fixes #226 Co-Authored-By: Claude Code --- src/SalmonEgg.Acp/Mcp/McpServerConfig.cs | 25 +++++++++++++++++++ .../Protocol/V2WireContractTests.cs | 20 +++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/src/SalmonEgg.Acp/Mcp/McpServerConfig.cs b/src/SalmonEgg.Acp/Mcp/McpServerConfig.cs index d740c7a4..b2eb1fc6 100644 --- a/src/SalmonEgg.Acp/Mcp/McpServerConfig.cs +++ b/src/SalmonEgg.Acp/Mcp/McpServerConfig.cs @@ -34,6 +34,11 @@ public enum McpServerTransport /// Configuration for a stdio MCP server. /// Communicates with the server over standard input/output. /// + // The base [JsonConverter] only applies when a contract is *declared* as McpServer, so a derived + // declared root (AcpJsonContext lists one per transport) would otherwise serialize raw properties + // with no "type" discriminator and no v1/v2 gate. Repeating the converter on each derived record + // closes that bypass: every root routes through the same polymorphic writer. + [JsonConverter(typeof(McpServerJsonConverter))] public sealed record StdioMcpServer : McpServer { /// @@ -85,6 +90,11 @@ public StdioMcpServer( /// Configuration for an HTTP MCP server. /// Communicates with the server over HTTP requests. /// + // The base [JsonConverter] only applies when a contract is *declared* as McpServer, so a derived + // declared root (AcpJsonContext lists one per transport) would otherwise serialize raw properties + // with no "type" discriminator and no v1/v2 gate. Repeating the converter on each derived record + // closes that bypass: every root routes through the same polymorphic writer. + [JsonConverter(typeof(McpServerJsonConverter))] public sealed record HttpMcpServer : McpServer { /// @@ -124,6 +134,11 @@ public HttpMcpServer(string name, string url, List? headers = nul /// Configuration for an SSE (Server-Sent Events) MCP server. /// Communicates with the server over an SSE stream. /// + // The base [JsonConverter] only applies when a contract is *declared* as McpServer, so a derived + // declared root (AcpJsonContext lists one per transport) would otherwise serialize raw properties + // with no "type" discriminator and no v1/v2 gate. Repeating the converter on each derived record + // closes that bypass: every root routes through the same polymorphic writer. + [JsonConverter(typeof(McpServerJsonConverter))] public sealed record SseMcpServer : McpServer { /// @@ -165,6 +180,11 @@ public SseMcpServer(string name, string url, List? headers = null /// stdio/http/sse): the spec requires a receiver to "preserve the raw payload" for a transport it does not /// recognize, leaving it to the Agent rather than the client to accept or reject it. /// + // The base [JsonConverter] only applies when a contract is *declared* as McpServer, so a derived + // declared root (AcpJsonContext lists one per transport) would otherwise serialize raw properties + // with no "type" discriminator and no v1/v2 gate. Repeating the converter on each derived record + // closes that bypass: every root routes through the same polymorphic writer. + [JsonConverter(typeof(McpServerJsonConverter))] public sealed record CustomMcpServer : McpServer { /// @@ -384,6 +404,11 @@ internal sealed class McpServerJsonConverter : JsonConverter internal const string SseV1OnlyMessage = "ACP MCP server transport sse is only available in protocolVersion 1."; + // The generated context validates a converter against its declared root with an exact-type + // CanConvert check; derived records reuse this converter for their own roots, so any type in + // the McpServer family is accepted and dispatch still happens on the runtime value in Write. + public override bool CanConvert(Type typeToConvert) => typeof(McpServer).IsAssignableFrom(typeToConvert); + public override McpServer? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) { using var document = JsonDocument.ParseValue(ref reader); diff --git a/tests/SalmonEgg.Acp.Tests/Protocol/V2WireContractTests.cs b/tests/SalmonEgg.Acp.Tests/Protocol/V2WireContractTests.cs index de7460df..141d1367 100644 --- a/tests/SalmonEgg.Acp.Tests/Protocol/V2WireContractTests.cs +++ b/tests/SalmonEgg.Acp.Tests/Protocol/V2WireContractTests.cs @@ -240,6 +240,26 @@ public void McpServerV2_TypedSse_RefusesToSerializeAtEveryRoot() Assert.Equal("events", stableSetup.RootElement.GetProperty("mcpServers")[0].GetProperty("name").GetString()); } + [Fact] + public void McpServerV2_DeclaredDerivedRoots_RouteThroughTheVersionGatedConverter() + { + // Arrange + var sse = new SseMcpServer("events", "https://example.test/events"); + var stdio = new StdioMcpServer { Name = "tools", Command = "tool" }; + + // Act + var gated = Assert.Throws(() => JsonSerializer.Serialize(sse, Wire.V2())); + using var v1Root = JsonDocument.Parse(JsonSerializer.Serialize(sse, Wire.V1())); + using var v2StdioRoot = JsonDocument.Parse(JsonSerializer.Serialize(stdio, Wire.V2())); + + // Assert + Assert.Contains(McpServerJsonConverter.SseV1OnlyMessage, gated.Message); + Assert.Equal("sse", v1Root.RootElement.GetProperty("type").GetString()); + // The discriminator must survive at every declared root too, or a derived-root payload + // could not round-trip through the v2 read path (which rejects a type-less object). + Assert.Equal("stdio", v2StdioRoot.RootElement.GetProperty("type").GetString()); + } + [Theory] [InlineData("")] [InlineData(",\"type\":null")] From fbbeb648b86fb84f35be0ba4e466f44a9d8c1118 Mon Sep 17 00:00:00 2001 From: Shangxin Date: Wed, 23 Sep 2026 14:03:09 +0000 Subject: [PATCH 2/2] fix(acp): keep the header and env List infos after the converter roots The derived McpServer records now carry their own converter, so the source generator no longer walks their properties; the List and List infos that used to be emitted as nested types vanished from AcpJsonContext and ApiCompat flagged their public ListMcpHttpHeader / ListMcpEnvVariable members as CP0002 removals. Declare both roots explicitly so the shipped context keeps its surface. Co-Authored-By: Claude Code --- src/SalmonEgg.Acp/Serialization/AcpJsonContext.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/SalmonEgg.Acp/Serialization/AcpJsonContext.cs b/src/SalmonEgg.Acp/Serialization/AcpJsonContext.cs index be0b819e..f06bbf49 100644 --- a/src/SalmonEgg.Acp/Serialization/AcpJsonContext.cs +++ b/src/SalmonEgg.Acp/Serialization/AcpJsonContext.cs @@ -187,6 +187,11 @@ namespace SalmonEgg.Acp.Serialization; [JsonSerializable(typeof(CustomMcpServer))] [JsonSerializable(typeof(McpHttpHeader))] [JsonSerializable(typeof(McpEnvVariable))] +// The derived McpServer records carry their own converter, so the generator no longer walks their +// properties and these two List<> infos must be declared explicitly: they are public members of the +// shipped context (AcpJsonContext.ListMcpHttpHeader / ListMcpEnvVariable). +[JsonSerializable(typeof(List))] +[JsonSerializable(typeof(List))] [JsonSerializable(typeof(ContentBlock))] [JsonSerializable(typeof(TextContentBlock))] [JsonSerializable(typeof(ImageContentBlock))]