fix: Fail fast without clearing the marker when a pause-point trigger rejects its arguments - #2014
Merged
hatayama merged 2 commits intoJul 26, 2026
Conversation
A --trigger command rejected by its own argument parsing performed no action, so the marker it was meant to hit can never fire from it. The wait still ran the full --timeout-seconds window, then reported a timeout with a PlayMode diagnosis describing neither what happened nor what to fix. Abort on that one failure only, in a dedicated internal wait state so the timeout path's unconditional marker clear is not inherited: the marker stays armed and a corrected trigger can be retried without re-enabling. A wait that resumed PlayMode itself puts it back into pause, otherwise unrelated gameplay could consume the preserved marker's single shot and the retry would return someone else's hit. Any other trigger failure (connection drop, disabled tool, unparseable output) keeps waiting, since the game itself may still hit the marker.
Contributor
📝 WalkthroughWalkthroughPause-point waits now detect rejected triggers before execution, return a dedicated structured error, preserve the marker, report trigger results, and re-pause PlayMode after abandoned resumed waits. Documentation and tests describe and validate these behaviors. ChangesPause-point trigger flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant PausePointWait
participant Trigger
participant Unity
CLI->>PausePointWait: arm marker and start wait
PausePointWait->>Trigger: dispatch --trigger
Trigger-->>PausePointWait: completion or pre-execution rejection
PausePointWait->>Unity: query final marker status
PausePointWait->>Unity: re-pause after abandoned resumed wait
Unity-->>CLI: wait and recovery results
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 |
A --trigger whose command name does not exist is rejected before it reaches Unity, exactly like a rejected argument value: it performs no action, so the marker can never be hit by it. It was still costing a full timeout window, a cleared marker, and a Retryable:true verdict on a permanent failure — the same incident this wait state was added to prevent, reachable by one typo. Widen the abort condition to that class of pre-execution rejection and rename the predicate accordingly, since the criterion is no longer about argument parsing alone. Drop the response-side detection: no CLI path writes an error envelope to stdout and no Unity tool emits INVALID_ARGUMENT, so that branch was unreachable. A Unity-side rejection arriving on stdout carries no error code to match on. Also restate the failure message so it no longer asserts a marker lifetime the response may not carry when the final status re-read fails, and document the new behavior in the pause-point skill.
hatayama
merged commit Jul 26, 2026
e0022fa
into
feature/cli-discoverability-integration
1 of 2 checks passed
This was referenced Jul 26, 2026
3 tasks
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
--triggercommand is rejected before it runs — a bad argument value or a command name that does not exist — now fails immediately instead of waiting out the full--timeout-seconds.User Impact
Before,
enable-pause-point --await --timeout-seconds 60 --trigger "simulate-keyboard --action Hold --key A"(an action that does not exist) blocked for the whole 60 seconds and then reportedPAUSE_POINT_WAIT_TIMEOUTwith the PlayMode diagnosis for a missed code path — a diagnosis describing neither what happened nor what to fix. The trigger had already been rejected in the first milliseconds and never performed any action, so the marker could not possibly be hit by it. Worse, the timeout path clears the marker, so fixing the typo also required re-enabling it.A mistyped command name (
--trigger "simulate-keybord ...") cost exactly the same: the command never reached Unity at all, yet the wait ran to completion, cleared the marker, and reported the failure as retryable.The advice to pass a generous
--timeout-secondswhen arming while paused is correct, which means following it doubled the time lost to this failure.After this change both cases return in under a second with a dedicated failure that carries:
Details.TriggerResult.ErrorDetails.RemainingMilliseconds, since the marker is deliberately left armed and its lifetime keeps counting down while the trigger is being fixedA wait that resumed PlayMode itself puts PlayMode back into pause when it abandons the wait, and reports that as
ResumePlayResult.Repaused. Without this the game keeps running while the trigger is fixed, unrelated gameplay can reach the marker's line, and a single-shot marker's hit gets consumed — the retry would then return someone else's hit as if it were the corrected trigger's result. That is a quieter and worse failure than waiting 60 seconds.Only a pre-execution rejection aborts the wait. A connection drop, a disabled tool, or unparseable trigger output keeps waiting, because the game itself may still hit the marker and abandoning that wait would turn a recoverable situation into a lost hit.
Changes
INVALID_ARGUMENTorUNKNOWN_COMMANDenvelope on the dispatched command's stderr. Only stderr is inspected, since that is where every error envelope is written.Completed:falseover a real result.RemainingMillisecondswith an emptyDetails.Statusmeans that re-read failed, rather than asserting a lifetime the response may not carry.PAUSE_POINT_TRIGGER_FAILEDto the shared error-code list, withRetryable/SafeToRetryfalse: retrying the identical command reproduces the same rejection. Export the existing unknown-command code so the trigger check can reference it.ResumePlayResult(Repaused,RepauseError), including when the re-pause itself fails. A resume that was a no-op because PlayMode was already running sends no Pause.waitForPausePointso the added re-pause and result handling stay under the repository complexity limit.Verification
Tests were written first and confirmed failing before the implementation (build failure on the not-yet-existing state, error code, and predicate), then re-run after it.
scripts/check-go-cli.sh— exit status 0, lint0 issues.for all four modules, all module test suitesokgo test ./internal/projectrunner/ -v -run '...'printed--- PASSfor each ofTestWaitForPausePointAbortsWhenTriggerRejectsArguments,TestRunWaitForPausePointKeepsMarkerWhenTriggerRejectsArguments,TestRunWaitForPausePointKeepsMarkerWhenTriggerCommandIsUnknown,TestWaitForPausePointDoesNotAbortWhenTriggerSucceeds,TestWaitForPausePointReportsHitRacingATriggerRejection,TestRunWaitForPausePointRepausesPlayModeWhenTriggerRejectsArguments,TestRunWaitForPausePointReportsRepauseFailure,TestWaitForPausePointDoesNotRepauseWhenResumeWasANoOp,TestWaitForPausePointDoesNotRepauseWhenItDidNotResume, andTestPausePointTriggerRejectedBeforeExecution(6 subcases)scripts/stamp-release-inputs.sh— run in this pull request, since it changes a non-testcli/common/sourceManual verification against a running Editor, with a marker armed on a real line (
enable-pause-point --file .../SpaceHoldPoller.cs --line 19 --timeout-seconds 30) andULOOP_PROJECT_RUNNER_PATHpointed at the locally built runner:--trigger "simulate-keyboard --action Hold --key A"returned in 0s (previously 30s) withPAUSE_POINT_TRIGGER_FAILED,Retryable:false,RemainingMilliseconds:29966, the verbatimINVALID_ARGUMENTrejection underTriggerResult.Error, and a recovery command carrying the real marker id.--trigger "simulate-keybord --action Press --key Space"(mistyped command name) returned in 0s with the same error code,Retryable:false,RemainingMilliseconds:29963, and theUNKNOWN_COMMANDrejection underTriggerResult.Error.pause-point-statusafter each run reportedStatus:Enabled,IsEnabled:true,HitCount:0— the marker was preserved.--resume-playadded, the abort reportedResumePlayResult {WasPaused:true, Resumed:true, Repaused:true}, andcontrol-play-mode --action Statusafterwards reportedIsPlaying:true, IsPaused:true— PlayMode was genuinely put back into pause.Note on CI: repository workflows are scoped to
pull_request.branches [main, v3-beta], so none of them run on a pull request targeting this integration branch. The results above are local runs; CI validation happens on the final integration pull request.