feat: Capture and format pause-point variables (locals, parameters, instance fields, UnityEngine.Object)#1682
Merged
Conversation
Source pause points need to report captured local/parameter/instance
field values alongside a hit, in addition to the existing bare
Hit(id) used by the handwritten UloopPausePoint.Pause("id") marker.
Adds UloopCapturedVariable (string/int only, no Editor API
dependency) plus scope/UnityObject-kind name constants, threads a
captured variable list and truncation flag through
UloopPausePointEntry/UloopPausePointSnapshot, and adds
UloopPausePointRegistry.HitWithCapturedVariables (kept separate from
Hit(id) per this project's no-overload rule) sharing a private
HitCore with the existing Hit(id) path. Also adds IsArmed(id) so the
upcoming injected Capture landing point can no-op on a single
dictionary lookup without paying reflection/formatting cost when the
marker isn't armed.
Refs: Phase 3 (ToDo 3-1) of the source pause point execution plan.
…d capture entry point Implements ToDo 3-2/3-3: formats captured locals/parameters/instance fields (with async/iterator hoisted-local demangling and one-level <>4__this following), classifies live UnityEngine.Object references into the 6-category kind set, degrades gracefully off the main thread, and caps both value length and variable count. SourcePausePointCapture is the landing point Harmony will call in PR4, gated on the registry's armed check.
Contributor
📝 WalkthroughWalkthroughThis change adds source pause-point variable capture, including formatting, Unity object classification, truncation, and safe string conversion. Captured data now flows through the pause-point registry into snapshots, with tests covering capture, arming, reflection, classification, and threading. ChangesSource pause-point variable capture
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SourcePausePointCapture
participant SourcePausePointVariableFormatter
participant SourcePausePointUnityObjectClassifier
participant UloopPausePointRegistry
participant UloopPausePointSnapshot
SourcePausePointCapture->>UloopPausePointRegistry: Check armed state
SourcePausePointCapture->>SourcePausePointVariableFormatter: Format captured values
SourcePausePointVariableFormatter->>SourcePausePointUnityObjectClassifier: Classify Unity objects
SourcePausePointCapture->>UloopPausePointRegistry: Record formatted variables
UloopPausePointRegistry->>UloopPausePointSnapshot: Include captured variables and truncation
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointVariableFormatter.cs (1)
166-206: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider value equality for
UloopCapturedVariableto make tests robust.The DTO doesn't override
Equals/GetHashCode, soPausePointTestsline 394 (Is.EqualTo(capturedVariables)) passes only because the registry stores the same array reference. If the implementation ever copies the array or reconstructs DTOs, the test would silently break. MakingUloopCapturedVariablearecord(or addingIEquatable<UloopCapturedVariable>) would make value comparisons explicit and future-proof.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointVariableFormatter.cs` around lines 166 - 206, Make UloopCapturedVariable use value-based equality by converting it to a record or implementing IEquatable<UloopCapturedVariable> with matching Equals and GetHashCode behavior across all fields. Update its definition and preserve existing construction semantics so PausePointTests comparisons remain correct even when arrays or DTO instances are copied or reconstructed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointVariableFormatter.cs`:
- Around line 166-206: Make UloopCapturedVariable use value-based equality by
converting it to a record or implementing IEquatable<UloopCapturedVariable> with
matching Equals and GetHashCode behavior across all fields. Update its
definition and preserve existing construction semantics so PausePointTests
comparisons remain correct even when arrays or DTO instances are copied or
reconstructed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a8f4f284-6da5-469a-8266-eb45edb8337d
⛔ Files ignored due to path filters (11)
Assets/Tests/Editor/SourcePausePointCapture.metais excluded by none and included by noneAssets/Tests/Editor/SourcePausePointCapture/SourcePausePointCaptureTests.cs.metais excluded by none and included by noneAssets/Tests/Editor/SourcePausePointCapture/SourcePausePointVariableFormatterTests.cs.metais excluded by none and included by noneAssets/Tests/Editor/SourcePausePointCapture/UnityCLILoop.Tests.Editor.SourcePausePointCapture.asmdefis excluded by none and included by noneAssets/Tests/Editor/SourcePausePointCapture/UnityCLILoop.Tests.Editor.SourcePausePointCapture.asmdef.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointCapture.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointUnityObjectClassifier.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointVariableFormatter.cs.metais excluded by none and included by nonePackages/src/Runtime/PausePoints/UloopCapturedVariable.cs.metais excluded by none and included by nonePackages/src/Runtime/PausePoints/UloopCapturedVariableScope.cs.metais excluded by none and included by nonePackages/src/Runtime/PausePoints/UloopCapturedVariableUnityObjectKind.cs.metais excluded by none and included by none
📒 Files selected for processing (15)
Assets/Tests/Editor/PausePointTests.csAssets/Tests/Editor/SourcePausePointCapture/SourcePausePointCaptureTests.csAssets/Tests/Editor/SourcePausePointCapture/SourcePausePointVariableFormatterTests.csPackages/src/Editor/FirstPartyTools/PausePoint/AssemblyInfo.csPackages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointCapture.csPackages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointConstants.csPackages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointUnityObjectClassifier.csPackages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointVariableFormatter.csPackages/src/Runtime/PausePoints/AssemblyInfo.csPackages/src/Runtime/PausePoints/UloopCapturedVariable.csPackages/src/Runtime/PausePoints/UloopCapturedVariableScope.csPackages/src/Runtime/PausePoints/UloopCapturedVariableUnityObjectKind.csPackages/src/Runtime/PausePoints/UloopPausePointEntry.csPackages/src/Runtime/PausePoints/UloopPausePointRegistry.csPackages/src/Runtime/PausePoints/UloopPausePointSnapshot.cs
…fication coverage Addresses Fable 5 review of PR #1682: - Separate "count cap reached" (stops enumeration) from "a value was truncated" (continues); a single over-long value no longer drops every subsequent local/parameter/instance field. - Marshal off-main-thread Capture hits onto the main thread via MainThreadSwitcher, since EditorApplication.isPaused and the registry's bookkeeping are main-thread-only; make the registry's Entries dictionary concurrent so IsArmed stays safe to read cross-thread while inactive markers remain allocation-free. - Report a hoisted async parameter as Scope=Parameter instead of InstanceField, since it belongs to the compiler-generated state machine rather than the calling object's own class. - Add PrefabAsset/Asset/Component classification tests and a regression test proving over-long values don't truncate capture. - Translate a stray Japanese comment to English per repo convention.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
Assets/Tests/Editor/SourcePausePointCapture/SourcePausePointVariableFormatterTests.cs (2)
282-299: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHardcoded asmdef path is fragile.
The test loads
"Assets/Tests/Editor/SourcePausePointCapture/UnityCLILoop.Tests.Editor.SourcePausePointCapture.asmdef"by a hardcoded path. If the file or directory is renamed, the test fails with a non-obvious null-reference rather than a clear path-mismatch message. TheAssert.That(asset, Is.Not.Null, ...)guard mitigates this, so the risk is low.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Assets/Tests/Editor/SourcePausePointCapture/SourcePausePointVariableFormatterTests.cs` around lines 282 - 299, The test relies on a fragile hardcoded asmdef path in Format_WithPersistedAssetValue_ClassifiesAsAsset. Replace the literal with a path resolved from the test fixture or project asset metadata, and retain the explicit non-null assertion so missing or renamed assets produce a clear failure.
330-357: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSame fire-and-forget exception-swallowing concern as the capture test.
If
Formatthrows on the background thread,completedstaysfalse, the test times out, and the actual exception is lost. Consider capturing the task and assertingIsFaultedon timeout for a clearer failure message.♻️ Suggested refactor
- Task.Run(() => + Task formatTask = Task.Run(() => { (capturedVariables, _) = SourcePausePointVariableFormatter.Format(null, Array.Empty<object>(), locals); completed = true; }); float timeoutTime = Time.realtimeSinceStartup + 5f; while (!completed && Time.realtimeSinceStartup < timeoutTime) { yield return null; } + if (formatTask.IsFaulted) + { + Assert.Fail($"Background Format threw: {formatTask.Exception?.Flatten().InnerException?.Message}"); + } Assert.That(completed, Is.True, "background formatting should complete within timeout");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Assets/Tests/Editor/SourcePausePointCapture/SourcePausePointVariableFormatterTests.cs` around lines 330 - 357, Update Format_WhenCalledOffMainThread_DegradesUnityObjectValueWithoutEngineApiAccess to retain the Task returned by Task.Run and capture its exception state. On timeout or completion, assert the task is not faulted and surface its Exception in the failure, ensuring background Format failures are reported directly instead of appearing only as a timeout.Assets/Tests/Editor/SourcePausePointCapture/SourcePausePointCaptureTests.cs (1)
78-101: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFire-and-forget
Task.Runmay swallow background exceptions.If
SourcePausePointCapture.Capturethrows on the background thread (e.g., a future regression), the unobserved task exception won't fail the test — it'll be silently swallowed and the test will simply time out with an unhelpful "hit should be recorded" message. Consider capturing the task and asserting no fault on timeout.♻️ Suggested refactor to surface background exceptions
UloopPausePointRegistry.Enable("jump", 30); object[] locals = { "speed", 5 }; - Task.Run(() => SourcePausePointCapture.Capture("jump", null, Array.Empty<object>(), locals)); + Task captureTask = Task.Run(() => SourcePausePointCapture.Capture("jump", null, Array.Empty<object>(), locals)); float timeoutTime = Time.realtimeSinceStartup + 5f; UloopPausePointSnapshot snapshot = UloopPausePointRegistry.GetStatus("jump"); while (!snapshot.IsHit && Time.realtimeSinceStartup < timeoutTime) { yield return null; snapshot = UloopPausePointRegistry.GetStatus("jump"); } + if (captureTask.IsFaulted) + { + Assert.Fail($"Background Capture threw: {captureTask.Exception?.Flatten().InnerException?.Message}"); + } Assert.That(snapshot.IsHit, Is.True, "hit should be recorded on the main thread within timeout");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Assets/Tests/Editor/SourcePausePointCapture/SourcePausePointCaptureTests.cs` around lines 78 - 101, Capture the Task returned by Task.Run in Capture_WhenCalledOffMainThread_RecordsHitOnNextMainThreadTick instead of discarding it, and observe/assert its completion and exception state during the wait or before the final assertions. Ensure any background exception from SourcePausePointCapture.Capture is surfaced directly, particularly when the timeout expires, rather than producing only the generic hit assertion failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Assets/Tests/Editor/SourcePausePointCapture/SourcePausePointCaptureTests.cs`:
- Around line 78-101: Capture the Task returned by Task.Run in
Capture_WhenCalledOffMainThread_RecordsHitOnNextMainThreadTick instead of
discarding it, and observe/assert its completion and exception state during the
wait or before the final assertions. Ensure any background exception from
SourcePausePointCapture.Capture is surfaced directly, particularly when the
timeout expires, rather than producing only the generic hit assertion failure.
In
`@Assets/Tests/Editor/SourcePausePointCapture/SourcePausePointVariableFormatterTests.cs`:
- Around line 282-299: The test relies on a fragile hardcoded asmdef path in
Format_WithPersistedAssetValue_ClassifiesAsAsset. Replace the literal with a
path resolved from the test fixture or project asset metadata, and retain the
explicit non-null assertion so missing or renamed assets produce a clear
failure.
- Around line 330-357: Update
Format_WhenCalledOffMainThread_DegradesUnityObjectValueWithoutEngineApiAccess to
retain the Task returned by Task.Run and capture its exception state. On timeout
or completion, assert the task is not faulted and surface its Exception in the
failure, ensuring background Format failures are reported directly instead of
appearing only as a timeout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d0b860f8-9e4e-40be-b856-764ddbcd2c19
📒 Files selected for processing (6)
Assets/Tests/Editor/SourcePausePointCapture/SourcePausePointCaptureTests.csAssets/Tests/Editor/SourcePausePointCapture/SourcePausePointVariableFormatterTests.csPackages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointCapture.csPackages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointVariableFormatter.csPackages/src/Editor/ToolContracts/AssemblyInfo.csPackages/src/Runtime/PausePoints/UloopPausePointRegistry.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointCapture.cs
- Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointVariableFormatter.cs
This was referenced Jul 11, 2026
Merged
This was referenced Jul 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
wait-for-pause-point.UnityEngine.Objectreferences into scene/prefab/asset/runtime/destroyed categories so an AI agent can act on what it's looking at.User Impact
ToString()throws, and async/iterator methods whose locals get hoisted into compiler-generated state machine fields.Changes
UloopCapturedVariable,UloopCapturedVariableScope,UloopCapturedVariableUnityObjectKind(Runtime DTOs) andUloopPausePointEntry/UloopPausePointSnapshot/UloopPausePointRegistryextensions to carry captured variables through to a pause-point snapshot (HitWithCapturedVariables,IsArmed).SourcePausePointVariableFormatter(Editor): formats locals/parameters (in order) and instance fields, demangles Roslyn/mcs hoisted async-local fields (<name>5__N→ local) and follows the hoisted<>4__thisone level deep, de-dupes names already reported as Local/Parameter, degradesUnityEngine.Objectvalues to a placeholder off the main thread, and caps value length / variable count.SourcePausePointUnityObjectClassifier(Editor): classifies a liveUnityEngine.Objectinto SceneObject / PrefabAsset / Asset / RuntimeInstance.SourcePausePointCapture(Editor): the landing point a later Harmony patch will call — checksUloopPausePointRegistry.IsArmedfirst so an inactive marker costs a single dictionary lookup.Verification
dist/darwin-arm64/uloop compile— 0 errors, 0 warnings.uloop run-tests(assembly filters):UnityCLILoop.Tests.Editor.SourcePausePointCapture15/15 passed,UnityCLILoop.Tests.Editor.SourcePausePointResolver15/15 passed,PausePointTests(regex filter) 30/30 passed.