feat:action trace feat:HookSystem #598
Conversation
Fixes CoplayDev#538 The System Requirements panel showed "UV Package Manager: Not Found" even when a valid UV path override was configured in Advanced Settings. Root cause: PlatformDetectorBase.DetectUv() only searched PATH with bare command names ("uvx", "uv") and never consulted PathResolverService which respects the user's override setting. Changes: - Refactor DetectUv() to use PathResolverService.GetUvxPath() which checks override path first, then system PATH, then falls back to "uvx" - Add TryValidateUvExecutable() to verify executables by running --version instead of just checking File.Exists - Prioritize PATH environment variable in EnumerateUvxCandidates() for better compatibility with official uv install scripts - Fix process output read order (ReadToEnd before WaitForExit) to prevent potential deadlocks Co-Authored-By: ChatGLM 4.7 <noreply@zhipuai.com>
- Read both stdout and stderr when validating uv/uvx executables
- Respect WaitForExit timeout return value instead of ignoring it
- Fix version parsing to handle extra tokens like "(Homebrew 2025-01-01)"
- Resolve bare commands ("uv"/"uvx") to absolute paths after validation
- Rename FindExecutableInPath to FindUvxExecutableInPath for clarity
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…s PATH augmentation Replace direct Process.Start calls with ExecPath.TryRun across all platform detectors. This change: - Fixes potential deadlocks by using async output reading - Adds proper timeout handling with process termination - Removes redundant fallback logic and simplifies version parsing - Adds Windows PATH augmentation with common uv, npm, and Python installation paths Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The version extraction logic now properly handles outputs like: - "uvx 0.9.18" -> "0.9.18" - "uvx 0.9.18 (hash date)" -> "0.9.18" - "uvx 0.9.18 extra info" -> "0.9.18" Uses Math.Min to find the first occurrence of either space or parenthesis. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add absolute path resolution in TryValidatePython and TryValidateUvWithPath for better UI display - Fix BuildAugmentedPath to avoid PATH duplication - Add comprehensive comments for version parsing logic - Ensure cross-platform consistency across all three detectors - Fix override path validation logic with clear state handling - Fix platform detector path resolution and Python version detection - Use UserProfile consistently in GetClaudeCliPath instead of Personal - All platforms now use protected BuildAugmentedPath method This change improves user experience by displaying full paths in the UI while maintaining robust fallback behavior if path resolution fails. Co-Authored-By: GLM4.7 <noreply@zhipuai.com>
- Rename TryValidateUvExecutable -> TryValidateUvxExecutable for consistency - Add cross-platform FindInPath() helper in ExecPath.cs - Remove platform-specific where/which implementations in favor of unified helper - Add Windows-specific DetectUv() override with enhanced uv/uvx detection - Add WinGet shim path support for Windows uvx installation - Update UI labels: "UV Path" -> "UVX Path" - Only show uvx path status when override is configured Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ides and system paths
…ethods across platform detectors
… WindowsPlatformDetector
Implement a comprehensive ActionTrace system that captures, stores, and queries Unity editor events for debugging, analysis, and undo/replay capabilities. **Core Features:** - Event capture layer with hooks for Unity editor events - Context tracking with stack and timeline support - Event store with in-memory persistence and query capabilities - Semantic analysis (categorization, scoring, intent inference) - VCS integration for version control context - Editor window with UI for visualizing events - MCP tools for remote query and control **Components Added:** - Capture: ActionTraceEventEmitter, EventFilter, PropertyChangeTracker, UnityEventHooks - Context: ContextStack, ContextTimeline, OperationContext, ContextMapping - Core: EventStore, EditorEvent, EventTypes, ActionTraceSettings, GlobalIdHelper - Query: ActionTraceQuery, EventSummarizer, TransactionAggregator - Semantics: DefaultCategorizer, DefaultEventScorer, DefaultIntentInferrer - UI: ActionTraceEditorWindow with UXML/USS styling - MCP Tools: get_action_trace, get_action_trace_settings, add_action_trace_note, undo_to_sequence **Server-side:** - Python models and resources for ActionTrace - MCP tools for querying events, managing settings, and undoing to sequence Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ility - Extract EventStore.Context.cs for context mapping management - Extract EventStore.Diagnostics.cs for memory diagnostics and dehydration - Extract EventStore.Merging.cs for event deduplication logic - Extract EventStore.Persistence.cs for save/load and domain reload survival - Add PropertyEventPayloadBuilder helper for consistent payload structure - Add PropertyFormatter helper to eliminate code duplication - Adjust ActionTraceSettings defaults (MaxEvents: 800, HotEventCount: 150) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ctionTraceEditorWindow
… clarity and consistency
…uery improvements
This commit introduces significant improvements to the ActionTrace system:
**Layered Settings Architecture:**
- Refactor ActionTraceSettings into layered structure (Filtering, Merging, Storage, Sampling)
- Add custom Editor with foldout sections for better UX
- Move GlobalIdHelper from Core/ to Helpers/ for better organization
**Preset System:**
- Add ActionTracePreset with Standard/Verbose/Minimal/Silent presets
- Enable quick configuration switching via ApplyPreset()
- Include preset descriptions and memory estimates
**Configurable Filtering:**
- Transform EventFilter from static blacklist to rule-based system
- Support Prefix, Extension, Regex, and GameObject rule types
- Add EventFilterSettings for persistence and customization
**Stable Cross-Session IDs:**
- Use GlobalIdHelper for all GameObject/Component event TargetIds
- Use Asset:{path} format for asset events
- Ensure TargetIds remain stable across domain reloads
**Query & Analysis Enhancements:**
- Add EventQueryBuilder for fluent query API
- Add ContextCompressor for event data optimization
- Add EventStatistics for comprehensive analytics
- Enhance EventSummarizer with better grouping
**Capture Layer Improvements:**
- Add AssetChangePostprocessor for asset change tracking
- Add SamplingMiddleware for high-frequency event throttling
- Add ToolCallScope for MCP tool call tracking
- Enhance UnityEventHooks with comprehensive event coverage
**UI/UX Improvements:**
- Add sort modes (ByTimeDesc, AIFiltered) to ActionTraceEditorWindow
- Improve filter menu with "All" option
- Add tool descriptions for better AI discoverability
**Helper Classes:**
- Add GameObjectTrackingHelper for GameObject lifecycle tracking
- Add UndoReflectionHelper for Undo system introspection
- Add BuildTargetUtility for build target detection
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…al loops and enhancing GameObject tracking
…capacity - Add Range attributes to settings fields with Inspector sliders: - MinImportanceForRecording: [Range(0f, 1f)] - MergeWindowMs: [Range(0, 5000)] - TransactionWindowMs: [Range(100, 10000)] - HotEventCount: [Range(10, 1000)] - MaxEvents: [Range(100, 5000)] (already existed) - Change ContextMappings from fixed 2000 to dynamic MaxEvents × 2 - Remove redundant validation code (now enforced by Range attributes) - Fix UndoReflectionHelper.GetPreviousValue to try nested value first - Clean up StdioBridgeHost: remove ineffective CleanupZombieListeners - Add task/conversation filtering to ActionTraceViewResource - Preserve modified-object identity in SelectionPropertyTracker Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…on in ActionTrace
…leaks Replace infinite recursive delayCall patterns with EditorApplication.update in PropertyChangeTracker and SamplingMiddleware. This ensures proper cleanup on domain reload and prevents memory leaks. Changes: - PropertyChangeTracker: Replace delayCall with update + FlushCheck() - SamplingMiddleware: Replace delayCall with update directly - Remove redundant helper methods (use UndoReflectionHelper/PropertyFormatter) - Clean up unused using statements Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Major ActionTrace system architecture refactoring:
- Add HookRegistry system for unified event hook registration
- Add EventCaptureRegistry for capture point management
- Add new Recorder architecture replacing legacy implementation
- Restructure directories: Core/{Models,Store,Settings}, Analysis/, Sources/
- Add Unity event hooks with EventArgs pattern
- Add built-in capture points system
Co-Authored-By: Claude <noreply@anthropic.com>
- Refactor ContextCompressor for better context handling - Enhance AssetCapture with improved tracking - Update SamplingMiddleware for better event sampling - Extend ToolCallScope with enhanced functionality - Remove ActionTraceSettings.Editor.cs (consolidated) - Update ActionTraceSettings core implementation - Improve EventStore.Diagnostics logging - Refine UndoToSequenceTool behavior - Major restructure of ActionTraceEditorWindow UI Co-Authored-By: Claude <noreply@anthropic.com>
…ActionTraceEditorWindow
…tion This architectural refactoring separates the Hooks system (general-purpose event infrastructure) from ActionTrace (one of its consumers) by introducing the IGameObjectCacheProvider interface for dependency injection. Changes: - Move HookEventArgs to Editor/Hooks/EventArgs/ namespace - Move UnityEventHooks to Editor/Hooks/UnityEventHooks/ namespace - Add IGameObjectCacheProvider interface for cache injection - Implement GameObjectTrackingCacheProvider in ActionTrace - Relocate GameObjectTrackingHelper to ActionTrace.Sources.Helpers - Simplify StdioBridgeHost socket binding logic - Add comprehensive documentation for Hooks system This allows UnityEventHooks to remain a pure detector without ActionTrace dependencies, while enabling other systems to provide their own cache provider implementations. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Deleted IEventCapturePoint interface, ContextStack class, ContextTimeline class, OperationContext class, ToolCallScope class, and associated event descriptors. - This cleanup reduces code complexity and removes unused features related to event capturing and context management in the MCPForUnity ActionTrace system.
- Add GetActionTraceSummaryTool: AI-friendly aggregated event summaries - Enhance GlobalIdHelper: add GetInstanceId and GetInstanceInfo methods - Refactor HookRegistry: simplify with generic InvokeSafely method - Update ActionTraceQuery: convert GlobalID to InstanceID/Name for display - Update Python models: add target_instance_id and target_name fields - Add query_mode parameter to get_action_trace tool - Fix undo_to_sequence example code Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ed values for simplification.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @whatevertogo, your pull request is larger than the review limit of 300000 diff characters
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughIntroduces a comprehensive ActionTrace system for Unity editor that captures, stores, analyzes, and queries editor events. Includes event capture infrastructure, semantic analysis (scoring, categorization, intent inference), filtering and sampling, persistence, context mapping, query projection, editor UI window, and Python MCP tools for accessing trace data. Spans capture hooks, event store, semantics pipeline, UX components, and server integration. Changes
Sequence DiagramssequenceDiagram
participant Unity as Unity Editor
participant Hook as HookRegistry
participant Cap as Capture<br/>(Recorder)
participant Store as EventStore
participant Query as ActionTraceQuery
participant Sem as Semantics<br/>(Scorer/Categorizer)
participant UI as Editor Window
Unity->>Hook: Property changed / Asset imported
Hook->>Cap: Event notification
Cap->>Store: Record(EditorEvent)
Note over Store: Apply SamplingMiddleware<br/>Apply Merging<br/>Notify subscribers
UI->>Store: Query(limit=50)
Store-->>UI: List<EditorEvent>
UI->>Query: Project(events)
Query->>Sem: Score(event)
Sem-->>Query: importance_score
Query->>Sem: Categorize(score)
Sem-->>Query: category
Query->>Sem: Infer(event, surrounding)
Sem-->>Query: inferred_intent
Query-->>UI: ActionTraceViewItem[]
UI->>UI: Render with colors & filters
sequenceDiagram
participant Server as Python Server
participant Unity as Unity Editor
participant Store as EventStore
participant Agg as TransactionAggregator
participant Client as AI Client
Client->>Server: get_action_trace(limit=50, include_semantics=true)
Server->>Unity: send command (ActionTraceViewResource)
Unity->>Store: QueryAll()
Store-->>Unity: events[]
alt summary_only=true
Unity->>Agg: Aggregate(events)
Agg-->>Unity: AtomicOperation[]
Unity-->>Server: aggregated_results
else include_semantics=true
Unity->>Query: Project(events)
Query-->>Unity: ActionTraceViewItem[] (with scores, categories, intents)
Unity-->>Server: semantic_results
else
Unity-->>Server: basic_results
end
Server-->>Client: EventQueryResult
Estimated Code Review Effort🎯 5 (Critical) | ⏱️ ~120 minutes This PR introduces a substantial, multi-layered system spanning event capture, storage, analysis, UI, and server integration. The changes are heterogeneous—mixing capture infrastructure, data structures, analytics, persistence, filtering, UI rendering, and tool/resource definitions—requiring separate reasoning for each cohort. Dense logic in sampling, merging, dehydration, and query projection; extensive infrastructure scaffolding across hooks and integrations; and novel semantic computation (scoring, categorization, intent inference) all contribute to high complexity. While some file changes are repetitive (e.g., many capture helpers), the breadth and interconnectedness of the system demand thorough cross-file reasoning and interaction validation. Possibly Related Issues
Possibly Related PRs
Suggested Reviewers
🚥 Pre-merge checks | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In `@MCPForUnity/Editor/ActionTrace/Core/Store/EventStore.Persistence.cs`:
- Around line 201-213: SaveToStorage currently copies _events and
_contextMappings without synchronization, risking concurrent mutation; take a
snapshot of the mutable state under the existing _queryLock and then perform
serialization and McpJobStateStore.SaveState outside the lock. Specifically,
inside SaveToStorage acquire _queryLock, copy _events and _contextMappings into
local lists and capture SequenceCounter/CurrentSchemaVersion into a new
EventStoreState local variable, release the lock, then call
McpJobStateStore.SaveState(StateKey, state) with that snapshot so SaveToStorage
no longer iterates shared _events/_contextMappings while unlocked.
- Around line 45-87: ScheduleSave currently returns immediately when throttled
(now - _lastSaveTime < MinSaveIntervalMs) which can leave _isDirty unsaved;
modify ScheduleSave so that when it skips due to throttling it schedules a retry
via EditorApplication.delayCall to re-call ScheduleSave after the remaining
throttle interval (or a small capped retry delay) instead of just returning,
while still respecting the _queryLock/_saveScheduled guards and avoiding
duplicate retries; reference ScheduleSave, _lastSaveTime, MinSaveIntervalMs,
_saveScheduled, _queryLock, _isDirty, EditorApplication.delayCall and
SaveToStorage to locate where to add the retry scheduling logic.
In `@MCPForUnity/Editor/Resources/ActionTrace/ActionTraceViewResource.cs`:
- Around line 349-440: The conversation_id filter currently allows AINote events
without a conversation_id to pass; update both ApplyTaskFilters and
ApplyTaskFiltersToProjected so that when conversationId is non-empty you require
the payload to contain "conversation_id" and that its ToString() equals
conversationId—i.e., if TryGetValue("conversation_id", out var convVal) returns
false, return false (just like the task_id branch), and otherwise compare
convVal?.ToString() != conversationId to reject non-matching values.
In `@MCPForUnity/Editor/Tools/GetActionTraceSummaryTool.cs`:
- Around line 73-88: The code ignores the requested time_range because
CalculateSinceSequence always returns null; update the flow so timeRange from
GetTimeRange(`@params`) is passed into CalculateSinceSequence and that function
computes a proper sinceSequence (e.g., convert timeRange to a timestamp/sequence
number or parse relative ranges) instead of returning null, then pass the
resulting sinceSequence into EventStore.Query(limit, sinceSequence) so Query
filters by the time window; apply the same fix to the other occurrence where
GetTimeRange/CalculateSinceSequence/EventStore.Query are used (the block around
the later occurrence referenced in the review).
- Around line 405-442: In ApplyTaskFilters, the conversation_id branch currently
only excludes events when a conversation_id exists but doesn't match; change it
to require a present matching value when conversationId is provided: in the
Where predicate for e.Type == "AINote" check
e.Payload.TryGetValue("conversation_id", out var convVal) and return false if
TryGetValue is false or convVal?.ToString() != conversationId (mirroring the
task_id logic), ensuring events missing conversation_id are filtered out when
conversationId is non-empty.
- Around line 185-196: The logic treats any event with importanceCategory ==
"critical" (or containing "error"/"exception" in evtTypeLower) as an error, but
AINote events are scored critical and should not be counted; update the
conditional in the error counting block (the if that currently uses
importanceCategory and evtTypeLower) to exclude events whose evt.Type equals
"AINote" (case-insensitive) — e.g., only increment errorCount and add to
errorEvents when the existing conditions are true AND evt.Type does not equal
"AINote" (use StringComparison.OrdinalIgnoreCase for the comparison).
In `@MCPForUnity/Editor/Tools/ManageAsset.cs`:
- Around line 290-294: The three callback invocations (OnAssetCreated,
OnAssetModified, OnAssetDeleted) should be wrapped in their own try-catch blocks
so subscriber exceptions cannot propagate and cause the outer method to return
ErrorResponse even after the asset operation and
AssetDatabase.SaveAssets()/DeleteAsset succeeded; modify the invocation sites
(the lines calling OnAssetCreated?.Invoke(fullPath, assetType),
OnAssetModified?.Invoke(fullPath, assetType), and
OnAssetDeleted?.Invoke(fullPath)) to catch and log any exception locally (e.g.,
processLogger or UnityEngine.Debug.LogException) and continue normal return
flow, preserving the successful response from the core create/modify/delete
methods.
In `@MCPForUnity/Editor/Windows/ActionTraceEditorWindow.cs`:
- Around line 682-808: SanitizeJson currently reuses and clears the shared field
_stringBuilder (used by BuildJsonExport), which corrupts the in-progress export;
modify SanitizeJson to allocate and use a local StringBuilder (e.g., new
StringBuilder with capacity based on input.Length) or otherwise avoid mutating
the shared _stringBuilder field, perform the escape loop on the local builder,
and return its ToString() so BuildJsonExport's _stringBuilder is not cleared or
modified.
- Around line 843-851: The Clear handler currently calls
_detailScrollView.Clear(), which removes
_detailPlaceholder/_detailContent/_detailActions from the visual tree and breaks
OnSelectionChanged; change OnClearClicked to preserve the placeholder element
instead of clearing the whole scroll view: do not call
_detailScrollView.Clear(), instead clear the contents of _detailContent and
_detailActions (e.g., remove their child elements or reset text) and ensure
_detailPlaceholder is made visible (or re-added) so the detail UI remains in the
visual tree for future selections; update references in OnSelectionChanged to
assume the placeholder exists.
- Around line 1203-1228: OnEditorUpdate currently calls RefreshEvents()
regardless of whether the UI was initialized in CreateGUI, which can lead to
null dereferences on _actionTraceQuery and _eventListView; add a guard at the
start of OnEditorUpdate to return early if those UI fields are null (e.g., if
_actionTraceQuery == null || _eventListView == null) or alternatively move the
call to RefreshEvents behind a boolean flag set by CreateGUI when initialization
succeeds; ensure the guard references OnEditorUpdate, RefreshEvents,
_actionTraceQuery, _eventListView and preserves the existing scheduled refresh
logic.
In `@Server/src/services/tools/add_action_trace_note.py`:
- Around line 113-122: The generated IDs for effective_task_id and
effective_conv_id are not persisted back to session context, causing new IDs to
be created on subsequent calls; after auto-generating (in the block that sets
effective_task_id = f"task-{uuid.uuid4().hex[:8]}" and effective_conv_id =
f"conv-{uuid.uuid4().hex[:8]}"), assign those values into the context stores
(_current_task_id.set(effective_task_id) and
_current_conversation_id.set(effective_conv_id)) so future calls that read
_current_task_id.get() and _current_conversation_id.get() see the same IDs and
preserve task-level grouping.
🟡 Minor comments (25)
docs/Hooks/CONTRIBUTING.md-337-337 (1)
337-337: Add language specifier to fenced code block.Markdownlint flagged this fenced code block for missing a language specifier.
📝 Proposed fix
-``` +```text feat(Hooks): add undo/redo event hooksdocs/ActionTrace/CONTRIBUTING.md-208-217 (1)
208-217: Clarify test command format and context.Line 210 shows
call_tool("run_tests", {"mode": "EditMode"})which appears to be MCP client syntax, not a bash command. This is confusing within abashcode block that also contains actual shell commands. Consider:
- Separating MCP tool invocations from bash commands into different code blocks
- Adding context about when to use MCP vs. Unity Editor directly
- Using a different language identifier (e.g.,
pythonortext) for the MCP tool syntax📝 Suggested improvement
### Run Tests +**Via MCP Tool:** +```python +# Unity tests via MCP client +call_tool("run_tests", {"mode": "EditMode"}) +``` + +**Via Command Line:** ```bash -# Unity tests via MCP -call_tool("run_tests", {"mode": "EditMode"}) - # Python tests cd Server && uv run pytest tests/ -v # With coverage uv run pytest tests/ --cov --cov-report=html</details> </blockquote></details> <details> <summary>docs/ActionTrace/CONTRIBUTING.md-35-39 (1)</summary><blockquote> `35-39`: **Update version requirements to match actual project minimums.** The CONTRIBUTING.md table has overstated requirements. According to project documentation and configuration: - **Python** should be **3.10+** (not 3.11+) — confirmed in `Server/pyproject.toml` (`requires-python = ">=3.10"`), main `README.md`, and `Server/README.md` - **Unity** should be **2021.3 LTS+** (not 2022.3+) — confirmed in main `README.md`, `MCPForUnity/package.json`, and `Server/README.md`; ActionTrace code contains no version-specific preprocessor directives requiring 2022.3+ - **Git** should specify a concrete minimum (e.g., **2.30+** instead of "Latest") for reproducibility <details> <summary>Updated table</summary> | Tool | Version | Purpose | |------|---------|---------| | Unity | 2021.3 LTS+ | Run and test | | Python | 3.10+ | MCP Server | | Git | 2.30+ | Version control | </details> </blockquote></details> <details> <summary>Server/src/services/tools/add_action_trace_note.py-32-67 (1)</summary><blockquote> `32-67`: **Handle leading whitespace in JSON-string arrays.** Line 52 checks `startswith("[")` without trimming, so valid inputs like `" [1,2]"` skip JSON parsing and the CSV fallback fails. Normalize the string once and use it for both JSON and CSV paths. <details> <summary>🔧 Suggested fix</summary> ```diff - if isinstance(value, str): - if not value.strip(): - return None - # Try parsing JSON array first - if value.startswith("["): - try: - parsed = json.loads(value) + if isinstance(value, str): + stripped = value.strip() + if not stripped: + return None + # Try parsing JSON array first + if stripped.startswith("["): + try: + parsed = json.loads(stripped) if isinstance(parsed, list): return _coerce_int_list(parsed) # Recursively handle parsed list except json.JSONDecodeError: pass # Try comma-delimited integers try: - return [int(v.strip()) for v in value.split(",") if v.strip()] + return [int(v.strip()) for v in stripped.split(",") if v.strip()] except ValueError: passdocs/ActionTrace/DESIGN.md-43-46 (1)
43-46: Add language identifiers to fenced code blocks (MD040).
Markdownlint flags several code blocks missing a language specifier.✅ Example fix (apply similarly to other blocks)
-``` +```text Query (Query) → Semantics (Semantics) → Context (Context) → Capture (Capture) → Sources (Event Sources) → Core (Data)```diff -```csharp +```csharp public readonly struct EditorEvent { public readonly long Sequence; public readonly long TimestampUnixMs; public readonly string Type; public readonly string Payload; // JSON }</details> Also applies to: 71-90, 114-117, 132-135, 202-205, 209-212, 216-219, 282-290, 294-298 </blockquote></details> <details> <summary>MCPForUnity/Editor/Hooks/EventArgs/HookEventArgs.cs-121-131 (1)</summary><blockquote> `121-131`: **Consider caching Owner data instead of holding a live GameObject reference.** The `Owner` property holds a live GameObject reference that can become invalid if the owning GameObject is destroyed before or during event consumption. Unlike `GameObjectDestroyedArgs`, which caches data (Name, InstanceId, GlobalId) to avoid this issue, `ComponentRemovedArgs` exposes a reference that may be destroyed in the same operation (e.g., parent destruction cascades to children and their components). While the current consumer (Recorder) implements defensive null checks, this pattern is inconsistent with the safer design of `GameObjectDestroyedArgs` and creates a trap for future consumers. Consider caching `Owner.name` and `Owner.GetInstanceID()` instead, matching the proven pattern used for GameObject destruction events. </blockquote></details> <details> <summary>docs/Hooks/DESIGN.md-36-63 (1)</summary><blockquote> `36-63`: **Add languages to fenced code blocks (MD040).** Both ASCII diagram and file-structure blocks should declare a language to satisfy markdownlint. <details> <summary>📝 Suggested doc lint fix</summary> ```diff -``` +```text @@ -``` +```text @@ -``` +```text @@ -``` +```textAlso applies to: 398-408
MCPForUnity/Editor/ActionTrace/Capture/Capturers/SelectionCapture.cs-56-191 (1)
56-191: Multi-selection edits to non-active objects are dropped.The code only matches modifications against the active selection (
_currentSelectionGlobalId). When multiple objects are selected and properties are edited, the Inspector modifies all selected objects, but modifications on non-active selections are filtered out byIsTargetMatchSelection(). Consider tracking all selected object IDs if multi-selection property modifications should be recorded, while keeping the active object forselection_context.💡 Possible adjustment to capture multi-selection
private static string _currentSelectionPath; + private static readonly HashSet<string> _currentSelectionIds = new(); private static void UpdateSelectionState() { var activeObject = Selection.activeObject; if (activeObject == null) { _currentSelectionGlobalId = null; _currentSelectionName = null; _currentSelectionType = null; _currentSelectionPath = null; + _currentSelectionIds.Clear(); return; } + _currentSelectionIds.Clear(); + foreach (var obj in Selection.objects) + { + var id = GlobalIdHelper.ToGlobalIdString(obj); + if (!string.IsNullOrEmpty(id)) + _currentSelectionIds.Add(id); + } + _currentSelectionGlobalId = GlobalIdHelper.ToGlobalIdString(activeObject); _currentSelectionName = activeObject.name; _currentSelectionType = activeObject.GetType().Name;private static bool IsTargetMatchSelection(UnityEngine.Object target, string targetGlobalId) { - if (targetGlobalId == _currentSelectionGlobalId) + if (_currentSelectionIds.Contains(targetGlobalId)) return true; if (target is Component comp) { string gameObjectId = GlobalIdHelper.ToGlobalIdString(comp.gameObject); - if (gameObjectId == _currentSelectionGlobalId) + if (_currentSelectionIds.Contains(gameObjectId)) return true; } return false;MCPForUnity/Editor/Tools/UndoToSequenceTool.cs-105-111 (1)
105-111: Handle “already at target” as a no‑op success.
If the target sequence is the most recent recorded event,eventsAfterTargetwill be empty and the tool currently returns an error. This should be treated as a successful no‑op.✅ Proposed fix
- if (eventsAfterTarget.Count == 0) - { - return new ErrorResponse($"No events found after sequence {targetSequence}."); - } + if (eventsAfterTarget.Count == 0) + { + return new SuccessResponse( + $"Already at sequence {targetSequence}; no Undo operations needed." + ); + }MCPForUnity/Editor/ActionTrace/Capture/Recorder.cs-221-301 (1)
221-301: Add null guards to detailed scene and build handlers for consistency.Five detailed handlers (
OnSceneOpenedDetailed,OnNewSceneCreatedDetailed,OnScriptCompiledDetailed,OnScriptCompilationFailedDetailed,OnBuildCompletedDetailed) directly access args properties without null checks, while similar handlers (OnComponentRemovedDetailed,OnGameObjectDestroyedDetailed) do include them. While current event sources always provide non-null args, the inconsistent pattern creates a defensive gap. Add null guards to all detailed handlers to maintain a uniform contract.MCPForUnity/Editor/ActionTrace/Helpers/ActionTraceHelper.cs-43-61 (1)
43-61: Handle underscores before uppercase segments in tool names.
The regex pattern(^|_)([a-z])only matches lowercase letters after underscores, so inputs like"add_ActionTrace_note"keep the underscore before"ActionTrace"untouched. The proposed change to(^|_)([A-Za-z])correctly handles both cases.🐛 Proposed fix
- "(^|_)([a-z])", + "(^|_)([A-Za-z])",MCPForUnity/Editor/ActionTrace/Integration/VCS/VcsContextProvider.cs-269-274 (1)
269-274: Missing timeout on PATH verification process.
FindGitExecutablecallsprocess.WaitForExit()without a timeout when verifying git is in PATH. If git is slow to start or hangs, this could freeze the editor.🔧 Suggested fix
using (var process = Process.Start(startInfo)) { - process.WaitForExit(); - if (process.ExitCode == 0) + if (process.WaitForExit(2000) && process.ExitCode == 0) return "git"; // Found in PATH }MCPForUnity/Editor/ActionTrace/Core/Store/EventStore.Diagnostics.cs-14-19 (1)
14-19: Documentation claims caller holds lock, but method acquires lock.The doc comment says "Caller must hold _queryLock before calling" but the method itself acquires the lock on line 25. This is redundant (reentrant lock) or the documentation is incorrect.
📝 Fix documentation or remove redundant lock
If
_queryLockis a reentrant lock (likeobjectwithlock), the nested lock works but is unnecessary overhead. If documentation is correct, remove the inner lock:- /// Thread safety: Caller must hold _queryLock before calling. - /// All _events access happens within the same lock to prevent concurrent modification. + /// Thread safety: Acquires _queryLock internally. /// </summary> private static void DehydrateOldEvents(int hotEventCount) { // Clamp to non-negative to prevent negative iteration hotEventCount = Math.Max(0, hotEventCount); - lock (_queryLock) - { int count = _events.Count; // ... - } }MCPForUnity/Editor/ActionTrace/Core/Store/EventStore.Diagnostics.cs-63-67 (1)
63-67: Remove unnecessary null check.The null check at lines 65-67 is defensive coding for a scenario that cannot occur.
Dehydrate()always returns anEditorEventinstance (never null), and no other code path introduces nulls into_events. The comment attributing nulls toDehydrateOldEventsis inaccurate—that method only replaces events with dehydrated instances.MCPForUnity/Editor/Hooks/UnityEventHooks/UnityEventHooks.Advanced.cs-239-257 (1)
239-257: Replace reflection on internal API with public alternatives or document the version dependency.
GetCompilationErrorCountuses reflection onUnityEditor.Scripting.ScriptCompilationErrorCount, which is not a documented or publicly supported Unity API. While the try-catch fallback is good defensive coding, consider:
- Replace with public alternatives like
CompilationPipeline,EditorUtility.scriptCompilationFailed, orEditorApplication.logMessageReceived(the codebase already usesCompilationPipelineelsewhere)- If you must use reflection, add an explanatory comment noting the internal API dependency and version fragility
- Test against Unity 6+ before release, as the codebase already has version-specific handling for this
MCPForUnity/Editor/Windows/ActionTraceEditorWindow.uss-446-451 (1)
446-451: Detail panelmax-widthis smaller thanmin-width, creating contradictory layout constraints.
min-width: 320pxwithmax-width: 300pxcauses themax-widthto be ignored by the layout system. Setmax-width≥min-widthor remove it.🔧 Possible adjustment
- min-width: 320px; - max-width: 300px; + min-width: 320px; + max-width: 420px;MCPForUnity/Editor/Windows/ActionTraceEditorWindow.cs-451-456 (1)
451-456: UseAppendSeparator()for menu separators instead of emptyAppendActionentries.
AppendActionwith an empty label creates a clickable empty entry. UI Toolkit providesAppendSeparator()specifically for adding visual separators to menus.🔧 Proposed fix
- _filterMenu?.menu.AppendAction("", a => { }); // Separator + _filterMenu?.menu.AppendSeparator(); ... - _filterMenu?.menu.AppendAction("", a => { }); // Separator + _filterMenu?.menu.AppendSeparator();MCPForUnity/Editor/Windows/ActionTraceEditorWindow.cs-1043-1051 (1)
1043-1051: Swap sort order to match UI label: "By Importance (AI First)"The UI menu and button label explicitly state "By Importance (AI First)" and "Sort: Importance," but the current
AIFilteredimplementation sorts by timestamp first, then importance. This contradicts the expected behavior. Importance should be the primary sort key.🔧 Required adjustment
- SortMode.AIFiltered => source - .OrderByDescending(e => e.Event.TimestampUnixMs) - .ThenByDescending(e => e.ImportanceScore), + SortMode.AIFiltered => source + .OrderByDescending(e => e.ImportanceScore) + .ThenByDescending(e => e.Event.TimestampUnixMs),MCPForUnity/Editor/Hooks/UnityEventHooks/UnityEventHooks.cs-177-183 (1)
177-183: SelectionChanged should useSelection.activeGameObject.
CastingSelection.activeObjecttoGameObjectdrops selections when a Component is active.🛠️ Proposed fix
- GameObject selectedGo = Selection.activeObject as GameObject; + GameObject selectedGo = Selection.activeGameObject;MCPForUnity/Editor/Tools/GetActionTraceSummaryTool.cs-146-150 (1)
146-150: Duration calculation shouldn’t assume event order.
If events are newest-first,durationMswill go negative. Using min/max timestamps is safer.🛠️ Proposed fix
- long startTimeMs = events[0].TimestampUnixMs; - long endTimeMs = events[events.Count - 1].TimestampUnixMs; + long startTimeMs = events.Min(e => e.TimestampUnixMs); + long endTimeMs = events.Max(e => e.TimestampUnixMs);MCPForUnity/Editor/Tools/AddActionTraceNoteTool.cs-147-163 (1)
147-163: Handle scalarrelated_sequencesinputs.
Right now only arrays parse successfully; a single numeric value is silently ignored. Consider supporting scalar values to avoid dropping valid links.🛠️ Proposed fix to accept scalar values
- var relatedSeqs = relatedSeqToken.ToObject<long[]>(); - if (relatedSeqs != null && relatedSeqs.Length > 0) - { - payload["related_sequences"] = relatedSeqs; - } + long[] relatedSeqs = null; + if (relatedSeqToken.Type == JTokenType.Array) + { + relatedSeqs = relatedSeqToken.ToObject<long[]>(); + } + else if (relatedSeqToken.Type == JTokenType.Integer) + { + relatedSeqs = new[] { relatedSeqToken.Value<long>() }; + } + else if (long.TryParse(relatedSeqToken.ToString(), out var seqVal)) + { + relatedSeqs = new[] { seqVal }; + } + + if (relatedSeqs != null && relatedSeqs.Length > 0) + { + payload["related_sequences"] = relatedSeqs; + }MCPForUnity/Editor/ActionTrace/Core/Models/EditorEvent.cs-163-171 (1)
163-171:GetSummary()mutates state, contradicting the "immutable class" documentation.The class documentation (line 12-13) states this is an immutable class ("once written, never modified"), but
GetSummary()mutatesPrecomputedSummary. While this is a benign lazy-initialization pattern, it technically violates immutability and could cause issues in multi-threaded scenarios (non-atomic write).Consider either:
- Computing the summary in the constructor (truly immutable)
- Using
Lazy<string>for thread-safe lazy initialization- Updating the documentation to clarify this is "effectively immutable"
MCPForUnity/Editor/ActionTrace/Analysis/Query/ActionTraceQuery.cs-200-201 (1)
200-201:ProjectWithContextalways passesnullfor surrounding context, limiting intent inference.Unlike
Project()which computes a context window for intent inference,ProjectWithContext()always passessurrounding: null. This means intent inference will be less accurate for events queried with context. The comment mentions "avoid List allocation" but this fundamentally changes the inference behavior.Consider aligning the behavior or documenting this as an intentional trade-off.
MCPForUnity/Editor/ActionTrace/Sources/Helpers/GameObjectTrackingHelper.cs-60-65 (1)
60-65: Redundant dictionary re-creation afterClear().Lines 63 and 65 create new dictionaries immediately after calling
Clear()on the existing ones. EitherClear()alone or new allocation is needed, not both.Proposed fix
_previousInstanceIds.Clear(); _previousInstanceIds.EnsureCapacity(256); - _nameCache.Clear(); - _nameCache = new Dictionary<int, string>(256); - _globalIdCache.Clear(); - _globalIdCache = new Dictionary<int, string>(256); + _nameCache.Clear(); + _globalIdCache.Clear();MCPForUnity/Editor/Helpers/GlobalIdHelper.cs-139-141 (1)
139-141:GameObject.Findmay fail to resolve objects in non-active scenes or with duplicate paths.
GameObject.Findonly searches active GameObjects in loaded scenes and returns the first match. If the scene is loaded but the object is inactive, or if multiple objects share the same hierarchy path, this lookup will fail or return the wrong object.Consider using scene-scoped search instead:
Proposed fix
// Find GameObject by hierarchy path - var found = GameObject.Find(hierarchyPath); - return found; + // Search within the specific scene for better accuracy + foreach (var rootGo in scene.GetRootGameObjects()) + { + if (rootGo.name == hierarchyPath || hierarchyPath.StartsWith(rootGo.name + "/")) + { + var found = rootGo.transform.Find(hierarchyPath.Substring(rootGo.name.Length).TrimStart('/')); + if (found != null) + return found.gameObject; + if (rootGo.name == hierarchyPath) + return rootGo; + } + } + return null;
🧹 Nitpick comments (43)
Server/src/models/action_trace.py (1)
71-84: Consider adding a computed property for hydration ratio.The
EventStatisticsmodel tracks bothdehydrated_countandhydrated_count. A computed property for the hydration ratio could be useful for monitoring memory optimization effectiveness.💡 Optional enhancement
`@property` def hydration_ratio(self) -> float | None: """Ratio of hydrated events (0.0-1.0), or None if no events.""" total = self.dehydrated_count + self.hydrated_count return self.hydrated_count / total if total > 0 else NoneNote: This would require
model_config = ConfigDict(computed=True)or exposing via a method instead.Server/src/services/tools/get_action_trace_settings.py (2)
12-12: Unused import.
Annotatedis imported but not used in this file sinceget_action_trace_settingstakes no annotated parameters.🧹 Remove unused import
-from typing import Annotated, Any +from typing import Any
53-54: Broad exception handling is acceptable here, but consider using explicit conversion.The broad
Exceptioncatch at the tool boundary is reasonable for providing user-friendly error messages. Per static analysis (RUF010), consider using an explicit conversion flag.💡 Use explicit conversion flag
except Exception as e: - return {"success": False, "message": f"Python error getting action trace settings: {str(e)}"} + return {"success": False, "message": f"Python error getting action trace settings: {e!s}"}Server/src/services/tools/undo_to_sequence.py (1)
91-99: Missing exception handling for consistency with other tools.Unlike
get_action_trace_settingsandget_action_trace, this tool lacks a try/except wrapper. Ifsend_with_unity_instanceraises an unexpected exception, it will propagate as an unhandled error instead of returning a structured failure response.🔧 Add exception handling for consistency
+ try: # Send command to Unity response = await send_with_unity_instance( async_send_command_with_retry, unity_instance, "undo_to_sequence", params_dict, ) return response if isinstance(response, dict) else {"success": False, "message": str(response)} + + except Exception as e: + return {"success": False, "message": f"Python error in undo_to_sequence: {e!s}"}Server/src/services/tools/get_action_trace_summary.py (1)
107-122: Missing exception handling for consistency.Similar to
undo_to_sequence.py, this tool lacks a try/except wrapper around the Unity transport call. Consider adding exception handling for consistent error reporting.🔧 Add exception handling
+ try: # Send command to Unity response = await send_with_unity_instance( async_send_command_with_retry, unity_instance, "get_action_trace_summary", params_dict, ) # Preserve structured failure data; unwrap success into a friendlier shape if isinstance(response, dict) and response.get("success"): return { "success": True, "message": response.get("message", "Generated ActionTrace summary."), "data": response.get("data") } return response if isinstance(response, dict) else {"success": False, "message": str(response)} + + except Exception as e: + return {"success": False, "message": f"Python error getting action trace summary: {e!s}"}Server/src/services/tools/get_action_trace.py (1)
102-103: Use explicit conversion flag per static analysis.💡 Use explicit conversion flag (RUF010)
except Exception as e: - return {"success": False, "message": f"Python error getting action trace: {str(e)}"} + return {"success": False, "message": f"Python error getting action trace: {e!s}"}Server/src/services/resources/action_trace.py (3)
17-17: Unused import.
ToolAnnotationsis imported but not used. This file usesmcp_for_unity_resourcewhich doesn't requireToolAnnotations.🧹 Remove unused import
from fastmcp import Context -from mcp.types import ToolAnnotations
171-176: Silent exception swallowing reduces observability.Unlike
get_action_trace(line 114), this function silently continues on parse errors without logging. Per static analysis (S112), consider logging for debugging purposes.🔧 Add logging for parse failures
for event_dict in events_data: try: event = EditorEvent(**event_dict) events.append(event) - except Exception: + except Exception as e: + ctx.error(f"Failed to parse event for statistics: {e}") # type: ignore[reportUnusedCoroutine] continue
155-161: Hardcoded limit of 1000 events for statistics.The statistics query uses a hardcoded
limit: 1000. For large event stores, this may not represent the full dataset. Consider making this configurable or documenting this limitation in the docstring.docs/ActionTrace/CONTRIBUTING.md (1)
3-3: Minor grammar improvement.The phrase "Welcome to contribute" is slightly awkward. Consider: "Welcome! Thanks for contributing to the Unity-mcp ActionTrace system!" or "Welcome to the Unity-mcp ActionTrace contributor guide!"
MCPForUnity/Editor/ActionTrace/Core/Store/EventStore.Merging.cs (1)
115-123: Consider handling potentialOverflowExceptionfromchecked()arithmetic.The
checked()casts on lines 118-119 can throwOverflowExceptioniflordexceedsint.MaxValue. While unlikely in practice (merge counts shouldn't reach billions), a defensive approach would be to cap the value or useunchecked()with saturation logic.♻️ Optional: Saturate instead of throwing
mergeCount = existingCount switch { int i => i + 1, - long l => checked((int)l + 1), - double d => checked((int)d + 1), + long l => l >= int.MaxValue ? int.MaxValue : (int)l + 1, + double d => d >= int.MaxValue ? int.MaxValue : (int)d + 1, string s when int.TryParse(s, out int parsed) => parsed + 1, _ => 2 // Fallback for unknown types };MCPForUnity/Editor/ActionTrace/Integration/AssetBridge.cs (1)
35-74: Preserve stack traces in warning logs.
Usingex.Messagedrops stack context; loggingex.ToString()keeps it for diagnostics.♻️ Suggested tweak
- McpLog.Warn($"[ManageAssetBridge] Failed to record asset modification: {ex.Message}"); + McpLog.Warn($"[ManageAssetBridge] Failed to record asset modification: {ex}"); ... - McpLog.Warn($"[ManageAssetBridge] Failed to record asset creation: {ex.Message}"); + McpLog.Warn($"[ManageAssetBridge] Failed to record asset creation: {ex}"); ... - McpLog.Warn($"[ManageAssetBridge] Failed to record asset deletion: {ex.Message}"); + McpLog.Warn($"[ManageAssetBridge] Failed to record asset deletion: {ex}");MCPForUnity/Editor/Tools/ActionTraceSettingsTool.cs (1)
15-16: Consider adding description to the tool attribute.Based on learnings, the
descriptionparameter in theMcpForUnityToolattribute should always be included as a best practice. Without it, there's a higher chance that MCP clients will not parse the tool correctly.Suggested change
- [McpForUnityTool("get_action_trace_settings")] + [McpForUnityTool("get_action_trace_settings", description: "Retrieves the current ActionTrace system configuration settings.")] public static class ActionTraceSettingsToolMCPForUnity/Editor/ActionTrace/Semantics/DefaultIntentInferrer.cs (1)
71-105: Code duplication withDefaultEventScorer.The helper methods
IsScript,IsScene, andIsPrefabduplicate similar logic found inDefaultEventScorer.cs. Consider extracting these into a shared utility class (e.g.,AssetTypeDetector) to centralize asset type detection and reduce maintenance burden.MCPForUnity/Editor/ActionTrace/Semantics/DefaultEventScorer.cs (2)
60-61: Redundant null check forPayload.The
Scoremethod already returns early at line 38-39 whenevt.Payload == null, so this null check inGetPayloadAdjustmentis unreachable under current call patterns. However, keeping it provides defensive safety if the method is ever called directly in the future.
80-106: Code duplication withDefaultIntentInferrer.The
IsScript,IsScene, andIsPrefabhelpers are similar to those inDefaultIntentInferrer.csbut with subtle differences (checking"asset_type"vs"component_type"). Consider unifying these into a shared utility with configurable payload keys, or document why the differences are intentional.MCPForUnity/Editor/ActionTrace/Core/Presets/ActionTracePreset.cs (1)
95-98:AllPresetslist is mutable and can be modified at runtime.The
List<ActionTracePreset>is exposed aspublic static readonly, butreadonlyonly prevents reassignment—not mutation. External code could callAllPresets.Add(),Clear(), or modify elements. Consider usingIReadOnlyList<ActionTracePreset>for the public API or returning a copy in accessor methods.Suggested change
- public static readonly List<ActionTracePreset> AllPresets = new() + private static readonly List<ActionTracePreset> _allPresets = new() { DebugAll, Standard, Lean, AIFocused, Realtime, Performance }; + + public static IReadOnlyList<ActionTracePreset> AllPresets => _allPresets;MCPForUnity/Editor/ActionTrace/Helpers/ActionTraceHelper.cs (1)
20-29: Deduplicate payload lookup usage.
MCPForUnity/Editor/ActionTrace/Analysis/Summarization/EventSummarizer.cs (Line 402-409) duplicates this helper; consider switching toActionTraceHelper.GetPayloadStringto centralize behavior.MCPForUnity/Editor/ActionTrace/Capture/Sampling/SamplingMiddleware.cs (1)
217-237: Unused variableoldestSample.
oldestSampleis assigned on line 226 but never used. The code correctly usesremovedSamplefromTryRemoveinstead.♻️ Remove unused variable
// Manual loop to find oldest entry (avoid LINQ allocation in hot path) string oldestKey = null; long oldestTimestamp = long.MaxValue; - PendingSample oldestSample = default; foreach (var kvp in _pendingSamples) { if (kvp.Value.TimestampMs < oldestTimestamp) { oldestTimestamp = kvp.Value.TimestampMs; oldestKey = kvp.Key; - oldestSample = kvp.Value; } }MCPForUnity/Editor/Hooks/UnityEventHooks/UnityEventHooks.Advanced.cs (1)
259-278: Consider direct call instead of reflection for same-assembly type.
GetBuildTargetNameuses reflection to callBuildTargetUtility.GetBuildTargetNamein the same assembly. Unless there's a specific reason (e.g., optional dependency, avoiding circular references), a direct call would be simpler and more maintainable.♻️ Suggested simplification
private static string GetBuildTargetName(BuildTarget target) { - try - { - var assembly = typeof(HookRegistry).Assembly; - var type = assembly.GetType("MCPForUnity.Editor.Helpers.BuildTargetUtility"); - if (type != null) - { - var method = type.GetMethod("GetBuildTargetName", System.Reflection.BindingFlags.Static | System.Reflection.BindingFlags.Public); - if (method != null) - { - var result = method.Invoke(null, new object[] { target }); - if (result is string name) return name; - } - } - } - catch { } - - return target.ToString(); + // Direct call if BuildTargetUtility exists and is accessible + // return MCPForUnity.Editor.Helpers.BuildTargetUtility.GetBuildTargetName(target); + return target.ToString(); // Fallback }MCPForUnity/Editor/ActionTrace/Integration/VCS/VcsContextProvider.cs (2)
51-67: Missing unsubscribe fromEditorApplication.updateon domain reload.The static constructor subscribes to
EditorApplication.updatebut doesn't unsubscribe before re-subscribing. UnlikeSamplingMiddleware.cswhich does-=then+=, this could accumulate duplicate subscriptions across domain reloads (though Unity typically clears static state on domain reload).🔧 Suggested fix for safe re-registration
static VcsContextProvider() { _currentContext = GetInitialContext(); + EditorApplication.update -= OnUpdate; EditorApplication.update += OnUpdate; }
172-174: UnusedgitPathvariable.
gitPathis declared but never used inRunGitCommand. It appears to be leftover from a different implementation.♻️ Remove unused variable
try { var projectPath = System.IO.Path.GetDirectoryName(UnityEngine.Application.dataPath); - var gitPath = System.IO.Path.Combine(projectPath, ".git"); // Find git executableMCPForUnity/Editor/ActionTrace/Core/Store/EventStore.Context.cs (1)
172-183: Events may appear multiple times in results (by design).The
SelectManycorrectly handles the multi-agent scenario where one event has multiple contexts. However, callers might not expect the sameEventto appear multiple times in the result. Consider adding documentation to the method signature about this behavior.📝 Suggested documentation enhancement
/// <summary> /// Query events with their context associations. - /// Returns a tuple of (Event, Context) where Context may be null. + /// Returns a tuple of (Event, Context) where Context may be null. + /// Note: An event with multiple contexts will appear multiple times in the result, + /// once for each context association. /// </summary>MCPForUnity/Editor/ActionTrace/Core/EventTypes.cs (1)
30-103: ExposeAINoteas a constant to keep the type list centralized.Metadata uses the
"AINote"literal, which undercuts the “single source of truth” goal and diverges from the otherEventTypesclass inActionTraceViewResource.cs. Add a constant here and reference it in the metadata map.♻️ Proposed fix
// Build events public const string BuildStarted = "BuildStarted"; public const string BuildCompleted = "BuildCompleted"; public const string BuildFailed = "BuildFailed"; + public const string AINote = "AINote"; @@ - ["AINote"] = new EventMetadata + [AINote] = new EventMetadata { Category = EventCategory.System, DefaultImportance = 1.0f, SummaryTemplate = "AI Note{if:agent_id, ({agent_id})}: {note}", },MCPForUnity/Editor/Hooks/HookTest.cs (1)
1-84: Consider removing or gating the commented-out HookTest scaffold.The entire file is commented out, so it adds maintenance noise without providing a runnable test. Either delete it or wrap an uncommented version in a compile symbol (e.g.,
MCP_HOOK_TEST) so it can be enabled intentionally.MCPForUnity/Editor/Helpers/GlobalIdHelper.cs (1)
262-325:GetInstanceInfoduplicates display name extraction logic fromGetDisplayName.The display name extraction logic in
GetInstanceInfo(lines 275-322) largely duplicates what's inGetDisplayName(lines 196-256). Consider extracting a shared helper to reduce maintenance burden.Refactor suggestion
+ /// <summary> + /// Extract display name from ID string when object is not resolvable. + /// </summary> + private static string ExtractDisplayNameFromId(string globalIdStr) + { +#if UNITY_2020_3_OR_NEWER + if (GlobalObjectId.TryParse(globalIdStr, out var globalId)) + { + var guidStr = globalId.assetGUID.ToString(); + return guidStr.Length >= 8 + ? $"[{globalId.identifierType} {guidStr.Substring(0, 8)}...]" + : $"[{globalId.identifierType} {guidStr}]"; + } +#endif + // ... rest of extraction logic + }MCPForUnity/Editor/Hooks/HookRegistry.cs (1)
107-127: Consider avoidingdynamicdispatch for better performance and type safety.Using
dynamicincurs DLR (Dynamic Language Runtime) overhead on every invocation, which could be problematic for high-frequency events likeOnEditorUpdate. Since delegate signatures are known at compile time, consider using strongly-typed overloads orDelegate.DynamicInvoke(which is still slow but avoids DLR).A more efficient pattern:
Suggested refactor
- private static void InvokeSafely<TDelegate>(TDelegate handler, string eventName, Action<dynamic> invoke) - where TDelegate : class + private static void InvokeSafely(Delegate handler, string eventName, object[] args) { if (handler == null) return; - // Cast to Delegate base type to access GetInvocationList() - var multicastDelegate = handler as Delegate; - if (multicastDelegate == null) return; - - foreach (Delegate subscriber in multicastDelegate.GetInvocationList()) + foreach (Delegate subscriber in handler.GetInvocationList()) { try { - invoke(subscriber); + subscriber.DynamicInvoke(args); } catch (Exception ex) { McpLog.Warn($"[HookRegistry] {eventName} subscriber threw exception: {ex.Message}"); } } } + + // Or better: use type-specific helpers + private static void InvokeSafely(Action handler, string eventName) + { + if (handler == null) return; + foreach (Action subscriber in handler.GetInvocationList()) + { + try { subscriber(); } + catch (Exception ex) { McpLog.Warn($"[HookRegistry] {eventName} subscriber threw: {ex.Message}"); } + } + }MCPForUnity/Editor/ActionTrace/Sources/Helpers/GameObjectTrackingHelper.cs (2)
125-150:changeslist includes all current objects, not just changed ones.The variable name
changesand the struct fieldChangessuggest only changed objects, but line 149 adds every current GameObject regardless of whether it's new. Only theisNewflag distinguishes them. Consider renaming tocurrentObjectsor filtering to only include new objects if that's the intent.
186-191: Consider swapping references instead of copying elements.The current code clears
_previousInstanceIdsand re-adds all elements fromcurrentIds. A more efficient approach:Proposed fix
- // Update tracking for next call - _previousInstanceIds.Clear(); - foreach (int id in currentIds) - { - _previousInstanceIds.Add(id); - } + // Update tracking for next call (swap references) + (_previousInstanceIds, currentIds) = (currentIds, _previousInstanceIds); + currentIds.Clear(); // Will be GC'd or reusedMCPForUnity/Editor/ActionTrace/Analysis/Query/ActionTraceQuery.cs (2)
103-104: Edge case: order detection may be unreliable with single-element lists.Line 104 compares
events[0].Sequence > events[events.Count - 1].Sequence, which works for lists with 2+ elements. For a single-element list, both indices refer to the same element, makingisDescending = false. This is correct but subtle—a brief comment would help.
288-372: Consider usinginitaccessors forActionTraceViewItemproperties.
ActionTraceViewItemis described as a computed projection, yet all properties have public setters allowing mutation after creation. Using C# 9+initaccessors would enforce immutability after construction.Example
- public EditorEvent Event { get; set; } + public EditorEvent Event { get; init; } // ... same for other propertiesMCPForUnity/Editor/ActionTrace/Capture/Capturers/AssetCapture.cs (5)
191-213: Inconsistent deduplication cleanup for deleted assets.For imported assets (lines 156-158), junk assets are removed from
_processedAssetsInSessionafter filtering. However, for deleted assets, the path is added to the set but not removed when it's a junk asset. This causes unnecessary cache growth.♻️ Suggested fix
// L1 Blacklist: Skip junk assets if (!EventFilter.ShouldTrackAsset(assetPath)) + { + _processedAssetsInSession.Remove(assetPath); continue; + }
216-241: Same inconsistency for moved assets.Similar to deleted assets, junk moved assets are added to
_processedAssetsInSessionbut not removed when filtered. Apply the same fix pattern for consistency.♻️ Suggested fix
// L1 Blacklist: Skip junk assets if (!EventFilter.ShouldTrackAsset(movedAssets[i])) + { + _processedAssetsInSession.Remove(movedAssets[i]); continue; + }
248-268: Comment/implementation mismatch.The doc comment states "time-based expiration (30 minutes)" but the implementation uses count-based cleanup (MaxCacheSize = 1000). Either update the comment to reflect the actual behavior or implement the time-based approach if that was the intent.
♻️ Suggested comment fix
/// <summary> /// Cleanup old entries from the cache to prevent unbounded growth. - /// Uses time-based expiration (30 minutes) instead of count-based. + /// Uses count-based cleanup (MaxCacheSize = 1000) to prevent unbounded growth. /// This is called at the start of each OnPostprocessAllAssets batch. /// </summary>
323-348: Consider consolidating asset type detection.This
GetAssetTypemethod duplicatesDetectAssetTypeinEmitter.cs(lines 517-539) with different extension coverage. For example, this version includes.tga,.bmp,.blendwhile the emitter includes audio types (.wav,.mp3) and data files (.xml,.json).Consider extracting a shared helper to ensure consistent asset type classification across the codebase.
353-384: Missing sampling middleware check.The
RecordEventmethod inRecorder.csappliesSamplingMiddleware.ShouldRecord(evt)before recording, but this implementation bypasses it and relies solely onEventStore.Record. WhileEventStorehas its own filters, the sampling middleware is designed to reduce noise at the capture layer.Consider applying the sampling middleware for consistency with the Recorder pattern:
♻️ Suggested enhancement
+using MCPForUnity.Editor.ActionTrace.Capture.Sampling; ... var evt = new EditorEvent( sequence: 0, timestampUnixMs: DateTimeOffset.UtcNow.ToUnixTimeMilliseconds(), type: type, targetId: targetId, payload: payload ); + // Apply sampling middleware to maintain consistency with ActionTraceRecorder + if (!SamplingMiddleware.ShouldRecord(evt)) + { + return; + } + // AssetPostprocessor callbacks run on main thread but outside update loop. // Use delayCall to defer recording to main thread update, avoiding thread warnings. UnityEditor.EditorApplication.delayCall += () => EventStore.Record(evt);MCPForUnity/Editor/ActionTrace/Capture/Emitter.cs (1)
460-482: Clarify distinction between EmitAssetDeleted overloads.There are two
EmitAssetDeletedmethods:
EmitAssetDeleted(string assetPath)(line 291) - for Unity-detected deletionsEmitAssetDeleted(string assetPath, string assetType)(line 464) - for MCP tool deletions withsource: "mcp_tool"tagThe distinction is important for understanding event provenance, but the doc comments could be more explicit about when to use each.
📝 Suggested doc enhancement
/// <summary> /// Emit an asset deleted event via MCP tool (manage_asset). /// Uses Asset:{path} format for cross-session stable target IDs. + /// + /// Use this overload for deletions initiated by MCP tools. + /// For Unity-detected deletions, use EmitAssetDeleted(string assetPath) instead. /// </summary>MCPForUnity/Editor/ActionTrace/Core/Store/EventStore.cs (1)
288-308: Minor thread safety consideration for merge tracking reset.Lines 298-300 reset
_lastRecordedEventand_lastRecordedTimeoutside_queryLock, but these fields are read/written inside the lock inRecord. Since the threading model specifies main-thread-only writes, this is safe in practice, but for consistency with the documented threading model, consider resetting these inside the lock.♻️ Suggested adjustment
lock (_queryLock) { _events.Clear(); _contextMappings.Clear(); _sequenceCounter = 0; + _lastRecordedEvent = null; + _lastRecordedTime = 0; + _lastDehydratedCount = -1; } - // Reset merge tracking and pending notifications - _lastRecordedEvent = null; - _lastRecordedTime = 0; - _lastDehydratedCount = -1; lock (_pendingNotifications)MCPForUnity/Editor/ActionTrace/Capture/Filters/EventFilter.cs (2)
35-50: Silent failure on invalid regex patterns.The empty catch block (lines 44-47) silently swallows regex parsing errors. Users won't know their regex pattern is invalid. Consider logging a warning on first failure.
♻️ Suggested enhancement
+[NonSerialized] +private bool _regexErrorLogged; private Regex GetRegex() { if (_cachedRegex != null) return _cachedRegex; if (!string.IsNullOrEmpty(Pattern)) { try { _cachedRegex = new Regex(Pattern, RegexOptions.Compiled | RegexOptions.IgnoreCase); } - catch + catch (ArgumentException ex) { - // Invalid regex, return null + if (!_regexErrorLogged) + { + UnityEngine.Debug.LogWarning($"[EventFilter] Invalid regex pattern in rule '{Name}': {ex.Message}"); + _regexErrorLogged = true; + } } } return _cachedRegex; }
139-140: Incorrect RuleType for full filenames.
.DS_StoreandThumbs.dbare filenames, not extensions. UsingRuleType.ExtensionwithEndsWithtechnically works, but it's semantically incorrect and could match unintended files likesomething.DS_Store.Consider using
RuleType.Regexwith end-of-string anchors:♻️ Suggested fix
- new() { Name = ".DS_Store", Type = RuleType.Extension, Pattern = ".DS_Store", Action = FilterAction.Block, Priority = 90 }, - new() { Name = "Thumbs.db", Type = RuleType.Extension, Pattern = "Thumbs.db", Action = FilterAction.Block, Priority = 90 }, + new() { Name = ".DS_Store", Type = RuleType.Regex, Pattern = @"(^|[/\\])\.DS_Store$", Action = FilterAction.Block, Priority = 90 }, + new() { Name = "Thumbs.db", Type = RuleType.Regex, Pattern = @"(^|[/\\])Thumbs\.db$", Action = FilterAction.Block, Priority = 90 },MCPForUnity/Editor/ActionTrace/Core/Settings/ActionTraceSettings.cs (3)
79-96: Consider adding Range constraints to sampling intervals.The sampling interval fields (
HierarchySamplingMs,SelectionSamplingMs,PropertySamplingMs) lackRangeattributes. Users could set negative or excessively large values via the Inspector. Consider adding constraints similar to other settings.♻️ Suggested enhancement
+ [Range(100, 10000)] [Tooltip("HierarchyChanged event sampling interval (milliseconds).")] public int HierarchySamplingMs = 1000; + [Range(50, 5000)] [Tooltip("SelectionChanged event sampling interval (milliseconds).")] public int SelectionSamplingMs = 500; + [Range(50, 2000)] [Tooltip("PropertyModified event sampling interval (milliseconds).")] public int PropertySamplingMs = 200;
102-154: Consider a more specific settings asset path.The hardcoded
SettingsPath = "Assets/ActionTraceSettings.asset"places the settings file in the project root. This could conflict with user files and clutters the Assets folder.Consider using a more specific location like
Assets/Editor Default Resources/ActionTraceSettings.assetor a subdirectory.
217-227: Redundant save operations during creation.
CreateSettingscallsApplyPreset(line 221) which internally callsSave()(line 184), then immediately callsAssetDatabase.SaveAssets()again (line 223). This results in redundant save operations.♻️ Suggested optimization
private static ActionTraceSettings CreateSettings() { var settings = CreateInstance<ActionTraceSettings>(); - // Apply Standard preset by default - settings.ApplyPreset(ActionTracePreset.Standard); + // Apply Standard preset values without triggering save + var preset = ActionTracePreset.Standard; + settings.Filtering.MinImportanceForRecording = preset.MinImportance; + settings.Storage.MaxEvents = preset.MaxEvents; + settings.Storage.HotEventCount = preset.HotEventCount; + settings.Merging.EnableEventMerging = preset.EnableEventMerging; + settings.Merging.MergeWindowMs = preset.MergeWindowMs; + settings.Merging.TransactionWindowMs = preset.TransactionWindowMs; + settings._currentPresetName = preset.Name; + AssetDatabase.CreateAsset(settings, SettingsPath); AssetDatabase.SaveAssets(); AssetDatabase.Refresh(); McpLog.Info($"[ActionTraceSettings] Created new settings at {SettingsPath}"); return settings; }
| private static void ScheduleSave() | ||
| { | ||
| // P1 Fix: Check throttling - skip if too soon since last save | ||
| long now = DateTimeOffset.UtcNow.ToUnixTimeMilliseconds(); | ||
| if (now - _lastSaveTime < MinSaveIntervalMs) | ||
| { | ||
| // Too soon, just mark dirty and skip save | ||
| return; | ||
| } | ||
|
|
||
| // Use lock to prevent race conditions with _saveScheduled | ||
| lock (_queryLock) | ||
| { | ||
| // Only schedule if not already scheduled (prevents callback queue bloat) | ||
| if (_saveScheduled) | ||
| return; | ||
|
|
||
| _saveScheduled = true; | ||
| } | ||
|
|
||
| // Use delayCall to coalesce multiple saves into one | ||
| EditorApplication.delayCall += () => | ||
| { | ||
| bool wasDirty; | ||
| lock (_queryLock) | ||
| { | ||
| _saveScheduled = false; | ||
| wasDirty = _isDirty; | ||
| if (_isDirty) | ||
| { | ||
| _isDirty = false; | ||
| // P1 Fix: Update last save time when actually saving | ||
| _lastSaveTime = DateTimeOffset.UtcNow.ToUnixTimeMilliseconds(); | ||
| } | ||
| } | ||
|
|
||
| // Perform save outside lock to avoid holding lock during I/O | ||
| if (wasDirty) | ||
| { | ||
| SaveToStorage(); | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "EventStore.Persistence.cs" -type fRepository: CoplayDev/unity-mcp
Length of output: 182
🏁 Script executed:
find . -name "EventStore*.cs" -type f | head -20Repository: CoplayDev/unity-mcp
Length of output: 392
🏁 Script executed:
fd EventStore --extension csRepository: CoplayDev/unity-mcp
Length of output: 382
🏁 Script executed:
wc -l MCPForUnity/Editor/ActionTrace/Core/Store/EventStore.Persistence.csRepository: CoplayDev/unity-mcp
Length of output: 134
🏁 Script executed:
cat -n MCPForUnity/Editor/ActionTrace/Core/Store/EventStore.Persistence.csRepository: CoplayDev/unity-mcp
Length of output: 10014
🏁 Script executed:
wc -l MCPForUnity/Editor/ActionTrace/Core/Store/EventStore.csRepository: CoplayDev/unity-mcp
Length of output: 122
🏁 Script executed:
cat -n MCPForUnity/Editor/ActionTrace/Core/Store/EventStore.csRepository: CoplayDev/unity-mcp
Length of output: 15857
🏁 Script executed:
rg -n "EditorApplication\.(update|wantsToQuit|quitting|playModeStateChanged)" MCPForUnity/Editor/ActionTrace/Repository: CoplayDev/unity-mcp
Length of output: 2254
🏁 Script executed:
rg -n "domain.*reload|shutdown|quitting|SaveToStorage" MCPForUnity/Editor/ActionTrace/Core/Store/ -A2Repository: CoplayDev/unity-mcp
Length of output: 1930
🏁 Script executed:
rg -n "InitializeOnLoad|wantsToQuit|domainReload|OnDisable" MCPForUnity/Editor/ActionTrace/Core/Store/EventStore*.csRepository: CoplayDev/unity-mcp
Length of output: 45
🏁 Script executed:
grep -rn "EventStore\." MCPForUnity/Editor/ActionTrace/ | grep -i "save\|quitting\|reload" | head -20Repository: CoplayDev/unity-mcp
Length of output: 192
🏁 Script executed:
rg -n "InitializeOnLoad|[OnDisable|OnDestroy" MCPForUnity/ --max-count=10Repository: CoplayDev/unity-mcp
Length of output: 178
🏁 Script executed:
rg -n "McpJobStateStore" MCPForUnity/Editor/ActionTrace/Core/Store/ -B2 -A2Repository: CoplayDev/unity-mcp
Length of output: 1683
🏁 Script executed:
rg -n "InitializeOnLoad|wantsToQuit" MCPForUnity/Repository: CoplayDev/unity-mcp
Length of output: 1927
🏁 Script executed:
find MCPForUnity -name "*.cs" -exec grep -l "EventStore\|McpJobStateStore" {} \; | head -5Repository: CoplayDev/unity-mcp
Length of output: 1017
🏁 Script executed:
cat -n MCPForUnity/Editor/Services/McpEditorShutdownCleanup.csRepository: CoplayDev/unity-mcp
Length of output: 3728
🏁 Script executed:
wc -l MCPForUnity/Editor/Helpers/McpJobStateStore.csRepository: CoplayDev/unity-mcp
Length of output: 112
🏁 Script executed:
sed -n '1,50p' MCPForUnity/Editor/Helpers/McpJobStateStore.csRepository: CoplayDev/unity-mcp
Length of output: 1690
Schedule a follow-up save when throttled to avoid losing dirty state.
When ScheduleSave() is throttled, it returns without scheduling any follow-up. If no new events arrive after the throttle hit, the dirty flag remains unsaved indefinitely. This can lead to data loss if the editor closes before the throttle window expires and triggers a new save.
The proposed fix—scheduling a retry delayCall when throttled that repeatedly checks the interval until it passes—correctly addresses this by ensuring eventual persistence of the dirty state.
🔧 Proposed fix (retry until the interval passes)
- if (now - _lastSaveTime < MinSaveIntervalMs)
- {
- // Too soon, just mark dirty and skip save
- return;
- }
+ if (now - _lastSaveTime < MinSaveIntervalMs)
+ {
+ lock (_queryLock)
+ {
+ if (_saveScheduled) return;
+ _saveScheduled = true;
+ }
+
+ // Retry on the next editor tick until interval passes
+ EditorApplication.delayCall += () =>
+ {
+ lock (_queryLock)
+ {
+ _saveScheduled = false;
+ }
+ ScheduleSave();
+ };
+ return;
+ }📝 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 static void ScheduleSave() | |
| { | |
| // P1 Fix: Check throttling - skip if too soon since last save | |
| long now = DateTimeOffset.UtcNow.ToUnixTimeMilliseconds(); | |
| if (now - _lastSaveTime < MinSaveIntervalMs) | |
| { | |
| // Too soon, just mark dirty and skip save | |
| return; | |
| } | |
| // Use lock to prevent race conditions with _saveScheduled | |
| lock (_queryLock) | |
| { | |
| // Only schedule if not already scheduled (prevents callback queue bloat) | |
| if (_saveScheduled) | |
| return; | |
| _saveScheduled = true; | |
| } | |
| // Use delayCall to coalesce multiple saves into one | |
| EditorApplication.delayCall += () => | |
| { | |
| bool wasDirty; | |
| lock (_queryLock) | |
| { | |
| _saveScheduled = false; | |
| wasDirty = _isDirty; | |
| if (_isDirty) | |
| { | |
| _isDirty = false; | |
| // P1 Fix: Update last save time when actually saving | |
| _lastSaveTime = DateTimeOffset.UtcNow.ToUnixTimeMilliseconds(); | |
| } | |
| } | |
| // Perform save outside lock to avoid holding lock during I/O | |
| if (wasDirty) | |
| { | |
| SaveToStorage(); | |
| } | |
| }; | |
| } | |
| private static void ScheduleSave() | |
| { | |
| // P1 Fix: Check throttling - skip if too soon since last save | |
| long now = DateTimeOffset.UtcNow.ToUnixTimeMilliseconds(); | |
| if (now - _lastSaveTime < MinSaveIntervalMs) | |
| { | |
| lock (_queryLock) | |
| { | |
| if (_saveScheduled) return; | |
| _saveScheduled = true; | |
| } | |
| // Retry on the next editor tick until interval passes | |
| EditorApplication.delayCall += () => | |
| { | |
| lock (_queryLock) | |
| { | |
| _saveScheduled = false; | |
| } | |
| ScheduleSave(); | |
| }; | |
| return; | |
| } | |
| // Use lock to prevent race conditions with _saveScheduled | |
| lock (_queryLock) | |
| { | |
| // Only schedule if not already scheduled (prevents callback queue bloat) | |
| if (_saveScheduled) | |
| return; | |
| _saveScheduled = true; | |
| } | |
| // Use delayCall to coalesce multiple saves into one | |
| EditorApplication.delayCall += () => | |
| { | |
| bool wasDirty; | |
| lock (_queryLock) | |
| { | |
| _saveScheduled = false; | |
| wasDirty = _isDirty; | |
| if (_isDirty) | |
| { | |
| _isDirty = false; | |
| // P1 Fix: Update last save time when actually saving | |
| _lastSaveTime = DateTimeOffset.UtcNow.ToUnixTimeMilliseconds(); | |
| } | |
| } | |
| // Perform save outside lock to avoid holding lock during I/O | |
| if (wasDirty) | |
| { | |
| SaveToStorage(); | |
| } | |
| }; | |
| } |
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/ActionTrace/Core/Store/EventStore.Persistence.cs` around
lines 45 - 87, ScheduleSave currently returns immediately when throttled (now -
_lastSaveTime < MinSaveIntervalMs) which can leave _isDirty unsaved; modify
ScheduleSave so that when it skips due to throttling it schedules a retry via
EditorApplication.delayCall to re-call ScheduleSave after the remaining throttle
interval (or a small capped retry delay) instead of just returning, while still
respecting the _queryLock/_saveScheduled guards and avoiding duplicate retries;
reference ScheduleSave, _lastSaveTime, MinSaveIntervalMs, _saveScheduled,
_queryLock, _isDirty, EditorApplication.delayCall and SaveToStorage to locate
where to add the retry scheduling logic.
| private static void SaveToStorage() | ||
| { | ||
| try | ||
| { | ||
| var state = new EventStoreState | ||
| { | ||
| SchemaVersion = CurrentSchemaVersion, | ||
| SequenceCounter = _sequenceCounter, | ||
| Events = _events.ToList(), | ||
| ContextMappings = _contextMappings.ToList() | ||
| }; | ||
| McpJobStateStore.SaveState(StateKey, state); | ||
| } |
There was a problem hiding this comment.
Serialize a snapshot under lock to avoid concurrent mutation errors.
SaveToStorage copies _events/_contextMappings without locking, so concurrent writers can throw or create inconsistent snapshots. Capture under _queryLock, then serialize outside the lock.
🔧 Proposed fix (snapshot under lock)
- var state = new EventStoreState
- {
- SchemaVersion = CurrentSchemaVersion,
- SequenceCounter = _sequenceCounter,
- Events = _events.ToList(),
- ContextMappings = _contextMappings.ToList()
- };
+ List<EditorEvent> eventsSnapshot;
+ List<ContextMapping> contextSnapshot;
+ long sequenceCounter;
+ lock (_queryLock)
+ {
+ sequenceCounter = _sequenceCounter;
+ eventsSnapshot = _events.ToList();
+ contextSnapshot = _contextMappings.ToList();
+ }
+
+ var state = new EventStoreState
+ {
+ SchemaVersion = CurrentSchemaVersion,
+ SequenceCounter = sequenceCounter,
+ Events = eventsSnapshot,
+ ContextMappings = contextSnapshot
+ };📝 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 static void SaveToStorage() | |
| { | |
| try | |
| { | |
| var state = new EventStoreState | |
| { | |
| SchemaVersion = CurrentSchemaVersion, | |
| SequenceCounter = _sequenceCounter, | |
| Events = _events.ToList(), | |
| ContextMappings = _contextMappings.ToList() | |
| }; | |
| McpJobStateStore.SaveState(StateKey, state); | |
| } | |
| private static void SaveToStorage() | |
| { | |
| try | |
| { | |
| List<EditorEvent> eventsSnapshot; | |
| List<ContextMapping> contextSnapshot; | |
| long sequenceCounter; | |
| lock (_queryLock) | |
| { | |
| sequenceCounter = _sequenceCounter; | |
| eventsSnapshot = _events.ToList(); | |
| contextSnapshot = _contextMappings.ToList(); | |
| } | |
| var state = new EventStoreState | |
| { | |
| SchemaVersion = CurrentSchemaVersion, | |
| SequenceCounter = sequenceCounter, | |
| Events = eventsSnapshot, | |
| ContextMappings = contextSnapshot | |
| }; | |
| McpJobStateStore.SaveState(StateKey, state); | |
| } |
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/ActionTrace/Core/Store/EventStore.Persistence.cs` around
lines 201 - 213, SaveToStorage currently copies _events and _contextMappings
without synchronization, risking concurrent mutation; take a snapshot of the
mutable state under the existing _queryLock and then perform serialization and
McpJobStateStore.SaveState outside the lock. Specifically, inside SaveToStorage
acquire _queryLock, copy _events and _contextMappings into local lists and
capture SequenceCounter/CurrentSchemaVersion into a new EventStoreState local
variable, release the lock, then call McpJobStateStore.SaveState(StateKey,
state) with that snapshot so SaveToStorage no longer iterates shared
_events/_contextMappings while unlocked.
| /// <summary> | ||
| /// P1.2: Apply task_id and conversation_id filters to raw event list. | ||
| /// Filters AINote events by matching task_id and conversation_id in payload. | ||
| /// </summary> | ||
| private static List<EditorEvent> ApplyTaskFilters(List<EditorEvent> events, string taskId, string conversationId) | ||
| { | ||
| // If no filters specified, return original list | ||
| if (string.IsNullOrEmpty(taskId) && string.IsNullOrEmpty(conversationId)) | ||
| return events; | ||
|
|
||
| return events.Where(e => | ||
| { | ||
| // Only AINote events have task_id and conversation_id | ||
| if (e.Type != EventTypes.AINote) | ||
| return true; | ||
|
|
||
| // Guard against dehydrated events (null payload) | ||
| if (e.Payload == null) | ||
| return false; // Can't match filters without payload | ||
|
|
||
| // Check task_id filter | ||
| if (!string.IsNullOrEmpty(taskId)) | ||
| { | ||
| if (e.Payload.TryGetValue("task_id", out var taskVal)) | ||
| { | ||
| string eventTaskId = taskVal?.ToString(); | ||
| if (eventTaskId != taskId) | ||
| return false; | ||
| } | ||
| else | ||
| { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| // Check conversation_id filter | ||
| if (!string.IsNullOrEmpty(conversationId)) | ||
| { | ||
| if (e.Payload.TryGetValue("conversation_id", out var convVal)) | ||
| { | ||
| string eventConvId = convVal?.ToString(); | ||
| if (eventConvId != conversationId) | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| return true; | ||
| }).ToList(); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// P1.2: Apply task filters to projected events (with semantics). | ||
| /// </summary> | ||
| private static List<ActionTraceViewItem> ApplyTaskFiltersToProjected(List<ActionTraceViewItem> projected, string taskId, string conversationId) | ||
| { | ||
| if (string.IsNullOrEmpty(taskId) && string.IsNullOrEmpty(conversationId)) | ||
| return projected; | ||
|
|
||
| return projected.Where(p => | ||
| { | ||
| if (p.Event.Type != EventTypes.AINote) | ||
| return true; | ||
|
|
||
| // Guard against dehydrated events (null payload) | ||
| if (p.Event.Payload == null) | ||
| return false; // Can't match filters without payload | ||
|
|
||
| if (!string.IsNullOrEmpty(taskId)) | ||
| { | ||
| if (p.Event.Payload.TryGetValue("task_id", out var taskVal)) | ||
| { | ||
| if (taskVal?.ToString() != taskId) | ||
| return false; | ||
| } | ||
| else | ||
| { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| if (!string.IsNullOrEmpty(conversationId)) | ||
| { | ||
| if (p.Event.Payload.TryGetValue("conversation_id", out var convVal)) | ||
| { | ||
| if (convVal?.ToString() != conversationId) | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| return true; | ||
| }).ToList(); | ||
| } |
There was a problem hiding this comment.
conversation_id filtering should require a match.
With a conversation filter set, AINote events lacking conversation_id currently pass through, which defeats the filter’s intent.
🛠️ Proposed fix
if (!string.IsNullOrEmpty(conversationId))
{
if (e.Payload.TryGetValue("conversation_id", out var convVal))
{
string eventConvId = convVal?.ToString();
if (eventConvId != conversationId)
return false;
}
+ else
+ {
+ return false;
+ }
} if (!string.IsNullOrEmpty(conversationId))
{
if (p.Event.Payload.TryGetValue("conversation_id", out var convVal))
{
if (convVal?.ToString() != conversationId)
return false;
}
+ else
+ {
+ return false;
+ }
}📝 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.
| /// <summary> | |
| /// P1.2: Apply task_id and conversation_id filters to raw event list. | |
| /// Filters AINote events by matching task_id and conversation_id in payload. | |
| /// </summary> | |
| private static List<EditorEvent> ApplyTaskFilters(List<EditorEvent> events, string taskId, string conversationId) | |
| { | |
| // If no filters specified, return original list | |
| if (string.IsNullOrEmpty(taskId) && string.IsNullOrEmpty(conversationId)) | |
| return events; | |
| return events.Where(e => | |
| { | |
| // Only AINote events have task_id and conversation_id | |
| if (e.Type != EventTypes.AINote) | |
| return true; | |
| // Guard against dehydrated events (null payload) | |
| if (e.Payload == null) | |
| return false; // Can't match filters without payload | |
| // Check task_id filter | |
| if (!string.IsNullOrEmpty(taskId)) | |
| { | |
| if (e.Payload.TryGetValue("task_id", out var taskVal)) | |
| { | |
| string eventTaskId = taskVal?.ToString(); | |
| if (eventTaskId != taskId) | |
| return false; | |
| } | |
| else | |
| { | |
| return false; | |
| } | |
| } | |
| // Check conversation_id filter | |
| if (!string.IsNullOrEmpty(conversationId)) | |
| { | |
| if (e.Payload.TryGetValue("conversation_id", out var convVal)) | |
| { | |
| string eventConvId = convVal?.ToString(); | |
| if (eventConvId != conversationId) | |
| return false; | |
| } | |
| } | |
| return true; | |
| }).ToList(); | |
| } | |
| /// <summary> | |
| /// P1.2: Apply task filters to projected events (with semantics). | |
| /// </summary> | |
| private static List<ActionTraceViewItem> ApplyTaskFiltersToProjected(List<ActionTraceViewItem> projected, string taskId, string conversationId) | |
| { | |
| if (string.IsNullOrEmpty(taskId) && string.IsNullOrEmpty(conversationId)) | |
| return projected; | |
| return projected.Where(p => | |
| { | |
| if (p.Event.Type != EventTypes.AINote) | |
| return true; | |
| // Guard against dehydrated events (null payload) | |
| if (p.Event.Payload == null) | |
| return false; // Can't match filters without payload | |
| if (!string.IsNullOrEmpty(taskId)) | |
| { | |
| if (p.Event.Payload.TryGetValue("task_id", out var taskVal)) | |
| { | |
| if (taskVal?.ToString() != taskId) | |
| return false; | |
| } | |
| else | |
| { | |
| return false; | |
| } | |
| } | |
| if (!string.IsNullOrEmpty(conversationId)) | |
| { | |
| if (p.Event.Payload.TryGetValue("conversation_id", out var convVal)) | |
| { | |
| if (convVal?.ToString() != conversationId) | |
| return false; | |
| } | |
| } | |
| return true; | |
| }).ToList(); | |
| } | |
| /// <summary> | |
| /// P1.2: Apply task_id and conversation_id filters to raw event list. | |
| /// Filters AINote events by matching task_id and conversation_id in payload. | |
| /// </summary> | |
| private static List<EditorEvent> ApplyTaskFilters(List<EditorEvent> events, string taskId, string conversationId) | |
| { | |
| // If no filters specified, return original list | |
| if (string.IsNullOrEmpty(taskId) && string.IsNullOrEmpty(conversationId)) | |
| return events; | |
| return events.Where(e => | |
| { | |
| // Only AINote events have task_id and conversation_id | |
| if (e.Type != EventTypes.AINote) | |
| return true; | |
| // Guard against dehydrated events (null payload) | |
| if (e.Payload == null) | |
| return false; // Can't match filters without payload | |
| // Check task_id filter | |
| if (!string.IsNullOrEmpty(taskId)) | |
| { | |
| if (e.Payload.TryGetValue("task_id", out var taskVal)) | |
| { | |
| string eventTaskId = taskVal?.ToString(); | |
| if (eventTaskId != taskId) | |
| return false; | |
| } | |
| else | |
| { | |
| return false; | |
| } | |
| } | |
| // Check conversation_id filter | |
| if (!string.IsNullOrEmpty(conversationId)) | |
| { | |
| if (e.Payload.TryGetValue("conversation_id", out var convVal)) | |
| { | |
| string eventConvId = convVal?.ToString(); | |
| if (eventConvId != conversationId) | |
| return false; | |
| } | |
| else | |
| { | |
| return false; | |
| } | |
| } | |
| return true; | |
| }).ToList(); | |
| } | |
| /// <summary> | |
| /// P1.2: Apply task filters to projected events (with semantics). | |
| /// </summary> | |
| private static List<ActionTraceViewItem> ApplyTaskFiltersToProjected(List<ActionTraceViewItem> projected, string taskId, string conversationId) | |
| { | |
| if (string.IsNullOrEmpty(taskId) && string.IsNullOrEmpty(conversationId)) | |
| return projected; | |
| return projected.Where(p => | |
| { | |
| if (p.Event.Type != EventTypes.AINote) | |
| return true; | |
| // Guard against dehydrated events (null payload) | |
| if (p.Event.Payload == null) | |
| return false; // Can't match filters without payload | |
| if (!string.IsNullOrEmpty(taskId)) | |
| { | |
| if (p.Event.Payload.TryGetValue("task_id", out var taskVal)) | |
| { | |
| if (taskVal?.ToString() != taskId) | |
| return false; | |
| } | |
| else | |
| { | |
| return false; | |
| } | |
| } | |
| if (!string.IsNullOrEmpty(conversationId)) | |
| { | |
| if (p.Event.Payload.TryGetValue("conversation_id", out var convVal)) | |
| { | |
| if (convVal?.ToString() != conversationId) | |
| return false; | |
| } | |
| else | |
| { | |
| return false; | |
| } | |
| } | |
| return true; | |
| }).ToList(); | |
| } |
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/Resources/ActionTrace/ActionTraceViewResource.cs` around
lines 349 - 440, The conversation_id filter currently allows AINote events
without a conversation_id to pass; update both ApplyTaskFilters and
ApplyTaskFiltersToProjected so that when conversationId is non-empty you require
the payload to contain "conversation_id" and that its ToString() equals
conversationId—i.e., if TryGetValue("conversation_id", out var convVal) returns
false, return false (just like the task_id branch), and otherwise compare
convVal?.ToString() != conversationId to reject non-matching values.
| // Check for errors | ||
| if (importanceCategory == "critical" || evtTypeLower.Contains("error") || evtTypeLower.Contains("exception")) | ||
| { | ||
| errorCount++; | ||
| errorEvents.Add(new | ||
| { | ||
| sequence = evt.Sequence, | ||
| type = evt.Type, | ||
| target = targetName, | ||
| summary = evt.GetSummary() | ||
| }); | ||
| } |
There was a problem hiding this comment.
Avoid classifying AINote as an error.
AINote is scored as “critical”, so the current logic flags notes as errors. That inflates error counts and warnings.
🛠️ Proposed fix
- if (importanceCategory == "critical" || evtTypeLower.Contains("error") || evtTypeLower.Contains("exception"))
- {
+ bool isError = evtTypeLower.Contains("error") || evtTypeLower.Contains("exception");
+ if (importanceCategory == "critical" && !string.Equals(evt.Type, "AINote", StringComparison.Ordinal))
+ isError = true;
+ if (isError)
+ {📝 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.
| // Check for errors | |
| if (importanceCategory == "critical" || evtTypeLower.Contains("error") || evtTypeLower.Contains("exception")) | |
| { | |
| errorCount++; | |
| errorEvents.Add(new | |
| { | |
| sequence = evt.Sequence, | |
| type = evt.Type, | |
| target = targetName, | |
| summary = evt.GetSummary() | |
| }); | |
| } | |
| // Check for errors | |
| bool isError = evtTypeLower.Contains("error") || evtTypeLower.Contains("exception"); | |
| if (importanceCategory == "critical" && !string.Equals(evt.Type, "AINote", StringComparison.Ordinal)) | |
| isError = true; | |
| if (isError) | |
| { | |
| errorCount++; | |
| errorEvents.Add(new | |
| { | |
| sequence = evt.Sequence, | |
| type = evt.Type, | |
| target = targetName, | |
| summary = evt.GetSummary() | |
| }); | |
| } |
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/Tools/GetActionTraceSummaryTool.cs` around lines 185 -
196, The logic treats any event with importanceCategory == "critical" (or
containing "error"/"exception" in evtTypeLower) as an error, but AINote events
are scored critical and should not be counted; update the conditional in the
error counting block (the if that currently uses importanceCategory and
evtTypeLower) to exclude events whose evt.Type equals "AINote"
(case-insensitive) — e.g., only increment errorCount and add to errorEvents when
the existing conditions are true AND evt.Type does not equal "AINote" (use
StringComparison.OrdinalIgnoreCase for the comparison).
| AssetDatabase.SaveAssets(); | ||
|
|
||
| // === ActionTrace Integration: Notify subscribers (low-coupling) === | ||
| OnAssetCreated?.Invoke(fullPath, assetType); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "ManageAsset.cs" -type fRepository: CoplayDev/unity-mcp
Length of output: 104
🏁 Script executed:
wc -l ./MCPForUnity/Editor/Tools/ManageAsset.csRepository: CoplayDev/unity-mcp
Length of output: 109
🏁 Script executed:
cat -n ./MCPForUnity/Editor/Tools/ManageAsset.cs | sed -n '285,300p'Repository: CoplayDev/unity-mcp
Length of output: 824
🏁 Script executed:
cat -n ./MCPForUnity/Editor/Tools/ManageAsset.cs | sed -n '493,510p'Repository: CoplayDev/unity-mcp
Length of output: 1101
🏁 Script executed:
cat -n ./MCPForUnity/Editor/Tools/ManageAsset.cs | sed -n '542,555p'Repository: CoplayDev/unity-mcp
Length of output: 786
🏁 Script executed:
cat -n ./MCPForUnity/Editor/Tools/ManageAsset.cs | sed -n '245,300p'Repository: CoplayDev/unity-mcp
Length of output: 3491
🏁 Script executed:
cat -n ./MCPForUnity/Editor/Tools/ManageAsset.cs | sed -n '200,310p'Repository: CoplayDev/unity-mcp
Length of output: 6407
🏁 Script executed:
cat -n ./MCPForUnity/Editor/Tools/ManageAsset.cs | sed -n '430,515p'Repository: CoplayDev/unity-mcp
Length of output: 5736
🏁 Script executed:
cat -n ./MCPForUnity/Editor/Tools/ManageAsset.cs | sed -n '350,380p'Repository: CoplayDev/unity-mcp
Length of output: 1560
🏁 Script executed:
cat -n ./MCPForUnity/Editor/Tools/ManageAsset.cs | sed -n '530,570p'Repository: CoplayDev/unity-mcp
Length of output: 2117
🏁 Script executed:
cat -n ./MCPForUnity/Editor/Tools/ManageAsset.cs | sed -n '515,530p'Repository: CoplayDev/unity-mcp
Length of output: 1003
Isolate subscriber exceptions from core asset operations.
All three callback invocations (OnAssetCreated, OnAssetModified, OnAssetDeleted) lack exception isolation. If a subscriber throws, the exception is caught by the outer method try-catch and the methods return ErrorResponse even though the asset operation (create, modify, delete) and SaveAssets/DeleteAsset already succeeded. This causes callers to incorrectly believe the asset operation failed.
Wrap each callback invocation in its own try-catch block to prevent external handlers from breaking the return contract:
🛡️ Suggested pattern (apply to each callback)
- OnAssetCreated?.Invoke(fullPath, assetType);
+ try
+ {
+ OnAssetCreated?.Invoke(fullPath, assetType);
+ }
+ catch (Exception ex)
+ {
+ McpLog.Warn($"[ManageAsset] OnAssetCreated subscriber failed: {ex.Message}");
+ }Also applies to: 499-503 (OnAssetModified), 548 (OnAssetDeleted)
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/Tools/ManageAsset.cs` around lines 290 - 294, The three
callback invocations (OnAssetCreated, OnAssetModified, OnAssetDeleted) should be
wrapped in their own try-catch blocks so subscriber exceptions cannot propagate
and cause the outer method to return ErrorResponse even after the asset
operation and AssetDatabase.SaveAssets()/DeleteAsset succeeded; modify the
invocation sites (the lines calling OnAssetCreated?.Invoke(fullPath, assetType),
OnAssetModified?.Invoke(fullPath, assetType), and
OnAssetDeleted?.Invoke(fullPath)) to catch and log any exception locally (e.g.,
processLogger or UnityEngine.Debug.LogException) and continue normal return
flow, preserving the successful response from the core create/modify/delete
methods.
| private string BuildJsonExport() | ||
| { | ||
| _stringBuilder.Clear(); | ||
| _stringBuilder.AppendLine("{"); | ||
| _stringBuilder.AppendLine($" \"exportTime\": \"{DateTime.Now:O}\","); | ||
| _stringBuilder.AppendLine($" \"totalEvents\": {_currentEvents.Count},"); | ||
| _stringBuilder.AppendLine(" \"events\": ["); | ||
|
|
||
| for (int i = 0; i < _currentEvents.Count; i++) | ||
| { | ||
| var e = _currentEvents[i]; | ||
| _stringBuilder.AppendLine(" {"); | ||
| _stringBuilder.AppendLine($" \"sequence\": {e.Event.Sequence},"); | ||
| _stringBuilder.AppendLine($" \"type\": \"{SanitizeJson(e.Event.Type)}\","); | ||
| _stringBuilder.AppendLine($" \"timestamp\": {e.Event.TimestampUnixMs},"); | ||
| _stringBuilder.AppendLine($" \"displayTime\": \"{SanitizeJson(e.DisplayTime)}\","); | ||
| _stringBuilder.AppendLine($" \"summary\": \"{SanitizeJson(e.DisplaySummary)}\","); | ||
|
|
||
| if (!string.IsNullOrEmpty(e.Event.TargetId)) | ||
| { | ||
| _stringBuilder.AppendLine($" \"targetId\": \"{SanitizeJson(e.Event.TargetId)}\","); | ||
| _stringBuilder.AppendLine($" \"targetName\": \"{SanitizeJson(e.TargetName)}\","); | ||
| _stringBuilder.AppendLine($" \"targetInstanceId\": {(e.TargetInstanceId.HasValue ? e.TargetInstanceId.Value.ToString() : "null")},"); | ||
| } | ||
| else | ||
| { | ||
| _stringBuilder.AppendLine($" \"targetId\": null,"); | ||
| _stringBuilder.AppendLine($" \"targetName\": null,"); | ||
| _stringBuilder.AppendLine($" \"targetInstanceId\": null,"); | ||
| } | ||
|
|
||
| _stringBuilder.AppendLine($" \"importanceScore\": {e.ImportanceScore:F2},"); | ||
| _stringBuilder.AppendLine($" \"importanceCategory\": \"{SanitizeJson(e.ImportanceCategory)}\""); | ||
|
|
||
| if (!string.IsNullOrEmpty(e.InferredIntent)) | ||
| _stringBuilder.AppendLine($" ,\"inferredIntent\": \"{SanitizeJson(e.InferredIntent)}\""); | ||
|
|
||
| if (e.Event.Payload != null && e.Event.Payload.Count > 0) | ||
| { | ||
| _stringBuilder.AppendLine(" ,\"payload\": {"); | ||
| var payloadKeys = e.Event.Payload.Keys.ToList(); | ||
| for (int j = 0; j < payloadKeys.Count; j++) | ||
| { | ||
| var key = payloadKeys[j]; | ||
| var value = e.Event.Payload[key]; | ||
| var valueStr = value?.ToString() ?? "null"; | ||
| _stringBuilder.AppendLine($" \"{SanitizeJson(key)}\": \"{SanitizeJson(valueStr)}\"{(j < payloadKeys.Count - 1 ? "," : "")}"); | ||
| } | ||
| _stringBuilder.AppendLine(" }"); | ||
| } | ||
|
|
||
| _stringBuilder.Append(i < _currentEvents.Count - 1 ? " }," : " }"); | ||
| if (i < _currentEvents.Count - 1) | ||
| _stringBuilder.AppendLine(); | ||
| } | ||
|
|
||
| _stringBuilder.AppendLine(); | ||
| _stringBuilder.AppendLine(" ]"); | ||
| _stringBuilder.AppendLine("}"); | ||
|
|
||
| return _stringBuilder.ToString(); | ||
| } | ||
|
|
||
| private string BuildCsvExport() | ||
| { | ||
| _stringBuilder.Clear(); | ||
| _stringBuilder.AppendLine("Sequence,Time,Type,Summary,Target Name,InstanceID,Importance,Category,Intent"); | ||
|
|
||
| foreach (var e in _currentEvents) | ||
| { | ||
| _stringBuilder.Append(e.Event.Sequence); | ||
| _stringBuilder.Append(",\""); | ||
| _stringBuilder.Append(SanitizeCsv(e.DisplayTime)); | ||
| _stringBuilder.Append("\",\""); | ||
| _stringBuilder.Append(SanitizeCsv(e.Event.Type)); | ||
| _stringBuilder.Append("\",\""); | ||
| _stringBuilder.Append(SanitizeCsv(e.DisplaySummary)); | ||
| _stringBuilder.Append("\",\""); | ||
|
|
||
| // Get instance info for export | ||
| if (!string.IsNullOrEmpty(e.TargetName)) | ||
| { | ||
| _stringBuilder.Append(SanitizeCsv(e.TargetName)); | ||
| _stringBuilder.Append("\",\""); | ||
| _stringBuilder.Append(SanitizeCsv(e.TargetInstanceId?.ToString() ?? "")); | ||
| } | ||
| else | ||
| { | ||
| _stringBuilder.Append("\",\""); | ||
| } | ||
|
|
||
| _stringBuilder.Append("\","); | ||
| _stringBuilder.Append(e.ImportanceScore.ToString("F2")); | ||
| _stringBuilder.Append(",\""); | ||
| _stringBuilder.Append(SanitizeCsv(e.ImportanceCategory)); | ||
| _stringBuilder.Append("\",\""); | ||
| _stringBuilder.Append(SanitizeCsv(e.InferredIntent ?? "")); | ||
| _stringBuilder.AppendLine("\""); | ||
| } | ||
|
|
||
| return _stringBuilder.ToString(); | ||
| } | ||
|
|
||
| private string SanitizeJson(string input) | ||
| { | ||
| if (string.IsNullOrEmpty(input)) return ""; | ||
|
|
||
| // Performance optimization: use StringBuilder to avoid intermediate strings from multiple Replace calls | ||
| var sb = _stringBuilder; | ||
| sb.Clear(); | ||
| sb.EnsureCapacity(input.Length * 2); | ||
|
|
||
| foreach (char c in input) | ||
| { | ||
| switch (c) | ||
| { | ||
| case '\\': sb.Append("\\\\"); break; | ||
| case '\"': sb.Append("\\\""); break; | ||
| case '\n': sb.Append("\\n"); break; | ||
| case '\r': sb.Append("\\r"); break; | ||
| case '\t': sb.Append("\\t"); break; | ||
| default: sb.Append(c); break; | ||
| } | ||
| } | ||
|
|
||
| return sb.ToString(); | ||
| } |
There was a problem hiding this comment.
SanitizeJson clears the export builder, corrupting JSON output.
BuildJsonExport builds JSON with _stringBuilder, but SanitizeJson clears that same builder, wiping the partially built payload.
🔧 Proposed fix (use a local builder in SanitizeJson)
- var sb = _stringBuilder;
- sb.Clear();
- sb.EnsureCapacity(input.Length * 2);
+ var sb = new StringBuilder(input.Length * 2);📝 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 string BuildJsonExport() | |
| { | |
| _stringBuilder.Clear(); | |
| _stringBuilder.AppendLine("{"); | |
| _stringBuilder.AppendLine($" \"exportTime\": \"{DateTime.Now:O}\","); | |
| _stringBuilder.AppendLine($" \"totalEvents\": {_currentEvents.Count},"); | |
| _stringBuilder.AppendLine(" \"events\": ["); | |
| for (int i = 0; i < _currentEvents.Count; i++) | |
| { | |
| var e = _currentEvents[i]; | |
| _stringBuilder.AppendLine(" {"); | |
| _stringBuilder.AppendLine($" \"sequence\": {e.Event.Sequence},"); | |
| _stringBuilder.AppendLine($" \"type\": \"{SanitizeJson(e.Event.Type)}\","); | |
| _stringBuilder.AppendLine($" \"timestamp\": {e.Event.TimestampUnixMs},"); | |
| _stringBuilder.AppendLine($" \"displayTime\": \"{SanitizeJson(e.DisplayTime)}\","); | |
| _stringBuilder.AppendLine($" \"summary\": \"{SanitizeJson(e.DisplaySummary)}\","); | |
| if (!string.IsNullOrEmpty(e.Event.TargetId)) | |
| { | |
| _stringBuilder.AppendLine($" \"targetId\": \"{SanitizeJson(e.Event.TargetId)}\","); | |
| _stringBuilder.AppendLine($" \"targetName\": \"{SanitizeJson(e.TargetName)}\","); | |
| _stringBuilder.AppendLine($" \"targetInstanceId\": {(e.TargetInstanceId.HasValue ? e.TargetInstanceId.Value.ToString() : "null")},"); | |
| } | |
| else | |
| { | |
| _stringBuilder.AppendLine($" \"targetId\": null,"); | |
| _stringBuilder.AppendLine($" \"targetName\": null,"); | |
| _stringBuilder.AppendLine($" \"targetInstanceId\": null,"); | |
| } | |
| _stringBuilder.AppendLine($" \"importanceScore\": {e.ImportanceScore:F2},"); | |
| _stringBuilder.AppendLine($" \"importanceCategory\": \"{SanitizeJson(e.ImportanceCategory)}\""); | |
| if (!string.IsNullOrEmpty(e.InferredIntent)) | |
| _stringBuilder.AppendLine($" ,\"inferredIntent\": \"{SanitizeJson(e.InferredIntent)}\""); | |
| if (e.Event.Payload != null && e.Event.Payload.Count > 0) | |
| { | |
| _stringBuilder.AppendLine(" ,\"payload\": {"); | |
| var payloadKeys = e.Event.Payload.Keys.ToList(); | |
| for (int j = 0; j < payloadKeys.Count; j++) | |
| { | |
| var key = payloadKeys[j]; | |
| var value = e.Event.Payload[key]; | |
| var valueStr = value?.ToString() ?? "null"; | |
| _stringBuilder.AppendLine($" \"{SanitizeJson(key)}\": \"{SanitizeJson(valueStr)}\"{(j < payloadKeys.Count - 1 ? "," : "")}"); | |
| } | |
| _stringBuilder.AppendLine(" }"); | |
| } | |
| _stringBuilder.Append(i < _currentEvents.Count - 1 ? " }," : " }"); | |
| if (i < _currentEvents.Count - 1) | |
| _stringBuilder.AppendLine(); | |
| } | |
| _stringBuilder.AppendLine(); | |
| _stringBuilder.AppendLine(" ]"); | |
| _stringBuilder.AppendLine("}"); | |
| return _stringBuilder.ToString(); | |
| } | |
| private string BuildCsvExport() | |
| { | |
| _stringBuilder.Clear(); | |
| _stringBuilder.AppendLine("Sequence,Time,Type,Summary,Target Name,InstanceID,Importance,Category,Intent"); | |
| foreach (var e in _currentEvents) | |
| { | |
| _stringBuilder.Append(e.Event.Sequence); | |
| _stringBuilder.Append(",\""); | |
| _stringBuilder.Append(SanitizeCsv(e.DisplayTime)); | |
| _stringBuilder.Append("\",\""); | |
| _stringBuilder.Append(SanitizeCsv(e.Event.Type)); | |
| _stringBuilder.Append("\",\""); | |
| _stringBuilder.Append(SanitizeCsv(e.DisplaySummary)); | |
| _stringBuilder.Append("\",\""); | |
| // Get instance info for export | |
| if (!string.IsNullOrEmpty(e.TargetName)) | |
| { | |
| _stringBuilder.Append(SanitizeCsv(e.TargetName)); | |
| _stringBuilder.Append("\",\""); | |
| _stringBuilder.Append(SanitizeCsv(e.TargetInstanceId?.ToString() ?? "")); | |
| } | |
| else | |
| { | |
| _stringBuilder.Append("\",\""); | |
| } | |
| _stringBuilder.Append("\","); | |
| _stringBuilder.Append(e.ImportanceScore.ToString("F2")); | |
| _stringBuilder.Append(",\""); | |
| _stringBuilder.Append(SanitizeCsv(e.ImportanceCategory)); | |
| _stringBuilder.Append("\",\""); | |
| _stringBuilder.Append(SanitizeCsv(e.InferredIntent ?? "")); | |
| _stringBuilder.AppendLine("\""); | |
| } | |
| return _stringBuilder.ToString(); | |
| } | |
| private string SanitizeJson(string input) | |
| { | |
| if (string.IsNullOrEmpty(input)) return ""; | |
| // Performance optimization: use StringBuilder to avoid intermediate strings from multiple Replace calls | |
| var sb = _stringBuilder; | |
| sb.Clear(); | |
| sb.EnsureCapacity(input.Length * 2); | |
| foreach (char c in input) | |
| { | |
| switch (c) | |
| { | |
| case '\\': sb.Append("\\\\"); break; | |
| case '\"': sb.Append("\\\""); break; | |
| case '\n': sb.Append("\\n"); break; | |
| case '\r': sb.Append("\\r"); break; | |
| case '\t': sb.Append("\\t"); break; | |
| default: sb.Append(c); break; | |
| } | |
| } | |
| return sb.ToString(); | |
| } | |
| private string SanitizeJson(string input) | |
| { | |
| if (string.IsNullOrEmpty(input)) return ""; | |
| // Performance optimization: use StringBuilder to avoid intermediate strings from multiple Replace calls | |
| var sb = new StringBuilder(input.Length * 2); | |
| foreach (char c in input) | |
| { | |
| switch (c) | |
| { | |
| case '\\': sb.Append("\\\\"); break; | |
| case '\"': sb.Append("\\\""); break; | |
| case '\n': sb.Append("\\n"); break; | |
| case '\r': sb.Append("\\r"); break; | |
| case '\t': sb.Append("\\t"); break; | |
| default: sb.Append(c); break; | |
| } | |
| } | |
| return sb.ToString(); | |
| } |
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/Windows/ActionTraceEditorWindow.cs` around lines 682 -
808, SanitizeJson currently reuses and clears the shared field _stringBuilder
(used by BuildJsonExport), which corrupts the in-progress export; modify
SanitizeJson to allocate and use a local StringBuilder (e.g., new StringBuilder
with capacity based on input.Length) or otherwise avoid mutating the shared
_stringBuilder field, perform the escape loop on the local builder, and return
its ToString() so BuildJsonExport's _stringBuilder is not cleared or modified.
| private void OnClearClicked() | ||
| { | ||
| EventStore.Clear(); | ||
| _currentEvents.Clear(); | ||
| _lastEventStoreCount = 0; | ||
| _eventListView.RefreshItems(); | ||
| _detailScrollView.Clear(); | ||
| UpdateStatus(); | ||
| McpLog.Debug("[ActionTraceEditorWindow] Clear clicked"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd MCPForUnity/Editor/Windows && cat -n ActionTraceEditorWindow.cs | head -100Repository: CoplayDev/unity-mcp
Length of output: 182
🏁 Script executed:
cat MCPForUnity/Editor/Windows/ActionTraceEditorWindow.cs | sed -n '1,150p'Repository: CoplayDev/unity-mcp
Length of output: 7270
🏁 Script executed:
# Look at the OnClearClicked method and surrounding methods
cat -n MCPForUnity/Editor/Windows/ActionTraceEditorWindow.cs | sed -n '770,870p'Repository: CoplayDev/unity-mcp
Length of output: 4076
🏁 Script executed:
# Search for OnSelectionChanged or similar methods that populate details
rg -n "OnSelectionChanged|SelectionChanged|DisplayEventDetails|RefreshDetail" MCPForUnity/Editor/Windows/ActionTraceEditorWindow.csRepository: CoplayDev/unity-mcp
Length of output: 204
🏁 Script executed:
# Check how the visual tree is structured
rg -n "visual tree|RemoveFromHierarchy|Add.*ScrollView|hierarchy" MCPForUnity/Editor/Windows/ActionTraceEditorWindow.csRepository: CoplayDev/unity-mcp
Length of output: 45
🏁 Script executed:
cat MCPForUnity/Editor/Windows/ActionTraceEditorWindow.csRepository: CoplayDev/unity-mcp
Length of output: 50011
Clearing the ScrollView removes UI elements from the visual tree permanently.
Calling _detailScrollView.Clear() removes all children (_detailPlaceholder, _detailContent, _detailActions) from the visual tree. When an event is subsequently selected, OnSelectionChanged tries to operate on orphaned elements, preventing the detail UI from rendering. The placeholder should remain visible until an item is selected.
🔧 Proposed fix
- _detailScrollView.Clear();
+ _detailContent.Clear();
+ _detailPlaceholder.style.display = DisplayStyle.Flex;
+ _detailContent.style.display = DisplayStyle.None;
+ _detailActions.style.display = DisplayStyle.None;📝 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 void OnClearClicked() | |
| { | |
| EventStore.Clear(); | |
| _currentEvents.Clear(); | |
| _lastEventStoreCount = 0; | |
| _eventListView.RefreshItems(); | |
| _detailScrollView.Clear(); | |
| UpdateStatus(); | |
| McpLog.Debug("[ActionTraceEditorWindow] Clear clicked"); | |
| private void OnClearClicked() | |
| { | |
| EventStore.Clear(); | |
| _currentEvents.Clear(); | |
| _lastEventStoreCount = 0; | |
| _eventListView.RefreshItems(); | |
| _detailContent.Clear(); | |
| _detailPlaceholder.style.display = DisplayStyle.Flex; | |
| _detailContent.style.display = DisplayStyle.None; | |
| _detailActions.style.display = DisplayStyle.None; | |
| UpdateStatus(); | |
| McpLog.Debug("[ActionTraceEditorWindow] Clear clicked"); | |
| } |
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/Windows/ActionTraceEditorWindow.cs` around lines 843 -
851, The Clear handler currently calls _detailScrollView.Clear(), which removes
_detailPlaceholder/_detailContent/_detailActions from the visual tree and breaks
OnSelectionChanged; change OnClearClicked to preserve the placeholder element
instead of clearing the whole scroll view: do not call
_detailScrollView.Clear(), instead clear the contents of _detailContent and
_detailActions (e.g., remove their child elements or reset text) and ensure
_detailPlaceholder is made visible (or re-added) so the detail UI remains in the
visual tree for future selections; update references in OnSelectionChanged to
assume the placeholder exists.
| private void OnEnable() | ||
| { | ||
| EditorApplication.update += OnEditorUpdate; | ||
| _isScheduledRefreshActive = true; | ||
| } | ||
|
|
||
| private void OnDisable() | ||
| { | ||
| EditorApplication.update -= OnEditorUpdate; | ||
| _isScheduledRefreshActive = false; | ||
|
|
||
| if (ActionTraceSettings.Instance != null && _previousBypassImportanceFilter.HasValue) | ||
| { | ||
| ActionTraceSettings.Instance.Filtering.BypassImportanceFilter = _previousBypassImportanceFilter.Value; | ||
| } | ||
| } | ||
|
|
||
| private void OnEditorUpdate() | ||
| { | ||
| // Performance optimization: use time interval to control refresh rate | ||
| if (_isScheduledRefreshActive && | ||
| EditorApplication.timeSinceStartup - _lastRefreshTime > RefreshInterval) | ||
| { | ||
| _lastRefreshTime = EditorApplication.timeSinceStartup; | ||
| RefreshEvents(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's locate and examine the ActionTraceEditorWindow.cs file
find . -name "ActionTraceEditorWindow.cs" -type fRepository: CoplayDev/unity-mcp
Length of output: 182
🏁 Script executed:
#!/bin/bash
# Get the file size to determine if we can read it completely
wc -l ./MCPForUnity/Editor/Windows/ActionTraceEditorWindow.csRepository: CoplayDev/unity-mcp
Length of output: 123
🏁 Script executed:
#!/bin/bash
# Search for CreateGUI method and OnEditorUpdate method to understand the lifecycle
rg -n "CreateGUI|OnEnable|OnDisable|OnEditorUpdate" ./MCPForUnity/Editor/Windows/ActionTraceEditorWindow.cs | head -20Repository: CoplayDev/unity-mcp
Length of output: 339
🏁 Script executed:
#!/bin/bash
# Look for _actionTraceQuery and _eventListView field declarations and usage
rg -n "_actionTraceQuery|_eventListView" ./MCPForUnity/Editor/Windows/ActionTraceEditorWindow.cs | head -30Repository: CoplayDev/unity-mcp
Length of output: 1369
🏁 Script executed:
#!/bin/bash
# Get the full OnEditorUpdate method implementation to see what it does
sed -n '1220,1250p' ./MCPForUnity/Editor/Windows/ActionTraceEditorWindow.csRepository: CoplayDev/unity-mcp
Length of output: 498
🏁 Script executed:
#!/bin/bash
# Find RefreshEvents method and see what it does
rg -n "RefreshEvents" ./MCPForUnity/Editor/Windows/ActionTraceEditorWindow.csRepository: CoplayDev/unity-mcp
Length of output: 416
🏁 Script executed:
#!/bin/bash
# Get the RefreshEvents method implementation
sed -n '920,960p' ./MCPForUnity/Editor/Windows/ActionTraceEditorWindow.csRepository: CoplayDev/unity-mcp
Length of output: 1607
🏁 Script executed:
#!/bin/bash
# Check the full context of CreateGUI and where fields are initialized
sed -n '160,280p' ./MCPForUnity/Editor/Windows/ActionTraceEditorWindow.csRepository: CoplayDev/unity-mcp
Length of output: 6121
🏁 Script executed:
#!/bin/bash
# Check the beginning of RefreshEvents to see if there are existing null checks
sed -n '890,920p' ./MCPForUnity/Editor/Windows/ActionTraceEditorWindow.csRepository: CoplayDev/unity-mcp
Length of output: 1767
Add null guards in OnEditorUpdate to prevent accessing uninitialized UI components.
RefreshEvents() directly accesses _actionTraceQuery and _eventListView without null checks. If CreateGUI returns early (when UXML fails to load), these fields remain uninitialized. Meanwhile, OnEnable immediately subscribes OnEditorUpdate to the editor update loop, causing it to invoke RefreshEvents() with null references on the first tick.
Proposed fix
private void OnEditorUpdate()
{
+ if (_actionTraceQuery == null || _eventListView == null)
+ return;
// Performance optimization: use time interval to control refresh rate
if (_isScheduledRefreshActive &&
EditorApplication.timeSinceStartup - _lastRefreshTime > RefreshInterval)🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/Windows/ActionTraceEditorWindow.cs` around lines 1203 -
1228, OnEditorUpdate currently calls RefreshEvents() regardless of whether the
UI was initialized in CreateGUI, which can lead to null dereferences on
_actionTraceQuery and _eventListView; add a guard at the start of OnEditorUpdate
to return early if those UI fields are null (e.g., if _actionTraceQuery == null
|| _eventListView == null) or alternatively move the call to RefreshEvents
behind a boolean flag set by CreateGUI when initialization succeeds; ensure the
guard references OnEditorUpdate, RefreshEvents, _actionTraceQuery,
_eventListView and preserves the existing scheduled refresh logic.
| # Retrieve from context if not explicitly provided | ||
| effective_task_id = task_id or _current_task_id.get() | ||
| effective_conv_id = conversation_id or _current_conversation_id.get() | ||
|
|
||
| # Auto-generate if still empty | ||
| if not effective_task_id: | ||
| effective_task_id = f"task-{uuid.uuid4().hex[:8]}" | ||
| if not effective_conv_id: | ||
| effective_conv_id = f"conv-{uuid.uuid4().hex[:8]}" | ||
|
|
There was a problem hiding this comment.
Persist auto-generated task/conversation IDs to session context.
Line 118-121 generates IDs when context is empty, but they aren’t written back to _current_task_id / _current_conversation_id. Subsequent calls without explicit IDs will generate new ones, breaking task-level grouping.
✅ Suggested fix
- if not effective_task_id:
- effective_task_id = f"task-{uuid.uuid4().hex[:8]}"
- if not effective_conv_id:
- effective_conv_id = f"conv-{uuid.uuid4().hex[:8]}"
+ if not effective_task_id:
+ effective_task_id = f"task-{uuid.uuid4().hex[:8]}"
+ _current_task_id.set(effective_task_id)
+ if not effective_conv_id:
+ effective_conv_id = f"conv-{uuid.uuid4().hex[:8]}"
+ _current_conversation_id.set(effective_conv_id)🤖 Prompt for AI Agents
In `@Server/src/services/tools/add_action_trace_note.py` around lines 113 - 122,
The generated IDs for effective_task_id and effective_conv_id are not persisted
back to session context, causing new IDs to be created on subsequent calls;
after auto-generating (in the block that sets effective_task_id =
f"task-{uuid.uuid4().hex[:8]}" and effective_conv_id =
f"conv-{uuid.uuid4().hex[:8]}"), assign those values into the context stores
(_current_task_id.set(effective_task_id) and
_current_conversation_id.set(effective_conv_id)) so future calls that read
_current_task_id.get() and _current_conversation_id.get() see the same IDs and
preserve task-level grouping.
|
@whatevertogo thank you for sharing these cool features, you are clearly spending quite a bit of time building and improving them, and they seem interesting and creative. However for now, they are not the right fit for the Unity MCP. For one thing, large PRs where you're adding 16,000 lines of code are difficult and time consuming for us to review -- we ourselves almost never make additions of that size. It's just too much to evaluate and maintain. Also, it's a bit tricky to decide if the hook and trace features make sense in the mcp project. They seem like a slightly different type of AI development feature, perhaps beyond the scope of MCP (or something to use alongside it). For this reason we're going to pass on these PRs for now. But we encourage you to continue working on them and share them with our community Discord for others to try. Or perhaps share some video demos of how you use them. If people start to use them and feel they should be included in the mcp project, we may be able to return to the discussion. Thanks again! |
Thank you for your feedback; I fully understand and share your concerns regarding the PR size. Regarding the design, I firmly believe that the Hook system will be essential to this project in the long run, as it enables the AI agent to automatically and precisely guide its next steps based on various Unity runtime states. The introduction of Action Trace stems from the reality that AI cannot yet achieve full autonomy and still requires human intervention. During this collaborative process, the agent often lacks complete situational awareness; therefore, tracing the context is vital for the agent to understand the current state and provide meaningful assistance. These are just my personal reflections on the matter. Thank you again for your professional insights—your perspective is truly invaluable. Actually,This is only about 7,200 lines of code, but I wrote a lot of comments, plus about 800 lines of documentation. If it helps, I would be happy to break this down into smaller, more manageable PRs to make the review process easier for you. |
|
I’ll show my Action Trace system this weekend. |
|
@whatevertogo Sorry for the late reply on my end - aside from personal things to attend to I needed to think about this one a bit more. I think the video will help a lot. It's not immediately clear to me how this resource actually helps AI tools when building something. It looks awesome, but what's the real world use case? For e.g. recently @dsarno added a tool specifically for interacting with scriptable objects. The capability existed, but it was inefficient and prone to error. So by adding the tool, the AI tool got over a hurdle with Unity MCP that it couldn't do before. In this case, what was it that your AI tool COULD NOT DO, and how does this actually make it accomplish more. As a side note, you've been contributing a lot and I really really appreciate it. I think all of the core maintainers do. Even with disagreements (which we have amongst ourselves too), we hope that you continue being a part of our community and contributing. You're awesome ⭐ |
There was a problem hiding this comment.
Pull request overview
This PR introduces a comprehensive event tracking and monitoring infrastructure for AI-assisted Unity development, consisting of two foundational systems:
Changes:
- Implements HookSystem: A general-purpose event dispatch infrastructure that abstracts Unity's native callbacks into a clean, thread-safe subscription interface with exception isolation
- Implements ActionTrace: A sophisticated event tracking system featuring event recording, semantic analysis, dehydration for memory efficiency, and AI-friendly query interfaces
- Adds MCP tools for AI interaction:
get_action_trace,get_action_trace_summary,add_action_trace_note,undo_to_sequence, andget_action_trace_settings
Reviewed changes
Copilot reviewed 76 out of 76 changed files in this pull request and generated 57 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/Hooks/DESIGN.md | Comprehensive design documentation for Hook system architecture |
| docs/Hooks/CONTRIBUTING.md | Contributing guide for extending the Hook system |
| docs/ActionTrace/DESIGN.md | Design philosophy and architectural decisions for ActionTrace |
| docs/ActionTrace/CONTRIBUTING.md | Contributing guide for ActionTrace extensions |
| Server/src/services/tools/*.py | Python MCP tool implementations for ActionTrace queries |
| Server/src/services/resources/action_trace.py | Python resource endpoints for ActionTrace |
| Server/src/models/action_trace.py | Pydantic models for ActionTrace data validation |
| MCPForUnity/Editor/Windows/ActionTraceEditorWindow.* | Unity Editor window for visualizing action traces |
| MCPForUnity/Editor/Tools/*Tool.cs | C# MCP tool handlers for ActionTrace operations |
| MCPForUnity/Editor/Resources/ActionTrace/*.cs | C# resource handlers and query infrastructure |
| MCPForUnity/Editor/Hooks/*.cs | Core Hook system implementation with event registry |
| MCPForUnity/Editor/Helpers/*.cs | Helper utilities for GlobalID resolution and build targets |
| MCPForUnity/Editor/ActionTrace/**/*.cs | ActionTrace subsystems (Semantics, Integration, Sources) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| foreach (var assetPath in importedAssets) | ||
| { | ||
| if (string.IsNullOrEmpty(assetPath)) continue; | ||
|
|
||
| // L0 Deduplication: Skip if already processed in this session | ||
| // This prevents duplicate events when Unity fires OnPostprocessAllAssets | ||
| // multiple times for the same asset (creation, compilation, re-import) | ||
| if (!_processedAssetsInSession.Add(assetPath)) | ||
| continue; // Already processed, skip to prevent duplicate events | ||
|
|
||
| hasChanges = true; // Mark that we added a new entry | ||
|
|
||
| // L1 Blacklist: Skip junk assets before creating events | ||
| if (!EventFilter.ShouldTrackAsset(assetPath)) | ||
| { | ||
| // Remove from tracking if it's a junk asset (we don't want to track it) | ||
| _processedAssetsInSession.Remove(assetPath); | ||
| continue; | ||
| } | ||
|
|
||
| string targetId = $"Asset:{assetPath}"; | ||
| string assetType = GetAssetType(assetPath); | ||
|
|
||
| var payload = new Dictionary<string, object> | ||
| { | ||
| ["path"] = assetPath, | ||
| ["extension"] = System.IO.Path.GetExtension(assetPath), | ||
| ["asset_type"] = assetType | ||
| }; | ||
|
|
||
| // Mutually exclusive event classification (prevents duplicate events) | ||
| if (IsNewlyCreatedAsset(assetPath)) | ||
| { | ||
| // Priority 1: Newly created assets (first-time existence) | ||
| RecordEvent(EventTypes.AssetCreated, targetId, payload); | ||
| } | ||
| else if (ShouldTrackModification(assetPath)) | ||
| { | ||
| // Priority 2: Existing assets with trackable modification types | ||
| // Covers: re-imports, content changes, settings updates | ||
| RecordEvent(EventTypes.AssetModified, targetId, payload); | ||
| } | ||
| else | ||
| { | ||
| // Priority 3: Generic imports (fallback for untracked types) | ||
| RecordEvent(EventTypes.AssetImported, targetId, payload); | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
| foreach (var assetPath in deletedAssets) | ||
| { | ||
| if (string.IsNullOrEmpty(assetPath)) continue; | ||
|
|
||
| // L0 Deduplication: Skip if already processed in this session | ||
| if (!_processedAssetsInSession.Add(assetPath)) | ||
| continue; | ||
|
|
||
| hasChanges = true; // Mark that we added a new entry | ||
|
|
||
| // L1 Blacklist: Skip junk assets | ||
| if (!EventFilter.ShouldTrackAsset(assetPath)) | ||
| continue; | ||
|
|
||
| string targetId = $"Asset:{assetPath}"; | ||
|
|
||
| var payload = new Dictionary<string, object> | ||
| { | ||
| ["path"] = assetPath | ||
| }; | ||
|
|
||
| RecordEvent(EventTypes.AssetDeleted, targetId, payload); | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
| foreach (string key in keyList) | ||
| { | ||
| if (HasMeaningfulValue(evt, key.Trim())) | ||
| return thenText; | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
| foreach (string key in keyList) | ||
| { | ||
| if (!HasMeaningfulValue(evt, key.Trim())) | ||
| return ""; | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
| foreach (var kvp in _pendingChanges) | ||
| { | ||
| // When forced, flush all entries. Otherwise, only flush expired entries. | ||
| if (force || nowMs - kvp.Value.LastUpdateMs >= DebounceWindowMs) | ||
| { | ||
| // Record the PropertyModified event | ||
| RecordPropertyModifiedEvent(kvp.Value); | ||
|
|
||
| // Return to object pool | ||
| ReturnPendingChange(kvp.Value); | ||
|
|
||
| // Mark for removal | ||
| _removedKeys.Add(kvp.Key); | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
| from typing import Annotated, Any, Literal, Optional | ||
|
|
||
| from fastmcp import Context | ||
| from mcp.types import ToolAnnotations |
There was a problem hiding this comment.
Import of 'ToolAnnotations' is not used.
| from models.action_trace import ( | ||
| EditorEvent, | ||
| EventQueryResult, | ||
| EventStatistics, | ||
| ActionTraceSettings, | ||
| ) |
There was a problem hiding this comment.
Import of 'ActionTraceSettings' is not used.
|
|
||
| from services.registry import mcp_for_unity_tool | ||
| from services.tools import get_unity_instance_from_context | ||
| from services.tools.utils import coerce_bool |
There was a problem hiding this comment.
Import of 'coerce_bool' is not used.
| Unity implementation: MCPForUnity/Editor/Tools/ActionTraceSettingsTool.cs | ||
| """ | ||
| from typing import Annotated, Any |
There was a problem hiding this comment.
Import of 'Annotated' is not used.
| parsed = json.loads(value) | ||
| if isinstance(parsed, list): | ||
| return _coerce_int_list(parsed) # Recursively handle parsed list | ||
| except json.JSONDecodeError: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
whatevertogo
force-pushed
the
feature/ActionTrace
branch
from
2cdeb19 to
6feadbf
Compare
… and improve property modification handling. Removed unused UndoCapture and UndoToSequenceTool files. Updated ActionTrace settings and enhanced Python tool descriptions for clarity.
|
Just re-iterataing @whatevertogo, please explain the exact use case where this feature improved what the model could do
|
https://www.youtube.com/watch?v=ykhhOa-g7PI By the way,I’m working on improving the manage_prefab implementation. |
3 participants
This PR introduces two foundational systems designed to provide AI agents with "eyes and memory" within the Unity Editor:
ActionTrace: A sophisticated event tracking system that records, summarizes, and analyzes editor operations. It features a unique Dehydration mechanism (reducing memory from ~10KB to ~100 bytes per event) to ensure long-term stability.
HookSystem: A decoupled event dispatching backbone that abstracts Unity's native callbacks into a clean, thread-safe subscription interface.
These systems collectively allow the AI assistant to reason about what happened in the scene, why it happened (intent), and how it relates to the current task.
🏗 Architecture & Event Flow
The following diagram illustrates how a raw Unity event is transformed into high-level AI context:
Data Pipeline: Unity Native Events → UnityEventHooks → HookRegistry → ActionTraceRecorder → EventStore → Semantic Analysis (Scorer/Categorizer) → MCP Tools
🛠 Changes Made
Capture Layer: Implemented Recorder.cs and SamplingMiddleware.cs for noise-reduced event logging.
Core Layer: Developed an immutable EditorEvent model with automatic dehydration and a thread-safe EventStore.
Analysis Layer: Added EventSummarizer and TransactionAggregator to turn raw logs into human-readable/AI-friendly narratives.
Semantics: Introduced scoring, categorization, and intent inference engines.
Unified API: HookRegistry.cs provides a single point of entry for all editor events.
Coverage: Support for Components, GameObjects, Scenes, Selection, Play Mode, Scripts, and Build events.
Robustness: Built-in exception isolation to prevent third-party subscriber crashes from affecting the Editor.
GetActionTrace: Core retrieval tool with query_mode support.
GetActionTraceSummary: Optimized for token efficiency.
AddActionTraceNote: Allows AI to inject its own reasoning into the timeline.
⚡ Key Highlights
Memory Efficiency: Events are automatically dehydrated to minimize footprint during long sessions.
Resilience: Survives domain reloads; ensures high availability of history.
Agent-Centric: Specifically tuned for AI context windows via intelligent sampling and transaction aggregation.
🧪 Testing & Validation
[x] Verified event capture across Scene saves/loads.
[x] Confirmed Dehydration logic triggers after the memory threshold is met.
Summary by CodeRabbit
Release Notes
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.