Collapse nullable type arrays so tools work with Anthropic-API MCP clients#18
Open
thereisnotime wants to merge 5 commits into
Open
Collapse nullable type arrays so tools work with Anthropic-API MCP clients#18thereisnotime wants to merge 5 commits into
thereisnotime wants to merge 5 commits into
Conversation
Optional parameters (int?, string?, bool?) cause the MCP tool-schema generator to emit "type": ["integer", "null"]. The Anthropic API tool-schema converter (used by Claude Code and any Anthropic-API MCP client) rejects array-valued "type" with a 400 error, so every tool with an optional parameter is unusable from those clients. Add SchemaTransform with a WithToolsAndSchemaTransform<T> extension that registers each [McpServerTool] method via McpServerTool.Create with an AIJsonSchemaCreateOptions.TransformSchemaNode hook. The hook collapses a ["<type>", "null"] type array down to its single non-null scalar type. Since optional params are already absent from "required", dropping the redundant "null" is semantically identical. McpServer.cs now registers all eleven tool classes through the new extension instead of WithTools<T>(), because WithTools<T>() in ModelContextProtocol 1.2.0 does not expose SchemaCreateOptions.
📝 WalkthroughWalkthroughThis PR introduces two complementary improvements: a SchemaTransform utility that normalizes nullable JSON-schema types by collapsing ChangesSchema Transform for MCP Tools
Tool Execution Threading
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/SchemaTransform.cs`:
- Around line 76-84: The code uses the null-forgiving operator on r.Services
within the lambda function passed to AddSingleton, but Services is a nullable
property on RequestContext that could be null at runtime. Replace the current
approach where ActivatorUtilities.CreateInstance is called with r.Services! by
adding an explicit null check that throws an InvalidOperationException with a
descriptive message if Services is null, then uses the validated Services
property for the CreateInstance call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1deaa80d-cc22-4695-a870-f2a128cf0a29
📒 Files selected for processing (2)
src/McpServer.cssrc/SchemaTransform.cs
The nullable-type collapse alone was not enough: a `ulong x = ulong.MaxValue` parameter (ScanTool.stopAddress) emits "default": 18446744073709551615 in the generated JSON schema, which overflows the Anthropic tool-schema converter and fails the request with `400 ... input_schema: int too big to convert`, killing any turn that loads the scan tool. Extend the schema transform to drop numeric schema keywords (default, const, minimum, maximum, exclusive*, multipleOf) whose value falls outside signed 64-bit range. The method still applies its own default at call time and these are only optional hints/bounds, so removing them is semantically identical. Also address review feedback: guard the instance-tool factory against a null RequestContext.Services instead of using the null-forgiving operator, and add an XML doc comment to the public extension method.
Stamp AssemblyVersion/FileVersion so this fixed build is distinguishable from the stock 1.0.0 in file properties. Patch bump for the schema-converter fixes.
execute_lua and memory_scan ran lua.DoString / MemScan.Scan directly on the MCP worker thread. CE's Lua state and scan engine are not thread-safe: a nextScan over a large found list (or enum_memory_regions) racing CE's main thread caused an access violation that killed the plugin server and crashed Cheat Engine. Reproducible on big found lists (1.5M, 834k entries). Wrap both tools in Synchronize(...) so the work runs on CE's main GUI thread, the same marshalling reset_memory_scan already uses. Bump to 1.0.2.
… range CE's Lua state and engine internals are not thread-safe. Tools were running directly on the MCP worker thread, racing CE's main thread and crashing the process on heavy operations (notably nextScan over a large found list, and enum_memory_regions). - Add Tools.ToolThread.OnMainThread (Synchronize + error normalization) and route every CE-touching tool through it: aob_scan, read/write_memory, the memory-view and disassembly tools, symbol tools, assemble/auto_assemble, process tools, and convert_string. (Debugger dbg_* left as-is: flow-control ops wait on debug events on CE's main loop, so wrapping them risks deadlock.) - memory_scan: deinitialize the found list before each scan via the new CESDK MemScan.DeinitializeResults() (a found list left initialized over the memscan crashes CE on the next scan), and default stopAddress to 0x7FFFFFFFFFFFFFFF (ulong.MaxValue collapsed to -1 -> empty range -> 0 results). - Add get_plugin_version so the loaded build can be verified at runtime. The MemScan change lives in the CESDK submodule; .gitmodules is repointed to the thereisnotime/CESDK fork (branch fix/deinit-results-before-scan) so this builds. Bump to 1.0.4.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Tools/ScanTool.cs (1)
107-193: 💤 Low valueConsider using
ToolThread.OnMainThreadfor consistency withAobScan.
AobScanusesToolThread.OnMainThread()which internally wrapsSynchronizewith exception handling, whileMemoryScanusesSynchronize<object>()directly with an outer try-catch. Both work correctly, but using the shared helper would align with the established pattern and eliminate the repetitive try-catch boilerplate.♻️ Suggested simplification
if (!IsProcessAttached()) return new { success = false, error = "No process is attached. Please open a process first using 'open_process' tool." }; - try - { - bool isMainScanner = string.IsNullOrEmpty(scannerName); + bool isMainScanner = string.IsNullOrEmpty(scannerName); - // Run all scanner work on CE's main GUI thread... - return Synchronize<object>(() => - { + return ToolThread.OnMainThread(() => + { MemScan scanner = isMainScanner ? GetMainScanner() : GetOrCreateIndependentScanner(scannerName!); // ... rest of scanning logic unchanged ... return new { success = true, count, results, syncedWithUI = isMainScanner }; - }); - } - catch (Exception ex) - { - return new { success = false, error = ex.Message }; - } + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Tools/ScanTool.cs` around lines 107 - 193, The MemoryScan method uses Synchronize<object>() directly with an outer try-catch block for exception handling, while the AobScan method uses ToolThread.OnMainThread() which provides the same synchronization with built-in exception handling. Replace the Synchronize<object>(() => {...}) call with ToolThread.OnMainThread() to match the established pattern and remove the outer try-catch block since ToolThread.OnMainThread() handles exceptions internally. This eliminates the repetitive try-catch boilerplate while maintaining the same functionality and consistency across the codebase.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Tools/MemoryTool.cs`:
- Around line 34-36: Move the IsProcessAttached() check from before the
OnMainThread call into inside the OnMainThread lambda for both the ReadMemory
and WriteMemory methods in MemoryTool.cs. This ensures the thread-safety check
and the subsequent CE SDK operation execute atomically on the main thread,
consistent with the ProcessTool pattern, and eliminates the TOCTOU race
condition where the process could detach between the check and the operation.
---
Nitpick comments:
In `@src/Tools/ScanTool.cs`:
- Around line 107-193: The MemoryScan method uses Synchronize<object>() directly
with an outer try-catch block for exception handling, while the AobScan method
uses ToolThread.OnMainThread() which provides the same synchronization with
built-in exception handling. Replace the Synchronize<object>(() => {...}) call
with ToolThread.OnMainThread() to match the established pattern and remove the
outer try-catch block since ToolThread.OnMainThread() handles exceptions
internally. This eliminates the repetitive try-catch boilerplate while
maintaining the same functionality and consistency across the codebase.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ca23cc39-0155-46e5-bf5c-d64fc401ec94
📒 Files selected for processing (13)
.gitmodulesCESDKCeMCP.csprojsrc/Tools/AssemblyTool.cssrc/Tools/AutoAssemblyTool.cssrc/Tools/ConversionTool.cssrc/Tools/LuaExecutionTool.cssrc/Tools/MemoryTool.cssrc/Tools/MemoryViewTool.cssrc/Tools/ProcessTool.cssrc/Tools/ScanTool.cssrc/Tools/SymbolTool.cssrc/Tools/ToolThread.cs
✅ Files skipped from review due to trivial changes (2)
- .gitmodules
- CeMCP.csproj
Comment on lines
34
to
+36
| if (!IsProcessAttached()) | ||
| return new { success = false, error = "No process is attached. Please open a process first using 'open_process' tool." }; | ||
| try | ||
| return ToolThread.OnMainThread(() => |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if other tools also call IsProcessAttached or Process.GetOpenedProcessID outside OnMainThread
rg -n "IsProcessAttached|GetOpenedProcessID" --type csRepository: ShadowNineX/ce-mcp
Length of output: 714
🏁 Script executed:
#!/bin/bash
# Get full context of MemoryTool.cs ReadMemory method
sed -n '20,70p' src/Tools/MemoryTool.csRepository: ShadowNineX/ce-mcp
Length of output: 2790
🏁 Script executed:
#!/bin/bash
# Get full context of MemoryTool.cs WriteMemory method
sed -n '75,115p' src/Tools/MemoryTool.csRepository: ShadowNineX/ce-mcp
Length of output: 2202
🏁 Script executed:
#!/bin/bash
# Check ScanTool.cs IsProcessAttached and its call sites
sed -n '25,35p' src/Tools/ScanTool.cs
sed -n '60,110p' src/Tools/ScanTool.csRepository: ShadowNineX/ce-mcp
Length of output: 3510
🏁 Script executed:
#!/bin/bash
# Check AssemblyTool to see the pattern the review claims it follows
rg -A 15 "def Execute" src/Tools/AssemblyTool.cs | head -40Repository: ShadowNineX/ce-mcp
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Check ToolThread.OnMainThread implementation
rg -B 5 -A 10 "OnMainThread" src/ToolThread.cs | head -50Repository: ShadowNineX/ce-mcp
Length of output: 120
🏁 Script executed:
#!/bin/bash
# Find ToolThread definition
find src -name "*.cs" -type f | xargs grep -l "class ToolThread\|namespace.*ToolThread"Repository: ShadowNineX/ce-mcp
Length of output: 86
🏁 Script executed:
#!/bin/bash
# Find OnMainThread method
rg -B 2 -A 10 "OnMainThread" src --type csRepository: ShadowNineX/ce-mcp
Length of output: 26378
🏁 Script executed:
#!/bin/bash
# Check if AssemblyTool exists and its pattern
fd "AssemblyTool" src --type fRepository: ShadowNineX/ce-mcp
Length of output: 118
🏁 Script executed:
#!/bin/bash
# Search for Process.GetOpenedProcessID definition or comments about thread-safety
rg -B 3 -A 3 "GetOpenedProcessID|Process\\.GetOpenedProcessID" src --type csRepository: ShadowNineX/ce-mcp
Length of output: 1194
🏁 Script executed:
#!/bin/bash
# Read full ToolThread.cs
cat -n src/Tools/ToolThread.csRepository: ShadowNineX/ce-mcp
Length of output: 1336
🏁 Script executed:
#!/bin/bash
# Check ProcessTool GetCurrentProcess to see the pattern used there
sed -n '102,125p' src/Tools/ProcessTool.csRepository: ShadowNineX/ce-mcp
Length of output: 997
🏁 Script executed:
#!/bin/bash
# Search for any comments about thread-safety in the codebase
rg -i "thread|synchronize|main.thread" src/Tools/ToolThread.cs --context 3Repository: ShadowNineX/ce-mcp
Length of output: 985
Move IsProcessAttached() check inside OnMainThread for consistency with ProcessTool pattern.
Process.GetOpenedProcessID() should execute on the main thread per the established pattern in ProcessTool.GetCurrentProcess(), which calls it inside OnMainThread. Moving the check inside the block ensures consistency and aligns with the thread-marshaling design where CE SDK operations run on the main thread. Additionally, this eliminates a TOCTOU race where the process could detach between check and operation.
Apply to both methods
- if (!IsProcessAttached())
- return new { success = false, error = "No process is attached. Please open a process first using 'open_process' tool." };
-
return ToolThread.OnMainThread(() =>
{
+ if (!IsProcessAttached())
+ return new { success = false, error = "No process is attached. Please open a process first using 'open_process' tool." };
+
if (string.IsNullOrWhiteSpace(address))Also applies to: WriteMemory method (lines 79-82)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Tools/MemoryTool.cs` around lines 34 - 36, Move the IsProcessAttached()
check from before the OnMainThread call into inside the OnMainThread lambda for
both the ReadMemory and WriteMemory methods in MemoryTool.cs. This ensures the
thread-safety check and the subsequent CE SDK operation execute atomically on
the main thread, consistent with the ProcessTool pattern, and eliminates the
TOCTOU race condition where the process could detach between the check and the
operation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Every tool that has an optional parameter (
int?,string?,bool?) is currently unusable from Claude Code and any other Anthropic-API MCP client.Optional parameters make the MCP tool-schema generator emit a nullable type as a JSON array, e.g. for
read_memory:The Anthropic API tool-schema converter rejects array-valued
"type"with a400error, so the entire server fails to register on connect. Affected tools includeread_memory,write_memory, and most others — anything with an optional arg.Fix
Add
src/SchemaTransform.cswith aWithToolsAndSchemaTransform<T>()builder extension. It registers each[McpServerTool]method viaMcpServerTool.Create(...)with anAIJsonSchemaCreateOptions.TransformSchemaNodehook that collapses a["<type>", "null"]type array down to its single non-null scalar type:Optional parameters are already omitted from
"required", so dropping the redundant"null"is semantically identical — it only changes the serialization the Anthropic converter chokes on.src/McpServer.csnow registers all eleven tool classes through the new extension instead ofWithTools<T>(). This is necessary becauseWithTools<T>()inModelContextProtocol1.2.0 hardcodes itsMcpServerToolCreateOptionsand does not exposeSchemaCreateOptions, so the transform hook cannot be supplied through it.Verification
dotnet build -c Releasesucceeds with 0 warnings / 0 errors.read_memorytool through the new options and dumps the generated input schema. Before the fixbyteCount/maxLengthserialize as["integer", "null"]; after the fix both are scalar"integer", and"required"is unchanged (["address", "dataType"]).Notes
"type"that is not a 2-element[scalar, "null"]array.Summary by CodeRabbit