feat: action trace feat: the HookSystem for Unity #570
Conversation
Fixes CoplayDev#538 The System Requirements panel showed "UV Package Manager: Not Found" even when a valid UV path override was configured in Advanced Settings. Root cause: PlatformDetectorBase.DetectUv() only searched PATH with bare command names ("uvx", "uv") and never consulted PathResolverService which respects the user's override setting. Changes: - Refactor DetectUv() to use PathResolverService.GetUvxPath() which checks override path first, then system PATH, then falls back to "uvx" - Add TryValidateUvExecutable() to verify executables by running --version instead of just checking File.Exists - Prioritize PATH environment variable in EnumerateUvxCandidates() for better compatibility with official uv install scripts - Fix process output read order (ReadToEnd before WaitForExit) to prevent potential deadlocks Co-Authored-By: ChatGLM 4.7 <[email protected]>
- Read both stdout and stderr when validating uv/uvx executables
- Respect WaitForExit timeout return value instead of ignoring it
- Fix version parsing to handle extra tokens like "(Homebrew 2025-01-01)"
- Resolve bare commands ("uv"/"uvx") to absolute paths after validation
- Rename FindExecutableInPath to FindUvxExecutableInPath for clarity
Co-Authored-By: Claude Opus 4.5 <[email protected]>
…s PATH augmentation Replace direct Process.Start calls with ExecPath.TryRun across all platform detectors. This change: - Fixes potential deadlocks by using async output reading - Adds proper timeout handling with process termination - Removes redundant fallback logic and simplifies version parsing - Adds Windows PATH augmentation with common uv, npm, and Python installation paths Co-Authored-By: Claude Opus 4.5 <[email protected]>
The version extraction logic now properly handles outputs like: - "uvx 0.9.18" -> "0.9.18" - "uvx 0.9.18 (hash date)" -> "0.9.18" - "uvx 0.9.18 extra info" -> "0.9.18" Uses Math.Min to find the first occurrence of either space or parenthesis. Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add absolute path resolution in TryValidatePython and TryValidateUvWithPath for better UI display - Fix BuildAugmentedPath to avoid PATH duplication - Add comprehensive comments for version parsing logic - Ensure cross-platform consistency across all three detectors - Fix override path validation logic with clear state handling - Fix platform detector path resolution and Python version detection - Use UserProfile consistently in GetClaudeCliPath instead of Personal - All platforms now use protected BuildAugmentedPath method This change improves user experience by displaying full paths in the UI while maintaining robust fallback behavior if path resolution fails. Co-Authored-By: GLM4.7 <[email protected]>
- Rename TryValidateUvExecutable -> TryValidateUvxExecutable for consistency - Add cross-platform FindInPath() helper in ExecPath.cs - Remove platform-specific where/which implementations in favor of unified helper - Add Windows-specific DetectUv() override with enhanced uv/uvx detection - Add WinGet shim path support for Windows uvx installation - Update UI labels: "UV Path" -> "UVX Path" - Only show uvx path status when override is configured Co-Authored-By: Claude Opus 4.5 <[email protected]>
…ides and system paths
…ethods across platform detectors
… WindowsPlatformDetector
Implement a comprehensive ActionTrace system that captures, stores, and queries Unity editor events for debugging, analysis, and undo/replay capabilities. **Core Features:** - Event capture layer with hooks for Unity editor events - Context tracking with stack and timeline support - Event store with in-memory persistence and query capabilities - Semantic analysis (categorization, scoring, intent inference) - VCS integration for version control context - Editor window with UI for visualizing events - MCP tools for remote query and control **Components Added:** - Capture: ActionTraceEventEmitter, EventFilter, PropertyChangeTracker, UnityEventHooks - Context: ContextStack, ContextTimeline, OperationContext, ContextMapping - Core: EventStore, EditorEvent, EventTypes, ActionTraceSettings, GlobalIdHelper - Query: ActionTraceQuery, EventSummarizer, TransactionAggregator - Semantics: DefaultCategorizer, DefaultEventScorer, DefaultIntentInferrer - UI: ActionTraceEditorWindow with UXML/USS styling - MCP Tools: get_action_trace, get_action_trace_settings, add_action_trace_note, undo_to_sequence **Server-side:** - Python models and resources for ActionTrace - MCP tools for querying events, managing settings, and undoing to sequence Co-Authored-By: Claude Opus 4.5 <[email protected]>
…ility - Extract EventStore.Context.cs for context mapping management - Extract EventStore.Diagnostics.cs for memory diagnostics and dehydration - Extract EventStore.Merging.cs for event deduplication logic - Extract EventStore.Persistence.cs for save/load and domain reload survival - Add PropertyEventPayloadBuilder helper for consistent payload structure - Add PropertyFormatter helper to eliminate code duplication - Adjust ActionTraceSettings defaults (MaxEvents: 800, HotEventCount: 150) Co-Authored-By: Claude Opus 4.5 <[email protected]>
…ctionTraceEditorWindow
… clarity and consistency
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @whatevertogo, your pull request is larger than the review limit of 300000 diff characters
📝 WalkthroughWalkthroughIntroduces a comprehensive ActionTrace system for the Unity editor, capturing operation history with event persistence, semantic analysis (importance scoring, categorization, intent inference), context mapping, query capabilities, UI visualization, and integrated tools including undo support, note recording, and Python server resources. Changes
Sequence Diagram(s)sequenceDiagram
actor User as Editor User
participant Unity as Unity Editor
participant Hooks as HookRegistry
participant Capture as Capture Points
participant EventStore as EventStore
participant Query as Query Engine
participant Semantics as Semantic Analysis
participant UI as ActionTrace Window
User->>Unity: Performs action (e.g., add component)
Unity->>Hooks: EditorCallback fires
Hooks->>Capture: Notify subscribers
Capture->>Capture: Apply filters & sampling
Capture->>EventStore: Record event
EventStore->>EventStore: Merge/dehydrate as configured
User->>UI: Opens ActionTrace window
UI->>EventStore: Query(limit=50)
EventStore->>EventStore: Build view snapshot
EventStore-->>UI: Events list
UI->>Query: Project events for display
Query->>Semantics: Score importance
Semantics-->>Query: Importance score
Query->>Semantics: Infer intent
Semantics-->>Query: Intent string
Query->>UI: ViewItems (with scores, category, intent, colors)
UI->>UI: Render list with styling
User->>UI: Click event for details
UI->>UI: Expand detail panel
UI->>UI: Display semantics, context, metadata
sequenceDiagram
actor AI as AI Agent (Python)
participant PythonTool as Python Tool
participant Transport as Unity Transport
participant CSharp as C# Tool Handler
participant EventStore as EventStore
participant ContextStack as ContextStack
AI->>PythonTool: Call get_action_trace(task_id)
PythonTool->>Transport: Send command with params
Transport->>CSharp: Invoke GetActionTraceTool.HandleCommand
CSharp->>EventStore: Query(limit, minImportance, filters)
EventStore-->>CSharp: Recent events
CSharp->>CSharp: Filter by task_id if provided
CSharp-->>PythonTool: Serialized EventQueryResult
PythonTool->>PythonTool: Parse into EditorEvent models
PythonTool-->>AI: ActionTrace data (events with semantics)
AI->>PythonTool: Call add_action_trace_note(note, task_id)
PythonTool->>Transport: Send AINote event with context
Transport->>CSharp: Invoke AddActionTraceNoteTool.HandleCommand
CSharp->>ContextStack: MarkAsAiOperation(agentId, sessionId)
ContextStack-->>CSharp: Disposable scope
CSharp->>EventStore: Record AINote event
EventStore->>EventStore: Create ContextMapping
EventStore-->>CSharp: Recorded sequence
CSharp-->>PythonTool: Success response with sequence
PythonTool-->>AI: Confirmation
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Rationale: This is a substantial, multi-faceted feature introducing 50+ new files with dense, interconnected logic spanning event capture, storage, semantic analysis, context tracking, UI rendering, query engines, compression, aggregation, and Python/C# bridging. The changes exhibit high heterogeneity (interfaces, implementations, configurations, nested types, helpers, tools, and UI), multiple architectural layers, complex state management (sampling middleware, context stacks, event merging), and intricate query/projection pipelines. Although individual file logic is often localized, the cumulative scope, architectural significance, and variety of patterns require comprehensive review across capture, semantic, storage, UI, and integration subsystems. Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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 (2)
MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs (1)
76-121: Claude Code stdio path leavestypestale or missing.When switching from HTTP to stdio for Claude Code,
typeis never updated to"stdio"and the removal guard preserves any old"http"value, which can misconfigure the client.🔧 Proposed fix (set type for Claude Code in both branches)
private static void PopulateUnityNode(JObject unity, string uvPath, McpClient client, bool isVSCode) { + bool isClaudeCode = string.Equals(client?.name, "Claude Code", StringComparison.OrdinalIgnoreCase); // Get transport preference (default to HTTP) bool prefValue = EditorPrefs.GetBool(EditorPrefKeys.UseHttpTransport, true); bool clientSupportsHttp = client?.SupportsHttpTransport != false; bool useHttpTransport = clientSupportsHttp && prefValue; @@ - if (isVSCode) + if (isVSCode || isClaudeCode) { unity["type"] = "http"; } - // Also add type for Claude Code (uses mcpServers layout but needs type field) - else if (client?.name == "Claude Code") - { - unity["type"] = "http"; - } } else { @@ - if (isVSCode) + if (isVSCode || isClaudeCode) { unity["type"] = "stdio"; } } // Remove type for non-VSCode clients (except Claude Code which needs it) - if (!isVSCode && client?.name != "Claude Code" && unity["type"] != null) + if (!isVSCode && !isClaudeCode && unity["type"] != null) { unity.Remove("type"); }MCPForUnity/Editor/Helpers/ExecPath.cs (1)
245-273: Avoid adding current directory to PATH whenprependPathis empty.When
prependPathis empty, the constructed PATH becomes:<existing>, which implicitly adds the current directory to PATH. That can causewhichto resolve a project-local executable unintentionally.🛠️ Safer PATH handling
string path = Environment.GetEnvironmentVariable("PATH") ?? string.Empty; - psi.EnvironmentVariables["PATH"] = string.IsNullOrEmpty(path) ? prependPath : (prependPath + Path.PathSeparator + path); + if (!string.IsNullOrEmpty(prependPath)) + { + psi.EnvironmentVariables["PATH"] = string.IsNullOrEmpty(path) + ? prependPath + : (prependPath + Path.PathSeparator + path); + }
🤖 Fix all issues with AI agents
In `@MCPForUnity/Editor/ActionTrace/Capture/PropertyChangeTracker.cs`:
- Around line 44-69: The PropertyChangeTracker class is never initialized
because it lacks the Unity Editor initialization attribute; add
[InitializeOnLoad] above the PropertyChangeTracker class declaration so its
static constructor runs and subscribes Undo.postprocessModifications and calls
ScheduleNextFlush(); ensure the attribute is placed directly before the class
definition to mirror SelectionPropertyTracker's initialization behavior so the
static constructor, Undo.postprocessModifications += mods =>
ProcessModifications(mods), and ScheduleNextFlush() execute on editor startup.
In `@MCPForUnity/Editor/ActionTrace/Capture/SamplingMiddleware.cs`:
- Around line 47-148: ShouldRecord currently stores the latest event for
SamplingMode.Debounce/DebounceByKey but never emits it; add a trailing-edge
flush: create a scheduled flush (e.g., using
UnityEditor.EditorApplication.delayCall or EditorApplication.update) that calls
a new FlushExpiredPendingSamples method which iterates _pendingSamples, finds
entries whose TimestampMs + strategy.WindowMs <= now, removes them and
emits/records their PendingSample.Event (reuse existing CleanupExpiredSamples as
a template or extend it to return expired items), and ensure ScheduleNextFlush
is called whenever you add/update a PendingSample so a flush will run after the
earliest window expires; also add the UnityEditor using and update references to
PendingSample, _pendingSamples, ShouldRecord, SamplingMode.Debounce and
SamplingMode.DebounceByKey accordingly.
In `@MCPForUnity/Editor/ActionTrace/Capture/SelectionPropertyTracker.cs`:
- Around line 104-165: The event currently uses _currentSelectionGlobalId as the
EditorEvent targetId which hides the actual modified object's identity; change
the call to RecordSelectionPropertyModified to pass the targetGlobalId (add a
string targetGlobalId parameter), update the RecordSelectionPropertyModified
signature to accept that id, and use that passed targetGlobalId as the
EditorEvent targetId when constructing evt; keep the existing selection_context
payload entries unchanged so selection info remains in the payload while the
event targetId preserves the real modified-object id.
In `@MCPForUnity/Editor/ActionTrace/Core/EventStore.cs`:
- Around line 226-235: The Clear() method currently only clears
_events/_contextMappings and resets _sequenceCounter but must also reset merge
tracking and pending notifications to avoid dropped events; update Clear() to
set _lastRecordedEvent to null, reset any merge-tracking fields (e.g.,
_mergeState, _mergeCandidates or similar flags used by the merge logic) and
clear any pending notification collections (e.g., _pendingNotifications or
pending operation queues) so all merge/notification state is fully reset after
Clear().
- Around line 75-147: The bug is that _lastRecordedEvent and _lastRecordedTime
are set before the merge logic, causing ShouldMergeWithLast to compare the
incoming event to itself; update Record so that assignment to _lastRecordedEvent
and _lastRecordedTime happens inside the _queryLock after you either call
MergeWithLastEventLocked or add evtWithSequence to _events (i.e., move the two
assignments to immediately after the merge/add block and before releasing the
lock), ensuring ShouldMergeWithLast and MergeWithLastEventLocked see the true
previous event.
In `@MCPForUnity/Editor/ActionTrace/Helpers/ActionTraceHelper.cs`:
- Around line 171-182: GetPreviousValue currently returns the
PropertyModification wrapper because it checks "previousValue" before
"previousValue.value"; change the method so it returns the actual previous
value: first try GetNestedValue(undoMod, "previousValue.value"), then try
GetNestedValue(undoMod, "previousValue"), and if the latter returns a
PropertyModification wrapper (inspect its type/name), extract its inner value
(e.g., via GetNestedValue on the returned object for "value" or use reflection
to read the "value" field) so callers receive the raw previous value; update
GetPreviousValue and use GetNestedValue/PropertyModification handling
consistently for UndoPropertyModification results.
In `@MCPForUnity/Editor/ActionTrace/Query/TransactionAggregator.cs`:
- Around line 100-139: ShouldSplit currently only splits when current has tool
metadata and first lacks it, making boundaries asymmetric; update ShouldSplit
(referencing GetToolCallId and GetTriggeredByTool and the local variables
firstToolCallId, currentToolCallId, firstTool, currentTool) to treat any
mismatch (non-empty vs empty) as a split in both priority checks: if
firstToolCallId is non-empty and currentToolCallId is empty, return true (same
for the reverse which already exists), and likewise if firstTool is non-empty
and currentTool is empty, return true; keep the existing time-window fallback
(transactionWindowMs) unchanged.
In `@MCPForUnity/Editor/ActionTrace/Semantics/DefaultIntentInferrer.cs`:
- Around line 16-63: Infer currently calls IsReparenting(surrounding) and
IsBatchOperation(surrounding) but doesn't guard for surrounding being null
(ActionTraceQuery passes surrounding: null), causing a NullReferenceException;
normalize surrounding to an empty IReadOnlyList<EditorEvent> at the start of the
Infer method (before the switch) so downstream helpers IsReparenting and
IsBatchOperation always receive a non-null list, or alternatively add null
handling inside those helper methods, ensuring neither dereferences a null
reference.
In `@MCPForUnity/Editor/ActionTrace/VCS/VcsContextProvider.cs`:
- Around line 54-205: RefreshContext/QueryGitStatus currently invoke three
synchronous git calls on the Editor update loop via RunGitCommand, which can
block the editor; move Git polling off the main thread by executing
QueryGitStatus (or the git invocations inside RunGitCommand) on a background
Task/Thread and marshal the resulting VcsContext back to the main thread (e.g.,
via EditorApplication.delayCall or scheduling _currentContext update in
OnUpdate) so the Editor is never blocked. Also harden RunGitCommand to use a
Process wait-with-timeout (WaitForExit(timeoutMs)), kill the process on timeout,
and return null/error instead of blocking. Finally, change
VcsContext.ToDictionary so it does not return a cached mutable Dictionary
instance—return a new Dictionary copy or an
IReadOnlyDictionary/ReadOnlyDictionary wrapper each call to avoid shared-mutable
state.
In `@MCPForUnity/Editor/Helpers/AssetPathUtility.cs`:
- Line 164: Remove the debug logging that exposes potentially sensitive data by
deleting the McpLog.Info call that logs sourceOverride in AssetPathUtility (the
line containing McpLog.Info($"[DEBUG] Using override: {sourceOverride}"));
ensure no other statements reference or re-log sourceOverride in the same method
so the override value is not written to logs.
- Line 161: Remove the unconditional debug log that prints sensitive data:
delete the McpLog.Info call that logs EditorPrefKeys.GitUrlOverride and the
sourceOverride value in AssetPathUtility.cs (the line referencing
McpLog.Info($"[DEBUG] GitUrlOverride key: '{EditorPrefKeys.GitUrlOverride}',
value: '{sourceOverride}'")). Ensure no other code in the same method/class logs
the raw GitUrlOverride value; if you need retain diagnostics, replace with a
non-sensitive message or guard it behind a secure debug flag that never runs in
production.
In `@MCPForUnity/Editor/Resources/ActionTrace/ActionTraceViewResource.cs`:
- Around line 165-252: QueryWithContext currently ignores taskId and
conversationId when includeSemantics (and context-only) is true; modify
QueryWithContext to filter eventsWithContext by the provided taskId and/or
conversationId before calling ActionTraceQuery.ProjectWithContext or before
applying the DefaultEventScorer. Specifically, apply the task/conversation
predicates to the collection returned by EventStore.QueryWithContext(limit,
sinceSequence) (use the local variable eventsWithContext) so both the projected
variable (used by ProjectWithContext) and the filtered variable in the else
branch only contain events matching taskId/conversationId; keep the subsequent
semantic projection (ActionTraceQuery.ProjectWithContext) and scoring
(DefaultEventScorer.Score) logic unchanged.
- Around line 366-405: Apply a null-check for EditorEvent.Payload inside
ApplyTaskFilters to avoid NullReferenceException for dehydrated AINote events:
when e.Type == "AINote" and either taskId or conversationId is specified, first
verify e.Payload != null and treat a null payload as a non-match (i.e., return
false) before calling Payload.TryGetValue for "task_id" or "conversation_id";
ensure the existing logic for comparing eventTaskId and eventConvId remains the
same but only runs after the payload null guard.
- Around line 90-118: In QueryBasic replace the call to
EventSummarizer.Summarize(e) with the event's own retrieval method
(e.GetSummary()) so dehydrated events return their PrecomputedSummary; update
the projection in QueryBasic (the anonymous object construction) to use
e.GetSummary() for the summary field and apply the same change to any other
query paths that currently call EventSummarizer.Summarize to ensure precomputed
summaries are preserved.
In `@MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs`:
- Around line 193-207: OnBeforeAssemblyReload currently calls
WriteHeartbeat(true, "domain_reload") then Stop(), but Stop() unconditionally
deletes the heartbeat/status file; fix by setting an explicit "reloading" flag
(e.g. a new static bool or a static property like s_isReloading) before calling
Stop() in OnBeforeAssemblyReload, update the ResumeStdioAfterReload EditorPref
deletion logic to check both EditorApplication.isCompiling and this new
s_isReloading flag, and guard the status-file deletion in Stop() (or the helper
that removes the file) to skip deletion when s_isReloading is true so the
heartbeat remains across the domain reload detection window; ensure symbol names
referenced: OnBeforeAssemblyReload, WriteHeartbeat, Stop,
ResumeStdioAfterReload, and EditorApplication.isCompiling.
- Around line 318-395: The CleanupZombieListeners method currently treats read
timeouts as fatal because stream.Read can throw IOException on ReceiveTimeout;
modify CleanupZombieListeners to wrap the stream.Read(...) call in a
try-catch(IOException) and treat that path the same as “no handshake” (log as
zombie and continue cleanup), and ensure you check bytesRead > 0 before decoding
the buffer; also avoid blocking the editor by reducing/blocking waits (replace
connectTask.Wait(500) and subsequent Thread.Sleep loops with short non-blocking
attempts or shorter timeouts when calling ConnectAsync on testClient/forceClient
and bail earlier) — update references in the method around
testClient.ConnectAsync, testClient.ReceiveTimeout, stream.Read, and the
forceClient ConnectAsync loop.
In `@MCPForUnity/Editor/Tools/AddTimelineNoteTool.cs`:
- Around line 29-30: The McpForUnityTool attribute on the AddActionTraceNoteTool
class lacks a Description parameter required for MCP clients; update the
attribute declaration on the AddActionTraceNoteTool class to include a
descriptive Description="..." argument (e.g., a short summary of the tool's
purpose) so MCP can parse the tool reliably, ensuring the Description is concise
and accurately reflects the tool's behavior.
In `@MCPForUnity/Editor/Tools/GetActionTraceTool.cs`:
- Around line 14-15: The McpForUnityTool attribute on the GetActionTraceTool
class is missing a Description property needed for MCP client parsing; update
the attribute usage on the GetActionTraceTool class (the
[McpForUnityTool("get_action_trace")] declaration) to include a Description =
"Get action trace for a specific entity/action" (or similar brief description)
so the attribute becomes [McpForUnityTool("get_action_trace", Description = "Get
action trace for a specific entity/action")].
♻️ Duplicate comments (3)
MCPForUnity/Editor/Resources/ActionTrace/ActionTraceViewResource.cs (3)
126-159: Same summary handling applies here.
Switch top.Event.GetSummary()to preserve dehydrated summaries.
411-445: Same null‑payload risk applies here.
Guardp.Event.Payloadbefore accessing task/conversation IDs.
450-484: Same null‑payload risk applies here.
Guardx.Event.Payloadbefore accessing task/conversation IDs.
🟡 Minor comments (25)
Server/src/services/tools/add_action_trace_note.py-113-122 (1)
113-122: Persist auto-generated task/conversation IDs to context.Right now, if callers omit IDs, each call gets a new
task_id/conversation_id, which defeats implicit grouping across a session. If the intended behavior is “generate once, then reuse,” set the ContextVars when you auto-generate.🔧 Suggested fix
if not effective_task_id: effective_task_id = f"task-{uuid.uuid4().hex[:8]}" + _current_task_id.set(effective_task_id) if not effective_conv_id: effective_conv_id = f"conv-{uuid.uuid4().hex[:8]}" + _current_conversation_id.set(effective_conv_id)Server/src/services/tools/add_action_trace_note.py-151-151 (1)
151-151: Preserve MCPResponse error details instead of stringifying.
async_send_command_with_retrycan return anMCPResponsewhich haserror,hint, anddatafields. The current code converts it to a string, losing these structured fields. Usemodel_dump()to convert to dict instead.🔧 Suggested 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)}Server/src/services/tools/undo_to_sequence.py-91-99 (1)
91-99: Preserve structured error details from non-dict responses.When Unity returns an
MCPResponse, collapsing it to{"message": str(response)}loses structured fields likeerror/hint. Consider normalizing it to a dict to keep a consistent schema.🩹 Suggested normalization
-from transport.unity_transport import send_with_unity_instance +from transport.unity_transport import send_with_unity_instance, normalize_unity_response ... - return response if isinstance(response, dict) else {"success": False, "message": str(response)} + return response if isinstance(response, dict) else normalize_unity_response(response)Server/src/services/tools/undo_to_sequence.py-58-61 (1)
58-61: Update the docstring example to use the correct tool name.Line 60 references
get_actionTrace, but the actual tool name isget_action_trace(snake_case). Update the example to:result = await get_action_trace(ctx, limit=1)Server/src/services/tools/undo_to_sequence.py-72-78 (1)
72-78: Add validation to guard against negative sequence_id values.
coerce_intaccepts negative values, but ActionTrace sequences are monotonically increasing and therefore must be non-negative. The C# handler's EventStore query may behave unexpectedly or cause issues with negativesinceSequencevalues. Add a validation check before sending to the C# handler.🩹 Suggested validation
- if coerced_sequence is None: + if coerced_sequence is None or coerced_sequence < 0: return { "success": False, - "message": "sequence_id parameter is required and must be a number." + "message": "sequence_id parameter is required and must be a non-negative number." }MCPForUnity/Editor/ActionTrace/Integration/ManageAssetBridge.cs-37-74 (1)
37-74: Include full exception details in warnings.Logging only
ex.Messagedrops stack traces and inner exception details, which makes ActionTrace failures harder to diagnose.🛠️ Proposed fix
- McpLog.Warn($"[ManageAssetBridge] Failed to record asset modification: {ex.Message}"); + McpLog.Warn($"[ManageAssetBridge] Failed to record asset modification: {ex}"); ... - McpLog.Warn($"[ManageAssetBridge] Failed to record asset creation: {ex.Message}"); + McpLog.Warn($"[ManageAssetBridge] Failed to record asset creation: {ex}"); ... - McpLog.Warn($"[ManageAssetBridge] Failed to record asset deletion: {ex.Message}"); + McpLog.Warn($"[ManageAssetBridge] Failed to record asset deletion: {ex}");MCPForUnity/Editor/ActionTrace/Descriptors/IEventDescriptor.cs-67-82 (1)
67-82: MakeGetTargetNamenull-safe for payload values.If a payload key exists but the value is null,
ToString()will throw. Use null-conditional conversion to avoid a hard crash.🔧 Proposed fix
- if (evt.Payload.TryGetValue("name", out var name)) - return name.ToString(); + if (evt.Payload.TryGetValue("name", out var name)) + return name?.ToString() ?? string.Empty; - if (evt.Payload.TryGetValue("game_object", out var goName)) - return goName.ToString(); + if (evt.Payload.TryGetValue("game_object", out var goName)) + return goName?.ToString() ?? string.Empty; - if (evt.Payload.TryGetValue("scene_name", out var sceneName)) - return sceneName.ToString(); + if (evt.Payload.TryGetValue("scene_name", out var sceneName)) + return sceneName?.ToString() ?? string.Empty; - if (evt.Payload.TryGetValue("component_type", out var componentType)) - return componentType.ToString(); + if (evt.Payload.TryGetValue("component_type", out var componentType)) + return componentType?.ToString() ?? string.Empty; - if (evt.Payload.TryGetValue("path", out var path)) - return path.ToString(); + if (evt.Payload.TryGetValue("path", out var path)) + return path?.ToString() ?? string.Empty;MCPForUnity/Editor/ActionTrace/Semantics/IIntentInferrer.cs-11-20 (1)
11-20: Fix return type to match documented nullable contract.The method documentation states it may return null, but the signature returns non-nullable
string. The implementation (DefaultIntentInferrer) explicitly returnsnullwhen unable to infer. Update the signature tostring?to align with the documented contract and actual behavior.🔧 Proposed fix
- string Infer(EditorEvent evt, IReadOnlyList<EditorEvent> surrounding); + string? Infer(EditorEvent evt, IReadOnlyList<EditorEvent> surrounding);MCPForUnity/Editor/ActionTrace/Core/GlobalIdHelper.cs-94-100 (1)
94-100: Guard against empty scene paths for unsaved scenes.
Line 95–99: For unsaved scenes,go.scene.pathis empty, producing IDs likeScene:@Foo``.ParseFallbackIdthen calls `GetSceneByPath("")`, which will fail, so the ID can’t resolve even within the same session. Consider falling back to Asset/Instance IDs when the scene path is empty.🔧 Suggested fix
- if (obj is GameObject go && go.scene.IsValid()) + if (obj is GameObject go && go.scene.IsValid() && !string.IsNullOrEmpty(go.scene.path)) { // Reuse GameObjectLookup.GetGameObjectPath() string hierarchyPath = GameObjectLookup.GetGameObjectPath(go); return $"{ScenePrefix}{go.scene.path}{PathSeparator}{hierarchyPath}"; }MCPForUnity/Editor/ActionTrace/Helpers/ActionTraceHelper.cs-28-39 (1)
28-39: Fix FormatToolName output to match the documented examples.
Line 33–38: The current regex removes underscores without inserting spaces and doesn’t title-case the first segment, so"manage_gameobject"becomes"manageGameobject"instead of"Manage GameObject"(per docs). Consider splitting and joining with spaces.🔧 Suggested fix
- return System.Text.RegularExpressions.Regex.Replace( - toolName, - "_([a-z])", - match => match.Groups[1].Value.ToUpper() - ); + var parts = toolName.Split('_', StringSplitOptions.RemoveEmptyEntries); + for (int i = 0; i < parts.Length; i++) + { + var part = parts[i]; + if (part.Length == 0) continue; + parts[i] = char.ToUpperInvariant(part[0]) + part.Substring(1); + } + return string.Join(" ", parts);MCPForUnity/Editor/ActionTrace/Semantics/DefaultEventScorer.cs-64-67 (1)
64-67: BuildCompleted score conflicts with its comment.The comment says success is less important than failure, but
BuildCompletedis scored1.0f(critical). Consider lowering the score or updating the comment.💡 Possible fix (align to comment)
- EventTypes.BuildCompleted => 1.0f, + EventTypes.BuildCompleted => 0.9f,MCPForUnity/Editor/Tools/ManageAsset.cs-31-38 (1)
31-38: Documented change map doesn’t match emitted payload.
The XML doc says the changes dictionary contains{old,new}, butModifyAssetforwards only the incoming properties (see Line 499-503). Either compute diffs or update the contract to avoid misleading consumers.✏️ Minimal doc fix (if raw properties are intended)
- /// - changesDictionary: property path -> {old, new} values + /// - changesDictionary: property path -> new values (or update implementation to capture old/new)MCPForUnity/Editor/Tools/ManageAsset.cs-291-294 (1)
291-294: Folder creations don’t trigger OnAssetCreated.
CreateAssetreturnsCreateFolderearly andcreate_folderalso callsCreateFolder, so the callback never fires for folders. Consider emitting the event on successful folder creation.🔧 Suggested hook in CreateFolder (success path)
// AssetDatabase.Refresh(); // CreateFolder usually handles refresh + OnAssetCreated?.Invoke(fullPath, "Folder"); return new SuccessResponse(MCPForUnity/Editor/Tools/GetActionTraceTool.cs-34-44 (1)
34-44: Remove unused EventTypes and IncludePayload parameters or implement their functionality.
These parameters are defined inGetActionTraceTool.Parameters(lines 34-44) but are never parsed or used inActionTraceViewResource.HandleCommand. The resource documentation does not list them as supported parameters. Either implement filtering by event types and payload inclusion logic, or remove these parameters to avoid misleading clients.MCPForUnity/Editor/ActionTrace/Core/EventStore.Persistence.cs-156-173 (1)
156-173: Missing lock inSaveToStoragecould cause concurrent modification issues.
SaveToStoragereads from_eventsand_contextMappingswithout holding_queryLock, while other methods likeTrimToMaxEventsLimitandQueryuse the lock. If a save occurs concurrently with event recording or trimming,.ToList()could throwInvalidOperationException.Suggested fix
private static void SaveToStorage() { try { + EventStoreState state; + lock (_queryLock) + { - var state = new EventStoreState + state = new EventStoreState - { + { - SchemaVersion = CurrentSchemaVersion, + SchemaVersion = CurrentSchemaVersion, - SequenceCounter = _sequenceCounter, + SequenceCounter = _sequenceCounter, - Events = _events.ToList(), + Events = _events.ToList(), - ContextMappings = _contextMappings.ToList() + ContextMappings = _contextMappings.ToList() - }; + }; + } McpJobStateStore.SaveState(StateKey, state); }MCPForUnity/Editor/Tools/UndoToSequenceTool.cs-133-142 (1)
133-142: Undo operations may silently fail without feedback.
Undo.PerformUndo()doesn't return a success indicator. If the undo history was cleared (e.g., by domain reload), the loop runs but accomplishes nothing. The user receives a "Successfully reverted X steps" message that may be misleading.Additionally,
MarkSceneDirtyis called unconditionally, which will mark the scene dirty even if no actual changes were reverted.Suggested improvement
Consider tracking the undo group before and after to detect if operations actually occurred:
+ int groupBefore = Undo.GetCurrentGroup(); for (int i = 0; i < stepsToUndo; i++) { Undo.PerformUndo(); } + int groupAfter = Undo.GetCurrentGroup(); + int actualSteps = groupBefore - groupAfter; // Force GUI refresh to update the scene - EditorApplication.delayCall += () => + if (actualSteps > 0) { - UnityEditor.SceneManagement.EditorSceneManager.MarkSceneDirty(UnityEditor.SceneManagement.EditorSceneManager.GetActiveScene()); - }; + EditorApplication.delayCall += () => + { + UnityEditor.SceneManagement.EditorSceneManager.MarkSceneDirty( + UnityEditor.SceneManagement.EditorSceneManager.GetActiveScene()); + }; + }MCPForUnity/Editor/ActionTrace/Capture/UndoGroupManager.cs-50-69 (1)
50-69: Missing re-entrancy guard could cause lost undo group context.If
BeginToolCallis called twice without an interveningEndToolCall, the first call's_currentUndoGroupStartis silently overwritten. This would prevent proper undo collapsing for the first tool call.Suggested fix
public static void BeginToolCall(string toolName, string toolCallId) { + if (_isInToolCall) + { + UnityEngine.Debug.LogWarning( + $"[UndoGroupManager] BeginToolCall({toolName}) called while already in tool call ({_currentToolName}). " + + "Ending previous tool call first."); + EndToolCall(); + } + if (string.IsNullOrEmpty(toolName)) {MCPForUnity/Editor/ActionTrace/Capture/EventFilter.cs-48-75 (1)
48-75: Non-extension entries inJunkExtensionswill not filter correctly.Several entries in
JunkExtensionsare not file extensions:
"~$"(Line 61) is a filename prefix pattern for Office temp files, not a suffix.".DS_Store","Thumbs.db","desktop.ini"(Lines 64-66) are complete filenames.The
EndsWithcheck on Line 127 won't match paths likefolder/~$document.docxorfolder/.DS_Storecorrectly—only paths literally ending with these strings (e.g.,something.DS_Store) would match.Suggested fix
Move these to separate collections with appropriate matching logic:
+ /// <summary> + /// Filename patterns to filter (matched at end of path after last separator). + /// </summary> + private static readonly HashSet<string> JunkFilenames = new(StringComparer.OrdinalIgnoreCase) + { + ".DS_Store", + "Thumbs.db", + "desktop.ini" + }; + + /// <summary> + /// Filename prefix patterns (e.g., Office temp files starting with ~$). + /// </summary> + private static readonly HashSet<string> JunkFilenamePrefixes = new(StringComparer.OrdinalIgnoreCase) + { + "~$" + };Then add matching logic in
IsJunkPath:// Extract filename and check against junk filenames/prefixes string filename = Path.GetFileName(path); if (JunkFilenames.Contains(filename)) return true; foreach (var prefix in JunkFilenamePrefixes) { if (filename.StartsWith(prefix, StringComparison.OrdinalIgnoreCase)) return true; }MCPForUnity/Editor/ActionTrace/Core/ActionTraceSettings.cs-183-198 (1)
183-198: "All Events (Debug)" and "Low+" presets are functionally identical.Both buttons call
SetImportance(0f), making "Low+" redundant. Either differentiate the values or remove the duplicate.🐛 Proposed fix - differentiate or remove
if (GUILayout.Button("All Events (Debug)")) { SetImportance(0f); } - if (GUILayout.Button("Low+")) - { - SetImportance(0f); - } + if (GUILayout.Button("Low+")) + { + SetImportance(0.2f); // Or remove this button entirely + }MCPForUnity/Editor/ActionTrace/Core/EventStore.Merging.cs-92-97 (1)
92-97: Unsafe cast ofmerge_countvalue could throw InvalidCastException.If
existingCountis stored as along(e.g., from JSON deserialization), the direct cast(int)existingCountwill fail. Use safe conversion instead.🐛 Proposed fix using Convert.ToInt32
- if (_lastRecordedEvent.Payload.TryGetValue("merge_count", out var existingCount)) - { - mergeCount = (int)existingCount + 1; - } + if (_lastRecordedEvent.Payload.TryGetValue("merge_count", out var existingCount) + && existingCount != null) + { + mergeCount = Convert.ToInt32(existingCount) + 1; + }MCPForUnity/Editor/ActionTrace/Core/EventStore.Context.cs-27-40 (1)
27-40: Optimization comment claims mappings are ordered, but order isn't enforced.Line 37 states "mappings are ordered by EventSequence" as an optimization, but
AddContextMappingsimply appends to the list without maintaining sort order. If events are recorded out of order (e.g., from async sources), this early-exit optimization could skip newer duplicates.🐛 Option 1: Remove the optimization and always scan fully
for (int i = _contextMappings.Count - 1; i >= 0; i--) { var existing = _contextMappings[i]; if (existing.EventSequence == mapping.EventSequence && existing.ContextId == mapping.ContextId) { isDuplicate = true; break; } - // Optimization: mappings are ordered by EventSequence - if (existing.EventSequence < mapping.EventSequence) - break; }MCPForUnity/Editor/ActionTrace/Core/EventStore.Context.cs-144-147 (1)
144-147: QueryWithContext loses multiple contexts per event.The
GroupBy(...).ToDictionary(..., g.FirstOrDefault())pattern keeps only the first context mapping per event sequence. Per the docstring at line 18-19, "Multiple mappings allowed for same eventSequence (different contexts)". Consider returning all mappings or documenting that only one is returned.🐛 Option: Return ILookup to preserve all mappings
- var mappingBySequence = mappingsSnapshot - .GroupBy(m => m.EventSequence) - .ToDictionary(g => g.Key, g => g.FirstOrDefault()); + var mappingsBySequence = mappingsSnapshot.ToLookup(m => m.EventSequence);Then update the select to return all contexts:
.Select(e => (Event: e, Contexts: mappingsBySequence[e.Sequence].ToList()))This would require changing the return type signature.
MCPForUnity/Editor/ActionTrace/Core/ActionTraceSettings.cs-14-14 (1)
14-14: Move settings file to a dedicated subfolder for better organization.The settings file is created at
Assets/ActionTraceSettings.asset, which places it at the root of the Assets folder. Consider moving it to a dedicated subfolder such asAssets/Editor/ActionTraceSettings.assetorAssets/Resources/ActionTraceSettings.asset(if runtime-accessible) to keep editor configuration organized and separate from user-created assets.MCPForUnity/Editor/ActionTrace/Descriptors/ComponentEventDescriptor.cs-46-53 (1)
46-53: Timestamp generated at extraction time may be inconsistent.
ExtractPayloadgenerates a new timestamp on every invocation rather than extracting it from the raw payload or the event's actual timestamp. If this method is called multiple times or at a later point, the timestamp won't reflect when the hierarchy actually changed.Consider extracting from
rawPayloadif available, or relying on the event'sTimestampUnixMs:Suggested fix
public override Dictionary<string, object> ExtractPayload(Dictionary<string, object> rawPayload) { return new Dictionary<string, object> { - ["timestamp"] = DateTimeOffset.UtcNow.ToUnixTimeMilliseconds() + ["timestamp"] = rawPayload?.GetValueOrDefault("timestamp", DateTimeOffset.UtcNow.ToUnixTimeMilliseconds()) + ?? DateTimeOffset.UtcNow.ToUnixTimeMilliseconds() }; }MCPForUnity/Editor/ActionTrace/Core/EditorEvent.cs-24-24 (1)
24-24: Potential thread-safety issue with lazy PrecomputedSummary mutation.The class is documented as immutable, but
GetSummary()mutatesPrecomputedSummaryvia the private setter. If multiple threads callGetSummary()concurrently on the same instance, there's a data race. While Unity editor code is typically single-threaded, this could cause issues if events are accessed from background threads (e.g., during persistence or MCP server calls).Consider using
Interlocked.CompareExchangeor accepting the benign race (since string assignment is atomic and all threads would compute the same value).🔒 Thread-safe lazy initialization option
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; + var summary = PrecomputedSummary; + if (summary != null) + return summary; + + // Compute and cache - benign race if multiple threads compute simultaneously + summary = ComputeSummary(); + PrecomputedSummary = summary; + return summary; }Also applies to: 76-76, 162-170
MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs
Outdated
Show resolved
Hide resolved
…uery improvements
This commit introduces significant improvements to the ActionTrace system:
**Layered Settings Architecture:**
- Refactor ActionTraceSettings into layered structure (Filtering, Merging, Storage, Sampling)
- Add custom Editor with foldout sections for better UX
- Move GlobalIdHelper from Core/ to Helpers/ for better organization
**Preset System:**
- Add ActionTracePreset with Standard/Verbose/Minimal/Silent presets
- Enable quick configuration switching via ApplyPreset()
- Include preset descriptions and memory estimates
**Configurable Filtering:**
- Transform EventFilter from static blacklist to rule-based system
- Support Prefix, Extension, Regex, and GameObject rule types
- Add EventFilterSettings for persistence and customization
**Stable Cross-Session IDs:**
- Use GlobalIdHelper for all GameObject/Component event TargetIds
- Use Asset:{path} format for asset events
- Ensure TargetIds remain stable across domain reloads
**Query & Analysis Enhancements:**
- Add EventQueryBuilder for fluent query API
- Add ContextCompressor for event data optimization
- Add EventStatistics for comprehensive analytics
- Enhance EventSummarizer with better grouping
**Capture Layer Improvements:**
- Add AssetChangePostprocessor for asset change tracking
- Add SamplingMiddleware for high-frequency event throttling
- Add ToolCallScope for MCP tool call tracking
- Enhance UnityEventHooks with comprehensive event coverage
**UI/UX Improvements:**
- Add sort modes (ByTimeDesc, AIFiltered) to ActionTraceEditorWindow
- Improve filter menu with "All" option
- Add tool descriptions for better AI discoverability
**Helper Classes:**
- Add GameObjectTrackingHelper for GameObject lifecycle tracking
- Add UndoReflectionHelper for Undo system introspection
- Add BuildTargetUtility for build target detection
Co-Authored-By: Claude Opus 4.5 <[email protected]>
…al loops and enhancing GameObject tracking
…capacity - Add Range attributes to settings fields with Inspector sliders: - MinImportanceForRecording: [Range(0f, 1f)] - MergeWindowMs: [Range(0, 5000)] - TransactionWindowMs: [Range(100, 10000)] - HotEventCount: [Range(10, 1000)] - MaxEvents: [Range(100, 5000)] (already existed) - Change ContextMappings from fixed 2000 to dynamic MaxEvents × 2 - Remove redundant validation code (now enforced by Range attributes) - Fix UndoReflectionHelper.GetPreviousValue to try nested value first - Clean up StdioBridgeHost: remove ineffective CleanupZombieListeners - Add task/conversation filtering to ActionTraceViewResource - Preserve modified-object identity in SelectionPropertyTracker Co-Authored-By: Claude Opus 4.5 <[email protected]>
…on in ActionTrace
…leaks Replace infinite recursive delayCall patterns with EditorApplication.update in PropertyChangeTracker and SamplingMiddleware. This ensures proper cleanup on domain reload and prevents memory leaks. Changes: - PropertyChangeTracker: Replace delayCall with update + FlushCheck() - SamplingMiddleware: Replace delayCall with update directly - Remove redundant helper methods (use UndoReflectionHelper/PropertyFormatter) - Clean up unused using statements Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Sorry @whatevertogo, your pull request is larger than the review limit of 300000 diff characters
There was a problem hiding this comment.
Actionable comments posted: 10
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/Capture/AssetChangePostprocessor.cs`:
- Around line 103-128: The code emits duplicate events for the same imported
assets because importedAssets are processed earlier to emit AssetImported (via
RecordEvent) and this later loop emits AssetModified for the same entries;
either remove this AssetModified loop or make it mutually exclusive by using
IsNewlyCreatedAsset correctly so an asset only triggers
AssetImported/AssetCreated OR AssetModified; update the logic around
importedAssets, IsNewlyCreatedAsset, ShouldTrackModification and RecordEvent (or
adjust ShouldTrackModification) to ensure only one event type is recorded per
asset.
In `@MCPForUnity/Editor/ActionTrace/Capture/PropertyChangeTracker.cs`:
- Around line 127-133: The pending-change cap check in PropertyChangeTracker is
insufficient because _pendingChanges.Count >= MaxPendingEntries only calls
FlushPendingChanges(), which currently only removes expired entries; update the
logic to enforce the cap by performing a real eviction or immediate flush when
the limit is reached: inside the block where MaxPendingEntries is checked
(referencing _pendingChanges, MaxPendingEntries and FlushPendingChanges()), call
a new or existing force flush method (e.g., ForceFlushPendingChanges()) that
writes out or clears all pending entries, or implement explicit eviction (remove
oldest entries from _pendingChanges until Count < MaxPendingEntries) before
adding the new entry so the cache cannot grow beyond the cap.
In `@MCPForUnity/Editor/ActionTrace/Capture/UnityEventHooks.cs`:
- Around line 533-536: There is a stray preprocessor directive "#endregion"
placed after the closing brace of the UnityEventHooks class; remove that
trailing "#endregion" (or move it so it matches the corresponding "#region"
inside the class) to ensure the "#endregion" is inside the class scope and
eliminate the syntax error in UnityEventHooks.cs.
In `@MCPForUnity/Editor/ActionTrace/Context/ToolCallScope.cs`:
- Around line 471-494: ExecuteAsync<T> creates a ToolCallScope but never
disposes it (scope leaks on both asynchronous and synchronous-failure paths);
fix by ensuring scope.Dispose() is always called: wrap the func() invocation so
synchronous exceptions are caught (try/catch around func()), and change the
continuation flow to guarantee disposal (either by converting ExecuteAsync<T> to
an async method that awaits func() and uses try { var result = await func();
scope.Complete(...); return result; } catch (Exception ex) { scope.Fail(...);
throw; } finally { scope.Dispose(); } or, if keeping ContinueWith, invoke
scope.Dispose() after calling scope.Complete/Fail inside every continuation
branch and also catch synchronous exceptions from func() to call scope.Fail(...)
and scope.Dispose() before rethrowing). Ensure you reference ExecuteAsync<T>,
ToolCallScope, scope, func(), ContinueWith when applying the fix.
In `@MCPForUnity/Editor/ActionTrace/Core/EventStore.Context.cs`:
- Around line 155-158: The code uses mappingsSnapshot.GroupBy(m =>
m.EventSequence).ToDictionary(g => g.Key, g => g.FirstOrDefault()), which drops
additional mappings for the same EventSequence; update the logic to preserve all
mappings by changing the dictionary value to a collection (e.g., use g =>
g.ToList()) and update any dependent return types/usages of mappingBySequence to
accept IEnumerable/List of mappings, or if you intentionally want only one
mapping, explicitly document that behavior and keep FirstOrDefault; reference
mappingBySequence, mappingsSnapshot, EventSequence, and FirstOrDefault when
making the change.
In `@MCPForUnity/Editor/ActionTrace/Helpers/GameObjectTrackingHelper.cs`:
- Around line 70-113: DetectChanges is currently calling
FindObjectsOfType<GameObject>(true) every editor frame (via the
EditorApplication.update hook), which is expensive; update DetectChanges (and
the registration in EditorApplication.update) to short-circuit and avoid calling
FindObjectsOfType each frame by adding simple throttling or event-driven checks:
add a static/frame counter or lastRunTime check and return cached results until
N frames or T milliseconds have passed, or only run the full traversal when
specific editor events occur, then keep the existing logic that fills results
and updates _previousInstanceIds when the check actually runs; ensure the method
still asserts main thread via AssertMainThread and preserves behavior of marking
isNew by comparing against _previousInstanceIds.
In `@MCPForUnity/Editor/ActionTrace/Query/EventQueryBuilder.cs`:
- Around line 94-323: WithAllSearchTerms currently adds terms into the same
_searchTerms set (making it behave like OR) and accepts null/empty entries;
change it to record required-all semantics and ignore null/empty terms.
Specifically, introduce or use a distinct collection/name (e.g., _allSearchTerms
or a boolean flag paired with _allSearchTerms) in the EventQueryBuilder and have
WithAllSearchTerms(params string[] terms) skip null/empty strings
(String.IsNullOrWhiteSpace) when adding; update the query evaluation logic to
check _allSearchTerms for an AND (all terms must match) requirement instead of
treating them as OR. Ensure WithSearchTerm retains case-insensitive HashSet
behavior and does not accept null/empty either.
In `@MCPForUnity/Editor/ActionTrace/Query/EventStatistics.cs`:
- Around line 414-447: FindTopTargets currently uses targetEventsList[^1] which
assumes the per-target events are chronological; instead sort each targetEvents
list by TimestampUnixMs (ascending) before calling CalculateRecencyFactor and
before reading the last event for LastActivityMs and DisplayName. Update
FindTopTargets to replace/derive targetEventsList with a timestamp-ordered
sequence (or in-place sort) and then compute recencyFactor =
CalculateRecencyFactor(orderedList, nowMs), ActivityScore, EventCount,
EventTypes, LastActivityMs and DisplayName from the orderedList.Last() (or
orderedList[^1]) so all derived values use the most recent event. Use the same
ordered list when building TargetStats to ensure consistency.
In `@MCPForUnity/Editor/ActionTrace/Query/EventSummarizer.cs`:
- Around line 102-112: The while-loop that replaces placeholders in sb can
infinite-loop if a formatted value contains the same placeholder; replace that
manual index loop with StringBuilder.Replace to perform a safe global
replacement: for each kvp in evt.Payload (or empty dict) compute placeholder =
"{" + kvp.Key + "}" and value = FormatValue(kvp.Value) and call
sb.Replace(placeholder, value). Ensure you keep the surrounding foreach over
evt.Payload (and the null-coalescing) and remove the manual index-based
removal/Insert logic to avoid the self-referential replacement bug.
In `@MCPForUnity/Editor/Windows/ActionTraceEditorWindow.cs`:
- Around line 119-121: The window currently forces
ActionTraceSettings.Instance.Filtering.BypassImportanceFilter = true in
ActionTraceEditorWindow (causing a global side-effect); change this so the
previous value is preserved and restored: in ActionTraceEditorWindow add a
private bool (e.g. _previousBypassValue), capture
ActionTraceSettings.Instance.Filtering.BypassImportanceFilter in OnEnable before
setting it to true, and restore that saved value in OnDisable (also ensure
EditorApplication.update handlers are balanced in OnEnable/OnDisable);
alternatively implement a query-level bypass inside the window’s data-fetching
path instead of touching
ActionTraceSettings.Instance.Filtering.BypassImportanceFilter.
♻️ Duplicate comments (1)
MCPForUnity/Editor/Tools/AddTimelineNoteTool.cs (1)
29-30: LGTM!The tool now includes a
Descriptionattribute for MCP compatibility, addressing the previous review feedback.
🟡 Minor comments (21)
MCPForUnity/Editor/Helpers/GlobalIdHelper.cs-152-161 (1)
152-161: Instance ID resolution may fail for non-GameObject objects.
GameObjectLookup.FindById(line 159) returns onlyGameObject, butGetFallbackIdcan store Instance IDs for anyUnityEngine.Objectincluding Components. Consider usingEditorUtility.InstanceIDToObjectdirectly for broader compatibility:🐛 Suggested fix
if (idStr.StartsWith(InstancePrefix)) { string instanceStr = idStr.Substring(InstancePrefix.Length); if (int.TryParse(instanceStr, out int instanceId)) { - return GameObjectLookup.FindById(instanceId); + return EditorUtility.InstanceIDToObject(instanceId); } }MCPForUnity/Editor/ActionTrace/VCS/VcsContextProvider.cs-264-269 (1)
264-269: Missing timeout in PATH verification could block editor.The
git --versioncheck when verifying PATH availability has no timeout. If Git exists but hangs (e.g., waiting for credentials), this will block indefinitely.🐛 Suggested fix: Add timeout to PATH check
using (var process = Process.Start(startInfo)) { - process.WaitForExit(); - if (process.ExitCode == 0) - return "git"; // Found in PATH + if (process.WaitForExit(2000) && process.ExitCode == 0) + return "git"; // Found in PATH + try { process.Kill(); } catch { } }MCPForUnity/Editor/ActionTrace/Context/ToolCallScope.cs-291-297 (1)
291-297: UnityEngine.Random is not thread-safe.
UnityEngine.Randomshould only be called from Unity's main thread. SinceToolCallScopeusesThreadLocalstorage (implying multi-thread usage), this could cause issues when scopes are created on background threads.🔧 Proposed fix using thread-safe random
+private static readonly System.Random _random = new(); +private static readonly object _randomLock = new(); 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; + lock (_randomLock) + { + random = _random.Next(1000, 9999); + } return $"{_toolId}_{timestamp}_{random}"; }MCPForUnity/Editor/Windows/ActionTraceEditorWindow.cs-249-255 (1)
249-255: Menu separator syntax may not work correctly.
AppendAction("/", ...)likely won't render as a separator in Unity'sToolbarMenu. UseAppendSeparator()instead.🔧 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/Helpers/GameObjectTrackingHelper.cs-28-29 (1)
28-29:MainThreadIdcaptured at static initialization may be incorrect.If this class is first accessed from a background thread (e.g., during async asset import),
MainThreadIdwould capture the wrong thread ID. Consider lazy initialization on first use from a known main-thread context.🔧 Suggested fix
- // Cache for the main thread ID to validate thread safety - private static readonly int MainThreadId = System.Threading.Thread.CurrentThread.ManagedThreadId; + // Cache for the main thread ID to validate thread safety + private static int _mainThreadId = -1; + + private static int MainThreadId + { + get + { + if (_mainThreadId < 0) + { + // Initialize on first access - caller must be on main thread + _mainThreadId = System.Threading.Thread.CurrentThread.ManagedThreadId; + } + return _mainThreadId; + } + }Alternatively, initialize it in
InitializeTracking()which is documented to be called from the main thread.MCPForUnity/Editor/ActionTrace/Core/EventStore.Context.cs-64-68 (1)
64-68: Potential data race:_isDirtymodified outside lock.
_isDirtyis written outside thelock (_queryLock)block. If another thread reads or writes_isDirtyconcurrently, this could lead to a data race. While typically benign on x86/x64, it's inconsistent with the thread-safety guarantees stated in the doc.🔒 Suggested fix
lock (_queryLock) { // ... existing code ... + + // Mark dirty inside lock for thread safety + _isDirty = true; } - // Mark dirty and schedule deferred save - _isDirty = true; ScheduleSave();MCPForUnity/Editor/ActionTrace/Core/EventStore.Persistence.cs-78-87 (1)
78-87: Inconsistent locking for_saveScheduled.
ClearPendingOperationssets_saveScheduled = false(line 85) outside any lock, butScheduleSaveaccesses_saveScheduledunder_queryLock. This inconsistency could lead to race conditions.🐛 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/ActionTrace/Core/EventStore.Merging.cs-100-105 (1)
100-105: PotentialInvalidCastExceptionwhen castingmerge_count.When
merge_countis deserialized from JSON (e.g., after domain reload), Newtonsoft.Json may deserialize it aslongrather thanint. The direct cast(int)existingCountwill throw anInvalidCastExceptionin that case.🐛 Suggested fix
int mergeCount = 1; if (_lastRecordedEvent.Payload.TryGetValue("merge_count", out var existingCount)) { - mergeCount = (int)existingCount + 1; + mergeCount = Convert.ToInt32(existingCount) + 1; }MCPForUnity/Editor/ActionTrace/Capture/AssetChangePostprocessor.cs-135-148 (1)
135-148:IsNewlyCreatedAssetimplementation is non-functional.The method attempts to load
.metafiles viaAssetDatabase.LoadMainAssetAtPath, but.metafiles are not Unity assets and cannot be loaded this way—this will always returnnull. Additionally, even if it worked, the result isn't used before returningfalse.The current implementation always returns
false, which may be acceptable as a placeholder but should be documented more clearly or implemented properly.🔧 Suggested approach if implementation is needed
To detect newly created assets, consider using file creation timestamps:
private static bool IsNewlyCreatedAsset(string assetPath) { try { - string metaPath = assetPath + ".meta"; - var meta = AssetDatabase.LoadMainAssetAtPath(metaPath); - // This is a simplified check - in production you'd check file creation time - return false; // Default to treating as imported for now + string fullPath = System.IO.Path.GetFullPath(assetPath); + if (!System.IO.File.Exists(fullPath)) + return false; + + var creationTime = System.IO.File.GetCreationTimeUtc(fullPath); + var timeSinceCreation = DateTime.UtcNow - creationTime; + + // Consider "newly created" if created within the last 2 seconds + return timeSinceCreation.TotalSeconds < 2; } catch { return false; } }MCPForUnity/Editor/ActionTrace/Core/EventStore.Persistence.cs-186-203 (1)
186-203: Missing lock when reading shared state inSaveToStorage.
SaveToStoragereads_sequenceCounter,_events, and_contextMappingswithout holding_queryLock. While theToList()calls create copies, there's a race window where another thread could modify these collections during iteration.The comment in
ScheduleSave(line 66) says "Perform save outside lock to avoid holding lock during I/O", but the state snapshot should be taken inside the lock.🐛 Suggested fix
private static void SaveToStorage() { try { - var state = new EventStoreState + EventStoreState state; + lock (_queryLock) { - SchemaVersion = CurrentSchemaVersion, - SequenceCounter = _sequenceCounter, - Events = _events.ToList(), - ContextMappings = _contextMappings.ToList() - }; + state = new EventStoreState + { + SchemaVersion = CurrentSchemaVersion, + SequenceCounter = _sequenceCounter, + Events = _events.ToList(), + ContextMappings = _contextMappings.ToList() + }; + } + // I/O happens outside the lock McpJobStateStore.SaveState(StateKey, state); } catch (Exception ex) { McpLog.Error($"[EventStore] Failed to save to storage: {ex.Message}\n{ex.StackTrace}"); } }MCPForUnity/Editor/Tools/AddTimelineNoteTool.cs-146-153 (1)
146-153: AddAINoteconstant toEventTypesclass for consistency with other event types.All event types in
Core/EventTypes.csare defined aspublic const stringconstants (e.g.,ComponentAdded,PropertyModified,GameObjectCreated), butAINoteis missing from this pattern and only appears in the metadata dictionary. Add the constantpublic const string AINote = "AINote";toEventTypesinMCPForUnity/Editor/ActionTrace/Core/EventTypes.cs, then useEventTypes.AINoteinstead of the hardcoded string here.MCPForUnity/Editor/ActionTrace/Capture/UnityEventHooks.cs-393-417 (1)
393-417: Consider adding debug logging to troubleshoot missing compilation error type.
GetCompilationErrorCountrelies on reflection to access the internalUnityEditor.Scripting.ScriptCompilationErrorCounttype, which may not exist in all Unity versions. The fallback of returning 0 is safe, but adding debug logging when the type is not found would help diagnose issues in environments where the reflection fails. For example, log once whentype == nullusing the existingMcpLoginfrastructure.MCPForUnity/Editor/Resources/ActionTrace/ActionTraceViewResource.cs-55-61 (1)
55-61: Guard against null@paramsto preserve default behavior.If
HandleCommandis called withnull, the helpers throw and return an error instead of applying defaults.🐛 Proposed fix
public static object HandleCommand(JObject `@params`) { try { + `@params` ??= new JObject(); int limit = GetLimit(`@params`); long? sinceSequence = GetSinceSequence(`@params`);MCPForUnity/Editor/ActionTrace/Core/ActionTraceSettings.cs-118-149 (1)
118-149: Preset name won’t survive domain reloads.
_currentPresetNameis[NonSerialized], soCurrentPresetNameresets to "Standard" even if values match another preset. Consider serializing it or recomputing on load.MCPForUnity/Editor/ActionTrace/Query/EventStatistics.cs-488-494 (1)
488-494: Build error details are dropped.Emitters use
error_message, butFindErrorsonly checkserror, so the detailed message is lost.♻️ Proposed fix
if (evt.Type == EventTypes.BuildFailed) { - if (evt.Payload != null && evt.Payload.TryGetValue("error", out var error)) - errors.Add($"Build: {error}"); - else - errors.Add("Build failed"); + if (evt.Payload != null && + (evt.Payload.TryGetValue("error", out var error) || + evt.Payload.TryGetValue("error_message", out error))) + errors.Add($"Build: {error}"); + else + errors.Add("Build failed"); }MCPForUnity/Editor/ActionTrace/Core/ActionTraceSettings.cs-266-272 (1)
266-272: Clamp negative cold-event counts.If
HotEventCountexceedsMaxEvents(e.g., via manual asset edits),coldEventsbecomes negative and the estimate underflows.♻️ Proposed fix
int hotEvents = Storage.HotEventCount; int coldEvents = Storage.MaxEvents - Storage.HotEventCount; + if (coldEvents < 0) coldEvents = 0; return (long)(hotEvents * 300 + coldEvents * 100);MCPForUnity/Editor/ActionTrace/Query/EventStatistics.cs-258-276 (1)
258-276:GetSummarycan null‑ref on empty stats.
TopTargets/ErrorMessagesare null whenAnalyzereturns early; guard before.Count.♻️ Proposed fix
- if (stats.TopTargets.Count > 0) + if (stats.TopTargets != null && stats.TopTargets.Count > 0) { summary.AppendLine("Top Targets:"); foreach (var target in stats.TopTargets.Take(5)) { summary.AppendLine($" - {target.DisplayName}: {target.EventCount} events"); } summary.AppendLine(); } - if (stats.ErrorCount > 0) + if (stats.ErrorMessages != null && stats.ErrorCount > 0) { summary.AppendLine($"Errors: {stats.ErrorCount}"); foreach (var error in stats.ErrorMessages.Take(3)) { summary.AppendLine($" - {error}"); } summary.AppendLine(); }MCPForUnity/Editor/ActionTrace/Query/EventQueryBuilder.cs-445-449 (1)
445-449: Null event types can throw under category filtering.
EventTypes.Metadata.Get(evt.Type)will throw ifevt.Typeis null; normalize before lookup.♻️ Proposed fix
- var meta = EventTypes.Metadata.Get(evt.Type); + var meta = EventTypes.Metadata.Get(evt.Type ?? string.Empty);MCPForUnity/Editor/ActionTrace/Query/ContextCompressor.cs-419-435 (1)
419-435: Guard againstwindowMs <= 0to avoid divide‑by‑zero.
evt.TimestampUnixMs / windowMswill throw for zero or negative values.♻️ Proposed fix
public static List<EditorEvent> Deduplicate(this IReadOnlyList<EditorEvent> events, int windowMs = 100) { if (events == null || events.Count == 0) return new List<EditorEvent>(); + if (windowMs <= 0) + windowMs = 1; + var seen = new HashSet<string>(); var result = new List<EditorEvent>();MCPForUnity/Editor/ActionTrace/Capture/ActionTraceEventEmitter.cs-509-515 (1)
509-515: UseToLowerInvariant()for file extension normalization.
ToLower()is culture-sensitive; file extensions should use invariant casing to avoid locale-specific bugs. This is the preferred pattern throughout the codebase.♻️ Proposed fix
- var extension = System.IO.Path.GetExtension(assetPath).ToLower(); + var extension = System.IO.Path.GetExtension(assetPath).ToLowerInvariant();MCPForUnity/Editor/ActionTrace/Core/ActionTraceSettings.cs-206-214 (1)
206-214: Reorder asset creation before preset application.
CreateSettings()callsApplyPreset()which invokesSave()beforeAssetDatabase.CreateAsset()is executed. SinceSave()callsEditorUtility.SetDirty()andAssetDatabase.SaveAssets()on an asset not yet registered in the AssetDatabase, the preset values may not persist properly on first creation. Create the asset first.♻️ Proposed fix
private static ActionTraceSettings CreateSettings() { var settings = CreateInstance<ActionTraceSettings>(); + AssetDatabase.CreateAsset(settings, SettingsPath); // Apply Standard preset by default settings.ApplyPreset(ActionTracePreset.Standard); - AssetDatabase.CreateAsset(settings, SettingsPath); AssetDatabase.SaveAssets(); AssetDatabase.Refresh(); McpLog.Info($"[ActionTraceSettings] Created new settings at {SettingsPath}"); return settings; }
🧹 Nitpick comments (19)
MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs (2)
172-192: Registration failure is silently swallowed.If the reflection-based event registration fails (e.g., API change in a future Unity version), the zombie listener prevention won't work and there's no indication of the failure. Consider adding a debug-level warning in the catch block to aid troubleshooting.
💡 Suggested enhancement
} - catch { } + catch (Exception ex) + { + if (IsDebugEnabled()) + McpLog.Warn($"[StdioBridgeHost] Failed to register beforeAssemblyReload handler: {ex.Message}"); + }
521-544: LGTM - Status file preserved during domain reload.This correctly addresses the past review concern about the status file being deleted during reload. The guard ensures the Python server can detect the reloading state.
Minor note: The status file path computation logic (lines 528-533) duplicates code in
WriteHeartbeat. Consider extracting to a helper method for DRY, though this is optional.MCPForUnity/Editor/ActionTrace/VCS/VcsContextProvider.cs (1)
68-76: Thread-safety claim may be overstated but is practically safe.The comment claims thread-safety, but
GetCurrentContexthas a check-then-act pattern (lines 70-73) that isn't atomic. In practice this is benign—worst case is a redundantGetInitialContext()call. Consider usingInterlocked.CompareExchangeor??=pattern if strict thread-safety is required, or soften the comment.MCPForUnity/Editor/Helpers/GlobalIdHelper.cs (1)
224-230: Method nameIsValidIdmay be misleading.
IsValidIdonly checks for non-empty strings, not actual format validity. Consider renaming toIsNonEmptyor enhancing to verify the format matches one of the known patterns (GlobalObjectId, Scene:, Asset:, Instance:).MCPForUnity/Editor/ActionTrace/Capture/UndoGroupManager.cs (1)
51-70: NestedBeginToolCalloverwrites previous state without warning.If
BeginToolCallis called while already in a tool call, the previous state is silently overwritten. This could lead to orphaned Undo operations from the first call not being collapsed.♻️ Suggested fix: Warn or auto-end previous tool call
public static void BeginToolCall(string toolName, string toolCallId) { + if (_isInToolCall) + { + McpLog.Warn($"[UndoGroupManager] BeginToolCall called while already in tool call '{_currentToolName}'. Ending previous call."); + EndToolCall(); + } + if (string.IsNullOrEmpty(toolName)) {MCPForUnity/Editor/ActionTrace/Context/ToolCallScope.cs (1)
272-289: Consider logging when Dispose finds scope not at stack top.The
Disposemethod silently skips popping ifthisisn't at the top of the stack (line 283). While properusing()patterns should prevent this, silent failure could hide bugs. Consider adding a warning log similar toContextStack.Pop's mismatch handling.♻️ Suggested diagnostic logging
// Pop from stack if (_scopeStack.Value.Count > 0 && _scopeStack.Value.Peek() == this) { _scopeStack.Value.Pop(); } +else if (_scopeStack.Value.Count > 0) +{ + UnityEngine.Debug.LogWarning( + $"[ToolCallScope] Dispose called but scope {CallId} is not at top of stack. " + + $"Top is {_scopeStack.Value.Peek().CallId}. Possible out-of-order disposal."); +}MCPForUnity/Editor/ActionTrace/Semantics/DefaultEventScorer.cs (1)
79-105: Consider case-insensitive extension matching.The extension checks (e.g.,
ext.ToString() == ".cs") are case-sensitive. While Unity typically uses lowercase extensions, imported assets from external sources might have uppercase extensions (e.g.,.CS,.Unity).♻️ Optional: Case-insensitive comparison
private static bool IsScript(EditorEvent e) { if (e.Payload.TryGetValue("extension", out var ext)) - return ext.ToString() == ".cs"; + return string.Equals(ext.ToString(), ".cs", StringComparison.OrdinalIgnoreCase); if (e.Payload.TryGetValue("asset_type", out var type)) return type.ToString()?.Contains("Script") == true || type.ToString()?.Contains("MonoScript") == true; return false; }Apply similar changes to
IsSceneandIsPrefab.MCPForUnity/Editor/ActionTrace/Capture/SelectionPropertyTracker.cs (1)
32-44: Event subscriptions are never unregistered.The static constructor subscribes to
Selection.selectionChangedandUndo.postprocessModificationsbut never unsubscribes. While Unity typically handles this during domain reloads, explicit cleanup in a domain-reload handler (AssemblyReloadEvents) would be more robust and prevent potential duplicate subscriptions.♻️ Suggested cleanup handling
static SelectionPropertyTracker() { // Initialize with current selection UpdateSelectionState(); // Monitor selection changes Selection.selectionChanged += OnSelectionChanged; // Monitor property modifications Undo.postprocessModifications += OnPropertyModified; + // Cleanup on domain reload + AssemblyReloadEvents.beforeAssemblyReload += OnBeforeAssemblyReload; + McpLog.Debug("[SelectionPropertyTracker] Initialized"); } +private static void OnBeforeAssemblyReload() +{ + Selection.selectionChanged -= OnSelectionChanged; + Undo.postprocessModifications -= OnPropertyModified; + AssemblyReloadEvents.beforeAssemblyReload -= OnBeforeAssemblyReload; +}MCPForUnity/Editor/ActionTrace/Core/ActionTraceSettings.Editor.cs (1)
74-78: Redundant save when applying presets.
ApplyPresetalready callsMarkDirty()andSave()internally (seeActionTraceSettings.cslines 159-175). SettingGUI.changed = truewill causeApplyModifiedProperties()to return true, triggering anotherMarkDirty()+Save()cycle at lines 58-59.Consider removing the redundant call:
♻️ Suggested fix
if (GUILayout.Button(preset.Name)) { settings.ApplyPreset(preset); - GUI.changed = true; }MCPForUnity/Editor/ActionTrace/Core/Presets/ActionTracePreset.cs (1)
95-98: Consider makingAllPresetsimmutable to prevent accidental modification.
AllPresetsis a mutableList<ActionTracePreset>that external code could accidentally or maliciously modify (e.g.,AllPresets.Clear()). Since this is a public API, consider using an immutable collection.♻️ Suggested fix
- public static readonly List<ActionTracePreset> AllPresets = new() + public static IReadOnlyList<ActionTracePreset> AllPresets { get; } = new List<ActionTracePreset> { DebugAll, Standard, Lean, AIFocused, Realtime, Performance - }; + }.AsReadOnly();Note: This also requires updating
ActionTraceSettings.FindPresetto use LINQ'sFirstOrDefaultinstead ofList.Find:return ActionTracePreset.AllPresets.FirstOrDefault(p => p.Name.Equals(name, StringComparison.OrdinalIgnoreCase));MCPForUnity/Editor/ActionTrace/Helpers/GameObjectTrackingHelper.cs (1)
100-105: Misleading comment: "swap hash sets" doesn't match implementation.The comment says "swap hash sets to avoid allocation" but the code clears
_previousInstanceIdsand re-adds items fromcurrentIds. This is not a swap operation and does allocate (the foreach iteration). Consider updating the comment or implementing an actual swap.♻️ Suggested fix - update comment
- // Update tracking: swap hash sets to avoid allocation + // Update tracking: replace previous IDs with current _previousInstanceIds.Clear(); foreach (int id in currentIds) { _previousInstanceIds.Add(id); }MCPForUnity/Editor/Windows/ActionTraceEditorWindow.cs (1)
16-23: Consider using English for code comments.The enum and its members have Chinese comments. For consistency and broader team accessibility, consider using English.
♻️ Suggested change
/// <summary> - /// 排序模式：控制事件列表的排序方式 + /// Sort mode: controls event list ordering /// </summary> public enum SortMode { - /// <summary>纯时间排序（最新优先）- 给用户查看记录</summary> + /// <summary>Sort by time (newest first) - for user viewing</summary> ByTimeDesc, - /// <summary>AI 视角排序 - 先按时间再按重要性分组</summary> + /// <summary>AI perspective sorting - by time then importance grouping</summary> AIFiltered }MCPForUnity/Editor/ActionTrace/Core/EventStore.Merging.cs (1)
69-71: Unused parameterevtWithSequence.The parameter
evtWithSequenceis declared but never used in the method body. Either remove it or use it for its intended purpose (the doc comment mentions "for updating _lastRecordedEvent").♻️ Suggested fix
If the parameter is not needed:
- private static void MergeWithLastEventLocked(EditorEvent evt, EditorEvent evtWithSequence) + private static void MergeWithLastEventLocked(EditorEvent evt)Update callers accordingly.
MCPForUnity/Editor/Tools/AddTimelineNoteTool.cs (1)
35-78: Parameters class is defined but not used for deserialization.The
Parametersclass withToolParameterattributes is defined butHandleCommandmanually parses theJObjectinstead of deserializing into aParametersinstance. This creates duplication and potential drift between the two.Consider either:
- Using the
Parametersclass for deserialization- Removing the
Parametersclass if it's only for documentation/schema generationAlso applies to: 80-144
MCPForUnity/Editor/ActionTrace/Capture/SamplingMiddleware.cs (1)
360-401: Thread safety concern:Strategiesdictionary is not thread-safe.
SamplingConfig.Strategiesis a regularDictionary<string, SamplingStrategy>that's publicly accessible and mutated viaSetStrategy/RemoveStrategy. However, it's read fromShouldRecordandFlushExpiredDebounceSamples, which run onEditorApplication.update.While Unity Editor is mostly single-threaded, the use of
ConcurrentDictionaryfor_pendingSamplessuggests thread safety is a design concern. Consider usingConcurrentDictionaryforStrategiesas well, or documenting that modifications must only occur from the main thread.♻️ Suggested fix
- public static readonly Dictionary<string, SamplingStrategy> Strategies = new() + public static readonly ConcurrentDictionary<string, SamplingStrategy> Strategies = new( + new Dictionary<string, SamplingStrategy> { // Hierarchy changes: Throttle to 1 event per second { EventTypes.HierarchyChanged, new SamplingStrategy(SamplingMode.Throttle, 1000) }, - }; + });And update methods:
public static void SetStrategy(string eventType, SamplingMode mode, long windowMs = 1000) { Strategies[eventType] = new SamplingStrategy(mode, windowMs); } public static void RemoveStrategy(string eventType) { - Strategies.Remove(eventType); + Strategies.TryRemove(eventType, out _); }MCPForUnity/Editor/ActionTrace/Capture/UnityEventHooks.cs (1)
198-212: UseDateTime.UtcNowinstead ofDateTime.Nowfor debouncing.Line 200 uses
DateTime.Nowwhile other timestamp operations in the codebase useDateTime.UtcNow(e.g., lines 343, 351, 428, 444). Using local time can cause issues during DST transitions.♻️ Suggested fix
private static void OnHierarchyChanged() { - var now = DateTime.Now; + var now = DateTime.UtcNow; lock (_lock) {MCPForUnity/Editor/ActionTrace/Capture/EventFilter.cs (1)
249-327: Consider caching active rules to avoid per-call allocations.
GetActiveRules()builds and sorts a list on every filter check. If these paths are hot, cache the computed list and invalidate it when settings or rules change.MCPForUnity/Editor/ActionTrace/Query/ContextCompressor.cs (1)
48-53: Unused config knob.
TargetCompressionRatioisn’t referenced; consider implementing or removing to avoid a misleading config surface.MCPForUnity/Editor/ActionTrace/Core/EventTypes.cs (1)
98-103: Avoid string literal duplication for AINote.
"AINote"is duplicated here and inMCPForUnity/Editor/Resources/ActionTrace/ActionTraceViewResource.cs. Add anEventTypes.AINoteconstant and use it here (and update the resource class) to prevent drift.♻️ Proposed fix (this file)
// Build events public const string BuildStarted = "BuildStarted"; public const string BuildCompleted = "BuildCompleted"; public const string BuildFailed = "BuildFailed"; + // AI/system events + public const string AINote = "AINote"; @@ - ["AINote"] = new EventMetadata + [AINote] = new EventMetadata { Category = EventCategory.System, DefaultImportance = 1.0f, SummaryTemplate = "AI Note{if:agent_id, ({agent_id})}: {note}", },
MCPForUnity/Editor/ActionTrace/Capture/AssetChangePostprocessor.cs
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| #endregion | ||
| } |
There was a problem hiding this comment.
Syntax error: stray #endregion outside class.
Line 535 has an #endregion directive after the class closing brace, which will cause a compilation error.
🐛 Suggested fix
}
- }
`#endregion`
+ }
}Or remove the stray #endregion if it's not needed:
}
}
-
- `#endregion`
}🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/ActionTrace/Capture/UnityEventHooks.cs` around lines 533 -
536, There is a stray preprocessor directive "#endregion" placed after the
closing brace of the UnityEventHooks class; remove that trailing "#endregion"
(or move it so it matches the corresponding "#region" inside the class) to
ensure the "#endregion" is inside the class scope and eliminate the syntax error
in UnityEventHooks.cs.
| // Build lookup dictionary outside lock | ||
| var mappingBySequence = mappingsSnapshot | ||
| .GroupBy(m => m.EventSequence) | ||
| .ToDictionary(g => g.Key, g => g.FirstOrDefault()); |
There was a problem hiding this comment.
FirstOrDefault discards multiple context mappings per event.
The class doc at line 28-29 states "Multiple mappings allowed for same eventSequence (different contexts)", but FirstOrDefault() only returns one mapping per event sequence. This loses context information when an event is associated with multiple contexts.
🐛 Suggested fix: Return all mappings or change the return type
Option 1 - Keep first mapping (document behavior):
- // Build lookup dictionary outside lock
- var mappingBySequence = mappingsSnapshot
- .GroupBy(m => m.EventSequence)
- .ToDictionary(g => g.Key, g => g.FirstOrDefault());
+ // Build lookup dictionary outside lock (returns first mapping per event)
+ var mappingBySequence = mappingsSnapshot
+ .GroupBy(m => m.EventSequence)
+ .ToDictionary(g => g.Key, g => g.First());Option 2 - Return all mappings (requires signature change):
// Change return type to include all mappings
var mappingBySequence = mappingsSnapshot
.GroupBy(m => m.EventSequence)
.ToDictionary(g => g.Key, g => g.ToList());🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/ActionTrace/Core/EventStore.Context.cs` around lines 155 -
158, The code uses mappingsSnapshot.GroupBy(m => m.EventSequence).ToDictionary(g
=> g.Key, g => g.FirstOrDefault()), which drops additional mappings for the same
EventSequence; update the logic to preserve all mappings by changing the
dictionary value to a collection (e.g., use g => g.ToList()) and update any
dependent return types/usages of mappingBySequence to accept IEnumerable/List of
mappings, or if you intentionally want only one mapping, explicitly document
that behavior and keep FirstOrDefault; reference mappingBySequence,
mappingsSnapshot, EventSequence, and FirstOrDefault when making the change.
| public static List<(GameObject obj, bool isNew)> DetectChanges() | ||
| { | ||
| AssertMainThread(); | ||
|
|
||
| if (!_hasInitialized) | ||
| { | ||
| InitializeTracking(); | ||
| return new List<(GameObject, bool)>(0); | ||
| } | ||
|
|
||
| var results = new List<(GameObject, bool)>(64); | ||
| var currentIds = new HashSet<int>(256); | ||
|
|
||
| try | ||
| { | ||
| GameObject[] currentObjects = GameObject.FindObjectsOfType<GameObject>(true); | ||
|
|
||
| // First pass: collect current IDs and detect new objects | ||
| foreach (var go in currentObjects) | ||
| { | ||
| if (go == null) continue; | ||
|
|
||
| int id = go.GetInstanceID(); | ||
| currentIds.Add(id); | ||
|
|
||
| // HashSet.Contains() is O(1) vs List.Contains() O(n) | ||
| bool isNew = !_previousInstanceIds.Contains(id); | ||
| results.Add((go, isNew)); | ||
| } | ||
|
|
||
| // Update tracking: swap hash sets to avoid allocation | ||
| _previousInstanceIds.Clear(); | ||
| foreach (int id in currentIds) | ||
| { | ||
| _previousInstanceIds.Add(id); | ||
| } | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Debug.LogError($"[ActionTrace] Failed to detect GameObject changes: {ex.Message}"); | ||
| } | ||
|
|
||
| return results; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how DetectChanges is called to understand frequency
rg -n "DetectChanges\(\)" --type=cs -C3Repository: CoplayDev/unity-mcp
Length of output: 1458
🏁 Script executed:
#!/bin/bash
# Check how TrackGameObjectChanges is called and what invokes it
rg -n "TrackGameObjectChanges" --type=cs -B5 -A5Repository: CoplayDev/unity-mcp
Length of output: 1607
🏁 Script executed:
#!/bin/bash
# Read UnityEventHooks.cs to understand the context and frequency of calls
rg -n "class UnityEventHooks|void.*\(\)|private static void\|public static void" MCPForUnity/Editor/ActionTrace/Capture/UnityEventHooks.cs -B2 -A10 | head -100Repository: CoplayDev/unity-mcp
Length of output: 3211
🏁 Script executed:
#!/bin/bash
# Search for registration of OnUpdate event
rg -n "OnUpdate|EditorApplication.update" MCPForUnity/Editor/ActionTrace/Capture/UnityEventHooks.cs -B2 -A2Repository: CoplayDev/unity-mcp
Length of output: 507
FindObjectsOfType is called every editor frame and will impact performance.
FindObjectsOfType<GameObject>(true) is registered to EditorApplication.update and runs unconditionally every frame (see line 70 in UnityEventHooks.cs). This method traverses the entire scene hierarchy and allocates an array on each invocation. In large scenes with many GameObjects, this will noticeably impact editor responsiveness. Consider adding throttling (e.g., checking changes only every N frames or on time-based intervals) or only running detection when specific events occur rather than every frame.
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/ActionTrace/Helpers/GameObjectTrackingHelper.cs` around
lines 70 - 113, DetectChanges is currently calling
FindObjectsOfType<GameObject>(true) every editor frame (via the
EditorApplication.update hook), which is expensive; update DetectChanges (and
the registration in EditorApplication.update) to short-circuit and avoid calling
FindObjectsOfType each frame by adding simple throttling or event-driven checks:
add a static/frame counter or lastRunTime check and return cached results until
N frames or T milliseconds have passed, or only run the full traversal when
specific editor events occur, then keep the existing logic that fills results
and updates _previousInstanceIds when the check actually runs; ensure the method
still asserts main thread via AssertMainThread and preserves behavior of marking
isNew by comparing against _previousInstanceIds.
| _maxImportance = maxScore; | ||
| return this; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Filter to events within an importance range. | ||
| /// </summary> | ||
| public EventQueryBuilder WithImportanceBetween(float minScore, float maxScore) | ||
| { | ||
| _minImportance = minScore; | ||
| _maxImportance = maxScore; | ||
| return this; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Filter to critical events only. | ||
| /// </summary> | ||
| public EventQueryBuilder CriticalOnly() | ||
| { | ||
| return WithMinImportance(0.9f); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Filter to important events (high and critical). | ||
| /// </summary> | ||
| public EventQueryBuilder ImportantOnly() | ||
| { | ||
| return WithMinImportance(0.7f); | ||
| } | ||
|
|
||
| // ========== Time Filters ========== | ||
|
|
||
| /// <summary> | ||
| /// Filter to events within the specified time range. | ||
| /// </summary> | ||
| public EventQueryBuilder InTimeRange(QueryTimeRange range) | ||
| { | ||
| _timeRange = range; | ||
| return this; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Filter to events in the last N minutes. | ||
| /// </summary> | ||
| public EventQueryBuilder InLastMinutes(int minutes) | ||
| { | ||
| _timeRange = QueryTimeRange.LastMinutes(minutes); | ||
| return this; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Filter to events in the last N hours. | ||
| /// </summary> | ||
| public EventQueryBuilder InLastHours(int hours) | ||
| { | ||
| _timeRange = QueryTimeRange.LastHours(hours); | ||
| return this; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Filter to events since a specific timestamp. | ||
| /// </summary> | ||
| public EventQueryBuilder Since(long timestampMs) | ||
| { | ||
| _timeRange = QueryTimeRange.Since(timestampMs); | ||
| return this; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Filter to events between two timestamps. | ||
| /// </summary> | ||
| public EventQueryBuilder Between(long startMs, long endMs) | ||
| { | ||
| _timeRange = QueryTimeRange.Between(startMs, endMs); | ||
| return this; | ||
| } | ||
|
|
||
| // ========== Search Filters ========== | ||
|
|
||
| /// <summary> | ||
| /// Filter to events containing any of the search terms (case-insensitive). | ||
| /// Searches in summary text and target ID. | ||
| /// </summary> | ||
| public EventQueryBuilder WithSearchTerm(string term) | ||
| { | ||
| _searchTerms ??= new HashSet<string>(StringComparer.OrdinalIgnoreCase); | ||
| _searchTerms.Add(term); | ||
| return this; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Filter to events containing all of the search terms. | ||
| /// </summary> | ||
| public EventQueryBuilder WithAllSearchTerms(params string[] terms) | ||
| { | ||
| _searchTerms ??= new HashSet<string>(StringComparer.OrdinalIgnoreCase); | ||
| foreach (string term in terms) | ||
| _searchTerms.Add(term); | ||
| return this; | ||
| } |
There was a problem hiding this comment.
WithAllSearchTerms behaves like OR and accepts null terms.
This contradicts the API contract and can throw on null entries; track “all terms” semantics and ignore empty terms.
🐛 Proposed fix
private HashSet<string> _includedTargets;
private HashSet<string> _searchTerms;
+ private bool _requireAllSearchTerms;
private float? _minImportance;
private float? _maxImportance;
@@
public EventQueryBuilder WithSearchTerm(string term)
{
- _searchTerms ??= new HashSet<string>(StringComparer.OrdinalIgnoreCase);
- _searchTerms.Add(term);
+ if (string.IsNullOrWhiteSpace(term)) return this;
+ _searchTerms ??= new HashSet<string>(StringComparer.OrdinalIgnoreCase);
+ _requireAllSearchTerms = false;
+ _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);
+ _requireAllSearchTerms = true;
+ foreach (string term in terms)
+ if (!string.IsNullOrWhiteSpace(term))
+ _searchTerms.Add(term);
return this;
}
@@
if (_searchTerms != null && _searchTerms.Count > 0)
{
string summary = (evt.GetSummary() ?? "").ToLowerInvariant();
string target = (evt.TargetId ?? "").ToLowerInvariant();
- bool matchesAny = false;
+ bool matchesAny = false;
+ bool matchesAll = true;
foreach (string term in _searchTerms)
{
string lowerTerm = term.ToLowerInvariant();
- if (summary.Contains(lowerTerm) || target.Contains(lowerTerm))
- {
- matchesAny = true;
- break;
- }
+ bool matched = summary.Contains(lowerTerm) || target.Contains(lowerTerm);
+ matchesAny |= matched;
+ matchesAll &= matched;
+ if (!_requireAllSearchTerms && matchesAny) break;
+ if (_requireAllSearchTerms && !matchesAll) break;
}
- if (!matchesAny)
+ if (_requireAllSearchTerms ? !matchesAll : !matchesAny)
return false;
}Also applies to: 478-495
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/ActionTrace/Query/EventQueryBuilder.cs` around lines 94 -
323, WithAllSearchTerms currently adds terms into the same _searchTerms set
(making it behave like OR) and accepts null/empty entries; change it to record
required-all semantics and ignore null/empty terms. Specifically, introduce or
use a distinct collection/name (e.g., _allSearchTerms or a boolean flag paired
with _allSearchTerms) in the EventQueryBuilder and have
WithAllSearchTerms(params string[] terms) skip null/empty strings
(String.IsNullOrWhiteSpace) when adding; update the query evaluation logic to
check _allSearchTerms for an AND (all terms must match) requirement instead of
treating them as OR. Ensure WithSearchTerm retains case-insensitive HashSet
behavior and does not accept null/empty either.
| private List<TargetStats> FindTopTargets(IReadOnlyList<EditorEvent> events) | ||
| { | ||
| var targetEvents = new Dictionary<string, List<EditorEvent>>(); | ||
|
|
||
| foreach (var evt in events) | ||
| { | ||
| if (string.IsNullOrEmpty(evt.TargetId)) continue; | ||
|
|
||
| if (!targetEvents.TryGetValue(evt.TargetId, out var list)) | ||
| { | ||
| list = new List<EditorEvent>(); | ||
| targetEvents[evt.TargetId] = list; | ||
| } | ||
| list.Add(evt); | ||
| } | ||
|
|
||
| var nowMs = DateTimeOffset.UtcNow.ToUnixTimeMilliseconds(); | ||
| var stats = new List<TargetStats>(); | ||
|
|
||
| foreach (var kvp in targetEvents) | ||
| { | ||
| var targetEventsList = kvp.Value; | ||
| double recencyFactor = CalculateRecencyFactor(targetEventsList, nowMs); | ||
| double activityScore = targetEventsList.Count * recencyFactor; | ||
|
|
||
| stats.Add(new TargetStats | ||
| { | ||
| TargetId = kvp.Key, | ||
| DisplayName = GetDisplayName(targetEventsList[^1]), | ||
| EventCount = targetEventsList.Count, | ||
| EventTypes = targetEventsList.Select(e => e.Type).Distinct().ToList(), | ||
| LastActivityMs = targetEventsList[^1].TimestampUnixMs, | ||
| ActivityScore = activityScore | ||
| }); |
There was a problem hiding this comment.
Top‑target recency uses list order, not timestamp order.
targetEventsList[^1] depends on input order, so LastActivityMs, DisplayName, and ActivityScore can be wrong if the input isn’t chronological. Sort per‑target lists by timestamp before using the last item.
🐛 Proposed fix
foreach (var kvp in targetEvents)
{
var targetEventsList = kvp.Value;
- double recencyFactor = CalculateRecencyFactor(targetEventsList, nowMs);
+ var orderedTargetEvents = targetEventsList.OrderBy(e => e.TimestampUnixMs).ToList();
+ double recencyFactor = CalculateRecencyFactor(orderedTargetEvents, nowMs);
double activityScore = targetEventsList.Count * recencyFactor;
stats.Add(new TargetStats
{
TargetId = kvp.Key,
- DisplayName = GetDisplayName(targetEventsList[^1]),
+ DisplayName = GetDisplayName(orderedTargetEvents[^1]),
EventCount = targetEventsList.Count,
- EventTypes = targetEventsList.Select(e => e.Type).Distinct().ToList(),
- LastActivityMs = targetEventsList[^1].TimestampUnixMs,
+ EventTypes = orderedTargetEvents.Select(e => e.Type).Distinct().ToList(),
+ LastActivityMs = orderedTargetEvents[^1].TimestampUnixMs,
ActivityScore = activityScore
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private List<TargetStats> FindTopTargets(IReadOnlyList<EditorEvent> events) | |
| { | |
| var targetEvents = new Dictionary<string, List<EditorEvent>>(); | |
| foreach (var evt in events) | |
| { | |
| if (string.IsNullOrEmpty(evt.TargetId)) continue; | |
| if (!targetEvents.TryGetValue(evt.TargetId, out var list)) | |
| { | |
| list = new List<EditorEvent>(); | |
| targetEvents[evt.TargetId] = list; | |
| } | |
| list.Add(evt); | |
| } | |
| var nowMs = DateTimeOffset.UtcNow.ToUnixTimeMilliseconds(); | |
| var stats = new List<TargetStats>(); | |
| foreach (var kvp in targetEvents) | |
| { | |
| var targetEventsList = kvp.Value; | |
| double recencyFactor = CalculateRecencyFactor(targetEventsList, nowMs); | |
| double activityScore = targetEventsList.Count * recencyFactor; | |
| stats.Add(new TargetStats | |
| { | |
| TargetId = kvp.Key, | |
| DisplayName = GetDisplayName(targetEventsList[^1]), | |
| EventCount = targetEventsList.Count, | |
| EventTypes = targetEventsList.Select(e => e.Type).Distinct().ToList(), | |
| LastActivityMs = targetEventsList[^1].TimestampUnixMs, | |
| ActivityScore = activityScore | |
| }); | |
| private List<TargetStats> FindTopTargets(IReadOnlyList<EditorEvent> events) | |
| { | |
| var targetEvents = new Dictionary<string, List<EditorEvent>>(); | |
| foreach (var evt in events) | |
| { | |
| if (string.IsNullOrEmpty(evt.TargetId)) continue; | |
| if (!targetEvents.TryGetValue(evt.TargetId, out var list)) | |
| { | |
| list = new List<EditorEvent>(); | |
| targetEvents[evt.TargetId] = list; | |
| } | |
| list.Add(evt); | |
| } | |
| var nowMs = DateTimeOffset.UtcNow.ToUnixTimeMilliseconds(); | |
| var stats = new List<TargetStats>(); | |
| foreach (var kvp in targetEvents) | |
| { | |
| var targetEventsList = kvp.Value; | |
| var orderedTargetEvents = targetEventsList.OrderBy(e => e.TimestampUnixMs).ToList(); | |
| double recencyFactor = CalculateRecencyFactor(orderedTargetEvents, nowMs); | |
| double activityScore = targetEventsList.Count * recencyFactor; | |
| stats.Add(new TargetStats | |
| { | |
| TargetId = kvp.Key, | |
| DisplayName = GetDisplayName(orderedTargetEvents[^1]), | |
| EventCount = targetEventsList.Count, | |
| EventTypes = orderedTargetEvents.Select(e => e.Type).Distinct().ToList(), | |
| LastActivityMs = orderedTargetEvents[^1].TimestampUnixMs, | |
| ActivityScore = activityScore | |
| }); |
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/ActionTrace/Query/EventStatistics.cs` around lines 414 -
447, FindTopTargets currently uses targetEventsList[^1] which assumes the
per-target events are chronological; instead sort each targetEvents list by
TimestampUnixMs (ascending) before calling CalculateRecencyFactor and before
reading the last event for LastActivityMs and DisplayName. Update FindTopTargets
to replace/derive targetEventsList with a timestamp-ordered sequence (or
in-place sort) and then compute recencyFactor =
CalculateRecencyFactor(orderedList, nowMs), ActivityScore, EventCount,
EventTypes, LastActivityMs and DisplayName from the orderedList.Last() (or
orderedList[^1]) so all derived values use the most recent event. Use the same
ordered list when building TargetStats to ensure consistency.
| // Handle regular placeholders | ||
| foreach (var kvp in evt.Payload ?? new Dictionary<string, object>()) | ||
| { | ||
| string placeholder = "{" + kvp.Key + "}"; | ||
| int index = 0; | ||
| while ((index = sb.ToString().IndexOf(placeholder, index, StringComparison.Ordinal)) >= 0) | ||
| { | ||
| string value = FormatValue(kvp.Value); | ||
| sb.Remove(index, placeholder.Length); | ||
| sb.Insert(index, value); | ||
| } |
There was a problem hiding this comment.
Avoid potential infinite loop when placeholder value contains itself.
The current replacement loop can spin indefinitely if a payload value contains its own placeholder (e.g., value = "{foo}"). Using StringBuilder.Replace avoids this hazard and is simpler.
🐛 Proposed fix
- foreach (var kvp in evt.Payload ?? new Dictionary<string, object>())
- {
- string placeholder = "{" + kvp.Key + "}";
- int index = 0;
- while ((index = sb.ToString().IndexOf(placeholder, index, StringComparison.Ordinal)) >= 0)
- {
- string value = FormatValue(kvp.Value);
- sb.Remove(index, placeholder.Length);
- sb.Insert(index, value);
- }
- }
+ foreach (var kvp in evt.Payload ?? new Dictionary<string, object>())
+ {
+ string placeholder = "{" + kvp.Key + "}";
+ string value = FormatValue(kvp.Value);
+ sb.Replace(placeholder, value);
+ }📝 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.
| // Handle regular placeholders | |
| foreach (var kvp in evt.Payload ?? new Dictionary<string, object>()) | |
| { | |
| string placeholder = "{" + kvp.Key + "}"; | |
| int index = 0; | |
| while ((index = sb.ToString().IndexOf(placeholder, index, StringComparison.Ordinal)) >= 0) | |
| { | |
| string value = FormatValue(kvp.Value); | |
| sb.Remove(index, placeholder.Length); | |
| sb.Insert(index, value); | |
| } | |
| // Handle regular placeholders | |
| foreach (var kvp in evt.Payload ?? new Dictionary<string, object>()) | |
| { | |
| string placeholder = "{" + kvp.Key + "}"; | |
| string value = FormatValue(kvp.Value); | |
| sb.Replace(placeholder, value); | |
| } |
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/ActionTrace/Query/EventSummarizer.cs` around lines 102 -
112, The while-loop that replaces placeholders in sb can infinite-loop if a
formatted value contains the same placeholder; replace that manual index loop
with StringBuilder.Replace to perform a safe global replacement: for each kvp in
evt.Payload (or empty dict) compute placeholder = "{" + kvp.Key + "}" and value
= FormatValue(kvp.Value) and call sb.Replace(placeholder, value). Ensure you
keep the surrounding foreach over evt.Payload (and the null-coalescing) and
remove the manual index-based removal/Insert logic to avoid the self-referential
replacement bug.
OverviewActionTrace is an event tracing and recording system for the Unity Editor, designed to capture, store, and query editor operation events. This system is built for AI-assisted development scenarios, providing traceability of operations and semantic understanding capabilities. Design Principles
Module Architecturetext ActionTrace/ ├── Core/ # Core data models and storage ├── Capture/ # Unity event capture and filtering ├── Context/ # Operation context management ├── Semantics/ # Semantic analysis (scoring, categorization, intent inference) ├── Query/ # Query and projection engine ├── VCS/ # Version control integration ├── Helpers/ # Utility classes └── Descriptors/ # Event descriptors File Inventory (40 CS Files)Core Module (10 files)
File | Description
-- | --
EventTypes.cs | Event type constant definitions
EventMetadata.cs | Event metadata (labels, templates)
EditorEvent.cs | Immutable event class supporting dehydration
EventStore.cs | Core API for event storage
EventStore.Merging.cs | Event merging/deduplication logic
EventStore.Persistence.cs | Persistence and domain reload survival
EventStore.Context.cs | Context mapping management
EventStore.Diagnostics.cs | Memory diagnostics and dehydration
ActionTraceSettings.cs | Persistent settings configuration (layered properties)
SamplingMode.cs | Sampling mode enumeration
Performance Characteristics
Key Design Decisions
|
whatevertogo
changed the title
- Add HookRegistry.cs: Central event hub with dual event system (simple + detailed) - Add UnityEventHooks.cs: Pure detector for Unity editor events - Add ActionTraceRecorder.cs: ActionTrace-specific recorder using HookRegistry - Add HookTest.cs: Test script for hook verification (commented out) Architecture: Unity Events → UnityEventHooks → HookRegistry → Subscribers This keeps UnityEventHooks as a pure detector without ActionTrace dependencies. Co-Authored-By: Claude Opus 4.5 <[email protected]>
whatevertogo
changed the title
Thread Safety (3 fixes): - Add exception handling to all HookRegistry Notify* methods Prevents subscriber errors from breaking invocation chain - Use foreach with GetInvocationList() for safe delegate iteration - Log warnings without breaking notification pipeline Memory Management (4 fixes): - Add MaxPendingNotifications limit (256) to EventStore Prevents unbounded notification buffer growth - Add save throttling (MinSaveIntervalMs = 1000ms) to EventStore.Persistence Prevents excessive disk writes during rapid event recording - Existing dehydration already optimal (hot/cold event split) Performance (5 fixes): - Add score caching (_scoreCache) to EventQueryBuilder Avoids repeated semantic analysis scoring of same events - Add rule caching (_cachedActiveRules) to EventFilterSettings Avoids repeated sorting of filter rules - Existing sampling strategies already optimal Design (8 fixes): - Improve filter performance with lazy cache invalidation - Ensure consistent exception handling across recording pipeline - Maintain backward compatibility with existing HookRegistry API This commit completes all P1 priority issues from the ActionTrace refactoring. The system now has better resilience, memory efficiency, and query performance for production use. Co-Authored-By: Claude <[email protected]>
- Redesigned styles in ActionTraceEditorWindow.uss for improved aesthetics and usability, including new color schemes and layout adjustments. - Updated UXML structure in ActionTraceEditorWindow.uxml to incorporate a new header bar, main toolbar with categorized sections, and enhanced event list display. - Added empty state and no results state visual elements to provide better user feedback. - Introduced responsive design elements for better usability on smaller screens.
Major ActionTrace system architecture refactoring:
- Add HookRegistry system for unified event hook registration
- Add EventCaptureRegistry for capture point management
- Add new Recorder architecture replacing legacy implementation
- Restructure directories: Core/{Models,Store,Settings}, Analysis/, Sources/
- Add Unity event hooks with EventArgs pattern
- Add built-in capture points system
Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In `@MCPForUnity/Editor/ActionTrace/Capture/Capturers/SelectionCapture.cs`:
- Around line 34-46: SelectionPropertyTracker subscribes to
Selection.selectionChanged and Undo.postprocessModifications in its static
constructor but never unsubscribes, causing duplicate handlers after domain
reloads; add a static UnregisterHandlers method that removes the delegates from
Selection.selectionChanged and Undo.postprocessModifications, and register that
UnregisterHandlers with AssemblyReloadEvents.beforeAssemblyReload (or a similar
domain-reload hook) from the static constructor so the handlers are removed
before reload and won't be doubled when re-initialized.
In `@MCPForUnity/Editor/ActionTrace/Capture/Emitter.cs`:
- Around line 415-431: EmitAssetModified is passing the IReadOnlyDictionary
changes straight into payload which later gets mis-sanitized; clone changes into
a concrete IDictionary before adding to payload so EditorEvent sees it as
IDictionary (e.g., create a new Dictionary<string, object> clonedChanges =
changes != null ? new Dictionary<string, object>(changes) : new
Dictionary<string, object>() and assign payload["changes"] = clonedChanges).
Update the EmitAssetModified method to use this cloned dictionary (reference
variables: EmitAssetModified, changes, payload) so the changes payload is
preserved.
In `@MCPForUnity/Editor/ActionTrace/Capture/Recorder.cs`:
- Around line 28-47: The static constructor of ActionTraceRecorder subscribes
many HookRegistry events (OnComponentAdded, OnComponentRemoved,
OnComponentRemovedDetailed, OnGameObjectCreated, OnGameObjectDestroyedDetailed,
OnSelectionChanged, OnHierarchyChanged, OnPlayModeChanged, OnSceneSaved,
OnSceneOpenedDetailed, OnNewSceneCreatedDetailed, OnScriptCompiledDetailed,
OnScriptCompilationFailedDetailed, OnBuildCompletedDetailed) but never
unsubscribes, causing duplicate handlers after domain reload; add a static
cleanup method on ActionTraceRecorder that unsubscribes the exact same handlers
from HookRegistry and ensure that cleanup is invoked on domain reload/assembly
reload or editor shutdown (e.g. via AssemblyReloadEvents.beforeAssemblyReload /
AppDomain.DomainUnload / EditorApplication.quitting hookup) so subscriptions are
removed before the static constructor can run again.
In `@MCPForUnity/Editor/ActionTrace/Capture/Sampling/SamplingMiddleware.cs`:
- Around line 177-186: The debounce branch in SamplingMiddleware (cases
SamplingMode.Debounce / DebounceByKey) resets PendingSample.TimestampMs to nowMs
on each incoming evt, which extends the debounce window; instead, when updating
the _pendingSamples[key] PendingSample struct replace only the Event payload
with evt but preserve the existing TimestampMs if an entry already exists
(create a new PendingSample with TimestampMs = existing.TimestampMs, Event =
evt), and only set TimestampMs = nowMs when inserting the first pending sample
(i.e., when no existing entry for key). This keeps the original window start
while still updating the last event payload.
In `@MCPForUnity/Editor/ActionTrace/Context/ContextTimeline.cs`:
- Around line 24-29: The static constructor in ContextTrace subscribes
EventStore.EventRecorded via OnEventRecorded but never unsubscribes, causing
handler accumulation across domain reloads; update ContextTrace to unsubscribe
in an assembly/domain-unload callback by registering a cleanup that calls
EventStore.EventRecorded -= OnEventRecorded (for example subscribe a lambda to
UnityEditor.AssemblyReloadEvents.beforeAssemblyReload or AppDomain.DomainUnload
if appropriate) so the OnEventRecorded handler is removed when the editor
reloads; ensure the unsubscribe references the same static method name
OnEventRecorded so duplicates are prevented.
In `@MCPForUnity/Editor/ActionTrace/Core/Store/EventStore.Context.cs`:
- Around line 76-85: RemoveContextMappings currently sets _isDirty = true and
calls ScheduleSave() outside the lock used for _contextMappings, creating the
same race as AddContextMapping; move the _isDirty = true assignment (and
optionally ScheduleSave() if you followed AddContextMapping pattern) inside the
lock protected by _queryLock so the mutation and dirty flag update are atomic
with the RemoveAll operation, referencing the RemoveContextMappings method,
_queryLock, _contextMappings, _isDirty and ScheduleSave.
- Around line 67-71: The race is that _isDirty = true and ScheduleSave() are
executed outside the _queryLock in AddContextMapping, allowing concurrent
AddContextMapping calls to interleave; move the _isDirty assignment and the
ScheduleSave() call inside the same lock scope protected by _queryLock (or
replace _isDirty with an atomic/Interlocked flag update and ensure
ScheduleSave() is invoked only once-safe) so the dirty flag and save scheduling
occur while holding the lock and cannot race with concurrent modifications.
In `@MCPForUnity/Editor/ActionTrace/Helpers/GameObjectTrackingHelper.cs`:
- Around line 22-26: Delete the unused static GameObjectTrackingHelper class
(the internal static class named GameObjectTrackingHelper with fields
_previousInstanceIds and _hasInitialized) to eliminate duplication with the
instance-based GameObjectTrackingHelper; remove the entire file or its class
declaration, confirm there are no remaining references to this static class, and
ensure the codebase continues to use the instance-based GameObjectTrackingHelper
(the one instantiated in UnityEventHooks.Advanced.cs) which provides the needed
name and GlobalID caching.
In `@MCPForUnity/Editor/ActionTrace/Semantics/DefaultEventScorer.cs`:
- Around line 37-47: The AINote override is bypassed by the early dehydrated
check in DefaultEventScorer; ensure AINote always scores 1.0f even when Payload
is null by moving or adding the evt.Type == "AINote" check before the
evt.Payload == null return (or explicitly return 1.0f for AINote when Payload is
null). Update the logic in the DefaultEventScorer method so the AINote branch
(evt.Type == "AINote") takes precedence over the payload-null low-score branch.
In `@MCPForUnity/Editor/ActionTrace/Sources/Helpers/GameObjectTrackingHelper.cs`:
- Around line 67-88: The method currently sets _hasInitialized = true even when
GameObject.FindObjectsOfType throws, leaving caches (_previousInstanceIds,
_nameCache, _globalIdCache) empty/partial; change the flow so _hasInitialized is
set only after the try block completes successfully (e.g., move the
_hasInitialized = true assignment inside the try after the foreach) and keep the
catch from setting the flag so future calls will retry enumeration; reference
GameObjectTrackingHelper, GameObject.FindObjectsOfType,
GlobalIdHelper.ToGlobalIdString, and the fields
_previousInstanceIds/_nameCache/_globalIdCache/_hasInitialized when making the
change.
In `@MCPForUnity/Editor/Windows/ActionTraceEditorWindow.cs`:
- Around line 477-495: Validate that the detail panel UI fields are present
before use: update ValidateRequiredElements() to also check _detailPlaceholder,
_detailContent, and _detailActions (the fields used in OnSelectionChanged) and
fail fast with a clear error or fallback if any are null; alternatively add a
unit/UITest that loads the UXML and asserts these three elements exist to
guarantee UXML completeness so OnSelectionChanged (and BuildDetailPanel) cannot
throw a NullReferenceException at runtime.
♻️ Duplicate comments (5)
MCPForUnity/Editor/ActionTrace/Integration/VCS/VcsContextProvider.cs (1)
60-66: Main‑thread Git polling still risks Editor stalls.
OnUpdate → RefreshContext → RunGitCommand executes Git synchronously on the update loop; even with a 5s timeout this can visibly freeze the Editor. Consider background polling and marshaling results to the main thread.Also applies to: 104-129, 168-228
MCPForUnity/Editor/ActionTrace/Context/ToolCallScope.cs (1)
495-526: Synchronous exception fromfunc()still leaks the scope.The
try/finallyinsideContinueWithhandles async completion, but iffunc()throws synchronously before returning a Task (line 503), the scope is never disposed. This was flagged in a previous review.MCPForUnity/Editor/ActionTrace/Semantics/DefaultIntentInferrer.cs (1)
18-26: LGTM - Null guard properly implemented.The
surrounding ??= Array.Empty<EditorEvent>()normalization on line 25 correctly addresses the null safety concern forIsReparentingandIsBatchOperationhelper methods.MCPForUnity/Editor/ActionTrace/Helpers/GameObjectTrackingHelper.cs (1)
96-139:FindObjectsOfTypeperformance concern already flagged.This concern was raised in a previous review. See past comment regarding throttling or event-driven detection to avoid per-frame scene traversal overhead.
MCPForUnity/Editor/ActionTrace/Analysis/Query/EventQueryBuilder.cs (1)
312-327:WithAllSearchTermsstill behaves like OR and accepts null terms.This remains the same issue as previously flagged: null/empty terms can throw, and "all terms" isn’t enforced.
Also applies to: 564-583
🟡 Minor comments (14)
MCPForUnity/Editor/ActionTrace/Integration/VCS/VcsContextProvider.cs-149-162 (1)
149-162: Detect Git worktrees and submodules (.git file) too.When a repository uses Git worktrees or submodules,
.gitis a plain text file (containinggitdir: <path>) rather than a directory. The current check using onlyDirectory.Exists()will fail in these cases, incorrectly reporting the repository as not found.Fix
- return System.IO.Directory.Exists(gitPath); + return System.IO.Directory.Exists(gitPath) + || System.IO.File.Exists(gitPath);MCPForUnity/Editor/ActionTrace/Core/Store/EventStore.Diagnostics.cs-51-60 (1)
51-60: Add null check for consistency withDehydrateOldEvents.
DehydrateOldEvents(line 23) checksevt != nullbefore accessing properties, butGetMemoryDiagnosticsdoesn't. If the_eventslist can contain null entries, this would throw aNullReferenceException.🐛 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/Capture/CapturePoints/EventCapturePointAttribute.cs-16-20 (1)
16-20: Guard against empty/invalid capture point IDs.
Fail fast ifidis null/empty to avoid silent misregistration.🐛 Proposed fix
public EventCapturePointAttribute(string id, string description = null, int priority = 0) { + if (string.IsNullOrWhiteSpace(id)) + throw new ArgumentException("Capture point id is required.", nameof(id)); Id = id; Description = description ?? id; Priority = priority; }MCPForUnity/Editor/Windows/ActionTraceEditorWindow.cs-714-726 (1)
714-726: JSON export may produce invalid or lossy JSON for payload values.All payload values are converted to strings (line 722-723), which:
- Quotes numeric values:
"size_bytes": "1234"instead of"size_bytes": 1234- Flattens nested objects via
ToString(): dictionaries becomeSystem.Collections.Generic.Dictionary...- Represents
nullas the string"null"instead of JSONnullConsider using Unity's
JsonUtilityor a JSON serializer for proper type handling.🐛 Sketch of improved approach
private string SerializePayloadValue(object value) { if (value == null) return "null"; if (value is bool b) return b ? "true" : "false"; if (value is int or long or float or double) return value.ToString(); if (value is IDictionary dict) return JsonUtility.ToJson(dict); // or custom serialization return $"\"{SanitizeJson(value.ToString())}\""; }MCPForUnity/Editor/ActionTrace/Capture/Recorder.cs-163-172 (1)
163-172: Semantic mismatch:EnteredEditModelogged asExitingPlayMode.
UnityEventHooks.OnPlayModeStateChangedsendsisPlaying=falseforPlayModeStateChange.EnteredEditMode(line 201-202 in UnityEventHooks.cs), but here it's mapped toExitingPlayMode. These are distinct Unity states—ExitingPlayModeis transient (about to exit), whileEnteredEditModeis the final state (already exited).🐛 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/Helpers/ActionTraceHelper.cs-20-29 (1)
20-29: Return value inconsistent with documentation when value isnull.The doc states: "Returns defaultValue (null) if not present or value is null." However, when the key exists but the value is
null, line 26 returnsnull?.ToString()(which isnull) instead ofdefaultValue.🐛 Proposed fix to match documentation
public static string GetPayloadString(this EditorEvent evt, string key, string defaultValue = null) { if (evt.Payload == null) return defaultValue; if (evt.Payload.TryGetValue(key, out var value)) - return value?.ToString(); + return value?.ToString() ?? defaultValue; return defaultValue; }MCPForUnity/Editor/Tools/ManageAsset.cs-497-504 (1)
497-504: Event invocation exception could mask successful modification.If
properties.ToObject<Dictionary<string, object>>()throws (e.g., due to unconvertible types), the outer catch block will return an error response even though the asset was successfully modified and saved. Consider wrapping the event invocation in a separate try-catch.Proposed fix
// === ActionTrace Integration: Notify subscribers (low-coupling) === - OnAssetModified?.Invoke( - fullPath, - asset.GetType().FullName, - properties.ToObject<Dictionary<string, object>>() - ); + try + { + OnAssetModified?.Invoke( + fullPath, + asset.GetType().FullName, + properties.ToObject<Dictionary<string, object>>() + ); + } + catch (Exception notifyEx) + { + McpLog.Warn($"[ManageAsset] Failed to notify OnAssetModified: {notifyEx.Message}"); + }MCPForUnity/Editor/ActionTrace/Capture/Capturers/UndoCapture.cs-51-70 (1)
51-70: BeginToolCall should handle already-in-progress tool calls.If
BeginToolCallis called while another tool call is in progress, it silently overwrites the existing state, potentially losing the ability to collapse the previous tool call's Undo operations. Consider either warning/aborting the previous call or rejecting the new one.Proposed fix
public static void BeginToolCall(string toolName, string toolCallId) { + if (_isInToolCall) + { + McpLog.Warn($"[UndoGroupManager] BeginToolCall called while already in tool call '{_currentToolName}'. Ending previous call."); + EndToolCall(); + } + if (string.IsNullOrEmpty(toolName)) { McpLog.Warn("[UndoGroupManager] BeginToolCall called with null toolName"); toolName = "AI Operation"; }MCPForUnity/Editor/ActionTrace/Capture/Capturers/AssetCapture.cs-181-202 (1)
181-202: Deduplication for deleted assets is unnecessary and pollutes the cache.Adding deleted asset paths to
_processedAssetsInSessionserves no purpose since deleted assets won't appear in subsequentOnPostprocessAllAssetscalls. This unnecessarily grows the cache and prevents proper cleanup. The same applies to moved assets (lines 209-211).Additionally,
hasChangesis only set for imported assets, so the cache won't be saved after processing deleted or moved assets.Proposed 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; - // L1 Blacklist: Skip junk assets if (!EventFilter.ShouldTrackAsset(assetPath)) continue; string targetId = $"Asset:{assetPath}"; var payload = new Dictionary<string, object> { ["path"] = assetPath }; RecordEvent(EventTypes.AssetDeleted, targetId, payload); } // ========== Moved Assets ========== for (int i = 0; i < movedAssets.Length; i++) { if (string.IsNullOrEmpty(movedAssets[i])) continue; - // L0 Deduplication: Skip if already processed in this session - if (!_processedAssetsInSession.Add(movedAssets[i])) - continue; - var fromPath = i < movedFromAssetPaths.Length ? movedFromAssetPaths[i] : ""; // L1 Blacklist: Skip junk assets if (!EventFilter.ShouldTrackAsset(movedAssets[i])) continue;MCPForUnity/Editor/Tools/UndoToSequenceTool.cs-131-143 (1)
131-143: Undo operations may silently fail if undo history is incomplete.
Undo.PerformUndo()fails silently if the undo stack is empty (e.g., after domain reload). The tool correctly documents this limitation (lines 25-27), but users won't know if fewer undos occurred than expected.Consider tracking actual undo count by checking
Undo.GetCurrentGroupName()before/after each operation, or warn users if undo count might be unreliable.Server/src/services/resources/action_trace.py-175-176 (1)
175-176: Silent exception swallowing loses debugging information.The static analysis correctly flags this: exceptions during event parsing are silently swallowed. Unlike
get_action_trace(line 114), this doesn't log the error, making debugging difficult when events fail to parse.🐛 Proposed fix: log the parsing error
for event_dict in events_data: try: event = EditorEvent(**event_dict) events.append(event) - except Exception: + except Exception as e: + # Log but continue - don't block statistics for one bad event + ctx.error(f"Failed to parse event for statistics: {e}") # type: ignore[reportUnusedCoroutine] continueMCPForUnity/Editor/ActionTrace/Capture/CapturePoints/EventCapturePointBase.cs-45-62 (1)
45-62: Remove the unusedcontextparameter fromRecordEvent.The
contextparameter (line 45) is never used.EventStore.Record()only accepts anEditorEventand does not have an overload that takes context. Either remove the parameter or, if context association is required, extendEventStore.Record()to accept it.MCPForUnity/Editor/ActionTrace/Core/Settings/ActionTraceSettings.cs-207-217 (1)
207-217:ApplyPresetis called before the asset is created, causing prematureSave().
ApplyPreset(line 211) callsMarkDirty()andSave(), which invokesEditorUtility.SetDirty(this)andAssetDatabase.SaveAssets(). However, the asset doesn't exist yet (created on line 212). This may cause warnings or silent failures.🐛 Proposed fix: apply preset values directly without saving
private static ActionTraceSettings CreateSettings() { var settings = CreateInstance<ActionTraceSettings>(); - // Apply Standard preset by default - settings.ApplyPreset(ActionTracePreset.Standard); + // Apply Standard preset values directly (don't call Save before asset exists) + var preset = ActionTracePreset.Standard; + settings.Filtering.MinImportanceForRecording = preset.MinImportance; + settings.Storage.MaxEvents = preset.MaxEvents; + settings.Storage.HotEventCount = preset.HotEventCount; + settings.Merging.EnableEventMerging = preset.EnableEventMerging; + settings.Merging.MergeWindowMs = preset.MergeWindowMs; + settings.Merging.TransactionWindowMs = preset.TransactionWindowMs; + settings._currentPresetName = preset.Name; AssetDatabase.CreateAsset(settings, SettingsPath); AssetDatabase.SaveAssets(); AssetDatabase.Refresh(); McpLog.Info($"[ActionTraceSettings] Created new settings at {SettingsPath}"); return settings; }MCPForUnity/Editor/ActionTrace/Analysis/Query/EventQueryBuilder.cs-40-53 (1)
40-53: Avoid int overflow in time range helpers.
minutes * 60000andhours * 3600000overflow on larger inputs; cast to long to keep range math correct.🛠️ Proposed fix
- return new QueryTimeRange(now - minutes * 60000, now); + return new QueryTimeRange(now - (long)minutes * 60000L, now); @@ - return new QueryTimeRange(now - hours * 3600000, now); + return new QueryTimeRange(now - (long)hours * 3600000L, now);
🧹 Nitpick comments (44)
MCPForUnity/Editor/ActionTrace/Sources/Helpers/GameObjectTrackingHelper.cs (1)
125-129: Cache GlobalId lookups to avoid repeated slow calls.
GlobalIdHelper.ToGlobalIdString(andGlobalObjectId.GetGlobalObjectIdSlow) are called for every object each detection pass. IfDetectChangesruns frequently, this becomes a hot path. Consider only recomputing when the object is first seen or renamed.♻️ Suggested optimization
- // Update name cache - _nameCache[id] = go.name; - // Update GlobalID cache (pre-death "will") - _globalIdCache[id] = GlobalIdHelper.ToGlobalIdString(go); + // Update name cache + var name = go.name; + if (!_nameCache.TryGetValue(id, out var prevName) || prevName != name) + { + _nameCache[id] = name; + // Recompute GlobalID only when first seen or renamed (fallbacks may use name/path) + _globalIdCache[id] = GlobalIdHelper.ToGlobalIdString(go); + } + else if (!_globalIdCache.ContainsKey(id)) + { + _globalIdCache[id] = GlobalIdHelper.ToGlobalIdString(go); + }MCPForUnity/Editor/ActionTrace/Integration/VCS/VcsContextProvider.cs (2)
45-81: Harden cross‑thread access to the cached context.
GetCurrentContext()is labeled thread‑safe, but_currentContextis mutable and published without a memory barrier; returning the shared instance also permits caller mutation. Considervolatile+ defensive copy (or makeVcsContextimmutable).♻️ Suggested hardening (defensive copy)
- private static VcsContext _currentContext; + private static volatile VcsContext _currentContext; @@ public static VcsContext GetCurrentContext() { - if (_currentContext == null) - { - _currentContext = GetInitialContext(); - } - - return _currentContext; + var ctx = _currentContext ?? (_currentContext = GetInitialContext()); + return ctx == null + ? VcsContext.CreateDefault() + : new VcsContext + { + CommitId = ctx.CommitId, + Branch = ctx.Branch, + IsDirty = ctx.IsDirty + }; }
240-282: Add a timeout to thegit --versionprobe.
WaitForExit()without a timeout can still hang the main thread in edge cases; mirror the timeout pattern used elsewhere.⏱️ Suggested timeout safeguard
using (var process = Process.Start(startInfo)) { - process.WaitForExit(); + if (!process.WaitForExit(2000)) + { + try { process.Kill(); } catch { } + return null; + } if (process.ExitCode == 0) return "git"; // Found in PATH }MCPForUnity/Editor/ActionTrace/Sources/Helpers/HookTest.cs (1)
1-84: Consider enabling this test code or removing it.This entire file is commented out. If it's intended as reusable test scaffolding, consider:
- Moving it to a dedicated test assembly where it can be conditionally compiled
- Using
#if UNITY_EDITOR_DEBUGor similar preprocessor directives instead of comment blocks- Removing it if it was only used during initial development
Keeping large blocks of commented code in the repository can lead to maintenance burden and confusion about whether the code is functional.
MCPForUnity/Editor/Windows/ActionTraceEditorWindow.uxml (1)
83-84: Consider using a more descriptive name for the duplicate clear-filters button.There's
clear-filters-buttonon line 45 andclear-filters-button-2on line 83. Using numeric suffixes can make code harder to maintain. Consider naming based on context, e.g.,clear-filters-button-no-resultsto clarify its purpose.♻️ Suggested fix
- <uie:ToolbarButton name="clear-filters-button-2" text="Clear Filters" class="empty-state-btn" /> + <uie:ToolbarButton name="clear-filters-button-no-results" text="Clear Filters" class="empty-state-btn" />MCPForUnity/Editor/Tools/AddActionTraceNoteTool.cs (2)
5-5: Unused import.
MCPForUnity.Editor.ActionTrace.Core.Settingsis imported but not used in this file.♻️ Suggested fix
using Newtonsoft.Json.Linq; using MCPForUnity.Editor.Helpers; -using MCPForUnity.Editor.ActionTrace.Core.Settings; using MCPForUnity.Editor.ActionTrace.Core.Store; using MCPForUnity.Editor.ActionTrace.Core.Models;
118-128: Inconsistent casing fallback patterns for optional fields.The fallback pattern for
intent(line 118) uses"intent" ?? "Intent"(lowercase → PascalCase), whileagent_model(line 124) uses"agent_model" ?? "agentModel"(snake_case → camelCase). This inconsistency could cause confusion.Consider standardizing on one pattern for all fields, e.g.,
snake_case ?? camelCaseto match the documented legacy/normalized approach.♻️ Suggested fix
// Optional fields - support both snake_case and camelCase - var intentToken = `@params`["intent"] ?? `@params`["Intent"]; + var intentToken = `@params`["intent"] ?? `@params`["intentValue"]; if (intentToken != null) { payload["intent"] = intentToken.ToString(); }Or if
intentis a single-word field that doesn't need snake_case/camelCase handling, simply use:- var intentToken = `@params`["intent"] ?? `@params`["Intent"]; + var intentToken = `@params`["intent"];Server/src/services/tools/get_action_trace.py (1)
95-96: Consider using explicit conversion flag for cleaner string formatting.The static analysis tool suggests using
{e!s}instead ofstr(e)for explicit conversion. This is a minor style improvement.♻️ Suggested improvement
except Exception as e: - return {"success": False, "message": f"Python error getting action trace: {str(e)}"} + return {"success": False, "message": f"Python error getting action trace: {e!s}"}MCPForUnity/Editor/ActionTrace/Capture/Filters/EventFilter.cs (3)
35-50: Silent exception swallowing hides invalid regex patterns.The catch block at lines 44-47 silently ignores regex compilation failures. Consider logging a warning so users can identify misconfigured patterns.
♻️ Suggested improvement
private Regex GetRegex() { if (_cachedRegex != null) return _cachedRegex; if (!string.IsNullOrEmpty(Pattern)) { try { _cachedRegex = new Regex(Pattern, RegexOptions.Compiled | RegexOptions.IgnoreCase); } - catch + catch (ArgumentException ex) { - // Invalid regex, return null + // Log warning for invalid regex pattern + UnityEngine.Debug.LogWarning($"[EventFilter] Invalid regex pattern '{Pattern}' in rule '{Name}': {ex.Message}"); } } return _cachedRegex; }
139-140: Incorrect rule type for.DS_StoreandThumbs.db.These are filenames, not extensions. The
Extensionrule type usesEndsWith, which will match paths ending with.DS_Storebut won't correctly identify these files in subdirectories (e.g.,foo/.DS_Storeworks, but conceptually these should beRegexor a dedicated filename type).More importantly,
Thumbs.dbdoesn't start with a dot, so treating it as an "extension" is semantically incorrect, though functionally it will still match paths ending withThumbs.db.♻️ Consider using Regex for filename matching
- new() { Name = ".DS_Store", Type = RuleType.Extension, Pattern = ".DS_Store", Action = FilterAction.Block, Priority = 90 }, - new() { Name = "Thumbs.db", Type = RuleType.Extension, Pattern = "Thumbs.db", Action = FilterAction.Block, Priority = 90 }, + new() { Name = ".DS_Store", Type = RuleType.Regex, Pattern = @"(^|/)\.DS_Store$", Action = FilterAction.Block, Priority = 90 }, + new() { Name = "Thumbs.db", Type = RuleType.Regex, Pattern = @"(^|/)Thumbs\.db$", Action = FilterAction.Block, Priority = 90 },
244-256: Volatile keyword is optional; Settings property is not actually written to in practice.The
_settingsfield is never assigned to via theSettingsproperty setter in the codebase. The only initialization occurs lazily in the getter (??= new EventFilterSettings()) on the main thread, and all reads throughEventFilter.ShouldTrackAsset()happen via Unity callbacks (OnPostprocessAllAssets,EditorApplication.update) which execute on the main thread. There are no actual background thread accesses to verify. If the design intent is to allow Settings modifications from background threads in the future, addingvolatilewould be a reasonable defensive measure; otherwise, it's unnecessary.MCPForUnity/Editor/ActionTrace/Analysis/Context/ContextCompressor.cs (2)
122-124: Potential integer overflow in time window calculation.If
RecentEventsWindowMinutesis large (e.g., > 35791 forint), the multiplication* 60 * 1000could overflow. Consider usinglongarithmetic explicitly.♻️ Suggested fix
long nowMs = DateTimeOffset.UtcNow.ToUnixTimeMilliseconds(); - long recentThresholdMs = nowMs - (_config.RecentEventsWindowMinutes * 60 * 1000); + long recentThresholdMs = nowMs - ((long)_config.RecentEventsWindowMinutes * 60 * 1000);
392-403: New scorer allocation on each call may impact performance.
GetHighImportancecreates a newDefaultEventScorerinstance on every invocation. For repeated calls or large event lists, consider accepting an optional scorer parameter or caching a static instance.♻️ Suggested improvement
- public static List<EditorEvent> GetHighImportance(this IReadOnlyList<EditorEvent> events, float threshold = 0.7f) + public static List<EditorEvent> GetHighImportance(this IReadOnlyList<EditorEvent> events, float threshold = 0.7f, IEventScorer scorer = null) { if (events == null || events.Count == 0) return new List<EditorEvent>(); - var scorer = new Semantics.DefaultEventScorer(); + scorer ??= new Semantics.DefaultEventScorer(); return events .Where(e => scorer.Score(e) >= threshold) .OrderByDescending(e => e.TimestampUnixMs) .ToList(); }MCPForUnity/Editor/ActionTrace/Core/Settings/ActionTraceSettings.Editor.cs (2)
65-75:ChangeCheckScopeis not being used correctly.The
EditorGUI.ChangeCheckScopeis created but thechangedproperty is never checked. The preset application logic inside the scope works by comparing indices, not by checking the scope'schangedflag. This works correctly but the scope wrapper is misleading.♻️ Suggested cleanup
using (new EditorGUILayout.HorizontalScope()) { EditorGUILayout.LabelField("Preset", GUILayout.Width(45)); - 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]); - } - } + 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]); + } }
86-96: Tooltip parameter is used as property label instead of hover tooltip.In
DrawProperty, thetooltipparameter is passed asGUIContenttext forPropertyField, but the display name is shown via a separateLabelFieldabove. This means the tooltip text appears as the property's label in the inspector rather than as a hover tooltip.♻️ Suggested fix for proper tooltip display
private void DrawProperty(string propertyName, string displayName, string tooltip) { var property = serializedObject.FindProperty(propertyName); if (property == null) return; EditorGUILayout.Space(2); - EditorGUILayout.LabelField(displayName, EditorStyles.boldLabel); - - // Expand nested property with all children visible - EditorGUILayout.PropertyField(property, new GUIContent(tooltip), includeChildren: true); + EditorGUILayout.PropertyField(property, new GUIContent(displayName, tooltip), includeChildren: true); }MCPForUnity/Editor/ActionTrace/Context/ToolCallScope.cs (1)
314-320: Consider thread-safety for CallId generation.
UnityEngine.Randomis not thread-safe. While the code already usesThreadLocalfor scope stack management and tracks thread IDs, theGenerateCallId()method at construction can still be called from multiple threads simultaneously (particularly throughExecuteAsync<T>). Consider usingSystem.Randomwith a thread-local instance orSystem.Security.Cryptography.RandomNumberGeneratorfor thread-safe random generation.MCPForUnity/Editor/ActionTrace/Capture/Sampling/SamplingStrategy.cs (1)
22-26: ValidatewindowMsto avoid invalid sampling windows.
A non‑positive window can cause undefined behavior in throttle/debounce logic. Consider guarding early.♻️ Proposed fix
- public SamplingStrategy(SamplingMode mode = SamplingMode.None, long windowMs = 1000) - { - Mode = mode; - WindowMs = windowMs; - } + public SamplingStrategy(SamplingMode mode = SamplingMode.None, long windowMs = 1000) + { + if (windowMs <= 0) + throw new ArgumentOutOfRangeException(nameof(windowMs), "windowMs must be > 0"); + Mode = mode; + WindowMs = windowMs; + }MCPForUnity/Editor/ActionTrace/Capture/Sampling/SamplingConfig.cs (1)
51-77: ValidateeventTypeinputs for sampling API.
Prevent accidental null/empty keys from polluting the strategy map.♻️ Proposed fix
public static void SetStrategy(string eventType, SamplingMode mode, long windowMs = 1000) { + if (string.IsNullOrWhiteSpace(eventType)) + throw new ArgumentException("eventType is required.", nameof(eventType)); Strategies[eventType] = new SamplingStrategy(mode, windowMs); } public static void RemoveStrategy(string eventType) { + if (string.IsNullOrWhiteSpace(eventType)) + throw new ArgumentException("eventType is required.", nameof(eventType)); Strategies.TryRemove(eventType, out _); } public static SamplingStrategy GetStrategy(string eventType) { + if (string.IsNullOrWhiteSpace(eventType)) + throw new ArgumentException("eventType is required.", nameof(eventType)); return Strategies.TryGetValue(eventType, out var strategy) ? strategy : null; }MCPForUnity/Editor/ActionTrace/Sources/Unity/UnityEventHooks.cs (2)
92-94: Lambda indelayCallcannot be unsubscribed on domain reload.The lambda
() => InitializeTracking()added toEditorApplication.delayCallcannot be removed duringUnsubscribeFromUnityEvents(). WhiledelayCallis a one-shot callback (auto-removed after execution), if a domain reload occurs before it fires, this could leave a dangling reference.Consider using a named method reference or a guard check inside
InitializeTracking()to ensure it's safe to call even after_isInitializedis reset.♻️ Suggested approach
- // Initialize tracking (one-time delayCall is safe) - EditorApplication.delayCall += () => InitializeTracking(); + // Initialize tracking (one-time delayCall is safe) + EditorApplication.delayCall += InitializeTrackingIfNeeded; + } + + private static void InitializeTrackingIfNeeded() + { + if (!_isInitialized) return; + InitializeTracking(); }
166-171: Consider usingDateTime.UtcNowfor debounce timing.
DateTime.UtcNowis marginally faster thanDateTime.Now(avoids timezone conversion) and eliminates edge cases during daylight saving time transitions. For debouncing logic, the absolute time doesn't matter—only the delta.MCPForUnity/Editor/ActionTrace/Capture/Capturers/SelectionCapture.cs (1)
194-213: Consider extractingGetGameObjectPathto a shared helper.This method is duplicated verbatim in
Recorder.cs(lines 312-326). Consider moving it to a shared location likeActionTraceHelperor creating aGameObjectPathHelper.MCPForUnity/Editor/Windows/ActionTraceEditorWindow.cs (2)
976-986: Redundant setting ofBypassImportanceFilterinSetSortMode.Line 982 sets
BypassImportanceFilter = true, but this is already done inCreateGUI(line 212). SinceOnDisablerestores the original value regardless, this is harmless but redundant.Consider removing the duplicate or adding a comment explaining why it's re-asserted here.
1077-1084: Consider optimizing auto-refresh to avoid unnecessary work.
OnEditorUpdatecallsRefreshEvents()every second unconditionally, which queries the store and rebuilds the view. For large event sets, this could impact editor performance.Consider checking if events have changed (e.g., comparing
EventStore.Countor a version counter) before performing a full refresh.♻️ Suggested optimization
private int _lastEventCount; private void OnEditorUpdate() { if (EditorApplication.timeSinceStartup - _lastRefreshTime > RefreshInterval) { _lastRefreshTime = EditorApplication.timeSinceStartup; // Only refresh if event count changed var currentCount = EventStore.Count; if (currentCount != _lastEventCount) { _lastEventCount = currentCount; RefreshEvents(); } } }MCPForUnity/Editor/ActionTrace/Capture/Capturers/AssetCapture.cs (2)
235-255: Documentation mismatch: comment says time-based expiration but implementation uses count-based.The comment states "Uses time-based expiration (30 minutes)" but the implementation clears the entire cache when it exceeds 1000 entries. Update the comment to match the actual behavior.
Proposed fix
/// <summary> /// Cleanup old entries from the cache to prevent unbounded growth. - /// Uses time-based expiration (30 minutes) instead of count-based. + /// Clears the entire cache when it exceeds MaxCacheSize entries. /// This is called at the start of each OnPostprocessAllAssets batch. /// </summary> private static void CleanupOldEntries() { if (_cachedProcessedAssets == null || _cachedProcessedAssets.Count == 0) return; - // Only cleanup periodically to avoid overhead - // Use a simple counter or timestamp-based approach const int MaxCacheSize = 1000; if (_cachedProcessedAssets.Count <= MaxCacheSize) return; - // If cache grows too large, clear it - // This is safe because re-processing old assets is extremely rare + // Clear entire cache when too large - safe because + // re-processing old assets is extremely rare _cachedProcessedAssets.Clear(); SaveProcessedAssets(); }
284-290: Consider logging exceptions instead of silently swallowing them.The empty
catchblock hides potential issues like permission errors or file system problems that could affect event classification accuracy.Proposed fix
catch (Exception ex) { - // On any error, default to treating as imported (conservative) + // On any error, log and default to treating as imported (conservative) + McpLog.Debug($"[AssetChangePostprocessor] IsNewlyCreatedAsset check failed for '{assetPath}': {ex.Message}"); return false; }MCPForUnity/Editor/ActionTrace/Analysis/Summarization/EventSummarizer.cs (1)
287-295: Edge case in Truncate when maxLength is very small.If
maxLengthis less than or equal toTruncatedSuffixLength(3), the substring length becomes 0 or negative (clamped to 0 byMath.Max), resulting in just"...". Consider adding a minimum length check.Proposed fix
private static string Truncate(string value, int maxLength) { if (string.IsNullOrEmpty(value) || value.Length <= maxLength) return value; + if (maxLength <= TruncatedSuffixLength) + return "..."; return value.Substring(0, Math.Max(0, maxLength - TruncatedSuffixLength)) + "..."; }MCPForUnity/Editor/ActionTrace/Capture/Capturers/UndoCapture.cs (1)
164-186: Minor code duplication in HandleActionTraceNote.The
groupNameassignment andUndo.SetCurrentGroupNamecall are duplicated in both branches. Consider extracting to reduce duplication.Proposed fix
public static string HandleActionTraceNote(string note, bool isTransactionEnd) { - string groupName; - - if (_isInToolCall) - { - // Use the AI note as the final Undo group name - groupName = $"AI: {note}"; - Undo.SetCurrentGroupName(groupName); - - if (isTransactionEnd) - { - EndToolCall(); - } - - return groupName; - } - - // Not in a tool call - just set the Undo name - groupName = $"AI: {note}"; + string groupName = $"AI: {note}"; Undo.SetCurrentGroupName(groupName); + + if (_isInToolCall && isTransactionEnd) + { + EndToolCall(); + } + return groupName; }MCPForUnity/Editor/ActionTrace/Capture/CapturePoints/EventCaptureRegistry.cs (2)
28-36: Consider checking for duplicate registrations.
Registerallows the same capture point to be added multiple times. If called accidentally with the same instance, duplicate entries would cause double initialization/shutdown and skewed statistics.♻️ Proposed fix
public void Register(IEventCapturePoint capturePoint) { if (capturePoint == null) return; + // Prevent duplicate registrations + if (_capturePoints.Exists(p => p.CapturePointId == capturePoint.CapturePointId)) + return; + _capturePoints.Add(capturePoint); // Sort by priority _capturePoints.Sort((a, b) => b.InitializationPriority.CompareTo(a.InitializationPriority)); }
111-122: Inconsistent snapshot semantics between accessor methods.
GetAllCapturePoints()returns a wrapper around the live list (modifications reflect immediately), whileGetEnabledCapturePoints()returns a true snapshot viaFindAll(). Consider making both consistent—either both return snapshots or document the difference.MCPForUnity/Editor/ActionTrace/Core/Store/EventStore.Persistence.cs (2)
45-87: Potential race in throttle check and_lastSaveTimeaccess.The throttle check at line 49 reads
_lastSaveTimewithout holding the lock, but_lastSaveTimeis written at line 77 inside the delayCall (also under lock). However, the initial read is unsynchronized. While unlikely to cause issues in Unity's single-threaded editor context, consider moving the throttle check inside the lock for correctness, or make_lastSaveTimevolatile.♻️ 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 scheduling + return; + } + // Only schedule if not already scheduled (prevents callback queue bloat) if (_saveScheduled) return; _saveScheduled = true; }
93-102: Minor:_saveScheduledreset outside lock.
_saveScheduled = falseat line 100 is outside_queryLock, which could race withScheduleSave. SinceClearPendingOperationsis called during shutdown/domain reload when no concurrent saves should be scheduled, this is likely safe in practice.MCPForUnity/Editor/ActionTrace/Capture/Capturers/PropertyCapture.cs (2)
90-159: Misleading indentation inProcessModifications.The
foreachloop body (lines 93-155) andreturnstatement (line 157) have inconsistent indentation relative to thelockblock. The code is functionally correct, but the formatting makes it harder to read. Consider reformatting for clarity.♻️ Suggested formatting
lock (_lock) { foreach (var undoMod in modifications) { - // UndoPropertyModification contains the PropertyModification and value changes + // UndoPropertyModification contains the PropertyModification and value changes // ... rest of body with consistent indentation ... - } + } - return modifications; + return modifications; } - }
229-253: Object pooling has limited benefit for structs.
PendingPropertyChangeis astruct(value type), so it's stored inline in the dictionary and copied by value. The object pool contains struct copies, not references, so eachPush/Popinvolves a copy anyway. The pool reduces the need to call the default constructor repeatedly but doesn't reduce GC pressure since structs aren't heap-allocated individually. Consider either:
- Making
PendingPropertyChangeaclassif pooling is important- Removing the pool since structs are already stack/inline allocated
MCPForUnity/Editor/ActionTrace/Helpers/GameObjectTrackingHelper.cs (1)
145-185:GetDestroyedInstanceIdsduplicates scene traversal logic fromDetectChanges.Both methods call
FindObjectsOfType<GameObject>(true)and update_previousInstanceIds. If both are called in the same frame, the scene is traversed twice. The instance-based helper inMCPForUnity/Editor/Helpers/GameObjectTrackingHelper.cshandles this by returning both changes and destroyed IDs from a singleDetectChanges()call viaGameObjectChangeResult.Consider either:
- Using the instance-based helper that already combines this logic
- Combining detection into a single method returning both new and destroyed objects
MCPForUnity/Editor/ActionTrace/Core/Store/EventStore.cs (1)
158-169: Pending notification limit check doesn't prevent growth beyond limit.The limit check resets
_notifyScheduledbut still adds the event at line 167. Between the check and the nextdelayCallexecution, more events can accumulate. Consider draining immediately when the limit is reached:♻️ Optional: Immediate drain when over limit
lock (_pendingNotifications) { // P1 Fix: Force drain if over limit to prevent unbounded growth if (_pendingNotifications.Count >= MaxPendingNotifications) { - _notifyScheduled = false; // Reset flag so we can drain immediately + // Drain immediately on main thread + var toNotify = new List<EditorEvent>(_pendingNotifications); + _pendingNotifications.Clear(); + _notifyScheduled = false; + foreach (var evt in toNotify) + EventRecorded?.Invoke(evt); } _pendingNotifications.Add(evtWithSequence); } - ScheduleNotify(); + if (!_notifyScheduled) + ScheduleNotify();MCPForUnity/Editor/Tools/UndoToSequenceTool.cs (1)
88-91: Consider usingQueryAll()instead ofQuery(limit: int.MaxValue).
EventStore.QueryAll()(defined at lines 244-266 in EventStore.cs) was specifically designed to avoid overflow checks and is more efficient for retrieving all events. The comment in EventStore says: "This is more efficient than Query(int.MaxValue) as it avoids overflow checks."♻️ Proposed change
- var eventsAfterTarget = EventStore.Query( - limit: int.MaxValue, // Get all events - sinceSequence: targetSequence - ).ToList(); + var eventsAfterTarget = EventStore.QueryAll() + .Where(e => e.Sequence > targetSequence) + .ToList();MCPForUnity/Editor/ActionTrace/Analysis/Summarization/TransactionAggregator.cs (1)
68-78: Minor: Redundant count check at line 72.The check
if (currentBatch.Count > 0)at line 72 is always true since we only enter this block whencurrentBatch.Count > 0(from line 68 where we getfirst = currentBatch[0]).♻️ Remove redundant check
if (ShouldSplit(first, evt)) { // Boundary reached - finalize current batch - if (currentBatch.Count > 0) - result.Add(CreateAtomicOperation(currentBatch)); + result.Add(CreateAtomicOperation(currentBatch)); // Start new batch with current event - clear and reuse list currentBatch.Clear();MCPForUnity/Editor/ActionTrace/Sources/Unity/UnityEventHooks.Advanced.cs (1)
90-108: Reflection on internal Unity type may break across Unity versions.
UnityEditor.Scripting.ScriptCompilationErrorCountis an internal Unity type that may not exist or have a different structure in future Unity versions. The fallback to0is safe but could mask compilation errors.Consider adding a log warning when reflection fails to help diagnose version incompatibility:
♻️ Add diagnostic logging
catch { + // Internal Unity API may not exist in this Unity version + // McpLog.Debug("[UnityEventHooks] ScriptCompilationErrorCount API not available"); return 0; }MCPForUnity/Editor/ActionTrace/Analysis/Query/ActionTraceQuery.cs (2)
177-178: Consider documenting the intent inference difference.
ProjectWithContextpassesnullfor surrounding events, producing potentially different intent inferences thanProject. While the comment mentions avoiding allocation, callers may expect consistent semantic quality. Consider adding a note in the XML doc (line 158-160) about this trade-off.
251-322: Consider usinginitaccessors for immutability.
ActionTraceViewItemis described as a "computed projection" but uses mutable setters. Usinginitaccessors would better communicate that these values should only be set during construction, aligning with the stated design intent.-public EditorEvent Event { get; set; } +public EditorEvent Event { get; init; }MCPForUnity/Editor/ActionTrace/Core/Settings/ActionTraceSettings.cs (1)
267-273:GetEstimatedMemoryUsagemay return incorrect value ifHotEventCount > MaxEvents.If settings are invalid (
HotEventCount > MaxEvents),coldEventsbecomes negative, producing an incorrect estimate. WhileValidate()catches this, defensive clamping would make this method robust.public long GetEstimatedMemoryUsage() { 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); }Server/src/services/resources/action_trace.py (1)
221-231: Consider single-pass min/max computation for efficiency.Creating intermediate lists (
sequences,timestamps) then computing min/max iterates the data twice. For 1000 events this is acceptable, but a single-pass approach would be more efficient.# Alternative: track min/max during the main loop min_seq = max_seq = events[0].sequence min_ts = max_ts = events[0].timestamp_unix_ms for event in events[1:]: min_seq = min(min_seq, event.sequence) max_seq = max(max_seq, event.sequence) # ... etcMCPForUnity/Editor/ActionTrace/Core/EventTypes.cs (1)
30-67: PromoteAINoteto a central constant.You already have a checklist for new events; using a string literal here risks drift (there’s also an
EventTypes.AINoteinActionTraceViewResource.cs). Consider adding a constant and reusing it in metadata.♻️ Proposed fix
// Build events public const string BuildStarted = "BuildStarted"; public const string BuildCompleted = "BuildCompleted"; public const string BuildFailed = "BuildFailed"; + public const string AINote = "AINote"; @@ - ["AINote"] = new EventMetadata + [AINote] = new EventMetadata { Category = EventCategory.System, DefaultImportance = 1.0f, SummaryTemplate = "AI Note{if:agent_id, ({agent_id})}: {note}", },Also applies to: 98-103
MCPForUnity/Editor/ActionTrace/Analysis/Query/EventQueryBuilder.cs (1)
792-797: Remove redundantToListextension method.This shadows LINQ's
ToList(which is imported viausing System.Linqat line 3). There are three calls to.ToList()in the same file (lines 412, 459, and 644), confirming the extension is used. Since the custom implementation is identical to LINQ'sToList(merely wraps the enumerable in aList<EditorEvent>), it adds no value and creates unnecessary shadowing. Remove it entirely and rely onSystem.Linq.ToList().
| static SelectionPropertyTracker() | ||
| { | ||
| // Initialize with current selection | ||
| UpdateSelectionState(); | ||
|
|
||
| // Monitor selection changes | ||
| Selection.selectionChanged += OnSelectionChanged; | ||
|
|
||
| // Monitor property modifications | ||
| Undo.postprocessModifications += OnPropertyModified; | ||
|
|
||
| McpLog.Debug("[SelectionPropertyTracker] Initialized"); | ||
| } |
There was a problem hiding this comment.
Missing unsubscription may cause duplicate event recording after domain reload.
Similar to ActionTraceRecorder, this class subscribes to Selection.selectionChanged and Undo.postprocessModifications without unsubscription logic. After a domain reload, duplicate subscriptions could cause events to be recorded multiple times.
🐛 Proposed fix pattern
static SelectionPropertyTracker()
{
+ // Unsubscribe first to prevent duplicates on domain reload
+ Selection.selectionChanged -= OnSelectionChanged;
+ Undo.postprocessModifications -= OnPropertyModified;
+
// Initialize with current selection
UpdateSelectionState();
// Monitor selection changes
Selection.selectionChanged += OnSelectionChanged;
// Monitor property modifications
Undo.postprocessModifications += OnPropertyModified;
McpLog.Debug("[SelectionPropertyTracker] Initialized");
}📝 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.
| static SelectionPropertyTracker() | |
| { | |
| // Initialize with current selection | |
| UpdateSelectionState(); | |
| // Monitor selection changes | |
| Selection.selectionChanged += OnSelectionChanged; | |
| // Monitor property modifications | |
| Undo.postprocessModifications += OnPropertyModified; | |
| McpLog.Debug("[SelectionPropertyTracker] Initialized"); | |
| } | |
| static SelectionPropertyTracker() | |
| { | |
| // Unsubscribe first to prevent duplicates on domain reload | |
| Selection.selectionChanged -= OnSelectionChanged; | |
| Undo.postprocessModifications -= OnPropertyModified; | |
| // Initialize with current selection | |
| UpdateSelectionState(); | |
| // Monitor selection changes | |
| Selection.selectionChanged += OnSelectionChanged; | |
| // Monitor property modifications | |
| Undo.postprocessModifications += OnPropertyModified; | |
| McpLog.Debug("[SelectionPropertyTracker] Initialized"); | |
| } |
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/ActionTrace/Capture/Capturers/SelectionCapture.cs` around
lines 34 - 46, SelectionPropertyTracker subscribes to Selection.selectionChanged
and Undo.postprocessModifications in its static constructor but never
unsubscribes, causing duplicate handlers after domain reloads; add a static
UnregisterHandlers method that removes the delegates from
Selection.selectionChanged and Undo.postprocessModifications, and register that
UnregisterHandlers with AssemblyReloadEvents.beforeAssemblyReload (or a similar
domain-reload hook) from the static constructor so the handlers are removed
before reload and won't be doubled when re-initialized.
| public static void EmitAssetModified(string assetPath, string assetType, IReadOnlyDictionary<string, object> changes) | ||
| { | ||
| if (string.IsNullOrEmpty(assetPath)) | ||
| { | ||
| McpLog.Warn("[ActionTraceEventEmitter] AssetModified with null path"); | ||
| return; | ||
| } | ||
|
|
||
| string targetId = $"Asset:{assetPath}"; | ||
|
|
||
| var payload = new Dictionary<string, object> | ||
| { | ||
| ["path"] = assetPath, | ||
| ["asset_type"] = assetType ?? "Unknown", | ||
| ["changes"] = changes ?? new Dictionary<string, object>(), | ||
| ["source"] = "mcp_tool" // Indicates this change came from an MCP tool call | ||
| }; |
There was a problem hiding this comment.
Preserve changes payload by cloning IReadOnlyDictionary.
EditorEvent sanitization only handles IDictionary; IReadOnlyDictionary values are treated as plain IEnumerable and get stripped. This drops changes for asset-modified events.
🐛 Proposed fix
- ["changes"] = changes ?? new Dictionary<string, object>(),
+ ["changes"] = changes != null
+ ? new Dictionary<string, object>(changes)
+ : new Dictionary<string, object>(),🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/ActionTrace/Capture/Emitter.cs` around lines 415 - 431,
EmitAssetModified is passing the IReadOnlyDictionary changes straight into
payload which later gets mis-sanitized; clone changes into a concrete
IDictionary before adding to payload so EditorEvent sees it as IDictionary
(e.g., create a new Dictionary<string, object> clonedChanges = changes != null ?
new Dictionary<string, object>(changes) : new Dictionary<string, object>() and
assign payload["changes"] = clonedChanges). Update the EmitAssetModified method
to use this cloned dictionary (reference variables: EmitAssetModified, changes,
payload) so the changes payload is preserved.
| case SamplingMode.Debounce: | ||
| case SamplingMode.DebounceByKey: | ||
| // Debounce: Keep only the last event in the window | ||
| // Note: Must update the dictionary entry since PendingSample is a struct | ||
| _pendingSamples[key] = new PendingSample | ||
| { | ||
| Event = evt, | ||
| TimestampMs = nowMs | ||
| }; | ||
| return false; |
There was a problem hiding this comment.
Debounce window resets on each update, preventing emission of rapidly-changing events.
When an event arrives within the debounce window, TimestampMs is reset to nowMs (line 184). This extends the window indefinitely for rapidly-changing values (e.g., continuous slider drag), potentially never emitting. Standard debounce behavior keeps the original window start and only updates the event payload.
🐛 Proposed fix: preserve original timestamp
case SamplingMode.Debounce:
case SamplingMode.DebounceByKey:
// Debounce: Keep only the last event in the window
- // Note: Must update the dictionary entry since PendingSample is a struct
_pendingSamples[key] = new PendingSample
{
Event = evt,
- TimestampMs = nowMs
+ TimestampMs = pending.TimestampMs // Keep original window start
};
return false;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case SamplingMode.Debounce: | |
| case SamplingMode.DebounceByKey: | |
| // Debounce: Keep only the last event in the window | |
| // Note: Must update the dictionary entry since PendingSample is a struct | |
| _pendingSamples[key] = new PendingSample | |
| { | |
| Event = evt, | |
| TimestampMs = nowMs | |
| }; | |
| return false; | |
| case SamplingMode.Debounce: | |
| case SamplingMode.DebounceByKey: | |
| // Debounce: Keep only the last event in the window | |
| _pendingSamples[key] = new PendingSample | |
| { | |
| Event = evt, | |
| TimestampMs = pending.TimestampMs // Keep original window start | |
| }; | |
| return false; |
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/ActionTrace/Capture/Sampling/SamplingMiddleware.cs` around
lines 177 - 186, The debounce branch in SamplingMiddleware (cases
SamplingMode.Debounce / DebounceByKey) resets PendingSample.TimestampMs to nowMs
on each incoming evt, which extends the debounce window; instead, when updating
the _pendingSamples[key] PendingSample struct replace only the Event payload
with evt but preserve the existing TimestampMs if an entry already exists
(create a new PendingSample with TimestampMs = existing.TimestampMs, Event =
evt), and only set TimestampMs = nowMs when inserting the first pending sample
(i.e., when no existing entry for key). This keeps the original window start
while still updating the last event payload.
| static ContextTrace() | ||
| { | ||
| // Subscribe to event recording | ||
| // EventStore already uses delayCall, so this won't cause re-entrancy | ||
| EventStore.EventRecorded += OnEventRecorded; | ||
| } |
There was a problem hiding this comment.
Missing unsubscription from EventStore.EventRecorded.
Following the pattern seen in other [InitializeOnLoad] classes, this subscription persists across domain reloads. If EventStore.EventRecorded uses standard C# events, duplicate handlers will accumulate.
🐛 Proposed fix
static ContextTrace()
{
// Subscribe to event recording
// EventStore already uses delayCall, so this won't cause re-entrancy
+ EventStore.EventRecorded -= OnEventRecorded;
EventStore.EventRecorded += OnEventRecorded;
}📝 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.
| static ContextTrace() | |
| { | |
| // Subscribe to event recording | |
| // EventStore already uses delayCall, so this won't cause re-entrancy | |
| EventStore.EventRecorded += OnEventRecorded; | |
| } | |
| static ContextTrace() | |
| { | |
| // Subscribe to event recording | |
| // EventStore already uses delayCall, so this won't cause re-entrancy | |
| EventStore.EventRecorded -= OnEventRecorded; | |
| EventStore.EventRecorded += OnEventRecorded; | |
| } |
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/ActionTrace/Context/ContextTimeline.cs` around lines 24 -
29, The static constructor in ContextTrace subscribes EventStore.EventRecorded
via OnEventRecorded but never unsubscribes, causing handler accumulation across
domain reloads; update ContextTrace to unsubscribe in an assembly/domain-unload
callback by registering a cleanup that calls EventStore.EventRecorded -=
OnEventRecorded (for example subscribe a lambda to
UnityEditor.AssemblyReloadEvents.beforeAssemblyReload or AppDomain.DomainUnload
if appropriate) so the OnEventRecorded handler is removed when the editor
reloads; ensure the unsubscribe references the same static method name
OnEventRecorded so duplicates are prevented.
| public static void RemoveContextMappings(Guid contextId) | ||
| { | ||
| lock (_queryLock) | ||
| { | ||
| _contextMappings.RemoveAll(m => m.ContextId == contextId); | ||
| } | ||
| // Mark dirty and schedule deferred save | ||
| _isDirty = true; | ||
| ScheduleSave(); | ||
| } |
There was a problem hiding this comment.
Same race condition pattern as AddContextMapping.
The _isDirty = true assignment is outside the lock, creating the same potential race condition. Apply the same fix here.
🐛 Proposed fix
lock (_queryLock)
{
_contextMappings.RemoveAll(m => m.ContextId == contextId);
+ _isDirty = true;
}
- // Mark dirty and schedule deferred save
- _isDirty = true;
ScheduleSave();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static void RemoveContextMappings(Guid contextId) | |
| { | |
| lock (_queryLock) | |
| { | |
| _contextMappings.RemoveAll(m => m.ContextId == contextId); | |
| } | |
| // Mark dirty and schedule deferred save | |
| _isDirty = true; | |
| ScheduleSave(); | |
| } | |
| public static void RemoveContextMappings(Guid contextId) | |
| { | |
| lock (_queryLock) | |
| { | |
| _contextMappings.RemoveAll(m => m.ContextId == contextId); | |
| _isDirty = true; | |
| } | |
| ScheduleSave(); | |
| } |
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/ActionTrace/Core/Store/EventStore.Context.cs` around lines
76 - 85, RemoveContextMappings currently sets _isDirty = true and calls
ScheduleSave() outside the lock used for _contextMappings, creating the same
race as AddContextMapping; move the _isDirty = true assignment (and optionally
ScheduleSave() if you followed AddContextMapping pattern) inside the lock
protected by _queryLock so the mutation and dirty flag update are atomic with
the RemoveAll operation, referencing the RemoveContextMappings method,
_queryLock, _contextMappings, _isDirty and ScheduleSave.
| internal static class GameObjectTrackingHelper | ||
| { | ||
| // Use HashSet for O(1) Contains() instead of List's O(n) | ||
| private static readonly HashSet<int> _previousInstanceIds = new(256); | ||
| private static bool _hasInitialized; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find all GameObjectTrackingHelper class definitions and usages
rg "GameObjectTrackingHelper" --type=cs -B2 -A5 | head -150Repository: CoplayDev/unity-mcp
Length of output: 6129
Remove unused static GameObjectTrackingHelper class.
Two GameObjectTrackingHelper implementations exist: a static version at MCPForUnity/Editor/ActionTrace/Helpers/GameObjectTrackingHelper.cs (the file under review) and an instance-based version at MCPForUnity/Editor/ActionTrace/Sources/Helpers/GameObjectTrackingHelper.cs. The instance-based version is the only one in use (instantiated in UnityEventHooks.Advanced.cs) and includes additional features like name and GlobalID caching. The static version appears unused and should be removed to eliminate duplication.
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/ActionTrace/Helpers/GameObjectTrackingHelper.cs` around
lines 22 - 26, Delete the unused static GameObjectTrackingHelper class (the
internal static class named GameObjectTrackingHelper with fields
_previousInstanceIds and _hasInitialized) to eliminate duplication with the
instance-based GameObjectTrackingHelper; remove the entire file or its class
declaration, confirm there are no remaining references to this static class, and
ensure the codebase continues to use the instance-based GameObjectTrackingHelper
(the one instantiated in UnityEventHooks.Advanced.cs) which provides the needed
name and GlobalID caching.
| // 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; |
There was a problem hiding this comment.
AINote can be mis-scored when dehydrated.
The early payload-null return prevents the AINote override from ever firing for dehydrated AINote events, contradicting “always critical.”
🐛 Proposed fix
- // 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;
+ // Special case: AINote is always critical
+ if (evt.Type == EventTypes.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;📝 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.
| // 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; | |
| // Special case: AINote is always critical | |
| if (evt.Type == EventTypes.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; |
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/ActionTrace/Semantics/DefaultEventScorer.cs` around lines
37 - 47, The AINote override is bypassed by the early dehydrated check in
DefaultEventScorer; ensure AINote always scores 1.0f even when Payload is null
by moving or adding the evt.Type == "AINote" check before the evt.Payload ==
null return (or explicitly return 1.0f for AINote when Payload is null). Update
the logic in the DefaultEventScorer method so the AINote branch (evt.Type ==
"AINote") takes precedence over the payload-null low-score branch.
| try | ||
| { | ||
| GameObject[] allObjects = GameObject.FindObjectsOfType<GameObject>(true); | ||
| foreach (var go in allObjects) | ||
| { | ||
| if (go != null) | ||
| { | ||
| int id = go.GetInstanceID(); | ||
| _previousInstanceIds.Add(id); | ||
| _nameCache[id] = go.name; | ||
| // Cache GlobalID for cross-session stable reference | ||
| _globalIdCache[id] = GlobalIdHelper.ToGlobalIdString(go); | ||
| } | ||
| } | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| McpLog.Warn($"[GameObjectTrackingHelper] Failed to initialize GameObject tracking: {ex.Message}"); | ||
| } | ||
|
|
||
| _hasInitialized = true; | ||
| } |
There was a problem hiding this comment.
Don’t mark tracking initialized on failed enumeration.
If FindObjectsOfType throws, Line 87 still sets _hasInitialized = true, so future calls won’t retry and caches may stay empty/partial.
🔧 Proposed fix
try
{
GameObject[] allObjects = GameObject.FindObjectsOfType<GameObject>(true);
foreach (var go in allObjects)
{
if (go != null)
{
int id = go.GetInstanceID();
_previousInstanceIds.Add(id);
_nameCache[id] = go.name;
// Cache GlobalID for cross-session stable reference
_globalIdCache[id] = GlobalIdHelper.ToGlobalIdString(go);
}
}
+ _hasInitialized = true;
}
catch (Exception ex)
{
McpLog.Warn($"[GameObjectTrackingHelper] Failed to initialize GameObject tracking: {ex.Message}");
}
-
- _hasInitialized = true;🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/ActionTrace/Sources/Helpers/GameObjectTrackingHelper.cs`
around lines 67 - 88, The method currently sets _hasInitialized = true even when
GameObject.FindObjectsOfType throws, leaving caches (_previousInstanceIds,
_nameCache, _globalIdCache) empty/partial; change the flow so _hasInitialized is
set only after the try block completes successfully (e.g., move the
_hasInitialized = true assignment inside the try after the foreach) and keep the
catch from setting the flag so future calls will retry enumeration; reference
GameObjectTrackingHelper, GameObject.FindObjectsOfType,
GlobalIdHelper.ToGlobalIdString, and the fields
_previousInstanceIds/_nameCache/_globalIdCache/_hasInitialized when making the
change.
| private void OnSelectionChanged(IEnumerable<object> items) | ||
| { | ||
| _detailPlaceholder.style.display = DisplayStyle.None; | ||
| _detailContent.style.display = DisplayStyle.Flex; | ||
| _detailActions.style.display = DisplayStyle.Flex; | ||
|
|
||
| _detailContent.Clear(); | ||
| _selectedItem = items.FirstOrDefault() as ActionTraceQuery.ActionTraceViewItem; | ||
|
|
||
| if (_selectedItem == null) | ||
| { | ||
| _detailPlaceholder.style.display = DisplayStyle.Flex; | ||
| _detailContent.style.display = DisplayStyle.None; | ||
| _detailActions.style.display = DisplayStyle.None; | ||
| return; | ||
| } | ||
|
|
||
| BuildDetailPanel(_selectedItem); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the ActionTraceEditorWindow.cs file
fd ActionTraceEditorWindow -t f --extension csRepository: CoplayDev/unity-mcp
Length of output: 116
🏁 Script executed:
# Search for where _detailPlaceholder, _detailContent, _detailActions are initialized
rg "_detailPlaceholder|_detailContent|_detailActions" --type=cs -B 2 -A 2Repository: CoplayDev/unity-mcp
Length of output: 3490
🏁 Script executed:
# Check for tests related to ActionTraceEditorWindow
fd -t f -g "*Test*" -g "*.cs" | xargs rg -l "ActionTraceEditorWindow" 2>/dev/null || echo "No test files found"Repository: CoplayDev/unity-mcp
Length of output: 329
🏁 Script executed:
# Look at the structure of ActionTraceEditorWindow class to understand initialization
rg "class ActionTraceEditorWindow|OnEnable|LoadUXML|CreateGUI|_detailPlaceholder\s*=" --type=cs -A 3Repository: CoplayDev/unity-mcp
Length of output: 5944
🏁 Script executed:
# Get the full CreateGUI method and surrounding code
sed -n '1,300p' MCPForUnity/Editor/Windows/ActionTraceEditorWindow.cs | tail -150Repository: CoplayDev/unity-mcp
Length of output: 6669
🏁 Script executed:
# Look for all test files in the repository
fd -t f "Test" --extension cs | head -20Repository: CoplayDev/unity-mcp
Length of output: 1965
🏁 Script executed:
# Search for references to ActionTraceEditorWindow in test files
find . -name "*.cs" -type f | xargs grep -l "ActionTraceEditorWindow" 2>/dev/null || echo "No test references found"Repository: CoplayDev/unity-mcp
Length of output: 193
🏁 Script executed:
# Look at the EditorPrefsWindow.cs that was mentioned in learnings to understand the pattern
sed -n '70,120p' MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.csRepository: CoplayDev/unity-mcp
Length of output: 1753
Add validation for detail panel UI elements or ensure UXML completeness through tests.
ValidateRequiredElements() checks only 4 UI elements but omits _detailPlaceholder, _detailContent, and _detailActions—all accessed immediately in OnSelectionChanged (lines 479-481, 486-488). If the UXML is missing these elements, the window will crash with NullReferenceException. Either extend ValidateRequiredElements() to include these elements, or ensure tests verify UXML contains all required elements.
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/Windows/ActionTraceEditorWindow.cs` around lines 477 -
495, Validate that the detail panel UI fields are present before use: update
ValidateRequiredElements() to also check _detailPlaceholder, _detailContent, and
_detailActions (the fields used in OnSelectionChanged) and fail fast with a
clear error or fallback if any are null; alternatively add a unit/UITest that
loads the UXML and asserts these three elements exist to guarantee UXML
completeness so OnSelectionChanged (and BuildDetailPanel) cannot throw a
NullReferenceException at runtime.
- Refactor ContextCompressor for better context handling - Enhance AssetCapture with improved tracking - Update SamplingMiddleware for better event sampling - Extend ToolCallScope with enhanced functionality - Remove ActionTraceSettings.Editor.cs (consolidated) - Update ActionTraceSettings core implementation - Improve EventStore.Diagnostics logging - Refine UndoToSequenceTool behavior - Major restructure of ActionTraceEditorWindow UI Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 17
🤖 Fix all issues with AI agents
In `@MCPForUnity/Editor/ActionTrace/Analysis/Context/ContextCompressor.cs`:
- Around line 73-79: SummaryEvents and SummaryCount in ContextCompressor are
declared but never populated because SummarizeEvents currently returns a string
instead of creating summary EditorEvent entries; either populate them or remove
the fields. Fix by updating the SummarizeEvents method (in class
ContextCompressor) to construct EditorEvent summary objects and add them to the
SummaryEvents list while incrementing SummaryCount (and ensure SummaryEvents is
initialized), or if summaries are not needed, remove the SummaryEvents field and
SummaryCount variable and any references to them (including any external uses)
to avoid dead state. Ensure tests or callers of SummarizeEvents expect the new
behavior (return value vs side-effects) and update method signature or docs
accordingly.
- Around line 49-53: TargetCompressionRatio on ContextCompressor is defined but
unused; either remove it or make compression honor it. Update ContextCompressor
(e.g., Compress or CompressContext/CompressBlock and any token scoring logic
like CompressTokenScores) so the compressor stops removing tokens or blocks when
compressedSize / originalSize <= TargetCompressionRatio (or inversely when
compression ratio target reached) — i.e., use TargetCompressionRatio as the
termination condition or factor into selection thresholds/scoring; alternatively
delete the TargetCompressionRatio field and related docs if you choose not to
support configurable ratio.
- Around line 437-444: The composite string key used in ContextCompressor
(inside the events.OrderBy loop) can collide if evt.Type or evt.TargetId contain
'|'; replace the string concat key with a robust composite key such as a
ValueTuple (e.g., (evt.Type, evt.TargetId, evt.TimestampUnixMs / windowMs)) and
change the seen collection to HashSet<ValueTuple<...>> (or use a delimiter
guaranteed not to appear like '\0' if you must keep strings) so seen.Add(...)
and the duplicate-check logic around result.Add(evt) use the tuple-based key
instead of the '|'-joined string.
In `@MCPForUnity/Editor/ActionTrace/Capture/Capturers/AssetCapture.cs`:
- Around line 253-268: The XML doc and comments for CleanupOldEntries
incorrectly claim time-based expiration; update the documentation and inline
comments to accurately describe that eviction is count-based using MaxCacheSize,
and mention that when _cachedProcessedAssets.Count exceeds MaxCacheSize the
cache is cleared and persisted via SaveProcessedAssets; reference the
CleanupOldEntries method, the _cachedProcessedAssets collection, the
MaxCacheSize constant and SaveProcessedAssets in your changes.
- Around line 141-159: Imported assets currently remove filtered paths from the
_processedAssetsInSession set when EventFilter.ShouldTrackAsset(assetPath)
returns false, but the deleted and moved assets handling does not, causing
inconsistent behavior; update the deleted-assets and moved-assets blocks (the
loops handling deletedAssets and movedAssets and the
movedFromAssets/movedToAssets pairs) to also call
_processedAssetsInSession.Remove(assetPath) (or the appropriate
moved-from/moved-to path) whenever EventFilter.ShouldTrackAsset returns false so
filtered entries are removed from the tracking set consistently across imported,
deleted, and moved handling.
- Around line 280-303: Replace the fragile filesystem creation-time heuristic in
IsNewlyCreatedAsset with a GUID-based check: open and parse the assetPath +
".meta" to extract the "guid" field (or use Unity's AssetDatabase GUID
utilities) and determine newness by comparing that GUID against your
known/previous asset GUIDs (or by checking whether AssetDatabase contains a
record for that GUID); stop using System.IO.File.GetCreationTimeUtc and the
5-second threshold, and handle/paranoid-log parsing errors instead of silently
swallowing exceptions so failures are visible (keep IsNewlyCreatedAsset
signature and assetPath usage to locate the file).
In `@MCPForUnity/Editor/ActionTrace/Capture/Recorder.cs`:
- Around line 106-123: OnComponentRemovedDetailed uses a raw
args.ComponentInstanceId when args.Owner is null which doesn't match the
"Instance:"-prefixed global-id format used elsewhere; update the null-owner
branch that sets targetId to produce a normalized global id (e.g. call
GlobalIdHelper.ToGlobalIdString with the component instance id or construct the
same "Instance:{id}" prefix) so RecordEvent receives a consistent ID format;
adjust the targetId assignment to use
GlobalIdHelper.ToGlobalIdString(args.ComponentInstanceId) (or equivalent)
instead of args.ComponentInstanceId.ToString().
In `@MCPForUnity/Editor/ActionTrace/Context/ToolCallScope.cs`:
- Around line 334-340: GenerateCallId in ToolCallScope currently uses
UnityEngine.Random.Range which is not main-thread safe; replace that call with a
thread-safe generator such as a ThreadLocal<System.Random> (or use
Guid.NewGuid()) to produce the random suffix; update GenerateCallId to compute
the random component via the thread-local Random instance (or a truncated Guid
string) instead of UnityEngine.Random so ToolCallScope can be created on
background threads safely.
In `@MCPForUnity/Editor/ActionTrace/Core/Settings/ActionTraceSettings.cs`:
- Around line 276-282: GetEstimatedMemoryUsage can produce a negative coldEvents
when Storage.HotEventCount > Storage.MaxEvents; update the calculation in
GetEstimatedMemoryUsage to clamp coldEvents to zero (e.g., coldEvents =
Math.Max(0, Storage.MaxEvents - Storage.HotEventCount)) before computing the
estimate so the returned memory never decreases due to a negative count;
reference Storage.HotEventCount, Storage.MaxEvents and Validate() (which flags
the condition) when locating the change.
- Around line 217-227: The CreateSettings method currently calls
ApplyPreset(ActionTracePreset.Standard) before making the ScriptableObject an
on-disk asset, which causes ApplyPreset->Save()->EditorUtility.SetDirty(this) to
operate on an in-memory object; change the order so
CreateInstance<ActionTraceSettings>() is followed by
AssetDatabase.CreateAsset(settings, SettingsPath) (and SaveAssets/Refresh), and
only then call settings.ApplyPreset(ActionTracePreset.Standard) so
Save()/EditorUtility.SetDirty operate on the actual asset; keep the McpLog.Info
and return unchanged.
- Around line 171-187: ApplyPreset currently calls Save() (which calls
MarkDirty/AssetDatabase.SaveAssets) while CreateSettings creates the instance
only after ApplyPreset is invoked; change the flow so that ApplyPreset does not
invoke Save() during instance creation: either move the ApplyPreset call to
after AssetDatabase.CreateAsset in CreateSettings, or split ApplyPreset into
ApplyPresetValues (which sets Filtering/Storage/Merging and _currentPresetName
and calls MarkDirty but does NOT call Save) and keep Save() separate; update
CreateSettings to CreateInstance<ActionTraceSettings>(),
AssetDatabase.CreateAsset(...), then call ApplyPreset/Save as appropriate,
referencing ApplyPreset, CreateSettings, Save, AssetDatabase.CreateAsset and
MarkDirty.
In `@MCPForUnity/Editor/ActionTrace/Core/Store/EventStore.Diagnostics.cs`:
- Around line 57-70: The comment "DehydrateOldEvents can leave nulls" is
misleading because DehydrateOldEvents (method DehydrateOldEvents) replaces
entries with evt.Dehydrate() rather than nulling them; update the foreach over
_events in EventStore.Diagnostics (the loop using variable evt and checking evt
== null) to either remove the incorrect comment or replace it with an accurate
note (e.g., guarding against unexpected nulls from other code paths) and keep
the null check only if you want to defensively handle unexpected nulls; ensure
references to evt.IsDehydrated, evt.Payload, hydratedCount, dehydratedCount, and
EstimatePayloadSize(evt.Payload) remain unchanged.
In `@MCPForUnity/Editor/Tools/UndoToSequenceTool.cs`:
- Around line 149-162: Wrap the Undo.PerformUndo loop in a try-catch so an
exception doesn't leave a partial undo state: track a counter of completed undos
and on exception log the exception plus how many steps completed (use
Debug.LogError or process logger) and rethrow or handle accordingly; replace the
EditorSceneManager.MarkSceneDirty call inside the EditorApplication.delayCall
with a non-destructive UI refresh such as calling
UnityEditorInternal.InternalEditorUtility.RepaintAllViews() (or
SceneView.RepaintAll and EditorApplication.RepaintHierarchyWindow() if
preferred) to avoid marking the scene dirty while still updating the UI; ensure
you reference the existing Undo.PerformUndo loop, the
EditorApplication.delayCall block, and remove or stop using
EditorSceneManager.MarkSceneDirty.
In `@MCPForUnity/Editor/Windows/ActionTraceEditorWindow.cs`:
- Around line 405-423: The early-return in RefreshEvents prevents UI changes
(importance toggle, context toggle, sort/clear) from updating the list; modify
RefreshEvents to accept a boolean forceRefresh (default false) or include the
relevant UI state in its guard, then call RefreshEvents(true) from the callbacks
wired in SetupToolbar (the _searchField, _importanceToggle, _contextToggle
RegisterValueChangedCallback handlers) and from places that change sort/filters;
alternatively remove the restrictive guard and instead compare current vs cached
filter/sort/_showSemantics/_showContext/_searchText to decide refresh so toggles
like _showSemantics/_showContext and _eventListView.RefreshItems always reflect
changes.
- Around line 1-5: The code in ActionTraceEditorWindow is formatting floats
using the current thread culture (around the float formatting at the locations
reported near lines 674 and 723), which breaks JSON/CSV in comma-decimal
locales; update the float-to-string calls to use CultureInfo.InvariantCulture
(e.g., value.ToString(CultureInfo.InvariantCulture) or
string.Format(CultureInfo.InvariantCulture, ...)) when building the JSON/CSV
output and add "using System.Globalization;" if missing so all numeric fields
are emitted with invariant (dot) decimal formatting.
- Around line 651-757: JSON export is corrupted because SanitizeJson reuses and
Clears the shared field _stringBuilder used by BuildJsonExport; change
SanitizeJson to allocate and use a local StringBuilder (e.g., new StringBuilder
with capacity based on input.Length) instead of _stringBuilder so it doesn't
clear the shared buffer, keep the same escape switch logic and same empty-string
return for null/empty input, and leave BuildJsonExport / BuildCsvExport
unchanged.
- Around line 971-981: The importance cutoff is currently only enforced when
_sortMode == SortMode.AIFiltered; update FilterEvent (method
FilterEvent(ActionTraceQuery.ActionTraceViewItem e)) to apply the _minImportance
check regardless of _sortMode so the UI's importance slider actually filters in
all modes—i.e., remove the SortMode.AIFiltered guard and return false when
e.ImportanceScore < _minImportance, then keep the existing search-text checks
using _searchText, e.DisplaySummaryLower, e.DisplayTargetIdLower and
e.Event.Type.ToLowerInvariant() as before.
♻️ Duplicate comments (3)
MCPForUnity/Editor/ActionTrace/Capture/Sampling/SamplingMiddleware.cs (1)
183-191: Debounce window keeps extending; trailing event may never emit.
ResettingTimestampMson each event (Line 187-191) means continuous changes can preventFlushExpiredDebounceSamplesfrom ever firing, which conflicts with “only record the last event within the window.” Preserve the original window start when a pending sample already exists.🐛 Proposed fix (preserve original window start)
case SamplingMode.Debounce: case SamplingMode.DebounceByKey: // Debounce: Keep only the last event in the window // Note: Must update the dictionary entry since PendingSample is a struct _pendingSamples[key] = new PendingSample { Event = evt, - TimestampMs = nowMs + TimestampMs = pending.TimestampMs }; return false;MCPForUnity/Editor/ActionTrace/Context/ToolCallScope.cs (1)
521-546: Synchronous exception fromfunc()still leaks the scope.The
finallyblock insideContinueWithhandles disposal after async completion, but iffunc()throws synchronously before returning aTask, the scope is never disposed.🐛 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/Windows/ActionTraceEditorWindow.cs (1)
211-221: Detail panel elements aren’t validated before use.OnSelectionChanged (Line 451+) assumes
_detailPlaceholder,_detailContent, and_detailActionsexist, butValidateRequiredElements()doesn’t verify them. If the UXML is missing these nodes, the window will throw at runtime. Prefer a UI/UX test that asserts these elements exist (or expand the centralized validation), rather than sprinkling null checks. Based on learnings, prefer tests to guarantee UXML completeness.Also applies to: 451-456
🧹 Nitpick comments (15)
Server/src/models/action_trace.py (1)
49-51: Update to Pydantic v2 configuration style.
class Configis deprecated in Pydantic v2. Additionally,populate_by_namehas no effect since no field aliases are defined (the Python field names already match the expected JSON keys).♻️ Suggested fix
If you need
populate_by_namefor future alias support, update to v2 style:+from pydantic import BaseModel, ConfigDict, Field ... - class Config: - """Pydantic configuration""" - populate_by_name = True # Allow using aliases + model_config = ConfigDict(populate_by_name=True)Otherwise, remove the unused config entirely.
MCPForUnity/Editor/Tools/UndoToSequenceTool.cs (2)
35-48: Parameters class appears unused.The
Parametersclass is defined with proper attributes butHandleCommandmanually parses theJObjectinstead of deserializing to this class. This creates a maintenance burden where changes must be made in two places.Consider using the
Parametersclass for deserialization:♻️ Suggested approach
public static object HandleCommand(JObject `@params`) { try { - // Parse required parameter - long targetSequence = 0; - var sequenceToken = `@params`["sequence_id"] ?? `@params`["sequenceId"]; - if (sequenceToken == null || !long.TryParse(sequenceToken.ToString(), out targetSequence)) - { - return new ErrorResponse("sequence_id parameter is required and must be a number."); - } - - // Optional dry_run parameter - bool dryRun = false; - var dryRunToken = `@params`["dry_run"] ?? `@params`["dryRun"]; - if (dryRunToken != null) - { - bool.TryParse(dryRunToken.ToString(), out dryRun); - } + var parameters = `@params.ToObject`<Parameters>(); + if (parameters == null || parameters.SequenceId == 0) + { + return new ErrorResponse("sequence_id parameter is required and must be a number."); + } + long targetSequence = parameters.SequenceId; + bool dryRun = parameters.DryRun;Alternatively, if manual parsing is intentional (e.g., to support both snake_case and camelCase), remove or mark the
Parametersclass as documentation-only.
64-78: Consider logging when boolean parsing fails.The
bool.TryParseon line 77 silently ignores parse failures. If a client sends"yes"or"1", it will silently default tofalserather thantrue, which might be unexpected.♻️ Optional: Add warning for unexpected values
var dryRunToken = `@params`["dry_run"] ?? `@params`["dryRun"]; if (dryRunToken != null) { - bool.TryParse(dryRunToken.ToString(), out dryRun); + if (!bool.TryParse(dryRunToken.ToString(), out dryRun)) + { + McpLog.Warning($"[UndoToSequenceTool] Could not parse dry_run value '{dryRunToken}', defaulting to false"); + } }MCPForUnity/Editor/ActionTrace/Analysis/Context/ContextCompressor.cs (3)
109-174: Code duplication withCompressWithDetails.The core compression logic (event categorization, sorting, size limiting) is duplicated between
CompressandCompressWithDetails. Consider extracting the shared logic into a private helper method that both public methods can delegate to.Additionally,
PreservedEventsis sorted at line 141, then the merged result is sorted again at line 171. Since both lists will be merged and re-sorted, the intermediate sort is redundant.♻️ Suggested refactor
public List<EditorEvent> Compress(IReadOnlyList<EditorEvent> events) { - if (events == null || events.Count == 0) - return new List<EditorEvent>(); - - var result = new CompressedContext - { - OriginalCount = events.Count, - PreservedEvents = new List<EditorEvent>(), - DehydratedEvents = new List<EditorEvent>(), - SummaryEvents = new List<EditorEvent>() - }; - - long nowMs = DateTimeOffset.UtcNow.ToUnixTimeMilliseconds(); - long recentThresholdMs = nowMs - (_config.RecentEventsWindowMinutes * 60 * 1000); - - // Separate events into categories - foreach (var evt in events) - { - if (ShouldPreserveFull(evt, nowMs, recentThresholdMs)) - { - result.PreservedEvents.Add(evt); - } - else - { - var dehydrated = evt.Dehydrate(); - result.DehydratedEvents.Add(dehydrated); - } - } - - // Sort preserved events by timestamp - result.PreservedEvents.Sort((a, b) => a.TimestampUnixMs.CompareTo(b.TimestampUnixMs)); - - // Limit total count if needed - int totalAfterPreserve = result.PreservedEvents.Count + result.DehydratedEvents.Count; - if (totalAfterPreserve > _config.MaxCompressedEvents) - { - // Keep all preserved, trim dehydrated - int maxDehydrated = _config.MaxCompressedEvents - result.PreservedEvents.Count; - if (maxDehydrated < 0) maxDehydrated = 0; - - // Keep most recent dehydrated events - result.DehydratedEvents = result.DehydratedEvents - .OrderByDescending(e => e.TimestampUnixMs) - .Take(maxDehydrated) - .ToList(); - } - - // Update statistics - result.PreservedCount = result.PreservedEvents.Count; - result.DehydratedCount = result.DehydratedEvents.Count; - result.CompressionRatio = result.OriginalCount > 0 - ? (float)result.TotalEvents / result.OriginalCount - : 1f; - - // Combine results - var compressed = new List<EditorEvent>(result.TotalEvents); - compressed.AddRange(result.PreservedEvents); - compressed.AddRange(result.DehydratedEvents); - - // Sort by timestamp - compressed.Sort((a, b) => a.TimestampUnixMs.CompareTo(b.TimestampUnixMs)); - - return compressed; + var details = CompressWithDetails(events); + var compressed = new List<EditorEvent>(details.TotalEvents); + compressed.AddRange(details.PreservedEvents); + compressed.AddRange(details.DehydratedEvents); + compressed.Sort((a, b) => a.TimestampUnixMs.CompareTo(b.TimestampUnixMs)); + return compressed; }
279-286: Consider using a HashSet forAlwaysKeepEventTypeslookups.The linear search through
AlwaysKeepEventTypesruns for every event processed. Converting this to aHashSet<string>(cached once in the constructor or lazily) would provide O(1) lookups instead of O(n).♻️ Suggested optimization
Add a cached HashSet field:
private readonly HashSet<string> _alwaysKeepTypesSet; public ContextCompressor(ContextCompressionConfig config = null, IEventScorer scorer = null) { _config = config ?? new ContextCompressionConfig(); _scorer = scorer ?? new Semantics.DefaultEventScorer(); _alwaysKeepTypesSet = _config.AlwaysKeepEventTypes != null ? new HashSet<string>(_config.AlwaysKeepEventTypes, StringComparer.Ordinal) : new HashSet<string>(); }Then simplify the check:
-if (_config.AlwaysKeepEventTypes != null) -{ - foreach (string type in _config.AlwaysKeepEventTypes) - { - if (evt.Type == type) - return true; - } -} +if (_alwaysKeepTypesSet.Contains(evt.Type)) + return true;
398-409: Consider allowing scorer injection or caching.A new
DefaultEventScoreris instantiated on every call. If this method is called frequently, consider either:
- Adding an optional
IEventScorerparameter for caller-provided scorers- Using a static cached instance since
DefaultEventScorerappears stateless♻️ Suggested approach
+private static readonly IEventScorer DefaultScorer = new DefaultEventScorer(); + public static List<EditorEvent> GetHighImportance( - this IReadOnlyList<EditorEvent> events, float threshold = 0.7f) + this IReadOnlyList<EditorEvent> events, + float threshold = 0.7f, + IEventScorer scorer = null) { if (events == null || events.Count == 0) return new List<EditorEvent>(); - var scorer = new Semantics.DefaultEventScorer(); + scorer ??= DefaultScorer; return events .Where(e => scorer.Score(e) >= threshold) .OrderByDescending(e => e.TimestampUnixMs) .ToList(); }MCPForUnity/Editor/ActionTrace/Context/ToolCallScope.cs (3)
157-163: Consider thread safety for_capturedEvents.If a
ToolCallScopeinstance is shared across threads (e.g., passed to a background task), concurrent calls toRecordEventcould corrupt the list. Since scopes are typically thread-local, this may be acceptable, but consider documenting the thread-affinity requirement or using a concurrent collection.
326-332: Silent no-op if scope is not on top of stack.
PopFromStacksilently does nothing if this scope isn't on top, which could mask stack corruption from improper nesting (e.g., disposing scopes out of order). Consider logging a warning in debug builds when the top of stack doesn't match expectations.🔧 Suggested diagnostic logging
private void PopFromStack() { if (_scopeStack.Value.Count > 0 && _scopeStack.Value.Peek() == this) { _scopeStack.Value.Pop(); } + `#if` UNITY_EDITOR && DEBUG + else if (_scopeStack.Value.Count > 0) + { + UnityEngine.Debug.LogWarning($"ToolCallScope stack mismatch: expected {CallId} on top, found {_scopeStack.Value.Peek()?.CallId}"); + } + `#endif` }
529-533: UseInnerExceptionfor clearer error messages.
t.Exceptionis anAggregateExceptionwhoseMessageis generic ("One or more errors occurred"). UseInnerExceptionfor more actionable error messages.♻️ Proposed improvement
if (t.IsFaulted) { - scope.Fail(t.Exception?.Message ?? "Async faulted"); - throw t.Exception ?? new Exception("Async task faulted"); + var innerEx = t.Exception?.InnerException; + scope.Fail(innerEx?.Message ?? "Async faulted"); + throw innerEx ?? t.Exception ?? new Exception("Async task faulted"); }MCPForUnity/Editor/ActionTrace/Capture/Capturers/AssetCapture.cs (1)
309-318: Consider adding.csto tracked modification types or documenting exclusion.Scripts (
.csfiles) are notably absent fromShouldTrackModification. This means script modifications fall through toAssetImported(line 186) rather thanAssetModified. If this is intentional (scripts trigger recompilation which is tracked elsewhere), consider adding a comment. Otherwise, this might be an oversight.MCPForUnity/Editor/ActionTrace/Core/Settings/ActionTraceSettings.cs (2)
54-73: Potential issue:MinKeepEventsis not validated againstMaxEvents.
MinKeepEventsdefaults to 100, but there's no validation that it's less than or equal toMaxEvents. If a user setsMaxEventsto 100 andMinKeepEventsto 100, the auto-cleaning might not behave as expected. TheValidate()method only checksHotEventCountvsMaxEvents.Add validation for MinKeepEvents
public List<string> Validate() { var issues = new List<string>(); // Dynamic validation: HotEventCount should not exceed MaxEvents (runtime check) if (Storage.HotEventCount > Storage.MaxEvents) issues.Add("HotEventCount should not exceed MaxEvents"); + if (Storage.MinKeepEvents > Storage.MaxEvents) + issues.Add("MinKeepEvents should not exceed MaxEvents"); + return issues; }
140-154: Singleton pattern lacks thread safety.The
Instanceproperty uses a simple null check without synchronization. While Unity editor code typically runs on the main thread, ifInstanceis accessed from a background thread (e.g., during async operations), this could lead to race conditions creating multiple instances.Given that the PR mentions thread-safety considerations elsewhere ("reads allowed from any thread"), consider adding a lock or using
Lazy<T>.Thread-safe singleton option
+ private static readonly object _instanceLock = new object(); + public static ActionTraceSettings Instance { get { - if (_instance == null) + if (_instance == null) { - _instance = LoadSettings(); - if (_instance == null) + lock (_instanceLock) { - _instance = CreateSettings(); + if (_instance == null) + { + _instance = LoadSettings(); + if (_instance == null) + { + _instance = CreateSettings(); + } + } } } return _instance; } }MCPForUnity/Editor/ActionTrace/Core/Store/EventStore.Diagnostics.cs (1)
49-51:hotEventsandcoldEventsin diagnostics may not match actual hydration state.Lines 50-51 compute
hotEventsandcoldEventsbased on settings and total count, but these are theoretical values based on configuration. The actual hydrated/dehydrated counts (lines 53-69) may differ if dehydration hasn't run or events were added rapidly. Consider clarifying in the output that "Hot Events" is the configured window, not actual hydration state.Clarify diagnostic output
return $"EventStore Memory Diagnostics:\n" + $" Total Events: {totalEvents}/{maxEvents}\n" + - $" Hot Events (full payload): {hotEvents}\n" + - $" Cold Events (dehydrated): {coldEvents}\n" + + $" Hot Window (configured): {hotEventCount}\n" + + $" Cold Window (configured): {Math.Max(0, totalEvents - hotEventCount)}\n" + $" Hydrated: {hydratedCount}\n" + $" Dehydrated: {dehydratedCount}\n" +MCPForUnity/Editor/ActionTrace/Capture/CapturePoints/EventCaptureRegistry.cs (2)
28-36: Avoid silent no‑op when registering after initialization.If
Registeris called afterInitializeAll, the new capture point never gets initialized. Consider auto‑initializing in that case or emitting a warning/exception to enforce the “register before Start” contract.
17-20: Document or enforce main-thread access for registry operations.
_capturePointsis accessed without synchronization while getters return liveAsReadOnlywrappers of the underlying list. Although this code runs in the Editor context (typically main-thread only), this assumption isn't documented. Add a comment clarifying that all registry operations must occur on the main thread, or guard mutations with a lock if background access is possible.
| /// <summary> | ||
| /// Target compression ratio (0.0 - 1.0). | ||
| /// 1.0 = no compression, 0.1 = aggressive compression. | ||
| /// </summary> | ||
| public float TargetCompressionRatio = 0.3f; |
There was a problem hiding this comment.
TargetCompressionRatio is defined but never used.
This configuration field is documented and exposed but has no effect on compression behavior. Either implement compression logic that respects this ratio or remove the unused field to avoid confusing consumers of the API.
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/ActionTrace/Analysis/Context/ContextCompressor.cs` around
lines 49 - 53, TargetCompressionRatio on ContextCompressor is defined but
unused; either remove it or make compression honor it. Update ContextCompressor
(e.g., Compress or CompressContext/CompressBlock and any token scoring logic
like CompressTokenScores) so the compressor stops removing tokens or blocks when
compressedSize / originalSize <= TargetCompressionRatio (or inversely when
compression ratio target reached) — i.e., use TargetCompressionRatio as the
termination condition or factor into selection thresholds/scoring; alternatively
delete the TargetCompressionRatio field and related docs if you choose not to
support configurable ratio.
| public List<EditorEvent> SummaryEvents; | ||
|
|
||
| // Statistics | ||
| public int OriginalCount; | ||
| public int PreservedCount; | ||
| public int DehydratedCount; | ||
| public int SummaryCount; |
There was a problem hiding this comment.
SummaryEvents and SummaryCount are never populated.
The SummaryEvents list is initialized but never receives any events. SummaryCount remains 0. The SummarizeEvents method exists but returns a string rather than populating this list. This appears to be an incomplete feature.
Either implement summary event generation or remove these unused fields to avoid confusion.
Would you like me to help implement summary event generation, or should these fields be removed?
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/ActionTrace/Analysis/Context/ContextCompressor.cs` around
lines 73 - 79, SummaryEvents and SummaryCount in ContextCompressor are declared
but never populated because SummarizeEvents currently returns a string instead
of creating summary EditorEvent entries; either populate them or remove the
fields. Fix by updating the SummarizeEvents method (in class ContextCompressor)
to construct EditorEvent summary objects and add them to the SummaryEvents list
while incrementing SummaryCount (and ensure SummaryEvents is initialized), or if
summaries are not needed, remove the SummaryEvents field and SummaryCount
variable and any references to them (including any external uses) to avoid dead
state. Ensure tests or callers of SummarizeEvents expect the new behavior
(return value vs side-effects) and update method signature or docs accordingly.
| foreach (var evt in events.OrderBy(e => e.TimestampUnixMs)) | ||
| { | ||
| string key = $"{evt.Type}|{evt.TargetId}|{evt.TimestampUnixMs / windowMs}"; | ||
|
|
||
| if (seen.Add(key)) | ||
| { | ||
| result.Add(evt); | ||
| } |
There was a problem hiding this comment.
Potential key collision in deduplication.
The composite key uses | as a delimiter. If evt.Type or evt.TargetId contains a pipe character, distinct events could hash to the same key, causing incorrect deduplication.
Consider using a tuple-based approach or a delimiter that's guaranteed not to appear in the values (e.g., null character \0).
🔧 Suggested fix
-string key = $"{evt.Type}|{evt.TargetId}|{evt.TimestampUnixMs / windowMs}";
+string key = $"{evt.Type}\0{evt.TargetId}\0{evt.TimestampUnixMs / windowMs}";Or use a tuple with a custom comparer if string allocation is a concern.
📝 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.
| foreach (var evt in events.OrderBy(e => e.TimestampUnixMs)) | |
| { | |
| string key = $"{evt.Type}|{evt.TargetId}|{evt.TimestampUnixMs / windowMs}"; | |
| if (seen.Add(key)) | |
| { | |
| result.Add(evt); | |
| } | |
| foreach (var evt in events.OrderBy(e => e.TimestampUnixMs)) | |
| { | |
| string key = $"{evt.Type}\0{evt.TargetId}\0{evt.TimestampUnixMs / windowMs}"; | |
| if (seen.Add(key)) | |
| { | |
| result.Add(evt); | |
| } |
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/ActionTrace/Analysis/Context/ContextCompressor.cs` around
lines 437 - 444, The composite string key used in ContextCompressor (inside the
events.OrderBy loop) can collide if evt.Type or evt.TargetId contain '|';
replace the string concat key with a robust composite key such as a ValueTuple
(e.g., (evt.Type, evt.TargetId, evt.TimestampUnixMs / windowMs)) and change the
seen collection to HashSet<ValueTuple<...>> (or use a delimiter guaranteed not
to appear like '\0' if you must keep strings) so seen.Add(...) and the
duplicate-check logic around result.Add(evt) use the tuple-based key instead of
the '|'-joined string.
| foreach (var assetPath in importedAssets) | ||
| { | ||
| if (string.IsNullOrEmpty(assetPath)) continue; | ||
|
|
||
| // L0 Deduplication: Skip if already processed in this session | ||
| // This prevents duplicate events when Unity fires OnPostprocessAllAssets | ||
| // multiple times for the same asset (creation, compilation, re-import) | ||
| if (!_processedAssetsInSession.Add(assetPath)) | ||
| continue; // Already processed, skip to prevent duplicate events | ||
|
|
||
| hasChanges = true; // Mark that we added a new entry | ||
|
|
||
| // L1 Blacklist: Skip junk assets before creating events | ||
| if (!EventFilter.ShouldTrackAsset(assetPath)) | ||
| { | ||
| // Remove from tracking if it's a junk asset (we don't want to track it) | ||
| _processedAssetsInSession.Remove(assetPath); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Inconsistent removal from tracking set on filter failure.
For imported assets, when EventFilter.ShouldTrackAsset returns false, the asset is removed from _processedAssetsInSession (line 157). However, for deleted assets (lines 201-203) and moved assets (lines 228-230), filtered assets remain in the set without removal. This inconsistency could cause filtered deleted/moved assets to never be processed if they appear again in future batches.
Suggested fix for consistency
// L1 Blacklist: Skip junk assets
if (!EventFilter.ShouldTrackAsset(assetPath))
+ {
+ _processedAssetsInSession.Remove(assetPath);
continue;
+ }Apply similar pattern to moved assets block (lines 228-230).
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/ActionTrace/Capture/Capturers/AssetCapture.cs` around
lines 141 - 159, Imported assets currently remove filtered paths from the
_processedAssetsInSession set when EventFilter.ShouldTrackAsset(assetPath)
returns false, but the deleted and moved assets handling does not, causing
inconsistent behavior; update the deleted-assets and moved-assets blocks (the
loops handling deletedAssets and movedAssets and the
movedFromAssets/movedToAssets pairs) to also call
_processedAssetsInSession.Remove(assetPath) (or the appropriate
moved-from/moved-to path) whenever EventFilter.ShouldTrackAsset returns false so
filtered entries are removed from the tracking set consistently across imported,
deleted, and moved handling.
| private static void CleanupOldEntries() | ||
| { | ||
| if (_cachedProcessedAssets == null || _cachedProcessedAssets.Count == 0) | ||
| return; | ||
|
|
||
| // Only cleanup periodically to avoid overhead | ||
| // Use a simple counter or timestamp-based approach | ||
| const int MaxCacheSize = 1000; | ||
| if (_cachedProcessedAssets.Count <= MaxCacheSize) | ||
| return; | ||
|
|
||
| // If cache grows too large, clear it | ||
| // This is safe because re-processing old assets is extremely rare | ||
| _cachedProcessedAssets.Clear(); | ||
| SaveProcessedAssets(); | ||
| } |
There was a problem hiding this comment.
Cleanup comment mentions time-based expiration but implementation is count-based.
The XML doc (lines 249-250) states "Uses time-based expiration (30 minutes)" but the implementation only checks MaxCacheSize (count-based). The comment is misleading.
Suggested doc fix
/// <summary>
/// Cleanup old entries from the cache to prevent unbounded growth.
- /// Uses time-based expiration (30 minutes) instead of count-based.
+ /// Uses count-based limit to prevent unbounded growth.
/// This is called at the start of each OnPostprocessAllAssets batch.
/// </summary>🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/ActionTrace/Capture/Capturers/AssetCapture.cs` around
lines 253 - 268, The XML doc and comments for CleanupOldEntries incorrectly
claim time-based expiration; update the documentation and inline comments to
accurately describe that eviction is count-based using MaxCacheSize, and mention
that when _cachedProcessedAssets.Count exceeds MaxCacheSize the cache is cleared
and persisted via SaveProcessedAssets; reference the CleanupOldEntries method,
the _cachedProcessedAssets collection, the MaxCacheSize constant and
SaveProcessedAssets in your changes.
| for (int i = 0; i < stepsToUndo; i++) | ||
| { | ||
| Undo.PerformUndo(); | ||
| } | ||
|
|
||
| // Force GUI refresh to update the scene (with validity check) | ||
| EditorApplication.delayCall += () => | ||
| { | ||
| var scene = UnityEditor.SceneManagement.EditorSceneManager.GetActiveScene(); | ||
| if (scene.IsValid() && scene.isLoaded) | ||
| { | ||
| UnityEditor.SceneManagement.EditorSceneManager.MarkSceneDirty(scene); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Partial undo state on exception and MarkSceneDirty side effect.
Two concerns with the execution logic:
-
Partial revert on exception: If an exception occurs mid-loop (e.g., after 3 of 5 undos), the editor is left in an intermediate state. Consider wrapping in a try-catch that at least reports how many steps completed.
-
MarkSceneDirty side effect: Using
MarkSceneDirty()for GUI refresh will mark the scene as having unsaved changes, which may confuse users who didn't make intentional edits.
🛠️ Suggested improvements
// Perform the undo operations
McpLog.Info($"[UndoToSequenceTool] Reverting {stepsToUndo} Undo steps to reach sequence {targetSequence}");
+int stepsCompleted = 0;
for (int i = 0; i < stepsToUndo; i++)
{
Undo.PerformUndo();
+ stepsCompleted++;
}
// Force GUI refresh to update the scene (with validity check)
EditorApplication.delayCall += () =>
{
- var scene = UnityEditor.SceneManagement.EditorSceneManager.GetActiveScene();
- if (scene.IsValid() && scene.isLoaded)
- {
- UnityEditor.SceneManagement.EditorSceneManager.MarkSceneDirty(scene);
- }
+ // Repaint scene views without marking dirty
+ UnityEditorInternal.InternalEditorUtility.RepaintAllViews();
};Using RepaintAllViews() refreshes the UI without marking the scene dirty. However, verify this provides sufficient refresh for your use case.
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/Tools/UndoToSequenceTool.cs` around lines 149 - 162, Wrap
the Undo.PerformUndo loop in a try-catch so an exception doesn't leave a partial
undo state: track a counter of completed undos and on exception log the
exception plus how many steps completed (use Debug.LogError or process logger)
and rethrow or handle accordingly; replace the EditorSceneManager.MarkSceneDirty
call inside the EditorApplication.delayCall with a non-destructive UI refresh
such as calling UnityEditorInternal.InternalEditorUtility.RepaintAllViews() (or
SceneView.RepaintAll and EditorApplication.RepaintHierarchyWindow() if
preferred) to avoid marking the scene dirty while still updating the UI; ensure
you reference the existing Undo.PerformUndo loop, the
EditorApplication.delayCall block, and remove or stop using
EditorSceneManager.MarkSceneDirty.
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using System.Text; | ||
| using MCPForUnity.Editor.Helpers; |
There was a problem hiding this comment.
Use invariant culture for JSON/CSV numeric fields.
Line 674 and Line 723 format floats with current culture. In comma-decimal locales, JSON becomes invalid and CSV columns shift. Use CultureInfo.InvariantCulture.
✅ Proposed fix
-using System.Text;
+using System.Text;
+using System.Globalization;
@@
- _stringBuilder.AppendLine($" \"importanceScore\": {e.ImportanceScore:F2},");
+ _stringBuilder.AppendLine($" \"importanceScore\": {e.ImportanceScore.ToString("F2", CultureInfo.InvariantCulture)},");
@@
- _stringBuilder.Append(e.ImportanceScore.ToString("F2"));
+ _stringBuilder.Append(e.ImportanceScore.ToString("F2", CultureInfo.InvariantCulture));Also applies to: 674-675, 723-724
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/Windows/ActionTraceEditorWindow.cs` around lines 1 - 5,
The code in ActionTraceEditorWindow is formatting floats using the current
thread culture (around the float formatting at the locations reported near lines
674 and 723), which breaks JSON/CSV in comma-decimal locales; update the
float-to-string calls to use CultureInfo.InvariantCulture (e.g.,
value.ToString(CultureInfo.InvariantCulture) or
string.Format(CultureInfo.InvariantCulture, ...)) when building the JSON/CSV
output and add "using System.Globalization;" if missing so all numeric fields
are emitted with invariant (dot) decimal formatting.
| private void SetupToolbar() | ||
| { | ||
| _searchField?.RegisterValueChangedCallback(e => | ||
| { | ||
| _searchText = e.newValue.ToLowerInvariant(); | ||
| RefreshEvents(); | ||
| }); | ||
|
|
||
| _importanceToggle?.RegisterValueChangedCallback(e => | ||
| { | ||
| _showSemantics = e.newValue; | ||
| _eventListView.RefreshItems(); | ||
| }); | ||
|
|
||
| _contextToggle?.RegisterValueChangedCallback(e => | ||
| { | ||
| _showContext = e.newValue; | ||
| RefreshEvents(); | ||
| }); |
There was a problem hiding this comment.
RefreshEvents early-return can block filter/sort/context updates.
The guard at Line 835 only checks store count and search text, so changing importance, sort, context toggle, or clearing filters can leave the UI stale. Consider adding a forced refresh path (or include filter/sort/context state in the guard).
💡 Proposed fix (force refresh when UI state changes)
- private void RefreshEvents()
+ private void RefreshEvents(bool force = false)
{
// 性能优化：检查是否有新事件
int currentStoreCount = EventStore.Count;
- if (currentStoreCount == _lastEventStoreCount &&
- _currentEvents.Count > 0 &&
- string.IsNullOrEmpty(_searchText))
+ if (!force && currentStoreCount == _lastEventStoreCount &&
+ _currentEvents.Count > 0 &&
+ string.IsNullOrEmpty(_searchText))
{
// 没有新事件且无搜索条件，跳过刷新
return;
}
_lastEventStoreCount = currentStoreCount;
@@
- _searchField?.RegisterValueChangedCallback(e =>
+ _searchField?.RegisterValueChangedCallback(e =>
{
_searchText = e.newValue.ToLowerInvariant();
- RefreshEvents();
+ RefreshEvents(force: true);
});
@@
- _contextToggle?.RegisterValueChangedCallback(e =>
+ _contextToggle?.RegisterValueChangedCallback(e =>
{
_showContext = e.newValue;
- RefreshEvents();
+ RefreshEvents(force: true);
});
@@
- RefreshEvents();
+ RefreshEvents(force: true);
@@
- RefreshEvents();
+ RefreshEvents(force: true);
@@
- RefreshEvents();
+ RefreshEvents(force: true);Also applies to: 784-789, 803-809, 832-843, 986-997, 999-1008
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/Windows/ActionTraceEditorWindow.cs` around lines 405 -
423, The early-return in RefreshEvents prevents UI changes (importance toggle,
context toggle, sort/clear) from updating the list; modify RefreshEvents to
accept a boolean forceRefresh (default false) or include the relevant UI state
in its guard, then call RefreshEvents(true) from the callbacks wired in
SetupToolbar (the _searchField, _importanceToggle, _contextToggle
RegisterValueChangedCallback handlers) and from places that change sort/filters;
alternatively remove the restrictive guard and instead compare current vs cached
filter/sort/_showSemantics/_showContext/_searchText to decide refresh so toggles
like _showSemantics/_showContext and _eventListView.RefreshItems always reflect
changes.
| private string BuildJsonExport() | ||
| { | ||
| _stringBuilder.Clear(); | ||
| _stringBuilder.AppendLine("{"); | ||
| _stringBuilder.AppendLine($" \"exportTime\": \"{DateTime.Now:O}\","); | ||
| _stringBuilder.AppendLine($" \"totalEvents\": {_currentEvents.Count},"); | ||
| _stringBuilder.AppendLine(" \"events\": ["); | ||
|
|
||
| for (int i = 0; i < _currentEvents.Count; i++) | ||
| { | ||
| var e = _currentEvents[i]; | ||
| _stringBuilder.AppendLine(" {"); | ||
| _stringBuilder.AppendLine($" \"sequence\": {e.Event.Sequence},"); | ||
| _stringBuilder.AppendLine($" \"type\": \"{SanitizeJson(e.Event.Type)}\","); | ||
| _stringBuilder.AppendLine($" \"timestamp\": {e.Event.TimestampUnixMs},"); | ||
| _stringBuilder.AppendLine($" \"displayTime\": \"{SanitizeJson(e.DisplayTime)}\","); | ||
| _stringBuilder.AppendLine($" \"summary\": \"{SanitizeJson(e.DisplaySummary)}\","); | ||
|
|
||
| if (!string.IsNullOrEmpty(e.Event.TargetId)) | ||
| _stringBuilder.AppendLine($" \"targetId\": \"{SanitizeJson(e.Event.TargetId)}\","); | ||
| else | ||
| _stringBuilder.AppendLine($" \"targetId\": null,"); | ||
|
|
||
| _stringBuilder.AppendLine($" \"importanceScore\": {e.ImportanceScore:F2},"); | ||
| _stringBuilder.AppendLine($" \"importanceCategory\": \"{SanitizeJson(e.ImportanceCategory)}\""); | ||
|
|
||
| if (!string.IsNullOrEmpty(e.InferredIntent)) | ||
| _stringBuilder.AppendLine($" ,\"inferredIntent\": \"{SanitizeJson(e.InferredIntent)}\""); | ||
|
|
||
| if (e.Event.Payload != null && e.Event.Payload.Count > 0) | ||
| { | ||
| _stringBuilder.AppendLine(" ,\"payload\": {"); | ||
| var payloadKeys = e.Event.Payload.Keys.ToList(); | ||
| for (int j = 0; j < payloadKeys.Count; j++) | ||
| { | ||
| var key = payloadKeys[j]; | ||
| var value = e.Event.Payload[key]; | ||
| var valueStr = value?.ToString() ?? "null"; | ||
| _stringBuilder.AppendLine($" \"{SanitizeJson(key)}\": \"{SanitizeJson(valueStr)}\"{(j < payloadKeys.Count - 1 ? "," : "")}"); | ||
| } | ||
| _stringBuilder.AppendLine(" }"); | ||
| } | ||
|
|
||
| _stringBuilder.Append(i < _currentEvents.Count - 1 ? " }," : " }"); | ||
| if (i < _currentEvents.Count - 1) | ||
| _stringBuilder.AppendLine(); | ||
| } | ||
|
|
||
| _stringBuilder.AppendLine(); | ||
| _stringBuilder.AppendLine(" ]"); | ||
| _stringBuilder.AppendLine("}"); | ||
|
|
||
| return _stringBuilder.ToString(); | ||
| } | ||
|
|
||
| private string BuildCsvExport() | ||
| { | ||
| _stringBuilder.Clear(); | ||
| _stringBuilder.AppendLine("Sequence,Time,Type,Summary,Target,Importance,Category,Intent"); | ||
|
|
||
| foreach (var e in _currentEvents) | ||
| { | ||
| _stringBuilder.Append(e.Event.Sequence); | ||
| _stringBuilder.Append(",\""); | ||
| _stringBuilder.Append(SanitizeCsv(e.DisplayTime)); | ||
| _stringBuilder.Append("\",\""); | ||
| _stringBuilder.Append(SanitizeCsv(e.Event.Type)); | ||
| _stringBuilder.Append("\",\""); | ||
| _stringBuilder.Append(SanitizeCsv(e.DisplaySummary)); | ||
| _stringBuilder.Append("\",\""); | ||
| _stringBuilder.Append(SanitizeCsv(e.Event.TargetId ?? "")); | ||
| _stringBuilder.Append("\","); | ||
| _stringBuilder.Append(e.ImportanceScore.ToString("F2")); | ||
| _stringBuilder.Append(",\""); | ||
| _stringBuilder.Append(SanitizeCsv(e.ImportanceCategory)); | ||
| _stringBuilder.Append("\",\""); | ||
| _stringBuilder.Append(SanitizeCsv(e.InferredIntent ?? "")); | ||
| _stringBuilder.AppendLine("\""); | ||
| } | ||
|
|
||
| return _stringBuilder.ToString(); | ||
| } | ||
|
|
||
| private string SanitizeJson(string input) | ||
| { | ||
| if (string.IsNullOrEmpty(input)) return ""; | ||
|
|
||
| // 性能优化：使用 StringBuilder 避免多次 Replace 产生的中间字符串 | ||
| var sb = _stringBuilder; | ||
| sb.Clear(); | ||
| sb.EnsureCapacity(input.Length * 2); | ||
|
|
||
| foreach (char c in input) | ||
| { | ||
| switch (c) | ||
| { | ||
| case '\\': sb.Append("\\\\"); break; | ||
| case '\"': sb.Append("\\\""); break; | ||
| case '\n': sb.Append("\\n"); break; | ||
| case '\r': sb.Append("\\r"); break; | ||
| case '\t': sb.Append("\\t"); break; | ||
| default: sb.Append(c); break; | ||
| } | ||
| } | ||
|
|
||
| return sb.ToString(); | ||
| } |
There was a problem hiding this comment.
JSON export is corrupted by shared _stringBuilder.
SanitizeJson clears _stringBuilder at Line 739, which is the same buffer BuildJsonExport is using. The first sanitize call wipes previously appended JSON, yielding invalid output.
🐛 Proposed fix
- private string SanitizeJson(string input)
+ private string SanitizeJson(string input)
{
if (string.IsNullOrEmpty(input)) return "";
- // 性能优化：使用 StringBuilder 避免多次 Replace 产生的中间字符串
- var sb = _stringBuilder;
- sb.Clear();
- sb.EnsureCapacity(input.Length * 2);
+ // 使用局部 StringBuilder，避免破坏主导出的缓冲区
+ var sb = new StringBuilder(input.Length * 2);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private string BuildJsonExport() | |
| { | |
| _stringBuilder.Clear(); | |
| _stringBuilder.AppendLine("{"); | |
| _stringBuilder.AppendLine($" \"exportTime\": \"{DateTime.Now:O}\","); | |
| _stringBuilder.AppendLine($" \"totalEvents\": {_currentEvents.Count},"); | |
| _stringBuilder.AppendLine(" \"events\": ["); | |
| for (int i = 0; i < _currentEvents.Count; i++) | |
| { | |
| var e = _currentEvents[i]; | |
| _stringBuilder.AppendLine(" {"); | |
| _stringBuilder.AppendLine($" \"sequence\": {e.Event.Sequence},"); | |
| _stringBuilder.AppendLine($" \"type\": \"{SanitizeJson(e.Event.Type)}\","); | |
| _stringBuilder.AppendLine($" \"timestamp\": {e.Event.TimestampUnixMs},"); | |
| _stringBuilder.AppendLine($" \"displayTime\": \"{SanitizeJson(e.DisplayTime)}\","); | |
| _stringBuilder.AppendLine($" \"summary\": \"{SanitizeJson(e.DisplaySummary)}\","); | |
| if (!string.IsNullOrEmpty(e.Event.TargetId)) | |
| _stringBuilder.AppendLine($" \"targetId\": \"{SanitizeJson(e.Event.TargetId)}\","); | |
| else | |
| _stringBuilder.AppendLine($" \"targetId\": null,"); | |
| _stringBuilder.AppendLine($" \"importanceScore\": {e.ImportanceScore:F2},"); | |
| _stringBuilder.AppendLine($" \"importanceCategory\": \"{SanitizeJson(e.ImportanceCategory)}\""); | |
| if (!string.IsNullOrEmpty(e.InferredIntent)) | |
| _stringBuilder.AppendLine($" ,\"inferredIntent\": \"{SanitizeJson(e.InferredIntent)}\""); | |
| if (e.Event.Payload != null && e.Event.Payload.Count > 0) | |
| { | |
| _stringBuilder.AppendLine(" ,\"payload\": {"); | |
| var payloadKeys = e.Event.Payload.Keys.ToList(); | |
| for (int j = 0; j < payloadKeys.Count; j++) | |
| { | |
| var key = payloadKeys[j]; | |
| var value = e.Event.Payload[key]; | |
| var valueStr = value?.ToString() ?? "null"; | |
| _stringBuilder.AppendLine($" \"{SanitizeJson(key)}\": \"{SanitizeJson(valueStr)}\"{(j < payloadKeys.Count - 1 ? "," : "")}"); | |
| } | |
| _stringBuilder.AppendLine(" }"); | |
| } | |
| _stringBuilder.Append(i < _currentEvents.Count - 1 ? " }," : " }"); | |
| if (i < _currentEvents.Count - 1) | |
| _stringBuilder.AppendLine(); | |
| } | |
| _stringBuilder.AppendLine(); | |
| _stringBuilder.AppendLine(" ]"); | |
| _stringBuilder.AppendLine("}"); | |
| return _stringBuilder.ToString(); | |
| } | |
| private string BuildCsvExport() | |
| { | |
| _stringBuilder.Clear(); | |
| _stringBuilder.AppendLine("Sequence,Time,Type,Summary,Target,Importance,Category,Intent"); | |
| foreach (var e in _currentEvents) | |
| { | |
| _stringBuilder.Append(e.Event.Sequence); | |
| _stringBuilder.Append(",\""); | |
| _stringBuilder.Append(SanitizeCsv(e.DisplayTime)); | |
| _stringBuilder.Append("\",\""); | |
| _stringBuilder.Append(SanitizeCsv(e.Event.Type)); | |
| _stringBuilder.Append("\",\""); | |
| _stringBuilder.Append(SanitizeCsv(e.DisplaySummary)); | |
| _stringBuilder.Append("\",\""); | |
| _stringBuilder.Append(SanitizeCsv(e.Event.TargetId ?? "")); | |
| _stringBuilder.Append("\","); | |
| _stringBuilder.Append(e.ImportanceScore.ToString("F2")); | |
| _stringBuilder.Append(",\""); | |
| _stringBuilder.Append(SanitizeCsv(e.ImportanceCategory)); | |
| _stringBuilder.Append("\",\""); | |
| _stringBuilder.Append(SanitizeCsv(e.InferredIntent ?? "")); | |
| _stringBuilder.AppendLine("\""); | |
| } | |
| return _stringBuilder.ToString(); | |
| } | |
| private string SanitizeJson(string input) | |
| { | |
| if (string.IsNullOrEmpty(input)) return ""; | |
| // 性能优化：使用 StringBuilder 避免多次 Replace 产生的中间字符串 | |
| var sb = _stringBuilder; | |
| sb.Clear(); | |
| sb.EnsureCapacity(input.Length * 2); | |
| foreach (char c in input) | |
| { | |
| switch (c) | |
| { | |
| case '\\': sb.Append("\\\\"); break; | |
| case '\"': sb.Append("\\\""); break; | |
| case '\n': sb.Append("\\n"); break; | |
| case '\r': sb.Append("\\r"); break; | |
| case '\t': sb.Append("\\t"); break; | |
| default: sb.Append(c); break; | |
| } | |
| } | |
| return sb.ToString(); | |
| } | |
| private string SanitizeJson(string input) | |
| { | |
| if (string.IsNullOrEmpty(input)) return ""; | |
| // 使用局部 StringBuilder，避免破坏主导出的缓冲区 | |
| var sb = new StringBuilder(input.Length * 2); | |
| foreach (char c in input) | |
| { | |
| switch (c) | |
| { | |
| case '\\': sb.Append("\\\\"); break; | |
| case '\"': sb.Append("\\\""); break; | |
| case '\n': sb.Append("\\n"); break; | |
| case '\r': sb.Append("\\r"); break; | |
| case '\t': sb.Append("\\t"); break; | |
| default: sb.Append(c); break; | |
| } | |
| } | |
| return sb.ToString(); | |
| } |
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/Windows/ActionTraceEditorWindow.cs` around lines 651 -
757, JSON export is corrupted because SanitizeJson reuses and Clears the shared
field _stringBuilder used by BuildJsonExport; change SanitizeJson to allocate
and use a local StringBuilder (e.g., new StringBuilder with capacity based on
input.Length) instead of _stringBuilder so it doesn't clear the shared buffer,
keep the same escape switch logic and same empty-string return for null/empty
input, and leave BuildJsonExport / BuildCsvExport unchanged.
| private bool FilterEvent(ActionTraceQuery.ActionTraceViewItem e) | ||
| { | ||
| if (_sortMode == SortMode.AIFiltered && e.ImportanceScore < _minImportance) | ||
| return false; | ||
|
|
||
| if (!string.IsNullOrEmpty(_searchText)) | ||
| { | ||
| return e.DisplaySummaryLower.Contains(_searchText) | ||
| || e.DisplayTargetIdLower.Contains(_searchText) | ||
| || e.Event.Type.ToLowerInvariant().Contains(_searchText); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and read the file
find . -name "ActionTraceEditorWindow.cs" -type fRepository: CoplayDev/unity-mcp
Length of output: 118
🏁 Script executed:
# Read the file to understand the FilterEvent method and its context
wc -l ./MCPForUnity/Editor/Windows/ActionTraceEditorWindow.csRepository: CoplayDev/unity-mcp
Length of output: 123
🏁 Script executed:
# Read the FilterEvent method and surrounding context
sed -n '960,1000p' ./MCPForUnity/Editor/Windows/ActionTraceEditorWindow.csRepository: CoplayDev/unity-mcp
Length of output: 1430
🏁 Script executed:
# Let's check how the importance filter UI is controlled and where it's displayed
rg -n "SetImportance|_minImportance|ImportanceScore" ./MCPForUnity/Editor/Windows/ActionTraceEditorWindow.cs | head -30Repository: CoplayDev/unity-mcp
Length of output: 2092
🏁 Script executed:
# Check where the filter menu is built and if it's conditional on sort mode
sed -n '420,440p' ./MCPForUnity/Editor/Windows/ActionTraceEditorWindow.csRepository: CoplayDev/unity-mcp
Length of output: 1261
🏁 Script executed:
# Let's check the OnClearFiltersClicked to understand how filters are meant to work
sed -n '800,820p' ./MCPForUnity/Editor/Windows/ActionTraceEditorWindow.csRepository: CoplayDev/unity-mcp
Length of output: 714
🏁 Script executed:
# Let's search for where FilterEvent is called to understand the filtering pipeline
rg -n "FilterEvent" ./MCPForUnity/Editor/Windows/ActionTraceEditorWindow.csRepository: CoplayDev/unity-mcp
Length of output: 182
🏁 Script executed:
# Check how FilterEvent is used in the RefreshEvents method
sed -n '840,880p' ./MCPForUnity/Editor/Windows/ActionTraceEditorWindow.csRepository: CoplayDev/unity-mcp
Length of output: 1435
🏁 Script executed:
# Let's verify the ApplySorting method to fully understand the sort modes
sed -n '955,970p' ./MCPForUnity/Editor/Windows/ActionTraceEditorWindow.csRepository: CoplayDev/unity-mcp
Length of output: 680
🏁 Script executed:
# Let's check if there are any comments near the FilterEvent method that explain the design
sed -n '965,985p' ./MCPForUnity/Editor/Windows/ActionTraceEditorWindow.csRepository: CoplayDev/unity-mcp
Length of output: 780
🏁 Script executed:
# Let's search for tests or documentation about the filtering behavior
find . -name "*.cs" -type f | xargs grep -l "ActionTraceEditorWindow\|FilterEvent" | head -5Repository: CoplayDev/unity-mcp
Length of output: 8962
🏁 Script executed:
# Let's check if there's any documentation or README that explains the intended filtering behavior
find . -name "README*" -o -name "*.md" | xargs grep -l "ActionTrace\|filter\|importance" 2>/dev/null | head -5Repository: CoplayDev/unity-mcp
Length of output: 45
Importance filter only applies in "AI Filtered" sort mode.
The condition on line 973 restricts the importance filter to SortMode.AIFiltered, but the filter UI is available in both sort modes. Users can set the importance threshold while sorting by time, and the UI displays it as an active filter, yet it won't actually filter results. The search filter works in all modes; the importance filter should too.
💡 Suggested fix (apply filter regardless of sort)
- if (_sortMode == SortMode.AIFiltered && e.ImportanceScore < _minImportance)
+ if (e.ImportanceScore < _minImportance)
return false;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private bool FilterEvent(ActionTraceQuery.ActionTraceViewItem e) | |
| { | |
| if (_sortMode == SortMode.AIFiltered && e.ImportanceScore < _minImportance) | |
| return false; | |
| if (!string.IsNullOrEmpty(_searchText)) | |
| { | |
| return e.DisplaySummaryLower.Contains(_searchText) | |
| || e.DisplayTargetIdLower.Contains(_searchText) | |
| || e.Event.Type.ToLowerInvariant().Contains(_searchText); | |
| } | |
| private bool FilterEvent(ActionTraceQuery.ActionTraceViewItem e) | |
| { | |
| if (e.ImportanceScore < _minImportance) | |
| return false; | |
| if (!string.IsNullOrEmpty(_searchText)) | |
| { | |
| return e.DisplaySummaryLower.Contains(_searchText) | |
| || e.DisplayTargetIdLower.Contains(_searchText) | |
| || e.Event.Type.ToLowerInvariant().Contains(_searchText); | |
| } |
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/Windows/ActionTraceEditorWindow.cs` around lines 971 -
981, The importance cutoff is currently only enforced when _sortMode ==
SortMode.AIFiltered; update FilterEvent (method
FilterEvent(ActionTraceQuery.ActionTraceViewItem e)) to apply the _minImportance
check regardless of _sortMode so the UI's importance slider actually filters in
all modes—i.e., remove the SortMode.AIFiltered guard and return false when
e.ImportanceScore < _minImportance, then keep the existing search-text checks
using _searchText, e.DisplaySummaryLower, e.DisplayTargetIdLower and
e.Event.Type.ToLowerInvariant() as before.
1 participant
#553
ActionTrace System - Feature Overview
Overview
ActionTrace is a comprehensive Unity Editor event tracking and replay system. It captures, stores, and queries editor operations for debugging, analysis, and undo/replay capabilities.
Core Functionality
1. Event Capture
2. Event Storage
3. Query & Analysis
4. Semantic Analysis
Component | Purpose -- | -- Categorizer | Classifies events (scene edit, asset import, etc.) Scorer | Assigns importance (critical, high, medium, low) Intent Inferrer | Derives user intent from action patternsDesign Principles
Use Cases
Here is the English translation of your architecture overview, maintaining the original formatting and structure:
Data Storage Location
Storage Architecture
┌─────────────────────────────────────────────────────────┐
│ In-Memory Storage │
│ ┌─────────────────────────────────────────────────┐ │
│ │ List<EditorEvent> _events │ │
│ │ - Latest 100: Full payload (Hot Data) │ │
│ │ - Older events: Dehydrated payload=null (Cold) │ │
│ └─────────────────────────────────────────────────┘ │
│ ┌─────────────────────────────────────────────────┐ │
│ │ List<ContextMapping> _contextMappings │ │
│ └─────────────────────────────────────────────────┘ │
└─────────────────────────────────────────────────────────┘
│ Delayed Save (delayCall)
▼
┌─────────────────────────────────────────────────────────┐
│ Library/McpState_timeline_events.json │
│ (JSON Serialization, Persistent across Domain Reload)│
└─────────────────────────────────────────────────────────┘
Specific Locations
Type | Path | Description -- | -- | -- Runtime Memory | List | Thread-safe collection Persistence | Library/McpState_timeline_events.json | JSON format, auto-savedSummary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.