Skip to content

feat: comprehensive fixes for 16 issues (security, performance, architecture, code quality) - #165

Open
korsan34 wants to merge 4 commits into
CoderGamester:mainfrom
korsan34:main
Open

korsan34 wants to merge 4 commits into
CoderGamester:mainfrom
korsan34:main

Conversation

@korsan34

Copy link
Copy Markdown

Summary

This PR addresses 16 issues identified during comprehensive codebase analysis:

Critical (3)

  1. Missing CreateSceneResource - Added on both Unity (C#) and Node (TypeScript) sides
  2. Missing GetSceneInfoResource - Added on both Unity (C#) and Node (TypeScript) sides
  3. Error response format mismatch - Standardized to include success: false, message, and error object for Node compatibility

High (6)

  1. GetServerPath returns error string - Now returns null on failure (prevents silent bugs)
  2. RunNpmCommand shell injection - Added EscapeForCmd() / EscapeForShell() helpers
  3. ConsoleLogsService reflection fragility - Cached reflection + retry logic + graceful fallback
  4. Port race condition - Node now retries settings file for 30s with 1s intervals
  5. CommandQueue 100 limit - Increased to 1000 + 80% capacity warning
  6. Logger sync I/O blocking - Async queue with setImmediate for non-blocking file writes

Medium (4)

  1. GameObjectTools DRY violation - Extracted validateGameObjectIdentifier() helper
  2. GetGameObjectResource list TODO - Implemented with 5s TTL cache
  3. TypeScript any usage - Added z.infer<typeof paramsSchema> types (createSceneTool example)
  4. Version mismatch - Synced to 1.4.0 (Unity + Node)

Low (3)

  1. BatchExecute atomic limitation - Documented Unity Editor-only behavior
  2. TestRunnerService callback leak - Added IDisposable + Dispose() + finalizer
  3. McpBackgroundTick static state - Added ResetForTesting() for test isolation

Verification

  • Tests: 123/123 passed (12 suites)
  • Build: TypeScript compilation successful
  • No breaking changes - All fixes are backward compatible

Files Changed (18)

  • Editor/UnityBridge/McpUnitySettings.cs
  • Editor/UnityBridge/McpUnitySocketHandler.cs
  • Editor/Services/ConsoleLogsService.cs
  • Editor/Services/TestRunnerService.cs
  • Editor/Tools/BatchExecuteTool.cs
  • Editor/Utils/McpBackgroundTick.cs
  • Editor/Utils/McpUtils.cs
  • Editor/Resources/CreateSceneResource.cs (new)
  • Editor/Resources/GetSceneInfoResource.cs (new)
  • Server~/src/resources/createSceneResource.ts (new)
  • Server~/src/resources/getSceneInfoResource.ts (new)
  • Server~/src/resources/getGameObjectResource.ts
  • Server~/src/tools/createSceneTool.ts
  • Server~/src/tools/gameObjectTools.ts
  • Server~/src/unity/commandQueue.ts
  • Server~/src/unity/unityConnectionConfig.ts
  • Server~/src/utils/logger.ts
  • Server~/src/tests/commandQueue.test.ts
  • Server~/src/tests/unityConnectionConfig.test.ts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant