feat: Pause point waits now explain their evidence#1338
Merged
Conversation
Group editor state, hit metadata, and matching log counts so E2E checks can distinguish clean single-hit evidence from stale, truncated, or repeated evidence. - Move pause point play state into EditorState with capture timing - Track hit timestamps and sequence numbers for pause point hits - Add EvidenceSummary to wait-for-pause-point results and bump the IPC protocol
Keep the checked-in Claude and Agents skill copies aligned with the updated pause point evidence contract so agents see EvidenceSummary and EditorState guidance.
Contributor
|
Warning Review limit reached
More reviews will be available in 13 minutes and 9 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR replaces flat ChangesEditorState restructuring and evidence summary
Sequence Diagram(s)sequenceDiagram
participant Caller
participant pausePointWait as pausePointWait (Go CLI)
participant Unity as Unity IPC
participant fetchLogs as fetchMatchingLogsFromUnity
participant buildEvidence as buildPausePointEvidenceSummary
Caller->>pausePointWait: wait-for-pause-point
loop Poll until hit or timeout
pausePointWait->>Unity: status request
Unity-->>pausePointWait: pausePointStatusResponse{EditorState{IsPlaying, IsPaused, CapturedAt}, FirstHitAtUtc, LastHitAtUtc, FirstHitSequence, LastHitSequence}
end
pausePointWait->>fetchLogs: fetchMatchingLogsFromUnity
fetchLogs-->>pausePointWait: pausePointMatchingLogsResult{TotalCount, DisplayedCount, Logs}
pausePointWait->>buildEvidence: buildPausePointEvidenceSummary(response, logsResult)
buildEvidence-->>pausePointWait: pausePointEvidenceSummary{PausePoint, MatchingLogs, Warning}
alt hit
pausePointWait-->>Caller: pausePointWaitResult{EvidenceSummary, MatchingLogs}
else timeout / expired
pausePointWait-->>Caller: error envelope{Details: {EvidenceSummary, editorState}}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 (2)
cli/internal/cli/pause_point_logs.go (1)
165-184: 💤 Low valueMinor redundant computation (optional optimization).
buildPausePointEvidenceWarninginternally callsbuildPausePointEvidenceLogs(line 166) to derive the boolean flags, but the caller (buildPausePointEvidenceSummaryat line 144) has already built the samepausePointEvidenceLogsstruct. You could refactor the warning builder to accept the pre-built evidence logs instead of rebuilding them.Current pattern is clear and self-contained, so this is purely a micro-optimization opportunity.
♻️ Optional refactor to eliminate redundant computation
-func buildPausePointEvidenceWarning(logs pausePointMatchingLogsResult, hitCount int) string { - evidenceLogs := buildPausePointEvidenceLogs(logs) +func buildPausePointEvidenceWarning(evidenceLogs pausePointEvidenceLogs, hitCount int) string { warnings := []string{} if evidenceLogs.MayBeTruncated {Then update the caller at line 145:
+ evidenceLogs := buildPausePointEvidenceLogs(logs) return pausePointEvidenceSummary{ EditorState: response.EditorState, PausePoint: pausePointEvidencePausePoint{ Id: response.Id, Status: response.Status, Generation: response.Generation, HitCount: response.HitCount, MultipleHitsObserved: response.HitCount > 1, FirstHitAtUtc: response.FirstHitAtUtc, LastHitAtUtc: response.LastHitAtUtc, FirstHitSequence: response.FirstHitSequence, LastHitSequence: response.LastHitSequence, }, - MatchingLogs: buildPausePointEvidenceLogs(logs), - Warning: buildPausePointEvidenceWarning(logs, response.HitCount), + MatchingLogs: evidenceLogs, + Warning: buildPausePointEvidenceWarning(evidenceLogs, response.HitCount), }🤖 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 `@cli/internal/cli/pause_point_logs.go` around lines 165 - 184, The buildPausePointEvidenceWarning function is redundantly calling buildPausePointEvidenceLogs to derive the boolean flags, but the caller buildPausePointEvidenceSummary has already built this same pausePointEvidenceLogs struct. Refactor buildPausePointEvidenceWarning to accept the pre-built pausePointEvidenceLogs as a parameter instead of accepting the raw logs and rebuilding it internally. Then update the call to buildPausePointEvidenceWarning in buildPausePointEvidenceSummary to pass the already-constructed evidence logs instead of passing the raw logs, eliminating the redundant computation.Packages/src/Editor/Infrastructure/Api/PausePointStatusBridgeCommand.cs (1)
106-118: ⚡ Quick winUse contract-style assertions in the new editor-state mappers.
These two internal mapping helpers treat
nullsnapshots as runtime argument errors, but in this Editor codebase that case is a programmer-contract violation rather than recoverable input.
Packages/src/Editor/Infrastructure/Api/PausePointStatusBridgeCommand.cs#L106-L118: replace theArgumentNullExceptionbranch inPausePointStatusEditorState.FromSnapshot(...)with aDebug.Assert(snapshot != null, ...)and keep the mapper as a straight projection.Packages/src/Editor/FirstPartyTools/PausePoint/PausePointTools.cs#L115-L127: apply the same assert-based precondition inPausePointEditorState.FromSnapshot(...)so both new mappers follow the same fail-fast contract style.Based on learnings, internal Editor-side preconditions in this repository are expected to use
Debug.Assertfor fail-fast contract programming rather than runtime exceptions.🤖 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/Infrastructure/Api/PausePointStatusBridgeCommand.cs` around lines 106 - 118, Replace the runtime argument validation with contract-style assertions in both internal snapshot mapper methods. At Packages/src/Editor/Infrastructure/Api/PausePointStatusBridgeCommand.cs lines 106-118, remove the null check that throws ArgumentNullException in the PausePointStatusEditorState.FromSnapshot method and replace it with Debug.Assert(snapshot != null, ...) to enforce the contract as a fail-fast assertion rather than a runtime exception. Apply the identical change at Packages/src/Editor/FirstPartyTools/PausePoint/PausePointTools.cs lines 115-127 in the PausePointEditorState.FromSnapshot method to ensure both mappers follow consistent contract-style precondition checking using Debug.Assert for programmer violations rather than recoverable input errors.Source: Learnings
🤖 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 `@cli/internal/cli/pause_point_logs.go`:
- Around line 165-184: The buildPausePointEvidenceWarning function is
redundantly calling buildPausePointEvidenceLogs to derive the boolean flags, but
the caller buildPausePointEvidenceSummary has already built this same
pausePointEvidenceLogs struct. Refactor buildPausePointEvidenceWarning to accept
the pre-built pausePointEvidenceLogs as a parameter instead of accepting the raw
logs and rebuilding it internally. Then update the call to
buildPausePointEvidenceWarning in buildPausePointEvidenceSummary to pass the
already-constructed evidence logs instead of passing the raw logs, eliminating
the redundant computation.
In `@Packages/src/Editor/Infrastructure/Api/PausePointStatusBridgeCommand.cs`:
- Around line 106-118: Replace the runtime argument validation with
contract-style assertions in both internal snapshot mapper methods. At
Packages/src/Editor/Infrastructure/Api/PausePointStatusBridgeCommand.cs lines
106-118, remove the null check that throws ArgumentNullException in the
PausePointStatusEditorState.FromSnapshot method and replace it with
Debug.Assert(snapshot != null, ...) to enforce the contract as a fail-fast
assertion rather than a runtime exception. Apply the identical change at
Packages/src/Editor/FirstPartyTools/PausePoint/PausePointTools.cs lines 115-127
in the PausePointEditorState.FromSnapshot method to ensure both mappers follow
consistent contract-style precondition checking using Debug.Assert for
programmer violations rather than recoverable input errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 521c5356-17b5-4067-af30-ff26cfd3b297
📒 Files selected for processing (13)
.agents/skills/uloop-wait-for-pause-point/SKILL.md.claude/skills/uloop-wait-for-pause-point/SKILL.mdAssets/Tests/Editor/PausePointTests.csPackages/src/Editor/CliOnlyTools~/PausePoint/Skill/SKILL.mdPackages/src/Editor/Domain/CliConstants.csPackages/src/Editor/FirstPartyTools/PausePoint/PausePointTools.csPackages/src/Editor/Infrastructure/Api/PausePointStatusBridgeCommand.csPackages/src/Runtime/PausePoints/UloopPausePointRegistry.cscli/contract.jsoncli/internal/cli/pause_point_errors.gocli/internal/cli/pause_point_logs.gocli/internal/cli/pause_point_wait.gocli/internal/cli/pause_point_wait_test.go
There was a problem hiding this comment.
2 issues found across 13 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Use observed wording for matching-log counts that may exceed returned rows, and limit skill guidance for timeout evidence summaries to the error that actually includes them.
Merged
This was referenced Jun 16, 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
User Impact
Changes
Verification