(WIP) Feature/mcp server by maximilien-noal · Pull Request #1924 · OpenRakis/Spice86

maximilien-noal · 2026-02-28T11:12:44Z

Description of Changes

WIP

Rationale behind Changes

WIP

Suggested Testing Steps

WIP

- Created src/Spice86.Core/Emulator/Mcp/ directory - Added IMcpServer interface, McpServer implementation, McpStdioTransport, and McpTypes - Added ModelContextProtocol.Core package dependency (v0.4.0-preview.3) - Updated Configuration class with McpServer boolean property - Integrated MCP server into Spice86DependencyInjection - Added MCP server documentation files - Extracted and verified McpServerTest (8 tests passing) Co-authored-by: maximilien-noal <1087524+maximilien-noal@users.noreply.github.com>

- Remove unused ModelContextProtocol.Server using directive - Use cfgCpu variable directly instead of casting cpuForEmulationLoop - Update comment for ReadLineAsync to reflect actual implementation - Extract test program name constant and helper method to reduce duplication - Refactor all test methods to use centralized test setup helper Co-authored-by: maximilien-noal <1087524+maximilien-noal@users.noreply.github.com>

- Deleted doc/mcpServerExample.md, doc/mcpServerIntegrationTestPlan.md, doc/mcpServerReadme.md - Removed obvious comments from test file (Arrange/Act/Assert patterns) - Removed self-explanatory comments from MCP server implementation - Kept valuable comments explaining thread-safety and pause behavior - All 8 MCP tests still passing Co-authored-by: maximilien-noal <1087524+maximilien-noal@users.noreply.github.com>

- Remove Arrange/Act/Assert comments that were missed in previous cleanup - Add proper resource disposal with using blocks for Spice86DependencyInjection instances - All 8 MCP tests still passing Co-authored-by: maximilien-noal <1087524+maximilien-noal@users.noreply.github.com>

- Refactor McpStdioTransport to use synchronous I/O with background thread (no async in Spice86.Core) - Replace generic catch(Exception) with specific exception types (IOException, ArgumentException, etc.) - Remove all null-forgiving operators (!) from tests, use proper null checks instead - Fix unused variable assignments in tests (use discard _ where appropriate) - Fix redundant null check in McpServer.ReadMemory - Reduce XML documentation to be more lightweight - All 8 MCP tests still passing Co-authored-by: maximilien-noal <1087524+maximilien-noal@users.noreply.github.com>

Co-authored-by: maximilien-noal <1087524+maximilien-noal@users.noreply.github.com>

- Removed IPauseHandler dependency from McpServer (addresses fatal flaw where MCP can't run if emulator is paused) - Refactored to avoid exception-based control flow - methods return tuples with error messages instead of throwing - Fixed error code assignment for parameter validation vs execution errors - Updated tests to work without pause handler - All 8 MCP tests passing Co-authored-by: maximilien-noal <1087524+maximilien-noal@users.noreply.github.com>

- Added VgaCard, IOPortDispatcher, IVgaRenderer, IPauseHandler dependencies to McpServer - New tools: read_io_port, write_io_port, get_video_state, screenshot, pause_emulator, resume_emulator - IO port tools check emulator is not paused before accessing (per requirement) - Screenshot returns raw BGRA32 data as base64 (client can decode) - Video state returns width/height/buffer size - Emulator control tools for pause/resume - All 8 tests passing Co-authored-by: maximilien-noal <1087524+maximilien-noal@users.noreply.github.com>

- Removed IDisposable from McpStdioTransport per requirement - Background thread runs independently with named thread "MCP-Server" - Call Stop() instead of Dispose() in cleanup - Aligns with mcp-openmsx architecture model Co-authored-by: maximilien-noal <1087524+maximilien-noal@users.noreply.github.com>

…y.github.com>

- Remove McpServer option from Configuration (now mandatory) - Make CFG CPU required (non-nullable) in McpServer constructor - Fix resource leaks: proper disposal in tests, CancellationTokenSource.Dispose in Stop() - Fix screenshot description (BGRA32 not PNG) - Remove unused _vgaCard field from McpServer - Make _mcpStdioTransport non-nullable in Spice86DependencyInjection - Fix all test resource leaks (7/8 passing, 1 failing due to FunctionCatalogue isolation issue) Co-authored-by: maximilien-noal <1087524+maximilien-noal@users.noreply.github.com>

- Added FunctionCatalogue public property to Spice86DependencyInjection - Updated test to use the same FunctionCatalogue instance as McpServer - All 8/8 MCP server tests now passing Co-authored-by: maximilien-noal <1087524+maximilien-noal@users.noreply.github.com>

Co-authored-by: maximilien-noal <1087524+maximilien-noal@users.noreply.github.com>

Comprehensive changes to address all remaining feedback: - Added query_ems tool to inspect Expanded Memory Manager state - Added query_xms tool to inspect Extended Memory Manager state - EMS tool returns: page frame info, allocated/free pages, handle details - XMS tool returns: total/free memory, HMA status, handle information - Updated McpServer constructor to accept EMS/XMS managers (nullable) - Updated Spice86DependencyInjection to pass dos.Ems and xms to McpServer - Added EmsStateResponse and XmsStateResponse record types to McpTypes - Tools only registered if respective manager is enabled - All 8/8 MCP tests passing - Build succeeds with 0 warnings/errors Note: Current transport already uses proper synchronous I/O on background thread per project guidelines. System.Text.Json records already implemented in McpTypes. Co-authored-by: maximilien-noal <1087524+maximilien-noal@users.noreply.github.com>

Comprehensive refactoring addressing code review feedback: **Type Safety** - Created McpToolResponse base record for all tool responses - Replaced object return type with McpToolResponse in tool methods - All 11 response types now inherit from McpToolResponse **Error Codes** - Created JsonRpcErrorCode enum with named values - ParseError=-32700, InvalidRequest=-32600, MethodNotFound=-32601 - InvalidParams=-32602, InternalError=-32603 - Replaced all magic numbers with enum casts **Request DTOs** - Created Request/ namespace with typed request records - ReadMemoryRequest (Address, Length properties) - ReadIoPortRequest (Port property) - WriteIoPortRequest (Port, Value properties) - ListFunctionsRequest (optional Limit property) - Replaced TryGetProperty calls with JsonSerializer.Deserialize<T> **LINQ Improvements** - Converted EMS handle collection from foreach to LINQ - Uses Where().Select().ToArray() pattern - Calculates allocatedPages with Sum() **Build & Tests** - Build succeeds: 0 warnings, 0 errors - All 8/8 MCP tests passing - No regressions introduced Deferred items (avoid extensive refactoring): - CallerMemberName helper method pattern - Test cyclomatic complexity reduction - Individual files for each record type (would be 30+ files) Co-authored-by: maximilien-noal <1087524+maximilien-noal@users.noreply.github.com>

Organization: - Response/ namespace: 18 response record types in separate files - CpuRegistersResponse, MemoryReadResponse, FunctionListResponse - CfgCpuGraphResponse, IoPortReadResponse, IoPortWriteResponse - VideoStateResponse, ScreenshotResponse, EmulatorControlResponse - EmsStateResponse, XmsStateResponse - Supporting: GeneralPurposeRegisters, SegmentRegisters, InstructionPointer - CpuFlags, FunctionInfo, EmsHandleInfo, XmsHandleInfo - Schema/ namespace: 11 schema record types in separate files - EmptyInputSchema, EmptySchemaProperties, JsonSchemaProperty - MemoryReadInputSchema, MemoryReadInputProperties - FunctionListInputSchema, FunctionListInputProperties - IoPortInputSchema, IoPortInputProperties - IoPortWriteInputSchema, IoPortWriteInputProperties - Deleted McpTypes.cs (all types moved to separate files) - Updated McpServer.cs with using statements for Response and Schema namespaces Benefits: - Avoids git merge conflicts with one class per file - Improves code navigation and discoverability - Follows C# best practices for type organization - Each type can be modified independently Build: ✅ 0 warnings, 0 errors Tests: ✅ All 8/8 MCP tests passing Co-authored-by: maximilien-noal <1087524+maximilien-noal@users.noreply.github.com>

Refactored McpServerTest to eliminate nested null checks: - Added ParseAndValidateResponse helper to validate JSON-RPC responses - Added ParseResultContent helper to extract and parse tool result content - Added ValidateErrorResponse helper to validate error responses - Refactored all 8 test methods to use helper methods - Eliminated deeply nested if statements throughout tests - Improved readability and maintainability All 8/8 tests passing, 0 warnings, 0 errors Co-authored-by: maximilien-noal <1087524+maximilien-noal@users.noreply.github.com>

* Initial plan * Implement read_ems_memory and read_xms_memory MCP tools - Add request/response DTOs for EMS and XMS memory reads - Add schema classes for input validation - Implement ReadEmsMemory and ReadXmsMemory handlers in McpServer - Add comprehensive tests for both tools - Tests validate error handling when memory managers are not enabled Co-authored-by: maximilien-noal <1087524+maximilien-noal@users.noreply.github.com> * Add McpStatusViewModel and integrate MCP status in UI - Create McpStatusViewModel to track MCP server state - Add MCP status indicator to status bar with icon and text - Add MCP menu with server status and tool count - Wire McpStatusViewModel in MainWindowViewModel and DependencyInjection - Update UI to display MCP connection status Co-authored-by: maximilien-noal <1087524+maximilien-noal@users.noreply.github.com> * Fix null reference issue in McpStatusViewModel initialization - Make McpStatusViewModel optional with nullable property - Set it after McpServer creation to avoid dependency ordering issue - Fixes code review feedback about null-forgiving operator usage Co-authored-by: maximilien-noal <1087524+maximilien-noal@users.noreply.github.com> * Remove reflection usage in read_xms_memory implementation - Make ExtendedMemoryManager.TryGetBlock public with documentation - Replace reflection-based block lookup with proper API call - Improves maintainability and type safety - Addresses code review feedback Co-authored-by: maximilien-noal <1087524+maximilien-noal@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: maximilien-noal <1087524+maximilien-noal@users.noreply.github.com>

Updated ModelContextProtocol.Core package from 0.4.0-preview.3 to 0.5.0-preview.1 to ensure compatibility with .NET 10. Changes: - src/Directory.Packages.props: ModelContextProtocol.Core version bump Testing: - ✅ Build succeeds (0 warnings, 0 errors) - ✅ All 10 MCP tests passing Co-authored-by: maximilien-noal <1087524+maximilien-noal@users.noreply.github.com>

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

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>

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

* feat: Implement MCP HTTP/SSE transport - Added McpHttpTransport using HttpListener for Server-Sent Events (SSE) and JSON-RPC over HTTP POST. - Integrated HTTP transport into Spice86DependencyInjection. - Enabling seamless connection for external AI clients on port 8081. * feat(mcp): add breakpoints, single stepping, and stack inspection support - Implemented HTTP/SSE transport for MCP server. - Integrated EmulatorBreakpointsManager into McpServer. - Added new MCP tools: add_breakpoint, list_breakpoints, remove_breakpoint, clear_breakpoints. - Added execution control tools: step (single instruction) and go (alias for resume). - Added read_stack tool for memory-based stack inspection. - Implemented real-time breakpoint-hit notifications over SSE and Stdio. - Improved tool descriptions for enhanced AI agent clarity. * fix(mcp): resolve test failures in Release configuration - Fixed ObjectDisposedException in McpHttpTransport.Dispose by adding checks for IsListening and wrapping Stop/Close in try-catch. - Fixed NullReferenceException in Renderer by adding defensive null checks for _state and _memory during teardown. - Restored read_ems_memory tool in McpServer dispatcher and ensured EMS/XMS tools are always registered for test compatibility.

…rver

src/Spice86.Core/Emulator/Mcp/McpHttpTransport.cs

+                response.Close();
+            }
+        } catch (Exception ex) {
+            _loggerService.Error(ex, "Error handling MCP HTTP request {Method} {Path}", request.HttpMethod, request.Url?.AbsolutePath);


In general, to fix log forging issues, ensure that any user-controlled data included in log messages is sanitized so it cannot inject new log entries or break the log format. For plain text logs, this usually means stripping or normalizing control characters, especially \r and \n, and possibly other non-printable characters. Then log only this sanitized representation.

For this specific code, the problematic log call is in the catch block in HandleRequestAsync, where _loggerService.Error logs request.HttpMethod and request.Url?.AbsolutePath. We should derive sanitized versions of these two values before the log call, by replacing carriage returns and newlines with safe alternatives (e.g., space) or removing them entirely. To keep behavior as close as possible to the original while preventing forging, we can create small helper local functions or inline sanitization logic that converts null to some placeholder and strips \r and \n. We will do this directly in the catch block so that only the log arguments change; the log message template and status code handling remain the same.

Concretely:

Inside the catch (Exception ex) block in HandleRequestAsync, before calling _loggerService.Error, introduce sanitized local variables, e.g. safeMethod and safePath, derived from request.HttpMethod and request.Url?.AbsolutePath by replacing \r and \n with spaces (or removing them).

Update the _loggerService.Error call to use safeMethod and safePath instead of the raw properties.

No new imports or external dependencies are needed; simple string.Replace calls suffice.

This change addresses all alert variants because they all point to the same log call and tainted request object.

src/Spice86.Core/Emulator/Mcp/McpHttpTransport.cs

+                response.Close();
+            }
+        } catch (Exception ex) {
+            _loggerService.Error(ex, "Error handling MCP HTTP request {Method} {Path}", request.HttpMethod, request.Url?.AbsolutePath);


In general, to fix log-forging issues when logging user-provided values, normalize or encode the values before logging: for plain-text logs, remove or replace newline and other control characters so a malicious user cannot inject extra log lines; for HTML logs, HTML-encode the values. It is also useful to clearly delimit or quote user input in the log message, but the key requirement here is to prevent embedded line breaks.

For this specific case, we should sanitize the path segment taken from request.Url?.AbsolutePath before passing it to _loggerService.Error. The minimal, behavior-preserving approach is to compute a sanitized version of the path inside the catch block, just before the logging call. We can, for example, take the absolute path string (or an empty string if null) and replace carriage returns (\r) and line feeds (\n) with safe placeholders such as spaces or their escaped representations (\\r, \\n). This will ensure that the logged message cannot span multiple lines or otherwise forge extra entries, while still preserving enough information for debugging. We only need to modify HandleRequestAsync in src/Spice86.Core/Emulator/Mcp/McpHttpTransport.cs, and no new imports are required if we stick to string operations like Replace.

Concretely: in the catch (Exception ex) block, introduce a local variable string pathForLog = (request.Url?.AbsolutePath ?? string.Empty).Replace("\r", "\\r").Replace("\n", "\\n"); and then change the logging call to use pathForLog instead of request.Url?.AbsolutePath. This addresses all alert variants referring to this sink, because the only tainted part is the URL path argument.

tests/Spice86.Tests/McpServerTest.cs

src/Spice86.Core/Emulator/Mcp/McpHttpTransport.cs

+        } catch (Exception ex) {
+            _loggerService.Error(ex, "Error processing MCP request in HTTP transport");
+            responseJson = $$"""
+            {
+              "jsonrpc": "2.0",
+              "error": {
+                "code": -32603,
+                "message": "Internal error: {{ex.Message.Replace("\"", "\\\"")}}"
+              },
+              "id": null
+            }
+            """;
+        }


General approach: narrow the generic catch (Exception) to exclude exceptions that should not be treated as a normal JSON-RPC “internal error”, particularly cancellation-related exceptions which are part of normal control flow. This reduces over-broad exception handling while preserving the existing behavior for real internal failures.

Best concrete fix here: keep a catch that handles most Exceptions but explicitly rethrow OperationCanceledException (and potentially TaskCanceledException, which derives from it) so that cancellation is not turned into an error payload. This does not change the observable behavior for all other exceptions (they are still logged and converted into the existing JSON-RPC error structure), but it avoids swallowing cancellation inappropriately. We can do this by adding a conditional check at the top of the catch block and rethrowing when we encounter a cancellation exception.

Specific changes:

File: src/Spice86.Core/Emulator/Mcp/McpHttpTransport.cs

In HandlePostAsync, modify the catch (Exception ex) block that starts at line 133:

Add an if (ex is OperationCanceledException) (or if (ex is OperationCanceledException or TaskCanceledException) if we prefer to be explicit) that immediately rethrows.

Leave the rest of the catch block unchanged, so existing logging and JSON-RPC error formatting are preserved.

No new methods or imports are necessary, since OperationCanceledException is in System, which is already imported.

src/Spice86.Core/Emulator/Mcp/McpHttpTransport.cs

src/Spice86.Core/Emulator/Mcp/McpServer.cs

Signed-off-by: Maximilien Noal <noal.maximilien@gmail.com>

src/Spice86.Core/Emulator/Mcp/McpServer.cs

+
+        ushort port = portElement.GetUInt16();
+        byte value = valueElement.GetByte();
+        _ioPortDispatcher.WriteByte((ushort)port, (byte)value);


To fix the problem, remove the unnecessary cast and pass the variable directly where a ushort is expected. In general, for “cast to same type” issues, you simply delete the cast whenever the expression’s static type already matches the target type and no user-defined conversion is involved.

Concretely here, in src/Spice86.Core/Emulator/Mcp/McpServer.cs, in the WriteIoPort method, update the _ioPortDispatcher.WriteByte call at line 483 to remove the (ushort) cast from port. Keep the (byte) cast for value because value is declared as byte but retaining or removing that specific cast may be a separate concern CodeQL did not flag; we will not alter it beyond what’s necessary. No new methods, imports, or additional definitions are required; we only adjust this single call site to streamline the code without altering functionality.

src/Spice86.Core/Emulator/Mcp/McpServer.cs

+
+        ushort port = portElement.GetUInt16();
+        byte value = valueElement.GetByte();
+        _ioPortDispatcher.WriteByte((ushort)port, (byte)value);


To fix this, we should remove the unnecessary cast of value to byte in the call to _ioPortDispatcher.WriteByte. The variable value is already declared as byte and assigned from valueElement.GetByte(), so passing value directly is sufficient. This preserves all existing functionality because the argument type remains byte and there are no behavior changes.

Concretely, in src/Spice86.Core/Emulator/Mcp/McpServer.cs, in the WriteIoPort method, update the _ioPortDispatcher.WriteByte call around line 483 by changing _ioPortDispatcher.WriteByte((ushort)port, (byte)value); to _ioPortDispatcher.WriteByte((ushort)port, value);. No imports, new methods, or additional definitions are required.

src/Spice86/Spice86DependencyInjection.cs

            hostStorageProvider != null && textClipboard != null) {
            IMessenger messenger = WeakReferenceMessenger.Default;

+            mainWindowViewModel?.McpStatusViewModel = new McpStatusViewModel(mcpServer);


General fix: Remove the constant-condition null check by replacing the null-conditional access to mainWindowViewModel with a direct access, since mainWindowViewModel is determined to be non-null inside this if block. This simplifies the code and eliminates the constant-condition warning.

Concrete change: On line 635, change mainWindowViewModel?.McpStatusViewModel = new McpStatusViewModel(mcpServer); to mainWindowViewModel.McpStatusViewModel = new McpStatusViewModel(mcpServer);. This is the only line that needs modification to address all four alert variants; the outer if (mainWindow != null && uiDispatcher != null && hostStorageProvider != null && textClipboard != null) condition remains unchanged. No new methods, imports, or definitions are needed, and existing behavior is preserved as long as mainWindowViewModel is indeed non-null when this code runs (which the analyzer asserts).

File/region to change: src/Spice86/Spice86DependencyInjection.cs, around lines 631–636 where the UI-related view models are instantiated and assigned.

src/Spice86.Core/Emulator/Mcp/McpHttpTransport.cs

+            } catch {
+                // Client probably disconnected
+                _clients.TryRemove(client.Key, out _);
+            }


In general, fix generic catch clauses by catching specific, expected exception types (e.g., IOException, ObjectDisposedException, OperationCanceledException) and, optionally, adding a final catch (Exception ex) that logs and rethrows or otherwise handles unexpected errors. This both documents which failures are anticipated and prevents completely swallowing unexpected problems.

For BroadcastNotificationAsync in src/Spice86.Core/Emulator/Mcp/McpHttpTransport.cs, the best fix that preserves existing functionality is:

Replace the bare catch { ... } with:

A catch (IOException) and catch (ObjectDisposedException) and catch (OperationCanceledException) that keep the current behavior (just remove the client, no logging).

An additional catch (Exception ex) that logs the unexpected error via _loggerService.Error(...) and then also removes the client (to avoid changing behavior with respect to the client list), but does not rethrow (to keep the broadcast loop resilient as originally designed).

This keeps the server robust against misbehaving/disconnecting clients, documents which exceptions are expected, and ensures that truly unexpected exceptions are at least logged for diagnosis. All changes are confined to the shown method body; no new imports are needed, since System.IO is already imported.

Concretely:

In McpHttpTransport.BroadcastNotificationAsync, replace the single catch { ... } block around the WriteAsync/FlushAsync calls with multiple typed catch blocks and logging of unexpected exceptions.

Use the existing _loggerService to log the error (e.g., _loggerService.Error(ex, "Error broadcasting notification to client {ClientId}", client.Key);).

Copilot

Pull request overview

This WIP PR introduces an in-process MCP (Model Context Protocol) server to expose emulator inspection/control tools over JSON-RPC, with UI surfacing of server status.

Changes:

Added McpServer with a set of JSON-RPC tools (registers/memory/IO/video/EMS/XMS/breakpoints, etc.) plus request/response/schema DTOs.
Added transports for MCP over stdio and over HTTP/SSE (localhost).
Wired MCP server + transports into Spice86DependencyInjection and added MCP status display in the main window menu/status bar.

Reviewed changes

Copilot reviewed 58 out of 58 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
src/Spice86/Views/MainWindow.axaml	Adds MCP menu/status bar UI bindings.
src/Spice86/ViewModels/McpStatusViewModel.cs	New VM for displaying MCP server status.
src/Spice86/ViewModels/MainWindowViewModel.cs	Adds `McpStatusViewModel` property for UI binding.
src/Spice86/Spice86DependencyInjection.cs	Constructs MCP server and starts stdio + HTTP transports; wires VM.
src/Spice86.Core/Spice86.Core.csproj	Adds ModelContextProtocol.Core package reference.
src/Spice86.Core/Emulator/Mcp/Schema/XmsMemoryReadInputSchema.cs	MCP JSON schema DTO for XMS memory read.
src/Spice86.Core/Emulator/Mcp/Schema/XmsMemoryReadInputProperties.cs	Properties DTO for XMS memory read schema.
src/Spice86.Core/Emulator/Mcp/Schema/MemoryReadInputSchema.cs	MCP JSON schema DTO for linear memory read.
src/Spice86.Core/Emulator/Mcp/Schema/MemoryReadInputProperties.cs	Properties DTO for memory read schema.
src/Spice86.Core/Emulator/Mcp/Schema/JsonSchemaProperty.cs	Common schema property descriptor DTO.
src/Spice86.Core/Emulator/Mcp/Schema/IoPortWriteInputSchema.cs	MCP JSON schema DTO for IO port write.
src/Spice86.Core/Emulator/Mcp/Schema/IoPortWriteInputProperties.cs	Properties DTO for IO port write schema.
src/Spice86.Core/Emulator/Mcp/Schema/IoPortInputSchema.cs	MCP JSON schema DTO for IO port read.
src/Spice86.Core/Emulator/Mcp/Schema/IoPortInputProperties.cs	Properties DTO for IO port read schema.
src/Spice86.Core/Emulator/Mcp/Schema/FunctionListInputSchema.cs	MCP JSON schema DTO for function listing.
src/Spice86.Core/Emulator/Mcp/Schema/FunctionListInputProperties.cs	Properties DTO for function listing schema.
src/Spice86.Core/Emulator/Mcp/Schema/EmsMemoryReadInputSchema.cs	MCP JSON schema DTO for EMS memory read.
src/Spice86.Core/Emulator/Mcp/Schema/EmsMemoryReadInputProperties.cs	Properties DTO for EMS memory read schema.
src/Spice86.Core/Emulator/Mcp/Schema/EmptySchemaProperties.cs	Empty schema properties DTO for parameterless tools.
src/Spice86.Core/Emulator/Mcp/Schema/EmptyInputSchema.cs	Parameterless tool schema DTO.
src/Spice86.Core/Emulator/Mcp/Response/XmsStateResponse.cs	Response DTO for XMS state tool.
src/Spice86.Core/Emulator/Mcp/Response/XmsMemoryReadResponse.cs	Response DTO for XMS memory read tool.
src/Spice86.Core/Emulator/Mcp/Response/XmsHandleInfo.cs	Response DTO for XMS handle listing.
src/Spice86.Core/Emulator/Mcp/Response/VideoStateResponse.cs	Response DTO for VGA state tool.
src/Spice86.Core/Emulator/Mcp/Response/StackResponse.cs	Response DTO for stack inspection tool.
src/Spice86.Core/Emulator/Mcp/Response/SegmentRegisters.cs	Response DTO for CPU segment registers.
src/Spice86.Core/Emulator/Mcp/Response/ScreenshotResponse.cs	Response DTO for screenshot tool (raw pixel payload).
src/Spice86.Core/Emulator/Mcp/Response/MemoryReadResponse.cs	Response DTO for linear memory read tool.
src/Spice86.Core/Emulator/Mcp/Response/IoPortWriteResponse.cs	Response DTO for IO port write tool.
src/Spice86.Core/Emulator/Mcp/Response/IoPortReadResponse.cs	Response DTO for IO port read tool.
src/Spice86.Core/Emulator/Mcp/Response/InstructionPointer.cs	Response DTO for instruction pointer.
src/Spice86.Core/Emulator/Mcp/Response/GeneralPurposeRegisters.cs	Response DTO for GPR register set.
src/Spice86.Core/Emulator/Mcp/Response/FunctionListResponse.cs	Response DTO for function listing tool.
src/Spice86.Core/Emulator/Mcp/Response/FunctionInfo.cs	Response DTO describing a function entry.
src/Spice86.Core/Emulator/Mcp/Response/EmulatorControlResponse.cs	Response DTO for pause/resume/step/bp commands.
src/Spice86.Core/Emulator/Mcp/Response/EmsStateResponse.cs	Response DTO for EMS state tool.
src/Spice86.Core/Emulator/Mcp/Response/EmsMemoryReadResponse.cs	Response DTO for EMS memory read tool.
src/Spice86.Core/Emulator/Mcp/Response/EmsHandleInfo.cs	Response DTO for EMS handle listing.
src/Spice86.Core/Emulator/Mcp/Response/CpuRegistersResponse.cs	Response DTO for CPU registers tool.
src/Spice86.Core/Emulator/Mcp/Response/CpuFlags.cs	Response DTO for CPU flags.
src/Spice86.Core/Emulator/Mcp/Response/CfgCpuGraphResponse.cs	Response DTO for CFG CPU stats tool.
src/Spice86.Core/Emulator/Mcp/Response/BreakpointListResponse.cs	Response DTO for listing MCP-managed breakpoints.
src/Spice86.Core/Emulator/Mcp/Response/BreakpointInfo.cs	Response DTO describing a breakpoint entry.
src/Spice86.Core/Emulator/Mcp/Request/WriteIoPortRequest.cs	Request DTO for IO port write tool.
src/Spice86.Core/Emulator/Mcp/Request/ReadXmsMemoryRequest.cs	Request DTO for XMS memory read tool.
src/Spice86.Core/Emulator/Mcp/Request/ReadMemoryRequest.cs	Request DTO for memory read tool.
src/Spice86.Core/Emulator/Mcp/Request/ReadIoPortRequest.cs	Request DTO for IO port read tool.
src/Spice86.Core/Emulator/Mcp/Request/ReadEmsMemoryRequest.cs	Request DTO for EMS memory read tool.
src/Spice86.Core/Emulator/Mcp/Request/ListFunctionsRequest.cs	Request DTO for function listing tool.
src/Spice86.Core/Emulator/Mcp/McpToolResponse.cs	Base type for MCP tool responses.
src/Spice86.Core/Emulator/Mcp/McpStdioTransport.cs	Stdio transport implementation for MCP JSON-RPC.
src/Spice86.Core/Emulator/Mcp/McpServer.cs	Core MCP JSON-RPC server + tool implementations.
src/Spice86.Core/Emulator/Mcp/McpHttpTransport.cs	HTTP + SSE transport for MCP server.
src/Spice86.Core/Emulator/Mcp/McpExceptions.cs	MCP exception types mapping to JSON-RPC error codes.
src/Spice86.Core/Emulator/Mcp/JsonRpcErrorCode.cs	JSON-RPC error code enum.
src/Spice86.Core/Emulator/Mcp/IMcpServer.cs	MCP server interface contract.
src/Spice86.Core/Emulator/InterruptHandlers/Dos/Xms/ExtendedMemoryManager.cs	Exposes XMS block lookup to support MCP reads.
src/Directory.Packages.props	Adds ModelContextProtocol.Core package version.

Copilot · 2026-03-10T21:16:08Z

src/Spice86.Core/Emulator/Mcp/McpStdioTransport.cs

+    public void Stop() {
+        _mcpServer.OnNotification -= HandleNotification;
+        if (_readerThread == null) {
+            return;
+        }
+
+        _loggerService.Information("MCP server stopping");
+        _cancellationTokenSource.Cancel();
+
+        if (!_readerThread.Join(TimeSpan.FromSeconds(5))) {
+            _loggerService.Warning("MCP server thread did not stop within timeout");
+        }
+        _cancellationTokenSource.Dispose();
+        _readerThreadStarted = false;
+    }


Stop() cancels the token, but the reader thread is blocked on _inputReader.ReadLine() and won't observe cancellation. This makes shutdown non-deterministic (often timing out) and leaves a background thread running after disposal; additionally _cancellationTokenSource is disposed, so calling Start() again will fail. Consider a transport design that can be cleanly interrupted (e.g., only enable stdio transport in dedicated MCP mode, or close the underlying input stream / use a non-blocking read loop).

Copilot · 2026-03-10T21:16:08Z

src/Spice86.Core/Emulator/Mcp/McpHttpTransport.cs

+        // Send endpoint event
+        string endpointMsg = $"event: endpoint\ndata: http://localhost:{_port}/messages\n\n";
+        byte[] buffer = Encoding.UTF8.GetBytes(endpointMsg);
+        await response.OutputStream.WriteAsync(buffer, 0, buffer.Length);
+        await response.OutputStream.FlushAsync();
+
+        // Keep connection open
+        while (!_cts.Token.IsCancellationRequested) {
+            await Task.Delay(15000, _cts.Token);
+            // Heartbeat
+            await response.OutputStream.WriteAsync(Encoding.UTF8.GetBytes(": heartbeat\n\n"));
+            await response.OutputStream.FlushAsync();


HandleSseAsync keeps the connection open and periodically writes heartbeats, but it never removes the client from _clients when the loop exits or the client disconnects, and write failures in the heartbeat loop are not handled (which can leak entries and responses). Wrap the SSE loop in try/finally to always remove/close the response and handle disconnect exceptions cleanly.

Suggested change

// Send endpoint event

string endpointMsg = $"event: endpoint\ndata: http://localhost:{_port}/messages\n\n";

byte[] buffer = Encoding.UTF8.GetBytes(endpointMsg);

await response.OutputStream.WriteAsync(buffer, 0, buffer.Length);

await response.OutputStream.FlushAsync();

// Keep connection open

while (!_cts.Token.IsCancellationRequested) {

await Task.Delay(15000, _cts.Token);

// Heartbeat

await response.OutputStream.WriteAsync(Encoding.UTF8.GetBytes(": heartbeat\n\n"));

await response.OutputStream.FlushAsync();

try {

string endpointMsg = $"event: endpoint\ndata: http://localhost:{_port}/messages\n\n";

byte[] buffer = Encoding.UTF8.GetBytes(endpointMsg);

await response.OutputStream.WriteAsync(buffer, 0, buffer.Length);

await response.OutputStream.FlushAsync();

while (!_cts.Token.IsCancellationRequested) {

await Task.Delay(15000, _cts.Token);

byte[] heartbeatBuffer = Encoding.UTF8.GetBytes(": heartbeat\n\n");

await response.OutputStream.WriteAsync(heartbeatBuffer, 0, heartbeatBuffer.Length);

await response.OutputStream.FlushAsync();

}

} catch (OperationCanceledException) {

// Expected on shutdown or client disconnect; no additional handling required.

} catch (IOException) {

_loggerService.Information("MCP SSE client disconnected due to I/O error: {ClientId}", clientId);

} catch (ObjectDisposedException) {

_loggerService.Information("MCP SSE client disconnected because response was disposed: {ClientId}", clientId);

} finally {

HttpListenerResponse removedResponse;

if (_clients.TryRemove(clientId, out removedResponse)) {

removedResponse.Close();

} else {

response.Close();

}

Copilot · 2026-03-10T21:16:09Z

src/Spice86.Core/Emulator/InterruptHandlers/Dos/Xms/ExtendedMemoryManager.cs

+    /// <param name="handle">The XMS handle to search for.</param>
+    /// <param name="block">The XMS block if found.</param>
+    /// <returns>True if the block was found, false otherwise.</returns>
+    public bool TryGetBlock(int handle, [NotNullWhen(true)] out XmsBlock? block) {


Making TryGetBlock public expands the public API surface of ExtendedMemoryManager. If this is only needed for in-assembly MCP tooling, prefer internal (or another internal accessor) to avoid committing to a public API contract and reduce accidental external coupling.

Suggested change

public bool TryGetBlock(int handle, [NotNullWhen(true)] out XmsBlock? block) {

internal bool TryGetBlock(int handle, [NotNullWhen(true)] out XmsBlock? block) {

Copilot · 2026-03-10T21:16:09Z

src/Spice86.Core/Emulator/Mcp/McpServer.cs

+            } catch (Exception ex) when (!IsFatalException(ex)) {
+                _loggerService.Error(ex, "Unexpected error executing tool {ToolName}", toolName);
+                return CreateErrorResponse(id, -32603, $"Internal error: {ex.Message}");
+            }
+        }
+    }
+
+    private static bool IsFatalException(Exception ex) =>
+        ex is OutOfMemoryException or
+        StackOverflowException or
+        ThreadAbortException or
+        AccessViolationException;
+


This PR introduces a new JSON-RPC/MCP surface (request parsing, tool routing, and transports) but there are no tests validating protocol compliance (e.g., initialize/tools/list/tools/call success + error cases, schema correctness, and a couple of representative tool calls). Adding automated tests for McpServer.HandleRequest would help prevent regressions as the tool set grows.

Suggested change

} catch (Exception ex) when (!IsFatalException(ex)) {

_loggerService.Error(ex, "Unexpected error executing tool {ToolName}", toolName);

return CreateErrorResponse(id, -32603, $"Internal error: {ex.Message}");

}

}

}

private static bool IsFatalException(Exception ex) =>

ex is OutOfMemoryException or

StackOverflowException or

ThreadAbortException or

AccessViolationException;

} catch (JsonException ex) {

_loggerService.Error(ex, "JSON error executing tool {ToolName}", toolName);

return CreateErrorResponse(id, (int)JsonRpcErrorCode.InternalError, $"Internal error: {ex.Message}");

} catch (FormatException ex) {

_loggerService.Error(ex, "Formatting error executing tool {ToolName}", toolName);

return CreateErrorResponse(id, (int)JsonRpcErrorCode.InternalError, $"Internal error: {ex.Message}");

}

}

}

Copilot · 2026-03-10T21:16:09Z

src/Spice86.Core/Emulator/Mcp/McpServer.cs

+using System.Runtime.CompilerServices;
+
+using Spice86.Core.Emulator.Devices.Video;
+using Spice86.Core.Emulator.IOPorts;
+using Spice86.Core.Emulator.InterruptHandlers.Dos.Ems;
+using Spice86.Core.Emulator.InterruptHandlers.Dos.Xms;
+using Spice86.Core.Emulator.Mcp.Request;
+using Spice86.Core.Emulator.Mcp.Response;
+using Spice86.Core.Emulator.Mcp.Schema;
+using Spice86.Shared.Emulator.Memory;
+using Spice86.Shared.Utils;


This file has unused using directives (System.Runtime.CompilerServices, Spice86.Core.Emulator.Mcp.Request, Spice86.Shared.Utils). Since Spice86.Core has <TreatWarningsAsErrors>true</TreatWarningsAsErrors>, these will break the build (CS8019). Please remove the unused usings.

Suggested change

using System.Runtime.CompilerServices;

using Spice86.Core.Emulator.Devices.Video;

using Spice86.Core.Emulator.IOPorts;

using Spice86.Core.Emulator.InterruptHandlers.Dos.Ems;

using Spice86.Core.Emulator.InterruptHandlers.Dos.Xms;

using Spice86.Core.Emulator.Mcp.Request;

using Spice86.Core.Emulator.Mcp.Response;

using Spice86.Core.Emulator.Mcp.Schema;

using Spice86.Shared.Emulator.Memory;

using Spice86.Shared.Utils;

using Spice86.Core.Emulator.Devices.Video;

using Spice86.Core.Emulator.IOPorts;

using Spice86.Core.Emulator.InterruptHandlers.Dos.Ems;

using Spice86.Core.Emulator.InterruptHandlers.Dos.Xms;

using Spice86.Core.Emulator.Mcp.Response;

using Spice86.Core.Emulator.Mcp.Schema;

using Spice86.Shared.Emulator.Memory;

Copilot · 2026-03-10T21:16:10Z

src/Spice86.Core/Emulator/Mcp/Response/BreakpointInfo.cs

@@ -0,0 +1,12 @@
+namespace Spice86.Core.Emulator.Mcp.Response;
+
+using Spice86.Shared.Emulator.Memory;


The using Spice86.Shared.Emulator.Memory; directive is unused in this file. With <TreatWarningsAsErrors>true</TreatWarningsAsErrors> in Spice86.Core, this will fail the build (CS8019). Remove the unused using.

Suggested change

using Spice86.Shared.Emulator.Memory;

Copilot · 2026-03-10T21:16:10Z

src/Spice86.Core/Emulator/Mcp/McpHttpTransport.cs

+    private async Task BroadcastNotificationAsync(string json) {
+        byte[] buffer = Encoding.UTF8.GetBytes($"data: {json}\n\n");
+        foreach (KeyValuePair<Guid, HttpListenerResponse> client in _clients) {
+            try {
+                await client.Value.OutputStream.WriteAsync(buffer, 0, buffer.Length);
+                await client.Value.OutputStream.FlushAsync();
+            } catch {
+                // Client probably disconnected
+                _clients.TryRemove(client.Key, out _);


BroadcastNotificationAsync uses a bare catch { ... } and silently ignores all exceptions. This can hide real failures and makes diagnosing issues hard; it also removes clients without disposing/closing their responses. Catch specific expected exceptions (e.g., IOException, ObjectDisposedException) and ensure disconnected clients are cleaned up deterministically.

Suggested change

private async Task BroadcastNotificationAsync(string json) {

byte[] buffer = Encoding.UTF8.GetBytes($"data: {json}\n\n");

foreach (KeyValuePair<Guid, HttpListenerResponse> client in _clients) {

try {

await client.Value.OutputStream.WriteAsync(buffer, 0, buffer.Length);

await client.Value.OutputStream.FlushAsync();

} catch {

// Client probably disconnected

_clients.TryRemove(client.Key, out _);

private void RemoveClient(Guid clientId, HttpListenerResponse response) {

bool removed = _clients.TryRemove(clientId, out _);

if (!removed) {

return;

}

try {

response.OutputStream.Close();

} catch (IOException) {

} catch (ObjectDisposedException) {

}

try {

response.Close();

} catch (ObjectDisposedException) {

}

}

private async Task BroadcastNotificationAsync(string json) {

byte[] buffer = Encoding.UTF8.GetBytes($"data: {json}\n\n");

foreach (KeyValuePair<Guid, HttpListenerResponse> client in _clients) {

Guid clientId = client.Key;

HttpListenerResponse response = client.Value;

try {

await response.OutputStream.WriteAsync(buffer, 0, buffer.Length);

await response.OutputStream.FlushAsync();

} catch (IOException) {

RemoveClient(clientId, response);

} catch (ObjectDisposedException) {

RemoveClient(clientId, response);

Copilot · 2026-03-10T21:16:10Z

src/Spice86.Core/Emulator/Mcp/McpServer.cs

+        FunctionInfo[] functions = _functionCatalogue.FunctionInformations.Values
+            .OrderByDescending(f => f.CalledCount)
+            .Take(limit)


list_functions currently does .Take(limit), so a limit of 0 returns an empty list. This contradicts the request DTO docs (ListFunctionsRequest.Limit: “If zero is specified, returns all functions”) and is likely surprising to MCP clients. Treat limit <= 0 as “no limit” (or validate and reject negatives) and keep the tool behavior consistent with the documented contract.

Suggested change

FunctionInfo[] functions = _functionCatalogue.FunctionInformations.Values

.OrderByDescending(f => f.CalledCount)

.Take(limit)

IEnumerable<FunctionInformation> orderedFunctions = _functionCatalogue.FunctionInformations.Values

.OrderByDescending(f => f.CalledCount);

if (limit > 0) {

orderedFunctions = orderedFunctions.Take(limit);

}

FunctionInfo[] functions = orderedFunctions

Copilot · 2026-03-10T21:16:10Z

src/Spice86/Spice86DependencyInjection.cs

+using System.Diagnostics;
+


using System.Diagnostics; is unused in this file. With <TreatWarningsAsErrors>true</TreatWarningsAsErrors> in the project, this will fail the build (CS8019). Remove the unused using directive.

Copilot · 2026-03-10T21:16:10Z

src/Spice86/Spice86DependencyInjection.cs

+        McpServer mcpServer = new(memory, state, functionCatalogue, cfgCpu, 
+            ioPortDispatcher, vgaRenderer, vgaFunctionality, pauseHandler, dos.Ems, xms,
+            emulatorBreakpointsManager, loggerService);
+
+        if (loggerService.IsEnabled(LogEventLevel.Information)) {
+            loggerService.Information("MCP server created...");
+        }
+
+        McpStdioTransport mcpStdioTransport = new(mcpServer, loggerService);
+        mcpStdioTransport.Start();
+
+        McpHttpTransport mcpHttpTransport = new(mcpServer, loggerService);
+        mcpHttpTransport.Start();
+


MCP stdio + HTTP transports are started unconditionally during DI. This opens a localhost HTTP endpoint (with permissive CORS) and also starts a background thread blocked on stdin for every normal UI run, which has security/operational impact and can crash on port conflicts. Consider gating MCP server startup behind an explicit configuration/CLI flag, and handle HttpListener startup failures gracefully (disable MCP rather than failing app startup).

Copilot AI and others added 29 commits December 6, 2025 07:22

Initial plan

47e836d

Changes before error encountered

91b7c66

Co-authored-by: maximilien-noal <1087524+maximilien-noal@users.noreply.github.com>

Co-authored-by: maximilien-noal <1087524+maximilien-noal@users.norepl…

cdfb43a

…y.github.com>

Changes before error encountered

388d92e

Co-authored-by: maximilien-noal <1087524+maximilien-noal@users.noreply.github.com>

Merge branch 'master' into copilot/extract-mcp-server-changes

960b7bf

Update src/Spice86.Core/Emulator/Mcp/Response/IoPortWriteResponse.cs

5c0302d

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

Potential fix for code scanning alert no. 2083: Generic catch clause

f883c7f

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>

Update src/Spice86.Core/Emulator/Mcp/Response/IoPortReadResponse.cs

d02b6b1

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

Update src/Spice86.Core/Emulator/Mcp/McpServer.cs

26dc0a2

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

Merge branch 'master' into copilot/extract-mcp-server-changes

b3102d5

Merge branch 'master' into copilot/extract-mcp-server-changes

64f7a36

Merge branch 'copilot/extract-mcp-server-changes' into feature/mcp_se…

717f4dd

…rver

maximilien-noal self-assigned this Feb 28, 2026

maximilien-noal added the reverse engineering Related to reverse enginneering (features, APIs, ...) label Feb 28, 2026

github-advanced-security bot found potential problems Feb 28, 2026

View reviewed changes

maximilien-noal added 2 commits February 28, 2026 12:19

refactor: reverted some Core files

3669856

Signed-off-by: Maximilien Noal <noal.maximilien@gmail.com>

refactor: do not use try/catch or strings

bb6fd11

github-advanced-security bot found potential problems Feb 28, 2026

View reviewed changes

Merge branch 'master' into feature/mcp_server

eb455f0

maximilien-noal marked this pull request as ready for review March 10, 2026 21:07

Copilot AI review requested due to automatic review settings March 10, 2026 21:07

Copilot started reviewing on behalf of maximilien-noal March 10, 2026 21:07 View session

Copilot AI reviewed Mar 10, 2026

View reviewed changes

@@ -92,7 +92,9 @@
                             response.Close();
                         }
                     } catch (Exception ex) {
-                        _loggerService.Error(ex, "Error handling MCP HTTP request {Method} {Path}", request.HttpMethod, request.Url?.AbsolutePath);
+                        string safeMethod = (request.HttpMethod ?? string.Empty).Replace("\r", " ").Replace("\n", " ");
+                        string safePath = (request.Url?.AbsolutePath ?? string.Empty).Replace("\r", " ").Replace("\n", " ");
+                        _loggerService.Error(ex, "Error handling MCP HTTP request {Method} {Path}", safeMethod, safePath);
                         response.StatusCode = (int)HttpStatusCode.InternalServerError;
                         response.Close();
                     }

@@ -632,7 +632,7 @@
                         hostStorageProvider != null && textClipboard != null) {
                         IMessenger messenger = WeakReferenceMessenger.Default;
-                        mainWindowViewModel?.McpStatusViewModel = new McpStatusViewModel(mcpServer);
+                        mainWindowViewModel.McpStatusViewModel = new McpStatusViewModel(mcpServer);
                         BreakpointsViewModel breakpointsViewModel = new(
                             state, pauseHandler, messenger, emulatorBreakpointsManager, uiDispatcher, textClipboard, memory);

	public bool TryGetBlock(int handle, [NotNullWhen(true)] out XmsBlock? block) {
	internal bool TryGetBlock(int handle, [NotNullWhen(true)] out XmsBlock? block) {

		@@ -0,0 +1,12 @@
		namespace Spice86.Core.Emulator.Mcp.Response;

		using Spice86.Shared.Emulator.Memory;

-        FunctionInfo[] functions = _functionCatalogue.FunctionInformations.Values
-            .OrderByDescending(f => f.CalledCount)
-            .Take(limit)
+        IEnumerable<FunctionInformation> orderedFunctions = _functionCatalogue.FunctionInformations.Values
+            .OrderByDescending(f => f.CalledCount);
+        if (limit > 0) {
+            orderedFunctions = orderedFunctions.Take(limit);
+        }
+        FunctionInfo[] functions = orderedFunctions

Conversation

maximilien-noal commented Feb 28, 2026

Description of Changes

Rationale behind Changes

Suggested Testing Steps

Uh oh!

Check failure

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot Autofix

Check failure

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot Autofix

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Check notice

Copilot Autofix

Uh oh!

Uh oh!

Uh oh!

Check warning

Copilot Autofix

Check warning

Copilot Autofix

Check warning

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot Autofix

Check notice

Copilot Autofix

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!