Address PR feedback

Author: stephentoub

src/ModelContextProtocol/Logging/Log.cs

@@ -114,7 +114,7 @@ internal static partial class Log
     internal static partial void TransportNotConnected(this ILogger logger, string endpointName);
 
     [LoggerMessage(Level = LogLevel.Information, Message = "Transport sending message for {endpointName} with ID {messageId}, JSON {json}")]
-    internal static partial void TransportSendingMessage(this ILogger logger, string endpointName, string messageId, string json);
+    internal static partial void TransportSendingMessage(this ILogger logger, string endpointName, string messageId, string? json = null);
 
     [LoggerMessage(Level = LogLevel.Information, Message = "Transport message sent for {endpointName} with ID {messageId}")]
     internal static partial void TransportSentMessage(this ILogger logger, string endpointName, string messageId);
@@ -347,4 +347,35 @@ public static partial void SSETransportPostNotAccepted(
         string endpointName,
         string messageId,
         string responseContent);
+
+    /// <summary>
+    /// Logs the byte representation of a message in UTF-8 encoding.
+    /// </summary>
+    /// <param name="logger">The logger to use.</param>
+    /// <param name="endpointName">The name of the endpoint.</param>
+    /// <param name="byteRepresentation">The byte representation as a hex string.</param>
+    [LoggerMessage(EventId = 39000, Level = LogLevel.Trace, Message = "Transport {EndpointName}: Message bytes (UTF-8): {ByteRepresentation}")]
+    private static partial void TransportMessageBytes(this ILogger logger, string endpointName, string byteRepresentation);
+
+    /// <summary>
+    /// Logs the byte representation of a message for diagnostic purposes.
+    /// This is useful for diagnosing encoding issues with non-ASCII characters.
+    /// </summary>
+    /// <param name="logger">The logger to use.</param>
+    /// <param name="endpointName">The name of the endpoint.</param>
+    /// <param name="message">The message to log bytes for.</param>
+    internal static void TransportMessageBytesUtf8(this ILogger logger, string endpointName, string message)
+    {
+        if (logger.IsEnabled(LogLevel.Trace))
+        {
+            var bytes = System.Text.Encoding.UTF8.GetBytes(message);
+            var byteRepresentation =
+#if NET
+                Convert.ToHexString(bytes);
+#else
+                BitConverter.ToString(bytes).Replace("-", " ");
+#endif
+            logger.TransportMessageBytes(endpointName, byteRepresentation);
+        }
+    }
 }

src/ModelContextProtocol/Logging/LoggerExtensions.cs

@@ -21,8 +21,6 @@ public sealed class StdioClientTransport : TransportBase, IClientTransport
     private readonly ILogger _logger;
     private readonly JsonSerializerOptions _jsonOptions;
     private Process? _process;
-    private StreamWriter? _stdInWriter;
-    private StreamReader? _stdOutReader;
     private Task? _readTask;
     private CancellationTokenSource? _shutdownCts;
     private bool _processStarted;
@@ -62,6 +60,8 @@ public async Task ConnectAsync(CancellationToken cancellationToken = default)
 
             _shutdownCts = new CancellationTokenSource();
 
+            UTF8Encoding noBomUTF8 = new(encoderShouldEmitUTF8Identifier: false);
+
             var startInfo = new ProcessStartInfo
             {
                 FileName = _options.Command,
@@ -71,6 +71,11 @@ public async Task ConnectAsync(CancellationToken cancellationToken = default)
                 UseShellExecute = false,
                 CreateNoWindow = true,
                 WorkingDirectory = _options.WorkingDirectory ?? Environment.CurrentDirectory,
+                StandardOutputEncoding = noBomUTF8,
+                StandardErrorEncoding = noBomUTF8,
+#if NET
+                StandardInputEncoding = noBomUTF8,
+#endif
             };
 
             if (!string.IsNullOrWhiteSpace(_options.Arguments))
@@ -95,19 +100,34 @@ public async Task ConnectAsync(CancellationToken cancellationToken = default)
             // Set up error logging
             _process.ErrorDataReceived += (sender, args) => _logger.TransportError(EndpointName, args.Data ?? "(no data)");
 
-            if (!_process.Start())
+            // We need both stdin and stdout to use a no-BOM UTF-8 encoding. On .NET Core,
+            // we can use ProcessStartInfo.StandardOutputEncoding/StandardInputEncoding, but
+            // StandardInputEncoding doesn't exist on .NET Framework; instead, it always picks
+            // up the encoding from Console.InputEncoding. As such, when not targeting .NET Core,
+            // we temporarily change Console.InputEncoding to no-BOM UTF-8 around the Process.Start
+            // call, to ensure it picks up the correct encoding.
+#if NET
+            _processStarted = _process.Start();
+#else
+            Encoding originalInputEncoding = Console.InputEncoding;
+            try
+            {
+                Console.InputEncoding = noBomUTF8;
+                _processStarted = _process.Start();
+            }
+            finally
+            {
+                Console.InputEncoding = originalInputEncoding;
+            }
+#endif
+
+            if (!_processStarted)
             {
                 _logger.TransportProcessStartFailed(EndpointName);
                 throw new McpTransportException("Failed to start MCP server process");
             }
+
             _logger.TransportProcessStarted(EndpointName, _process.Id);
-            _processStarted = true;
-            
-            // Create streams with explicit UTF-8 encoding to ensure proper Unicode character handling
-            // This is especially important for non-ASCII characters like Chinese text and emoji
-            var utf8Encoding = new UTF8Encoding(false); // No BOM
-            _stdInWriter = new StreamWriter(_process.StandardInput.BaseStream, utf8Encoding) { AutoFlush = true };
-            _stdOutReader = new StreamReader(_process.StandardOutput.BaseStream, utf8Encoding);
 
             _process.BeginErrorReadLine();
 
@@ -128,7 +148,7 @@ public async Task ConnectAsync(CancellationToken cancellationToken = default)
     /// <inheritdoc/>
     public override async Task SendMessageAsync(IJsonRpcMessage message, CancellationToken cancellationToken = default)
     {
-        if (!IsConnected || _process?.HasExited == true || _stdInWriter == null)
+        if (!IsConnected || _process?.HasExited == true)
         {
             _logger.TransportNotConnected(EndpointName);
             throw new McpTransportException("Transport is not connected");
@@ -147,8 +167,8 @@ public override async Task SendMessageAsync(IJsonRpcMessage message, Cancellatio
             _logger.TransportMessageBytesUtf8(EndpointName, json);
 
             // Write the message followed by a newline using our UTF-8 writer
-            await _stdInWriter.WriteLineAsync(json).ConfigureAwait(false);
-            await _stdInWriter.FlushAsync(cancellationToken).ConfigureAwait(false);
+            await _process!.StandardInput.WriteLineAsync(json).ConfigureAwait(false);
+            await _process.StandardInput.FlushAsync(cancellationToken).ConfigureAwait(false);
 
             _logger.TransportSentMessage(EndpointName, id);
         }
@@ -172,10 +192,10 @@ private async Task ReadMessagesAsync(CancellationToken cancellationToken)
         {
             _logger.TransportEnteringReadMessagesLoop(EndpointName);
 
-            while (!cancellationToken.IsCancellationRequested && !_process!.HasExited && _stdOutReader != null)
+            while (!cancellationToken.IsCancellationRequested && !_process!.HasExited)
             {
                 _logger.TransportWaitingForMessage(EndpointName);
-                var line = await _stdOutReader.ReadLineAsync(cancellationToken).ConfigureAwait(false);
+                var line = await _process.StandardOutput.ReadLineAsync(cancellationToken).ConfigureAwait(false);
                 if (line == null)
                 {
                     _logger.TransportEndOfStream(EndpointName);
@@ -240,25 +260,8 @@ private async Task ProcessMessageAsync(string line, CancellationToken cancellati
     private async Task CleanupAsync(CancellationToken cancellationToken)
     {
         _logger.TransportCleaningUp(EndpointName);
-        
-        if (_stdInWriter != null)
-        {
-            try
-            {
-                _logger.TransportClosingStdin(EndpointName);
-                _stdInWriter.Close();
-            }
-            catch (Exception ex)
-            {
-                _logger.TransportShutdownFailed(EndpointName, ex);
-            }
 
-            _stdInWriter = null;
-        }
-        
-        _stdOutReader = null;
-        
-        if (_process != null && _processStarted && !_process.HasExited)
+        if (_process is Process process && _processStarted && !process.HasExited)
         {
             try
             {
@@ -267,15 +270,17 @@ private async Task CleanupAsync(CancellationToken cancellationToken)
 
                 // Kill the while process tree because the process may spawn child processes
                 // and Node.js does not kill its children when it exits properly
-                _process.KillTree(_options.ShutdownTimeout);
+                process.KillTree(_options.ShutdownTimeout);
             }
             catch (Exception ex)
             {
                 _logger.TransportShutdownFailed(EndpointName, ex);
             }
-
-            _process.Dispose();
-            _process = null;
+            finally
+            {
+                process.Dispose();
+                _process = null;
+            }
         }
 
         if (_shutdownCts is { } shutdownCts)
@@ -285,29 +290,30 @@ private async Task CleanupAsync(CancellationToken cancellationToken)
             _shutdownCts = null;
         }
 
-        if (_readTask != null)
+        if (_readTask is Task readTask)
         {
             try
             {
                 _logger.TransportWaitingForReadTask(EndpointName);
-                await _readTask.WaitAsync(TimeSpan.FromSeconds(5), cancellationToken).ConfigureAwait(false);
+                await readTask.WaitAsync(TimeSpan.FromSeconds(5), cancellationToken).ConfigureAwait(false);
             }
             catch (TimeoutException)
             {
                 _logger.TransportCleanupReadTaskTimeout(EndpointName);
-                // Continue with cleanup
             }
             catch (OperationCanceledException)
             {
                 _logger.TransportCleanupReadTaskCancelled(EndpointName);
-                // Ignore cancellation
             }
             catch (Exception ex)
             {
                 _logger.TransportCleanupReadTaskFailed(EndpointName, ex);
             }
-            _readTask = null;
-            _logger.TransportReadTaskCleanedUp(EndpointName);
+            finally
+            {
+                _logger.TransportReadTaskCleanedUp(EndpointName);
+                _readTask = null;
+            }
         }
 
         SetConnected(false);