fix(package): Stop tools from hanging when a pause point fires mid-command#1685
Conversation
The skill previously documented the hand-written UloopPausePoint.Pause("id")
marker workflow: edit source to add a marker, recompile, then enable the
pause point by id. That workflow is now superseded by source-based pause
points (enable-pause-point --file <path> --line <N>), which require no
source edit and no recompile, and work mid-PlayMode.
Rewrite the skill to document only the source-based workflow, plus:
- the new CapturedVariables snapshot response (locals, parameters, and
instance fields, UnityObject classification handles, truncation
semantics)
- the Debug code-optimization requirement
- domain-reload patch lifetime
- the Mono JIT inlining limitation
The generated copies under .claude/ and .agents/ were regenerated via
`uloop skills install --claude --agents` and are byte-identical to the
source (verified).
This is PR 6, commit 1 of the uloop Source Pause Point plan.
Source pause points and captured variables are new ubiquitous-language terms introduced by the file/line pause-point feature; the glossary needs entries so docs, commit messages, and reviews use them consistently with the existing pause-point marker terminology.
EditorApplication.update keeps ticking while paused, but Time.realtimeSinceStartup freezes, so every duration-based wait loop (overlay animations, LongPress hold, drag interpolation) spun forever once a Pause Point fired mid-command. Give MouseUiEditorFrameWaiter an explicit Paused outcome and thread it through every mouse UI action so a mid-command pause returns InterruptedByPausePoint (with the same PausePointId/PausePointHitCount/PausePointHits fields SimulateKeyboard and SimulateMouseInput already expose) instead of hanging the tool call.
The same captured-SynchronizationContext hang that hit simulate-mouse-ui was latent in nine other tool wrappers: their outermost `await useCase.ExecuteAsync(...)` still captured UnitySynchronizationContext, so any of them could hang forever if a Pause Point fired while the call was in flight. Add ConfigureAwait(false) to all of them and document the invariant once on the shared base class instead of per wrapper, so new tools inherit the rule from where they implement ExecuteAsync.
Investigating the mouse-ui hang needed a way to tell "main thread still ticking, tool genuinely running long" apart from a frozen Editor without native stack sampling. EditorMainThreadLivenessTracker already tracked this; wire its SecondsSinceLastMainThreadTick() into ServerBusyErrorData so BUSY responses carry it too, and cover the wire shape with a test.
…ontmatter The skill rewrite changed the wait-for-pause-point frontmatter description prefix from "Pauses Unity's playback" to "Pauses Unity playback", but SkillInstallLayoutTests still pinned the old prefix and failed.
…kill Pausing on a line that handles simulated input no longer risks hanging the command: simulate-* now returns promptly with InterruptedByPausePoint=true, so the Line Placement caution is replaced with the fixed behavior and when to still prefer post-consumption lines. Also add two enable-failure findings from E2E validation to Requirements & Safety: scripts under Packages/ resolve only by the package-id path form, and a "No sequence point found" error on every line means the assembly lacks debug sequence points.
📝 WalkthroughWalkthroughThe PR revises pause-point documentation, adds main-thread liveness diagnostics, prevents paused-editor continuation hangs, and propagates paused or timed-out frame outcomes through mouse UI simulation responses. ChangesPause-point guidance
Async execution and busy diagnostics
Mouse UI pause interruption handling
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SimulateMouseUi
participant MouseUiEditorFrameWaiter
participant MouseUiGestureExecutor
participant MouseUiSimulationResponseFactory
SimulateMouseUi->>MouseUiGestureExecutor: execute click or drag
MouseUiGestureExecutor->>MouseUiEditorFrameWaiter: wait for editor frame
MouseUiEditorFrameWaiter-->>MouseUiGestureExecutor: Completed, Paused, or TimedOut
MouseUiGestureExecutor->>MouseUiSimulationResponseFactory: create outcome response
MouseUiSimulationResponseFactory-->>SimulateMouseUi: return pause-point metadata
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/src/Editor/ToolContracts/UnityCliLoopTool.cs (1)
61-61: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPropagate
ConfigureAwait(false)through the delegated use cases.CompileUseCase,RunTestsUseCase, andExecuteDynamicCodeUseCasestill have plain awaits, so the deadlock risk extends beyond the tool wrapper.🤖 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/ToolContracts/UnityCliLoopTool.cs` at line 61, Update CompileUseCase, RunTestsUseCase, and ExecuteDynamicCodeUseCase so every await on their delegated asynchronous operations uses ConfigureAwait(false), propagating the context-free await behavior beyond the tool wrapper.
🤖 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.
Inline comments:
In
`@Packages/src/Editor/FirstPartyTools/SimulateMouseUi/MouseUiSimulationResponseFactory.cs`:
- Around line 148-196: Marshal pause-point registry access to the main thread
before invoking AttachPausePointHit from CreateInterruptedResult, ensuring both
GetLatestHitSnapshot and GetHitSnapshots are read on the owning thread. Preserve
the existing response construction and pause-point population behavior while
routing the continuation through the project’s standard main-thread dispatch
mechanism; do not make unsynchronized registry reads from background
continuations.
---
Outside diff comments:
In `@Packages/src/Editor/ToolContracts/UnityCliLoopTool.cs`:
- Line 61: Update CompileUseCase, RunTestsUseCase, and ExecuteDynamicCodeUseCase
so every await on their delegated asynchronous operations uses
ConfigureAwait(false), propagating the context-free await behavior beyond the
tool wrapper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f3c54c14-cc37-47e1-a5f9-3c327e630066
⛔ Files ignored due to path filters (1)
Packages/src/Editor/FirstPartyTools/SimulateMouseUi/UnityCLILoop.FirstPartyTools.SimulateMouseUi.Editor.asmdefis excluded by none and included by none
📒 Files selected for processing (28)
.agents/skills/uloop-wait-for-pause-point/SKILL.md.claude/skills/uloop-wait-for-pause-point/SKILL.mdAssets/Tests/Editor/JsonRpcResponseFactoryWireShapeCharacterizationTests.csAssets/Tests/Editor/SkillInstallLayoutTests.csPackages/src/Editor/CliOnlyTools~/PausePoint/Skill/SKILL.mdPackages/src/Editor/FirstPartyTools/Compile/CompileTool.csPackages/src/Editor/FirstPartyTools/ControlPlayMode/ControlPlayModeTool.csPackages/src/Editor/FirstPartyTools/FindGameObjects/FindGameObjectsTool.csPackages/src/Editor/FirstPartyTools/GetHierarchy/GetHierarchyTool.csPackages/src/Editor/FirstPartyTools/GetLogs/GetLogsTool.csPackages/src/Editor/FirstPartyTools/ReplayInput/ReplayInputTool.csPackages/src/Editor/FirstPartyTools/RunTests/RunTestsTool.csPackages/src/Editor/FirstPartyTools/SimulateKeyboard/SimulateKeyboardTool.csPackages/src/Editor/FirstPartyTools/SimulateMouseInput/SimulateMouseInputTool.csPackages/src/Editor/FirstPartyTools/SimulateMouseUi/MouseUiDragEventExecutor.csPackages/src/Editor/FirstPartyTools/SimulateMouseUi/MouseUiEditorFrameWaiter.csPackages/src/Editor/FirstPartyTools/SimulateMouseUi/MouseUiIncrementalDragExecutor.csPackages/src/Editor/FirstPartyTools/SimulateMouseUi/MouseUiOneShotDragExecutor.csPackages/src/Editor/FirstPartyTools/SimulateMouseUi/MouseUiOverlayAnimator.csPackages/src/Editor/FirstPartyTools/SimulateMouseUi/MouseUiPressActionExecutor.csPackages/src/Editor/FirstPartyTools/SimulateMouseUi/MouseUiSimulationResponseFactory.csPackages/src/Editor/FirstPartyTools/SimulateMouseUi/SimulateMouseUiResponse.csPackages/src/Editor/FirstPartyTools/SimulateMouseUi/SimulateMouseUiTool.csPackages/src/Editor/Infrastructure/Api/JsonRpcResponseFactory.csPackages/src/Editor/Infrastructure/Api/ServerBusyErrorData.csPackages/src/Editor/ToolContracts/UnityCliLoopTool.csPackages/src/Runtime/PausePoints/AssemblyInfo.csdocs/glossary.md
| private static void AttachPausePointHit(SimulateMouseUiResponse result) | ||
| { | ||
| if (result == null) | ||
| { | ||
| Debug.Assert(false, "result must not be null"); | ||
| return; | ||
| } | ||
|
|
||
| UloopPausePointSnapshot? snapshot = UloopPausePointRegistry.GetLatestHitSnapshot(); | ||
| if (snapshot == null) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| if (!snapshot.IsHit) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| string? snapshotId = snapshot.Id; | ||
| if (string.IsNullOrEmpty(snapshotId)) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| result.PausePointId = snapshotId; | ||
| result.PausePointHitCount = snapshot.HitCount; | ||
| result.PausePointHits = CollectPausePointHits(); | ||
| } | ||
|
|
||
| // One input can hit several markers in the same frame; the representative | ||
| // PausePointId alone forced agents into extra status calls to find the others. | ||
| private static List<UnityCliLoopPausePointHit> CollectPausePointHits() | ||
| { | ||
| List<UnityCliLoopPausePointHit> hits = new(); | ||
| foreach (UloopPausePointSnapshot snapshot in UloopPausePointRegistry.GetHitSnapshots()) | ||
| { | ||
| if (!snapshot.IsHit || string.IsNullOrEmpty(snapshot.Id)) | ||
| { | ||
| continue; | ||
| } | ||
| hits.Add(new UnityCliLoopPausePointHit | ||
| { | ||
| Id = snapshot.Id, | ||
| HitCount = snapshot.HitCount | ||
| }); | ||
| } | ||
| return hits; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the registry and inspect snapshot accessors + backing storage for thread-safety.
fd -tf 'UloopPausePointRegistry*.cs' --exec sh -c 'echo "== {} =="; cat -n "{}"'
rg -nP --type=cs -C3 'GetLatestHitSnapshot|GetHitSnapshots|class UloopPausePointRegistry'Repository: hatayama/unity-cli-loop
Length of output: 11600
🏁 Script executed:
#!/bin/bash
# Inspect the call flow around CreateInterruptedResult and any thread-affinity assumptions.
fd -tf 'MouseUiSimulationResponseFactory.cs' --exec sh -c 'echo "== {} =="; sed -n "1,260p" "{}"'
rg -n --type=cs 'CreateInterruptedResult|ConfigureAwait\(false\)|SwitchToMainThread|GetLatestHitSnapshot|GetHitSnapshots' Packages/src/Editor/FirstPartyTools/SimulateMouseUi Packages/src/Runtime/PausePointsRepository: hatayama/unity-cli-loop
Length of output: 21962
🏁 Script executed:
#!/bin/bash
# Inspect the awaited helpers and the paused branches that call CreateInterruptedResult.
sed -n '1,220p' Packages/src/Editor/FirstPartyTools/SimulateMouseUi/MouseUiOverlayAnimator.cs
printf '\n====\n'
sed -n '1,220p' Packages/src/Editor/FirstPartyTools/SimulateMouseUi/MouseUiEditorFrameWaiter.cs
printf '\n====\n'
sed -n '1,240p' Packages/src/Editor/FirstPartyTools/SimulateMouseUi/MouseUiPressActionExecutor.cs
printf '\n====\n'
sed -n '1,240p' Packages/src/Editor/FirstPartyTools/SimulateMouseUi/MouseUiOneShotDragExecutor.csRepository: hatayama/unity-cli-loop
Length of output: 27304
Marshal pause-point snapshot reads back to the main thread CreateInterruptedResult can resume on a non-main-thread continuation here, but UloopPausePointRegistry keeps _latestHitSnapshot and _hitSnapshots as main-thread-only state. Switch threads before calling AttachPausePointHit, or make those registry reads synchronized.
🤖 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/SimulateMouseUi/MouseUiSimulationResponseFactory.cs`
around lines 148 - 196, Marshal pause-point registry access to the main thread
before invoking AttachPausePointHit from CreateInterruptedResult, ensuring both
GetLatestHitSnapshot and GetHitSnapshots are read on the owning thread. Preserve
the existing response construction and pause-point population behavior while
routing the continuation through the project’s standard main-thread dispatch
mechanism; do not make unsynchronized registry reads from background
continuations.
There was a problem hiding this comment.
AttachPausePointHit is only reachable from CreateInterruptedResult, which is only called inside MouseUiFrameWaitOutcome.Paused branches (e.g. MouseUiPressActionExecutor.cs:59-65,77-83,141-147,166-173,195-201, and the equivalent branches in the drag executors) — the branch-to-factory-call path has no further await, it's synchronous.
MouseUiEditorFrameWaiter only produces Paused in two places (MouseUiEditorFrameWaiter.cs:35-38,49-53): the entry check (which runs on whatever thread the previous loop iteration's MainThreadSwitcher.SwitchToMainThread call left us on) or the check immediately after MainThreadSwitcher.SwitchToMainThread. So a Paused return always happens on the main thread.
The ConfigureAwait(false) here only opts the continuation out of being posted to UnitySynchronizationContext — it does not force a thread hop. Since the awaited operation is already on the main thread when it completes (per the above), the continuation runs inline on the main thread, not on a threadpool thread.
On the write side, UloopPausePointRegistry's hit-recording (HitCore) is likewise main-thread-only — the Harmony patch target is Play Mode game code, and off-thread hits are already marshaled to the main thread before reaching the registry (established in an earlier PR in this series).
So reads and writes of _hitSnapshots/_latestHitSnapshot are serialized on the main thread by construction; there's no race here. This is the same invariant the shipped simulate-keyboard/simulate-mouse-input response factories already rely on (KeyboardInputSimulationResponseFactory.AttachPausePointHit follows the identical pattern).
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
sed -n '1,80p' Packages/src/Editor/FirstPartyTools/SimulateMouseUi/MouseUiEditorFrameWaiter.cs
echo "===MainThreadSwitcher==="
fd -tf 'MainThreadSwitcher*.cs' --exec sh -c 'echo "== {} =="; cat -n "{}"'
echo "===Registry HitCore==="
fd -tf 'UloopPausePointRegistry*.cs' --exec sh -c 'echo "== {} =="; cat -n "{}"'
echo "===Keyboard factory pattern==="
fd -tf 'KeyboardInputSimulationResponseFactory*.cs' --exec sh -c 'echo "== {} =="; cat -n "{}"'Length of output: 26161
Confirmed — the trace holds up: MouseUiEditorFrameWaiter only returns Paused from the pre-check or right after await MainThreadSwitcher.SwitchToMainThread(ct), both of which are main-thread-only completions, and SwitchToMainThreadAwaitable.Awaiter.OnCompleted inlines the continuation synchronously when already on the main thread. ConfigureAwait(false) on the earlier EditorFrameWaiter.WaitFramesOrTimeoutAsync await doesn't change that, since it only affects whether SynchronizationContext.Post is used, not which thread the continuation runs on. Combined with HitCore being main-thread-only on the write side, _latestHitSnapshot/_hitSnapshots reads and writes are indeed serialized on the main thread here. Withdrawing the concern — thanks for the detailed walkthrough.
🧬 Code Graph Analysis Results
Packages/src/Editor/FirstPartyTools/SimulateMouseUi/SimulateMouseUiResponse.cs (relevant snippet)
Lines 12-30
public class SimulateMouseUiResponse : UnityCliLoopToolResponse
{
public bool Success { get; set; }
public string Message { get; set; } = "";
public string Action { get; set; } = "";
public string? HitGameObjectName { get; set; }
public float PositionX { get; set; }
public float PositionY { get; set; }
public float? EndPositionX { get; set; }
public float? EndPositionY { get; set; }
public bool InterruptedByPausePoint { get; set; }
public string? PausePointId { get; set; }
public int? PausePointHitCount { get; set; }
public List<UnityCliLoopPausePointHit>? PausePointHits { get; set; }
public SimulateMouseUiResponse()
{
}
}✏️ Learnings added
Learnt from: hatayama
Repo: hatayama/unity-cli-loop PR: 1685
File: Packages/src/Editor/FirstPartyTools/SimulateMouseUi/MouseUiSimulationResponseFactory.cs:148-196
Timestamp: 2026-07-11T06:59:48.161Z
Learning: In the unity-cli-loop repo, `MouseUiEditorFrameWaiter.WaitForEditorFrameAndSwitchToMainThreadAsync` (Packages/src/Editor/FirstPartyTools/SimulateMouseUi/MouseUiEditorFrameWaiter.cs) only returns `MouseUiFrameWaitOutcome.Paused` from two points: the entry check before any await, or immediately after `await MainThreadSwitcher.SwitchToMainThread(ct)`. `MainThreadSwitcher.SwitchToMainThreadAwaitable.Awaiter.OnCompleted` (Packages/src/Editor/ToolContracts/MainThreadSwitcher.cs) inlines the continuation synchronously if already on the main thread, so a `Paused` outcome is always observed on the main thread. `ConfigureAwait(false)` on the earlier `EditorFrameWaiter.WaitFramesOrTimeoutAsync` await only opts out of `SynchronizationContext.Post`; it does not force a thread hop, so if that await completes on the main thread the continuation still runs there. Therefore, code paths that reach `MouseUiSimulationResponseFactory.CreateInterruptedResult`/`AttachPausePointHit` (and the analogous `KeyboardInputSimulationResponseFactory.AttachPausePointHit`) from a `Paused` branch are guaranteed to run on the main thread, matching `UloopPausePointRegistry`'s main-thread-only invariant for `_latestHitSnapshot`/`_hitSnapshots` (writes happen in `HitCore`, which is only reached via the main-thread-marshaled Harmony patch path). No synchronization is needed for these registry reads in this pattern.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
Re: the outside-diff comment about propagating
Filed as #1686 with the affected lines, the mechanism, the "don't just add ConfigureAwait(false)" warning, candidate fixes, and verification requirements. Left the |
hatayama
dismissed
coderabbitai[bot]’s stale review
Both points addressed: (1) replied with the structural main-thread-serialization argument for AttachPausePointHit — see inline reply; (2) CompileUseCase/RunTestsUseCase plain-await gap confirmed valid but tracked as follow-up #1686 with a warning against a mechanical ConfigureAwait(false) fix, since it would trade a rare hang for a frequent Unity-API-off-main-thread bug; ExecuteDynamicCodeUseCase confirmed a false positive. Dismissing to unblock merge; CI is fully green and Fable 5 (final reviewer for this workflow) is doing the human review pass.
hatayama
changed the title
1 participant
Summary
uloop-wait-for-pause-pointskill to describe the source-based (file:line) pause point workflow, replacing stale wording left over from the old debugger-style approach.simulate-mouse-ui) hung forever if a pause point fired while the tool call was still in flight.User Impact
InterruptedByPausePoint: trueand identifies which pause point(s) fired, so callers can tell "the click landed, only the follow-up animation was cut short" apart from "the click never reached the target."Changes
MouseUiEditorFrameWaitergains an explicitPausedoutcome (distinct fromCompleted/TimedOut), sinceTime.realtimeSinceStartupfreezes while Play Mode is paused butEditorApplication.updatekeeps ticking — duration-based wait loops (overlay animations,LongPresshold, drag interpolation) would otherwise spin forever.simulate-mouse-uiactions (Click, LongPress, Drag, DragStart, DragMove, DragEnd) now reportInterruptedByPausePoint/PausePointId/PausePointHitCount/PausePointHitson the same pattern already used bysimulate-keyboard/simulate-mouse-input.await useCase.ExecuteAsync(...)in several tool wrappers lackedConfigureAwait(false), so it captured Unity'sSynchronizationContext. That context's pump is tied to the player loop and does not run while paused, so a continuation posted back to it after a pause-triggered completion never executed. AddedConfigureAwait(false)to all affected wrappers (ControlPlayMode,Compile,GetLogs,GetHierarchy,FindGameObjects,ReplayInput,RunTests,SimulateMouseInput,SimulateKeyboard,SimulateMouseUi) and documented the invariant once on the sharedUnityCliLoopTool.ExecuteAsyncbase method.EditorMainThreadLivenessTrackerintoServerBusyErrorData/JsonRpcResponseFactoryso BUSY responses includesecondsSinceLastMainThreadTick.Packages/, and "No sequence point found" meaning an assembly was built without sequence points).Verification
uloop compile: 0 errors / 0 warnings.uloop run-tests(full unfiltered suite): all tests passing, including the updatedSkillInstallLayoutTestsexpectation.simulate-mouse-ui --action Click), pause point hit mid-command:InterruptedByPausePoint: true, ~0.17s response, Editor fully responsive afterward (control-play-mode --action Stepin ~0.07s, no BUSY, no Editor restart needed). Repeated after all three fix commits landed as a final check against the diagnostic-instrumentation removal.simulate-keyboard --action Press), pause point on a key-guarded line hit while the press was already in flight (not before it started):InterruptedByPausePoint: true,PressEdgeObserved: true, ~0.39s response, Editor fully responsive afterward.