Conversation
WalkthroughThis PR replaces the OWIN startup/DI wrapper with a new McpServer host, refactors controllers and tools to SDK-backed implementations, renames and expands public models/APIs (AOB, Disassembler, MemScan), migrates the config UI from Page to Window, introduces several new SDK helpers, updates project settings, and extends the Python client. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Plugin as McpPlugin
participant UI as ConfigWindow
participant Server as McpServer
participant OWIN as OWIN WebApp
participant API as Web API Controllers
User->>Plugin: Toggle start
Plugin->>Server: Start(ConfigBaseUrl)
Server->>OWIN: WebApp.Start<McpServer>(...)
OWIN-->>API: Route + Swagger configured
Plugin-->>UI: Update status (Running)
User->>Plugin: Toggle stop
Plugin->>Server: Stop()
Server-->>OWIN: Dispose
Plugin-->>UI: Update status (Stopped)
sequenceDiagram
autonumber
actor Client
participant Controller as CheatEngineController
participant Tool as MemScanTool
participant SDK as CESDK.MemScan
participant FL as FoundList
Client->>Controller: POST /api/cheatengine/memscan (MemScanScanRequest)
Controller->>Tool: Scan(request)
Tool->>SDK: Scan(parameters)
SDK-->>Tool: scan started/completed
Tool->>SDK: GetFoundList()
SDK-->>Tool: FoundList
Tool->>FL: Initialize/Count/Iterate
FL-->>Tool: Address/Value pairs
Tool-->>Controller: MemScanResponse(Results)
Controller-->>Client: 200 OK + JSON
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
dosubot
bot
added
Lua
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 27
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
mcp-client/cheat_engine_mcp_server.py (1)
31-45: Harden make_request: explicit httpx timeouts, endpoint sanitize, and JSON-parse error handling
- Use httpx.Timeout to keep connect short while allowing long-running scans.
- Strip leading slashes to avoid accidental double “//”.
- Currently, a non-JSON response will raise and bubble up; catch ValueError to return a structured error.
Apply this diff:
@@ - url = f"{CHEAT_ENGINE_BASE_URL}{API_BASE_PATH}/{endpoint}" + endpoint = endpoint.lstrip("/") + url = f"{CHEAT_ENGINE_BASE_URL}{API_BASE_PATH}/{endpoint}" @@ - async with httpx.AsyncClient() as client: + async with httpx.AsyncClient(timeout=httpx.Timeout(connect=5.0, read=600.0, write=600.0)) as client: try: if method.upper() == "POST": - response = await client.post(url, json=data, timeout=600.0) + response = await client.post(url, json=data) else: - response = await client.get(url, timeout=600.0) + response = await client.get(url) @@ response.raise_for_status() - return response.json() + return response.json() except httpx.RequestError as e: return {"success": False, "error": f"Request failed: {e}"} except httpx.HTTPStatusError as e: return {"success": False, "error": f"HTTP error: {e.response.status_code}"} + except ValueError as e: + # JSON decode error + return {"success": False, "error": f"Invalid JSON in response: {e}"}SDK/CEObjectWrapper.cs (1)
20-35: Do not throw from finalizers — convert to IDisposable and swallow finalizer errors
~CEObjectWrapper()throws whendestroyis missing. Throwing in a finalizer can terminate the process unpredictably. Implement the standard IDisposable pattern; in the finalizer path, never throw and be resilient to Lua failures.Follow-up implementation outside the changed range (example):
using System; using System.Diagnostics; namespace CESDK { class CEObjectWrapper : IDisposable { protected CESDKLua lua = CESDK.currentPlugin.sdk.lua; protected IntPtr CEObject; public IntPtr obj => CEObject; private bool _disposed; ~CEObjectWrapper() => Dispose(false); public void Dispose() { Dispose(true); GC.SuppressFinalize(this); } protected virtual void Dispose(bool disposing) { if (_disposed) return; try { if (CEObject != IntPtr.Zero) { lua.PushCEObject(CEObject); lua.PushString("destroy"); lua.GetTable(-2); if (lua.IsFunction(-1)) lua.PCall(0, 0); // best-effort } } catch (Exception ex) { Debug.WriteLine($"CEObjectWrapper.Dispose ignored error: {ex.Message}"); } finally { lua.SetTop(0); CEObject = IntPtr.Zero; _disposed = true; } } } }SDK/CESDK.cs (1)
176-186: Robust pointer parsing for both 32/64-bit and invalid input.ulong.Parse may throw; casting ulong to IntPtr can overflow on 32-bit. Use TryParse with platform-sized IntPtr construction and better errors.
- ulong a = ulong.Parse(parameters); + if (!ulong.TryParse(parameters, out var addr)) + return 0; // invalid parameters provided by host + + IntPtr pInit; + if (IntPtr.Size == 8) + pInit = new IntPtr(unchecked((long)addr)); + else + pInit = new IntPtr(unchecked((int)addr)); ... - Marshal.StructureToPtr<TPluginInit>(bla, (IntPtr)a, false); + Marshal.StructureToPtr(bla, pInit, false);SDK/CESDKLua.cs (2)
203-205: Bug: Replace(IntPtr L, ...) also ignores L.Use the provided state for both copy and pop.
- public void Replace(IntPtr L, int idx) { lua_copy(State, -1, idx); Pop(State, 1); } + public void Replace(IntPtr L, int idx) { lua_copy(L, -1, idx); Pop(L, 1); }
191-194: Remove unused parameter fromCopyoverloadWe verified that the only references to
Copy(in the entire codebase are the two overload definitions inSDK/CESDKLua.cs—no external call sites invoke the 3-argument overload. The leadingidxparameter is never passed tolua_copyand should be removed to match the Lua C API (and mirror the pattern used byRotate).• File: SDK/CESDKLua.cs (lines 192–193)
• No 3-argCopyusages found beyond these definitions.Recommended refactor:
- public int Copy(int idx, int fromidx, int toidx) { return lua_copy(State, fromidx, toidx); } + public int Copy(int fromidx, int toidx) { return lua_copy(State, fromidx, toidx); }Tools/ConversionTool.cs (2)
9-20: Null request handling is missingAccessing
request.Inputcan throw ifrequestis null. Add a guard.- public ConversionResponse Convert(ConversionRequest request) + public ConversionResponse Convert(ConversionRequest request) { try { + if (request == null) + { + return new ConversionResponse { Success = false, Error = "Request cannot be null" }; + } if (string.IsNullOrWhiteSpace(request.Input)) { return new ConversionResponse { Success = false, Error = "Input parameter is required" }; }
31-40: Over-restrictive type gate duplicates SDK mapping and blocks supported aliases
GetLuaFunctionhere only recognizesmd5, butStringConvertersupportsstringtomd5stringandstringtomd5as well. You’re rejecting valid inputs before delegating to the converter.Two options:
- Option A (simplest): remove the pre-check and let
StringConverter.ConvertStringvalidate types.- string luaFunction = GetLuaFunction(request.ConversionType); - - if (string.IsNullOrEmpty(luaFunction)) - { - return new ConversionResponse - { - Success = false, - Error = $"Unsupported conversion type: {request.ConversionType}" - }; - } + // Let the converter validate supported conversion types
- Option B: expand the mapping to match the SDK and keep the early error.
- private static string GetLuaFunction(string conversionType) + private static string GetLuaFunction(string conversionType) { string lowerType = conversionType.ToLower(); - if (lowerType == "ansitoutf8") + if (lowerType == "ansitoutf8") return "ansiToUtf8"; - else if (lowerType == "utf8toansi") + else if (lowerType == "utf8toansi") return "utf8ToAnsi"; - else if (lowerType == "md5") + else if (lowerType == "md5" || lowerType == "stringtomd5string" || lowerType == "stringtomd5") return "stringToMD5String"; else return null; }If you choose Option A, you can delete
GetLuaFunctionentirely.Also applies to: 61-71
🧹 Nitpick comments (74)
mcp-client/cheat_engine_mcp_server.py (3)
36-39: Timeout increase is fine; consider per-phase tuning (short connect, long read).You bumped to 600s for both GET/POST. That’s appropriate for long scans, but keep connect short to fail fast on a dead CE server.
If you prefer not to change the context manager, minimally adjust per-call:
- response = await client.post(url, json=data, timeout=600.0) + response = await client.post(url, json=data, timeout=httpx.Timeout(connect=5, read=600, write=600))
751-752: Docstring wording nit: “blocks” → “awaits.”This is an async tool; it awaits the backend. Consider rephrasing to avoid confusion.
- This function blocks and waits until the scan is complete before returning. + This function awaits the backend until the scan completes before returning.
846-867: Guard memscan_results pagination (negative indices, unbounded counts).
- Validate start_index >= 0 and count > 0.
- Impose an upper bound (env-tunable) to avoid huge payloads.
@@ async def memscan_results(scan_id: str, start_index: int = 0, count: int = 100) -> Dict[str, Any]: @@ - return await make_request("memscan-results", "POST", { - "scanId": scan_id, - "startIndex": start_index, - "count": count - }) + if start_index < 0: + return {"success": False, "error": "start_index must be >= 0"} + if count <= 0: + return {"success": False, "error": "count must be > 0"} + max_results = int(os.getenv("MCP_MEMSCAN_MAX_RESULTS", "5000")) + count = min(count, max_results) + + payload = { + "scanId": scan_id, + "startIndex": start_index, + "count": count + } + return await make_request("memscan-results", "POST", payload)SDK/MemoryRead.cs (3)
11-52: Return byte[] for ReadBytes for correctness and API ergonomicsLua returns byte values in [0..255]. Returning
int[]is surprising and wastes memory. Returnbyte[]and clamp cast safely.Apply:
- public int[] ReadBytes(string address, int byteCount, bool returnAsTable = true) + public byte[] ReadBytes(string address, int byteCount, bool returnAsTable = true) { try { lua.GetGlobal("readBytes"); @@ - var bytes = new List<int>(); + var bytes = new List<byte>(); if (lua.IsTable(-1)) { int tableLength = lua.ObjLen(-1); for (int i = 1; i <= tableLength; i++) { lua.PushInteger(i); lua.GetTable(-2); if (lua.IsNumber(-1)) { - bytes.Add((int)lua.ToNumber(-1)); + // Lua numbers are doubles; cast via int then unchecked to byte + var n = (int)lua.ToNumber(-1); + bytes.Add(unchecked((byte)n)); } lua.Pop(1); } } @@ - return bytes.ToArray(); + return bytes.ToArray();
26-33: Remove unreachable PCall error checks and prefer finally for stack cleanup
lua.PCall(...)already throws on failure per your CESDKLua wrapper; the subsequentif (result != 0)blocks are unreachable. Also, prefer afinallyto guaranteelua.SetTop(0)even when exceptions are thrown early.Example refactor for one method (apply similarly to others):
- int result = lua.PCall(2, 1); - if (result != 0) - { - string error = lua.ToString(-1); - lua.SetTop(0); - throw new InvalidOperationException($"readInteger call failed: {error}"); - } - - int value = (int)lua.ToInteger(-1); - lua.SetTop(0); - return value; + try + { + lua.PCall(2, 1); // throws on error + return (int)lua.ToInteger(-1); + } + finally + { + lua.SetTop(0); + }Also applies to: 77-84, 112-118, 147-153, 188-194
63-76: Confirm readInteger signature — is the ‘signed’ flag actually supported?Cheat Engine’s Lua API commonly exposes
readInteger(address); asignedflag may not be part of that signature (varies by build/extensions). If it’s unsupported it will be ignored, but the extra arg adds noise.If not supported in your target CE version, remove the parameter and overload it at the SDK level instead.
If you want, I can adjust the method signatures to match the exact CE Lua APIs you target and add unit tests against a mocked
CESDKLua.McpServer.cs (3)
14-29: OWIN Startup is correct; consider adding CORS and JSON-only formatting.Configuration is minimal and fine. If you expect browser clients, enable CORS and consider removing XML to avoid content-negotiation surprises.
Apply:
using Swashbuckle.Application; +using Microsoft.Owin.Cors; +using System.Net.Http.Formatting; public void Configuration(IAppBuilder appBuilder) { var config = new HttpConfiguration(); config.MapHttpAttributeRoutes(); config.Routes.MapHttpRoute( name: "DefaultApi", routeTemplate: "api/{controller}/{id}", defaults: new { id = RouteParameter.Optional } ); config.EnableSwagger(c => c.SingleApiVersion("v1", "Cheat Engine MCP API")) .EnableSwaggerUi(); - appBuilder.UseWebApi(config); + // Optional hardenings: + appBuilder.UseCors(CorsOptions.AllowAll); + config.Formatters.Clear(); + config.Formatters.Add(new JsonMediaTypeFormatter()); + appBuilder.UseWebApi(config); }
31-35: Gracefully handle invalid/occupied baseUrl when starting.WebApp.Start will throw for malformed URLs or occupied ports. Wrap with basic validation and exception to help callers.
public void Start(string baseUrl) { - if (_webApp != null) return; - _webApp = WebApp.Start<McpServer>(url: baseUrl); + if (_webApp != null) return; + if (!Uri.TryCreate(baseUrl, UriKind.Absolute, out var _) || string.IsNullOrWhiteSpace(baseUrl)) + throw new ArgumentException("Invalid baseUrl", nameof(baseUrl)); + try + { + _webApp = WebApp.Start<McpServer>(url: baseUrl); + } + catch (Exception ex) + { + // Surface actionable start failure + throw new InvalidOperationException($"Failed to start MCP server at {baseUrl}: {ex.Message}", ex); + } }
37-44: Start/Stop are not thread-safe; benign but easy to harden.Concurrent Start/Stop could race. A simple lock prevents odd states.
public class McpServer : IDisposable { - private IDisposable _webApp; + private IDisposable _webApp; + private readonly object _gate = new object(); public void Start(string baseUrl) { - if (_webApp != null) return; - _webApp = WebApp.Start<McpServer>(url: baseUrl); + lock (_gate) + { + if (_webApp != null) return; + _webApp = WebApp.Start<McpServer>(url: baseUrl); + } } public void Stop() { - _webApp?.Dispose(); - _webApp = null; + lock (_gate) + { + _webApp?.Dispose(); + _webApp = null; + } } public bool IsRunning => _webApp != null;SDK/CESDK.cs (3)
155-156: Prefer IntPtr.Zero over numeric cast for clarity and platform safety.- if ((long)PluginNamePtr == 0) + if (PluginNamePtr == IntPtr.Zero)
157-166: Minor perf/readability: avoid LINQ Count() on an array in a for-loop.- Type[] x = typeof(CESDKPluginClass).Assembly.GetTypes(); - - for (int i = 0; i < x.Count(); i++) + var types = typeof(CESDKPluginClass).Assembly.GetTypes(); + for (int i = 0; i < types.Length; i++) { - if (x[i].IsSubclassOf(typeof(CESDKPluginClass))) + if (types[i].IsSubclassOf(typeof(CESDKPluginClass))) { - currentPlugin = (CESDKPluginClass)Activator.CreateInstance(x[i]); + currentPlugin = (CESDKPluginClass)Activator.CreateInstance(types[i]); break; } }
118-135: Guard against missing exports to avoid NullReferenceException later.If CESDKLua fails to wire delegates, later calls will NRE. Add simple validation.
//setup the delegates if (delProcessMessages == null) delProcessMessages = Marshal.GetDelegateForFunctionPointer<delegateProcessMessages>(pluginexports.ProcessMessages); if (delCheckSynchronize == null) delCheckSynchronize = Marshal.GetDelegateForFunctionPointer<delegateCheckSynchronize>(pluginexports.CheckSynchronize); if (lua == null) lua = new CESDKLua(this); currentPlugin.sdk = this; - return currentPlugin.EnablePlugin(); + if (currentPlugin == null) + return false; + return currentPlugin.EnablePlugin();ServerConfig.cs (3)
9-13: Build ConfigBaseUrl via UriBuilder to handle IPv6 and accidental schemes.Current interpolation breaks for IPv6 or when MCP_HOST includes a scheme. UriBuilder also validates.
- public static string ConfigBaseUrl => $"http://{ConfigHost}:{ConfigPort}"; + public static string ConfigBaseUrl + { + get + { + var host = ConfigHost?.Trim() ?? "127.0.0.1"; + if (host.StartsWith("http://", StringComparison.OrdinalIgnoreCase)) + host = host.Substring("http://".Length); + if (host.StartsWith("https://", StringComparison.OrdinalIgnoreCase)) + host = host.Substring("https://".Length); + var ub = new UriBuilder(Uri.UriSchemeHttp, host, ConfigPort); + return ub.Uri.ToString().TrimEnd('/'); + } + }
18-27: Support MCP_SERVER_NAME and sanitize host from env.public static void LoadFromEnvironment() { var hostEnv = Environment.GetEnvironmentVariable("MCP_HOST"); if (!string.IsNullOrEmpty(hostEnv)) - ConfigHost = hostEnv; + ConfigHost = hostEnv.Trim(); var portEnv = Environment.GetEnvironmentVariable("MCP_PORT"); if (!string.IsNullOrEmpty(portEnv) && int.TryParse(portEnv, out int port)) ConfigPort = port; + + var nameEnv = Environment.GetEnvironmentVariable("MCP_SERVER_NAME"); + if (!string.IsNullOrEmpty(nameEnv)) + ConfigServerName = nameEnv.Trim(); }
47-50: Remove placeholder comments; optionally log exceptions.“Hi error / Goodbye error” reads like leftover debug. Either log or drop.
- // Hi error - // If loading fails, use defaults - // Goodbye error + // If loading fails, fall back to defaults + // Consider logging the exception for diagnostics.SDK/CESDKLua.cs (1)
417-441: Handle missing lua53 DLLs explicitly; avoid silent null delegates.If neither DLL loads, subsequent calls will NRE. Fail fast with a clear exception, or attempt to use CE-exported Lua if available.
- IntPtr hLibLua = LoadLibraryA("lua53-32.dll"); + IntPtr hLibLua = LoadLibraryA("lua53-32.dll"); if (hLibLua == IntPtr.Zero) hLibLua = LoadLibraryA("lua53-64.dll"); if (hLibLua != IntPtr.Zero) { // load functions... } + else + { + throw new DllNotFoundException("Failed to load lua53-32.dll or lua53-64.dll required for CESDKLua."); + }Optionally store hLibLua and FreeLibrary on plugin unload.
Models/LuaModels.cs (4)
67-71: Avoid nested ByteCount object; use a nullable int for simpler JSON.Nested { "byteCount": { "value": 4 } } is clunky and inconsistent with MemoryReadRequest. Align with a plain nullable int.
- public class ByteCount - { - // Represents the number of bytes to write - public int Value { get; set; } - } + // Remove this type and use int? in MemoryWriteRequestNote: See change suggestion below in MemoryWriteRequest.
73-93: Unify ByteCount type with MemoryReadRequest and add minimal validation hints.Keep API consistent and make RequestType an enum to avoid stringly-typed errors.
public class MemoryWriteRequest { // Memory address to write to public string Address { get; set; } // Value to write (can be int, long, float, string, etc.) public object Value { get; set; } // Data type to write ("bytes", "integer", "float", "string", etc.) public string DataType { get; set; } - // Only used when writing bytes - public ByteCount ByteCount { get; set; } + // Only used when writing bytes + public int? ByteCount { get; set; } // Only used when writing strings public int? MaxLength { get; set; } public bool? WideChar { get; set; } // Optional for integer writes public bool? Signed { get; set; } }Upstream tool (MemoryWriteTool) already validates DataType; consider adding bounds checks for ByteCount/MaxLength in that tool.
172-199: ResultList cap and API are good; consider IEnumerable exposure.Capped storage prevents large payloads. Optionally expose IEnumerable for easier iteration.
public class ResultList { private readonly List<ResultItem> _items = new List<ResultItem>(); private const int MaxStored = 1000; + public IEnumerable<ResultItem> Items => _items;
125-134: Use an enum forDisassemblerRequest.RequestTypeSwitching on a lower-cased string is brittle and risks runtime errors if the request type is misspelled or casing differs. Defining a dedicated
enummakes the intent explicit, enables compile-time checking, and simplifies the switch inDisassembleTool.• In Models/LuaModels.cs (lines 125–134):
– Define a newDisassemblerRequestTypeenum.
– ChangeRequestTypefromstringtoDisassemblerRequestType, with a sensible default.• In Tools/DisassembleTool.cs (around line 25):
– Replace theswitch (request.RequestType?.ToLower())on string values
– With aswitch (request.RequestType)on the enum, updating eachcaseto use the new enum members.--- Models/LuaModels.cs +++ Models/LuaModels.cs @@ - public class DisassemblerRequest - { - public string RequestType { get; set; } // disassemble, get-instruction-size - public string Address { get; set; } - } + public enum DisassemblerRequestType + { + Disassemble, + GetInstructionSize + } + + public class DisassemblerRequest + { + public DisassemblerRequestType RequestType { get; set; } = DisassemblerRequestType.Disassemble; + public string Address { get; set; } + }--- Tools/DisassembleTool.cs +++ Tools/DisassembleTool.cs @@ - switch (request.RequestType?.ToLower()) - { - case "get-instruction-size": - result = DisassembleTool.GetInstructionLength(request.Address).ToString(); - break; - default: - result = DisassembleTool.Disassemble(request.Address); - break; - } + switch (request.RequestType) + { + case DisassemblerRequestType.GetInstructionSize: + result = DisassembleTool.GetInstructionLength(request.Address).ToString(); + break; + case DisassemblerRequestType.Disassemble: + default: + result = DisassembleTool.Disassemble(request.Address); + break; + }SDK/MemScan.cs (5)
136-136: Guard style is fine; consider consistent exception types.The direct throw is consistent with neighboring code. If you touch this later, consider a more specific exception (e.g., InvalidOperationException) for consistency with new SDK wrappers.
232-235: Fix mismatched error text: expecting createMemScan, message says createFoundList.The constructor fetches "createMemScan" but the error mentions "createFoundList", which will mislead users when troubleshooting.
- if (lua.IsNil(-1)) - throw new System.ApplicationException("You have no createFoundList (WTF)"); + if (lua.IsNil(-1)) + throw new System.ApplicationException("You have no createMemScan (WTF)");
252-257: Strengthen diagnostics on unexpected return type.The generic "No idea what it returned" makes triage harder. Include the Lua type name or stack dump to aid debugging.
- else - throw new System.ApplicationException("No idea what it returned"); + else + { + var typeInfo = lua.ToString(-1) ?? $"type={lua.TypeName(-1)}"; + throw new System.ApplicationException($"createMemScan returned unexpected type/value: {typeInfo}"); + }(Adjust if TypeName helper isn’t available; any contextual info helps.)
118-122: Ensure FoundList Is Initialized or Offer OverloadVerification shows that within the codebase, the only usage of GetFoundList is in Tools/MemScanTool.cs (line 39), and it correctly calls Initialize() before accessing Count/Address/Value:
- Tools/MemScanTool.cs:39 →
var foundList = memScan.GetFoundList();- Tools/MemScanTool.cs:40 →
foundList.Initialize();However, because GetFoundList() is part of the public SDK surface and FoundList requires Initialize() to populate its data, callers could easily forget to invoke Initialize() and end up with an empty result set.
To improve the API’s usability and reduce the risk of misuse, consider one of these optional refactors:
• Option A (auto-initialize by default):
- public FoundList GetFoundList() - { - return new FoundList(this); - } + public FoundList GetFoundList() + { + var fl = new FoundList(this); + fl.Initialize(); + return fl; + }• Option B (add overload without breaking existing behavior):
+ public FoundList GetFoundList(bool initialize) + { + var fl = new FoundList(this); + if (initialize) fl.Initialize(); + return fl; + }Either approach guards against the common pitfall of an uninitialized FoundList while preserving backward compatibility.
214-216: Use the callback’s Lua state (L) for stack operationsInside the Lua C callback, calls like GetTop() and ToInteger(idx) default to the global state. To ensure this handler remains correct if multiple Lua states are ever in use, switch to the overloads that accept the callback’s local L pointer:
• SDK/MemScan.cs (around lines 214–216):
- if (lua.GetTop() >= 4) - OnGuiUpdate(this, (UInt64)lua.ToInteger(2), (UInt64)lua.ToInteger(3), (UInt64)lua.ToInteger(4)); + if (lua.GetTop(L) >= 4) + OnGuiUpdate(this, + (UInt64)lua.ToInteger(L, 2), + (UInt64)lua.ToInteger(L, 3), + (UInt64)lua.ToInteger(L, 4));These IntPtr-overloads (e.g. ToInteger(IntPtr L, int idx)) are already exposed in CESDKLua, so you can use them directly. If, for any reason, your version lacked those overloads, at least update the GetTop call to GetTop(L).
Models/ConfigurationModel.cs (2)
86-97: Harden status handling: null-safety, culture, and single normalization.
- value.ToLower() will throw on null and is culture-sensitive.
- You compute lowerStatus once; reuse it below as well.
- // Update color based on status - string lowerStatus = value.ToLower(); + // Update color based on status + string lowerStatus = (value ?? string.Empty).ToLowerInvariant(); if (lowerStatus == "running") ServerStatusColor = Brushes.Green; else if (lowerStatus == "stopped") ServerStatusColor = Brushes.Red; else if (lowerStatus == "starting" || lowerStatus == "stopping") ServerStatusColor = Brushes.Orange; else ServerStatusColor = Brushes.Gray;Also update IsServerRunning below to reuse lowerStatus and avoid another ToLower call.
98-100: Reuse lowerStatus and use OrdinalIgnoreCase for Start/Stop label consistency.Avoid a second ToLower and prefer ordinal comparisons.
- IsServerRunning = value.ToLower() == "running"; + IsServerRunning = string.Equals(lowerStatus, "running", StringComparison.Ordinal); OnPropertyChanged(nameof(StartStopButtonText));Optionally, modernize StartStopButtonText:
- public string StartStopButtonText => ServerStatus.ToLower() == "running" ? "Stop Server" : "Start Server"; + public string StartStopButtonText => + string.Equals(ServerStatus, "running", StringComparison.OrdinalIgnoreCase) ? "Stop Server" : "Start Server";SDK/FoundList.cs (2)
100-114: Remove unused local and tighten constructor failure messages.int pcr is assigned but never used; and on failure you throw a generic message later.
- int pcr = lua.PCall(1, 1); + lua.PCall(1, 1);Optionally, include the actual Lua error (top of stack) in the exception thrown below, similar to other wrappers.
32-47: Minor: API ergonomics for GetAddress/GetValue.Returning literal "Error" complicates callers. Consider returning null and letting callers decide, or throw on unexpected shapes to be consistent with other wrappers.
- return "Error"; + return null;Document nullability if you make this change.
SDK/LuaExecution.cs (3)
28-36: Remove dead check after PCall; CESDKLua.PCall already throws on error.PCall throws on non-zero return, so result will always be 0 here. The extra error path is unreachable.
- // Execute the loaded chunk - int result = lua.PCall(0, -1); // -1 means return all results - if (result != 0) - { - string error = lua.ToString(-1); - lua.SetTop(0); - throw new InvalidOperationException($"Lua execution failed: {error}"); - } + // Execute the loaded chunk (-1: return all results) + lua.PCall(0, -1);
41-51: Multiple return values: only top-of-stack is returned.If you care about multiple returned values, consider concatenating them or returning a JSON array. If single-value semantics are intended, add a remark in XML docs.
- if (stackSize > 0) - { - // Get the top value from the stack as a string - returnValue = lua.ToString(-1); - lua.SetTop(0); - } + if (stackSize > 0) + { + // Example: collect all values (top to bottom) into a CSV + var parts = new string[stackSize]; + for (int i = 1; i <= stackSize; i++) + parts[i - 1] = lua.ToString(i); + returnValue = string.Join(",", parts); + lua.SetTop(0); + }Alternatively, keep current behavior and document “returns the stringified top value; returns '0' when no values”.
57-59: Consistent exception wrapping.Catching and resetting the stack before wrapping the error is good hygiene. Consider preserving the Lua error string verbatim in a dedicated property if you plan to surface structured errors later.
Tools/MemScanTool.cs (3)
47-51: Avoid duplicating the 1000 cap and consider indexing base
- Magic number:
ResultListalready caps storage at 1000; duplicating this constant here risks drift. Prefer a shared constant or loop bycountResultsand break whenresults.StoredCountreaches its cap.- Indexing:
FoundList.GetAddress/GetValueread Lua tables; ensure index base (0 vs 1) is correct. Current code starts at 0. If CE’s tables are 1-based here, you’ll miss the first item.- for (int i = 0; i < Math.Min(countResults, 1000); i++) + const int MaxReturn = 1000; // keep in sync with ResultList cap + for (int i = 0; i < countResults && i < MaxReturn; i++) { results.Add(foundList.GetAddress(i), foundList.GetValue(i)); }If
FoundListexpects 1-based indices, adjust tofor (int i = 1; i <= Math.Min(countResults, MaxReturn); i++). Confirm with the SDK implementation and I can update both sides.
8-11: Namespace/file-location inconsistencyThis tool lives under Tools/ but uses the
CESDKnamespace, whereas other tools useCeMCP.Tools. Either move it under the SDK namespace folder or change the namespace for consistency and discoverability.
31-34: Model/SDK mismatch: IsNotABinaryString is ignored
MemScanScanRequestexposesIsNotABinaryString, butScanParameters/MemScan.Scanforces this totrue. Either remove the request field, document that binary string scans aren’t supported, or plumb it through end-to-end.Tools/ThreadListTool.cs (1)
24-29: Consider returning an empty array instead of null on errorReturning
Array.Empty<string>()can simplify client code by avoiding null checks in error paths while still signaling failure viaSuccessandError.- ThreadList = null, + ThreadList = Array.Empty<string>(),Tools/ProcessTool.cs (2)
77-78: Surface an error message when opening failsWhen
OpenByPid/OpenByNamereturnsfalse, you returnSuccess = falsewithout context. Providing a message improves UX and parity with other methods.- return new BaseResponse { Success = success }; + return new BaseResponse + { + Success = success, + Error = success ? null : "Failed to open the specified process (check PID/name and permissions)." + };
1-8: Type name shadowing (optional)Using
CESDK.ProcessalongsideSystem.Diagnostics.Processcan be confusing in mixed contexts. Consider renamingCESDK.ProcesstoCeProcessor alias it locally:using CeProcess = CESDK.Process;.SDK/Disassembler.cs (2)
6-9: Namespace/file-location inconsistencyThis file lives under SDK/ but declares
namespace CeMCP.Tools. Align it with other SDK wrappers (CESDK) or move the file under Tools/. This reduces confusion during navigation and DI/registration.
20-34: Broaden accepted address types (optional)Consider adding
ulongandIntPtrsupport in both methods to cover more callers without forcing casts.Also applies to: 51-66
ConfigWindow.xaml (3)
205-209: Port field accepts arbitrary text; enforce numeric-only + validation.Prevent invalid port inputs early. Minimal XAML changes plus small code-behind will do.
Apply:
-<TextBox Grid.Row="2" - Grid.Column="1" - x:Name="PortTextBox" - Text="{Binding Port, UpdateSourceTrigger=PropertyChanged}" - Margin="5,2"/> +<TextBox Grid.Row="2" + Grid.Column="1" + x:Name="PortTextBox" + Text="{Binding ConfigPort, UpdateSourceTrigger=PropertyChanged}" + PreviewTextInput="Port_PreviewTextInput" + DataObject.Pasting="Port_Pasting" + Margin="5,2"/>And in ConfigWindow.xaml.cs (supporting snippet):
// Add once in code-behind using System.Text.RegularExpressions; using System.Windows; using System.Windows.Input; private static readonly Regex Digits = new(@"^\d+$"); private void Port_PreviewTextInput(object sender, TextCompositionEventArgs e) { e.Handled = !Digits.IsMatch(e.Text); } private void Port_Pasting(object sender, DataObjectPastingEventArgs e) { if (e.DataObject.GetDataPresent(DataFormats.Text)) { string text = e.DataObject.GetData(DataFormats.Text)?.ToString() ?? ""; if (!Digits.IsMatch(text)) e.CancelCommand(); } else e.CancelCommand(); }
14-160: App-defined styles are unused; wire them to controls or remove.You declare DarkTextBoxStyle/LightTextBoxStyle and DarkButtonStyle/LightButtonStyle but do not apply them, leaving the UI visually inconsistent.
Option A (quick): apply style to buttons now; switch at runtime based on theme in code-behind.
-<Button x:Name="SaveButton" +<Button x:Name="SaveButton" + Style="{StaticResource DarkButtonStyle}" Content="Save Configuration" Click="SaveButton_Click" Padding="12,8" MinWidth="120"/>Repeat for TestButton, StartStopButton, OpenSwaggerButton. If you detect theme in code, you can set Button.Style = (Style)FindResource("LightButtonStyle") accordingly.
Option B (cleaner): set a default Button style via a merged ResourceDictionary and avoid per-control Style attributes. I can draft that if you want.
Also applies to: 254-275
1-12: Duplicate title (Window.Title and a header TextBlock).You show “MCP Server Configuration” twice. Consider removing the header TextBlock or using it for additional context (e.g., environment/process) to avoid redundancy.
-<TextBlock x:Name="TitleTextBlock" - Grid.Row="0" - Grid.Column="0" - Grid.ColumnSpan="2" - Text="MCP Server Configuration" - FontSize="16" - FontWeight="Bold" - Margin="0,0,0,20"/> +<!-- Optional: keep only Window.Title -->Also applies to: 180-187
SDK/Converter.cs (1)
2-2: Remove unused using.System.Collections.Generic isn’t used in this file.
-using System.Collections.Generic;Tools/MemoryReadTool.cs (3)
36-36: Use ToLowerInvariant for culture-independent dispatch.String casing for type keys should be invariant to avoid locale surprises.
-switch (request.DataType.ToLower()) +switch (request.DataType.ToLowerInvariant())
33-35: Constructing MemoryRead per call; consider injecting for testability/perf.Creating MemoryRead for every request adds overhead and makes unit testing harder.
Option:
- Add a field and ctor:
private readonly MemoryRead _memoryRead; public MemoryReadTool(MemoryRead memoryRead = null) => _memoryRead = memoryRead ?? new MemoryRead();
- Then replace local var with the field:
-var memoryRead = new MemoryRead(); -object value; +var memoryRead = _memoryRead; +object value;
95-103: Error messaging: include inner exception safely.Returning only ex.Message can hide root cause. Consider aggregating inner messages while avoiding stack traces in user responses.
-Error = ex.Message +Error = ex.InnerException != null ? $"{ex.Message} ({ex.InnerException.Message})" : ex.MessageTools/AOBScanTool.cs (1)
9-9: API naming consistency (optional).Class is AobScanTool and models are AobScan*, but method is AOBScan. Consider renaming to AobScan for consistency (update call sites accordingly).
SDK/ThreadList.cs (1)
97-110: Align destroy-call pattern with existing SDK usage (safer stack discipline).Elsewhere (SDK/AOB.cs) you duplicate ‘self’ with PushValue(-2) before PCall; mirror that here for consistency and fewer stack mutations.
-// Destroy the StringList -lua.PushCEObject(stringListObj); -lua.PushString("destroy"); -lua.GetTable(-2); -if (lua.IsFunction(-1)) -{ - lua.PushCEObject(stringListObj); - lua.PCall(1, 0); -} -else -{ - lua.Pop(1); -} -lua.Pop(1); // Remove StringList +// Destroy the StringList +lua.PushCEObject(stringListObj); +lua.PushString("destroy"); +lua.GetTable(-2); // stack: [obj, destroy] +if (lua.IsFunction(-1)) +{ + lua.PushValue(-2); // duplicate obj as 'self' + lua.PCall(1, 0); +} +else +{ + lua.Pop(1); // pop non-function +} +lua.Pop(1); // pop objTools/DisassembleTool.cs (1)
25-26: Use culture-invariant casingUse
ToLowerInvariant()to avoid locale-dependent behavior when matching request types.- switch (request.RequestType?.ToLower()) + switch (request.RequestType?.ToLowerInvariant())SDK/Process.cs (6)
28-36: Remove unreachable error handling after PCall; it already throws on failure
CESDKLua.PCallthrows when the call fails. Theresultcheck is dead code and will never run. Rely on the exception path and keep stack cleanup incatch.- // Call the function - int result = lua.PCall(0, 1); - if (result != 0) - { - string error = lua.ToString(-1); - lua.SetTop(0); - throw new InvalidOperationException($"getProcessList call failed: {error}"); - } + // Call the function (throws on failure) + lua.PCall(0, 1);
93-99: Remove redundantPCallresult checks (OpenByPid)Same issue:
PCallthrows on error; the manual check is redundant.- int result = lua.PCall(1, 1); - if (result != 0) - { - string error = lua.ToString(-1); - lua.SetTop(0); - throw new InvalidOperationException($"openProcess call failed: {error}"); - } + lua.PCall(1, 1);
130-136: Remove redundantPCallresult checks (OpenByName)Same clean-up as above.
- int result = lua.PCall(1, 1); - if (result != 0) - { - string error = lua.ToString(-1); - lua.SetTop(0); - throw new InvalidOperationException($"openProcess call failed: {error}"); - } + lua.PCall(1, 1);
165-171: Remove redundantPCallresult checks (GetStatus#getOpenedProcessID)Rely on
PCallthrowing instead of checking its return code.- int result = lua.PCall(0, 1); - if (result != 0) - { - string error = lua.ToString(-1); - lua.SetTop(0); - throw new InvalidOperationException($"getOpenedProcessID call failed: {error}"); - } + lua.PCall(0, 1);
186-207: Simplify getProcessList path and drop unusedresultvariable
PCallthrows on error; after it returns, just check the table. This also removes a now-undefinedresultvariable later in the method.- lua.GetGlobal("getProcessList"); - if (lua.IsFunction(-1)) - { - result = lua.PCall(0, 1); - if (result == 0 && lua.IsTable(-1)) - { + lua.GetGlobal("getProcessList"); + if (lua.IsFunction(-1)) + { + lua.PCall(0, 1); + if (lua.IsTable(-1)) + { // getProcessList() returns keys as numbers, not strings lua.PushInteger(processId); lua.GetTable(-2); if (!lua.IsNil(-1)) { isOpen = true; if (lua.IsString(-1)) { processName = lua.ToString(-1) ?? ""; } } lua.Pop(1); // Remove the process name/nil - } - lua.Pop(1); // Remove process list table + } + lua.Pop(1); // Remove process list table
8-9: Consider renaming to avoid confusion with System.Diagnostics.ProcessHaving an internal class named
Processin theCESDKnamespace can be confusing alongsideSystem.Diagnostics.Process. A more specific name likeCeProcessorSdkProcesswould improve readability in call sites.If you want, I can scan the repo to gauge rename blast radius and suggest a safe migration plan.
CheatEngineTools.cs (1)
59-62: AlignAobScanmethod name with PascalCase types and add null-check• In
CheatEngineTools.cs(lines 59–62), rename the wrapper fromAOBScantoAobScan, and add a guard for a nullrequest.
• InControllers/CheatEngineController.cs(around line 70), update the controller signature and call site toAobScan.Suggested diffs:
--- CheatEngineTools.cs @@ -59,7 +59,11 @@ public class CheatEngineTools - public AobScanResponse AOBScan(AobScanRequest request) - { - return _aobScanTool.AOBScan(request); - } + public AobScanResponse AobScan(AobScanRequest request) + { + if (request == null) + return new AobScanResponse { Success = false, Error = "Request cannot be null" }; + return _aobScanTool.AOBScan(request); + }--- Controllers/CheatEngineController.cs @@ -68,7 +68,7 @@ public class CheatEngineController : ControllerBase - public AobScanResponse AOBScan([FromBody] AobScanRequest request) - { - return tools.AOBScan(request); - } + public AobScanResponse AobScan([FromBody] AobScanRequest request) + { + return tools.AobScan(request); + }Plugin.cs (3)
169-178: Ensure stop path is exception-safe and consistentGuard against exceptions during stop to keep the UI state consistent and avoid dangling references.
- void StopMCPServer() - { - if (!isServerRunning) return; - - mcpServer?.Stop(); - mcpServer = null; - isServerRunning = false; - sdk.lua.DoString("print('MCP API stopped')"); - UpdateButtonText(); - } + void StopMCPServer() + { + if (!isServerRunning) return; + try + { + mcpServer?.Stop(); + } + catch (Exception ex) + { + sdk.lua.DoString($"print('Error stopping MCP API: {ex.Message}')"); + } + finally + { + mcpServer = null; + isServerRunning = false; + sdk.lua.DoString("print('MCP API stopped')"); + UpdateButtonText(); + } + }
11-17: Single source of truth for running stateYou keep both
isServerRunningandmcpServer.IsRunning. Consider using onlymcpServer?.IsRunning == trueto reduce drift between flags and actual host state.I can scan for all reads/writes of
isServerRunningand propose a minimal change set if you want to collapse to the wrapper’s state.
140-143: RenameGetServerWrappertoGetServerfor clarityThe method currently reads:
public McpServer GetServerWrapper() { return mcpServer; }Since it returns an
McpServerinstance (not a wrapper type), renaming it toGetServerbetter reflects its purpose and avoids confusion.• Update declaration in Plugin.cs:
- public McpServer GetServerWrapper() + public McpServer GetServer()• Update all call sites in ConfigWindow.xaml.cs:
- if (_plugin?.GetServerWrapper()?.IsRunning == true) + if (_plugin?.GetServer()?.IsRunning == true)(on lines 32 and 82)
This is a low-risk, internal/UI-only change, but it does require adjusting these two usages to match the new method name.
Tools/MemoryWriteTool.cs (2)
21-23: Use culture-invariant casing
ToLowerInvariant()avoids locale-dependent surprises.- string dataType = request.DataType.ToLower(); + string dataType = request.DataType.ToLowerInvariant();
21-31: Prefer aswitchfor data type dispatchMinor readability/maintainability: replace the long
if/elsechain with aswitchondataType. Easier to extend and less error-prone.I can provide a clean
switchrewrite if you want to adopt it in this PR.Also applies to: 33-35
SDK/AOB.cs (4)
35-51: Guard alignment parameters to prevent invalid Lua calls.When alignmentType is 1 or 2, AOBScan expects an alignment parameter. Right now, alignmentType can be provided without alignmentParam, which can lead to unpredictable behavior on the Lua side.
Apply this diff to enforce a required alignmentParam when alignmentType is 1 or 2:
- if (alignmentType.HasValue) + if (alignmentType.HasValue) { lua.PushInteger(alignmentType.Value); paramCount++; - if (!string.IsNullOrWhiteSpace(alignmentParam)) - { - lua.PushString(alignmentParam); - paramCount++; - } + // alignmentType: 0=none, 1=divisible by param, 2=address ends with param + if (alignmentType.Value == 1 || alignmentType.Value == 2) + { + if (string.IsNullOrWhiteSpace(alignmentParam)) + throw new ArgumentException("alignmentParam is required when alignmentType is 1 or 2.", nameof(alignmentParam)); + lua.PushString(alignmentParam); + paramCount++; + } + else if (!string.IsNullOrWhiteSpace(alignmentParam)) + { + // If type is 0 but a param is supplied, keep current behavior (pass-through) or consider warning. + lua.PushString(alignmentParam); + paramCount++; + } }
54-61: Remove unreachable error branch after PCall; CESDKLua.PCall already throws on error.
CESDKLua.PCallthrows on non-zero return codes, soif (result != 0)can never execute. It also risks obscuring the original Lua error.Apply this diff to simplify:
- int result = lua.PCall(paramCount, 1); - if (result != 0) - { - string error = lua.ToString(-1); - lua.SetTop(0); - throw new InvalidOperationException($"AOBScan call failed: {error}"); - } + lua.PCall(paramCount, 1);
90-101: Ensure StringList is destroyed even if Count retrieval fails.If
Countisn’t a number (unexpected), the current path pops the non-number and exits without destroying the StringList, leaking resources.Apply this diff to fall back to destroying the list even when
Countis unavailable:- } - else - { - lua.Pop(1); // Pop the non-number - } + } + else + { + lua.Pop(1); // Pop the non-number + // Try to destroy anyway + lua.PushString("destroy"); + lua.GetTable(-2); + if (lua.IsFunction(-1)) + { + lua.PushValue(-2); + lua.PCall(1, 0); + } + else + { + lua.Pop(1); + } + }
109-116: Avoid double-wrapping PCall exceptions where possible.You’re wrapping all exceptions into
InvalidOperationException("AOB scan error: ...", ex). That’s fine, but bear in mindPCallalready provides a detailed message. Consider rethrowing with context only where it adds value to avoid overly nested messages.ConfigWindow.xaml.cs (4)
51-62: Prefer a shared HttpClient for repeated test calls.Creating/disposing HttpClient per click can be expensive and may hit ephemeral port limits under rapid repeated clicks. A static/shared instance with a per-request timeout is typically sufficient here.
Apply this diff:
+ private static readonly HttpClient Http = new HttpClient { Timeout = TimeSpan.FromSeconds(5) }; @@ - using (var client = new HttpClient { Timeout = TimeSpan.FromSeconds(5) }) - { - var response = await client.GetAsync($"{_viewModel.BaseUrl}/api/cheatengine/health"); + { + var response = await Http.GetAsync($"{_viewModel.BaseUrl}/api/cheatengine/health"); _viewModel.TestResult = response.IsSuccessStatusCode ? "✓ Connection successful! Server is responding." : $"✗ Server responded with status: {response.StatusCode}"; }
78-99: Debounce start/stop to prevent double toggles.Rapid clicks can race and leave UI state out of sync with the server state. Disable the button while toggling, or check and short-circuit multiple concurrent operations.
Example minimal tweak within handler:
private void StartStopButton_Click(object sender, RoutedEventArgs e) { + StartStopButton.IsEnabled = false; try { if (_plugin?.GetServerWrapper()?.IsRunning == true) { _plugin.StopServer(); _viewModel.TestResult = "Server stopped."; } else { _viewModel.SaveToServerConfig(); _plugin?.StartServer(); _viewModel.TestResult = "Server started."; } UpdateServerStatus(); } catch (Exception ex) { _viewModel.TestResult = $"Error: {ex.Message}"; } + finally + { + StartStopButton.IsEnabled = true; + } }
115-147: Reduce theme assignment repetition via styles or visual tree iteration.Manually setting Foreground on each named element is brittle. Prefer a resource-level style keyed on TextBlock and Label, or iterate the visual tree once.
If you want to keep it local, you can apply a TextElement.Foreground scoped style in XAML or consolidate with:
foreach (var tb in MainGrid.Children.OfType<TextBlock>()) tb.Foreground = textBrush;But a ResourceDictionary-based style is cleaner and scales better.
149-162: Dark-mode detection: consider high-contrast and missing registry values.You already catch exceptions. Optionally add a high-contrast override to ensure accessibility themes take precedence.
- private static bool IsWindowsInDarkMode() + private static bool IsWindowsInDarkMode() { try { + if (SystemParameters.HighContrast) return false; // prefer HC styles var value = Microsoft.Win32.Registry.GetValue( @"HKEY_CURRENT_USER\SOFTWARE\Microsoft\Windows\CurrentVersion\Themes\Personalize", "AppsUseLightTheme", 1); return (int)value == 0; } catch { return false; } }SDK/MemoryWrite.cs (2)
55-76: Remove redundant PCall return-code checks across methods.
CESDKLua.PCallthrows on failure, so checkingif (result != 0)is dead code. Simplify and preserve the original error.Example for WriteInteger (repeat for others):
- int result = lua.PCall(2, 0); - if (result != 0) - { - string error = lua.ToString(-1); - lua.SetTop(0); - throw new InvalidOperationException($"writeInteger call failed: {error}"); - } + lua.PCall(2, 0);Optionally, extract a small helper to DRY the pattern of “get global, validate function, push args, call, reset stack”.
Also applies to: 89-110, 123-144, 157-189
5-11: Consider exposing overloads for common types (ergonomics).For WriteBytes, accepting
IEnumerable<byte>orReadOnlySpan<byte>would avoid forcing callers to allocate int[] and perform range checks. This is a nicety, not a blocker.Controllers/CheatEngineController.cs (2)
9-9: Reintroduce testability via constructor injection (keep default for Web API).Direct
new CheatEngineTools()makes unit testing/controllers harder. Provide a default constructor and an overload for DI/mocking without changing runtime behavior.Apply this diff:
- private readonly CheatEngineTools tools = new CheatEngineTools(); + private readonly CheatEngineTools tools; + + public CheatEngineController() : this(new CheatEngineTools()) + { + } + + public CheatEngineController(CheatEngineTools tools) + { + this.tools = tools ?? throw new ArgumentNullException(nameof(tools)); + }
93-99: Health payload looks good; consider adding uptime if available.As a future enhancement, exposing server uptime (if McpServer tracks it) can help clients distinguish just-started vs long-running instances.
| }, | ||
| } No newline at end of file |
There was a problem hiding this comment.
Fix trailing comma — current JSON is invalid
The trailing comma after the object value and before the closing brace breaks JSON parsing (caught by Biome). Remove it.
Apply this diff:
"sonarlint.connectedMode.project": {
"connectionId": "hedgehogform",
"projectKey": "hedgehogform_ce-mcp"
- },
-}
+ }
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| }, | |
| } | |
| "sonarlint.connectedMode.project": { | |
| "connectionId": "hedgehogform", | |
| "projectKey": "hedgehogform_ce-mcp" | |
| } | |
| } |
🧰 Tools
🪛 Biome (2.1.2)
[error] 5-6: Expected a property but instead found '}'.
Expected a property here.
(parse)
🤖 Prompt for AI Agents
In .vscode/settings.json around lines 5 to 6 the file contains a trailing comma
before the closing brace which makes the JSON invalid; remove the extra comma
after the last property value so the object ends with "}" only (update the file
to valid JSON and re-run the linter/biome to confirm).
| public BaseResponse OpenProcess(OpenProcessRequest request) | ||
| { | ||
| return _openProcessTool.OpenProcess(request); | ||
| return _processTool.OpenProcess(request); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null request guard for OpenProcess
Pre-empt NullReferenceException by validating request before delegating.
- public BaseResponse OpenProcess(OpenProcessRequest request)
- {
- return _processTool.OpenProcess(request);
- }
+ public BaseResponse OpenProcess(OpenProcessRequest request)
+ {
+ if (request == null)
+ return new BaseResponse { Success = false, Error = "Request cannot be null" };
+ return _processTool.OpenProcess(request);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public BaseResponse OpenProcess(OpenProcessRequest request) | |
| { | |
| return _openProcessTool.OpenProcess(request); | |
| return _processTool.OpenProcess(request); | |
| } | |
| public BaseResponse OpenProcess(OpenProcessRequest request) | |
| { | |
| if (request == null) | |
| return new BaseResponse { Success = false, Error = "Request cannot be null" }; | |
| return _processTool.OpenProcess(request); | |
| } |
🤖 Prompt for AI Agents
In CheatEngineTools.cs around lines 29 to 32, the OpenProcess method delegates
without validating the request which can cause a NullReferenceException; add a
null-check at the start of the method (throw ArgumentNullException or return an
appropriate error BaseResponse) and only call _processTool.OpenProcess(request)
when request is non-null, preserving existing behavior and error handling
conventions used elsewhere in the codebase.
| public DisassemblerResponse Disassemble(DisassemblerRequest request) | ||
| { | ||
| return _disassembleTool.Disassemble(request); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null request guard for Disassemble
Mirror defensive checks used elsewhere to avoid NREs on null requests.
- public DisassemblerResponse Disassemble(DisassemblerRequest request)
- {
- return _disassembleTool.Disassemble(request);
- }
+ public DisassemblerResponse Disassemble(DisassemblerRequest request)
+ {
+ if (request == null)
+ return new DisassemblerResponse { Success = false, Error = "Request cannot be null" };
+ return _disassembleTool.Disassemble(request);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public DisassemblerResponse Disassemble(DisassemblerRequest request) | |
| { | |
| return _disassembleTool.Disassemble(request); | |
| } | |
| public DisassemblerResponse Disassemble(DisassemblerRequest request) | |
| { | |
| if (request == null) | |
| return new DisassemblerResponse { Success = false, Error = "Request cannot be null" }; | |
| return _disassembleTool.Disassemble(request); | |
| } |
🤖 Prompt for AI Agents
In CheatEngineTools.cs around lines 64 to 67, the Disassemble method lacks a
null guard for the request parameter; add a defensive check at the start that
validates request is not null and throw an
ArgumentNullException(nameof(request)) (matching the pattern used elsewhere in
the class) before delegating to _disassembleTool.Disassemble(request).
| public MemScanResponse Scan(MemScanScanRequest request) | ||
| { | ||
| return _getInstructionSizeTool.GetInstructionSize(request); | ||
| return _memScanTool.Scan(request); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null request guard for MemScan.Scan
Consistent API boundary validation improves resilience and error messages.
- public MemScanResponse Scan(MemScanScanRequest request)
- {
- return _memScanTool.Scan(request);
- }
+ public MemScanResponse Scan(MemScanScanRequest request)
+ {
+ if (request == null)
+ return new MemScanResponse { Success = false, Error = "Request cannot be null" };
+ return _memScanTool.Scan(request);
+ }🤖 Prompt for AI Agents
In CheatEngineTools.cs around lines 69 to 72, the Scan method forwards the
request without validating it; add a null-guard to validate the request and
throw an ArgumentNullException (or equivalent) when request is null before
calling _memScanTool.Scan(request). Ensure the exception message names the
parameter ("request") and keep behavior consistent with other API boundary
validations in the codebase.
| @mcp.tool() | ||
| async def memscan_first(scan_option: str, var_type: str, input1: str = None, input2: str = None, | ||
| rounding_type: str = "rtRounded", start_address: str = "0", | ||
| stop_address: str = "0xffffffffffffffff", protection_flags: str = None, | ||
| alignment_type: str = None, alignment_param: str = None, | ||
| is_hexadecimal: bool = False, is_not_binary_string: bool = False, | ||
| is_unicode: bool = False, is_case_sensitive: bool = False) -> Dict[str, Any]: | ||
| """ | ||
| Perform a first memory scan to search for values in the target process | ||
| This function blocks and waits until the scan is complete before returning. | ||
|
|
||
| Args: | ||
| scan_option: Type of scan (soUnknownValue, soExactValue, soValueBetween, soBiggerThan, soSmallerThan) | ||
| var_type: Variable type (vtByte, vtWord, vtDword, vtQword, vtSingle, vtDouble, vtString, vtByteArray, vtGrouped, vtBinary, vtAll) | ||
| input1: Primary search value (required for most scan types except soUnknownValue) | ||
| input2: Secondary search value (required for soValueBetween) | ||
| rounding_type: Rounding type for floating point values (rtRounded, rtTruncated, rtExtremerounded) | ||
| start_address: Start address for scan (default: "0") | ||
| stop_address: Stop address for scan (default: "0xffffffffffffffff") | ||
| protection_flags: Memory protection flags (e.g., "+W+X") | ||
| alignment_type: Alignment type (fsmNotAligned, fsmAligned, fsmLastDigits) | ||
| alignment_param: Alignment parameter | ||
| is_hexadecimal: Whether input values are hexadecimal | ||
| is_not_binary_string: Whether to handle binary as decimal instead of binary string | ||
| is_unicode: Whether to use Unicode (UTF-16) for string scans | ||
| is_case_sensitive: Whether string comparison is case sensitive | ||
|
|
||
| Returns: | ||
| Dictionary with scan ID and result count after completion | ||
| """ | ||
| data = { | ||
| "scanOption": scan_option, | ||
| "varType": var_type, | ||
| "roundingType": rounding_type, | ||
| "startAddress": start_address, | ||
| "stopAddress": stop_address, | ||
| "isHexadecimalInput": is_hexadecimal, | ||
| "isNotABinaryString": is_not_binary_string, | ||
| "isUnicodeScan": is_unicode, | ||
| "isCaseSensitive": is_case_sensitive | ||
| } | ||
|
|
||
| if input1 is not None: | ||
| data["input1"] = input1 | ||
| if input2 is not None: | ||
| data["input2"] = input2 | ||
| if protection_flags is not None: | ||
| data["protectionFlags"] = protection_flags | ||
| if alignment_type is not None: | ||
| data["alignmentType"] = alignment_type | ||
| if alignment_param is not None: | ||
| data["alignmentParam"] = alignment_param | ||
|
|
||
| return await make_request("memscan-first", "POST", data) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and consistent enums for memscan_first to fail fast on bad requests.
- Validate scan_option, var_type, rounding_type.
- Enforce input1/input2 presence for the relevant modes to avoid 400s from the backend.
- Optional: Align alignment_type’s type across tools (here str; aob_scan uses int).
Apply this diff at the top of the function body (right after the docstring):
@@
async def memscan_first(scan_option: str, var_type: str, input1: str = None, input2: str = None,
rounding_type: str = "rtRounded", start_address: str = "0",
stop_address: str = "0xffffffffffffffff", protection_flags: str = None,
alignment_type: str = None, alignment_param: str = None,
is_hexadecimal: bool = False, is_not_binary_string: bool = False,
is_unicode: bool = False, is_case_sensitive: bool = False) -> Dict[str, Any]:
@@
- """
+ """
@@
- data = {
+ # Basic validation to fail fast before calling the API
+ allowed_scan_options = {
+ "soUnknownValue", "soExactValue", "soValueBetween", "soBiggerThan", "soSmallerThan"
+ }
+ allowed_var_types = {
+ "vtByte", "vtWord", "vtDword", "vtQword", "vtSingle", "vtDouble", "vtString",
+ "vtByteArray", "vtGrouped", "vtBinary", "vtAll"
+ }
+ allowed_rounding = {"rtRounded", "rtTruncated", "rtExtremerounded"}
+
+ if scan_option not in allowed_scan_options:
+ return {"success": False, "error": f"Invalid scan_option: {scan_option}"}
+ if var_type not in allowed_var_types:
+ return {"success": False, "error": f"Invalid var_type: {var_type}"}
+ if rounding_type not in allowed_rounding:
+ return {"success": False, "error": f"Invalid rounding_type: {rounding_type}"}
+
+ requires_input1 = {"soExactValue", "soValueBetween", "soBiggerThan", "soSmallerThan"}
+ if scan_option in requires_input1 and input1 is None:
+ return {"success": False, "error": f"input1 is required for {scan_option}"}
+ if scan_option == "soValueBetween" and input2 is None:
+ return {"success": False, "error": "input2 is required for soValueBetween"}
+
+ data = {
"scanOption": scan_option,
"varType": var_type,
"roundingType": rounding_type,
"startAddress": start_address,
"stopAddress": stop_address,
"isHexadecimalInput": is_hexadecimal,
"isNotABinaryString": is_not_binary_string,
"isUnicodeScan": is_unicode,
"isCaseSensitive": is_case_sensitive
}Optional follow-up: unifying alignment_type (string here vs int in aob_scan) across the public surface.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @mcp.tool() | |
| async def memscan_first(scan_option: str, var_type: str, input1: str = None, input2: str = None, | |
| rounding_type: str = "rtRounded", start_address: str = "0", | |
| stop_address: str = "0xffffffffffffffff", protection_flags: str = None, | |
| alignment_type: str = None, alignment_param: str = None, | |
| is_hexadecimal: bool = False, is_not_binary_string: bool = False, | |
| is_unicode: bool = False, is_case_sensitive: bool = False) -> Dict[str, Any]: | |
| """ | |
| Perform a first memory scan to search for values in the target process | |
| This function blocks and waits until the scan is complete before returning. | |
| Args: | |
| scan_option: Type of scan (soUnknownValue, soExactValue, soValueBetween, soBiggerThan, soSmallerThan) | |
| var_type: Variable type (vtByte, vtWord, vtDword, vtQword, vtSingle, vtDouble, vtString, vtByteArray, vtGrouped, vtBinary, vtAll) | |
| input1: Primary search value (required for most scan types except soUnknownValue) | |
| input2: Secondary search value (required for soValueBetween) | |
| rounding_type: Rounding type for floating point values (rtRounded, rtTruncated, rtExtremerounded) | |
| start_address: Start address for scan (default: "0") | |
| stop_address: Stop address for scan (default: "0xffffffffffffffff") | |
| protection_flags: Memory protection flags (e.g., "+W+X") | |
| alignment_type: Alignment type (fsmNotAligned, fsmAligned, fsmLastDigits) | |
| alignment_param: Alignment parameter | |
| is_hexadecimal: Whether input values are hexadecimal | |
| is_not_binary_string: Whether to handle binary as decimal instead of binary string | |
| is_unicode: Whether to use Unicode (UTF-16) for string scans | |
| is_case_sensitive: Whether string comparison is case sensitive | |
| Returns: | |
| Dictionary with scan ID and result count after completion | |
| """ | |
| data = { | |
| "scanOption": scan_option, | |
| "varType": var_type, | |
| "roundingType": rounding_type, | |
| "startAddress": start_address, | |
| "stopAddress": stop_address, | |
| "isHexadecimalInput": is_hexadecimal, | |
| "isNotABinaryString": is_not_binary_string, | |
| "isUnicodeScan": is_unicode, | |
| "isCaseSensitive": is_case_sensitive | |
| } | |
| if input1 is not None: | |
| data["input1"] = input1 | |
| if input2 is not None: | |
| data["input2"] = input2 | |
| if protection_flags is not None: | |
| data["protectionFlags"] = protection_flags | |
| if alignment_type is not None: | |
| data["alignmentType"] = alignment_type | |
| if alignment_param is not None: | |
| data["alignmentParam"] = alignment_param | |
| return await make_request("memscan-first", "POST", data) | |
| @mcp.tool() | |
| async def memscan_first(scan_option: str, var_type: str, input1: str = None, input2: str = None, | |
| rounding_type: str = "rtRounded", start_address: str = "0", | |
| stop_address: str = "0xffffffffffffffff", protection_flags: str = None, | |
| alignment_type: str = None, alignment_param: str = None, | |
| is_hexadecimal: bool = False, is_not_binary_string: bool = False, | |
| is_unicode: bool = False, is_case_sensitive: bool = False) -> Dict[str, Any]: | |
| """ | |
| Perform a first memory scan to search for values in the target process | |
| This function blocks and waits until the scan is complete before returning. | |
| Args: | |
| scan_option: Type of scan (soUnknownValue, soExactValue, soValueBetween, soBiggerThan, soSmallerThan) | |
| var_type: Variable type (vtByte, vtWord, vtDword, vtQword, vtSingle, vtDouble, vtString, vtByteArray, vtGrouped, vtBinary, vtAll) | |
| input1: Primary search value (required for most scan types except soUnknownValue) | |
| input2: Secondary search value (required for soValueBetween) | |
| rounding_type: Rounding type for floating point values (rtRounded, rtTruncated, rtExtremerounded) | |
| start_address: Start address for scan (default: "0") | |
| stop_address: Stop address for scan (default: "0xffffffffffffffff") | |
| protection_flags: Memory protection flags (e.g., "+W+X") | |
| alignment_type: Alignment type (fsmNotAligned, fsmAligned, fsmLastDigits) | |
| alignment_param: Alignment parameter | |
| is_hexadecimal: Whether input values are hexadecimal | |
| is_not_binary_string: Whether to handle binary as decimal instead of binary string | |
| is_unicode: Whether to use Unicode (UTF-16) for string scans | |
| is_case_sensitive: Whether string comparison is case sensitive | |
| Returns: | |
| Dictionary with scan ID and result count after completion | |
| """ | |
| # Basic validation to fail fast before calling the API | |
| allowed_scan_options = { | |
| "soUnknownValue", "soExactValue", "soValueBetween", "soBiggerThan", "soSmallerThan" | |
| } | |
| allowed_var_types = { | |
| "vtByte", "vtWord", "vtDword", "vtQword", "vtSingle", "vtDouble", "vtString", | |
| "vtByteArray", "vtGrouped", "vtBinary", "vtAll" | |
| } | |
| allowed_rounding = {"rtRounded", "rtTruncated", "rtExtremerounded"} | |
| if scan_option not in allowed_scan_options: | |
| return {"success": False, "error": f"Invalid scan_option: {scan_option}"} | |
| if var_type not in allowed_var_types: | |
| return {"success": False, "error": f"Invalid var_type: {var_type}"} | |
| if rounding_type not in allowed_rounding: | |
| return {"success": False, "error": f"Invalid rounding_type: {rounding_type}"} | |
| requires_input1 = {"soExactValue", "soValueBetween", "soBiggerThan", "soSmallerThan"} | |
| if scan_option in requires_input1 and input1 is None: | |
| return {"success": False, "error": f"input1 is required for {scan_option}"} | |
| if scan_option == "soValueBetween" and input2 is None: | |
| return {"success": False, "error": "input2 is required for soValueBetween"} | |
| data = { | |
| "scanOption": scan_option, | |
| "varType": var_type, | |
| "roundingType": rounding_type, | |
| "startAddress": start_address, | |
| "stopAddress": stop_address, | |
| "isHexadecimalInput": is_hexadecimal, | |
| "isNotABinaryString": is_not_binary_string, | |
| "isUnicodeScan": is_unicode, | |
| "isCaseSensitive": is_case_sensitive | |
| } | |
| if input1 is not None: | |
| data["input1"] = input1 | |
| if input2 is not None: | |
| data["input2"] = input2 | |
| if protection_flags is not None: | |
| data["protectionFlags"] = protection_flags | |
| if alignment_type is not None: | |
| data["alignmentType"] = alignment_type | |
| if alignment_param is not None: | |
| data["alignmentParam"] = alignment_param | |
| return await make_request("memscan-first", "POST", data) |
🤖 Prompt for AI Agents
In mcp-client/cheat_engine_mcp_server.py around lines 742–796, add input
validation at the top of memscan_first (immediately after the docstring):
validate that scan_option is one of the allowed scan options (e.g.,
soUnknownValue, soExactValue, soValueBetween, soBiggerThan, soSmallerThan),
var_type is one of the allowed var types (vtByte, vtWord, vtDword, vtQword,
vtSingle, vtDouble, vtString, vtByteArray, vtGrouped, vtBinary, vtAll), and
rounding_type is one of the allowed rounding values (rtRounded, rtTruncated,
rtExtremerounded); enforce presence of input1 for scan modes that require a
primary value (e.g., soExactValue, soBiggerThan, soSmallerThan) and require both
input1 and input2 for soValueBetween, raising ValueError with a clear message on
missing/invalid values so the function fails fast instead of sending bad
requests to the backend; optionally coerce or validate alignment_type’s type (or
document it) to keep consistency with aob_scan (string vs int) but do not change
external signature here.
| if (!float.TryParse(request.Value.ToString(), out float floatValue)) | ||
| throw new ArgumentException("Value must be a valid float"); | ||
|
|
||
| memoryWrite.WriteFloat(request.Address, floatValue); | ||
| return floatValue; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Parse floats with invariant culture
float.TryParse is culture-sensitive. Use invariant culture to ensure predictable behavior for decimal separators irrespective of OS locale.
- if (!float.TryParse(request.Value.ToString(), out float floatValue))
+ if (!float.TryParse(
+ request.Value.ToString(),
+ System.Globalization.NumberStyles.Float,
+ System.Globalization.CultureInfo.InvariantCulture,
+ out float floatValue))
throw new ArgumentException("Value must be a valid float");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!float.TryParse(request.Value.ToString(), out float floatValue)) | |
| throw new ArgumentException("Value must be a valid float"); | |
| memoryWrite.WriteFloat(request.Address, floatValue); | |
| return floatValue; | |
| if (!float.TryParse( | |
| request.Value.ToString(), | |
| System.Globalization.NumberStyles.Float, | |
| System.Globalization.CultureInfo.InvariantCulture, | |
| out float floatValue)) | |
| throw new ArgumentException("Value must be a valid float"); | |
| memoryWrite.WriteFloat(request.Address, floatValue); | |
| return floatValue; |
🤖 Prompt for AI Agents
In Tools/MemoryWriteTool.cs around lines 117 to 121, the float parsing is
culture-sensitive; change the TryParse call to use invariant culture (and
appropriate NumberStyles) so decimal separators are parsed predictably across
locales — e.g. call float.TryParse(request.Value.ToString(), NumberStyles.Float
| NumberStyles.AllowThousands, CultureInfo.InvariantCulture, out float
floatValue) (ensure System.Globalization is imported); keep the same exception
and write path.
| { | ||
| public class MemScanTool | ||
| { | ||
| private readonly MemScan memScan = new MemScan(); |
There was a problem hiding this comment.
Scan session state leaks across requests due to shared MemScan instance
MemScan keeps internal state (scanStarted). Holding a single instance at field scope means a caller’s “first scan” may incorrectly become a “next scan” if a previous call already ran. This can corrupt scan semantics across requests/users.
Apply this diff to instantiate MemScan per operation (minimal, one-off scans). If you need multi-step “next scan” flows, scope/cache MemScan by a session key instead of as a singleton field.
- private readonly MemScan memScan = new MemScan();
+ // Intentionally avoid cross-request state; create per call or session-scope this by key if supporting Next Scan.
+ // private readonly MemScan memScan = new MemScan();And replace the call site:
- memScan.Scan(parameters);
+ var memScan = new MemScan();
+ memScan.Scan(parameters);If you plan to support NextScan, I can propose a session-scoped design (ConcurrentDictionary<string, MemScan> + Reset endpoints). Want me to draft it?
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private readonly MemScan memScan = new MemScan(); | |
| // Intentionally avoid cross-request state; create per call or session-scope this by key if supporting Next Scan. | |
| // private readonly MemScan memScan = new MemScan(); |
| private readonly MemScan memScan = new MemScan(); | |
| var memScan = new MemScan(); | |
| memScan.Scan(parameters); |
🤖 Prompt for AI Agents
In Tools/MemScanTool.cs around line 12, the MemScan instance is declared as a
field which causes scan state (scanStarted) to leak across requests; change the
code to instantiate a new MemScan object inside the method that performs a
single scan (i.e., replace the field-scoped readonly MemScan memScan = new
MemScan() with creating new MemScan() at the start of the operation), and update
the call site(s) to use that local instance for the duration of the operation;
if you later need multi-step NextScan flows, implement a session-scoped cache
(e.g., ConcurrentDictionary<string, MemScan>) keyed by session id and provide
reset/cleanup endpoints instead of a singleton field.
| public MemScanResponse Scan(MemScanScanRequest request) | ||
| { | ||
| // Convert MemScanScanRequest to ScanParameters | ||
| ScanParameters parameters = new ScanParameters() | ||
| { | ||
| Value = request.Input1, | ||
| Value2 = request.Input2, | ||
| ScanOption = request.ScanOption, | ||
| VarType = request.VarType, | ||
| RoundingType = request.RoundingType, | ||
| StartAddress = request.StartAddress, | ||
| StopAddress = request.StopAddress, | ||
| ProtectionFlags = request.ProtectionFlags, | ||
| AlignmentType = request.AlignmentType, | ||
| AlignmentValue = request.AlignmentParam, | ||
| isHexadecimalInput = request.IsHexadecimalInput, | ||
| isUTF16Scan = request.IsUnicodeScan, | ||
| isCaseSensitive = request.IsCaseSensitive, | ||
| isPercentageScan = request.IsPercentageScan | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add request null-check and wrap in try/catch to return structured errors
Unlike other tools in this PR, Scan can throw and bypass the BaseResponse contract. Add argument validation and consistent error handling.
- public MemScanResponse Scan(MemScanScanRequest request)
- {
- // Convert MemScanScanRequest to ScanParameters
+ public MemScanResponse Scan(MemScanScanRequest request)
+ {
+ if (request == null)
+ {
+ return new MemScanResponse { Success = false, Error = "Request cannot be null" };
+ }
+ try
+ {
+ // Convert MemScanScanRequest to ScanParameters
ScanParameters parameters = new ScanParameters()
{
Value = request.Input1,
Value2 = request.Input2,
ScanOption = request.ScanOption,
VarType = request.VarType,
RoundingType = request.RoundingType,
- StartAddress = request.StartAddress,
- StopAddress = request.StopAddress,
+ StartAddress = request.StartAddress > (ulong)long.MaxValue ? (ulong)long.MaxValue : request.StartAddress,
+ StopAddress = request.StopAddress > (ulong)long.MaxValue ? (ulong)long.MaxValue : request.StopAddress,
ProtectionFlags = request.ProtectionFlags,
AlignmentType = request.AlignmentType,
AlignmentValue = request.AlignmentParam,
isHexadecimalInput = request.IsHexadecimalInput,
isUTF16Scan = request.IsUnicodeScan,
isCaseSensitive = request.IsCaseSensitive,
isPercentageScan = request.IsPercentageScan
};
-
- memScan.Scan(parameters);
+ var memScan = new MemScan();
+ memScan.Scan(parameters);
var foundList = memScan.GetFoundList();
foundList.Initialize();
var countResults = foundList.Count;
var results = new ResultList
{
TotalCount = countResults
};
// Only return max 1000 results by for loop.
for (int i = 0; i < Math.Min(countResults, 1000); i++)
{
results.Add(foundList.GetAddress(i), foundList.GetValue(i));
}
return new MemScanResponse
{
Success = true,
Results = results
};
+ }
+ catch (Exception ex)
+ {
+ return new MemScanResponse { Success = false, Error = ex.Message, Results = new ResultList { TotalCount = 0 } };
+ }
}Also applies to: 37-57
🤖 Prompt for AI Agents
In Tools/MemScanTool.cs around lines 16-35 (and similarly apply to 37-57), the
Scan method lacks argument validation and global error handling and can throw,
violating the BaseResponse contract; add a guard that returns a failed
MemScanResponse if request is null (validate required fields as appropriate) and
wrap the method body in a try/catch that catches Exception, logs or captures the
exception details, and returns a MemScanResponse populated with error
information (error message/code) instead of letting exceptions propagate so the
method always returns a structured BaseResponse-style result.
| StartAddress = request.StartAddress, | ||
| StopAddress = request.StopAddress, |
There was a problem hiding this comment.
Potential 64-bit overflow when passing Start/Stop addresses to Lua
MemScan.Scan pushes these as a 64-bit integer cast to long. Your request model defaults StopAddress to ulong.MaxValue, which will overflow to negative when cast. Clamp to long.MaxValue.
- StartAddress = request.StartAddress,
- StopAddress = request.StopAddress,
+ StartAddress = request.StartAddress > (ulong)long.MaxValue ? (ulong)long.MaxValue : request.StartAddress,
+ StopAddress = request.StopAddress > (ulong)long.MaxValue ? (ulong)long.MaxValue : request.StopAddress,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| StartAddress = request.StartAddress, | |
| StopAddress = request.StopAddress, | |
| StartAddress = request.StartAddress > (ulong)long.MaxValue | |
| ? (ulong)long.MaxValue | |
| : request.StartAddress, | |
| StopAddress = request.StopAddress > (ulong)long.MaxValue | |
| ? (ulong)long.MaxValue | |
| : request.StopAddress, |
🤖 Prompt for AI Agents
In Tools/MemScanTool.cs around lines 26-27, passing request.StartAddress and
request.StopAddress directly as (long) risks overflow when the source is ulong
(StopAddress defaults to ulong.MaxValue); clamp both before casting so values
above long.MaxValue become long.MaxValue and then cast safely (e.g. set
StartAddress = request.StartAddress > (ulong)long.MaxValue ? long.MaxValue :
(long)request.StartAddress and likewise for StopAddress).
| public BaseResponse OpenProcess(OpenProcessRequest request) | ||
| { | ||
| try | ||
| { | ||
| if (string.IsNullOrWhiteSpace(request.Process)) | ||
| { | ||
| return new BaseResponse | ||
| { | ||
| Success = false, | ||
| Error = "Process parameter is required" | ||
| }; | ||
| } |
There was a problem hiding this comment.
Null request handling is missing
request can be null, causing a NullReference when accessing request.Process. Add an upfront null check.
- public BaseResponse OpenProcess(OpenProcessRequest request)
+ public BaseResponse OpenProcess(OpenProcessRequest request)
{
- try
+ try
{
+ if (request == null)
+ {
+ return new BaseResponse { Success = false, Error = "Request cannot be null" };
+ }
if (string.IsNullOrWhiteSpace(request.Process))
{
return new BaseResponse
{
Success = false,
Error = "Process parameter is required"
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public BaseResponse OpenProcess(OpenProcessRequest request) | |
| { | |
| try | |
| { | |
| if (string.IsNullOrWhiteSpace(request.Process)) | |
| { | |
| return new BaseResponse | |
| { | |
| Success = false, | |
| Error = "Process parameter is required" | |
| }; | |
| } | |
| public BaseResponse OpenProcess(OpenProcessRequest request) | |
| { | |
| try | |
| { | |
| if (request == null) | |
| { | |
| return new BaseResponse { Success = false, Error = "Request cannot be null" }; | |
| } | |
| if (string.IsNullOrWhiteSpace(request.Process)) | |
| { | |
| return new BaseResponse | |
| { | |
| Success = false, | |
| Error = "Process parameter is required" | |
| }; | |
| } | |
| // ... the rest of the method ... | |
| } | |
| catch (Exception ex) | |
| { | |
| // ... existing exception handling ... | |
| } | |
| } |
🤖 Prompt for AI Agents
In Tools/ProcessTool.cs around lines 33 to 44, the method OpenProcess accesses
request.Process without checking whether request is null; add an upfront null
check at the start of the method that returns a BaseResponse with Success =
false and an appropriate Error message (e.g., "Request cannot be null") before
any access to request.Process so the method safely handles null callers.
|
Looks good to me. I will make it better later. I just want to have it work after banging my head on the error I had for days. |
1 participant
Refactor to be better.
Summary by CodeRabbit
New Features
Changes
Documentation
Chores