Feat:actiontrace Feat:HookSystem

[whatevertogo]

Summary by CodeRabbit

• New Features

  • Comprehensive ActionTrace: records editor events (assets, properties, GameObjects, builds, tools) and stores context.
  • ActionTrace Editor Window: browse, search, filter, sort, export, and inspect event details and semantics.
  • Query & Tools: get action history, statistics, settings; add AI notes; undo editor state to a specific trace sequence.
  • Presets & Settings: configurable capture/merging/sampling presets and runtime settings.

• Documentation

  • Added presets and settings guidance.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

Add HookSystem

Hook UnityEvents

[sourcery-ai]

Sorry @whatevertogo, your pull request is larger than the review limit of 300000 diff characters

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds a comprehensive ActionTrace subsystem to the Unity editor: event capture points, sampling, filtering, scoring/categorization, context and tool-call tracking, an append-only EventStore with persistence and merging, query/aggregation/compression layers, an ActionTrace editor window, Python server endpoints/tools, and multiple helpers/integrations.

Changes

Cohort / File(s)	Summary
Context & Operation Tracking MCPForUnity/Editor/ActionTrace/Context/*	New immutable OperationContext, thread-local ContextStack, ContextMapping side-table, ContextTrace subscription to EventStore.EventRecorded, and ToolCallScope/ToolCall helpers for hierarchical tool-call instrumentation.
Core Models & Registry MCPForUnity/Editor/ActionTrace/Core/*	New EditorEvent model, EventCategory, SamplingMode, EventMetadata, EventTypes registry, presets, GlobalIdHelper, and ActionTraceSettings (layered ScriptableObject with presets & persistence).
Event Store & Persistence MCPForUnity/Editor/ActionTrace/Core/Store/*	New thread-safe EventStore with Record/Query/QueryAll/Count/CurrentSequence, merging, dehydration, context mapping storage, diagnostics, deferred persistence, trimming logic and domain-reload safety.
Capture Points & Emission MCPForUnity/Editor/ActionTrace/Capture/*	New capture framework: capture point interfaces/registry, built-in capture points, Asset/Property/Selection/Undo capturers, ManageAsset bridge, ActionTraceEventEmitter, sampling middleware and sampling strategies, event filtering and stats.
Analysis, Query & Summarization MCPForUnity/Editor/ActionTrace/Analysis/*	EventQueryBuilder DSL, ActionTraceQuery projection (semantic enrichment), TransactionAggregator, ContextCompressor and compression extensions, EventSummarizer templating and summarization.
Semantics MCPForUnity/Editor/ActionTrace/Semantics/*	New IEventScorer/IEventCategorizer/IIntentInferrer interfaces and default implementations (scorer, categorizer, intent inferrer).
UI & Editor Integration MCPForUnity/Editor/Windows/*, MenuItems/*, Resources/*	New ActionTraceEditorWindow (UXML/USS), menu entry, resources endpoint ActionTraceViewResource, export/clipboard support, and window UI wiring.
Tools & Server Integration MCPForUnity/Editor/Tools/*, Server/src/*	Editor tools (AddActionTraceNoteTool, GetActionTraceTool, ActionTraceSettingsTool, UndoToSequenceTool), Python Pydantic models, server resources and tools (get_action_trace, statistics, settings, add_action_trace_note, undo_to_sequence).
Hooks, Sources & VCS MCPForUnity/Editor/Hooks/*, Sources/Unity/*, Integration/VCS/*	New HookRegistry events, UnityEventHooks for many editor lifecycle events, VcsContextProvider injecting Git context into events.
Helpers & Utilities MCPForUnity/Editor/ActionTrace/Helpers/*, MCPForUnity/Editor/Helpers/*	Formatting, payload builders, property formatting, undo reflection, gameobject tracking, build target utility, GlobalId helper and other utilities.

Sequence Diagram(s)

sequenceDiagram
    participant Capture as "Capture Point\n(Asset/Property/Selection)"
    participant Sampling as "SamplingMiddleware\n(Debounce/Throttle)"
    participant Store as "EventStore\n(Record, Merge, Persist)"
    participant Context as "ContextStack / ContextTrace"
    participant UI as "ActionTraceEditorWindow"
    participant API as "Python Resource\nget_action_trace"

    rect rgba(200,200,255,0.5)
      Capture->>Sampling: Emit EditorEvent
      Sampling->>Store: ShouldRecord? -> Record or Cache
      Store->>Context: EventRecorded notification
      Context->>Store: AddContextMapping (if current context exists)
    end

    rect rgba(200,255,200,0.5)
      UI->>API: query (limit, semantics)
      API->>Store: request events (via Unity transport)
      Store-->>API: return events (with semantics/context if requested)
      API-->>UI: response
    end

Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Possibly related PRs

• 🔧 Clean up & Consolidate Shared Services Across MCP Tools #519 — Related shared editor helper services and Manage* tool refactors that overlap with ActionTrace helpers and ManageAsset integrations.
• Fix stdio reloads #402 — Changes to StdioBridgeHost domain-reload and listener lifecycle, directly related to domain-reload handling added here.
• Async Test Infrastructure & Editor Readiness Status + new refresh_unity tool #507 — Modifies ManageAsset behavior touching the same file where this PR adds ActionTrace events/invocations.

Suggested reviewers

• justinpbarnett
• msanatan

Poem

🐰
I hopped through editor logs with care,
Captured each click, each change in air.
Context and notes in tidy rows,
Now every tool-call kindly shows.
Hop! The trace is stitched — what a flair! ✨

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 69.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title 'Feat:actiontrace Feat:HookSystem' is partially related to the changeset but lacks clarity and conciseness. While the PR does introduce ActionTrace and a Hook system, the title uses vague formatting with multiple feature markers and does not clearly communicate the primary change or scope.	Consider revising to a single, concise title that highlights the main feature, such as 'Add ActionTrace event tracing system with hook registry' or 'Implement ActionTrace and unified editor event hooks'.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 18

Note

Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

MCPForUnity/Editor/Tools/ManageAsset.cs (1)

218-223: Folder creation bypasses OnAssetCreated

assetType == "folder" routes to CreateFolder, and the create_folder action also uses CreateFolder, but that method never raises OnAssetCreated. If ActionTrace expects folder creation events, emit the callback on success.

🛠️ Suggested fix (emit on success)

                 string guid = AssetDatabase.CreateFolder(parentDir, folderName);
                 if (string.IsNullOrEmpty(guid))
                 {
                     return new ErrorResponse(
                         $"Failed to create folder '{fullPath}'. Check logs and permissions."
                     );
                 }
 
+                OnAssetCreated?.Invoke(fullPath, "folder");
+
                 // AssetDatabase.Refresh(); // CreateFolder usually handles refresh
                 return new SuccessResponse(
                     $"Folder '{fullPath}' created successfully.",
                     GetAssetData(fullPath)

Also applies to: 333-355

🤖 Fix all issues with AI agents

In `@MCPForUnity/Editor/ActionTrace/Capture/AssetChangePostprocessor.cs`:
- Around line 103-128: Duplicate events occur because the same asset can be
emitted as both AssetImported and AssetModified; update the modified-assets loop
in the importedAssets foreach to skip assets already emitted as imports by
checking the same condition used for "import" emission (e.g., if
IsNewlyCreatedAsset(assetPath) or a flag indicating you emitted AssetImported),
so only one event is recorded; modify the foreach that calls
ShouldTrackModification and RecordEvent(EventTypes.AssetModified, ...) to
early-continue when the asset was already handled as imported (use
IsNewlyCreatedAsset or introduce/consult a local HashSet<string> of
alreadyEmitted assets) and keep RecordEvent and ShouldTrackModification logic
unchanged for truly modified assets.

In `@MCPForUnity/Editor/ActionTrace/Capture/PropertyChangeTracker.cs`:
- Around line 155-188: FlushPendingChanges currently only emits entries older
than DebounceWindowMs causing ForceFlush to drop recent changes; change
FlushPendingChanges to accept a bool force (default false) and when force==true
bypass the debounce age check so all _pendingChanges are recorded, returned to
pool, and removed; update the EditorApplication.update caller to call the
default path and modify the ForceFlush method (the one around lines 243-250) to
call FlushPendingChanges(true) so shutdown/force flush emits all pending
entries.

In `@MCPForUnity/Editor/ActionTrace/Capture/SamplingMiddleware.cs`:
- Around line 360-401: SamplingConfig.Strategies is a non-thread-safe Dictionary
accessed concurrently (e.g., ShouldRecord, FlushExpiredDebounceSamples via
EditorApplication.update, and SetStrategy/RemoveStrategy), which can throw
during enumeration; fix by making access thread-safe — replace the Dictionary
with a ConcurrentDictionary<string, SamplingStrategy> or guard all reads/writes
with a lock object, update SetStrategy and RemoveStrategy to use the new
concurrent API or acquire the lock, and ensure ShouldRecord and
FlushExpiredDebounceSamples read from the thread-safe store (or snapshot
keys/values under the same lock) to avoid modification-during-enumeration
exceptions.

In `@MCPForUnity/Editor/ActionTrace/Context/ToolCallScope.cs`:
- Around line 471-494: ExecuteAsync<T> creates a ToolCallScope (scope) but never
disposes it, leaking the scope on the stack; update ExecuteAsync<T> (the method
and its use of the local variable scope) to ensure scope.Dispose() is always
called: either refactor to an async/await implementation that wraps the awaited
func() in a try/finally and calls scope.Complete()/scope.Fail() then
scope.Dispose() in the finally, or keep ContinueWith but invoke scope.Dispose()
in both the faulted and success branches (ensuring the original exception is
rethrown unchanged). Ensure the disposal happens after calling
scope.Complete/Fail and that the thrown exception semantics remain the same.

In `@MCPForUnity/Editor/ActionTrace/Core/EventStore.Context.cs`:
- Around line 155-176: The current grouping creates mappingBySequence with
GroupBy(...).ToDictionary(... FirstOrDefault()) which discards extra context
mappings for the same EventSequence; update the grouping logic so
mappingBySequence stores all mappings per sequence (e.g., ToDictionary(g =>
g.Key, g => g.ToList()) or similar) and then change the selection that builds
results (the Select over eventsSnapshot) to either emit one tuple per mapping
(SelectMany producing (Event, Context) for each mapping) or aggregate the
mappings into a collection on the Context field so all contexts added via
AddContextMapping are preserved; update references to mappingBySequence,
mappingsSnapshot, and the Select/SelectMany that constructs results accordingly.

In `@MCPForUnity/Editor/ActionTrace/Core/EventStore.Merging.cs`:
- Around line 99-105: The merge_count extraction in EventStore.Merging.cs
currently casts existingCount with (int) which will throw for deserialized
longs/doubles; update the logic in the block that reads from
_lastRecordedEvent.Payload and writes to newPayload to safely convert numeric or
string types to int (e.g., check types: if existingCount is int/long/double use
Convert.ToInt32 or cast from long after bounds check, or if string attempt
int.TryParse, and default to 1 on failure) so mergeCount increments robustly
without throwing during deserialization.

In `@MCPForUnity/Editor/ActionTrace/Core/SamplingMode.cs`:
- Around line 1-20: There are duplicate SamplingMode enums; remove the duplicate
enum declaration from the Capture namespace (the one inside
SamplingMiddleware.cs), update SamplingMiddleware to use the single canonical
enum in Core (SamplingMode) by adding using MCPForUnity.Editor.ActionTrace.Core;
and update any types that referenced the removed local enum (e.g., references in
SamplingMiddleware) to reference the Core versions used by EventMetadata and
EventTypes so all code uses the single SamplingMode definition.

In `@MCPForUnity/Editor/ActionTrace/Descriptors/IEventDescriptor.cs`:
- Around line 52-84: GetString and GetTargetName must guard against a null
EditorEvent.Payload and null values inside the payload to avoid
NullReferenceException; update GetString(EditorEvent evt, string key, string
defaultValue = "") to first check evt and evt.Payload for null and return
defaultValue if missing, then use TryGetValue and return value?.ToString() ??
defaultValue; update GetTargetName(EditorEvent evt) to return early if evt or
evt.Payload is null (fallback to evt?.TargetId ?? ""), and for each TryGetValue
check the out value for null before calling ToString(), falling back to the next
key and finally to evt?.TargetId ?? "".

In `@MCPForUnity/Editor/ActionTrace/Helpers/GameObjectTrackingHelper.cs`:
- Around line 28-29: The static MainThreadId is captured during type
initialization which can occur on a background thread; change it to be lazily
captured on first use in the main-thread check to avoid wrong values. Replace
the current readonly field initialization with a mutable backing field in
GameObjectTrackingHelper (e.g., int _mainThreadId = 0) and add a small helper
(e.g., EnsureMainThreadIdCaptured or a MainThreadId getter) that uses
thread-safe initialization (Interlocked.CompareExchange or Lazy<int>) to set the
backing field to Thread.CurrentThread.ManagedThreadId if still zero, and update
all places that reference MainThreadId to call that helper so the main thread id
is recorded when the real main thread first performs checks.

In `@MCPForUnity/Editor/ActionTrace/Query/EventQueryBuilder.cs`:
- Around line 369-387: The Execute() method calls EventStore.Query(int.MaxValue)
which triggers overflow and huge memory allocation; change Execute() to stop
passing int.MaxValue and instead call a new EventStore.QueryAll() (or a
Query(streaming:true) overload) so the store can handle streaming/pre-filtering,
or pass a sensible upper bound computed from _limit/_offset; update EventStore
to add QueryAll() that safely enumerates all events (or streams results) and
ensure Execute() continues to use MatchesFilters, ApplySorting, _offset and
_limit on the returned sequence rather than forcing full in-memory load.

In `@MCPForUnity/Editor/ActionTrace/Query/EventSummarizer.cs`:
- Around line 525-542: GetTargetName should guard against null values in
evt.Payload entries before calling ToString; update the method (GetTargetName in
EventSummarizer) to check that evt.Payload is non-null and that each TryGetValue
returned object is not null (or use a safe conversion) before returning its
string, otherwise continue to the next key and ultimately fall back to
evt.TargetId; ensure checks cover keys "name", "game_object", "scene_name",
"component_type", and "path" to avoid NullReferenceException from ToString().
- Around line 99-113: The placeholder replacement loop in EventSummarizer (the
block using var sb = new StringBuilder(result) and iterating evt.Payload) can
spin forever when a replacement value contains the same placeholder text;
replace the manual while loop with a single StringBuilder.Replace call per
placeholder (or if you prefer to keep the loop, advance the index by the
inserted value length after insertion) so the code uses sb.Replace(placeholder,
value) (or increments index by value.Length) when handling each kvp and calls
FormatValue(kvp.Value) once per replacement to avoid infinite matching.

In `@MCPForUnity/Editor/Hooks/UnityEventHooks.cs`:
- Around line 103-121: TrackGameObjectChanges currently calls
DetectGameObjectChanges which updates _previousInstanceIds before
GetDestroyedInstanceIds runs, preventing destroy detection; change the logic to
capture the current snapshot once, compare it to the existing
_previousInstanceIds to compute both created and destroyed sets in the same
pass, invoke HookRegistry.NotifyGameObjectCreated(change.obj) for newly created
GameObjects (using the created IDs to resolve objects) and Invoke
HookRegistry.NotifyGameObjectDestroyed(null) for each destroyed ID, then update
_previousInstanceIds to the current snapshot; adjust or replace
DetectGameObjectChanges/GetDestroyedInstanceIds accordingly so no intermediate
update to _previousInstanceIds happens before destroy detection.
- Around line 158-162: OnPlayModeStateChanged currently notifies for every
PlayModeStateChange (including transitional states); change it so it only emits
when the state is a final state: call HookRegistry.NotifyPlayModeChanged(true)
only when state == PlayModeStateChange.EnteredPlayMode and call
HookRegistry.NotifyPlayModeChanged(false) only when state ==
PlayModeStateChange.EnteredEditMode, and do nothing (return) for all other
states; locate the logic in the OnPlayModeStateChanged method and update the
conditional accordingly.

In `@MCPForUnity/Editor/Tools/ManageAsset.cs`:
- Around line 31-38: The event OnAssetModified currently emits the raw
properties object (new values only) but the docstring promises a
changesDictionary of {old,new} pairs; update the code that raises
OnAssetModified to compute a true diff: for each property path in the incoming
properties compare against the asset's current value, build a dictionary mapping
propertyPath -> { old = previousValue, new = newValue } (include only entries
where old != new), and pass that dictionary to OnAssetModified; ensure the event
payload matches the documentation (IReadOnlyDictionary with objects containing
old and new) and reuse the existing properties variable names when performing
the comparisons so consumers of OnAssetModified receive the described diff.

In `@MCPForUnity/Editor/Windows/ActionTraceEditorWindow.cs`:
- Around line 283-286: SetSortMode currently mutates the global
ActionTraceSettings.Instance.Filtering.BypassImportanceFilter unconditionally
(same as CreateGUI), causing unwanted global side effects; change SetSortMode to
stop modifying the global setting and instead maintain mode-specific behavior
locally (e.g., a private field on ActionTraceEditorWindow like currentSortMode
or bypassImportanceFilter) and use that local flag when applying filters during
queries/rendering, or only set BypassImportanceFilter on the global settings
when absolutely required for a global change; update any calls that read
ActionTraceSettings.Instance.Filtering.BypassImportanceFilter to consult the
local mode flag in SetSortMode/OnQuery instead.
- Around line 119-122: The window currently mutates the global
ActionTraceSettings by setting
ActionTraceSettings.Instance.Filtering.BypassImportanceFilter = true during
initialization; instead preserve and restore the original value or avoid
mutating the global state. In ActionTraceEditorWindow, capture the prior value
into a private field (e.g. _prevBypassImportanceFilter) when you set it, and
restore that value in OnDisable or OnDestroy; alternatively, replace usage with
a local filtering flag inside ActionTraceEditorWindow so you never touch
ActionTraceSettings.Instance.Filtering.BypassImportanceFilter. Ensure the
restore happens unconditionally to avoid leaving the global flag altered if the
window closes.

In `@MCPForUnity/Editor/Windows/ActionTraceEditorWindow.uss`:
- Around line 119-124: The .detail-placeholder USS rule uses the standard CSS
property text-align which Unity USS ignores; update the rule for
.detail-placeholder to remove text-align and use Unity's proprietary
-unity-text-align with the same value (e.g., -unity-text-align: middle-center)
so the text alignment works in Unity UI; keep the other properties (color,
font-size, padding-top) unchanged.

🟡 Minor comments (23)

Server/src/services/tools/add_action_trace_note.py-144-151 (1)

144-151: Handle MCPResponse objects explicitly to preserve error structure.

When async_send_command_with_retry fails, it returns an MCPResponse object with structured fields (success, error, hint). Converting it to a string loses this information. The same issue exists in manage_asset.py and manage_editor.py.

Use the established pattern from integration tests and HTTP transport handlers:

Proposed fix

-    return response if isinstance(response, dict) else {"success": False, "message": str(response)}
+    if isinstance(response, dict):
+        return response
+    if hasattr(response, 'model_dump'):
+        return response.model_dump()
+    return {"success": False, "message": str(response)}

MCPForUnity/Editor/ActionTrace/VCS/VcsContextProvider.cs-163-169 (1)

163-169: Remove unused variable gitPath.

The variable gitPath on line 168 is declared but never used in this method.

         try
         {
             var projectPath = System.IO.Path.GetDirectoryName(UnityEngine.Application.dataPath);
-            var gitPath = System.IO.Path.Combine(projectPath, ".git");

             // Find git executable
             string gitExe = FindGitExecutable();

MCPForUnity/Editor/ActionTrace/Semantics/DefaultEventScorer.cs-34-51 (1)

34-51: Potential logic inconsistency: AINote with null payload returns 0.1 instead of 1.0.

The comment states "AINote is always critical" (line 44), but the null-payload check (line 37-38) returns early with 0.1f before reaching the AINote check. If a dehydrated AINote event is possible, it would score 0.1 instead of 1.0.

If AINote should truly always be critical regardless of payload state, move the AINote check before the null check:

💡 Suggested reordering

 public float Score(EditorEvent evt)
 {
+    // Special case: AINote is always critical
+    if (evt.Type == "AINote")
+        return 1.0f;
+
     // Dehydrated events (Payload is null) use low default score
     if (evt.Payload == null)
         return 0.1f;

     // Get base score from metadata
     var meta = EventTypes.Metadata.Get(evt.Type);
     float baseScore = meta.DefaultImportance;

-    // Special case: AINote is always critical
-    if (evt.Type == "AINote")
-        return 1.0f;
-
     // Apply payload-based adjustments for asset events
     float adjustment = GetPayloadAdjustment(evt);
     return Mathf.Clamp01(baseScore + adjustment);
 }

MCPForUnity/Editor/ActionTrace/VCS/VcsContextProvider.cs-64-76 (1)

64-76: Thread-safety claim in documentation is not accurate.

The comment states "Thread-safe (called from any thread during event recording)" but the implementation has no synchronization:

• _currentContext can be read and written concurrently from GetCurrentContext() and RefreshContext().
• The null-check-then-assign pattern (lines 70-73) is a classic race condition.

If this is only called from Unity's main thread (which is typical for Editor code), update the documentation to reflect that:

 /// <summary>
 /// Gets the current VCS context for event injection.
-/// Thread-safe (called from any thread during event recording).
+/// Must be called from the main thread.
 /// </summary>

Alternatively, if true thread-safety is needed, consider using Volatile.Read/Volatile.Write or a lock.

MCPForUnity/Editor/ActionTrace/Helpers/ActionTraceHelper.cs-28-47 (1)

28-47: Documentation examples don't match actual behavior.

The regex only capitalizes the first letter after each underscore. For example:

• "manage_gameobject" → "Manage Gameobject" (not "Manage GameObject" as documented)
• "add_ActionTrace_note" → "Add ActionTrace Note" (this one works because "ActionTrace" already has capital letters)

The current implementation is reasonable for snake_case conversion, but the doc examples are misleading.

📝 Fix documentation

         /// Examples:
-        /// - "manage_gameobject" → "Manage GameObject"
+        /// - "manage_gameobject" → "Manage Gameobject"
         /// - "add_ActionTrace_note" → "Add ActionTrace Note"
         /// - "get_ActionTrace" → "Get ActionTrace"

MCPForUnity/Editor/Helpers/GlobalIdHelper.cs-131-142 (1)

131-142: GameObject.Find may resolve to wrong object in multi-scene setup.

GameObject.Find(hierarchyPath) searches all loaded scenes globally, not just the scene specified in scenePath. If multiple scenes are loaded with identically-named objects at the same hierarchy path, this could return the wrong object.

Consider searching within the specific scene's root objects:

Proposed scene-scoped resolution

                 // Find GameObject by hierarchy path
-                var found = GameObject.Find(hierarchyPath);
-                return found;
+                // Search within the specific scene
+                foreach (var root in scene.GetRootGameObjects())
+                {
+                    if (hierarchyPath.StartsWith(root.name))
+                    {
+                        // If path is just the root name
+                        if (hierarchyPath == root.name)
+                            return root;
+                        
+                        // Otherwise find child by relative path
+                        string relativePath = hierarchyPath.Substring(root.name.Length + 1);
+                        var found = root.transform.Find(relativePath);
+                        if (found != null)
+                            return found.gameObject;
+                    }
+                }
+                return null;

MCPForUnity/Editor/Hooks/HookTest.cs-1-84 (1)

1-84: Entirely commented-out file should be removed or converted.

This file contains only commented-out code, which creates noise in the codebase. Consider one of these alternatives:

1. Remove the file – If it's not needed, don't commit it
2. Convert to actual tests – Place in a test project with proper test framework assertions
3. Use conditional compilation – Wrap with #if DEBUG_HOOK_EVENTS if needed for debugging
4. Create documentation – If serving as an example, a markdown file or XML doc would be clearer

MCPForUnity/Editor/ActionTrace/Semantics/DefaultIntentInferrer.cs-62-63 (1)

62-63: Batch detection should key off the current event type.

Line 155 uses the first surrounding event’s type; if it differs from the current event, batch inference may be missed.

🛠️ Suggested update

-                EventTypes.HierarchyChanged when IsBatchOperation(surrounding) => "Batch Operation",
+                EventTypes.HierarchyChanged when IsBatchOperation(evt, surrounding) => "Batch Operation",
@@
-        private static bool IsBatchOperation(IReadOnlyList<EditorEvent> surrounding)
+        private static bool IsBatchOperation(EditorEvent evt, IReadOnlyList<EditorEvent> surrounding)
         {
             // Many events of the same type suggest a batch operation
             if (surrounding.Count < 5) return false;
 
-            var firstType = surrounding[0].Type;
+            var targetType = evt.Type;
             int sameTypeCount = 0;
             foreach (var e in surrounding)
             {
-                if (e.Type == firstType) sameTypeCount++;
+                if (e.Type == targetType) sameTypeCount++;
             }
             return sameTypeCount >= 5;
         }

Also applies to: 150-161

MCPForUnity/Editor/ActionTrace/Core/EventStore.Diagnostics.cs-16-27 (1)

16-27: Guard against negative hotEventCount to avoid out-of-range indexing.

If settings are misconfigured, _events.Count - hotEventCount can exceed the list bounds.

🛠️ Suggested update

         private static void DehydrateOldEvents(int hotEventCount)
         {
+            hotEventCount = Math.Max(0, hotEventCount);
             // Find events that need dehydration (not already dehydrated and beyond hot count)
             for (int i = 0; i < _events.Count - hotEventCount; i++)
             {
                 var evt = _events[i];

MCPForUnity/Editor/ActionTrace/Semantics/DefaultIntentInferrer.cs-70-115 (1)

70-115: Make extension checks case-insensitive to avoid missed intent inference.

Line 72+ compares extensions with case-sensitive equality; uppercase extensions (e.g., “.PNG”) won’t match.

🛠️ Suggested update

-        private static bool IsScript(EditorEvent e)
-        {
-            if (e.Payload.TryGetValue("extension", out var ext))
-                return ext.ToString() == ".cs";
-            if (e.Payload.TryGetValue("component_type", out var type))
-                return type.ToString()?.Contains("MonoBehaviour") == true;
-            return false;
-        }
+        private static bool HasExtension(EditorEvent e, params string[] extensions)
+        {
+            if (!e.Payload.TryGetValue("extension", out var ext) || ext == null) return false;
+            var extStr = ext.ToString();
+            foreach (var candidate in extensions)
+            {
+                if (string.Equals(extStr, candidate, StringComparison.OrdinalIgnoreCase))
+                    return true;
+            }
+            return false;
+        }
+
+        private static bool IsScript(EditorEvent e)
+        {
+            if (HasExtension(e, ".cs"))
+                return true;
+            if (e.Payload.TryGetValue("component_type", out var type))
+                return type.ToString()?.Contains("MonoBehaviour") == true;
+            return false;
+        }
@@
-        private static bool IsScene(EditorEvent e)
-        {
-            if (e.Payload.TryGetValue("extension", out var ext))
-                return ext.ToString() == ".unity";
-            return false;
-        }
+        private static bool IsScene(EditorEvent e)
+        {
+            return HasExtension(e, ".unity");
+        }
@@
-        private static bool IsPrefab(EditorEvent e)
-        {
-            if (e.Payload.TryGetValue("extension", out var ext))
-                return ext.ToString() == ".prefab";
-            return false;
-        }
+        private static bool IsPrefab(EditorEvent e)
+        {
+            return HasExtension(e, ".prefab");
+        }
@@
-        private static bool IsTexture(EditorEvent e)
-        {
-            if (e.Payload.TryGetValue("extension", out var ext))
-            {
-                var extStr = ext.ToString();
-                return extStr == ".png" || extStr == ".jpg" || extStr == ".jpeg" ||
-                       extStr == ".psd" || extStr == ".tga" || extStr == ".exr";
-            }
-            if (e.Payload.TryGetValue("type", out var type))
-                return type.ToString()?.Contains("Texture") == true;
-            return false;
-        }
+        private static bool IsTexture(EditorEvent e)
+        {
+            if (HasExtension(e, ".png", ".jpg", ".jpeg", ".psd", ".tga", ".exr"))
+                return true;
+            if (e.Payload.TryGetValue("type", out var type))
+                return type.ToString()?.Contains("Texture") == true;
+            return false;
+        }
@@
-        private static bool IsAudio(EditorEvent e)
-        {
-            if (e.Payload.TryGetValue("extension", out var ext))
-            {
-                var extStr = ext.ToString();
-                return extStr == ".wav" || extStr == ".mp3" || extStr == ".ogg" ||
-                       extStr == ".aif" || extStr == ".aiff";
-            }
-            return false;
-        }
+        private static bool IsAudio(EditorEvent e)
+        {
+            if (HasExtension(e, ".wav", ".mp3", ".ogg", ".aif", ".aiff"))
+                return true;
+            return false;
+        }

MCPForUnity/Editor/ActionTrace/Capture/ActionTraceRecorder.cs-64-69 (1)

64-69: Skip recording when GameObject is null to prevent collapsing distinct deletions.

The current code records all destroyed GameObjects with null references as Instance:0, which collapses multiple distinct deletions into a single target ID. Since UnityEventHooks detects destroyed objects after they're already destroyed (passing null to the handler), adopt the same pattern used in OnGameObjectCreated: skip recording when go is null.

Suggested fix

-            // GameObject might already be destroyed
-            int instanceId = go != null ? go.GetInstanceID() : 0;
-            _actionTrace.RecordGameObjectDestroyed(instanceId);
+            if (go == null) return;
+            _actionTrace.RecordGameObjectDestroyed(go.GetInstanceID());

MCPForUnity/Editor/Tools/GetActionTraceTool.cs-20-75 (1)

20-75: Remove unused parameters or implement event type filtering.

Parameters exposes EventTypes and IncludePayload, but ActionTraceViewResource.HandleCommand has no getter functions for these fields and does not parse them. These parameters are silently ignored, breaking the tool's contract. Either implement filtering by event types and payload control, or remove these parameters.

MCPForUnity/Editor/ActionTrace/Capture/AssetChangePostprocessor.cs-135-148 (1)

135-148: Placeholder implementation with incorrect logic and empty catch.

1. AssetDatabase.LoadMainAssetAtPath(metaPath) won't work - .meta files aren't Unity assets
2. The function always returns false, so AssetCreated is never emitted
3. The empty catch swallows all exceptions silently

If distinguishing created vs imported isn't feasible now, consider removing the AssetCreated event type or using a heuristic (e.g., checking file creation timestamp via System.IO.File.GetCreationTime).

MCPForUnity/Editor/ActionTrace/Core/EventStore.cs-240-260 (1)

240-260: Clearing sequence counter may cause sequence reuse.

Setting _sequenceCounter = 0 at line 246 means new events after Clear() will reuse sequence numbers from before the clear. If any external system (e.g., an AI agent, logs, or persisted state) cached old sequence numbers for since_sequence queries, the results could be confusing or incorrect.

Consider:

1. Not resetting the counter (sequences continue incrementing)
2. Starting from DateTimeOffset.UtcNow.ToUnixTimeMilliseconds() as an epoch base

MCPForUnity/Editor/ActionTrace/Core/EventStore.Persistence.cs-78-87 (1)

78-87: Potential race condition when clearing _saveScheduled outside lock.

_saveScheduled is read and written inside _queryLock in ScheduleSave (lines 43-50, 56-58), but here at line 85 it's set to false outside the lock. This could cause a race if ScheduleSave is called concurrently.

🔧 Suggested fix

         public static void ClearPendingOperations()
         {
             lock (_pendingNotifications)
             {
                 _pendingNotifications.Clear();
                 _notifyScheduled = false;
             }
-            _saveScheduled = false;
+            lock (_queryLock)
+            {
+                _saveScheduled = false;
+            }
             _lastDehydratedCount = -1;  // Reset dehydration optimization marker
         }

MCPForUnity/Editor/Windows/ActionTraceEditorWindow.cs-250-251 (1)

250-251: Menu separator creates a clickable empty item.

AppendAction("/", ...) creates an action labeled "/" rather than a visual separator. Unity's DropdownMenu uses AppendSeparator() for separators.

🔧 Suggested fix

             _filterMenu?.menu.AppendAction("All", a => SetImportance(0f));
-            _filterMenu?.menu.AppendAction("/", a => { });
+            _filterMenu?.menu.AppendSeparator();
             _filterMenu?.menu.AppendAction("AI Can See", a => SetImportanceFromSettings());

MCPForUnity/Editor/ActionTrace/Core/EventStore.Persistence.cs-174-179 (1)

174-179: Index-based trimming of context mappings should use EventSequence filtering instead.

The code at lines 174-179 removes mappings by index position (RemoveRange(0, removeCount)), which assumes mappings are strictly ordered by EventSequence. While a cascade delete using RemoveAll(m => removedSequences.Contains(m.EventSequence)) already executes at line 168, the subsequent index-based trim is fragile and inconsistent with the safer removal pattern used elsewhere in the codebase (e.g., EventStore.cs line 144).

Replace the index-based trim with a sequence-based approach to ensure orphaned mappings are properly removed regardless of insertion order:

if (_contextMappings.Count > maxContextMappings)
{
    var mappingsToKeep = _contextMappings
        .OrderByDescending(m => m.EventSequence)
        .Take(maxContextMappings)
        .Select(m => m.EventSequence)
        .ToHashSet();
    _contextMappings.RemoveAll(m => !mappingsToKeep.Contains(m.EventSequence));
}

MCPForUnity/Editor/ActionTrace/Capture/ActionTraceEventEmitter.cs-289-306 (1)

289-306: Two EmitAssetDeleted overloads emit same event type with inconsistent payloads.

• EmitAssetDeleted(string assetPath) (Line 289): No asset_type, no source field
• EmitAssetDeleted(string assetPath, string assetType) (Line 462): Includes asset_type and source = "mcp_tool"

Consumers querying AssetDeleted events may receive inconsistent payloads. Consider:

1. Adding asset_type to the first overload (via DetectAssetType)
2. Renaming the second overload to distinguish MCP-originated deletions

🔧 Suggested fix for consistency

 public static void EmitAssetDeleted(string assetPath)
 {
     if (string.IsNullOrEmpty(assetPath))
     {
         McpLog.Warn("[ActionTraceEventEmitter] Attempted to emit AssetDeleted with null or empty path");
         return;
     }

     string targetId = $"Asset:{assetPath}";

     var payload = new Dictionary<string, object>
     {
         ["path"] = assetPath,
-        ["extension"] = System.IO.Path.GetExtension(assetPath)
+        ["extension"] = System.IO.Path.GetExtension(assetPath),
+        ["asset_type"] = DetectAssetType(assetPath)
     };

     EmitEvent(EventTypes.AssetDeleted, targetId, payload);
 }

Also applies to: 462-480

MCPForUnity/Editor/ActionTrace/Capture/ActionTraceEventEmitter.cs-514-514 (1)

514-514: Use ToLowerInvariant() instead of ToLower() for culture-invariant comparison.

ToLower() is culture-sensitive and could produce unexpected results in certain locales (e.g., Turkish "I" handling). File extensions should use invariant comparison.

🐛 Proposed fix

-var extension = System.IO.Path.GetExtension(assetPath).ToLower();
+var extension = System.IO.Path.GetExtension(assetPath).ToLowerInvariant();

Server/src/services/resources/action_trace.py-170-177 (1)

170-177: Silent exception swallowing loses debugging information.

Unlike the similar block in get_action_trace (which logs via ctx.error), this loop silently discards parsing failures. This makes debugging production issues harder.

🐛 Proposed fix to add logging

     for event_dict in events_data:
         try:
             event = EditorEvent(**event_dict)
             events.append(event)
-        except Exception:
+        except (TypeError, ValueError, KeyError) as e:
+            # Log but continue - statistics should still work with partial data
+            import logging
+            logging.warning(f"Failed to parse event for statistics: {e}")
             continue

MCPForUnity/Editor/ActionTrace/Core/EventTypes.cs-98-103 (1)

98-103: Define AINote as a public const like all other event types.

All other event types are defined as public const string (lines 31–71), but AINote is hardcoded as a string literal at line 98. The file's own checklist (lines 4–16) explicitly states step 1 is "Add event constant."

♻️ Suggested fix

Add the constant with the other event types:

 // Build events
 public const string BuildStarted = "BuildStarted";
 public const string BuildCompleted = "BuildCompleted";
 public const string BuildFailed = "BuildFailed";
+
+// AI events
+public const string AINote = "AINote";

Then update the metadata dictionary:

-["AINote"] = new EventMetadata
+[AINote] = new EventMetadata

MCPForUnity/Editor/ActionTrace/Core/ActionTraceSettings.cs-266-272 (1)

266-272: Memory estimation can produce incorrect results if HotEventCount > MaxEvents.

If a user configures HotEventCount greater than MaxEvents (which Validate() warns about but doesn't prevent), coldEvents becomes negative, resulting in an underestimated memory usage.

Clamp coldEvents to non-negative

         public long GetEstimatedMemoryUsage()
         {
             // Approximate: each event ~300 bytes when hydrated, ~100 bytes when dehydrated
             int hotEvents = Storage.HotEventCount;
-            int coldEvents = Storage.MaxEvents - Storage.HotEventCount;
+            int coldEvents = Math.Max(0, Storage.MaxEvents - Storage.HotEventCount);
             return (long)(hotEvents * 300 + coldEvents * 100);
         }

MCPForUnity/Editor/ActionTrace/Core/EditorEvent.cs-162-170 (1)

162-170: Race condition in lazy summary computation.

GetSummary() mutates PrecomputedSummary without synchronization. If called concurrently from multiple threads, this could result in redundant computation or inconsistent state reads.

While the class is described as "immutable," this lazy initialization pattern breaks that guarantee.

Consider using Lazy<T> or lock for thread-safe lazy evaluation

+        private readonly object _summaryLock = new object();
+
         public string GetSummary()
         {
             if (PrecomputedSummary != null)
                 return PrecomputedSummary;

-            // Compute and cache (this mutates the object, but it's just a string field)
-            PrecomputedSummary = ComputeSummary();
-            return PrecomputedSummary;
+            lock (_summaryLock)
+            {
+                if (PrecomputedSummary != null)
+                    return PrecomputedSummary;
+                PrecomputedSummary = ComputeSummary();
+                return PrecomputedSummary;
+            }
         }

Alternatively, use Interlocked.CompareExchange for lock-free approach if ComputeSummary() is idempotent.

[coderabbitai]

Actionable comments posted: 13

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/Analysis/Context/ContextCompressor.cs`:
- Around line 420-433: The Deduplicate extension method can divide by zero when
windowMs <= 0; update Deduplicate to validate windowMs (e.g., if windowMs <= 0)
either throw an ArgumentOutOfRangeException or normalize it to a sensible
default (like 100 or 1) before computing string key =
$"{evt.Type}|{evt.TargetId}|{evt.TimestampUnixMs / windowMs}"; ensure the check
happens at the start of Deduplicate so all subsequent uses of windowMs are safe.
- Around line 179-183: The early return in CompressWithDetails produces a
CompressedContext with null list properties causing NREs; change that return to
initialize PreservedEvents, DehydratedEvents, and SummaryEvents as empty
collections and set OriginalCount = 0 (i.e., construct a CompressedContext with
its list properties assigned empty lists) so consumers can safely iterate;
locate this in the CompressWithDetails method and update the returned
CompressedContext instance accordingly.

In `@MCPForUnity/Editor/ActionTrace/Analysis/Query/EventQueryBuilder.cs`:
- Around line 371-449: Execute() uses CalculateEffectiveLimit() heuristics that
may under-fetch events for a given _timeRange; modify Execute() to verify
coverage when _timeRange.HasValue by checking the oldest event returned (e.g.,
examine timestamps on the last item of allEvents after
EventStore.Query(effectiveLimit)) and if that event's timestamp is newer than
_timeRange.Value.StartMs, iteratively increase the queried limit (or immediately
fall back to EventStore.QueryAll()) until the oldest fetched event is
older-or-equal to StartMs or a safe max threshold is reached; update
CalculateEffectiveLimit/Execute to perform this requery loop (using
EventStore.Query and EventStore.QueryAll as fallbacks) so filtering by time is
exact instead of silently dropping earlier events.

In
`@MCPForUnity/Editor/ActionTrace/Capture/CapturePoints/EventCaptureRegistry.cs`:
- Around line 41-49: The Unregister method can leave the registry inconsistent
if point.Shutdown() throws; wrap the Shutdown() call in a try/catch (or
try/finally) so that _capturePoints.Remove(point) always runs even on error, and
log the exception (e.g., via UnityEngine.Debug.LogException or your project's
logger) including the capturePointId and context; ensure the method still
returns true for successful removal and avoid swallowing errors silently
(rethrow only if caller must know), referencing Unregister, _capturePoints,
Shutdown(), and _isInitialized to locate and update the code.

In `@MCPForUnity/Editor/ActionTrace/Capture/Capturers/AssetCapture.cs`:
- Around line 344-347: VcsContextProvider.GetCurrentContext() can return null
but the code calls ToDictionary() unconditionally; update the block around
VcsContextProvider.GetCurrentContext(), payload["vcs_context"], and
ToDictionary() to guard against null by checking the returned context and only
calling ToDictionary() when non-null (or assign a safe default such as null or
an empty dictionary into payload["vcs_context"] if the context is null) so no
NullReferenceException is possible.

In `@MCPForUnity/Editor/ActionTrace/Capture/Capturers/UndoCapture.cs`:
- Around line 51-69: Guard against nested BeginToolCall by checking the
_isInToolCall flag at the start of BeginToolCall; if a tool call is already
active, log a warning (including _currentToolName and _currentToolCallId),
safely close the existing group by invoking EndToolCall (so
_currentUndoGroupStart is collapsed correctly) or otherwise finalize the prior
undo group before proceeding, then create the new undo group via
Undo.SetCurrentGroupName and Undo.GetCurrentGroup and update
_currentToolName/_currentToolCallId/_isInToolCall; ensure no silent overwrites
of _currentUndoGroupStart occur.

In `@MCPForUnity/Editor/ActionTrace/Capture/Recorder.cs`:
- Around line 28-47: The static constructor of ActionTraceRecorder subscribes
many HookRegistry events but never unsubscribes, causing stale delegates across
domain reloads; add a static cleanup handler that unsubscribes all the same
HookRegistry events (the ones added in the static constructor: OnComponentAdded,
OnComponentRemoved, OnComponentRemovedDetailed, OnGameObjectCreated,
OnGameObjectDestroyedDetailed, OnSelectionChanged, OnHierarchyChanged,
OnPlayModeChanged, OnSceneSaved, OnSceneOpenedDetailed,
OnNewSceneCreatedDetailed, OnScriptCompiledDetailed,
OnScriptCompilationFailedDetailed, OnBuildCompletedDetailed) and register that
cleanup to Unity's domain-reload event (e.g.,
AssemblyReloadEvents.beforeAssemblyReload) from the static constructor so
UnsubscribeFromHookRegistry is invoked on reload and prevents stale delegates.

In `@MCPForUnity/Editor/ActionTrace/Capture/Sampling/SamplingMiddleware.cs`:
- Around line 124-133: In SamplingMiddleware (the asset-event handling block),
guard against a null evt.Payload before calling evt.Payload.TryGetValue to avoid
throwing on asset events; update the logic that sets assetPath (currently using
evt.TargetId and evt.Payload.TryGetValue("path", ...)) to first check
evt.Payload != null (or use a null-conditional) and only call TryGetValue when
payload exists, leaving assetPath as TargetId or null otherwise so the sampling
pipeline doesn't hard-crash on asset events.
- Around line 257-266: The CleanupExpiredSamples function currently uses a fixed
CleanupAgeMs to drop pending entries, which can remove samples before a
strategy's WindowMs has elapsed; change the expiration logic in
CleanupExpiredSamples to compute each sample's expiry using its strategy's
configured WindowMs (from the sample/strategy metadata stored in
_pendingSamples) and only remove when nowMs - kvp.Value.TimestampMs >
kvp.Value.Strategy.WindowMs (or the strategy-specific max), and when removing
due to window expiry ensure you record/emit the expired debounce sample (so
debounce handlers see the late event) rather than silently TryRemove; update
references to CleanupAgeMs so it becomes a fallback or max cap if needed.

In `@MCPForUnity/Editor/ActionTrace/Context/ToolCallScope.cs`:
- Around line 494-503: ExecuteAsync is calling func() directly so if func()
throws synchronously the ToolCallScope (variable scope) is never completed or
disposed; wrap the func() invocation in a try/catch that catches synchronous
exceptions, call the appropriate scope cleanup/completion (e.g.,
scope.Complete/fail and scope.Dispose) before rethrowing the exception, and keep
the existing continuation logic that completes/disposes the scope when the
returned Task finishes; refer to ExecuteAsync<T>, the local variable scope and
the task = func() call to locate where to add the try/catch and cleanup.

In `@MCPForUnity/Editor/ActionTrace/Core/Store/EventStore.Diagnostics.cs`:
- Around line 17-29: DehydrateOldEvents mutates the shared _events list without
synchronization; wrap the body (the for-loop that reads and assigns _events[i])
in a lock on _queryLock to prevent races with readers like GetMemoryDiagnostics,
i.e. acquire _queryLock before iterating and releasing after, so every access
that mutates or reads _events is protected.

In `@MCPForUnity/Editor/Tools/UndoToSequenceTool.cs`:
- Around line 80-96: The code currently only compares targetSequence to
EventStore.CurrentSequence and fails to detect if the target is older than the
oldest retained event; call the EventStore API that exposes the oldest retained
sequence (e.g., EventStore.OldestSequence or EventStore.GetOldestSequence())
before querying, and if targetSequence < oldest return an ErrorResponse stating
the target has been trimmed and include the oldest and current sequences; then
proceed to call EventStore.Query (as in the existing code) only when
targetSequence is within the stored window to avoid under‑counting undo steps.

In `@Server/src/models/action_trace.py`:
- Around line 112-124: The TransactionInfo model's triggered_by_tool field is
typed incorrectly as Optional[bool]; change its type to Optional[str] and update
the Field description accordingly so it matches the C#
AtomicOperation.TriggeredByTool (which contains the tool name string, e.g.,
"manage_gameobject"); modify the TransactionInfo class's triggered_by_tool
declaration to accept a string (Optional[str]) and ensure any
serialization/deserialization expectations reflect a string value rather than a
boolean.

♻️ Duplicate comments (1)

MCPForUnity/Editor/ActionTrace/Helpers/GameObjectTrackingHelper.cs (1)

28-55: Main thread ID capture issue has been properly addressed.

The lazy initialization pattern with [InitializeOnLoadMethod] correctly ensures the main thread ID is captured during Unity's editor initialization rather than during static type initialization. This resolves the previous concern about incorrect thread ID capture.

🟡 Minor comments (18)

MCPForUnity/Editor/Tools/ManageAsset.cs-292-294 (1)

292-294: Use actual created asset type for OnAssetCreated to keep payloads consistent.

OnAssetCreated currently emits the user‑supplied assetType, while modify/delete emit the runtime type. This can produce inconsistent payloads for consumers.

🛠️ Suggested fix

-                OnAssetCreated?.Invoke(fullPath, assetType);
+                var createdType =
+                    newAsset != null
+                        ? newAsset.GetType().FullName
+                        : AssetDatabase.GetMainAssetTypeAtPath(fullPath)?.FullName ?? assetType;
+                OnAssetCreated?.Invoke(fullPath, createdType);

Also applies to: 498-503, 539-548

MCPForUnity/Editor/ActionTrace/Capture/CapturePoints/EventCapturePointAttribute.cs-16-20 (1)

16-20: Validate capture point Id to avoid null/empty registry keys.

If a capture point is declared with a null/whitespace id, discovery/registration can fail later (e.g., dictionary key errors). Guard early and normalize the description.

🛠️ Suggested fix

 public EventCapturePointAttribute(string id, string description = null, int priority = 0)
 {
+    if (string.IsNullOrWhiteSpace(id))
+        throw new ArgumentException("Capture point id must be non-empty.", nameof(id));
     Id = id;
-    Description = description ?? id;
+    Description = string.IsNullOrWhiteSpace(description) ? id : description;
     Priority = priority;
 }

MCPForUnity/Editor/ActionTrace/Integration/VCS/VcsContextProvider.cs-149-157 (1)

149-157: Detect both .git directory and file for worktree/submodule support.

Git worktrees store .git as a file pointing to the worktree's metadata, not as a directory. The current check using only Directory.Exists() causes false negatives, disabling VCS context for worktrees.

🛠️ Suggested fix

-                return System.IO.Directory.Exists(gitPath);
+                return System.IO.Directory.Exists(gitPath) || System.IO.File.Exists(gitPath);

MCPForUnity/Editor/ActionTrace/Core/Settings/ActionTraceSettings.cs-267-273 (1)

267-273: Potential negative value when HotEventCount > MaxEvents.

If HotEventCount exceeds MaxEvents (an invalid state that Validate() flags but doesn't prevent), coldEvents becomes negative, producing an incorrect memory estimate.

🔧 Proposed fix: Clamp to zero

 public long GetEstimatedMemoryUsage()
 {
     // Approximate: each event ~300 bytes when hydrated, ~100 bytes when dehydrated
     int hotEvents = Storage.HotEventCount;
-    int coldEvents = Storage.MaxEvents - Storage.HotEventCount;
+    int coldEvents = Math.Max(0, Storage.MaxEvents - Storage.HotEventCount);
     return (long)(hotEvents * 300 + coldEvents * 100);
 }

MCPForUnity/Editor/ActionTrace/Capture/Filters/EventFilter.cs-123-148 (1)

123-148: DefaultRules contains mutable FilterRule instances.

DefaultRules is static readonly, but the FilterRule objects within are mutable. If any code modifies a rule (e.g., DefaultRules[0].Enabled = false), that change persists globally. Consider making FilterRule immutable or creating defensive copies in GetActiveRules().

🔧 Option: Create copies in GetActiveRules

 if (EnableDefaultFilters)
 {
     foreach (var rule in DefaultRules)
     {
         if (rule.Enabled)
-            rules.Add(rule);
+            rules.Add(new FilterRule
+            {
+                Name = rule.Name,
+                Enabled = rule.Enabled,
+                Type = rule.Type,
+                Pattern = rule.Pattern,
+                Action = rule.Action,
+                Priority = rule.Priority
+            });
     }
 }

Note: This adds allocation overhead. Alternatively, document that DefaultRules should not be modified.

MCPForUnity/Editor/ActionTrace/Core/Settings/ActionTraceSettings.cs-207-217 (1)

207-217: ApplyPreset is called before the asset exists on disk, causing premature Save() calls.

CreateSettings calls ApplyPreset (line 211) before CreateAsset (line 212). Since ApplyPreset invokes MarkDirty() and Save(), EditorUtility.SetDirty(this) and AssetDatabase.SaveAssets() are called on an asset that hasn't been created yet, which may silently fail or cause unexpected behavior.

🔧 Proposed fix: Reorder to create asset first

 private static ActionTraceSettings CreateSettings()
 {
     var settings = CreateInstance<ActionTraceSettings>();
-    // Apply Standard preset by default
-    settings.ApplyPreset(ActionTracePreset.Standard);
     AssetDatabase.CreateAsset(settings, SettingsPath);
+    // Apply Standard preset by default (after asset exists)
+    settings.ApplyPreset(ActionTracePreset.Standard);
     AssetDatabase.SaveAssets();
     AssetDatabase.Refresh();
     McpLog.Info($"[ActionTraceSettings] Created new settings at {SettingsPath}");
     return settings;
 }

MCPForUnity/Editor/ActionTrace/Core/EventTypes.cs-98-103 (1)

98-103: Missing constant for "AINote" event type.

The "AINote" string literal is used directly in the metadata dictionary (line 98), but unlike all other event types, it lacks a corresponding public const string declaration. This breaks the type-safe pattern established for the rest of the file.

🔧 Proposed fix: Add the constant

Add with the other event type constants (around line 70):

 // Build events
 public const string BuildStarted = "BuildStarted";
 public const string BuildCompleted = "BuildCompleted";
 public const string BuildFailed = "BuildFailed";
+
+// AI events
+public const string AINote = "AINote";

Then update the metadata dictionary entry:

-["AINote"] = new EventMetadata
+[AINote] = new EventMetadata

MCPForUnity/Editor/ActionTrace/Analysis/Query/EventQueryBuilder.cs-312-327 (1)

312-327: Guard against null/empty search terms.
Line 314+: a null term will cause term.ToLowerInvariant() to throw in MatchesFilters. Accepting empty strings also bloats the set without adding value.

🐛 Proposed fix

 public EventQueryBuilder WithSearchTerm(string term)
 {
+    if (string.IsNullOrWhiteSpace(term)) return this;
     _searchTerms ??= new HashSet<string>(StringComparer.OrdinalIgnoreCase);
     _searchTerms.Add(term);
     return this;
 }

 public EventQueryBuilder WithAllSearchTerms(params string[] terms)
 {
     _searchTerms ??= new HashSet<string>(StringComparer.OrdinalIgnoreCase);
-    foreach (string term in terms)
-        _searchTerms.Add(term);
+    foreach (string term in terms)
+    {
+        if (string.IsNullOrWhiteSpace(term)) continue;
+        _searchTerms.Add(term);
+    }
     return this;
 }

MCPForUnity/Editor/ActionTrace/Capture/Capturers/AssetCapture.cs-181-232 (1)

181-232: Cache persistence skipped for deleted and moved assets.

The hasChanges flag is only set when processing imported assets (line 142). When a batch contains only deleted or moved assets, the cache entries are added but SaveProcessedAssets() is never called (line 231-232), so changes won't persist across domain reloads.

Suggested fix

         // ========== Deleted Assets ==========
         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;
+
             // L1 Blacklist: Skip junk assets
             if (!EventFilter.ShouldTrackAsset(assetPath))
                 continue;

Apply similarly to the moved assets section (after line 210-211).

MCPForUnity/Editor/ActionTrace/Capture/Capturers/SelectionCapture.cs-139-170 (1)

139-170: Missing VCS context and undo_group injection.

Other capture points (e.g., Recorder.RecordEvent and AssetChangePostprocessor.RecordEvent) inject vcs_context and undo_group into the payload for consistency and to support the undo_to_sequence functionality. This event is missing both.

Suggested fix

         private static void RecordSelectionPropertyModified(UndoPropertyModification undoMod, UnityEngine.Object target, string targetGlobalId, string propertyPath)
         {
             var currentValue = UndoReflectionHelper.GetCurrentValue(undoMod);
             var prevValue = UndoReflectionHelper.GetPreviousValue(undoMod);

             var payload = new Dictionary<string, object>
             {
                 ["target_name"] = target.name,
                 ["component_type"] = target.GetType().Name,
                 ["property_path"] = propertyPath,
                 ["start_value"] = PropertyFormatter.FormatPropertyValue(prevValue),
                 ["end_value"] = PropertyFormatter.FormatPropertyValue(currentValue),
                 ["value_type"] = PropertyFormatter.GetPropertyTypeName(currentValue),
                 ["selection_context"] = new Dictionary<string, object>
                 {
                     ["selection_id"] = _currentSelectionGlobalId,
                     ["selection_name"] = _currentSelectionName,
                     ["selection_type"] = _currentSelectionType,
                     ["selection_path"] = _currentSelectionPath ?? string.Empty
-                }
+                },
+                ["undo_group"] = Undo.GetCurrentGroup()
             };

+            // Inject VCS context if available
+            var vcsContext = MCPForUnity.Editor.ActionTrace.Integration.VCS.VcsContextProvider.GetCurrentContext();
+            if (vcsContext != null)
+            {
+                payload["vcs_context"] = vcsContext.ToDictionary();
+            }
+
             var evt = new EditorEvent(

MCPForUnity/Editor/ActionTrace/Core/Store/EventStore.Persistence.cs-93-102 (1)

93-102: Inconsistent locking for _saveScheduled.

_saveScheduled is reset on line 100 outside any lock, but it's set on line 62 inside _queryLock. This inconsistency could lead to race conditions between ScheduleSave() and ClearPendingOperations().

🔧 Proposed fix

 public static void ClearPendingOperations()
 {
     lock (_pendingNotifications)
     {
         _pendingNotifications.Clear();
         _notifyScheduled = false;
     }
+    lock (_queryLock)
+    {
-    _saveScheduled = false;
+        _saveScheduled = false;
+    }
     _lastDehydratedCount = -1;  // Reset dehydration optimization marker
 }

MCPForUnity/Editor/ActionTrace/Capture/Recorder.cs-163-172 (1)

163-172: Semantic mismatch: ExitingPlayMode used for isPlaying=false.

When isPlaying is false, the state is set to ExitingPlayMode, but according to UnityEventHooks.OnPlayModeStateChanged, NotifyPlayModeChanged(false) is only called when the state is EnteredEditMode. This creates a semantic inconsistency between what the recorder logs and what actually happened.

🐛 Proposed fix

 private static void OnPlayModeChanged(bool isPlaying)
 {
-    var state = isPlaying ? PlayModeStateChange.EnteredPlayMode : PlayModeStateChange.ExitingPlayMode;
+    var state = isPlaying ? PlayModeStateChange.EnteredPlayMode : PlayModeStateChange.EnteredEditMode;
     var payload = new Dictionary<string, object>
     {
         ["state"] = state.ToString()
     };

     RecordEvent("PlayModeChanged", "Editor", payload);
 }

MCPForUnity/Editor/ActionTrace/Core/Store/EventStore.Diagnostics.cs-51-60 (1)

51-60: Missing null check on evt before accessing IsDehydrated.

The loop iterates over _events and accesses evt.IsDehydrated without a null check. If _events can contain null entries (as suggested by the null check in DehydrateOldEvents), this will throw a NullReferenceException.

🐛 Proposed fix

 foreach (var evt in _events)
 {
+    if (evt == null) continue;
+
     if (evt.IsDehydrated)
         dehydratedCount++;
     else if (evt.Payload != null)
     {
         hydratedCount++;
         estimatedPayloadBytes += EstimatePayloadSize(evt.Payload);
     }
 }

MCPForUnity/Editor/ActionTrace/Core/Store/EventStore.Persistence.cs-45-87 (1)

45-87: Throttle check has potential race condition.

The throttle check on lines 48-53 reads _lastSaveTime without holding the lock, but _lastSaveTime is updated on line 77 inside the lock. This could lead to missed saves if two threads check throttling simultaneously.

Consider moving the throttle check inside the lock, or use Interlocked for atomic access.

🔧 Proposed fix

 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)
     {
+        // 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;
+        }
+
         // Only schedule if not already scheduled (prevents callback queue bloat)
         if (_saveScheduled)
             return;

         _saveScheduled = true;
     }

MCPForUnity/Editor/ActionTrace/Capture/CapturePoints/CapturePointStats.cs-20-31 (1)

20-31: Guard EndCapture when StartCapture wasn’t called (and reset timing state).

If EndCapture() is invoked before StartCapture() (or after Reset()), _startTimeTicks is 0 and elapsed time becomes enormous, skewing stats. A simple guard + reset avoids that.

🐛 Proposed fix

 public void EndCapture()
 {
+    if (_startTimeTicks == 0)
+        return;
+
     long elapsedTicks = DateTimeOffset.UtcNow.Ticks - _startTimeTicks;
     TotalCaptureTimeMs += elapsedTicks / 10000;
     TotalEventsCaptured++;
+    _startTimeTicks = 0;
     UpdateAverage();
 }
@@
 public void Reset()
 {
     TotalEventsCaptured = 0;
     EventsFiltered = 0;
     EventsSampled = 0;
     TotalCaptureTimeMs = 0;
     AverageCaptureTimeMs = 0;
     ErrorCount = 0;
+    _startTimeTicks = 0;
 }

Also applies to: 55-63

MCPForUnity/Editor/Windows/ActionTraceEditorWindow.cs-258-261 (1)

258-261: Use a real menu separator instead of a "/" action.

The "/" character in AppendAction() is interpreted as a menu path separator for nested submenus, not as a separator indicator. Use AppendSeparator() to properly add a menu separator and avoid unintended UI behavior.

Proposed fix

-            _filterMenu?.menu.AppendAction("/", a => { });
+            _filterMenu?.menu.AppendSeparator();

MCPForUnity/Editor/ActionTrace/Core/Store/EventStore.cs-94-101 (1)

94-101: Importance threshold should be inclusive.
Using <= drops events that match the configured minimum. Query‑side filtering is inclusive, so this is inconsistent.

🛠️ Proposed fix

-                if (importance <= settings.Filtering.MinImportanceForRecording)
+                if (importance < settings.Filtering.MinImportanceForRecording)

MCPForUnity/Editor/ActionTrace/Analysis/Context/ContextCompressor.cs-226-235 (1)

226-235: Guard against non-positive count in summaries.

A negative count produces a misleading negative summary and skips the loop silently.

🔧 Proposed fix

-            if (events == null || startIdx < 0 || startIdx >= events.Count)
+            if (events == null || startIdx < 0 || startIdx >= events.Count || count <= 0)
                 return string.Empty;

🧹 Nitpick comments (28)

MCPForUnity/Editor/ActionTrace/Capture/Capturers/UndoCapture.cs (1)

164-185: Avoid empty Undo labels when note is blank.
If note is null/whitespace, the Undo history entry becomes "AI: " which is not very helpful. A small fallback makes the history clearer.

♻️ Suggested tweak

 public static string HandleActionTraceNote(string note, bool isTransactionEnd)
 {
     string groupName;
+    var safeNote = string.IsNullOrWhiteSpace(note) ? (_currentToolName ?? "AI Operation") : note;
 
     if (_isInToolCall)
     {
         // Use the AI note as the final Undo group name
-        groupName = $"AI: {note}";
+        groupName = $"AI: {safeNote}";
         Undo.SetCurrentGroupName(groupName);
 
         if (isTransactionEnd)
         {
             EndToolCall();
         }
 
         return groupName;
     }
 
     // Not in a tool call - just set the Undo name
-    groupName = $"AI: {note}";
+    groupName = $"AI: {safeNote}";
     Undo.SetCurrentGroupName(groupName);
     return groupName;
 }

MCPForUnity/Editor/ActionTrace/Integration/VCS/VcsContextProvider.cs (1)

44-81: Clarify the thread‑safety guarantee for _currentContext.

GetCurrentContext is documented as thread‑safe, but reads/writes have no memory barriers. If background event recording is expected, make the intent explicit with volatile/Volatile.Read or drop the guarantee.

🛠️ Suggested fix

-        private static VcsContext _currentContext;
+        private static volatile VcsContext _currentContext;
@@
         public static VcsContext GetCurrentContext()
         {
-            if (_currentContext == null)
+            var current = _currentContext;
+            if (current == null)
             {
-                _currentContext = GetInitialContext();
+                current = GetInitialContext();
+                _currentContext = current;
             }
 
-            return _currentContext;
+            return current;
         }

MCPForUnity/Editor/ActionTrace/Metadata/Descriptors/IEventDescriptor.cs (1)

62-88: Avoid duplicated target‑name resolution logic.

GetTargetName mirrors the logic in EventSummarizer.GetTargetName. Consider extracting a shared helper to prevent drift if the key list changes.

MCPForUnity/Editor/ActionTrace/Core/Settings/ActionTraceSettings.Editor.cs (1)

65-75: Unused ChangeCheckScope wrapper.

The ChangeCheckScope on line 65 is created but its changed property is never read. The manual newIndex != currentIndex check makes the scope redundant.

♻️ Optional simplification

-                using (new EditorGUI.ChangeCheckScope())
-                {
                     var presetNames = ActionTracePreset.AllPresets.ConvertAll(p => p.Name);
                     var currentIndex = presetNames.FindIndex(n => n == settings.CurrentPresetName);
                     var newIndex = EditorGUILayout.Popup(currentIndex, presetNames.ToArray());

                     if (newIndex != currentIndex && newIndex >= 0)
                     {
                         settings.ApplyPreset(ActionTracePreset.AllPresets[newIndex]);
                     }
-                }

MCPForUnity/Editor/ActionTrace/Core/EventTypes.cs (1)

112-117: BuildCompleted importance (1.0f) doesn't match section comment.

BuildCompleted has DefaultImportance = 1.0f which qualifies as "Critical", but it's placed under the "High (0.7-0.9)" section comment. Consider moving it to the Critical section for consistency.

MCPForUnity/Editor/ActionTrace/Capture/Capturers/PropertyCapture.cs (1)

92-158: Inconsistent indentation in ProcessModifications.

The foreach loop body (lines 93-155) has inconsistent indentation - it appears to be indented by 8 spaces instead of the expected 12 (3 levels within the lock). The return modifications; on line 157 is correctly inside the lock but visually appears at an unexpected level.

♻️ Fix indentation

The foreach block content starting at line 93 should be indented one more level to align with being inside both lock (_lock) and foreach (var undoMod ...).

MCPForUnity/Editor/ActionTrace/Capture/Filters/EventFilter.cs (2)

139-140: Thumbs.db rule won't match correctly with Extension type.

RuleType.Extension uses EndsWith, so ".DS_Store" will match paths ending in .DS_Store, but "Thumbs.db" won't match "path/to/Thumbs.db" since the path doesn't end with Thumbs.db (it does, but the pattern lacks the leading dot that extensions typically have).

Actually, "path/to/Thumbs.db".EndsWith("Thumbs.db") is true, so this works. However, semantically these are filenames, not extensions. Consider using RuleType.Regex with @"(^|/)Thumbs\.db$" for clarity, or document that Extension also works for filename matching.

---

44-47: Silent exception swallowing on invalid regex patterns.

When regex compilation fails, the exception is caught and ignored. Consider logging a warning so users can diagnose why their custom regex rules aren't matching.

♻️ Proposed enhancement

 catch
 {
-    // Invalid regex, return null
+    // Invalid regex - log warning for diagnostics
+    UnityEngine.Debug.LogWarning($"[EventFilter] Invalid regex pattern in rule '{Name}': {Pattern}");
 }

MCPForUnity/Editor/ActionTrace/Capture/CapturePoints/EventCaptureRegistry.cs (1)

28-36: Consider guarding duplicate IDs and late registration.
Line 32 onward: duplicate CapturePointId values make SetEnabled/Unregister ambiguous, and registering after InitializeAll() leaves the new point uninitialized. A small guard keeps the registry deterministic.

♻️ Suggested update (duplicate guard + late-init)

 public void Register(IEventCapturePoint capturePoint)
 {
     if (capturePoint == null) return;
+    if (_capturePoints.Exists(p => p.CapturePointId == capturePoint.CapturePointId))
+    {
+        Debug.LogWarning($"[EventCaptureRegistry] Duplicate capture point id: {capturePoint.CapturePointId}");
+        return;
+    }

     _capturePoints.Add(capturePoint);

     // Sort by priority
     _capturePoints.Sort((a, b) => b.InitializationPriority.CompareTo(a.InitializationPriority));
+
+    if (_isInitialized)
+    {
+        try { capturePoint.Initialize(); }
+        catch (Exception ex)
+        {
+            Debug.LogError($"[EventCaptureRegistry] Failed to initialize {capturePoint.CapturePointId}: {ex.Message}");
+        }
+    }
 }

MCPForUnity/Editor/Hooks/HookRegistry.cs (1)

102-450: Optional: factor out the repeated subscriber-invocation pattern.
Line 104+ repeats the same try/catch and logging logic across many methods. A small helper (e.g., InvokeSafely<T>(Action<T>, T)) would reduce duplication and keep logging consistent.

MCPForUnity/Editor/ActionTrace/Capture/Sampling/PendingSample.cs (1)

8-18: Optional: make the struct immutable.
If mutation isn’t required, consider a readonly struct with readonly fields or properties to avoid accidental copies and improve clarity.

MCPForUnity/Editor/ActionTrace/Capture/Sampling/SamplingStrategy.cs (1)

3-4: Namespace doesn't match folder structure.

The file is located in Capture/Sampling/ but the namespace is MCPForUnity.Editor.ActionTrace.Capture. Consider aligning the namespace with the folder path for consistency.

Suggested fix

-namespace MCPForUnity.Editor.ActionTrace.Capture
+namespace MCPForUnity.Editor.ActionTrace.Capture.Sampling

MCPForUnity/Editor/ActionTrace/Semantics/DefaultEventScorer.cs (2)

45-47: Redundant special case for AINote.

The AINote event type is already configured with DefaultImportance = 1.0f in EventTypes.Metadata (per the relevant context). This hardcoded check duplicates that configuration and creates a maintenance burden—if the metadata changes, this won't update accordingly.

Remove redundant check

            // Get base score from metadata
            var meta = EventTypes.Metadata.Get(evt.Type);
            float baseScore = meta.DefaultImportance;

-           // Special case: AINote is always critical
-           if (evt.Type == "AINote")
-               return 1.0f;
-
            // Apply payload-based adjustments for asset events
            float adjustment = GetPayloadAdjustment(evt);
            return Mathf.Clamp01(baseScore + adjustment);

---

58-68: Redundant null check.

The Score() method already returns early when evt.Payload == null (line 38-39), so GetPayloadAdjustment() will never receive an event with a null payload. The null check at line 60-61 is unreachable.

Remove redundant check

        private static float GetPayloadAdjustment(EditorEvent evt)
        {
-           if (evt.Payload == null)
-               return 0f;
-
            // Asset type adjustments (only for AssetCreated/AssetImported)
            bool isAssetEvent = evt.Type == EventTypes.AssetCreated ||
                               evt.Type == EventTypes.AssetImported;

MCPForUnity/Editor/ActionTrace/Capture/Capturers/AssetCapture.cs (2)

296-305: Use ToLowerInvariant() for consistent cross-locale behavior.

ToLower() uses the current culture, which can produce unexpected results for certain characters (e.g., Turkish 'I'). Use ToLowerInvariant() for file extension comparisons.

Suggested fix

         private static bool ShouldTrackModification(string assetPath)
         {
-            string ext = System.IO.Path.GetExtension(assetPath).ToLower();
+            string ext = System.IO.Path.GetExtension(assetPath).ToLowerInvariant();
             // Track modifications for these asset types

Also apply this change to GetAssetType() at line 312.

---

284-290: Silent exception swallowing hinders debugging.

Catching all exceptions without logging makes it difficult to diagnose issues with the .meta file heuristic.

Suggested fix

             catch
+            catch (Exception ex)
             {
                 // On any error, default to treating as imported (conservative)
+                McpLog.Debug($"[AssetChangePostprocessor] IsNewlyCreatedAsset check failed for {assetPath}: {ex.Message}");
                 return false;
             }

MCPForUnity/Editor/ActionTrace/Sources/Unity/UnityEventHooks.cs (3)

40-42: Consider naming the lock more specifically.

_lock is only used for hierarchy change debouncing. A more descriptive name like _hierarchyDebounceLock would clarify its purpose.

---

92-94: Potential for multiple InitializeTracking calls.

If SubscribeToUnityEvents is somehow invoked multiple times before the first delayCall executes, multiple InitializeTracking calls could be queued. While the _isInitialized guard should prevent re-subscription, consider adding a guard inside InitializeTracking as well.

---

194-205: Missing handling for ExitingPlayMode and ExitingEditMode states.

PlayModeStateChange has four states, but only EnteredPlayMode and EnteredEditMode are handled. If downstream systems need to react to the exiting phases (e.g., for cleanup or preparation), those cases should be added.

♻️ Proposed enhancement to handle all states

 private static void OnPlayModeStateChanged(PlayModeStateChange state)
 {
     switch (state)
     {
         case PlayModeStateChange.EnteredPlayMode:
             HookRegistry.NotifyPlayModeChanged(true);
             break;
         case PlayModeStateChange.EnteredEditMode:
             HookRegistry.NotifyPlayModeChanged(false);
             break;
+        case PlayModeStateChange.ExitingEditMode:
+            // Optionally notify subscribers about exiting edit mode
+            break;
+        case PlayModeStateChange.ExitingPlayMode:
+            // Optionally notify subscribers about exiting play mode
+            break;
     }
 }

MCPForUnity/Editor/ActionTrace/Capture/Recorder.cs (1)

312-326: Consider using StringBuilder for deeply nested hierarchies.

GetGameObjectPath builds paths via repeated string concatenation in a while loop. For deeply nested hierarchies, this creates many intermediate string allocations. If performance becomes a concern, consider using StringBuilder.

MCPForUnity/Editor/ActionTrace/Capture/Sampling/SamplingConfig.cs (1)

51-54: Consider validating windowMs parameter.

SetStrategy accepts any long value for windowMs, including negative values. Consider adding validation to ensure windowMs > 0 when mode is Throttle or Debounce.

♻️ Proposed validation

 public static void SetStrategy(string eventType, SamplingMode mode, long windowMs = 1000)
 {
+    if (mode != SamplingMode.None && windowMs <= 0)
+        throw new ArgumentOutOfRangeException(nameof(windowMs), "Window must be positive when sampling is enabled");
     Strategies[eventType] = new SamplingStrategy(mode, windowMs);
 }

MCPForUnity/Editor/ActionTrace/Core/Store/EventStore.Diagnostics.cs (1)

82-112: EstimatePayloadSize doesn't recurse into nested dictionaries.

For nested IDictionary<string, object> values, the estimation uses a flat dict.Count * 100 approximation instead of recursively calling EstimatePayloadSize. This may underestimate memory for deeply nested payloads.

If precision matters, consider recursive estimation.

MCPForUnity/Editor/ActionTrace/Core/Store/EventStore.Persistence.cs (1)

184-186: Log message slightly misleading.

The log states "now {maxEvents}" but _events.Count after RemoveRange equals maxEvents only if exactly removeCount items were removed. The message is technically correct but could be clearer by using _events.Count directly.

♻️ Suggested improvement

-                    McpLog.Info($"[EventStore] Trimmed {removeCount} old events " +
-                        $"(was {_events.Count + removeCount}, now {maxEvents}, hard limit was {hardLimit})");
+                    McpLog.Info($"[EventStore] Trimmed {removeCount} old events " +
+                        $"(was {_events.Count + removeCount}, now {_events.Count}, hard limit was {hardLimit})");

MCPForUnity/Editor/Resources/ActionTrace/ActionTraceViewResource.cs (1)

188-227: Confirm aggregation input ordering.
EventStore.Query returns newest‑first. If TransactionAggregator.Aggregate expects chronological input, sort by Sequence before aggregating.

♻️ Suggested adjustment (if chronological order is required)

-            var operations = TransactionAggregator.Aggregate(filteredEvents);
+            var orderedEvents = filteredEvents.OrderBy(e => e.Sequence).ToList();
+            var operations = TransactionAggregator.Aggregate(orderedEvents);

MCPForUnity/Editor/ActionTrace/Core/Store/EventStore.cs (1)

332-349: Shield batch notifications from subscriber exceptions.
One faulty subscriber can interrupt the batch. Consider guarding each invoke, mirroring HookRegistry’s pattern.

♻️ Suggested hardening

-            foreach (var evt in toNotify)
-            {
-                EventRecorded?.Invoke(evt);
-            }
+            foreach (var evt in toNotify)
+            {
+                try
+                {
+                    EventRecorded?.Invoke(evt);
+                }
+                catch (Exception ex)
+                {
+                    McpLog.Warn($"[EventStore] EventRecorded subscriber threw exception: {ex.Message}");
+                }
+            }

+using MCPForUnity.Editor.Helpers;

MCPForUnity/Editor/ActionTrace/Sources/Unity/UnityEventHooks.Advanced.cs (1)

114-136: Ensure build telemetry runs even when BuildPlayer throws.
If BuildPipeline.BuildPlayer throws, _currentBuildPlatform isn’t reset and no failure event is emitted. A try/catch/finally keeps telemetry consistent.

🛠️ Suggested hardening

-            _buildStartTime = DateTime.UtcNow;
-            _currentBuildPlatform = GetBuildTargetName(options.target);
-
-            BuildReport result = BuildPipeline.BuildPlayer(options);
-
-            var duration = DateTime.UtcNow - _buildStartTime;
-            bool success = result.summary.result == BuildResult.Succeeded;
-
-            HookRegistry.NotifyBuildCompleted(success);
-            HookRegistry.NotifyBuildCompletedDetailed(new BuildArgs
-            {
-                Platform = _currentBuildPlatform,
-                Location = options.locationPathName,
-                DurationMs = (long)duration.TotalMilliseconds,
-                SizeBytes = success ? result.summary.totalSize : null,
-                Success = success,
-                Summary = success ? null : result.summary.ToString()
-            });
-
-            _currentBuildPlatform = null;
+            _buildStartTime = DateTime.UtcNow;
+            _currentBuildPlatform = GetBuildTargetName(options.target);
+            var platform = _currentBuildPlatform;
+
+            try
+            {
+                BuildReport result = BuildPipeline.BuildPlayer(options);
+
+                var duration = DateTime.UtcNow - _buildStartTime;
+                bool success = result.summary.result == BuildResult.Succeeded;
+
+                HookRegistry.NotifyBuildCompleted(success);
+                HookRegistry.NotifyBuildCompletedDetailed(new BuildArgs
+                {
+                    Platform = platform,
+                    Location = options.locationPathName,
+                    DurationMs = (long)duration.TotalMilliseconds,
+                    SizeBytes = success ? result.summary.totalSize : null,
+                    Success = success,
+                    Summary = success ? null : result.summary.ToString()
+                });
+            }
+            catch (Exception ex)
+            {
+                var duration = DateTime.UtcNow - _buildStartTime;
+                HookRegistry.NotifyBuildCompleted(false);
+                HookRegistry.NotifyBuildCompletedDetailed(new BuildArgs
+                {
+                    Platform = platform,
+                    Location = options.locationPathName,
+                    DurationMs = (long)duration.TotalMilliseconds,
+                    Success = false,
+                    Summary = ex.Message
+                });
+                throw;
+            }
+            finally
+            {
+                _currentBuildPlatform = null;
+            }

MCPForUnity/Editor/ActionTrace/Capture/CapturePoints/EventCapturePointBase.cs (1)

45-62: Unused context parameter.

The ContextMapping context parameter is declared but never used in the method body. If this is intentional for future extension, consider documenting it or removing it until needed to avoid confusion.

♻️ Option 1: Remove unused parameter

-protected void RecordEvent(EditorEvent evt, ContextMapping context = null)
+protected void RecordEvent(EditorEvent evt)

♻️ Option 2: Add TODO comment if intentional

 protected void RecordEvent(EditorEvent evt, ContextMapping context = null)
 {
     if (!IsEnabled) return;

     _stats.StartCapture();

     try
     {
-        // Create event and record via EventStore
+        // TODO: Integrate ContextMapping with EventStore.RecordWithContext when context layer is connected
         EventStore.Record(evt);
         _stats.EndCapture();
     }

MCPForUnity/Editor/ActionTrace/Metadata/Descriptors/ComponentEventDescriptor.cs (1)

83-117: Consider sharing asset-type detection with the emitter.

DetectAssetType duplicates the extension mapping already present in MCPForUnity/Editor/ActionTrace/Emitter.cs (lines 516-538). A shared helper would prevent drift and keep classification consistent across emission and metadata.

[sourcery-ai]

Sorry @whatevertogo, your pull request is larger than the review limit of 300000 diff characters

[coderabbitai]

Actionable comments posted: 11

🤖 Fix all issues with AI agents

In `@MCPForUnity/Editor/ActionTrace/Analysis/Context/ContextCompressor.cs`:
- Around line 232-241: SummarizeEvents currently allows a negative count which
makes endIdx and actualCount negative and can produce odd summaries; add an
input guard at the start of SummarizeEvents to handle invalid count values
(e.g., if count <= 0) and return string.Empty (or throw) instead of proceeding;
update the logic around endIdx, actualCount, and the check that follows so they
assume count is non-negative (refer to SummarizeEvents, startIdx, count, endIdx,
actualCount).

In `@MCPForUnity/Editor/ActionTrace/Capture/Capturers/AssetCapture.cs`:
- Around line 181-228: The dedup set updates in AssetCapture.cs aren't flagged
for persistence for deletions/moves because hasChanges is only set in the
imported-assets loop; update the deleted-assets foreach and moved-assets for
loop so that whenever _processedAssetsInSession.Add(assetPath) (or
_processedAssetsInSession.Add(movedAssets[i])) returns true you also set
hasChanges = true, ensuring the same persistence/save logic that runs after
imports will persist these dedup entries and prevent duplicates after domain
reloads.
- Around line 85-98: SaveProcessedAssets currently returns early when
_cachedProcessedAssets is null or empty, so CleanupOldEntries can clear the
in-memory list but the on-disk cache remains stale; update SaveProcessedAssets
(and use GetCacheFilePath) to persist the cleared state by either writing an
empty JSON array to the cache file or deleting the cache file when
_cachedProcessedAssets is empty/null, ensuring the on-disk cache reflects the
in-memory _cachedProcessedAssets after CleanupOldEntries.

In `@MCPForUnity/Editor/ActionTrace/Capture/Sampling/SamplingMiddleware.cs`:
- Around line 124-133: evt.TargetId may contain a prefix like "Asset:{path}",
causing EventFilter.ShouldTrackAsset to misclassify assets; in
SamplingMiddleware (the asset event handling block) prefer using
evt.Payload["path"] when present and otherwise normalize evt.TargetId by
stripping any "Asset:" (or similar) prefix before passing the path to
EventFilter.ShouldTrackAsset so blacklist checks run against the real filesystem
path.

In `@MCPForUnity/Editor/ActionTrace/Context/ToolCallScope.cs`:
- Around line 28-110: The scope is popped from a ThreadLocal stack on the wrong
thread when disposed off-thread (e.g., via ContinueWith in ExecuteAsync),
causing leaks; capture the creator's SynchronizationContext in the ToolCallScope
constructor (store in a new _syncContext field) and, in disposal/complete logic
(the methods that pop the scope and run cleanup—used by
ExecuteAsync/ContinueWith), if SynchronizationContext.Current != _syncContext,
marshal the cleanup back by calling _syncContext.Post(...); if _syncContext is
null fall back to EditorApplication.delayCall as before; ensure all places that
currently call EditorApplication.delayCall (and the dispose path used by
ExecuteAsync) use this marshaling so the original thread's ThreadLocal stack is
accessed.

In `@MCPForUnity/Editor/ActionTrace/Core/Store/EventStore.Diagnostics.cs`:
- Around line 17-23: Clamp the incoming hotEventCount to a non‑negative value
before using it in loops to prevent iterating past _events.Count; e.g. at the
start of DehydrateOldEvents(int hotEventCount) and the other similar method
(lines referenced) replace uses of hotEventCount with a defensive value such as
Math.Max(0, hotEventCount) (or assign hotEventCount = Math.Max(0,
hotEventCount)) so subsequent logic that uses _events.Count - hotEventCount
cannot go negative or produce negative diagnostics.
- Around line 54-63: GetMemoryDiagnostics iterates _events without guarding
against null entries (DehydrateOldEvents can leave nulls), causing a
NullReferenceException when accessing evt.IsDehydrated; update the loop in
GetMemoryDiagnostics (in EventStore.Diagnostics.cs) to skip or account for null
events (e.g., if evt == null continue / increment a null counter) before
checking evt.IsDehydrated or evt.Payload and still call EstimatePayloadSize only
when evt.Payload is non-null so the diagnostics loop is null-safe.

In `@MCPForUnity/Editor/Tools/UndoToSequenceTool.cs`:
- Around line 155-158: The delayed lambda should guard against an invalid active
scene: call UnityEditor.SceneManagement.EditorSceneManager.GetActiveScene(),
store it in a local variable, check scene.IsValid() (and optionally
scene.isLoaded) and only call EditorSceneManager.MarkSceneDirty(scene) if the
check passes; otherwise skip the call to avoid throwing on no/invalid scene.
Target the lambda registered with EditorApplication.delayCall and replace the
direct GetActiveScene()/MarkSceneDirty call with this guarded flow.

In `@MCPForUnity/Editor/Windows/ActionTraceEditorWindow.uss`:
- Around line 661-685: Replace the unsupported `@media` block with a class-based
responsive solution: in the window's C# editor (where you build the UI and have
access to rootVisualElement) register a GeometryChangedEvent callback, compute
isCompact = evt.newRect.width < 800 and call
rootVisualElement.EnableInClass("compact-layout", isCompact); then remove the
media query from ActionTraceEditorWindow.uss and add equivalent class-based
selectors (e.g., .compact-layout .event-list-container, .compact-layout
.detail-scroll-view, .compact-layout .toolbar-display, .compact-layout
.event-type, .compact-layout .event-time) that mirror the styles inside the
former media block so the UI toggles styles based on container width.
- Around line 590-594: The CSS selector .importance-bar-label is duplicated;
remove the later duplicate block (the one at the end with font-size: 10px;
color: `#808080`; margin-right: 8px;) so only the original definition (the earlier
.importance-bar-label selector) remains; ensure no other uses depend on the
second block and run a quick style check to confirm no regressions.
- Around line 377-379: The USS attribute selectors
(data-source="Human"/"AI"/"System") are never applied because the C# code in
bindItem does not set the attribute on the contextIndicator Label; update the
bindItem method to call contextIndicator.SetAttribute("data-source", sourceType)
(where sourceType is the event's context/source string you derive) whenever you
set contextIndicator.text or style so the selectors
(context-indicator[data-source="Human"], etc.) will match and the colors will
apply.

♻️ Duplicate comments (1)

MCPForUnity/Editor/ActionTrace/Context/ToolCallScope.cs (1)

495-525: Handle synchronous exceptions from func() to avoid scope leaks.

If func() throws before returning a Task, the scope is never completed or disposed and the failure event is lost.

🐛 Proposed fix

-            var task = func();
+            System.Threading.Tasks.Task<T> task;
+            try
+            {
+                task = func();
+            }
+            catch (Exception ex)
+            {
+                scope.Fail(ex.Message);
+                scope.Dispose();
+                throw;
+            }

🧹 Nitpick comments (9)

Server/src/models/action_trace.py (1)

49-52: Consider using Pydantic v2 model_config syntax.

The populate_by_name = True setting enables population by field name when aliases are defined, but no aliases are currently defined on fields. If aliases are needed for C# JSON interop (e.g., timestamp_unix_ms ↔ timestampUnixMs), consider defining them explicitly with alias=... or serialization_alias=....

Additionally, Pydantic v2 recommends model_config = ConfigDict(...) over nested class Config:

from pydantic import ConfigDict

class EditorEvent(BaseModel):
    model_config = ConfigDict(populate_by_name=True)
    # ...fields...

MCPForUnity/Editor/ActionTrace/Capture/Capturers/UndoCapture.cs (1)

66-71: Consider calling Undo.IncrementCurrentGroup() before setting the group name.

The nested-call guard (lines 53-58) is a good addition. However, without Undo.IncrementCurrentGroup() before line 68, any pre-existing operations in the current Undo group will be collapsed together with the tool call's operations when EndToolCall() runs. This could result in unrelated operations being bundled into the AI tool's undo entry.

🔧 Proposed fix

+            // Start a fresh Undo group to isolate this tool call's operations
+            Undo.IncrementCurrentGroup();
+
             // Set the current Undo group name
             // This name will appear in the Undo history (e.g., "Ctrl+Z AI: Create Player")
             Undo.SetCurrentGroupName($"AI: {ActionTraceHelper.FormatToolName(toolName)}");

MCPForUnity/Editor/Windows/ActionTraceEditorWindow.uss (1)

711-725: CSS variables are defined but never used in this stylesheet.

The custom properties (e.g., --accent-primary, --bg-primary) are defined but all color and styling values throughout the file are hardcoded. Consider using these variables for consistency and easier theming:

 .root-container {
     flex-direction: column;
     flex-grow: 1;
     overflow: hidden;
-    background-color: `#252525`;
+    background-color: var(--bg-primary);
 }

This would apply to many other hardcoded values like #4a90e2 → var(--accent-primary), #e0e0e0 → var(--text-primary), etc.

MCPForUnity/Editor/ActionTrace/Analysis/Context/ContextCompressor.cs (2)

49-54: TargetCompressionRatio is currently unused.
Callers setting this will see no effect. Consider either wiring it into ApplySizeLimits (e.g., derive MaxCompressedEvents from the ratio) or removing the field until it’s supported to avoid a misleading config surface.

---

382-388: Validate minutes in GetRecent to avoid surprising results.
Negative values shift the threshold into the future and silently return empty lists. Consider guarding or clamping.

🔧 Suggested fix

         public static List<EditorEvent> GetRecent(this IReadOnlyList<EditorEvent> events, int minutes)
         {
             if (events == null || events.Count == 0)
                 return new List<EditorEvent>();
+            if (minutes <= 0)
+                return new List<EditorEvent>();
 
             long thresholdMs = DateTimeOffset.UtcNow.AddMinutes(-minutes).ToUnixTimeMilliseconds();

MCPForUnity/Editor/ActionTrace/Capture/CapturePoints/EventCaptureRegistry.cs (2)

24-36: Guard duplicate IDs and clarify late registration behavior.
Without a uniqueness check, duplicates make GetCapturePoint/Unregister ambiguous and can double-init/shutdown. Also, if Register can occur after InitializeAll, decide whether to auto-initialize or explicitly warn.

♻️ Proposed improvement

 public void Register(IEventCapturePoint capturePoint)
 {
     if (capturePoint == null) return;

+    if (_capturePoints.Exists(p => p.CapturePointId == capturePoint.CapturePointId))
+    {
+        Debug.LogWarning($"[EventCaptureRegistry] Duplicate capture point id '{capturePoint.CapturePointId}' ignored.");
+        return;
+    }
+
     _capturePoints.Add(capturePoint);

     // Sort by priority
     _capturePoints.Sort((a, b) => b.InitializationPriority.CompareTo(a.InitializationPriority));
+
+    if (_isInitialized)
+        Debug.LogWarning($"[EventCaptureRegistry] Registered '{capturePoint.CapturePointId}' after initialization; it will not be initialized automatically.");
 }

---

66-83: Avoid collection mutation hazards during init/shutdown.
If a capture point registers/unregisters inside Initialize() or Shutdown(), the current loops can throw or skip items. Consider iterating over a snapshot or enforce a no-mutation contract.

♻️ Snapshot iteration option

 public void InitializeAll()
 {
     if (_isInitialized) return;

-    foreach (var point in _capturePoints)
+    var points = _capturePoints.ToArray();
+    foreach (var point in points)
     {
         try
         {
             point.Initialize();
         }
         catch (Exception ex)
         {
             Debug.LogError($"[EventCaptureRegistry] Failed to initialize {point.CapturePointId}: {ex.Message}");
         }
     }
@@
 public void ShutdownAll()
 {
     if (!_isInitialized) return;

     // Shutdown in reverse order
-    for (int i = _capturePoints.Count - 1; i >= 0; i--)
+    var points = _capturePoints.ToArray();
+    for (int i = points.Length - 1; i >= 0; i--)
     {
         try
         {
-            _capturePoints[i].Shutdown();
+            points[i].Shutdown();
         }
         catch (Exception ex)
         {
-            Debug.LogError($"[EventCaptureRegistry] Failed to shutdown {_capturePoints[i].CapturePointId}: {ex.Message}");
+            Debug.LogError($"[EventCaptureRegistry] Failed to shutdown {points[i].CapturePointId}: {ex.Message}");
         }
     }

Also applies to: 89-104

MCPForUnity/Editor/ActionTrace/Capture/Capturers/AssetCapture.cs (1)

235-249: Align cleanup comments with actual behavior.

The XML comment says time-based expiry (30 minutes), but the implementation is size-based (MaxCacheSize). Please update the comment or implement time-based expiry.

MCPForUnity/Editor/ActionTrace/Context/ToolCallScope.cs (1)

314-320: Prefer Guid-based suffix to avoid UnityEngine.Random thread affinity.

UnityEngine.Random.Range can be problematic off the main thread and relies on global state. A Guid suffix is thread-safe and avoids Unity dependencies here.

♻️ Proposed change

-            int random = UnityEngine.Random.Range(1000, 9999);
-            return $"{_toolId}_{timestamp}_{random}";
+            string suffix = Guid.NewGuid().ToString("N")[..4];
+            return $"{_toolId}_{timestamp}_{suffix}";

[coderabbitai]

Actionable comments posted: 8

🤖 Fix all issues with AI agents

In `@MCPForUnity/Editor/ActionTrace/Capture/Capturers/AssetCapture.cs`:
- Around line 1-10: Add the missing using for the EventFilter type so
EventFilter.ShouldTrackAsset(...) resolves: update AssetCapture.cs to include
the directive "using MCPForUnity.Editor.ActionTrace.Filters;" near the other
using statements so references to EventFilter (used in methods in AssetCapture
class at calls to ShouldTrackAsset) compile correctly.

In `@MCPForUnity/Editor/ActionTrace/Capture/Sampling/SamplingMiddleware.cs`:
- Around line 200-224: The eviction code that forcibly removes the oldest entry
from _pendingSamples when _pendingSamples.Count >= MaxSampleCache can drop
debounce samples silently; before calling _pendingSamples.TryRemove(oldestKey,
out _), retrieve the sample (TryGetValue/out var sample), detect if it is a
debounce sample (check the sample's debounce indicator, e.g. sample.IsDebounce
or sample.Type == SampleType.Debounce), and if so invoke the middleware's normal
recording/flushing path for finalized debounce samples (the same code used when
debounce samples are normally emitted) to persist/emit it, then remove the
entry; otherwise proceed with the TryRemove as before — use existing helpers
like CleanupExpiredSamples and the middleware's sample-recording method to avoid
duplicating logic.

In `@MCPForUnity/Editor/ActionTrace/Context/ToolCallScope.cs`:
- Around line 529-538: Add an explicit check for task cancellation before
accessing t.Result: when t.IsCanceled is true, call scope.Fail with a
cancellation message (e.g., "Async canceled") and throw a TaskCanceledException
instead of falling through to the else branch; update the block around the
existing t.IsFaulted/t.Result logic in ToolCallScope.cs (referencing the task
variable t and the scope instance) to handle t.IsCanceled first so t.Result is
never accessed for canceled tasks.
- Around line 334-340: GenerateCallId in ToolCallScope uses
UnityEngine.Random.Range which requires the Unity main thread; replace it with a
thread-safe RNG (e.g.,
System.Security.Cryptography.RandomNumberGenerator.GetInt32 or a
ThreadLocal<System.Random>) so GenerateCallId can be called from background
threads. Update GenerateCallId to compute the random suffix with a thread-safe
call (keep the same numeric range 1000–9999) and leave the rest of the ID format
unchanged so background construction via ExecuteAsync or direct construction no
longer throws UnityException.

In `@MCPForUnity/Editor/ActionTrace/Core/Settings/ActionTraceSettings.cs`:
- Around line 217-227: CreateSettings currently calls ApplyPreset (which calls
Save -> EditorUtility.SetDirty(this) and AssetDatabase.SaveAssets()) before the
ScriptableObject is added to the AssetDatabase, making SetDirty/SaveAssets
ineffective; fix by creating/adding the asset first with
AssetDatabase.CreateAsset(settings, SettingsPath) and
AssetDatabase.SaveAssets()/Refresh(), then call
settings.ApplyPreset(ActionTracePreset.Standard) (so SetDirty/SaveAssets operate
on an asset already registered), followed by final
AssetDatabase.SaveAssets()/Refresh() and the McpLog.Info call referencing
SettingsPath.

In `@MCPForUnity/Editor/Windows/ActionTraceEditorWindow.cs`:
- Around line 651-757: BuildJsonExport is corrupting its output because
SanitizeJson clears and reuses the shared field _stringBuilder; change
SanitizeJson to use a new local StringBuilder (or plain string operations)
instead of the shared _stringBuilder so it no longer mutates the in-progress
JSON, keeping BuildJsonExport and any other callers safe; locate the
SanitizeJson method and replace uses of the field _stringBuilder with a local
variable (e.g., var sb = new StringBuilder()) and return sb.ToString() without
touching the class-level _stringBuilder.
- Around line 830-841: The early-return in RefreshEvents only checks
EventStore.Count, _lastEventStoreCount, _currentEvents and _searchText, so
changes to filter/sort/context fields (_minImportance, _showContext, _sortMode)
won’t trigger a refresh; update RefreshEvents to also detect changes in those
values by adding cached last-seen fields (e.g., _lastMinImportance,
_lastShowContext, _lastSortMode) and include comparisons against them in the
short-circuit condition (or alternatively reset _lastEventStoreCount when any of
_minImportance/_showContext/_sortMode/_searchText change), then update those
cached fields
(_lastMinImportance/_lastShowContext/_lastSortMode/_lastSearchText) when
performing the refresh so filter/sort/context changes correctly force a list
refresh in RefreshEvents.

In `@MCPForUnity/Editor/Windows/ActionTraceEditorWindow.uss`:
- Around line 446-452: The .detail-scroll-view CSS has conflicting width
constraints (min-width: 320px vs max-width: 300px); update the rule for
.detail-scroll-view to make min-width <= max-width — either lower min-width to
300px or raise max-width to 320px (or choose a consistent value like min-width:
300px; max-width: 320px) so the width constraints are not contradictory; modify
the declaration in the ActionTraceEditorWindow.uss .detail-scroll-view block
accordingly.

♻️ Duplicate comments (3)

MCPForUnity/Editor/ActionTrace/Core/Store/EventStore.Diagnostics.cs (1)

45-51: Clamp hotEventCount in diagnostics to avoid negative counts (Line 46).

If settings allow negative values, hotEvents can go negative and coldEvents can exceed totalEvents. Add a non‑negative clamp here too.

🔧 Proposed fix

-                int hotEventCount = settings != null ? settings.Storage.HotEventCount : 100;
-                int maxEvents = settings != null ? settings.Storage.MaxEvents : 800;
+                int hotEventCount = settings != null ? settings.Storage.HotEventCount : 100;
+                int maxEvents = settings != null ? settings.Storage.MaxEvents : 800;
+                hotEventCount = Math.Max(0, hotEventCount);

MCPForUnity/Editor/ActionTrace/Context/ToolCallScope.cs (1)

521-524: Synchronous exception from func() still leaks the scope.

If func() throws synchronously before returning a Task, the scope is never completed or disposed. The ContinueWith callback never runs because there's no task to continue from.

🐛 Proposed fix wrapping func() invocation

         public static System.Threading.Tasks.Task<T> ExecuteAsync<T>(
             string toolName,
             Func<System.Threading.Tasks.Task<T>> func,
             string toolId = null,
             Dictionary<string, object> parameters = null)
         {
             var scope = new ToolCallScope(toolName, toolId, parameters);

-            var task = func();
+            System.Threading.Tasks.Task<T> task;
+            try
+            {
+                task = func();
+            }
+            catch (Exception ex)
+            {
+                scope.Fail(ex.Message);
+                scope.Dispose();
+                throw;
+            }

             return task.ContinueWith(t =>

MCPForUnity/Editor/ActionTrace/Capture/Sampling/SamplingMiddleware.cs (1)

266-306: Previous cleanup concern has been addressed with 2x safety margin.

The implementation now uses a 2x window safety margin for debounce modes (line 281), which prevents samples from being cleaned up before FlushExpiredDebounceSamples can record them. While this differs from the previously suggested fix, it's a reasonable approach that addresses the core concern.

🧹 Nitpick comments (10)

MCPForUnity/Editor/Tools/UndoToSequenceTool.cs (1)

35-48: Parameters class is defined but never instantiated.

HandleCommand manually parses the JObject instead of deserializing to Parameters. This creates dead code and risks drift if someone updates Parameters without updating the parsing logic.

Consider either:

1. Deserialize to Parameters and use it, or
2. Remove the Parameters class if manual parsing is preferred.

MCPForUnity/Editor/ActionTrace/Core/Settings/ActionTraceSettings.cs (3)

17-28: Potential documentation inconsistency in importance scoring.

The MinImportanceForRecording field uses a 0.0-1.0 range (line 17-19), but the EnableEmergencyAITrigger tooltip references "Critical events (score 10.0)" (line 27). This inconsistency could confuse future maintainers about whether importance is measured on a 0-1 or 0-10 scale.

Consider clarifying the tooltip to use consistent terminology, e.g., "importance 1.0" instead of "score 10.0".

---

65-72: Consider adding Range attributes for consistency.

MinKeepEvents and AutoSaveIntervalSeconds lack [Range] attributes unlike other numeric fields in this class. This allows potentially invalid values (e.g., negative or extremely large) to be set in the Inspector.

♻️ Proposed fix

-        [Tooltip("Minimum number of events to keep when auto-cleaning.")]
-        public int MinKeepEvents = 100;
+        [Range(10, 1000)]
+        [Tooltip("Minimum number of events to keep when auto-cleaning.")]
+        public int MinKeepEvents = 100;

-        [Tooltip("Auto-save interval in seconds. 0 = disable auto-save.")]
-        public int AutoSaveIntervalSeconds = 30;
+        [Range(0, 300)]
+        [Tooltip("Auto-save interval in seconds. 0 = disable auto-save.")]
+        public int AutoSaveIntervalSeconds = 30;

---

276-282: Add defensive check for invalid HotEventCount configuration.

If HotEventCount > MaxEvents (which the Validate() method checks but doesn't prevent), coldEvents becomes negative, producing an incorrect memory estimate.

♻️ Proposed fix

 public long GetEstimatedMemoryUsage()
 {
     // Approximate: each event ~300 bytes when hydrated, ~100 bytes when dehydrated
     int hotEvents = Storage.HotEventCount;
-    int coldEvents = Storage.MaxEvents - Storage.HotEventCount;
+    int coldEvents = Math.Max(0, Storage.MaxEvents - Storage.HotEventCount);
     return (long)(hotEvents * 300 + coldEvents * 100);
 }

MCPForUnity/Editor/ActionTrace/Capture/Capturers/AssetCapture.cs (3)

248-268: Documentation mismatch and aggressive cache clearing.

The method comment (lines 249-250) mentions "time-based expiration (30 minutes)" but the implementation uses count-based clearing (MaxCacheSize = 1000). Additionally, clearing the entire cache when the threshold is exceeded is aggressive and could potentially cause duplicate events.

♻️ Suggested improvements

1. Update the comment to accurately describe the count-based approach:

         /// <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 with a maximum cache size of 1000 entries.
         /// This is called at the start of each OnPostprocessAllAssets batch.
         /// </summary>

2. Optionally, consider partial eviction instead of full clear to reduce duplicate event risk:

// Remove oldest 20% of entries instead of clearing all
int removeCount = _cachedProcessedAssets.Count / 5;
var toRemove = _cachedProcessedAssets.Take(removeCount).ToList();
foreach (var item in toRemove)
    _cachedProcessedAssets.Remove(item);

---

280-303: Consider logging suppressed exceptions.

The empty catch block at line 298 silently swallows all exceptions. While returning false (treating as imported) is a safe conservative default, adding debug-level logging would help diagnose unexpected issues.

♻️ Suggested improvement

             catch (Exception ex)
             {
-                // On any error, default to treating as imported (conservative)
+                // On any error, default to treating as imported (conservative)
+                McpLog.Debug($"[AssetChangePostprocessor] IsNewlyCreatedAsset check failed for '{assetPath}': {ex.Message}");
                 return false;
             }

---

309-348: Use culture-invariant string comparison for file extensions.

Both ShouldTrackModification (line 311) and GetAssetType (line 325) use .ToLower(), which is culture-sensitive. For file extension comparisons, .ToLowerInvariant() is more appropriate to avoid locale-specific issues (e.g., the Turkish 'İ'/'I' problem).

♻️ Proposed fix

         private static bool ShouldTrackModification(string assetPath)
         {
-            string ext = System.IO.Path.GetExtension(assetPath).ToLower();
+            string ext = System.IO.Path.GetExtension(assetPath).ToLowerInvariant();
             // Track modifications for these asset types
             ...
         }

         private static string GetAssetType(string assetPath)
         {
-            string ext = System.IO.Path.GetExtension(assetPath).ToLower();
+            string ext = System.IO.Path.GetExtension(assetPath).ToLowerInvariant();
             return ext switch
             ...
         }

MCPForUnity/Editor/ActionTrace/Context/ToolCallScope.cs (2)

24-27: Thread safety concern with shared lists.

_capturedEvents and _childScopes are regular List<T> instances that can be accessed concurrently:

• _childScopes is modified by child scopes via _parentScope?._childScopes.Add(this) (line 120), which may run on a different thread than the parent
• _capturedEvents is accessed via RecordEvent which could be called from any thread

Consider using ConcurrentBag<T> or adding synchronization to prevent race conditions in multi-threaded scenarios.

♻️ Suggested fix using concurrent collections

-        private readonly List<EditorEvent> _capturedEvents;
+        private readonly System.Collections.Concurrent.ConcurrentBag<EditorEvent> _capturedEvents;
         private readonly long _startTimestampMs;
-        private readonly List<ToolCallScope> _childScopes;
+        private readonly System.Collections.Concurrent.ConcurrentBag<ToolCallScope> _childScopes;

Note: This would require updating the IReadOnlyList property types and iteration patterns accordingly.

---

326-332: Consider logging when out-of-order disposal is detected.

If Peek() != this, the scope won't be popped and will remain on the stack. This defensive check is good, but the silent failure could mask bugs where scopes are disposed out of order.

♻️ Suggested diagnostic logging

 private void PopFromStack()
 {
     if (_scopeStack.Value.Count > 0 && _scopeStack.Value.Peek() == this)
     {
         _scopeStack.Value.Pop();
     }
+    else if (_scopeStack.Value.Count > 0)
+    {
+        UnityEngine.Debug.LogWarning($"[ToolCallScope] Out-of-order disposal detected for '{_toolName}'. Expected top: {_scopeStack.Value.Peek()._toolName}");
+    }
 }

MCPForUnity/Editor/ActionTrace/Capture/Sampling/SamplingMiddleware.cs (1)

312-322: Minor race window between iteration and Clear().

If another thread adds an item between the iteration (lines 316-319) and Clear() (line 320), that item will be lost. For the documented "shutdown" use case this is likely acceptable, but consider using a draining loop if atomicity is needed.

♻️ Optional: Atomic drain approach

         public static List<EditorEvent> FlushPending()
         {
-            // Manual loop instead of LINQ Select to avoid allocation
-            var result = new List<EditorEvent>(_pendingSamples.Count);
-            foreach (var kvp in _pendingSamples)
+            var result = new List<EditorEvent>();
+            // Drain atomically by removing each entry
+            foreach (var key in _pendingSamples.Keys.ToArray())
             {
-                result.Add(kvp.Value.Event);
+                if (_pendingSamples.TryRemove(key, out var sample))
+                {
+                    result.Add(sample.Event);
+                }
             }
-            _pendingSamples.Clear();
             return result;
         }

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Force-eviction can silently drop pending debounce events.

When the cache hits MaxSampleCache, the oldest entry is removed without checking if it's a debounce sample that should be recorded. This can cause silent data loss under high event load.

🛠️ Proposed fix: Record debounce samples before eviction

                 if (_pendingSamples.Count >= MaxSampleCache)
                 {
                     // 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;
                         }
                     }
-                    if (!string.IsNullOrEmpty(oldestKey))
+                    if (!string.IsNullOrEmpty(oldestKey) && _pendingSamples.TryRemove(oldestKey, out _))
                     {
-                        _pendingSamples.TryRemove(oldestKey, out _);
+                        // Record evicted debounce samples to prevent data loss
+                        if (SamplingConfig.Strategies.TryGetValue(oldestSample.Event.Type, out var evictedStrategy) &&
+                            (evictedStrategy.Mode == SamplingMode.Debounce || evictedStrategy.Mode == SamplingMode.DebounceByKey))
+                        {
+                            EventStore.Record(oldestSample.Event);
+                        }
                     }
                 }

🤖 Prompt for AI Agents

In `@MCPForUnity/Editor/ActionTrace/Capture/Sampling/SamplingMiddleware.cs` around
lines 200 - 224, The eviction code that forcibly removes the oldest entry from
_pendingSamples when _pendingSamples.Count >= MaxSampleCache can drop debounce
samples silently; before calling _pendingSamples.TryRemove(oldestKey, out _),
retrieve the sample (TryGetValue/out var sample), detect if it is a debounce
sample (check the sample's debounce indicator, e.g. sample.IsDebounce or
sample.Type == SampleType.Debounce), and if so invoke the middleware's normal
recording/flushing path for finalized debounce samples (the same code used when
debounce samples are normally emitted) to persist/emit it, then remove the
entry; otherwise proceed with the TryRemove as before — use existing helpers
like CleanupExpiredSamples and the middleware's sample-recording method to avoid
duplicating logic.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

UnityEngine.Random is not thread-safe and requires main thread.

UnityEngine.Random.Range() can only be called from Unity's main thread. If a ToolCallScope is created on a background thread (which is possible with ExecuteAsync or direct construction), this will throw a UnityException.

🐛 Proposed fix using thread-safe random

+        private static readonly ThreadLocal<System.Random> _random = 
+            new(() => new System.Random(Guid.NewGuid().GetHashCode()));
+
         private string GenerateCallId()
         {
             // Compact ID: tool name + timestamp + random suffix
             long timestamp = _startTimestampMs % 1000000; // Last 6 digits of timestamp
-            int random = UnityEngine.Random.Range(1000, 9999);
+            int random = _random.Value.Next(1000, 9999);
             return $"{_toolId}_{timestamp}_{random}";
         }

📝 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.

Suggested change

	private string GenerateCallId()
	{
	// Compact ID: tool name + timestamp + random suffix
	long timestamp = _startTimestampMs % 1000000; // Last 6 digits of timestamp
	int random = UnityEngine.Random.Range(1000, 9999);
	return $"{_toolId}_{timestamp}_{random}";
	}
	private static readonly ThreadLocal<System.Random> _random =
	new(() => new System.Random(Guid.NewGuid().GetHashCode()));
	private string GenerateCallId()
	{
	// Compact ID: tool name + timestamp + random suffix
	long timestamp = _startTimestampMs % 1000000; // Last 6 digits of timestamp
	int random = _random.Value.Next(1000, 9999);
	return $"{_toolId}_{timestamp}_{random}";
	}

🤖 Prompt for AI Agents

In `@MCPForUnity/Editor/ActionTrace/Context/ToolCallScope.cs` around lines 334 -
340, GenerateCallId in ToolCallScope uses UnityEngine.Random.Range which
requires the Unity main thread; replace it with a thread-safe RNG (e.g.,
System.Security.Cryptography.RandomNumberGenerator.GetInt32 or a
ThreadLocal<System.Random>) so GenerateCallId can be called from background
threads. Update GenerateCallId to compute the random suffix with a thread-safe
call (keep the same numeric range 1000–9999) and leave the rest of the ID format
unchanged so background construction via ExecuteAsync or direct construction no
longer throws UnityException.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Task cancellation not handled - will throw wrong exception.

When t.IsCanceled is true, the code falls through to the else branch and accesses t.Result, which throws AggregateException wrapping TaskCanceledException instead of properly handling the cancellation.

🐛 Proposed fix handling cancellation

             return task.ContinueWith(t =>
             {
                 try
                 {
                     if (t.IsFaulted)
                     {
                         scope.Fail(t.Exception?.Message ?? "Async faulted");
                         throw t.Exception ?? new Exception("Async task faulted");
                     }
+                    else if (t.IsCanceled)
+                    {
+                        scope.Fail("Task was canceled");
+                        throw new System.Threading.Tasks.TaskCanceledException(t);
+                    }
                     else
                     {
                         scope.Complete(t.Result?.ToString() ?? "");
                         return t.Result;
                     }
                 }

📝 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.

Suggested change

	if (t.IsFaulted)
	{
	scope.Fail(t.Exception?.Message ?? "Async faulted");
	throw t.Exception ?? new Exception("Async task faulted");
	}
	else
	{
	scope.Complete(t.Result?.ToString() ?? "");
	return t.Result;
	}
	try
	{
	if (t.IsFaulted)
	{
	scope.Fail(t.Exception?.Message ?? "Async faulted");
	throw t.Exception ?? new Exception("Async task faulted");
	}
	else if (t.IsCanceled)
	{
	scope.Fail("Task was canceled");
	throw new System.Threading.Tasks.TaskCanceledException(t);
	}
	else
	{
	scope.Complete(t.Result?.ToString() ?? "");
	return t.Result;
	}
	}
	finally
	{
	scope.Dispose();
	}

🤖 Prompt for AI Agents

In `@MCPForUnity/Editor/ActionTrace/Context/ToolCallScope.cs` around lines 529 -
538, Add an explicit check for task cancellation before accessing t.Result: when
t.IsCanceled is true, call scope.Fail with a cancellation message (e.g., "Async
canceled") and throw a TaskCanceledException instead of falling through to the
else branch; update the block around the existing t.IsFaulted/t.Result logic in
ToolCallScope.cs (referencing the task variable t and the scope instance) to
handle t.IsCanceled first so t.Result is never accessed for canceled tasks.

[coderabbitai]

Actionable comments posted: 2

🤖 Fix all issues with AI agents

In `@MCPForUnity/Editor/Windows/ActionTraceEditorWindow.cs`:
- Around line 792-801: The Clear handler is clearing the ScrollView children
instead of resetting the detail panel state; in OnClearClicked replace the
_detailScrollView.Clear() call with logic that resets the detail area by
clearing _detailContent's children (or removing its current detail element),
re-attaching or showing _detailPlaceholder, and ensuring any selected/detail
state is nullified so the panel is consistent (refer to _detailContent,
_detailPlaceholder, and OnClearClicked to locate the change).

In `@MCPForUnity/Editor/Windows/ActionTraceEditorWindow.uss`:
- Around line 380-383: The ContextMapping side-table currently lacks the
OperationSource enum value so the UI can only apply context-source--System; add
an OperationSource (or Source) property of type OperationSource to the
ContextMapping class, update all places that construct/save/deserialize
ContextMapping instances so they persist this value to the event store, ensure
the code that reads ContextMappings for display (used by
ActionTraceEditorWindow) uses the new OperationSource to choose the CSS class
(context-source--Human / --AI / --System), and update any
migration/serialization routines to maintain backwards compatibility for
existing mappings.

♻️ Duplicate comments (3)

MCPForUnity/Editor/Windows/ActionTraceEditorWindow.cs (2)

734-757: JSON export is corrupted by shared StringBuilder usage.

SanitizeJson reuses the class-level _stringBuilder (line 739), but BuildJsonExport is also using _stringBuilder to build the JSON output. Each call to SanitizeJson from within BuildJsonExport clears the builder at line 740, wiping the partially built JSON.

This is a critical bug that will produce corrupt/empty JSON exports.

🔧 Proposed fix - use a local StringBuilder in SanitizeJson

 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);
+    // Use a local StringBuilder to avoid corrupting shared state
+    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();
 }

---

830-841: Refresh short-circuit skips filter/sort/context changes.

The early return at lines 834-839 only considers EventStore.Count and _searchText. If _minImportance, _showContext, or _sortMode change without new events, the list won't refresh, leaving stale data.

🔧 Proposed fix - track additional state for refresh decisions

Add tracking fields:

 private int _lastEventStoreCount = -1;
+private string _lastSearchText = string.Empty;
+private float _lastMinImportance = -1f;
+private bool _lastShowContext;
+private SortMode _lastSortMode = SortMode.ByTimeDesc;

Update the short-circuit logic:

-if (currentStoreCount == _lastEventStoreCount && 
-    _currentEvents.Count > 0 && 
-    string.IsNullOrEmpty(_searchText))
+bool canSkip = currentStoreCount == _lastEventStoreCount
+    && _currentEvents.Count > 0
+    && _lastSearchText == _searchText
+    && Mathf.Approximately(_lastMinImportance, _minImportance)
+    && _lastShowContext == _showContext
+    && _lastSortMode == _sortMode;
+if (canSkip)
 {
     // No new events and no search criteria, skip refresh
     return;
 }
 _lastEventStoreCount = currentStoreCount;
+_lastSearchText = _searchText;
+_lastMinImportance = _minImportance;
+_lastShowContext = _showContext;
+_lastSortMode = _sortMode;

MCPForUnity/Editor/Windows/ActionTraceEditorWindow.uss (1)

446-455: Conflicting width constraints and duplicate min-height.

Lines 448-451 have issues:

1. min-width: 320px conflicts with max-width: 300px (min > max)
2. min-height is declared twice (lines 449-450)

The detail pane may render unexpectedly due to the impossible width constraints.

🔧 Proposed fix

 .detail-scroll-view {
     flex: 1;
-    min-width: 320px;
-    min-height: 100%;
+    min-width: 300px;
     min-height: 100px;
-    max-width: 300px;
+    max-width: 450px;
     background-color: `#252525`;
     border-left-width: 1px;
     border-left-color: `#3a3a3a`;
 }

🧹 Nitpick comments (3)

MCPForUnity/Editor/Windows/ActionTraceEditorWindow.cs (1)

997-1006: Redundant global state mutation in SetSortMode.

Line 1002 unconditionally sets BypassImportanceFilter = true on every sort mode change. Since CreateGUI already sets this and stores the original value for restoration in OnDisable, this repeated mutation is redundant and could cause confusion.

Consider removing this line since the bypass is already active for the window's lifetime:

💡 Suggested fix

 private void SetSortMode(SortMode mode)
 {
     _sortMode = mode;

-    if (ActionTraceSettings.Instance != null)
-        ActionTraceSettings.Instance.Filtering.BypassImportanceFilter = true;
-
     UpdateSortButtonText();
     RefreshEvents();
 }

MCPForUnity/Editor/Windows/ActionTraceEditorWindow.uss (1)

661-683: Class-based responsive styles added, but C# implementation missing.

The USS defines .compact-layout rules (lines 663-683), which is the correct approach for responsive behavior in Unity. However, the C# window code does not currently add/remove the compact-layout class based on window size.

To enable responsive behavior, add a GeometryChangedEvent handler in CreateGUI:

💡 C# code to add in ActionTraceEditorWindow.cs

rootVisualElement.RegisterCallback<GeometryChangedEvent>(evt =>
{
    bool isCompact = evt.newRect.width < 800;
    rootVisualElement.EnableInClassList("compact-layout", isCompact);
});

MCPForUnity/Editor/Tools/ActionTraceSettingsTool.cs (1)

27-63: Add null check for ActionTraceSettings.Instance for consistency with codebase patterns.

While ActionTraceSettings.Instance includes fallback logic (LoadSettings → CreateSettings), it can theoretically return null in edge cases (e.g., if CreateInstance fails). Throughout the codebase, other code defensively checks for null (ActionTraceEditorWindow.cs uses ?., EventStore.cs uses explicit null checks, ActionTraceViewResource.cs early-returns on null), but this method does not. Add an explicit null check to align with the defensive patterns used elsewhere:

💡 Suggested improvement

 try
 {
     var settings = ActionTraceSettings.Instance;
+    if (settings == null)
+    {
+        return new ErrorResponse("ActionTraceSettings not initialized.");
+    }

     return new SuccessResponse("Retrieved ActionTrace settings.", new

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

OnClearClicked clears wrong element for detail panel.

Line 798 calls _detailScrollView.Clear(), which removes child elements from the ScrollView. However, the detail content is managed separately via _detailContent and _detailPlaceholder. This may leave the detail panel in an inconsistent state.

Consider resetting the detail panel properly:

💡 Suggested fix

 private void OnClearClicked()
 {
     EventStore.Clear();
     _currentEvents.Clear();
     _lastEventStoreCount = 0;
     _eventListView.RefreshItems();
-    _detailScrollView.Clear();
+    _selectedItem = null;
+    _detailContent?.Clear();
+    if (_detailPlaceholder != null)
+        _detailPlaceholder.style.display = DisplayStyle.Flex;
+    if (_detailContent != null)
+        _detailContent.style.display = DisplayStyle.None;
+    if (_detailActions != null)
+        _detailActions.style.display = DisplayStyle.None;
     UpdateStatus();
     McpLog.Debug("[ActionTraceEditorWindow] Clear clicked");
 }

📝 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.

Suggested change

	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();
	_selectedItem = null;
	_detailContent?.Clear();
	if (_detailPlaceholder != null)
	_detailPlaceholder.style.display = DisplayStyle.Flex;
	if (_detailContent != null)
	_detailContent.style.display = DisplayStyle.None;
	if (_detailActions != null)
	_detailActions.style.display = DisplayStyle.None;
	UpdateStatus();
	McpLog.Debug("[ActionTraceEditorWindow] Clear clicked");
	}

🤖 Prompt for AI Agents

In `@MCPForUnity/Editor/Windows/ActionTraceEditorWindow.cs` around lines 792 -
801, The Clear handler is clearing the ScrollView children instead of resetting
the detail panel state; in OnClearClicked replace the _detailScrollView.Clear()
call with logic that resets the detail area by clearing _detailContent's
children (or removing its current detail element), re-attaching or showing
_detailPlaceholder, and ensuring any selected/detail state is nullified so the
panel is consistent (refer to _detailContent, _detailPlaceholder, and
OnClearClicked to locate the change).

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, check for context-source references in C# code
rg -n "context-source" --type cs -C 3

Repository: CoplayDev/unity-mcp

Length of output: 801

---

🏁 Script executed:

# Also search for ContextSource or related enums/mappings
rg -n "ContextSource|ContextMapping|ContextType" --type cs -A 2

Repository: CoplayDev/unity-mcp

Length of output: 12190

---

🏁 Script executed:

# Read the USS file to see the actual selectors
find . -name "ActionTraceEditorWindow.uss" -exec head -400 {} \; | tail -30

Repository: CoplayDev/unity-mcp

Length of output: 755

---

🏁 Script executed:

# Find and read the C# file's BindListItem method
find . -name "ActionTraceEditorWindow.cs" -type f

Repository: CoplayDev/unity-mcp

Length of output: 118

---

🏁 Script executed:

# Search for OperationContext.Source property and how context source is determined
rg -n "OperationContext" --type cs -A 10 | head -80

Repository: CoplayDev/unity-mcp

Length of output: 7386

---

🏁 Script executed:

# Look for context source enumeration or Source property
rg -n "Source.*Human|Source.*AI|class OperationContext" --type cs -B 2 -A 8

Repository: CoplayDev/unity-mcp

Length of output: 8103

---

🏁 Script executed:

# Check OperationContext.cs file content
find . -name "OperationContext.cs" -type f -exec cat -n {} \;

Repository: CoplayDev/unity-mcp

Length of output: 5350

---

🏁 Script executed:

# Find the BindListItem method signature and implementation
rg -n "void BindListItem|BindListItem" --type cs -B 2 -A 15 MCPForUnity/Editor/Windows/ActionTraceEditorWindow.cs

Repository: CoplayDev/unity-mcp

Length of output: 1434

---

🏁 Script executed:

# Search for methods that retrieve OperationContext by contextId
rg -n "GetContext|RetrieveContext|LookupContext|Find.*Context" --type cs

Repository: CoplayDev/unity-mcp

Length of output: 45

---

🏁 Script executed:

# Check the type of _currentEvents and how it's populated
rg -n "_currentEvents" --type cs -B 2 -A 2 MCPForUnity/Editor/Windows/ActionTraceEditorWindow.cs

Repository: CoplayDev/unity-mcp

Length of output: 3951

---

🏁 Script executed:

# Look for QueryResult or similar class that holds event + context
rg -n "class QueryResult|struct QueryResult" --type cs -A 10

Repository: CoplayDev/unity-mcp

Length of output: 45

---

🏁 Script executed:

# Check the full BindListItem method
sed -n '320,450p' MCPForUnity/Editor/Windows/ActionTraceEditorWindow.cs

Repository: CoplayDev/unity-mcp

Length of output: 5731

---

🏁 Script executed:

# Find ActionTraceViewItem definition to understand the Context property
rg -n "class ActionTraceViewItem|struct ActionTraceViewItem" --type cs -A 20

Repository: CoplayDev/unity-mcp

Length of output: 2286

---

🏁 Script executed:

# Check how the query populates the context information
rg -n "new ActionTraceViewItem|ActionTraceViewItem\(" --type cs -B 3 -A 3

Repository: CoplayDev/unity-mcp

Length of output: 3134

---

🏁 Script executed:

# Check the complete ContextMapping class definition
rg -n "public sealed class ContextMapping" --type cs -A 15

Repository: CoplayDev/unity-mcp

Length of output: 1546

---

🏁 Script executed:

# Look for any public API to retrieve OperationContext by contextId
rg -n "public.*GetContext|public.*LookupContext|public.*FindContext|OperationContextCache" --type cs

Repository: CoplayDev/unity-mcp

Length of output: 45

---

🏁 Script executed:

# Check how ContextTimeline or EventStore manages the mapping between contextId and OperationContext
sed -n '1,100p' MCPForUnity/Editor/ActionTrace/Context/ContextTimeline.cs

Repository: CoplayDev/unity-mcp

Length of output: 2828

---

🏁 Script executed:

# Check if there's a way to retrieve OperationContext by contextId in EventStore or elsewhere
rg -n "ContextId|contextId.*=>|contextStack|_contextMappings" --type cs | grep -i "operationcontext\|retrieve\|lookup\|get" | head -20

Repository: CoplayDev/unity-mcp

Length of output: 739

---

🏁 Script executed:

# Look at the full ContextStack implementation to see if it stores contexts by ID
cat MCPForUnity/Editor/ActionTrace/Context/ContextStack.cs | head -100

Repository: CoplayDev/unity-mcp

Length of output: 3189

---

🏁 Script executed:

# Check the complete ContextMapping definition to see if Source is stored anywhere
cat MCPForUnity/Editor/ActionTrace/Context/ContextMapping.cs

Repository: CoplayDev/unity-mcp

Length of output: 1896

---

🏁 Script executed:

# Search for any cache or lookup mechanism for OperationContext
rg -n "static.*Dictionary|static.*Cache|_contexts|_operationContexts" --type cs MCPForUnity/Editor/ActionTrace/

Repository: CoplayDev/unity-mcp

Length of output: 4047

---

🏁 Script executed:

# Check if Source is serialized/persisted anywhere in the ContextMapping storage
rg -n "OperationSource|Source.*serialize|Source.*json" --type cs MCPForUnity/Editor/ActionTrace/Context/ MCPForUnity/Editor/ActionTrace/Core/Store/

Repository: CoplayDev/unity-mcp

Length of output: 952

---

🏁 Script executed:

# Look at EventStore persistence to see what's actually stored
sed -n '1,60p' MCPForUnity/Editor/ActionTrace/Core/Store/EventStore.Persistence.cs

Repository: CoplayDev/unity-mcp

Length of output: 2434

---

Context source information not persisted—Human/AI classes cannot be determined at display time.

The USS selectors are correct, but the C# code can only apply context-source--System because the ContextMapping side-table stores only contextId and eventSequence—not the OperationSource value. The OperationContext.Source (Human/AI/System) only exists in the thread-local ContextStack during execution and is not persisted in the store.

To display the correct context source colors, OperationSource must be added to the ContextMapping class and persisted alongside ContextMappings in the event store.

🤖 Prompt for AI Agents

In `@MCPForUnity/Editor/Windows/ActionTraceEditorWindow.uss` around lines 380 -
383, The ContextMapping side-table currently lacks the OperationSource enum
value so the UI can only apply context-source--System; add an OperationSource
(or Source) property of type OperationSource to the ContextMapping class, update
all places that construct/save/deserialize ContextMapping instances so they
persist this value to the event store, ensure the code that reads
ContextMappings for display (used by ActionTraceEditorWindow) uses the new
OperationSource to choose the CSS class (context-source--Human / --AI /
--System), and update any migration/serialization routines to maintain backwards
compatibility for existing mappings.

[whatevertogo]

Because the first version doesn't have this feature yet

[coderabbitai]

@whatevertogo, understood — this is a v1 limitation and the feature isn't implemented yet. The CSS classes are in place for when you're ready to add context-source persistence in a future version. Feel free to reach out if you'd like help implementing it later!