feat: Add --expect to pause-point-status and clarify capture diagnostics - #2017
Merged
hatayama merged 13 commits intoJul 26, 2026
Merged
Conversation
A hit can be inspected either by awaiting it or by querying it later, but only await-pause-point could assert on the captured values. Callers that queried an already-recorded hit had to re-read CapturedVariables by hand. pause-point-status now accepts --expect and reports Expectations / AllExpectationsPassed under the same field names await-pause-point uses, so one response shape works for both commands. Expectations are evaluated before the --captured-variable-names filter and the --captured-variables mode run, since those can narrow the response or strip values that an expectation targets. A failed expectation leaves the exit code at 0: whether the query succeeded and whether the state matched are separate questions. Also guards both runner-owned commands against help/parser drift: every flag their --help advertises is now asserted to be accepted by their own parser.
Passing an enable-pause-point flag to a query command used to be answered with "the installed project runner may be older than the docs — update the CLI". That hint fired exactly when it was false: the flag does exist in this build, just on another command. The one case it was written for (documentation ahead of the binary) is not reachable in practice, because the package and the runner are pinned together and each runner-owned command renders its own --help from the same table its parser reads. Unknown flags are now split in two. A flag another pause-point command accepts names that command, and an enable-time setting whose value Unity reports back on every later response also says it does not need to be passed again. A flag that exists nowhere is reported as a plain unknown option pointing at --help. The owning command is resolved through a fixed search order rather than map iteration, so a flag several commands accept always reports the same owner.
--captured-variable-names only ever showed the names that did match, so a typo in one of several names looked identical to a clean partial capture: the caller had to diff the flag value against the response by hand. CapturedVariableNamesNotFound now lists the requested names that matched no captured variable, in the order they were requested. CapturedVariableNameFilterNoMatch keeps its exact meaning and stays in place, so nothing that reads it changes; both fields appear when nothing matched at all. A name matched in any history frame counts as found, matching what the filter itself considers a match. The shared status-response contract fixture is deliberately left alone: like CapturedVariableNameFilterNoMatch and TriggerResult, this field is produced by the CLI, and the fixture pins the Unity-side contract only.
A simulate or record call made while a pause point holds PlayMode paused is refused before it does anything, and until now the only trace of which pause point caused that was inside the message text. Callers that need to tell "refused by the marker I am waiting on" from "refused for some other reason" had no option but to match on the wording — and PausePointId stays null here, because that field reports a marker hit *during* the call, not a refusal. Every response that shares this preflight — simulate-keyboard, simulate-mouse-input, simulate-mouse-ui, record-input and replay-input — now carries RejectedByActivePausePointId. The preflight returns its own result type instead of the general-purpose ValidationResult, which has no place to put an id and is used by argument validation that must not grow the field. The paused branches are unreachable from EditMode tests, since PlayMode never runs there, so the decision is split into Evaluate(isPlaying, isPaused, activePausePointId, ...) with the editor-state read left in the thin wrapper.
Both the Unity status response and the CLI's hit payload expose a field named Warning, and the CLI's outer field shadows the embedded Unity one. Any hit where the CLI had something to say therefore silently discarded Unity's enable-time diagnostic — the physics-callback dispatch warning among them. The outer Warning is now the join of Unity's text and the CLI's, the same treatment the enable path already gives it. The failed-log-fetch branch gained a Warning field for the same reason: it had none at all, so Unity's warning was the only one that could ever appear there, and only by accident of shadowing.
A marker armed on a line that runs every frame is hit before --trigger's input reaches Unity, so the trigger is refused for running while PlayMode is paused and injects nothing. The wait still reports Success:true and Status:Hit, with the refusal three levels down in TriggerResult.Response — where a reader looking at the top-level verdict never sees it, and the hit passes for input-driven evidence it is not. TriggerFailed is now promoted to the top level whenever a completed trigger reports Success:false or its dispatch failed, and a refusal by the very marker being awaited also produces a warning explaining how to arm the marker so the input is actually in effect. The id must match the awaited marker: a PlayMode paused by something else is a different problem. InterruptedByPausePoint is deliberately not part of the predicate. It marks the working case — the marker hit while the input was being applied — so an earlier draft that keyed on it warned about exactly the runs that behaved correctly. Known gap: a trigger that has not reported back within the join grace window has no known outcome, so it is neither warned about nor marked failed. Treating an unfinished trigger as failed would misreport every long-running hold.
Unity generated these after the test scripts were added; without them the scripts have no stable GUID for other checkouts.
The trigger diagnosis (TriggerFailed plus the refusal warning) was wired only into await-pause-point, while enable-pause-point --await builds its hit payload in a second place. A real run of the documented form `enable-pause-point --await --trigger "simulate-keyboard ..."` therefore still reported a plain success with no warning, which is the exact scenario the diagnosis exists for. Rather than duplicating the wiring, both hit paths now build their payload through one buildPausePointHitPayload helper, so a field added for one command cannot silently stay missing from the other. The warning join also drops repeats, because the same Unity text can reach the payload from both the enable response and the status poll that observed the hit.
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe change adds structured pause-point rejection metadata to PlayMode tool responses and tests, while extending the project runner with expectation reporting, captured-variable diagnostics, deterministic option ownership, trigger diagnosis, warning composition, and unified awaited pause-point payload construction. ChangesPlayMode rejection metadata
Pause-point CLI behavior
Estimated code review effort: 4 (Complex) | ~60 minutes 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 |
Self-review follow-up. A name passed twice in --captured-variable-names was listed twice in CapturedVariableNamesNotFound, reading as two separate missing variables; the list answers which names have no value, not how often each was asked for. Also pins what --expect reports for a marker that is armed but not yet hit, since that is the state a polling caller queries most.
The owner sentence hardcoded "is an <owner> ... not a <command> one", so a vowel-initial command in the second slot read as "not a await-pause-point one". One helper now decides the article for both slots from the command name.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@cli/project-runner/internal/projectrunner/pause_point_captured_variable_names_filter_test.go`:
- Around line 107-113: Strengthen the test case for
filterPausePointCapturedVariablesByName by asserting that the sole
CapturedVariableNamesNotFound entry is exactly "shield", in addition to
verifying its length is one.
🪄 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 Plus
Run ID: 9ad4ccf5-a0e7-422c-9e27-fb8e8b3c9e39
📒 Files selected for processing (5)
cli/project-runner/internal/projectrunner/pause_point_captured_variable_names_filter.gocli/project-runner/internal/projectrunner/pause_point_captured_variable_names_filter_test.gocli/project-runner/internal/projectrunner/pause_point_status_expect_test.gocli/project-runner/internal/projectrunner/pause_point_unknown_option.gocli/project-runner/internal/projectrunner/pause_point_unknown_option_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- cli/project-runner/internal/projectrunner/pause_point_captured_variable_names_filter.go
- cli/project-runner/internal/projectrunner/pause_point_unknown_option.go
- cli/project-runner/internal/projectrunner/pause_point_unknown_option_test.go
- cli/project-runner/internal/projectrunner/pause_point_status_expect_test.go
Comment on lines
+107
to
+113
| t.Run("a name requested twice is reported missing once", func(t *testing.T) { | ||
| result := filterPausePointCapturedVariablesByName( | ||
| baseResponse(), []string{"shield", "shield"}) | ||
| if len(result.CapturedVariableNamesNotFound) != 1 { | ||
| t.Fatalf("expected the repeated name once: %#v", result.CapturedVariableNamesNotFound) | ||
| } | ||
| }) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert the duplicated name, not only the count.
This would pass even if the function returned an incorrect single-element list. Assert that the value is exactly "shield".
Proposed assertion
- if len(result.CapturedVariableNamesNotFound) != 1 {
+ if len(result.CapturedVariableNamesNotFound) != 1 ||
+ result.CapturedVariableNamesNotFound[0] != "shield" {📝 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
| t.Run("a name requested twice is reported missing once", func(t *testing.T) { | |
| result := filterPausePointCapturedVariablesByName( | |
| baseResponse(), []string{"shield", "shield"}) | |
| if len(result.CapturedVariableNamesNotFound) != 1 { | |
| t.Fatalf("expected the repeated name once: %#v", result.CapturedVariableNamesNotFound) | |
| } | |
| }) | |
| t.Run("a name requested twice is reported missing once", func(t *testing.T) { | |
| result := filterPausePointCapturedVariablesByName( | |
| baseResponse(), []string{"shield", "shield"}) | |
| if len(result.CapturedVariableNamesNotFound) != 1 || | |
| result.CapturedVariableNamesNotFound[0] != "shield" { | |
| t.Fatalf("expected the repeated name once: %#v", result.CapturedVariableNamesNotFound) | |
| } | |
| }) |
🤖 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/project-runner/internal/projectrunner/pause_point_captured_variable_names_filter_test.go`
around lines 107 - 113, Strengthen the test case for
filterPausePointCapturedVariablesByName by asserting that the sole
CapturedVariableNamesNotFound entry is exactly "shield", in addition to
verifying its length is one.
SKILL.md still said --expect was not available on pause-point-status, which this branch makes false, and none of the new diagnostic fields were described. Covers --expect on pause-point-status (including what an unhit marker reports and that the verdict never moves the exit code), the top-level TriggerFailed and its warning, CapturedVariableNamesNotFound, and RejectedByActivePausePointId on a refused simulate call. Generated copies regenerated with skills install.
Both files stopped referencing any ToolContracts type when they moved to PlayModeToolPreflightResult; the test file's copy was already removed.
The five subtests added for CapturedVariableNamesNotFound pushed TestFilterPausePointCapturedVariablesByName to cyclomatic complexity 24 against the repository maximum of 15, so they move to their own test function sharing a hoisted fixture. The refusal test now also asserts the raw stdout contains "TriggerFailed": true, since every existing assertion goes through a Go struct and would survive a renamed json tag.
hatayama
merged commit Jul 26, 2026
d4fcf16
into
feature/cli-discoverability-integration
1 of 2 checks passed
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
pause-point-statusnow accepts--expect, so a marker's captured variables can be asserted from a plain status query, not only fromawait-pause-point.--triggerthat Unity refused to run is now reported at the top level of the response instead of being buried several levels deep, and misplaced pause-point flags now name the command that actually owns them.User Impact
Asserting captured variables (
--expect)--expectexisted only onawait-pause-point. Once a marker had already been hit, re-checking its variables meant readingCapturedVariablesby eye.pause-point-status --id <id> --expect "Name=value"returns the sameExpectations/AllExpectationsPassedfields the await path returns. Failing expectations do not change the exit code — whether the status query succeeded and whether the values matched stay separate.A trigger that never ran
--trigger "simulate-keyboard --action Press --key W"producedSuccess: true/Status: Hit, while the triggered command had in fact been refused by Unity for running against a paused PlayMode — no input reached the game at all. The refusal was only visible insideTriggerResult.Response, and its cause was only in a message string.TriggerFailed: truewhenever the trigger is known to have failed, plus a warning explaining that the marker was hit before the trigger ran, that this hit is not evidence about the trigger's input, and what to do instead (hold the key with a separateKeyDownbefore arming, or move the marker past the line where the input is applied). Unity's refusal now also carries the responsible pause point's id as a field, so the CLI can tell "refused by the marker I am waiting on" from "paused for some unrelated reason" without matching message text.await-pause-point --triggerandenable-pause-point --await --triggerreport this. They previously built their hit payloads in two separate places, so both now go through one shared builder — a field added for one command can no longer stay silently missing from the other.Misplaced and unknown options
await-pause-point/pause-point-statusclaimed the installed project runner might be older than the docs. For the misuse that actually happens —pause-point-status --max-preview-elements, a realenable-pause-pointoption — that hint pointed at the wrong cause entirely.--max-preview-elements is an enable-pause-point option, not a pause-point-status one.), and for options whoseenable-pause-pointvalue already carries into later responses it adds that the value does not need passing again. An option that exists nowhere is treated as a typo and just points at--help.Captured-variable names that matched nothing
--captured-variable-names "isHeld,Health"silently returned only what matched; a typo looked the same as a variable that was genuinely out of scope.CapturedVariableNamesNotFound, in request order. The existingCapturedVariableNameFilterNoMatchflag is unchanged.Changes
--expectparsing and evaluation forpause-point-status, evaluated before the name/mode filters can strip values; a status-side wrapper type carryingExpectations/AllExpectationsPassedunder the same names the await path uses.buildPausePointHitPayloadhelper now shapes both hit paths;TriggerFailedand the refusal diagnosis live in one place; the warning join drops empty and repeated entries, and no longer lets an outerWarningfield shadow — and discard — Unity's own text.PlayModeToolPreflightResultreturn type forPlayModeToolPreflightService(valid / message / refusing pause-point id), with all five tools that share this preflight — simulate-keyboard, simulate-mouse-input, simulate-mouse-ui, record-input, replay-input — returningRejectedByActivePausePointIdon a pause-point refusal. The generic argument-validation result type is untouched.Known limitation
When a marker is hit so fast that the triggered command's response misses the join grace window, the trigger reports neither success nor failure (
Completed: false, empty response). No diagnosis is emitted in that case: treating "unfinished" as failure would wrongly warn about long-running hold-style triggers.Verification
sh scripts/check-go-cli.sh— format, vet, lint (0 issues.for every module) and all Go tests pass.go test ./internal/projectrunner/ -run TestRunPausePointWaitAfterEnable -v— the three new enable-path tests execute and pass (verified in the-voutput, not inferred).uloop compile— ErrorCount 0, WarningCount 0.uloop run-tests(EditMode,PlayModeToolPreflight|PausePointRejectionResponseField) — 19/19 pass.enable-pause-point --file ... --line 16 --await --trigger "simulate-keyboard --action Press --key W"returnsStatus: Hit/Success: truewith top-levelTriggerFailed: true, the refusal warning, andTriggerResult.Response.RejectedByActivePausePointIdset to the awaited marker's id.pause-point-status --id <id> --expectpass/fail/not-found with exit code 0 alongside--captured-variables names;CapturedVariableNamesNotFoundlisting only the unmatched names; and the three unknown-option message shapes.SimulateMouseInputTests.Click_WhenPausePointMarkerHits_Should_ReturnMarkerDetailsandClick_WhenUnityPausesDuringObservation_Should_CompleteAsPausePointInterruption. Neither the mouse-input implementation nor its tests are touched by this PR.