Skip to content

Conversation

@whatevertogo
Copy link
Contributor

@whatevertogo whatevertogo commented Jan 21, 2026

ActionTrace System - Unity Editor Event Tracking


Type of Change

  • New feature (non-breaking change that adds functionality)
  • Bug fix (non-breaking change that fixes an issue)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • Refactoring (no functional changes)
  • Test update

Changes Made

Overview

ActionTrace is a comprehensive Unity Editor event tracking and recording system that provides complete event context for AI assistants. This PR introduces the complete ActionTrace architecture with 8000 lines of code across 60+ files.

Core Architecture

Foundation Layer (Core/):

  • EditorEvent.cs - Event data model with serialization support
  • EventStore.cs - Central event storage with hot/cold event management
  • EventStore.Persistence.cs - Domain reload survival and disk persistence
  • EventStore.Diagnostics.cs - Memory management and event dehydration
  • EventStore.Merging.cs - Event deduplication and transaction aggregation
  • EventStore.Context.cs - Context mapping management
  • ActionTraceSettings.cs - Layered settings architecture (Filtering, Merging, Storage)
  • ActionTracePreset.cs - Preset system (Standard, Verbose, Minimal, Silent)
  • EventTypes.cs, EventCategory.cs, EventMetadata.cs - Type definitions

Capture Layer (Capture/):

  • Recorder.cs - Central recording orchestrator
  • Emitter.cs - Event emission coordination
  • Capturers/AssetCapture.cs - Asset import/modify/delete tracking
  • Capturers/PropertyCapture.cs - Property modification tracking
  • Capturers/SelectionCapture.cs - Selection change tracking
  • Filters/EventFilter.cs - Rule-based event filtering (Prefix, Extension, Regex, GameObject)
  • Sampling/SamplingMiddleware.cs - High-frequency event throttling
  • Sampling/SamplingStrategy.cs - Pluggable sampling algorithms
  • Sampling/SamplingConfig.cs - Sampling configuration

Analysis Layer (Analysis/):

  • Query/ActionTraceQuery.cs - Fluent query API with filtering and aggregation
  • Summarization/EventSummarizer.cs - AI-friendly event summaries
  • Summarization/TransactionAggregator.cs - Transaction grouping

Context & Semantics:

  • Context/ContextMapping.cs - Operation context tracking
  • Semantics/DefaultEventScorer.cs - Event importance scoring (0-10)
  • Semantics/DefaultIntentInferrer.cs - Intent inference from events
  • Semantics/DefaultCategorizer.cs - Event categorization

Integration:

  • Integration/AssetBridge.cs - Asset operation tracking
  • Integration/VCS/VcsContextProvider.cs - Version control context

Helpers:

  • Helpers/GlobalIdHelper.cs - Cross-session stable object identification
  • Helpers/GameObjectTrackingHelper.cs - GameObject lifecycle tracking
  • Helpers/PropertyEventPayloadBuilder.cs - Consistent payload structure
  • Helpers/PropertyFormatter.cs - Property value formatting
  • Helpers/PropertyModificationHelper.cs - Modification detection
  • Helpers/BuildTargetUtility.cs - Build target detection

Independent Hook System (Hooks/):

  • Hooks/HookRegistry.cs - Dependency-injected hook registration system (decoupled from ActionTrace)
  • Hooks/UnityEventHooks/ - Comprehensive Unity event hooks
  • Hooks/EventArgs/ - Type-safe event argument definitions

UI (Windows/):

  • ActionTraceEditorWindow.cs - Editor window with timeline, filtering, and export
  • ActionTraceEditorWindow.uxml - UI layout
  • ActionTraceEditorWindow.uss - UI styling

MCP Tools:

  • GetActionTraceTool.cs - Query events with filtering
  • GetActionTraceSummaryTool.cs - AI-friendly aggregated summaries
  • AddActionTraceNoteTool.cs - Add timeline notes
  • ActionTraceSettingsTool.cs - Settings management

Python Server:

  • Server/src/models/action_trace.py - Event data models
  • Server/src/services/resources/action_trace.py - Event stream resource
  • Server/src/services/tools/get_action_trace.py - Query tool wrapper
  • Server/src/services/tools/get_action_trace_summary.py - Summary tool wrapper
  • Server/src/services/tools/add_action_trace_note.py - Note tool wrapper

Key Features

  1. Hot/Cold Event Storage

    • Latest 150 events retain full payload(we can set the number in settings)
    • Older events automatically dehydrated (payload cleared, metadata preserved)
    • Configurable capacity limits
  2. Cross-Session Stability

    • GlobalIdHelper provides stable IDs across domain reloads
    • Asset events use Asset:{path} format
    • GameObject/Component tracking with instance IDs
  3. Event Filtering & Sampling

    • Rule-based filtering (prefix, extension, regex, GameObject)
    • Configurable sampling strategies for high-frequency events
    • Importance-based recording (0-10 threshold)
  4. Query & Analysis

    • Fluent query builder with time range, type, importance filters
    • Event statistics and aggregation
    • Transaction grouping for atomic operations
  5. Context Awareness

    • Distinguishes Human/AI/System operation sources (we have the feat but not used yet)
    • Tool call scope tracking for MCP operations
    • VCS integration for branch/commit context
  6. Preset System

    • Standard (default balanced)
    • Verbose (maximum detail)
    • Minimal (essential events only)
    • Silent (recording disabled)

Performance Optimizations

  • Replaced LINQ with manual loops for GC reduction
  • Fixed memory leaks (delayCall → EditorApplication.update)
  • Thread-safe collections and locking
  • Early-exit patterns for idle state

Breaking Changes

None - All changes are backward compatible.


Testing/Screenshots/Recordings

Manual Testing

  • Event recording and persistence
  • Domain reload survival
  • Event filtering and querying
  • Preset switching
  • UI rendering and interaction
  • MCP tool integration
  • Memory diagnostics
  • Hook system independence

Test Scripts

  • Hooks/HookTest.cs - Hook validation script

Documentation Updates

  • I have added/removed/modified tools or resources

Documentation Updates Using

  • The LLM prompt at tools/UPDATE_DOCS_PROMPT.md (recommended)
  • Manual updates following the guide at tools/UPDATE_DOCS.md

Updated Files

  • README.md - Added ActionTrace tools to available tools list
  • manifest.json - Registered new ActionTrace tools
  • docs/i18n/README-zh.md - Chinese documentation updates
  • CLAUDE.md - Added ActionTrace architecture section

Additional Notes

Architecture Evolution

This PR represents the culmination of 16 major commits:

Commit Description
b0eb274 Initial implementation (9485+ lines)
ec59a59 EventStore partial class refactoring
f475bee Layered settings, preset system, query enhancements
af77674 Performance optimization (LINQ → manual loops)
f547208 Settings enhancement with Range attributes
44b7045 GlobalIdHelper for cross-session stability
a86506c Memory leak fix
25e1253 Built-in HookRegistry system
4609817 Architecture & UI improvements
8920715 DI decoupling of Hooks system
4e4feae Sampling middleware enhancement
eb14322 Comments & translations update
87280a8 Filter summary bar update
0d4e6a4 Obsolete components removal
7831025 Summary tool & GlobalID enhancement
1044319 Capture classes refactoring

Future Considerations

  1. Add unit tests for EventStore and HookRegistry
  2. Establish performance benchmark suite
  3. Consider API freeze for next major version
  4. Expand Hook system documentation

Summary by Sourcery

Implement comprehensive Unity Editor event tracking system for AI-assisted development context.

Features:

Event Capture & Storage:

  • Record all Unity Editor operations with full metadata
  • Hot/cold storage with automatic dehydration (latest 150 events full payload)
  • Domain reload survival with disk persistence
  • Cross-session stable object identification via GlobalIdHelper

Query & Analysis:

  • Fluent query API with time range, type, importance filters
  • AI-friendly event summaries with transaction aggregation
  • Event statistics and context compression
  • Multiple query modes (raw, aggregated, summarized)

Architecture:

  • Layered settings with preset system (Standard/Verbose/Minimal/Silent)
  • Rule-based event filtering (prefix, extension, regex, GameObject)
  • Configurable sampling strategies for high-frequency events
  • Independent Hook system with dependency injection

Integration:

  • MCP tools for event querying and summarization
  • Editor window with timeline visualization
  • Tool call scope tracking for MCP operations

Performance:

  • GC allocation optimization (LINQ → manual loops)
  • Memory leak fixes (delayCall → EditorApplication.update)
  • Thread-safe collections with early-exit patterns
  • Configurable capacity limits with memory diagnostics

Technical Details:

  • ~15,000+ lines across 60+ files
  • Zero breaking changes
  • Full backward compatibility
  • Comprehensive documentation updates

Summary by CodeRabbit

  • New Features

    • Full ActionTrace system: capture, persist, dedupe, merge, and query editor events.
    • ActionTrace Editor Window for browsing, filtering, and inspecting events with semantic fields.
    • Semantic analysis: importance scoring, categorical labels, inferred intents, and AI note support.
    • Tools & APIs: query presets, summaries, add-note tool, and live settings/diagnostics.
  • Documentation

    • Added CONTRIBUTING and DESIGN docs for ActionTrace and Hooks.

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

whatevertogo and others added 30 commits January 11, 2026 19:30
Fixes CoplayDev#538

The System Requirements panel showed "UV Package Manager: Not Found" even
when a valid UV path override was configured in Advanced Settings.

Root cause: PlatformDetectorBase.DetectUv() only searched PATH with bare
command names ("uvx", "uv") and never consulted PathResolverService which
respects the user's override setting.

Changes:
- Refactor DetectUv() to use PathResolverService.GetUvxPath() which checks
  override path first, then system PATH, then falls back to "uvx"
- Add TryValidateUvExecutable() to verify executables by running --version
  instead of just checking File.Exists
- Prioritize PATH environment variable in EnumerateUvxCandidates() for
  better compatibility with official uv install scripts
- Fix process output read order (ReadToEnd before WaitForExit) to prevent
  potential deadlocks

Co-Authored-By: ChatGLM 4.7 <noreply@zhipuai.com>
- Read both stdout and stderr when validating uv/uvx executables
- Respect WaitForExit timeout return value instead of ignoring it
- Fix version parsing to handle extra tokens like "(Homebrew 2025-01-01)"
- Resolve bare commands ("uv"/"uvx") to absolute paths after validation
- Rename FindExecutableInPath to FindUvxExecutableInPath for clarity

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…s PATH augmentation

Replace direct Process.Start calls with ExecPath.TryRun across all platform detectors.
This change:
- Fixes potential deadlocks by using async output reading
- Adds proper timeout handling with process termination
- Removes redundant fallback logic and simplifies version parsing
- Adds Windows PATH augmentation with common uv, npm, and Python installation paths

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The version extraction logic now properly handles outputs like:
- "uvx 0.9.18" -> "0.9.18"
- "uvx 0.9.18 (hash date)" -> "0.9.18"
- "uvx 0.9.18 extra info" -> "0.9.18"

Uses Math.Min to find the first occurrence of either space or parenthesis.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add absolute path resolution in TryValidatePython and TryValidateUvWithPath for better UI display
- Fix BuildAugmentedPath to avoid PATH duplication
- Add comprehensive comments for version parsing logic
- Ensure cross-platform consistency across all three detectors
- Fix override path validation logic with clear state handling
- Fix platform detector path resolution and Python version detection
- Use UserProfile consistently in GetClaudeCliPath instead of Personal
- All platforms now use protected BuildAugmentedPath method

This change improves user experience by displaying full paths in the UI
while maintaining robust fallback behavior if path resolution fails.

Co-Authored-By: GLM4.7 <noreply@zhipuai.com>
- Rename TryValidateUvExecutable -> TryValidateUvxExecutable for consistency
- Add cross-platform FindInPath() helper in ExecPath.cs
- Remove platform-specific where/which implementations in favor of unified helper
- Add Windows-specific DetectUv() override with enhanced uv/uvx detection
- Add WinGet shim path support for Windows uvx installation
- Update UI labels: "UV Path" -> "UVX Path"
- Only show uvx path status when override is configured

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Implement a comprehensive ActionTrace system that captures, stores, and queries Unity editor events for debugging, analysis, and undo/replay capabilities.

**Core Features:**
- Event capture layer with hooks for Unity editor events
- Context tracking with stack and timeline support
- Event store with in-memory persistence and query capabilities
- Semantic analysis (categorization, scoring, intent inference)
- VCS integration for version control context
- Editor window with UI for visualizing events
- MCP tools for remote query and control

**Components Added:**
- Capture: ActionTraceEventEmitter, EventFilter, PropertyChangeTracker, UnityEventHooks
- Context: ContextStack, ContextTimeline, OperationContext, ContextMapping
- Core: EventStore, EditorEvent, EventTypes, ActionTraceSettings, GlobalIdHelper
- Query: ActionTraceQuery, EventSummarizer, TransactionAggregator
- Semantics: DefaultCategorizer, DefaultEventScorer, DefaultIntentInferrer
- UI: ActionTraceEditorWindow with UXML/USS styling
- MCP Tools: get_action_trace, get_action_trace_settings, add_action_trace_note, undo_to_sequence

**Server-side:**
- Python models and resources for ActionTrace
- MCP tools for querying events, managing settings, and undoing to sequence

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ility

- Extract EventStore.Context.cs for context mapping management
- Extract EventStore.Diagnostics.cs for memory diagnostics and dehydration
- Extract EventStore.Merging.cs for event deduplication logic
- Extract EventStore.Persistence.cs for save/load and domain reload survival
- Add PropertyEventPayloadBuilder helper for consistent payload structure
- Add PropertyFormatter helper to eliminate code duplication
- Adjust ActionTraceSettings defaults (MaxEvents: 800, HotEventCount: 150)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…uery improvements

This commit introduces significant improvements to the ActionTrace system:

**Layered Settings Architecture:**
- Refactor ActionTraceSettings into layered structure (Filtering, Merging, Storage, Sampling)
- Add custom Editor with foldout sections for better UX
- Move GlobalIdHelper from Core/ to Helpers/ for better organization

**Preset System:**
- Add ActionTracePreset with Standard/Verbose/Minimal/Silent presets
- Enable quick configuration switching via ApplyPreset()
- Include preset descriptions and memory estimates

**Configurable Filtering:**
- Transform EventFilter from static blacklist to rule-based system
- Support Prefix, Extension, Regex, and GameObject rule types
- Add EventFilterSettings for persistence and customization

**Stable Cross-Session IDs:**
- Use GlobalIdHelper for all GameObject/Component event TargetIds
- Use Asset:{path} format for asset events
- Ensure TargetIds remain stable across domain reloads

**Query & Analysis Enhancements:**
- Add EventQueryBuilder for fluent query API
- Add ContextCompressor for event data optimization
- Add EventStatistics for comprehensive analytics
- Enhance EventSummarizer with better grouping

**Capture Layer Improvements:**
- Add AssetChangePostprocessor for asset change tracking
- Add SamplingMiddleware for high-frequency event throttling
- Add ToolCallScope for MCP tool call tracking
- Enhance UnityEventHooks with comprehensive event coverage

**UI/UX Improvements:**
- Add sort modes (ByTimeDesc, AIFiltered) to ActionTraceEditorWindow
- Improve filter menu with "All" option
- Add tool descriptions for better AI discoverability

**Helper Classes:**
- Add GameObjectTrackingHelper for GameObject lifecycle tracking
- Add UndoReflectionHelper for Undo system introspection
- Add BuildTargetUtility for build target detection

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…capacity

- Add Range attributes to settings fields with Inspector sliders:
  - MinImportanceForRecording: [Range(0f, 1f)]
  - MergeWindowMs: [Range(0, 5000)]
  - TransactionWindowMs: [Range(100, 10000)]
  - HotEventCount: [Range(10, 1000)]
  - MaxEvents: [Range(100, 5000)] (already existed)
- Change ContextMappings from fixed 2000 to dynamic MaxEvents × 2
- Remove redundant validation code (now enforced by Range attributes)
- Fix UndoReflectionHelper.GetPreviousValue to try nested value first
- Clean up StdioBridgeHost: remove ineffective CleanupZombieListeners
- Add task/conversation filtering to ActionTraceViewResource
- Preserve modified-object identity in SelectionPropertyTracker

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…leaks

Replace infinite recursive delayCall patterns with EditorApplication.update
in PropertyChangeTracker and SamplingMiddleware. This ensures proper
cleanup on domain reload and prevents memory leaks.

Changes:
- PropertyChangeTracker: Replace delayCall with update + FlushCheck()
- SamplingMiddleware: Replace delayCall with update directly
- Remove redundant helper methods (use UndoReflectionHelper/PropertyFormatter)
- Clean up unused using statements

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@msanatan
Copy link
Member

@whatevertogo Sorry for the late reply on my end - aside from personal things to attend to I needed to think about this one a bit more.

I think the video will help a lot. It's not immediately clear to me how this resource actually helps AI tools when building something. It looks awesome, but what's the real world use case? For e.g. recently @dsarno added a tool specifically for interacting with scriptable objects. The capability existed, but it was inefficient and prone to error. So by adding the tool, the AI tool got over a hurdle with Unity MCP that it couldn't do before.

In this case, what was it that your AI tool COULD NOT DO, and how does this actually make it accomplish more.

As a side note, you've been contributing a lot and I really really appreciate it. I think all of the core maintainers do. Even with disagreements (which we have amongst ourselves too), we hope that you continue being a part of our community and contributing. You're awesome ⭐

Copilot AI review requested due to automatic review settings January 22, 2026 07:42
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a comprehensive event tracking and monitoring infrastructure for AI-assisted Unity development, consisting of two foundational systems:

Changes:

  • Implements HookSystem: A general-purpose event dispatch infrastructure that abstracts Unity's native callbacks into a clean, thread-safe subscription interface with exception isolation
  • Implements ActionTrace: A sophisticated event tracking system featuring event recording, semantic analysis, dehydration for memory efficiency, and AI-friendly query interfaces
  • Adds MCP tools for AI interaction: get_action_trace, get_action_trace_summary, add_action_trace_note, undo_to_sequence, and get_action_trace_settings

Reviewed changes

Copilot reviewed 76 out of 76 changed files in this pull request and generated 57 comments.

Show a summary per file
File Description
docs/Hooks/DESIGN.md Comprehensive design documentation for Hook system architecture
docs/Hooks/CONTRIBUTING.md Contributing guide for extending the Hook system
docs/ActionTrace/DESIGN.md Design philosophy and architectural decisions for ActionTrace
docs/ActionTrace/CONTRIBUTING.md Contributing guide for ActionTrace extensions
Server/src/services/tools/*.py Python MCP tool implementations for ActionTrace queries
Server/src/services/resources/action_trace.py Python resource endpoints for ActionTrace
Server/src/models/action_trace.py Pydantic models for ActionTrace data validation
MCPForUnity/Editor/Windows/ActionTraceEditorWindow.* Unity Editor window for visualizing action traces
MCPForUnity/Editor/Tools/*Tool.cs C# MCP tool handlers for ActionTrace operations
MCPForUnity/Editor/Resources/ActionTrace/*.cs C# resource handlers and query infrastructure
MCPForUnity/Editor/Hooks/*.cs Core Hook system implementation with event registry
MCPForUnity/Editor/Helpers/*.cs Helper utilities for GlobalID resolution and build targets
MCPForUnity/Editor/ActionTrace/**/*.cs ActionTrace subsystems (Semantics, Integration, Sources)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

from typing import Annotated, Any, Literal, Optional

from fastmcp import Context
from mcp.types import ToolAnnotations
Copy link

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Import of 'ToolAnnotations' is not used.

Copilot uses AI. Check for mistakes.
Comment on lines +20 to +25
from models.action_trace import (
EditorEvent,
EventQueryResult,
EventStatistics,
ActionTraceSettings,
)
Copy link

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Import of 'ActionTraceSettings' is not used.

Copilot uses AI. Check for mistakes.

from services.registry import mcp_for_unity_tool
from services.tools import get_unity_instance_from_context
from services.tools.utils import coerce_bool
Copy link

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Import of 'coerce_bool' is not used.

Copilot uses AI. Check for mistakes.

Unity implementation: MCPForUnity/Editor/Tools/ActionTraceSettingsTool.cs
"""
from typing import Annotated, Any
Copy link

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Import of 'Annotated' is not used.

Copilot uses AI. Check for mistakes.
parsed = json.loads(value)
if isinstance(parsed, list):
return _coerce_int_list(parsed) # Recursively handle parsed list
except json.JSONDecodeError:
Copy link

Copilot AI Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'except' clause does nothing but pass and there is no explanatory comment.

Copilot uses AI. Check for mistakes.
@whatevertogo whatevertogo marked this pull request as draft January 22, 2026 07:51
… and improve property modification handling. Removed unused UndoCapture and UndoToSequenceTool files. Updated ActionTrace settings and enhanced Python tool descriptions for clarity.
@msanatan
Copy link
Member

Just re-iterataing @whatevertogo, please explain the exact use case where this feature improved what the model could do

@whatevertogo Sorry for the late reply on my end - aside from personal things to attend to I needed to think about this one a bit more.

I think the video will help a lot. It's not immediately clear to me how this resource actually helps AI tools when building something. It looks awesome, but what's the real world use case? For e.g. recently @dsarno added a tool specifically for interacting with scriptable objects. The capability existed, but it was inefficient and prone to error. So by adding the tool, the AI tool got over a hurdle with Unity MCP that it couldn't do before.

In this case, what was it that your AI tool COULD NOT DO, and how does this actually make it accomplish more.

As a side note, you've been contributing a lot and I really really appreciate it. I think all of the core maintainers do. Even with disagreements (which we have amongst ourselves too), we hope that you continue being a part of our community and contributing. You're awesome ⭐

@whatevertogo
Copy link
Contributor Author

whatevertogo commented Jan 22, 2026

Just re-iterataing @whatevertogo, please explain the exact use case where this feature improved what the model could do

@whatevertogo Sorry for the late reply on my end - aside from personal things to attend to I needed to think about this one a bit more.
I think the video will help a lot. It's not immediately clear to me how this resource actually helps AI tools when building something. It looks awesome, but what's the real world use case? For e.g. recently @dsarno added a tool specifically for interacting with scriptable objects. The capability existed, but it was inefficient and prone to error. So by adding the tool, the AI tool got over a hurdle with Unity MCP that it couldn't do before.
In this case, what was it that your AI tool COULD NOT DO, and how does this actually make it accomplish more.
As a side note, you've been contributing a lot and I really really appreciate it. I think all of the core maintainers do. Even with disagreements (which we have amongst ourselves too), we hope that you continue being a part of our community and contributing. You're awesome ⭐

https://www.youtube.com/watch?v=ykhhOa-g7PI
sorry i put it in discord.

By the way,I’m working on improving the manage_prefab implementation.

@msanatan msanatan changed the base branch from main to beta January 22, 2026 19:18
whatevertogo and others added 4 commits January 24, 2026 04:46
  Remove unused PropertyEventPayloadBuilder class (75 lines) that had
  no references in the codebase.

  Rename classes to match their filenames:
  - AssetChangePostprocessor → AssetCapture
  - PropertyChangeTracker → PropertyCapture
  - SelectionPropertyTracker → SelectionCapture

  This improves code discoverability and follows C# naming conventions
  where class names should match file names for clarity.

  Files removed:
  - Helpers/PropertyEventPayloadBuilder.cs
  - Helpers/PropertyEventPayloadBuilder.cs.meta
…ctionCapture

- Add OnPropertiesModified event to HookRegistry for Undo property changes
- Add Undo.postprocessModifications subscription in UnityEventHooks
- Refactor PropertyCapture to use HookRegistry.OnPropertiesModified
- Refactor SelectionCapture to use HookRegistry events
- Remove redundant -= operations in UnityEventHooks constructor
- Remove NotifyGameObjectDestroyed(null) call, use only Detailed version
- Refactor ActionTraceEditorWindow into modular folder structure
- Add EventListSection component for better separation of concerns
- Fix PlayModeChanged event importance score (0.3 -> 0.5)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@whatevertogo whatevertogo marked this pull request as ready for review January 24, 2026 01:16
@whatevertogo whatevertogo requested a review from Copilot January 24, 2026 01:16
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 73 out of 73 changed files in this pull request and generated 26 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +26 to +33
```csharp
public readonly struct EditorEvent
{
public readonly long Sequence;
public readonly long TimestampUnixMs;
public readonly string Type;
public readonly string Payload; // JSON
}
Copy link

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The design doc claims EditorEvent is a readonly struct, but the implementation in MCPForUnity/Editor/ActionTrace/Core/Models/EditorEvent.cs is a sealed class. This mismatch can confuse contributors (e.g., around mutability/GC). Update the snippet and surrounding rationale to reflect the actual type used by the codebase (or change the code to match the doc).

Copilot uses AI. Check for mistakes.
if (settings != null && !settings.Filtering.BypassImportanceFilter)
{
float importance = DefaultEventScorer.Instance.Score(@event);
if (importance <= settings.Filtering.MinImportanceForRecording)
Copy link

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Importance filtering rejects events when importance == MinImportanceForRecording (uses <=), but the tooltip says “below this value will not be recorded”. If the threshold is meant to be inclusive, change the comparison to importance < MinImportanceForRecording to avoid dropping borderline events.

Suggested change
if (importance <= settings.Filtering.MinImportanceForRecording)
if (importance < settings.Filtering.MinImportanceForRecording)

Copilot uses AI. Check for mistakes.
// Wait for process to actually exit after Kill
process.WaitForExit(1000);
}
catch { }
Copy link

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Poor error handling: empty catch block.

Copilot uses AI. Check for mistakes.
var token = @params["min_importance"] ?? @params["minImportance"];
if (token != null)
{
string value = token?.ToString()?.ToLower()?.Trim();
Copy link

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Condition is always not null because of ... != ....

Copilot uses AI. Check for mistakes.
bool inStaticMode = _uiMinImportance >= 0;

// Skip if nothing changed
if (noNewEvents && noSearchText && !effectiveImportanceChanged &&
Copy link

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Condition is always false because of access to local variable effectiveImportanceChanged.

Copilot uses AI. Check for mistakes.
try
{
var projectPath = System.IO.Path.GetDirectoryName(UnityEngine.Application.dataPath);
var gitPath = System.IO.Path.Combine(projectPath, ".git");
Copy link

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assignment to gitPath is useless, since its value is never read.

Copilot uses AI. Check for mistakes.
Comment on lines +462 to +468
if (token != null)
{
if (bool.TryParse(token.ToString(), out bool summaryOnly))
{
return summaryOnly;
}
}
Copy link

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These 'if' statements can be combined.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 17

🤖 Fix all issues with AI agents
In `@MCPForUnity/Editor/ActionTrace/Capture/Capturers/AssetCapture.cs`:
- Around line 145-246: The current dedup set _processedAssetsInSession prevents
legitimate later events because entries never expire; replace the
HashSet<string> with Dictionary<string,long> storing last-processed Unix-ms,
update the add/check logic in the three places that currently call
_processedAssetsInSession.Add(...) to instead prune via
CleanupOldEntries(ttlMs), then treat an entry as "already processed" only if
(now - lastProcessed) < ttlMs and otherwise update the timestamp and allow
processing; persist/load the dictionary in SaveProcessedAssets and the
corresponding load routine so timestamps survive domain reloads and implement
CleanupOldEntries to remove aged entries (TTL configurable, e.g., 5–30 minutes).

In `@MCPForUnity/Editor/ActionTrace/Capture/Capturers/SelectionCapture.cs`:
- Around line 35-47: The static ctor in SelectionCapture registers
HookRegistry.OnSelectionChanged -> OnSelectionChanged and
HookRegistry.OnPropertiesModified -> OnPropertyModified but never unsubscribes
before domain/assembly reload; add a cleanup subscription to
UnityEditor.AssemblyReloadEvents.beforeAssemblyReload (or
AssemblyReloadEvents.beforeAssemblyReload) that calls a new private static
UnsubscribeHooks() which removes those two handlers, and update the McpLog.Debug
tag from "[SelectionPropertyTracker]" to "[SelectionCapture]" so the log
reflects the correct class name.
- Around line 162-170: The code creates an EditorEvent (evt) and calls
EventStore.Record(evt) directly; change this to apply the sampling middleware
check used by Recorder.RecordEvent(): call SamplingMiddleware.ShouldRecord(evt)
(or delegate to Recorder.RecordEvent(evt) if available) and only invoke
EventStore.Record(evt) when it returns true so rapid property changes are
throttled; ensure you reference the existing EditorEvent instance (evt) and
perform the check immediately before recording.

In `@MCPForUnity/Editor/ActionTrace/Capture/Recorder.cs`:
- Around line 45-47: Both OnComponentRemoved and OnComponentRemovedDetailed are
subscribed and both emit "ComponentRemoved" events, causing duplicates; mirror
the GameObjectDestroyed/Detailed fix by unsubscribing the non-detailed handler
and only handling the detailed event (or add a de-dup guard inside the
handlers). Concretely: remove or stop subscribing to
HookRegistry.OnComponentRemoved in Recorder.cs and ensure
HookRegistry.OnComponentRemovedDetailed is the sole source for recording
component removals (also update the corresponding handler logic that writes to
EventStore/RecordEvent and remove any duplicate recording logic in the
alternative handler paths referenced around lines 108-140).

In `@MCPForUnity/Editor/ActionTrace/Core/Settings/ActionTraceSettings.cs`:
- Around line 254-260: GetEstimatedMemoryUsage can return a negative estimate
when Storage.HotEventCount > Storage.MaxEvents; change the calculation in
GetEstimatedMemoryUsage to clamp coldEvents to zero (e.g., coldEvents =
Math.Max(0, Storage.MaxEvents - Storage.HotEventCount)) or otherwise compute
totalEvents = Math.Max(Storage.HotEventCount, Storage.MaxEvents) and derive
hot/cold from that so the returned long is never negative; update references to
Storage.HotEventCount and Storage.MaxEvents inside the GetEstimatedMemoryUsage
method accordingly.
- Around line 58-60: The tooltip for HotEventCount in ActionTraceSettings.cs is
wrong about which events are retained; update the attribute on the HotEventCount
field so it explains that the last N events (most recent/newest) retain full
payload while older events beyond that limit are dehydrated (Payload=null),
referencing the HotEventCount field in ActionTraceSettings and matching the
dehydration logic in EventStore.Diagnostics.cs which keeps payloads for the most
recent entries.

In `@MCPForUnity/Editor/ActionTrace/Integration/VCS/VcsContextProvider.cs`:
- Around line 272-277: The subprocess in FindGitExecutable uses
process.WaitForExit() without a timeout which can hang the editor; update the
code that starts the git probe (the Process created from startInfo in
VcsContextProvider.FindGitExecutable) to use a bounded wait like
process.WaitForExit(5000) (or a configurable timeout), handle the case where
WaitForExit returns false (timed out) by killing/disposing the process and
treating it as failure, and ensure you still check process.ExitCode == 0 only
after a successful wait before returning "git".
- Around line 54-58: The static constructor of VcsContextProvider currently runs
whenever the type is referenced and unconditionally assigns _currentContext via
GetInitialContext() and registers EditorApplication.update += OnUpdate, causing
unnecessary Git polling even when the feature is disabled; update the static
initialization to avoid registering the update callback unless the feature is
enabled (e.g., add a guard that checks the feature flag or a IsEnabled method
before calling EditorApplication.update) or remove the static constructor
entirely and lazily initialize _currentContext and register OnUpdate inside
GetCurrentContext() (or an explicit Initialize method) so registration only
happens when the VCS feature is active.

In `@MCPForUnity/Editor/Tools/GetActionTraceSummaryTool.cs`:
- Around line 147-149: The duration calculation assumes events are in ascending
order but EventStore.Query returns newest-first, causing negative durations;
update the calculation in GetActionTraceSummaryTool so startTimeMs =
Math.Min(events[0].TimestampUnixMs, events[events.Count - 1].TimestampUnixMs)
and endTimeMs = Math.Max(events[0].TimestampUnixMs, events[events.Count -
1].TimestampUnixMs) (or use events.Min(e => e.TimestampUnixMs) and events.Max(e
=> e.TimestampUnixMs)), then set durationMs = endTimeMs - startTimeMs; reference
the existing variables events, startTimeMs, endTimeMs, and durationMs when
applying the change.
- Around line 323-344: Event selection is wrong because EventStore.Query returns
events newest-first, so FirstOrDefault(e => e.TimestampUnixMs >= thresholdMs)
picks the newest matching event instead of the oldest at/after the threshold;
update the logic in GetActionTraceSummaryTool to pick the oldest matching event
by either: scanning for the last matching element (use LastOrDefault on
sampleEvents with the same predicate) or explicitly sort/iterate sampleEvents in
ascending TimestampUnixMs then use FirstOrDefault, and return that element's
Sequence (leave the try/catch and null fallback intact).

In
`@MCPForUnity/Editor/Windows/ActionTraceEditorWindow/ActionTraceEditorWindow.cs`:
- Around line 165-172: InitializeSettings currently mutates the shared
ActionTraceSettings.Instance.Filtering.BypassImportanceFilter which can leak if
the window crashes; instead stop writing to the global setting: introduce a
window-local flag (e.g. _bypassImportanceFilterOverride) and assign it true in
InitializeSettings (and set _previousBypassImportanceFilter only if you still
need to detect prior state), remove the direct assignment to
ActionTraceSettings.Instance.Filtering.BypassImportanceFilter, and update all
internal filtering code in this window to consult the local override (use
_bypassImportanceFilterOverride ||
ActionTraceSettings.Instance.Filtering.BypassImportanceFilter) so global state
is never mutated; remove reliance on restoring in OnDisable or keep restoration
conditional if you must preserve original behavior.
- Around line 654-658: AddImportanceBar currently calls
category.ToLowerInvariant() when building the CSS class which can NRE if
category is null; update AddImportanceBar to guard against null by deriving a
safeName (e.g., use category ?? "unknown" or string.Empty) before calling
ToLowerInvariant or skip adding the importance-bar-fill--... class when category
is null, then use that safeName when calling fill.AddToClassList so
VisualElement fill and AddToClassList never receive a null-derived string.

In
`@MCPForUnity/Editor/Windows/ActionTraceEditorWindow/ActionTraceEditorWindow.uss`:
- Around line 446-454: The .detail-scroll-view CSS rule has contradictory width
constraints (min-width: 320px vs max-width: 300px); update the rule so max-width
is >= min-width (for example set max-width to 400px or 450px) to remove the
conflict and ensure predictable layout behavior in ActionTraceEditorWindow.

In
`@MCPForUnity/Editor/Windows/ActionTraceEditorWindow/Components/EventListSection.cs`:
- Around line 290-296: SetSortMode currently ignores its parameter; add a
private field (e.g. _sortMode) to store the passed SortMode in the
EventListSection class, set _sortMode = sortMode inside SetSortMode, and update
ApplySorting to switch on _sortMode instead of always sorting by timestamp
(handle the different SortMode cases such as Timestamp, Importance, etc.). Also
update any comment references that mention _sortMode to match the new field and
ensure callers rely on SetSortMode to change sorting behavior; alternatively
remove the parameter and callers if you prefer to eliminate sort-by-mode
support.

In `@Server/src/services/tools/get_action_trace_summary.py`:
- Around line 27-33: The decorator description in `@mcp_for_unity_tool` claims a
"15min summary" and "Defaults: 15min window" but the function
get_action_trace_summary has a default time_range="1h" (also referenced on the
description lines ~40-41); make them consistent by either changing the function
parameter default time_range to "15m" (or an equivalent value used elsewhere) or
updating the decorator description to state the actual default "1h" in both
places; update the description text and any other decorator lines that mention
the default so callers and docs match the function signature.
- Around line 97-102: The code currently coerces limit via coerce_int but
doesn't enforce the documented 1–500 range, allowing invalid values; update the
block in get_action_trace_summary (the section that handles limit/coerced_limit
and params_dict) to validate and enforce the range—either clamp coerced_limit to
the inclusive range 1..500 or raise/reject when out of range—then only set
params_dict["limit"] to the validated value (e.g., validated_limit = max(1,
min(coerced_limit, 500)) before assignment) so Unity always receives a limit
within 1–500.

In `@Server/src/services/tools/get_action_trace.py`:
- Around line 64-68: The current handling of limit in get_action_trace
(variables: limit, coerced_limit, coerce_int, params_dict["limit"]) accepts any
integer; modify the logic to enforce the documented 1–1000 range by validating
coerced_limit after coerce_int returns a value and either clamp it into the
[1,1000] range or raise a clear validation error; then only assign the bounded
value to params_dict["limit"]. Ensure you perform this check where coerced_limit
is set so out-of-range negatives or huge values are prevented before being sent
downstream.
♻️ Duplicate comments (4)
MCPForUnity/Editor/ActionTrace/Integration/VCS/VcsContextProvider.cs (2)

175-176: Unused variable gitPath.

gitPath is assigned but never used in RunGitCommand. This was previously flagged.

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

203-209: Empty catch block swallows exceptions.

The empty catch at line 209 silently swallows any exception from process.Kill(). Consider logging at debug level or adding a comment explaining why it's safe to ignore.

 catch { }
+// Kill may throw if process already exited; safe to ignore
MCPForUnity/Editor/ActionTrace/Capture/Sampling/SamplingMiddleware.cs (1)

219-228: Unused variable oldestSample.

The variable oldestSample is assigned on line 226 but never used - the code uses removedSample from TryRemove on line 230 instead. This is dead code.

♻️ Proposed fix
                     // Manual loop to find oldest entry (avoid LINQ allocation in hot path)
                     string oldestKey = null;
                     long oldestTimestamp = long.MaxValue;
-                    PendingSample oldestSample = default;
                     foreach (var kvp in _pendingSamples)
                     {
                         if (kvp.Value.TimestampMs < oldestTimestamp)
                         {
                             oldestTimestamp = kvp.Value.TimestampMs;
                             oldestKey = kvp.Key;
-                            oldestSample = kvp.Value;
                         }
                     }
MCPForUnity/Editor/ActionTrace/Capture/Recorder.cs (1)

30-30: Field _trackingHelper can be readonly.

Since _trackingHelper is only assigned in the static constructor and never reassigned, it should be marked as readonly.

♻️ Proposed fix
-        private static GameObjectTrackingHelper _trackingHelper;
+        private static readonly GameObjectTrackingHelper _trackingHelper;
🧹 Nitpick comments (17)
Server/src/services/tools/get_action_trace.py (1)

106-107: Avoid swallowing unexpected exceptions without logging.

A blanket Exception catch hides root causes; at least log the traceback so failures are diagnosable.

♻️ Suggested logging
+import logging
+
+logger = logging.getLogger(__name__)
@@
     except Exception as e:
+        logger.exception("get_action_trace failed")
         return {"success": False, "message": f"Python error getting action trace: {str(e)}"}
MCPForUnity/Editor/ActionTrace/Helpers/PropertyModificationHelper.cs (2)

21-53: Consider caching reflection results for hot-path performance.

GetNestedValue is called frequently from ProcessModifications (see PropertyCapture.cs lines 76-171) which processes every property modification. The path.Split('.') allocation and repeated GetProperty/GetField reflection lookups on each call may cause GC pressure in high-frequency edit scenarios.

For a "Chill" review, this is acceptable for now, but if profiling shows this as a bottleneck, consider caching PropertyInfo/FieldInfo lookups by type+path key.


76-83: Minor: Redundant null check pattern.

Line 79 checks result != null then casts again with as string. Since GetNestedValue returns object, a simpler pattern would be:

-            var result = GetNestedValue(modification, "currentValue.propertyPath");
-            if (result != null) return result as string;
+            if (GetNestedValue(modification, "currentValue.propertyPath") is string current)
+                return current;

Not a bug—just a style nitpick.

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

72-84: Thread-safety claim is overstated.

The comment states "Thread-safe (called from any thread)" but lines 78-81 have a check-then-act race: two threads could both see _currentContext == null and redundantly call GetInitialContext(). Since VcsContext is effectively immutable after creation and reference assignment is atomic, this won't corrupt data, but it's not truly thread-safe.

Either add a lock/Interlocked.CompareExchange or soften the comment to "Safe for concurrent reads; initialization may run redundantly."

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

117-126: Inconsistent locking for _saveScheduled.

ScheduleSave accesses _saveScheduled under _queryLock, but ClearPendingOperations sets it to false outside any lock (line 124). This inconsistency could cause races if ClearPendingOperations is called while a save is being scheduled.

Since this is typically called during shutdown, it's low risk, but for consistency:

♻️ Suggested fix
 public static void ClearPendingOperations()
 {
     lock (_pendingNotifications)
     {
         _pendingNotifications.Clear();
         _notifyScheduled = false;
     }
+    lock (_queryLock)
+    {
         _saveScheduled = false;
+    }
     _lastDehydratedCount = -1;  // Reset dehydration optimization marker
 }
MCPForUnity/Editor/Windows/ActionTraceEditorWindow/Components/EventListSection.cs (2)

173-259: BindListItem performs repeated Q<> queries on each bind.

Lines 180-188 call element.Q<>() for 9 different elements on every bind. For virtualized lists with frequent scrolling, this causes repeated tree traversals. Consider caching element references in a user data object attached to the root element during MakeListItem.

For a Chill review, this is acceptable if performance is adequate, but it's a known pattern for optimization.


366-406: Complex change detection logic; consider simplifying or documenting.

ShouldSkipRefresh has intricate state tracking across 5+ variables. While the logic appears correct, it's difficult to reason about edge cases. Consider adding a brief comment block explaining the refresh-skip conditions, or refactoring into named helper methods for each condition category.

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

40-44: Consider consistent naming for duplicate "Clear Filters" buttons.

Lines 43 and 81 define two "Clear Filters" buttons (clear-filters-button and clear-filters-button-2). If they share the same handler, consider using a common CSS class for event binding rather than name-based lookups, or document why two distinct names are needed.

Also applies to: 73-83

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

199-214: Duplicate code: GetGameObjectPath exists in both SelectionCapture and Recorder.

This same method implementation appears in Recorder.cs (lines 343-357). Consider extracting this to a shared helper class to avoid duplication and ensure consistent behavior.

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

274-322: FilterSection stores callbacks but lacks cleanup method.

Callbacks are stored in fields (lines 274-278) and registered (lines 318-322), but there's no UnregisterCallbacks method like EventListSection has. While the window lifetime typically matches the section lifetime, explicitly unregistering provides cleaner resource management.

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

204-209: Negative padding value may not behave as expected.

padding-top: -2px on line 206 is unusual. Unity's UIToolkit USS may not support negative padding values consistently. If this is intended to adjust spacing, consider using margin-top with a negative value or adjusting the layout differently.

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

121-152: Prefer avoiding manual Monitor.Exit/Enter inside lock.
This pattern is non-idiomatic and can be fragile; consider moving the flush outside the critical section with a flag/recheck to keep lock discipline straightforward.

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

98-103: Promote AINote to a first-class constant.
Using a literal here sidesteps the checklist and can drift from other references.

♻️ Suggested tweak
+        public const string AINote = "AINote";
...
-                ["AINote"] = new EventMetadata
+                [AINote] = new EventMetadata
MCPForUnity/Editor/Tools/GetActionTraceSummaryTool.cs (1)

159-161: Avoid creating a new DefaultEventScorer instance per event.

A DefaultEventScorer is already instantiated at line 90. Reuse it inside the loop to avoid N allocations.

♻️ Proposed fix
-                // Count by importance (using scorer)
-                var scorer = new DefaultEventScorer();
-                float importance = scorer.Score(evt);
+                // Count by importance (reuse scorer from outer scope)
+                float importance = scorer.Score(evt);

This requires renaming the outer scorer variable or passing it into GenerateSummary. Alternatively, use DefaultEventScorer.Instance:

-                var scorer = new DefaultEventScorer();
-                float importance = scorer.Score(evt);
+                float importance = DefaultEventScorer.Instance.Score(evt);
MCPForUnity/Editor/ActionTrace/Capture/Emitter.cs (1)

459-477: Consider clarifying the distinction between EmitAssetDeleted overloads.

There are two EmitAssetDeleted methods:

  1. EmitAssetDeleted(string assetPath) (line 286) - general asset deletion
  2. EmitAssetDeleted(string assetPath, string assetType) (line 459) - MCP tool-initiated deletion with source = "mcp_tool"

This design appears intentional, but the subtle difference (source tracking) may not be obvious to callers. Consider renaming the MCP-specific variant to EmitAssetDeletedViaTool or adding a clearer XML doc note explaining when to use each.

MCPForUnity/Editor/ActionTrace/Analysis/Query/ActionTraceQuery.cs (1)

144-151: Consider combining the if statements for consistency with ProjectWithContext.

ProjectWithContext uses a combined conditional pattern (lines 220-221), while this method uses separate nested if statements. This is functionally correct but could be unified for consistency.

♻️ Proposed fix
                // Convert GlobalID to InstanceID and Name (with caching)
                (int? instanceId, string displayName) targetInfo = (null, null);
-               if (!string.IsNullOrEmpty(evt.TargetId))
+               if (!string.IsNullOrEmpty(evt.TargetId) &&
+                   !instanceInfoCache.TryGetValue(evt.TargetId, out targetInfo))
                {
-                   if (!instanceInfoCache.TryGetValue(evt.TargetId, out targetInfo))
-                   {
-                       targetInfo = GlobalIdHelper.GetInstanceInfo(evt.TargetId);
-                       instanceInfoCache[evt.TargetId] = targetInfo;
-                   }
+                   targetInfo = GlobalIdHelper.GetInstanceInfo(evt.TargetId);
+                   instanceInfoCache[evt.TargetId] = targetInfo;
                }
MCPForUnity/Editor/Hooks/HookRegistry.cs (1)

119-139: dynamic dispatch introduces runtime overhead; consider typed alternatives for hot paths.

The InvokeSafely helper uses Action<dynamic> which incurs DLR (Dynamic Language Runtime) overhead on each invocation. For high-frequency events like OnEditorUpdate, this could add measurable cost.

For editor tooling this is likely acceptable, but if profiling reveals issues, consider strongly-typed overloads or cached delegate invocations.

Comment on lines +145 to +246
// L0 Deduplication: Skip if already processed in this session
// This prevents duplicate events when Unity fires OnPostprocessAllAssets
// multiple times for the same asset (creation, compilation, re-import)
if (!_processedAssetsInSession.Add(assetPath))
continue; // Already processed, skip to prevent duplicate events

hasChanges = true; // Mark that we added a new entry

// L1 Blacklist: Skip junk assets before creating events
if (!EventFilter.ShouldTrackAsset(assetPath))
{
// Remove from tracking if it's a junk asset (we don't want to track it)
_processedAssetsInSession.Remove(assetPath);
continue;
}

string targetId = $"Asset:{assetPath}";
string assetType = GetAssetType(assetPath);

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

// Mutually exclusive event classification (prevents duplicate events)
if (IsNewlyCreatedAsset(assetPath))
{
// Priority 1: Newly created assets (first-time existence)
RecordEvent(EventTypes.AssetCreated, targetId, payload);
}
else if (ShouldTrackModification(assetPath))
{
// Priority 2: Existing assets with trackable modification types
// Covers: re-imports, content changes, settings updates
RecordEvent(EventTypes.AssetModified, targetId, payload);
}
else
{
// Priority 3: Generic imports (fallback for untracked types)
RecordEvent(EventTypes.AssetImported, targetId, payload);
}
}

// ========== Deleted Assets ==========
foreach (var assetPath in deletedAssets)
{
if (string.IsNullOrEmpty(assetPath)) continue;

// L0 Deduplication: Skip if already processed in this session
if (!_processedAssetsInSession.Add(assetPath))
continue;

hasChanges = true; // Mark that we added a new entry

// L1 Blacklist: Skip junk assets
if (!EventFilter.ShouldTrackAsset(assetPath))
continue;

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

var payload = new Dictionary<string, object>
{
["path"] = assetPath
};

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

// ========== 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;

hasChanges = true; // Mark that we added a new entry

var fromPath = i < movedFromAssetPaths.Length ? movedFromAssetPaths[i] : "";

// L1 Blacklist: Skip junk assets
if (!EventFilter.ShouldTrackAsset(movedAssets[i]))
continue;

string targetId = $"Asset:{movedAssets[i]}";

var payload = new Dictionary<string, object>
{
["to_path"] = movedAssets[i],
["from_path"] = fromPath
};

RecordEvent(EventTypes.AssetMoved, targetId, payload);
}

// Persist the cache to disk if there were any changes
if (hasChanges)
SaveProcessedAssets();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Dedup cache suppresses legitimate future asset events.
Because _processedAssetsInSession never expires by time, any later import/modify/move on the same path in the same session is skipped until the cache clears (currently only when size > 1000). That risks losing real asset changes and contradicts the “time-based expiration” comment.

Consider tracking last-processed timestamps and expiring entries by age (e.g., 5–30 minutes), or scoping dedup to a batch window instead of the entire session.

[suggested approach]

  • Replace the HashSet<string> with Dictionary<string, long> (last processed Unix ms).
  • Prune by TTL in CleanupOldEntries.
  • Persist timestamps to disk so domain reloads keep the TTL semantics.
🛠️ Sketch of the intent (conceptual)
- private static HashSet<string> _cachedProcessedAssets;
+ private static Dictionary<string, long> _cachedProcessedAssets;

- if (!_processedAssetsInSession.Add(assetPath)) continue;
+ if (IsRecentlyProcessed(assetPath)) continue;
+ MarkProcessed(assetPath);

+ private static bool IsRecentlyProcessed(string path)
+ {
+     return _cachedProcessedAssets.TryGetValue(path, out var ts)
+            && (DateTimeOffset.UtcNow.ToUnixTimeMilliseconds() - ts) < ExpireWindowMs;
+ }

Also applies to: 253-268

🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/ActionTrace/Capture/Capturers/AssetCapture.cs` around
lines 145 - 246, The current dedup set _processedAssetsInSession prevents
legitimate later events because entries never expire; replace the
HashSet<string> with Dictionary<string,long> storing last-processed Unix-ms,
update the add/check logic in the three places that currently call
_processedAssetsInSession.Add(...) to instead prune via
CleanupOldEntries(ttlMs), then treat an entry as "already processed" only if
(now - lastProcessed) < ttlMs and otherwise update the timestamp and allow
processing; persist/load the dictionary in SaveProcessedAssets and the
corresponding load routine so timestamps survive domain reloads and implement
CleanupOldEntries to remove aged entries (TTL configurable, e.g., 5–30 minutes).

Comment on lines +35 to +47
static SelectionCapture()
{
// Initialize with current selection
UpdateSelectionState();

// Monitor selection changes via HookRegistry
HookRegistry.OnSelectionChanged += OnSelectionChanged;

// Monitor property modifications via HookRegistry
HookRegistry.OnPropertiesModified += OnPropertyModified;

McpLog.Debug("[SelectionPropertyTracker] Initialized");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Missing event unsubscription on domain reload.

The static constructor subscribes to HookRegistry.OnSelectionChanged and HookRegistry.OnPropertiesModified, but there's no corresponding unsubscription before assembly reload. This can cause issues with stale references after domain reload.

Additionally, the log message on line 46 references [SelectionPropertyTracker] but the class is named SelectionCapture.

🔧 Proposed fix
 static SelectionCapture()
 {
     // Initialize with current selection
     UpdateSelectionState();

     // Monitor selection changes via HookRegistry
     HookRegistry.OnSelectionChanged += OnSelectionChanged;

     // Monitor property modifications via HookRegistry
     HookRegistry.OnPropertiesModified += OnPropertyModified;

-    McpLog.Debug("[SelectionPropertyTracker] Initialized");
+    McpLog.Debug("[SelectionCapture] Initialized");
+
+    // Subscribe to cleanup on domain reload
+    AssemblyReloadEvents.beforeAssemblyReload += OnBeforeAssemblyReload;
+}
+
+private static void OnBeforeAssemblyReload()
+{
+    HookRegistry.OnSelectionChanged -= OnSelectionChanged;
+    HookRegistry.OnPropertiesModified -= OnPropertyModified;
+    AssemblyReloadEvents.beforeAssemblyReload -= OnBeforeAssemblyReload;
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
static SelectionCapture()
{
// Initialize with current selection
UpdateSelectionState();
// Monitor selection changes via HookRegistry
HookRegistry.OnSelectionChanged += OnSelectionChanged;
// Monitor property modifications via HookRegistry
HookRegistry.OnPropertiesModified += OnPropertyModified;
McpLog.Debug("[SelectionPropertyTracker] Initialized");
}
static SelectionCapture()
{
// Initialize with current selection
UpdateSelectionState();
// Monitor selection changes via HookRegistry
HookRegistry.OnSelectionChanged += OnSelectionChanged;
// Monitor property modifications via HookRegistry
HookRegistry.OnPropertiesModified += OnPropertyModified;
McpLog.Debug("[SelectionCapture] Initialized");
// Subscribe to cleanup on domain reload
AssemblyReloadEvents.beforeAssemblyReload += OnBeforeAssemblyReload;
}
private static void OnBeforeAssemblyReload()
{
HookRegistry.OnSelectionChanged -= OnSelectionChanged;
HookRegistry.OnPropertiesModified -= OnPropertyModified;
AssemblyReloadEvents.beforeAssemblyReload -= OnBeforeAssemblyReload;
}
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/ActionTrace/Capture/Capturers/SelectionCapture.cs` around
lines 35 - 47, The static ctor in SelectionCapture registers
HookRegistry.OnSelectionChanged -> OnSelectionChanged and
HookRegistry.OnPropertiesModified -> OnPropertyModified but never unsubscribes
before domain/assembly reload; add a cleanup subscription to
UnityEditor.AssemblyReloadEvents.beforeAssemblyReload (or
AssemblyReloadEvents.beforeAssemblyReload) that calls a new private static
UnsubscribeHooks() which removes those two handlers, and update the McpLog.Debug
tag from "[SelectionPropertyTracker]" to "[SelectionCapture]" so the log
reflects the correct class name.

Comment on lines +162 to +170
var evt = new EditorEvent(
sequence: 0,
timestampUnixMs: DateTimeOffset.UtcNow.ToUnixTimeMilliseconds(),
type: EventTypes.SelectionPropertyModified,
targetId: targetGlobalId,
payload: payload
);

EventStore.Record(evt);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Missing sampling middleware check before recording.

Unlike Recorder.RecordEvent() which applies SamplingMiddleware.ShouldRecord(evt) before recording, this method directly calls EventStore.Record(evt). This inconsistency could lead to event floods during rapid property modifications (e.g., dragging a slider).

🔧 Proposed fix
+using MCPForUnity.Editor.ActionTrace.Capture;
 // ... at the recording site:
 
             var evt = new EditorEvent(
                 sequence: 0,
                 timestampUnixMs: DateTimeOffset.UtcNow.ToUnixTimeMilliseconds(),
                 type: EventTypes.SelectionPropertyModified,
                 targetId: targetGlobalId,
                 payload: payload
             );

+            // Apply sampling middleware to prevent event floods
+            if (!SamplingMiddleware.ShouldRecord(evt))
+                return;
+
             EventStore.Record(evt);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
var evt = new EditorEvent(
sequence: 0,
timestampUnixMs: DateTimeOffset.UtcNow.ToUnixTimeMilliseconds(),
type: EventTypes.SelectionPropertyModified,
targetId: targetGlobalId,
payload: payload
);
EventStore.Record(evt);
var evt = new EditorEvent(
sequence: 0,
timestampUnixMs: DateTimeOffset.UtcNow.ToUnixTimeMilliseconds(),
type: EventTypes.SelectionPropertyModified,
targetId: targetGlobalId,
payload: payload
);
// Apply sampling middleware to prevent event floods
if (!SamplingMiddleware.ShouldRecord(evt))
return;
EventStore.Record(evt);
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/ActionTrace/Capture/Capturers/SelectionCapture.cs` around
lines 162 - 170, The code creates an EditorEvent (evt) and calls
EventStore.Record(evt) directly; change this to apply the sampling middleware
check used by Recorder.RecordEvent(): call SamplingMiddleware.ShouldRecord(evt)
(or delegate to Recorder.RecordEvent(evt) if available) and only invoke
EventStore.Record(evt) when it returns true so rapid property changes are
throttled; ensure you reference the existing EditorEvent instance (evt) and
perform the check immediately before recording.

Comment on lines +45 to +47
HookRegistry.OnComponentAdded += OnComponentAdded;
HookRegistry.OnComponentRemoved += OnComponentRemoved;
HookRegistry.OnComponentRemovedDetailed += OnComponentRemovedDetailed;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Potential duplicate ComponentRemoved events.

Both OnComponentRemoved and OnComponentRemovedDetailed handlers are subscribed and both record events with type "ComponentRemoved". If HookRegistry fires both events for the same component removal, this will result in duplicate entries in the EventStore.

Note the comment on lines 49-50 explains why OnGameObjectDestroyed is skipped in favor of OnGameObjectDestroyedDetailed. Consider applying similar logic here.

🔧 Suggested approach
             // Subscribe to HookRegistry events
             HookRegistry.OnComponentAdded += OnComponentAdded;
-            HookRegistry.OnComponentRemoved += OnComponentRemoved;
+            // Note: We only use OnComponentRemovedDetailed since it has complete cached data
+            // OnComponentRemoved may be called with partially valid data, so we skip it to avoid duplicates
             HookRegistry.OnComponentRemovedDetailed += OnComponentRemovedDetailed;

Also applies to: 108-140

🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/ActionTrace/Capture/Recorder.cs` around lines 45 - 47,
Both OnComponentRemoved and OnComponentRemovedDetailed are subscribed and both
emit "ComponentRemoved" events, causing duplicates; mirror the
GameObjectDestroyed/Detailed fix by unsubscribing the non-detailed handler and
only handling the detailed event (or add a de-dup guard inside the handlers).
Concretely: remove or stop subscribing to HookRegistry.OnComponentRemoved in
Recorder.cs and ensure HookRegistry.OnComponentRemovedDetailed is the sole
source for recording component removals (also update the corresponding handler
logic that writes to EventStore/RecordEvent and remove any duplicate recording
logic in the alternative handler paths referenced around lines 108-140).

Comment on lines +446 to +454
.detail-scroll-view {
flex: 1;
min-width: 320px;
min-height: 100px;
max-width: 300px;
background-color: #252525;
border-left-width: 1px;
border-left-color: #3a3a3a;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Contradictory width constraints: min-width: 320px and max-width: 300px.

The detail panel has min-width: 320px but max-width: 300px, which is impossible to satisfy. This will cause unpredictable layout behavior. The max-width should likely be larger (e.g., 400px or 450px).

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

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
.detail-scroll-view {
flex: 1;
min-width: 320px;
min-height: 100px;
max-width: 300px;
background-color: #252525;
border-left-width: 1px;
border-left-color: #3a3a3a;
}
.detail-scroll-view {
flex: 1;
min-width: 320px;
min-height: 100px;
max-width: 450px;
background-color: `#252525`;
border-left-width: 1px;
border-left-color: `#3a3a3a`;
}
🤖 Prompt for AI Agents
In
`@MCPForUnity/Editor/Windows/ActionTraceEditorWindow/ActionTraceEditorWindow.uss`
around lines 446 - 454, The .detail-scroll-view CSS rule has contradictory width
constraints (min-width: 320px vs max-width: 300px); update the rule so max-width
is >= min-width (for example set max-width to 400px or 450px) to remove the
conflict and ensure predictable layout behavior in ActionTraceEditorWindow.

Comment on lines +290 to +296
public void SetSortMode(SortMode sortMode)
{
if (ActionTraceSettings.Instance != null)
ActionTraceSettings.Instance.Filtering.BypassImportanceFilter = true;
_forceRefreshNextCall = true;
RefreshData();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

SetSortMode parameter is unused; sorting is hardcoded.

SetSortMode(SortMode sortMode) accepts a parameter but never stores or uses it. The comment at line 410 mentions "_sortMode" which doesn't exist, and ApplySorting always sorts by timestamp.

This appears to be incomplete implementation. Either implement the sort mode logic or remove the parameter:

♻️ Option: Store and use sort mode
+        private SortMode _sortMode = SortMode.TimeDescending;
+
         public void SetSortMode(SortMode sortMode)
         {
+            _sortMode = sortMode;
             if (ActionTraceSettings.Instance != null)
                 ActionTraceSettings.Instance.Filtering.BypassImportanceFilter = true;
             _forceRefreshNextCall = true;
             RefreshData();
         }

         private IEnumerable<ActionTraceQuery.ActionTraceViewItem> ApplySorting(...)
         {
-            // Sort mode is controlled externally via _sortMode
-            // For now, default to time-based sorting
-            return source.OrderByDescending(e => e.Event.TimestampUnixMs);
+            return _sortMode switch
+            {
+                SortMode.TimeAscending => source.OrderBy(e => e.Event.TimestampUnixMs),
+                SortMode.ImportanceDescending => source.OrderByDescending(e => e.ImportanceScore),
+                _ => source.OrderByDescending(e => e.Event.TimestampUnixMs)
+            };
         }

Also applies to: 408-413

🤖 Prompt for AI Agents
In
`@MCPForUnity/Editor/Windows/ActionTraceEditorWindow/Components/EventListSection.cs`
around lines 290 - 296, SetSortMode currently ignores its parameter; add a
private field (e.g. _sortMode) to store the passed SortMode in the
EventListSection class, set _sortMode = sortMode inside SetSortMode, and update
ApplySorting to switch on _sortMode instead of always sorting by timestamp
(handle the different SortMode cases such as Timestamp, Importance, etc.). Also
update any comment references that mention _sortMode to match the new field and
ensure callers rely on SetSortMode to change sorting behavior; alternatively
remove the parameter and callers if you prefer to eliminate sort-by-mode
support.

Comment on lines +27 to +33
@mcp_for_unity_tool(
description="""⚠️ CALL BEFORE CHANGES - Prevents conflicts & wasted work

15min summary of recent Unity ops. Detects conflicts before you break things.
Defaults: 15min window, medium importance.

Use when: modifying existing objects, debugging, or user mentions "刚才/之前/那个".""",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Align the tool description with the actual defaults.

The decorator says “15min summary” and “Defaults: 15min window,” but the function default is time_range="1h". Please update one or the other to avoid confusing callers.

Also applies to: 40-41

🤖 Prompt for AI Agents
In `@Server/src/services/tools/get_action_trace_summary.py` around lines 27 - 33,
The decorator description in `@mcp_for_unity_tool` claims a "15min summary" and
"Defaults: 15min window" but the function get_action_trace_summary has a default
time_range="1h" (also referenced on the description lines ~40-41); make them
consistent by either changing the function parameter default time_range to "15m"
(or an equivalent value used elsewhere) or updating the decorator description to
state the actual default "1h" in both places; update the description text and
any other decorator lines that mention the default so callers and docs match the
function signature.

Comment on lines +97 to +102
# Coerce and add optional parameters
if limit is not None:
coerced_limit = coerce_int(limit)
if coerced_limit is not None:
params_dict["limit"] = coerced_limit

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Enforce the documented 1–500 limit before sending to Unity.

limit is coerced but never range-validated, so 0/negative or huge values can slip through and cause backend errors or heavy payloads. Please enforce the contract here (clamp or reject).

🔧 Suggested fix
     if limit is not None:
         coerced_limit = coerce_int(limit)
         if coerced_limit is not None:
-            params_dict["limit"] = coerced_limit
+            if 1 <= coerced_limit <= 500:
+                params_dict["limit"] = coerced_limit
+            else:
+                return {
+                    "success": False,
+                    "message": "limit must be between 1 and 500"
+                }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# Coerce and add optional parameters
if limit is not None:
coerced_limit = coerce_int(limit)
if coerced_limit is not None:
params_dict["limit"] = coerced_limit
# Coerce and add optional parameters
if limit is not None:
coerced_limit = coerce_int(limit)
if coerced_limit is not None:
if 1 <= coerced_limit <= 500:
params_dict["limit"] = coerced_limit
else:
return {
"success": False,
"message": "limit must be between 1 and 500"
}
🤖 Prompt for AI Agents
In `@Server/src/services/tools/get_action_trace_summary.py` around lines 97 - 102,
The code currently coerces limit via coerce_int but doesn't enforce the
documented 1–500 range, allowing invalid values; update the block in
get_action_trace_summary (the section that handles limit/coerced_limit and
params_dict) to validate and enforce the range—either clamp coerced_limit to the
inclusive range 1..500 or raise/reject when out of range—then only set
params_dict["limit"] to the validated value (e.g., validated_limit = max(1,
min(coerced_limit, 500)) before assignment) so Unity always receives a limit
within 1–500.

Comment on lines +64 to +68
if limit is not None:
coerced_limit = coerce_int(limit)
if coerced_limit is not None:
params_dict["limit"] = coerced_limit

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Enforce the documented limit range (1–1000).

The annotation promises a bounded range, but any integer is currently accepted; negative or huge values could cause heavy payloads or Unity-side errors. Consider clamping or rejecting out-of-range values.

✅ Proposed guard
         if limit is not None:
             coerced_limit = coerce_int(limit)
             if coerced_limit is not None:
+                if coerced_limit < 1 or coerced_limit > 1000:
+                    return {
+                        "success": False,
+                        "message": "limit must be between 1 and 1000"
+                    }
                 params_dict["limit"] = coerced_limit
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if limit is not None:
coerced_limit = coerce_int(limit)
if coerced_limit is not None:
params_dict["limit"] = coerced_limit
if limit is not None:
coerced_limit = coerce_int(limit)
if coerced_limit is not None:
if coerced_limit < 1 or coerced_limit > 1000:
return {
"success": False,
"message": "limit must be between 1 and 1000"
}
params_dict["limit"] = coerced_limit
🤖 Prompt for AI Agents
In `@Server/src/services/tools/get_action_trace.py` around lines 64 - 68, The
current handling of limit in get_action_trace (variables: limit, coerced_limit,
coerce_int, params_dict["limit"]) accepts any integer; modify the logic to
enforce the documented 1–1000 range by validating coerced_limit after coerce_int
returns a value and either clamp it into the [1,1000] range or raise a clear
validation error; then only assign the bounded value to params_dict["limit"].
Ensure you perform this check where coerced_limit is set so out-of-range
negatives or huge values are prevented before being sent downstream.

@whatevertogo
Copy link
Contributor Author

Hook System Overview

Purpose

HookSystem is an event bus layer that allows systems to subscribe to Unity editor events without directly depending on Unity APIs.

ActionTrace subscribes to all editor events through HookSystem, recording user operation history so AI can understand "what just happened."

Unity Events → UnityEventHooks (detection) → HookRegistry (public API) → Subscribers
                                                          ↑
                                                    ActionTrace → EventStore

Core Benefits:

  • Decoupling: Subscribers don't need to know EditorApplication.*, Selection.*, etc.
  • Reuse: Multiple systems can subscribe to the same event without duplicating boilerplate
  • Unified cleanup: Only one place to handle unsubscribe on domain reload

Available Events

Category HookRegistry Events
Script Compilation OnScriptCompiled, OnScriptCompilationFailed
Scene Operations OnSceneSaved, OnSceneOpened, OnNewSceneCreated
GameObject OnGameObjectCreated, OnGameObjectDestroyed
Component OnComponentAdded, OnComponentRemoved
Selection OnSelectionChanged
Property Modification OnPropertiesModified
Play Mode OnPlayModeChanged
Hierarchy OnHierarchyChanged
Build OnBuildCompleted
Editor OnEditorUpdate, OnEditorIdle

Files That Could Use HookSystem

✅ Already Migrated

File Was Using Now Using
PropertyCapture.cs Undo.postprocessModifications, EditorApplication.update HookRegistry.OnPropertiesModified, OnEditorUpdate
SelectionCapture.cs Selection.selectionChanged, Undo.postprocessModifications HookRegistry.OnSelectionChanged, OnPropertiesModified
Recorder.cs (ActionTrace) Various Unity events HookRegistry.*

🔶 Could Migrate (Needs Evaluation)

File Currently Listens To Suggested
StdioBridgeReloadHandler.cs AssemblyReloadEvents.before/afterAssemblyReload Add OnDomainReloading/Reloaded to HookRegistry first
HttpBridgeReloadHandler.cs AssemblyReloadEvents.* Same as above
EditorStateCache.cs EditorApplication.update, playModeStateChanged OnEditorUpdate, OnPlayModeChanged

❌ Not Recommended

File Reason
EditorStateCache.cs (update part) Has custom throttling and state-change detection that would be broken by moving to a generic hook

Usage Example

// Subscribe to events
HookRegistry.OnGameObjectCreated += (go) => Debug.Log($"Created: {go.name}");
HookRegistry.OnScriptCompiled += () => RefreshMySystem();

// Unsubscribe
HookRegistry.OnGameObjectCreated -= myHandler;

@whatevertogo
Copy link
Contributor Author

ActionTrace: What it can do / What it cannot do

✅ What it CAN do

Feature Description
Operation History Records what you do in the Editor: creating objects, modifying properties, saving assets, compiling scripts, etc.
Trace Object Origin "Who created this pink cube?" → Query ActionTrace to find the exact operation that created it
Context Understanding "Make that cube red" → AI uses history to know which object "that" refers to
Filter by Importance Ignore noise, focus on important operations (errors, creates/deletes, etc.)
Diagnose Issues When something breaks, trace back: "What happened after I modified that material?"
Distinguish Operation Sources(It’s not finished yet because I haven't modified the code from others.) Know whether an action was done manually by you, by an AI tool call,by another ai tool call or triggered by the system

❌ What it CANNOT do

Limitation Description
Cannot Execute Actions It's just a "logger" — it cannot directly modify scenes or run commands
Cannot See Runtime State Only records Editor operations; Play Mode runtime behavior is not captured
Cannot Undo Actions Cannot directly rollback history (Unity's built-in Undo system handles that)
Cannot Access Project-External Content Only knows what happens inside Unity Editor
Cannot Predict the Future Only sees what has already happened

In one sentence: ActionTrace is AI's "memory notebook" — it remembers what you did, but doesn't act on its own.

whatevertogo and others added 3 commits January 24, 2026 09:31
…ce.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…cs.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…gs.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@MCPForUnity/Editor/ActionTrace/Core/Settings/ActionTraceSettings.cs`:
- Around line 195-205: CreateSettings calls
ApplyPreset(ActionTracePreset.Standard) before the asset exists on disk, but
ApplyPreset calls Save() which uses EditorUtility.SetDirty(this) and
AssetDatabase.SaveAssets() causing warnings; to fix, delay applying the preset
until after the asset is created on disk or inline the preset values directly:
move the ApplyPreset(ActionTracePreset.Standard) call to after
AssetDatabase.CreateAsset(settings, SettingsPath) (and keep subsequent
AssetDatabase.SaveAssets()/Refresh()), or alternatively set the fields on the
new settings instance instead of calling ApplyPreset/Save before creation;
reference CreateSettings, ApplyPreset, Save, AssetDatabase.CreateAsset,
SettingsPath when updating the code.
♻️ Duplicate comments (1)
MCPForUnity/Editor/ActionTrace/Core/Settings/ActionTraceSettings.cs (1)

254-260: GetEstimatedMemoryUsage can return negative if settings are invalid.

Line 258 computes coldEvents = Storage.MaxEvents - Storage.HotEventCount. If HotEventCount > MaxEvents (which Validate() warns about but doesn't prevent), coldEvents becomes negative, yielding an incorrect memory estimate.

♻️ Suggested defensive fix
 public long GetEstimatedMemoryUsage()
 {
     // Approximate: each event ~300 bytes when hydrated, ~100 bytes when dehydrated
     int hotEvents = Storage.HotEventCount;
-    int coldEvents = Storage.MaxEvents - Storage.HotEventCount;
+    int coldEvents = Math.Max(0, Storage.MaxEvents - Storage.HotEventCount);
     return (long)(hotEvents * 300 + coldEvents * 100);
 }
🧹 Nitpick comments (1)
MCPForUnity/Editor/ActionTrace/Core/Store/EventStore.Persistence.cs (1)

136-145: _saveScheduled reset outside lock could race with ScheduleSave.

Line 143 sets _saveScheduled = false outside any lock, while ScheduleSave checks and sets this flag under _queryLock. In a multi-threaded scenario this could cause a race. However, since this is called during shutdown/domain reload when other operations shouldn't be active, and Unity Editor is predominantly single-threaded, this is low-risk.

♻️ Optional: wrap in lock for consistency
 public static void ClearPendingOperations()
 {
     lock (_pendingNotifications)
     {
         _pendingNotifications.Clear();
         _notifyScheduled = false;
     }
-    _saveScheduled = false;
+    lock (_queryLock)
+    {
+        _saveScheduled = false;
+    }
     _lastDehydratedCount = -1;  // Reset dehydration optimization marker
 }

Comment on lines +195 to +205
private static ActionTraceSettings CreateSettings()
{
var settings = CreateInstance<ActionTraceSettings>();
// 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;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

ApplyPreset calls Save() before the asset exists on disk.

ApplyPreset(ActionTracePreset.Standard) internally calls Save(), which invokes EditorUtility.SetDirty(this) and AssetDatabase.SaveAssets(). However, at this point, the asset hasn't been created yet (that happens on line 200). This likely generates a warning or fails silently.

🔧 Proposed fix: defer preset application or inline the values
 private static ActionTraceSettings CreateSettings()
 {
     var settings = CreateInstance<ActionTraceSettings>();
-    // Apply Standard preset by default
-    settings.ApplyPreset(ActionTracePreset.Standard);
+    // Apply Standard preset values directly (without Save)
+    var preset = ActionTracePreset.Standard;
+    settings.Filtering.MinImportanceForRecording = preset.MinImportance;
+    settings.Storage.MaxEvents = preset.MaxEvents;
+    settings.Storage.HotEventCount = preset.HotEventCount;
+    settings.Merging.EnableEventMerging = preset.EnableEventMerging;
+    settings.Merging.MergeWindowMs = preset.MergeWindowMs;
+    settings.Merging.TransactionWindowMs = preset.TransactionWindowMs;
+    settings._currentPresetName = preset.Name;
+
     AssetDatabase.CreateAsset(settings, SettingsPath);
     AssetDatabase.SaveAssets();
     AssetDatabase.Refresh();
     McpLog.Info($"[ActionTraceSettings] Created new settings at {SettingsPath}");
     return settings;
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
private static ActionTraceSettings CreateSettings()
{
var settings = CreateInstance<ActionTraceSettings>();
// 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;
}
private static ActionTraceSettings CreateSettings()
{
var settings = CreateInstance<ActionTraceSettings>();
AssetDatabase.CreateAsset(settings, SettingsPath);
// Apply Standard preset by default (after asset is registered)
settings.ApplyPreset(ActionTracePreset.Standard);
AssetDatabase.SaveAssets();
AssetDatabase.Refresh();
McpLog.Info($"[ActionTraceSettings] Created new settings at {SettingsPath}");
return settings;
}
🤖 Prompt for AI Agents
In `@MCPForUnity/Editor/ActionTrace/Core/Settings/ActionTraceSettings.cs` around
lines 195 - 205, CreateSettings calls ApplyPreset(ActionTracePreset.Standard)
before the asset exists on disk, but ApplyPreset calls Save() which uses
EditorUtility.SetDirty(this) and AssetDatabase.SaveAssets() causing warnings; to
fix, delay applying the preset until after the asset is created on disk or
inline the preset values directly: move the
ApplyPreset(ActionTracePreset.Standard) call to after
AssetDatabase.CreateAsset(settings, SettingsPath) (and keep subsequent
AssetDatabase.SaveAssets()/Refresh()), or alternatively set the fields on the
new settings instance instead of calling ApplyPreset/Save before creation;
reference CreateSettings, ApplyPreset, Save, AssetDatabase.CreateAsset,
SettingsPath when updating the code.

@dsarno
Copy link
Collaborator

dsarno commented Jan 26, 2026

@whatevertogo Thanks again for your work here. We still think it's not a fit for the mcp repo right now for reasons mentioned before. So I'll close the PR tomorrow. But by all means keep developing the feature independently, and as we said, we're happy to track your progress and have you share it on the Discord etc. Thanks again!

@whatevertogo
Copy link
Contributor Author

Understood

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants