feat: Enable pause points by source file and line#1684
Merged
Conversation
Add File/Line parameters to enable-pause-point alongside the existing Id marker path. PausePointUseCase.Enable now validates that exactly one of Id or File+Line is provided, rejects the File/Line path when CompilationPipeline.codeOptimization is Release (Debug symbols are required to resolve a patch location), resolves File/Line via SourcePausePointResolver, patches the resolved method via SourcePausePointPatcher, derives the pause point id as "<file>:<line>" from the originally requested line (not the resolved/rounded line) so repeated calls at the same location stay idempotent, and returns ResolvedLine/ResolvedMethod plus any Patcher warning merged with the existing domain-reload warning. cli/common/tools/default-tools.json is updated in the same commit so DefaultToolsCatalogDriftTests keeps passing. A new frozen fixture file backs a full Resolver -> Patcher -> Registry integration test driven through the public tool surface. This is commit 1 of PR 5 in the source pause point plan; Clear() is intentionally unchanged here (Unpatch-on-clear wiring is commit 2).
Wire SourcePausePointPatcher.Unpatch/UnpatchAll into both clear paths so a source pause point's Harmony injection is actually removed, not just its registry entry: - PausePointUseCase.Clear now calls Unpatch(id) for a specific id and UnpatchAll() for --all (a safe no-op for marker-only ids that were never Harmony-patched). - PausePointStatusBridgeCommand.Clear (the path Go's wait-for-pause-point timeout auto-clear and clear-pause-point-status hit) now calls Unpatch(id) too, so a source pause point left armed past its timeout does not leave a dangling injection after Unity reports it Cleared. Infrastructure needs a new reference to the PausePoint editor assembly plus an InternalsVisibleTo grant to call the internal SourcePausePointPatcher; Tests.Editor gets the same grant so PausePointTests can prove Unpatch actually ran (re-Patch-ing the same id with a deliberately stale Mvid only reaches the Patcher's stale-assembly gate once the id is no longer in its ledger, since an already-patched id short-circuits before that gate runs).
The previous commit (feat(package): Unpatch source pause points on clear) wired SourcePausePointPatcher.Unpatch/UnpatchAll into PausePointStatusBridgeCommand.Clear by adding a direct asmdef reference and InternalsVisibleTo grant from the Infrastructure asmdef to the FirstPartyTools.PausePoint.Editor asmdef. Running the full Unity EditMode suite revealed this broke OnionAssemblyDependencyTests.InfrastructureAsmdef_WhenLoaded_DependsOnApplicationRuntimeAndDoesNotReferencePresentation, which asserts Infrastructure's asmdef reference set is a fixed, closed list that excludes individual FirstPartyTools implementation asmdefs; Infrastructure may only depend on the shared PausePointsRuntime assembly, InternalApiBridge, Application, Domain, and ToolContracts. Fix the root cause instead of adjusting the test: move the unpatch wiring out of Infrastructure entirely by adding an Action<string> OnCleared and an Action OnClearedAll hook to UloopPausePointRegistry (Runtime assembly, already an allowed Infrastructure dependency). SourcePausePointPatcher's static constructor wires Unpatch/UnpatchAll into these hooks; Registry.Clear/ClearAll invoke them internally. Both PausePointUseCase.Clear and PausePointStatusBridgeCommand.Clear go back to calling only UloopPausePointRegistry.Clear/ClearAll, with neither referencing SourcePausePointPatcher directly. The now-unneeded Infrastructure asmdef reference and InternalsVisibleTo grant are removed.
Enabling a pause point by File/Line already captures local variables, parameters, and this fields into the registry snapshot, but the CLI's wait-for-pause-point/pause-point-status commands only surfaced marker bookkeeping fields (Status, IsHit, EditorState, etc.). A user hitting a source pause point had no way to see what values were actually captured. Add CapturedVariables and CapturedVariablesTruncated to both the C# PausePointStatusResponse (Infrastructure bridge) and the Go pausePointStatusResponse, mirroring UloopCapturedVariable's fields field-for-field so the Go CLI passes Unity's captured values straight through to its JSON stdout. normalizePausePointStatusResponse is left untouched since it only derives Expired/RemainingMilliseconds for older Unity packages.
Extends the get-logs contract pattern to the pause-point status response so the CapturedVariables/CapturedVariablesTruncated fields added for source file:line pause points stay in sync between the Go CLI and the Unity package: a shared JSON fixture plus a Go test and a C# test that each round-trip it through their own DTO and compare the normalized field shape.
Contributor
📝 WalkthroughWalkthroughPause points can now be enabled by source file and line, automatically unpatched when cleared, and reported with captured-variable metadata. Unity and Go tests validate the new behavior against a shared JSON response contract. ChangesPause-point pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant EnablePausePointTool
participant SourcePausePointResolver
participant SourcePausePointPatcher
participant UloopPausePointRegistry
CLI->>EnablePausePointTool: Send File and Line
EnablePausePointTool->>SourcePausePointResolver: Resolve source location
SourcePausePointResolver-->>EnablePausePointTool: Return method and line
EnablePausePointTool->>SourcePausePointPatcher: Patch resolved method
SourcePausePointPatcher-->>EnablePausePointTool: Return patch result
EnablePausePointTool->>UloopPausePointRegistry: Register pause point
UloopPausePointRegistry-->>SourcePausePointPatcher: Unpatch on clear
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/src/Runtime/PausePoints/UloopPausePointRegistry.cs (1)
217-226: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
ResetForTestsshould triggerOnClearedAllto unpatch stale source pause points.
ResetForTestsclearsEntriesand snapshots but never invokesOnClearedAll, so Harmony patches from source pause points enabled in a previous test survive the reset. Subsequent tests that exercise the patched method will hit stale injected IL. CallingOnClearedAll?.Invoke()before clearing entries ensures the patcher'sUnpatchAllruns and removes all transpilers.🛡️ Proposed fix
public static void ResetForTests() { + OnClearedAll?.Invoke(); Entries.Clear(); _nextGeneration = 0;🤖 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/Runtime/PausePoints/UloopPausePointRegistry.cs` around lines 217 - 226, ResetForTests currently clears registry state without removing Harmony patches from previously registered source pause points. Invoke OnClearedAll before clearing Entries so the patcher’s UnpatchAll logic runs, then continue resetting counters, snapshots, and providers as before.
🤖 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.
Outside diff comments:
In `@Packages/src/Runtime/PausePoints/UloopPausePointRegistry.cs`:
- Around line 217-226: ResetForTests currently clears registry state without
removing Harmony patches from previously registered source pause points. Invoke
OnClearedAll before clearing Entries so the patcher’s UnpatchAll logic runs,
then continue resetting counters, snapshots, and providers as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 346a263d-f592-400f-b37a-bf5ee994702e
⛔ Files ignored due to path filters (2)
Assets/Tests/Editor/PausePointStatusResponseContractTests.cs.metais excluded by none and included by noneAssets/Tests/Editor/PausePointToolsFixture.cs.metais excluded by none and included by none
📒 Files selected for processing (14)
Assets/Tests/Editor/PausePointStatusResponseContractTests.csAssets/Tests/Editor/PausePointTests.csAssets/Tests/Editor/PausePointToolsFixture.csPackages/src/Editor/FirstPartyTools/PausePoint/AssemblyInfo.csPackages/src/Editor/FirstPartyTools/PausePoint/PausePointTools.csPackages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointConstants.csPackages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointPatcher.csPackages/src/Editor/Infrastructure/Api/PausePointStatusBridgeCommand.csPackages/src/Runtime/PausePoints/UloopPausePointRegistry.cscli/common/tools/default-tools.jsoncli/project-runner/internal/projectrunner/pause_point_status_response_contract_test.gocli/project-runner/internal/projectrunner/pause_point_wait.gocli/project-runner/internal/projectrunner/pause_point_wait_test.gotests/contracts/pause_point_status_response_contract.json
3 tasks
Merged
This was referenced Jul 12, 2026
This was referenced Jul 19, 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
enable-pause-pointnow acceptsFile+Linein addition to the existingIdmarker mode, resolving a source location and patching a pause point into it without any source edit.ClearAll) automatically removes the underlying Harmony patch, so no injected instrumentation is left behind after the marker reports Cleared.CapturedVariables(locals, parameters, and instance fields captured at that line) and aCapturedVariablesTruncatedflag.User Impact
enable-pause-point --file Assets/Scripts/Enemy.cs --line 42) instead of requiring a hand-writtenUloopPausePoint.Pause("id")marker in code.wait-for-pause-point) now surfaces captured variable values directly in the response, so agents can inspect state at the pause point without a separate step.Changes
PausePointTools.EnablePausePointSchema/PausePointUseCase.Enable: validates that exactly one ofIdorFile+Lineis provided, rejects Release code optimization for the source-line path, resolves the location via the existing Resolver, patches viaSourcePausePointPatcher, and merges any resolver/patcher warning with the existing enable warning.UloopPausePointRegistrygainsOnCleared/OnClearedAllhooks invoked fromClear/ClearAll;SourcePausePointPatcher"'s static constructor wiresUnpatch/UnpatchAll` into these hooks. This keeps the onion-architecture dependency direction intact: the Runtime registry and Infrastructure bridge never reference the Editor-only patching assembly directly.PausePointStatusBridgeCommand/Gopause_point_wait.go: both response DTOs gainCapturedVariables/CapturedVariablesTruncated, backed by a new shared JSON contract fixture (tests/contracts/pause_point_status_response_contract.json) with matching Go and C# field-shape tests.default-tools.json:enable-pause-point's schema documents the newFile/Lineproperties as mutually exclusive withId.Verification
uloop compile— 0 errors, 0 warningsuloop run-tests --filter-value "PausePoint"— 98/98 passedscripts/check-go-cli.sh— all green (fmt, vet, lint, tests, build)