From 49248f19ebe2eec3066100734d324d8633f42f59 Mon Sep 17 00:00:00 2001 From: hatayama Date: Sun, 26 Jul 2026 23:54:46 +0900 Subject: [PATCH 1/2] Abandon a pause-point wait when its --trigger is rejected 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. --- cli/common/errors/error_envelope.go | 1 + cli/dispatcher/shared-inputs-stamp.json | 2 +- .../projectrunner/pause_point_errors.go | 41 ++ .../projectrunner/pause_point_resume_play.go | 55 +- .../pause_point_resume_play_test.go | 12 +- .../projectrunner/pause_point_trigger.go | 50 ++ .../projectrunner/pause_point_wait_poll.go | 166 +++-- .../pause_point_wait_poll_test.go | 604 ++++++++++++++++++ cli/project-runner/shared-inputs-stamp.json | 2 +- 9 files changed, 870 insertions(+), 63 deletions(-) create mode 100644 cli/project-runner/internal/projectrunner/pause_point_wait_poll_test.go diff --git a/cli/common/errors/error_envelope.go b/cli/common/errors/error_envelope.go index 3e1a947f80..719e856200 100644 --- a/cli/common/errors/error_envelope.go +++ b/cli/common/errors/error_envelope.go @@ -33,6 +33,7 @@ const ( ErrorCodePausePointWaitTimeout = "PAUSE_POINT_WAIT_TIMEOUT" ErrorCodePausePointExpired = "PAUSE_POINT_EXPIRED" ErrorCodePausePointCleared = "PAUSE_POINT_CLEARED" + ErrorCodePausePointTriggerFailed = "PAUSE_POINT_TRIGGER_FAILED" ErrorCodeInternalError = "INTERNAL_ERROR" ErrorPhaseArgumentParsing = "argument_parsing" diff --git a/cli/dispatcher/shared-inputs-stamp.json b/cli/dispatcher/shared-inputs-stamp.json index ff8ce993af..38aa1cf2a6 100644 --- a/cli/dispatcher/shared-inputs-stamp.json +++ b/cli/dispatcher/shared-inputs-stamp.json @@ -1,4 +1,4 @@ { "schemaVersion": 1, - "sharedInputsHash": "417686b615796ec8240c7bb104c4abd43c76dceb" + "sharedInputsHash": "7bf1869ee95b32433e74738131488c4343ec8a80" } diff --git a/cli/project-runner/internal/projectrunner/pause_point_errors.go b/cli/project-runner/internal/projectrunner/pause_point_errors.go index 23003500f0..5351756cff 100644 --- a/cli/project-runner/internal/projectrunner/pause_point_errors.go +++ b/cli/project-runner/internal/projectrunner/pause_point_errors.go @@ -39,6 +39,22 @@ func pausePointWaitError( expiredError.Details["Hint"] = hint } return expiredError + case pausePointWaitStateTriggerFailed: + triggerFailedError := pausePointStateError( + clierrors.ErrorCodePausePointTriggerFailed, + "The --trigger command was rejected by its own argument parsing, so it never ran and the "+ + "wait was abandoned instead of waiting out the remaining timeout. The marker is still "+ + "armed: see Details.TriggerResult for the rejection and Details.RemainingMilliseconds "+ + "for how long it stays armed.", + projectRoot, + options, + response, + // Retrying the identical command reproduces the same rejection: the trigger value has to + // change first. Reporting this as retryable is what made the original incident waste a + // full timeout window on a permanent failure. + false) + triggerFailedError.NextActions = pausePointTriggerFailedNextActions(options.id) + return triggerFailedError case pausePointWaitStateCleared: return pausePointStateError( clierrors.ErrorCodePausePointCleared, @@ -63,6 +79,31 @@ func pausePointWaitError( } } +// pausePointTriggerFailedNextActions replaces the generic enable/id-mismatch guidance, which does +// not apply here: the marker was confirmed armed and only the --trigger value is wrong. +// +// Why re-running the same command comes first: this response answers the command the caller just +// ran, so "fix the --trigger value in that command and run it again" asks them to change one value +// they already typed, with no argument they have to guess. Re-enabling is also the cleaner reset — +// it starts a fresh marker entry (HitCount and IsHit back to zero, the --timeout-seconds countdown +// restarted) while re-patching an id that is already patched is a no-op. +// +// Why the await form carries the real id: it is the one recovery command this function can spell +// out completely, and naming a command without its arguments is exactly the failure this guidance +// exists to prevent. +func pausePointTriggerFailedNextActions(id string) []string { + return []string{ + "Fix the --trigger value in the command you just ran and run that command again. Re-running " + + "`enable-pause-point --await` is safe and is the cleanest reset: it restarts the marker's " + + "HitCount and --timeout-seconds countdown, and re-patching an already patched id is a no-op.", + fmt.Sprintf( + "The marker is still armed, so you can also wait on it directly: "+ + "uloop await-pause-point --id %q --trigger \"\"", id), + "Check the rejected value against the triggered command's own `--help` before retrying, so the " + + "same value is not retried twice.", + } +} + const ( pausePointHintPlayModeNotRunning = "PlayMode is not running. Start PlayMode (or trigger the marker code path in Edit Mode), then wait again." pausePointHintEditorAlreadyPaused = "Unity is already paused, so gameplay cannot reach the marker. Resume PlayMode before waiting again." diff --git a/cli/project-runner/internal/projectrunner/pause_point_resume_play.go b/cli/project-runner/internal/projectrunner/pause_point_resume_play.go index 08c38e88e6..00f1db771c 100644 --- a/cli/project-runner/internal/projectrunner/pause_point_resume_play.go +++ b/cli/project-runner/internal/projectrunner/pause_point_resume_play.go @@ -21,6 +21,16 @@ type pausePointResumePlayResult struct { WasPaused bool `json:"WasPaused"` Resumed bool `json:"Resumed"` Error string `json:"Error,omitempty"` + + // Repaused reports that the wait put PlayMode back into pause after resuming it, which happens + // when the wait is abandoned because the --trigger command was rejected before it ran. Reported + // rather than kept internal: a caller that asked for a resume must be able to see that the + // Editor is paused again, otherwise the next command's behavior looks unexplained. + Repaused bool `json:"Repaused,omitempty"` + + // RepauseError explains why the re-pause failed. Not hidden: a failed re-pause means gameplay + // keeps running and can still consume the preserved marker's single shot. + RepauseError string `json:"RepauseError,omitempty"` } type controlPlayModeToolResponse struct { @@ -33,9 +43,10 @@ type controlPlayModeToolResponse struct { // trigger ordering without a live Unity connection. var resumePlayModeForPausePoint = resumePlayModeForPausePointFromUnity -// sendControlPlayModeForPausePointResume is overridden in tests so -// resumePlayModeForPausePointFromUnity's Status/Play branches can be exercised without IPC. -var sendControlPlayModeForPausePointResume = sendControlPlayModeForPausePointResumeFromUnity +// sendControlPlayModeForPausePoint is overridden in tests so resumePlayModeForPausePointFromUnity's +// Status/Play branches and the Pause request sent when a wait is abandoned can be exercised +// without IPC. +var sendControlPlayModeForPausePoint = sendControlPlayModeForPausePointFromUnity // resumePlayModeForPausePointFromUnity asks control-play-mode for Status, then sends Play only when // the Editor is currently paused. A Status or Play failure is returned as Error so the wait path @@ -47,7 +58,7 @@ func resumePlayModeForPausePointFromUnity( resumeContext, cancel := context.WithTimeout(ctx, pausePointResumePlayTimeout) defer cancel() - statusResponse, err := sendControlPlayModeForPausePointResume(resumeContext, connection, "Status") + statusResponse, err := sendControlPlayModeForPausePoint(resumeContext, connection, "Status") if err != nil { return pausePointResumePlayResult{ Error: fmt.Sprintf("control-play-mode Status failed: %v", err), @@ -65,7 +76,7 @@ func resumePlayModeForPausePointFromUnity( return pausePointResumePlayResult{WasPaused: false, Resumed: false} } - playResponse, err := sendControlPlayModeForPausePointResume(resumeContext, connection, "Play") + playResponse, err := sendControlPlayModeForPausePoint(resumeContext, connection, "Play") if err != nil { return pausePointResumePlayResult{ WasPaused: true, @@ -88,7 +99,39 @@ func resumePlayModeForPausePointFromUnity( return pausePointResumePlayResult{WasPaused: true, Resumed: true} } -func sendControlPlayModeForPausePointResumeFromUnity( +// repausePlayModeAfterAbandonedWait puts PlayMode back into pause after this command's own +// --resume-play resumed it, once the wait is abandoned because the --trigger command was rejected +// before it ran. Why re-pause: the marker is deliberately left armed so the trigger can be fixed +// and retried, but a marker left armed while gameplay keeps running can have its single shot +// consumed by unrelated game activity reaching the same line — the retry would then return +// someone else's hit as if it were the fixed trigger's result. +func repausePlayModeAfterAbandonedWait( + ctx context.Context, + connection unityipc.Connection, + resumeResult pausePointResumePlayResult, +) pausePointResumePlayResult { + pauseContext, cancel := context.WithTimeout(ctx, pausePointResumePlayTimeout) + defer cancel() + + response, err := sendControlPlayModeForPausePoint(pauseContext, connection, "Pause") + if err != nil { + resumeResult.RepauseError = fmt.Sprintf("control-play-mode Pause failed: %v", err) + return resumeResult + } + if !response.Success { + message := response.Message + if message == "" { + message = "control-play-mode Pause returned Success=false" + } + resumeResult.RepauseError = message + return resumeResult + } + + resumeResult.Repaused = true + return resumeResult +} + +func sendControlPlayModeForPausePointFromUnity( ctx context.Context, connection unityipc.Connection, action string, diff --git a/cli/project-runner/internal/projectrunner/pause_point_resume_play_test.go b/cli/project-runner/internal/projectrunner/pause_point_resume_play_test.go index 5eb4124ab0..acc5bd08fd 100644 --- a/cli/project-runner/internal/projectrunner/pause_point_resume_play_test.go +++ b/cli/project-runner/internal/projectrunner/pause_point_resume_play_test.go @@ -491,12 +491,12 @@ func TestWaitForPausePointResumesWithoutTriggerWhenArmed(t *testing.T) { // Verifies resumePlayModeForPausePointFromUnity's Status/Play branches without a live Unity IPC. func TestResumePlayModeForPausePointFromUnityBranches(t *testing.T) { - originalSend := sendControlPlayModeForPausePointResume - defer func() { sendControlPlayModeForPausePointResume = originalSend }() + originalSend := sendControlPlayModeForPausePoint + defer func() { sendControlPlayModeForPausePoint = originalSend }() t.Run("Status transport failure", func(t *testing.T) { actions := make([]string, 0, 1) - sendControlPlayModeForPausePointResume = func( + sendControlPlayModeForPausePoint = func( ctx context.Context, connection unityipc.Connection, action string, @@ -516,7 +516,7 @@ func TestResumePlayModeForPausePointFromUnityBranches(t *testing.T) { t.Run("Status Success=false", func(t *testing.T) { actions := make([]string, 0, 1) - sendControlPlayModeForPausePointResume = func( + sendControlPlayModeForPausePoint = func( ctx context.Context, connection unityipc.Connection, action string, @@ -536,7 +536,7 @@ func TestResumePlayModeForPausePointFromUnityBranches(t *testing.T) { t.Run("IsPaused=false skips Play", func(t *testing.T) { actions := make([]string, 0, 1) - sendControlPlayModeForPausePointResume = func( + sendControlPlayModeForPausePoint = func( ctx context.Context, connection unityipc.Connection, action string, @@ -556,7 +556,7 @@ func TestResumePlayModeForPausePointFromUnityBranches(t *testing.T) { t.Run("Play transport failure", func(t *testing.T) { actions := make([]string, 0, 2) - sendControlPlayModeForPausePointResume = func( + sendControlPlayModeForPausePoint = func( ctx context.Context, connection unityipc.Connection, action string, diff --git a/cli/project-runner/internal/projectrunner/pause_point_trigger.go b/cli/project-runner/internal/projectrunner/pause_point_trigger.go index 49bf1daec5..4062f6362d 100644 --- a/cli/project-runner/internal/projectrunner/pause_point_trigger.go +++ b/cli/project-runner/internal/projectrunner/pause_point_trigger.go @@ -39,6 +39,46 @@ type pausePointTriggerResult struct { Error string `json:"Error,omitempty"` } +// pausePointTriggerFailedWithArgumentError reports whether the trigger command was rejected by +// argument parsing, the one failure that makes waiting out the marker's remaining lifetime +// pointless: the trigger performed no action, so the marker can never be hit by it. +// +// Deliberately narrow. A connection drop, a disabled tool, or unparseable output must not abort the +// wait — the marker may still be hit by the game itself, and abandoning that wait would turn a +// recoverable situation into a lost hit. Anything this function cannot positively identify as an +// argument rejection keeps the wait running. +func pausePointTriggerFailedWithArgumentError(result *pausePointTriggerResult) bool { + if result == nil { + return false + } + + // Error carries what the dispatched command wrote to stderr, which is where a CLI-side argument + // rejection lands; Response carries its stdout, which is where a Unity-side argument rejection + // arrives with the same code. + return isPausePointInvalidArgumentPayload([]byte(result.Error)) || + isPausePointInvalidArgumentPayload(result.Response) +} + +func isPausePointInvalidArgumentPayload(payload []byte) bool { + trimmed := bytes.TrimSpace(payload) + if len(trimmed) == 0 { + return false + } + + envelope := struct { + ErrorCode string `json:"ErrorCode"` + Error struct { + ErrorCode string `json:"ErrorCode"` + } `json:"Error"` + }{} + if err := json.Unmarshal(trimmed, &envelope); err != nil { + return false + } + + return envelope.Error.ErrorCode == clierrors.ErrorCodeInvalidArgument || + envelope.ErrorCode == clierrors.ErrorCodeInvalidArgument +} + // parsePausePointTriggerCommand splits a --trigger value into a command name and its arguments, // and rejects shapes that cannot behave sensibly when dispatched from inside this CLI process. func parsePausePointTriggerCommand(command string, value string) (string, []string, error) { @@ -187,6 +227,16 @@ func startPausePointTrigger( return handle } +// doneChannel exposes the completion channel for the pause-point poll loop's select, so a trigger +// that fails on its own arguments can be observed the moment it reports instead of only at join +// time. Nil-safe: a nil handle yields a nil channel, which a select case never selects. +func (h *pausePointTriggerHandle) doneChannel() <-chan *pausePointTriggerResult { + if h == nil { + return nil + } + return h.done +} + // join waits briefly for the trigger goroutine started by startPausePointTrigger, once the // pause-point wait itself has already settled. The grace window mirrors // pausePointFinalStatusProbeTimeout: a genuine hit interrupts simulate-* commands immediately diff --git a/cli/project-runner/internal/projectrunner/pause_point_wait_poll.go b/cli/project-runner/internal/projectrunner/pause_point_wait_poll.go index f00e8b4ab5..f553aded93 100644 --- a/cli/project-runner/internal/projectrunner/pause_point_wait_poll.go +++ b/cli/project-runner/internal/projectrunner/pause_point_wait_poll.go @@ -20,6 +20,14 @@ const ( pausePointWaitStateNotEnabled pausePointWaitState = "not_enabled" pausePointWaitStateExpired pausePointWaitState = "expired" pausePointWaitStateCleared pausePointWaitState = "cleared" + + // pausePointWaitStateTriggerFailed means the wait was abandoned because the --trigger command + // was rejected by argument parsing, so it never performed the action the marker was waiting for. + // Why not reuse the timeout state: the timeout path unconditionally clears the marker, which + // would force a re-enable just to retry a corrected trigger, and it reports a "not hit within + // %ds" message with a PlayMode diagnosis that describes neither what happened nor what to fix. + // Internal to this package — it never appears on the wire. + pausePointWaitStateTriggerFailed pausePointWaitState = "trigger_failed" ) // waitForPausePoint confirms the marker is actually armed with one status query before starting @@ -34,60 +42,88 @@ func waitForPausePoint( connection unityipc.Connection, options waitForPausePointOptions, ) (pausePointStatusResponse, pausePointWaitState, *pausePointTriggerResult, *pausePointResumePlayResult, error) { - var triggerHandle *pausePointTriggerHandle - var skippedTriggerResult *pausePointTriggerResult - var resumeResult *pausePointResumePlayResult + triggerHandle, skippedTriggerResult, resumeResult := startPausePointWaitSideEffects(ctx, connection, options) - if options.triggerCommand != "" || options.resumePlay { - if pausePointIsArmed(ctx, connection, options.id) { - if options.resumePlay { - result := resumePlayModeForPausePoint(ctx, connection) - resumeResult = &result - if result.Error != "" { - if options.triggerCommand != "" { - skippedTriggerResult = &pausePointTriggerResult{ - Command: pausePointTriggerCommandString(options.triggerCommand, options.triggerArgs), - Error: "trigger was not dispatched: --resume-play failed to resume play mode", - } - } - } else if options.triggerCommand != "" { - triggerHandle = startPausePointTrigger(ctx, connection, options.startPath, options.triggerCommand, options.triggerArgs) - } - } else if options.triggerCommand != "" { - triggerHandle = startPausePointTrigger(ctx, connection, options.startPath, options.triggerCommand, options.triggerArgs) - } - } else { - if options.resumePlay { - // --resume-play always yields a ResumePlayResult when given, even when skipped: a - // silently omitted result would hide "arm was never confirmed" behind a plain - // timeout/not-enabled error with no clue why Play was never resumed. - resumeResult = &pausePointResumePlayResult{ - Error: "resume was not dispatched: the marker could not be confirmed armed at wait start", - } - } - if options.triggerCommand != "" { - // --trigger always yields a TriggerResult when given, even when skipped: a silently - // omitted TriggerResult would be indistinguishable from "the trigger ran but this CLI - // exited before it reported anything," hiding the real reason (marker not confirmed - // armed) behind a plain timeout/not-enabled error with no clue why the trigger never fired. - skippedTriggerResult = &pausePointTriggerResult{ - Command: pausePointTriggerCommandString(options.triggerCommand, options.triggerArgs), - Error: "trigger was not dispatched: the marker could not be confirmed armed at wait start", - } - } - } + response, state, polledTriggerResult, err := waitForPausePointStatus(ctx, connection, options, triggerHandle) + + if state == pausePointWaitStateTriggerFailed && resumeResult != nil && resumeResult.Resumed { + repaused := repausePlayModeAfterAbandonedWait(ctx, connection, *resumeResult) + resumeResult = &repaused } - response, state, err := waitForPausePointStatus(ctx, connection, options) if skippedTriggerResult != nil { return response, state, skippedTriggerResult, resumeResult, err } + // The trigger goroutine's buffered channel yields exactly one value, so a result already + // received by the poll loop must be reused here instead of joining again — a second receive + // would block for the whole grace window and then report Completed:false over a real result. + if polledTriggerResult != nil { + return response, state, polledTriggerResult, resumeResult, err + } if triggerHandle != nil { return response, state, triggerHandle.join(), resumeResult, err } return response, state, nil, resumeResult, err } +// startPausePointWaitSideEffects performs the pre-wait --resume-play / --trigger work, returning +// either a live trigger handle or the fixed TriggerResult explaining why the trigger was skipped. +func startPausePointWaitSideEffects( + ctx context.Context, + connection unityipc.Connection, + options waitForPausePointOptions, +) (*pausePointTriggerHandle, *pausePointTriggerResult, *pausePointResumePlayResult) { + if options.triggerCommand == "" && !options.resumePlay { + return nil, nil, nil + } + + if !pausePointIsArmed(ctx, connection, options.id) { + var resumeResult *pausePointResumePlayResult + var skippedTriggerResult *pausePointTriggerResult + if options.resumePlay { + // --resume-play always yields a ResumePlayResult when given, even when skipped: a + // silently omitted result would hide "arm was never confirmed" behind a plain + // timeout/not-enabled error with no clue why Play was never resumed. + resumeResult = &pausePointResumePlayResult{ + Error: "resume was not dispatched: the marker could not be confirmed armed at wait start", + } + } + if options.triggerCommand != "" { + // --trigger always yields a TriggerResult when given, even when skipped: a silently + // omitted TriggerResult would be indistinguishable from "the trigger ran but this CLI + // exited before it reported anything," hiding the real reason (marker not confirmed + // armed) behind a plain timeout/not-enabled error with no clue why the trigger never fired. + skippedTriggerResult = &pausePointTriggerResult{ + Command: pausePointTriggerCommandString(options.triggerCommand, options.triggerArgs), + Error: "trigger was not dispatched: the marker could not be confirmed armed at wait start", + } + } + return nil, skippedTriggerResult, resumeResult + } + + var resumeResult *pausePointResumePlayResult + if options.resumePlay { + result := resumePlayModeForPausePoint(ctx, connection) + resumeResult = &result + if result.Error != "" { + if options.triggerCommand == "" { + return nil, nil, resumeResult + } + return nil, &pausePointTriggerResult{ + Command: pausePointTriggerCommandString(options.triggerCommand, options.triggerArgs), + Error: "trigger was not dispatched: --resume-play failed to resume play mode", + }, resumeResult + } + } + + if options.triggerCommand == "" { + return nil, nil, resumeResult + } + + handle := startPausePointTrigger(ctx, connection, options.startPath, options.triggerCommand, options.triggerArgs) + return handle, nil, resumeResult +} + // pausePointIsArmed reports whether the marker is enabled or already hit. A query failure is // treated as not armed: dispatching a --trigger command against a marker this CLI cannot even // confirm exists would inject the trigger's action into the game with no corresponding wait. @@ -104,13 +140,16 @@ func waitForPausePointStatus( ctx context.Context, connection unityipc.Connection, options waitForPausePointOptions, -) (pausePointStatusResponse, pausePointWaitState, error) { + triggerHandle *pausePointTriggerHandle, +) (pausePointStatusResponse, pausePointWaitState, *pausePointTriggerResult, error) { waitContext, cancel := context.WithTimeout(ctx, options.timeout) defer cancel() lastResponse := pausePointStatusResponse{Id: options.id} var lastErr error + var triggerResult *pausePointTriggerResult hasResponse := false + triggerDone := triggerHandle.doneChannel() ticker := time.NewTicker(pausePointStatusPoll) defer ticker.Stop() for { @@ -120,7 +159,7 @@ func waitForPausePointStatus( hasResponse = true state := pausePointWaitStateForStatus(response.Status) if state != "" { - return response, state, nil + return response, state, triggerResult, nil } } else { lastErr = err @@ -129,30 +168,59 @@ func waitForPausePointStatus( select { case <-waitContext.Done(): if ctx.Err() != nil { - return lastResponse, "", ctx.Err() + return lastResponse, "", triggerResult, ctx.Err() } finalResponse, finalState, hasFinalResponse, finalErr := queryPausePointStatusAtTimeout(ctx, connection, options.id) if hasFinalResponse { lastResponse = finalResponse hasResponse = true if finalState != "" { - return finalResponse, finalState, nil + return finalResponse, finalState, triggerResult, nil } } else if lastErr == nil { lastErr = finalErr } if hasResponse { - return lastResponse, pausePointWaitStateTimeout, nil + return lastResponse, pausePointWaitStateTimeout, triggerResult, nil } if lastErr != nil { - return lastResponse, "", fmt.Errorf("timed out waiting for pause point status: %w", lastErr) + return lastResponse, "", triggerResult, fmt.Errorf("timed out waiting for pause point status: %w", lastErr) + } + return lastResponse, pausePointWaitStateTimeout, triggerResult, nil + case result := <-triggerDone: + // Nil the channel so this case can never fire twice: the handle's channel holds a single + // buffered value, and the caller reuses the result received here instead of joining. + triggerResult = result + triggerDone = nil + if pausePointTriggerFailedWithArgumentError(result) { + abortResponse, abortState := abortPausePointWaitAfterTriggerRejection( + ctx, connection, options.id, lastResponse) + return abortResponse, abortState, triggerResult, nil } - return lastResponse, pausePointWaitStateTimeout, nil case <-ticker.C: } } } +// abortPausePointWaitAfterTriggerRejection takes one last status reading before abandoning the +// wait, because a trigger rejection can race a genuine hit produced by the game itself: without +// this the hit would be discarded and reported as a trigger failure. +func abortPausePointWaitAfterTriggerRejection( + ctx context.Context, + connection unityipc.Connection, + id string, + lastResponse pausePointStatusResponse, +) (pausePointStatusResponse, pausePointWaitState) { + response, state, hasResponse, _ := queryPausePointStatusAtTimeout(ctx, connection, id) + if !hasResponse { + return lastResponse, pausePointWaitStateTriggerFailed + } + if state != "" { + return response, state + } + return response, pausePointWaitStateTriggerFailed +} + func queryPausePointStatusAtTimeout( ctx context.Context, connection unityipc.Connection, diff --git a/cli/project-runner/internal/projectrunner/pause_point_wait_poll_test.go b/cli/project-runner/internal/projectrunner/pause_point_wait_poll_test.go new file mode 100644 index 0000000000..2ce5eca4c9 --- /dev/null +++ b/cli/project-runner/internal/projectrunner/pause_point_wait_poll_test.go @@ -0,0 +1,604 @@ +package projectrunner + +import ( + "bytes" + "context" + "io" + "strings" + "testing" + "time" + + clierrors "github.com/hatayama/unity-cli-loop/common/errors" + + "github.com/hatayama/unity-cli-loop/common/unityipc" +) + +// argumentErrorTriggerStderr is the error envelope a trigger command writes when its own argument +// parsing rejects a value, which is exactly the case that must abort the wait instead of letting +// the marker's full --timeout-seconds elapse. +const argumentErrorTriggerStderr = `{"Success":false,"Error":{"ErrorCode":"INVALID_ARGUMENT",` + + `"Phase":"argument_parsing","Message":"Invalid value for --action: \"Hold\"","Retryable":false,` + + `"SafeToRetry":false,"NextActions":["Pass one of: Press, KeyDown, KeyUp, ReleaseAll."]}}` + +// pausePointArmedStatusResponse is an armed-but-never-hit marker with a lifetime long enough that +// RemainingMilliseconds stays positive, so the abort response can be asserted to carry it. +func pausePointArmedStatusResponse(id string) pausePointStatusResponse { + return pausePointStatusResponse{ + Id: id, + Status: pausePointStatusEnabled, + IsEnabled: true, + TimeoutSeconds: 60, + ElapsedSinceEnabledMilliseconds: 1_000, + EditorState: pausePointEditorState{IsPlaying: true, CapturedAt: "Current"}, + } +} + +// Verifies a --trigger rejected by its own argument parsing aborts the wait immediately, instead of +// waiting out the marker's --timeout-seconds, and reports the rejection on TriggerResult. +func TestWaitForPausePointAbortsWhenTriggerRejectsArguments(t *testing.T) { + originalQuery := queryPausePointStatus + originalDispatch := dispatchPausePointTriggerCommand + originalPoll := pausePointStatusPoll + pausePointStatusPoll = time.Millisecond + defer func() { + queryPausePointStatus = originalQuery + dispatchPausePointTriggerCommand = originalDispatch + pausePointStatusPoll = originalPoll + }() + + queryPausePointStatus = func( + ctx context.Context, + connection unityipc.Connection, + id string, + ) (pausePointStatusResponse, error) { + return pausePointArmedStatusResponse(id), nil + } + dispatchPausePointTriggerCommand = func( + ctx context.Context, + connection unityipc.Connection, + command string, + commandArgs []string, + startPath string, + stdout io.Writer, + stderr io.Writer, + ) int { + _, _ = stderr.Write([]byte(argumentErrorTriggerStderr)) + return 1 + } + + startedAt := time.Now() + _, state, triggerResult, _, err := waitForPausePoint(context.Background(), unityipc.Connection{}, waitForPausePointOptions{ + id: "jump", + timeoutSeconds: 60, + timeout: 10 * time.Second, + triggerCommand: "simulate-keyboard", + triggerArgs: []string{"--action", "Hold", "--key", "A"}, + }) + elapsed := time.Since(startedAt) + + if err != nil { + t.Fatalf("waitForPausePoint failed: %v", err) + } + if state != pausePointWaitStateTriggerFailed { + t.Fatalf("expected trigger_failed state, got %q", state) + } + if elapsed >= 5*time.Second { + t.Fatalf("expected an early abort, waited %v of a 10s timeout", elapsed) + } + if triggerResult == nil { + t.Fatal("expected a TriggerResult reporting the rejection, got nil") + } + if !strings.Contains(triggerResult.Error, "INVALID_ARGUMENT") { + t.Fatalf("expected the trigger's own rejection in Error, got %#v", triggerResult) + } + if triggerResult.Command != "simulate-keyboard --action Hold --key A" { + t.Fatalf("TriggerResult command mismatch: %#v", triggerResult) + } +} + +// Verifies the abort keeps the marker armed (no clear) and reports a dedicated error code with the +// marker's remaining lifetime and a copy-pasteable recovery command, rather than reusing the +// timeout error's message and PlayMode diagnosis. +func TestRunWaitForPausePointKeepsMarkerWhenTriggerRejectsArguments(t *testing.T) { + originalQuery := queryPausePointStatus + originalClear := clearPausePointStatus + originalDispatch := dispatchPausePointTriggerCommand + originalPoll := pausePointStatusPoll + pausePointStatusPoll = time.Millisecond + defer func() { + queryPausePointStatus = originalQuery + clearPausePointStatus = originalClear + dispatchPausePointTriggerCommand = originalDispatch + pausePointStatusPoll = originalPoll + }() + + queryPausePointStatus = func( + ctx context.Context, + connection unityipc.Connection, + id string, + ) (pausePointStatusResponse, error) { + return pausePointArmedStatusResponse(id), nil + } + clearCalled := false + clearPausePointStatus = func( + ctx context.Context, + connection unityipc.Connection, + id string, + ) (pausePointStatusResponse, error) { + clearCalled = true + return pausePointStatusResponse{Id: id, Status: pausePointStatusCleared}, nil + } + dispatchPausePointTriggerCommand = func( + ctx context.Context, + connection unityipc.Connection, + command string, + commandArgs []string, + startPath string, + stdout io.Writer, + stderr io.Writer, + ) int { + _, _ = stderr.Write([]byte(argumentErrorTriggerStderr)) + return 1 + } + + var stdout bytes.Buffer + var stderr bytes.Buffer + code := runWaitForPausePoint(context.Background(), unityipc.Connection{}, waitForPausePointOptions{ + id: "jump", + timeoutSeconds: 60, + timeout: 10 * time.Second, + triggerCommand: "simulate-keyboard", + triggerArgs: []string{"--action", "Hold", "--key", "A"}, + }, &stdout, &stderr) + + if code != 1 { + t.Fatalf("expected failure exit code, got %d with stdout %s", code, stdout.String()) + } + if clearCalled { + t.Fatal("expected the marker to stay armed: clear must not be called when the trigger was rejected") + } + + envelope := parsePausePointErrorEnvelope(t, stderr.Bytes()) + if envelope.Error.ErrorCode != clierrors.ErrorCodePausePointTriggerFailed { + t.Fatalf("error code mismatch: %#v", envelope.Error) + } + if envelope.Error.Retryable || envelope.Error.SafeToRetry { + t.Fatalf("a rejected trigger is not fixed by retrying the same command: %#v", envelope.Error) + } + if strings.Contains(envelope.Error.Message, "was not hit within") { + t.Fatalf("the timeout message must not be reused for an aborted wait: %#v", envelope.Error) + } + if _, hasHint := envelope.Error.Details["Hint"]; hasHint { + t.Fatalf("the timeout PlayMode diagnosis must not be attached to an aborted wait: %#v", envelope.Error.Details) + } + triggerResult, ok := envelope.Error.Details["TriggerResult"].(map[string]any) + if !ok { + t.Fatalf("TriggerResult detail missing or wrong shape: %#v", envelope.Error.Details) + } + if errorText, _ := triggerResult["Error"].(string); !strings.Contains(errorText, "INVALID_ARGUMENT") { + t.Fatalf("TriggerResult must carry the trigger's own rejection: %#v", triggerResult) + } + remaining, ok := envelope.Error.Details["RemainingMilliseconds"].(float64) + if !ok || remaining <= 0 { + t.Fatalf("expected a positive RemainingMilliseconds for the preserved marker: %#v", envelope.Error.Details) + } + + nextActions := strings.Join(envelope.Error.NextActions, "\n") + if !strings.Contains(nextActions, "--trigger") { + t.Fatalf("recovery guidance must point at the --trigger value to fix: %#v", envelope.Error.NextActions) + } + if !strings.Contains(nextActions, `uloop await-pause-point --id "jump"`) { + t.Fatalf("expected a copy-pasteable await command carrying the real id: %#v", envelope.Error.NextActions) + } +} + +// Verifies a trigger that completes normally mid-wait never aborts the wait and is still reported +// once, so receiving it inside the poll loop does not regress the existing join behavior. +func TestWaitForPausePointDoesNotAbortWhenTriggerSucceeds(t *testing.T) { + originalQuery := queryPausePointStatus + originalDispatch := dispatchPausePointTriggerCommand + originalPoll := pausePointStatusPoll + pausePointStatusPoll = time.Millisecond + defer func() { + queryPausePointStatus = originalQuery + dispatchPausePointTriggerCommand = originalDispatch + pausePointStatusPoll = originalPoll + }() + + queryPausePointStatus = func( + ctx context.Context, + connection unityipc.Connection, + id string, + ) (pausePointStatusResponse, error) { + return pausePointArmedStatusResponse(id), nil + } + dispatchPausePointTriggerCommand = func( + ctx context.Context, + connection unityipc.Connection, + command string, + commandArgs []string, + startPath string, + stdout io.Writer, + stderr io.Writer, + ) int { + _, _ = stdout.Write([]byte(`{"Success":true}`)) + return 0 + } + + _, state, triggerResult, _, err := waitForPausePoint(context.Background(), unityipc.Connection{}, waitForPausePointOptions{ + id: "jump", + timeoutSeconds: 60, + timeout: 50 * time.Millisecond, + triggerCommand: "simulate-keyboard", + triggerArgs: []string{"--action", "Press"}, + }) + if err != nil { + t.Fatalf("waitForPausePoint failed: %v", err) + } + if state != pausePointWaitStateTimeout { + t.Fatalf("a successful trigger must leave the wait to settle on its own, got state %q", state) + } + if triggerResult == nil || !triggerResult.Completed { + t.Fatalf("expected a completed trigger result, got %#v", triggerResult) + } + if triggerResult.Error != "" { + t.Fatalf("expected no trigger error, got %#v", triggerResult) + } +} + +// Verifies an already-hit marker observed by the status query taken just before aborting is +// reported as a hit, so a trigger rejection racing a real hit does not discard the hit. +func TestWaitForPausePointReportsHitRacingATriggerRejection(t *testing.T) { + originalQuery := queryPausePointStatus + originalDispatch := dispatchPausePointTriggerCommand + originalPoll := pausePointStatusPoll + pausePointStatusPoll = time.Hour + defer func() { + queryPausePointStatus = originalQuery + dispatchPausePointTriggerCommand = originalDispatch + pausePointStatusPoll = originalPoll + }() + + // Only the abort-time query can observe the hit: the poll ticker is parked for an hour, so the + // loop's own next status query never runs within this test. + queryCount := 0 + queryPausePointStatus = func( + ctx context.Context, + connection unityipc.Connection, + id string, + ) (pausePointStatusResponse, error) { + queryCount++ + if queryCount <= 2 { + return pausePointArmedStatusResponse(id), nil + } + return pausePointStatusResponse{ + Id: id, + Status: pausePointStatusHit, + IsHit: true, + HitCount: 1, + EditorState: pausePointEditorState{IsPlaying: true, IsPaused: true, CapturedAt: "PausePointHit"}, + }, nil + } + dispatchPausePointTriggerCommand = func( + ctx context.Context, + connection unityipc.Connection, + command string, + commandArgs []string, + startPath string, + stdout io.Writer, + stderr io.Writer, + ) int { + _, _ = stderr.Write([]byte(argumentErrorTriggerStderr)) + return 1 + } + + response, state, triggerResult, _, err := waitForPausePoint(context.Background(), unityipc.Connection{}, waitForPausePointOptions{ + id: "jump", + timeoutSeconds: 60, + timeout: 10 * time.Second, + triggerCommand: "simulate-keyboard", + triggerArgs: []string{"--action", "Hold"}, + }) + if err != nil { + t.Fatalf("waitForPausePoint failed: %v", err) + } + if state != pausePointWaitStateHit { + t.Fatalf("expected the raced hit to win, got state %q", state) + } + if !response.IsHit { + t.Fatalf("expected the hit response to be returned, got %#v", response) + } + if triggerResult == nil || triggerResult.Error == "" { + t.Fatalf("expected the rejection to still be reported alongside the hit, got %#v", triggerResult) + } +} + +// Verifies aborting a wait that resumed PlayMode itself puts PlayMode back into pause and reports +// that on ResumePlayResult, so gameplay cannot consume the preserved marker's single shot while the +// --trigger value is being fixed. +func TestRunWaitForPausePointRepausesPlayModeWhenTriggerRejectsArguments(t *testing.T) { + originalQuery := queryPausePointStatus + originalClear := clearPausePointStatus + originalDispatch := dispatchPausePointTriggerCommand + originalResume := resumePlayModeForPausePoint + originalSend := sendControlPlayModeForPausePoint + originalPoll := pausePointStatusPoll + pausePointStatusPoll = time.Millisecond + defer func() { + queryPausePointStatus = originalQuery + clearPausePointStatus = originalClear + dispatchPausePointTriggerCommand = originalDispatch + resumePlayModeForPausePoint = originalResume + sendControlPlayModeForPausePoint = originalSend + pausePointStatusPoll = originalPoll + }() + + queryPausePointStatus = func( + ctx context.Context, + connection unityipc.Connection, + id string, + ) (pausePointStatusResponse, error) { + return pausePointArmedStatusResponse(id), nil + } + clearPausePointStatus = func( + ctx context.Context, + connection unityipc.Connection, + id string, + ) (pausePointStatusResponse, error) { + t.Fatal("clearPausePointStatus must not be called when the trigger was rejected") + return pausePointStatusResponse{}, nil + } + resumePlayModeForPausePoint = func( + ctx context.Context, + connection unityipc.Connection, + ) pausePointResumePlayResult { + return pausePointResumePlayResult{WasPaused: true, Resumed: true} + } + actions := make([]string, 0, 1) + sendControlPlayModeForPausePoint = func( + ctx context.Context, + connection unityipc.Connection, + action string, + ) (controlPlayModeToolResponse, error) { + actions = append(actions, action) + return controlPlayModeToolResponse{Success: true, IsPaused: true}, nil + } + dispatchPausePointTriggerCommand = func( + ctx context.Context, + connection unityipc.Connection, + command string, + commandArgs []string, + startPath string, + stdout io.Writer, + stderr io.Writer, + ) int { + _, _ = stderr.Write([]byte(argumentErrorTriggerStderr)) + return 1 + } + + var stdout bytes.Buffer + var stderr bytes.Buffer + code := runWaitForPausePoint(context.Background(), unityipc.Connection{}, waitForPausePointOptions{ + id: "jump", + timeoutSeconds: 60, + timeout: 10 * time.Second, + triggerCommand: "simulate-keyboard", + triggerArgs: []string{"--action", "Hold"}, + resumePlay: true, + }, &stdout, &stderr) + + if code != 1 { + t.Fatalf("expected failure exit code, got %d with stdout %s", code, stdout.String()) + } + if len(actions) != 1 || actions[0] != "Pause" { + t.Fatalf("expected exactly one Pause request on abort, got %#v", actions) + } + + envelope := parsePausePointErrorEnvelope(t, stderr.Bytes()) + resumeResult, ok := envelope.Error.Details["ResumePlayResult"].(map[string]any) + if !ok { + t.Fatalf("ResumePlayResult detail missing or wrong shape: %#v", envelope.Error.Details) + } + if resumeResult["Resumed"] != true { + t.Fatalf("ResumePlayResult must still report the resume it performed: %#v", resumeResult) + } + if resumeResult["Repaused"] != true { + t.Fatalf("ResumePlayResult must report the re-pause performed on abort: %#v", resumeResult) + } + if _, hasError := resumeResult["RepauseError"]; hasError { + t.Fatalf("expected no RepauseError for a successful re-pause: %#v", resumeResult) + } +} + +// Verifies a failed re-pause is reported rather than silently dropped, since gameplay then keeps +// running and can still consume the preserved marker. +func TestRunWaitForPausePointReportsRepauseFailure(t *testing.T) { + originalQuery := queryPausePointStatus + originalDispatch := dispatchPausePointTriggerCommand + originalResume := resumePlayModeForPausePoint + originalSend := sendControlPlayModeForPausePoint + originalPoll := pausePointStatusPoll + pausePointStatusPoll = time.Millisecond + defer func() { + queryPausePointStatus = originalQuery + dispatchPausePointTriggerCommand = originalDispatch + resumePlayModeForPausePoint = originalResume + sendControlPlayModeForPausePoint = originalSend + pausePointStatusPoll = originalPoll + }() + + queryPausePointStatus = func( + ctx context.Context, + connection unityipc.Connection, + id string, + ) (pausePointStatusResponse, error) { + return pausePointArmedStatusResponse(id), nil + } + resumePlayModeForPausePoint = func( + ctx context.Context, + connection unityipc.Connection, + ) pausePointResumePlayResult { + return pausePointResumePlayResult{WasPaused: true, Resumed: true} + } + sendControlPlayModeForPausePoint = func( + ctx context.Context, + connection unityipc.Connection, + action string, + ) (controlPlayModeToolResponse, error) { + return controlPlayModeToolResponse{Success: false, Message: "pause denied"}, nil + } + dispatchPausePointTriggerCommand = func( + ctx context.Context, + connection unityipc.Connection, + command string, + commandArgs []string, + startPath string, + stdout io.Writer, + stderr io.Writer, + ) int { + _, _ = stderr.Write([]byte(argumentErrorTriggerStderr)) + return 1 + } + + var stdout bytes.Buffer + var stderr bytes.Buffer + code := runWaitForPausePoint(context.Background(), unityipc.Connection{}, waitForPausePointOptions{ + id: "jump", + timeoutSeconds: 60, + timeout: 10 * time.Second, + triggerCommand: "simulate-keyboard", + triggerArgs: []string{"--action", "Hold"}, + resumePlay: true, + }, &stdout, &stderr) + + if code != 1 { + t.Fatalf("expected failure exit code, got %d with stdout %s", code, stdout.String()) + } + envelope := parsePausePointErrorEnvelope(t, stderr.Bytes()) + resumeResult, ok := envelope.Error.Details["ResumePlayResult"].(map[string]any) + if !ok { + t.Fatalf("ResumePlayResult detail missing or wrong shape: %#v", envelope.Error.Details) + } + if resumeResult["Repaused"] == true { + t.Fatalf("a denied Pause must not be reported as re-paused: %#v", resumeResult) + } + if repauseError, _ := resumeResult["RepauseError"].(string); !strings.Contains(repauseError, "pause denied") { + t.Fatalf("expected the Pause failure to be reported: %#v", resumeResult) + } +} + +// Verifies a wait that did not resume PlayMode itself never sends Pause on abort: pausing a game +// this command did not resume would be an unrequested side effect. +func TestWaitForPausePointDoesNotRepauseWhenItDidNotResume(t *testing.T) { + originalQuery := queryPausePointStatus + originalDispatch := dispatchPausePointTriggerCommand + originalSend := sendControlPlayModeForPausePoint + originalPoll := pausePointStatusPoll + pausePointStatusPoll = time.Millisecond + defer func() { + queryPausePointStatus = originalQuery + dispatchPausePointTriggerCommand = originalDispatch + sendControlPlayModeForPausePoint = originalSend + pausePointStatusPoll = originalPoll + }() + + queryPausePointStatus = func( + ctx context.Context, + connection unityipc.Connection, + id string, + ) (pausePointStatusResponse, error) { + return pausePointArmedStatusResponse(id), nil + } + sendControlPlayModeForPausePoint = func( + ctx context.Context, + connection unityipc.Connection, + action string, + ) (controlPlayModeToolResponse, error) { + t.Fatalf("unexpected control-play-mode %q request", action) + return controlPlayModeToolResponse{}, nil + } + dispatchPausePointTriggerCommand = func( + ctx context.Context, + connection unityipc.Connection, + command string, + commandArgs []string, + startPath string, + stdout io.Writer, + stderr io.Writer, + ) int { + _, _ = stderr.Write([]byte(argumentErrorTriggerStderr)) + return 1 + } + + _, state, _, resumeResult, err := waitForPausePoint(context.Background(), unityipc.Connection{}, waitForPausePointOptions{ + id: "jump", + timeoutSeconds: 60, + timeout: 10 * time.Second, + triggerCommand: "simulate-keyboard", + triggerArgs: []string{"--action", "Hold"}, + }) + if err != nil { + t.Fatalf("waitForPausePoint failed: %v", err) + } + if state != pausePointWaitStateTriggerFailed { + t.Fatalf("expected trigger_failed state, got %q", state) + } + if resumeResult != nil { + t.Fatalf("expected nil ResumePlayResult when --resume-play was not given, got %#v", resumeResult) + } +} + +// Verifies only an argument-parsing rejection aborts the wait: any other trigger failure (a +// connection drop, a disabled tool, unparseable output) must leave the hit wait running. +func TestPausePointTriggerFailedWithArgumentError(t *testing.T) { + cases := []struct { + name string + result *pausePointTriggerResult + want bool + }{ + { + name: "no trigger result", + result: nil, + want: false, + }, + { + name: "argument rejection on stderr", + result: &pausePointTriggerResult{Completed: true, Error: argumentErrorTriggerStderr}, + want: true, + }, + { + name: "argument rejection carried in the response", + result: &pausePointTriggerResult{ + Completed: true, + Response: []byte(argumentErrorTriggerStderr), + }, + want: true, + }, + { + name: "another error code does not abort", + result: &pausePointTriggerResult{ + Completed: true, + Error: `{"Success":false,"Error":{"ErrorCode":"UNITY_NOT_REACHABLE","Phase":"connection"}}`, + }, + want: false, + }, + { + name: "unparseable error does not abort", + result: &pausePointTriggerResult{Completed: true, Error: "unknown command: bogus-command"}, + want: false, + }, + { + name: "successful trigger response does not abort", + result: &pausePointTriggerResult{Completed: true, Response: []byte(`{"Success":true}`)}, + want: false, + }, + } + + for _, testCase := range cases { + t.Run(testCase.name, func(t *testing.T) { + if got := pausePointTriggerFailedWithArgumentError(testCase.result); got != testCase.want { + t.Fatalf("pausePointTriggerFailedWithArgumentError mismatch: got %v, want %v", got, testCase.want) + } + }) + } +} diff --git a/cli/project-runner/shared-inputs-stamp.json b/cli/project-runner/shared-inputs-stamp.json index 9894200bf5..95962c26d1 100644 --- a/cli/project-runner/shared-inputs-stamp.json +++ b/cli/project-runner/shared-inputs-stamp.json @@ -1,4 +1,4 @@ { "schemaVersion": 1, - "sharedInputsHash": "d40ac2627a8c1782cc77db9c76d45054df44a20e" + "sharedInputsHash": "504ccc5a729153bc12482f4e8eab73330fdf3f36" } From 831c1b7a66523d14f6815962b596df97eecc766f Mon Sep 17 00:00:00 2001 From: hatayama Date: Mon, 27 Jul 2026 00:23:23 +0900 Subject: [PATCH 2/2] Abandon the wait for a mistyped trigger command name too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .agents/skills/uloop-pause-point/SKILL.md | 2 +- .../references/fast-progressing-games.md | 2 +- .claude/skills/uloop-pause-point/SKILL.md | 2 +- .../references/fast-progressing-games.md | 2 +- .../CliOnlyTools~/PausePoint/Skill/SKILL.md | 2 +- .../references/fast-progressing-games.md | 2 +- cli/common/errors/error_envelope.go | 4 +- cli/common/errors/error_envelope_test.go | 2 +- cli/dispatcher/shared-inputs-stamp.json | 2 +- .../projectrunner/pause_point_errors.go | 14 +- .../projectrunner/pause_point_trigger.go | 32 ++-- .../projectrunner/pause_point_wait_poll.go | 6 +- .../pause_point_wait_poll_test.go | 179 ++++++++++++++++-- cli/project-runner/shared-inputs-stamp.json | 2 +- 14 files changed, 203 insertions(+), 50 deletions(-) diff --git a/.agents/skills/uloop-pause-point/SKILL.md b/.agents/skills/uloop-pause-point/SKILL.md index 7c5eaf280d..68702eb471 100644 --- a/.agents/skills/uloop-pause-point/SKILL.md +++ b/.agents/skills/uloop-pause-point/SKILL.md @@ -18,7 +18,7 @@ uloop enable-pause-point --file Assets/Scripts/Enemy.cs --line 42 --timeout-seco Digit keys are `Digit0`-`Digit9` or `Numpad0`-`Numpad9` — bare `0`-`9` is rejected. -Before writing a `--trigger` command that differs from the example, load the skill of the tool you are about to trigger. `--trigger` runs a single uloop subcommand in-process only after the marker's arming is confirmed, so the input cannot land before arming and nothing needs to run in the background. One race does remain: the marker itself can hit before the trigger executes (for example on a line that runs every frame), in which case the trigger is rejected because PlayMode is already paused and runs nothing — check `TriggerResult` before treating such a hit as input-driven. The hit response additionally carries `TriggerResult` with the triggered command's own response (or, when the trigger was skipped, `Completed: false` and the reason in `Error`). The trigger string cannot name another pause-point wait (`await-pause-point`/`enable-pause-point`) and cannot pass `--project-path` — the enclosing command's project is used. `await-pause-point --id --trigger ...` accepts the same flag for a marker enabled earlier. Both commands also accept `--resume-play` — see Fast-Progressing Games. +Before writing a `--trigger` command that differs from the example, load the skill of the tool you are about to trigger. `--trigger` runs a single uloop subcommand in-process only after the marker's arming is confirmed, so the input cannot land before arming and nothing needs to run in the background. One race does remain: the marker itself can hit before the trigger executes (for example on a line that runs every frame), in which case the trigger is rejected because PlayMode is already paused and runs nothing — check `TriggerResult` before treating such a hit as input-driven. If the trigger command itself is rejected before it runs — its argument parsing fails (`INVALID_ARGUMENT`) or the command name is unknown (`UNKNOWN_COMMAND`) — the wait is abandoned immediately with a `PAUSE_POINT_TRIGGER_FAILED` error instead of waiting out `--timeout-seconds`: the marker stays armed, a PlayMode resumed by `--resume-play` is paused again, and `Error.NextActions` carries the recovery commands — fix the trigger value and re-run the same command. The hit response additionally carries `TriggerResult` with the triggered command's own response (or, when the trigger was skipped, `Completed: false` and the reason in `Error`). The trigger string cannot name another pause-point wait (`await-pause-point`/`enable-pause-point`) and cannot pass `--project-path` — the enclosing command's project is used. `await-pause-point --id --trigger ...` accepts the same flag for a marker enabled earlier. Both commands also accept `--resume-play` — see Fast-Progressing Games. When the game reaches the line on its own, omit `--trigger`. Fall back to split steps only when the triggering action is not a single uloop command (several inputs in sequence, an external event): run `enable-pause-point` without `--await` in the foreground (its response returning is the arm confirmation), then start `uloop await-pause-point --id ` in the background, then send the inputs. Do not approximate arm-waiting with a fixed sleep after a backgrounded enable. diff --git a/.agents/skills/uloop-pause-point/references/fast-progressing-games.md b/.agents/skills/uloop-pause-point/references/fast-progressing-games.md index 4a1482ca18..60017b8600 100644 --- a/.agents/skills/uloop-pause-point/references/fast-progressing-games.md +++ b/.agents/skills/uloop-pause-point/references/fast-progressing-games.md @@ -22,7 +22,7 @@ Digit keys are `Digit0`-`Digit9` or `Numpad0`-`Numpad9` — bare `0`-`9` is reje ## --resume-play Semantics -`--resume-play` runs after the marker's arming is confirmed and before `--trigger` is dispatched: it resumes PlayMode only when PlayMode is actually paused, and reports what it did in `ResumePlayResult` (`WasPaused` / `Resumed` / `Error`). If the resume fails, the trigger is not dispatched and `TriggerResult.Error` says so. When the game reaches the line on its own after resuming (gravity, physics), omit `--trigger` and keep `--resume-play`. +`--resume-play` runs after the marker's arming is confirmed and before `--trigger` is dispatched: it resumes PlayMode only when PlayMode is actually paused, and reports what it did in `ResumePlayResult` (`WasPaused` / `Resumed` / `Error`; an abandoned wait adds `Repaused` / `RepauseError`). If the resume fails, the trigger is not dispatched and `TriggerResult.Error` says so. If the trigger itself is rejected before it runs, the wait is abandoned and the resume is undone: `Repaused: true` (or `RepauseError`) reports PlayMode being paused again, so gameplay cannot consume the preserved marker while the trigger value is being fixed. When the game reaches the line on its own after resuming (gravity, physics), omit `--trigger` and keep `--resume-play`. ## Do Not Use Time.timeScale = 0 diff --git a/.claude/skills/uloop-pause-point/SKILL.md b/.claude/skills/uloop-pause-point/SKILL.md index 7c5eaf280d..68702eb471 100644 --- a/.claude/skills/uloop-pause-point/SKILL.md +++ b/.claude/skills/uloop-pause-point/SKILL.md @@ -18,7 +18,7 @@ uloop enable-pause-point --file Assets/Scripts/Enemy.cs --line 42 --timeout-seco Digit keys are `Digit0`-`Digit9` or `Numpad0`-`Numpad9` — bare `0`-`9` is rejected. -Before writing a `--trigger` command that differs from the example, load the skill of the tool you are about to trigger. `--trigger` runs a single uloop subcommand in-process only after the marker's arming is confirmed, so the input cannot land before arming and nothing needs to run in the background. One race does remain: the marker itself can hit before the trigger executes (for example on a line that runs every frame), in which case the trigger is rejected because PlayMode is already paused and runs nothing — check `TriggerResult` before treating such a hit as input-driven. The hit response additionally carries `TriggerResult` with the triggered command's own response (or, when the trigger was skipped, `Completed: false` and the reason in `Error`). The trigger string cannot name another pause-point wait (`await-pause-point`/`enable-pause-point`) and cannot pass `--project-path` — the enclosing command's project is used. `await-pause-point --id --trigger ...` accepts the same flag for a marker enabled earlier. Both commands also accept `--resume-play` — see Fast-Progressing Games. +Before writing a `--trigger` command that differs from the example, load the skill of the tool you are about to trigger. `--trigger` runs a single uloop subcommand in-process only after the marker's arming is confirmed, so the input cannot land before arming and nothing needs to run in the background. One race does remain: the marker itself can hit before the trigger executes (for example on a line that runs every frame), in which case the trigger is rejected because PlayMode is already paused and runs nothing — check `TriggerResult` before treating such a hit as input-driven. If the trigger command itself is rejected before it runs — its argument parsing fails (`INVALID_ARGUMENT`) or the command name is unknown (`UNKNOWN_COMMAND`) — the wait is abandoned immediately with a `PAUSE_POINT_TRIGGER_FAILED` error instead of waiting out `--timeout-seconds`: the marker stays armed, a PlayMode resumed by `--resume-play` is paused again, and `Error.NextActions` carries the recovery commands — fix the trigger value and re-run the same command. The hit response additionally carries `TriggerResult` with the triggered command's own response (or, when the trigger was skipped, `Completed: false` and the reason in `Error`). The trigger string cannot name another pause-point wait (`await-pause-point`/`enable-pause-point`) and cannot pass `--project-path` — the enclosing command's project is used. `await-pause-point --id --trigger ...` accepts the same flag for a marker enabled earlier. Both commands also accept `--resume-play` — see Fast-Progressing Games. When the game reaches the line on its own, omit `--trigger`. Fall back to split steps only when the triggering action is not a single uloop command (several inputs in sequence, an external event): run `enable-pause-point` without `--await` in the foreground (its response returning is the arm confirmation), then start `uloop await-pause-point --id ` in the background, then send the inputs. Do not approximate arm-waiting with a fixed sleep after a backgrounded enable. diff --git a/.claude/skills/uloop-pause-point/references/fast-progressing-games.md b/.claude/skills/uloop-pause-point/references/fast-progressing-games.md index 4a1482ca18..60017b8600 100644 --- a/.claude/skills/uloop-pause-point/references/fast-progressing-games.md +++ b/.claude/skills/uloop-pause-point/references/fast-progressing-games.md @@ -22,7 +22,7 @@ Digit keys are `Digit0`-`Digit9` or `Numpad0`-`Numpad9` — bare `0`-`9` is reje ## --resume-play Semantics -`--resume-play` runs after the marker's arming is confirmed and before `--trigger` is dispatched: it resumes PlayMode only when PlayMode is actually paused, and reports what it did in `ResumePlayResult` (`WasPaused` / `Resumed` / `Error`). If the resume fails, the trigger is not dispatched and `TriggerResult.Error` says so. When the game reaches the line on its own after resuming (gravity, physics), omit `--trigger` and keep `--resume-play`. +`--resume-play` runs after the marker's arming is confirmed and before `--trigger` is dispatched: it resumes PlayMode only when PlayMode is actually paused, and reports what it did in `ResumePlayResult` (`WasPaused` / `Resumed` / `Error`; an abandoned wait adds `Repaused` / `RepauseError`). If the resume fails, the trigger is not dispatched and `TriggerResult.Error` says so. If the trigger itself is rejected before it runs, the wait is abandoned and the resume is undone: `Repaused: true` (or `RepauseError`) reports PlayMode being paused again, so gameplay cannot consume the preserved marker while the trigger value is being fixed. When the game reaches the line on its own after resuming (gravity, physics), omit `--trigger` and keep `--resume-play`. ## Do Not Use Time.timeScale = 0 diff --git a/Packages/src/Editor/CliOnlyTools~/PausePoint/Skill/SKILL.md b/Packages/src/Editor/CliOnlyTools~/PausePoint/Skill/SKILL.md index 7c5eaf280d..68702eb471 100644 --- a/Packages/src/Editor/CliOnlyTools~/PausePoint/Skill/SKILL.md +++ b/Packages/src/Editor/CliOnlyTools~/PausePoint/Skill/SKILL.md @@ -18,7 +18,7 @@ uloop enable-pause-point --file Assets/Scripts/Enemy.cs --line 42 --timeout-seco Digit keys are `Digit0`-`Digit9` or `Numpad0`-`Numpad9` — bare `0`-`9` is rejected. -Before writing a `--trigger` command that differs from the example, load the skill of the tool you are about to trigger. `--trigger` runs a single uloop subcommand in-process only after the marker's arming is confirmed, so the input cannot land before arming and nothing needs to run in the background. One race does remain: the marker itself can hit before the trigger executes (for example on a line that runs every frame), in which case the trigger is rejected because PlayMode is already paused and runs nothing — check `TriggerResult` before treating such a hit as input-driven. The hit response additionally carries `TriggerResult` with the triggered command's own response (or, when the trigger was skipped, `Completed: false` and the reason in `Error`). The trigger string cannot name another pause-point wait (`await-pause-point`/`enable-pause-point`) and cannot pass `--project-path` — the enclosing command's project is used. `await-pause-point --id --trigger ...` accepts the same flag for a marker enabled earlier. Both commands also accept `--resume-play` — see Fast-Progressing Games. +Before writing a `--trigger` command that differs from the example, load the skill of the tool you are about to trigger. `--trigger` runs a single uloop subcommand in-process only after the marker's arming is confirmed, so the input cannot land before arming and nothing needs to run in the background. One race does remain: the marker itself can hit before the trigger executes (for example on a line that runs every frame), in which case the trigger is rejected because PlayMode is already paused and runs nothing — check `TriggerResult` before treating such a hit as input-driven. If the trigger command itself is rejected before it runs — its argument parsing fails (`INVALID_ARGUMENT`) or the command name is unknown (`UNKNOWN_COMMAND`) — the wait is abandoned immediately with a `PAUSE_POINT_TRIGGER_FAILED` error instead of waiting out `--timeout-seconds`: the marker stays armed, a PlayMode resumed by `--resume-play` is paused again, and `Error.NextActions` carries the recovery commands — fix the trigger value and re-run the same command. The hit response additionally carries `TriggerResult` with the triggered command's own response (or, when the trigger was skipped, `Completed: false` and the reason in `Error`). The trigger string cannot name another pause-point wait (`await-pause-point`/`enable-pause-point`) and cannot pass `--project-path` — the enclosing command's project is used. `await-pause-point --id --trigger ...` accepts the same flag for a marker enabled earlier. Both commands also accept `--resume-play` — see Fast-Progressing Games. When the game reaches the line on its own, omit `--trigger`. Fall back to split steps only when the triggering action is not a single uloop command (several inputs in sequence, an external event): run `enable-pause-point` without `--await` in the foreground (its response returning is the arm confirmation), then start `uloop await-pause-point --id ` in the background, then send the inputs. Do not approximate arm-waiting with a fixed sleep after a backgrounded enable. diff --git a/Packages/src/Editor/CliOnlyTools~/PausePoint/Skill/references/fast-progressing-games.md b/Packages/src/Editor/CliOnlyTools~/PausePoint/Skill/references/fast-progressing-games.md index 4a1482ca18..60017b8600 100644 --- a/Packages/src/Editor/CliOnlyTools~/PausePoint/Skill/references/fast-progressing-games.md +++ b/Packages/src/Editor/CliOnlyTools~/PausePoint/Skill/references/fast-progressing-games.md @@ -22,7 +22,7 @@ Digit keys are `Digit0`-`Digit9` or `Numpad0`-`Numpad9` — bare `0`-`9` is reje ## --resume-play Semantics -`--resume-play` runs after the marker's arming is confirmed and before `--trigger` is dispatched: it resumes PlayMode only when PlayMode is actually paused, and reports what it did in `ResumePlayResult` (`WasPaused` / `Resumed` / `Error`). If the resume fails, the trigger is not dispatched and `TriggerResult.Error` says so. When the game reaches the line on its own after resuming (gravity, physics), omit `--trigger` and keep `--resume-play`. +`--resume-play` runs after the marker's arming is confirmed and before `--trigger` is dispatched: it resumes PlayMode only when PlayMode is actually paused, and reports what it did in `ResumePlayResult` (`WasPaused` / `Resumed` / `Error`; an abandoned wait adds `Repaused` / `RepauseError`). If the resume fails, the trigger is not dispatched and `TriggerResult.Error` says so. If the trigger itself is rejected before it runs, the wait is abandoned and the resume is undone: `Repaused: true` (or `RepauseError`) reports PlayMode being paused again, so gameplay cannot consume the preserved marker while the trigger value is being fixed. When the game reaches the line on its own after resuming (gravity, physics), omit `--trigger` and keep `--resume-play`. ## Do Not Use Time.timeScale = 0 diff --git a/cli/common/errors/error_envelope.go b/cli/common/errors/error_envelope.go index 719e856200..986187a2ad 100644 --- a/cli/common/errors/error_envelope.go +++ b/cli/common/errors/error_envelope.go @@ -11,7 +11,7 @@ import ( const ( ErrorCodeInvalidArgument = "INVALID_ARGUMENT" - errorCodeUnknownCommand = "UNKNOWN_COMMAND" + ErrorCodeUnknownCommand = "UNKNOWN_COMMAND" errorCodeProjectNotFound = "PROJECT_NOT_FOUND" ErrorCodeUnityNotReachable = "UNITY_NOT_REACHABLE" ErrorCodeUnityStartupTimeout = "UNITY_STARTUP_TIMEOUT" @@ -260,7 +260,7 @@ func unityServerNotRespondingAfterDispatchError(err UnityServerNotRespondingErro func UnknownCommandError(command string, availableCommands []string, context ErrorContext) CLIError { return CLIError{ - ErrorCode: errorCodeUnknownCommand, + ErrorCode: ErrorCodeUnknownCommand, Phase: ErrorPhaseDispatch, Message: "Unknown command: " + command, Retryable: false, diff --git a/cli/common/errors/error_envelope_test.go b/cli/common/errors/error_envelope_test.go index 438f04798b..a788e4eae7 100644 --- a/cli/common/errors/error_envelope_test.go +++ b/cli/common/errors/error_envelope_test.go @@ -444,7 +444,7 @@ func TestUnknownCommandErrorIncludesAvailableCommands(t *testing.T) { ErrorContext{ProjectRoot: "/tmp/MyProject"}, ) - if cliErr.ErrorCode != errorCodeUnknownCommand { + if cliErr.ErrorCode != ErrorCodeUnknownCommand { t.Fatalf("error code mismatch: %#v", cliErr) } available, ok := cliErr.Details["AvailableCommands"].([]string) diff --git a/cli/dispatcher/shared-inputs-stamp.json b/cli/dispatcher/shared-inputs-stamp.json index 38aa1cf2a6..e665d3dbb9 100644 --- a/cli/dispatcher/shared-inputs-stamp.json +++ b/cli/dispatcher/shared-inputs-stamp.json @@ -1,4 +1,4 @@ { "schemaVersion": 1, - "sharedInputsHash": "7bf1869ee95b32433e74738131488c4343ec8a80" + "sharedInputsHash": "8a863e4239e16efaf67ef4b7a2dfa7ed17a36b65" } diff --git a/cli/project-runner/internal/projectrunner/pause_point_errors.go b/cli/project-runner/internal/projectrunner/pause_point_errors.go index 5351756cff..a1484486ca 100644 --- a/cli/project-runner/internal/projectrunner/pause_point_errors.go +++ b/cli/project-runner/internal/projectrunner/pause_point_errors.go @@ -42,16 +42,18 @@ func pausePointWaitError( case pausePointWaitStateTriggerFailed: triggerFailedError := pausePointStateError( clierrors.ErrorCodePausePointTriggerFailed, - "The --trigger command was rejected by its own argument parsing, so it never ran and the "+ - "wait was abandoned instead of waiting out the remaining timeout. The marker is still "+ - "armed: see Details.TriggerResult for the rejection and Details.RemainingMilliseconds "+ - "for how long it stays armed.", + "The --trigger command was rejected before it ran (argument parsing or an unknown command "+ + "name), so the wait was abandoned instead of waiting out the remaining timeout. This "+ + "command did not clear the marker: see Details.TriggerResult for the rejection and "+ + "Details.RemainingMilliseconds for how long the marker stays armed. A zero "+ + "RemainingMilliseconds with an empty Details.Status means the final status re-read "+ + "failed — run pause-point-status to confirm the marker.", projectRoot, options, response, // Retrying the identical command reproduces the same rejection: the trigger value has to - // change first. Reporting this as retryable is what made the original incident waste a - // full timeout window on a permanent failure. + // change first. Reporting a permanent failure as retryable is what made the original + // incident waste a full timeout window on it. false) triggerFailedError.NextActions = pausePointTriggerFailedNextActions(options.id) return triggerFailedError diff --git a/cli/project-runner/internal/projectrunner/pause_point_trigger.go b/cli/project-runner/internal/projectrunner/pause_point_trigger.go index 4062f6362d..87a4cbdcab 100644 --- a/cli/project-runner/internal/projectrunner/pause_point_trigger.go +++ b/cli/project-runner/internal/projectrunner/pause_point_trigger.go @@ -39,35 +39,31 @@ type pausePointTriggerResult struct { Error string `json:"Error,omitempty"` } -// pausePointTriggerFailedWithArgumentError reports whether the trigger command was rejected by -// argument parsing, the one failure that makes waiting out the marker's remaining lifetime -// pointless: the trigger performed no action, so the marker can never be hit by it. +// pausePointTriggerRejectedBeforeExecution reports whether the trigger command was permanently +// rejected before it executed anything: its arguments did not parse, or its command name does not +// exist. Either way the trigger performed no action, so the marker can never be hit by it and +// waiting out the marker's remaining lifetime cannot change the outcome. Retrying the identical +// command reproduces the same rejection, so the value has to change first. // // Deliberately narrow. A connection drop, a disabled tool, or unparseable output must not abort the // wait — the marker may still be hit by the game itself, and abandoning that wait would turn a -// recoverable situation into a lost hit. Anything this function cannot positively identify as an -// argument rejection keeps the wait running. -func pausePointTriggerFailedWithArgumentError(result *pausePointTriggerResult) bool { +// recoverable situation into a lost hit. Anything this function cannot positively identify as a +// pre-execution rejection keeps the wait running. +// +// Only the dispatched command's stderr is inspected, because that is where every error envelope is +// written; a Unity-side rejection arriving on stdout has no error code to match on. +func pausePointTriggerRejectedBeforeExecution(result *pausePointTriggerResult) bool { if result == nil { return false } - // Error carries what the dispatched command wrote to stderr, which is where a CLI-side argument - // rejection lands; Response carries its stdout, which is where a Unity-side argument rejection - // arrives with the same code. - return isPausePointInvalidArgumentPayload([]byte(result.Error)) || - isPausePointInvalidArgumentPayload(result.Response) -} - -func isPausePointInvalidArgumentPayload(payload []byte) bool { - trimmed := bytes.TrimSpace(payload) + trimmed := bytes.TrimSpace([]byte(result.Error)) if len(trimmed) == 0 { return false } envelope := struct { - ErrorCode string `json:"ErrorCode"` - Error struct { + Error struct { ErrorCode string `json:"ErrorCode"` } `json:"Error"` }{} @@ -76,7 +72,7 @@ func isPausePointInvalidArgumentPayload(payload []byte) bool { } return envelope.Error.ErrorCode == clierrors.ErrorCodeInvalidArgument || - envelope.ErrorCode == clierrors.ErrorCodeInvalidArgument + envelope.Error.ErrorCode == clierrors.ErrorCodeUnknownCommand } // parsePausePointTriggerCommand splits a --trigger value into a command name and its arguments, diff --git a/cli/project-runner/internal/projectrunner/pause_point_wait_poll.go b/cli/project-runner/internal/projectrunner/pause_point_wait_poll.go index f553aded93..c5a835ce35 100644 --- a/cli/project-runner/internal/projectrunner/pause_point_wait_poll.go +++ b/cli/project-runner/internal/projectrunner/pause_point_wait_poll.go @@ -21,8 +21,8 @@ const ( pausePointWaitStateExpired pausePointWaitState = "expired" pausePointWaitStateCleared pausePointWaitState = "cleared" - // pausePointWaitStateTriggerFailed means the wait was abandoned because the --trigger command - // was rejected by argument parsing, so it never performed the action the marker was waiting for. + // pausePointWaitStateTriggerFailed means the wait was abandoned because the --trigger command was + // rejected before it executed anything, so it never performed the action the marker was waiting for. // Why not reuse the timeout state: the timeout path unconditionally clears the marker, which // would force a re-enable just to retry a corrected trigger, and it reports a "not hit within // %ds" message with a PlayMode diagnosis that describes neither what happened nor what to fix. @@ -192,7 +192,7 @@ func waitForPausePointStatus( // buffered value, and the caller reuses the result received here instead of joining. triggerResult = result triggerDone = nil - if pausePointTriggerFailedWithArgumentError(result) { + if pausePointTriggerRejectedBeforeExecution(result) { abortResponse, abortState := abortPausePointWaitAfterTriggerRejection( ctx, connection, options.id, lastResponse) return abortResponse, abortState, triggerResult, nil diff --git a/cli/project-runner/internal/projectrunner/pause_point_wait_poll_test.go b/cli/project-runner/internal/projectrunner/pause_point_wait_poll_test.go index 2ce5eca4c9..63b88d220a 100644 --- a/cli/project-runner/internal/projectrunner/pause_point_wait_poll_test.go +++ b/cli/project-runner/internal/projectrunner/pause_point_wait_poll_test.go @@ -20,6 +20,12 @@ const argumentErrorTriggerStderr = `{"Success":false,"Error":{"ErrorCode":"INVAL `"Phase":"argument_parsing","Message":"Invalid value for --action: \"Hold\"","Retryable":false,` + `"SafeToRetry":false,"NextActions":["Pass one of: Press, KeyDown, KeyUp, ReleaseAll."]}}` +// unknownCommandTriggerStderr is the error envelope a mistyped trigger command name produces. It +// never reaches Unity at all, so it is as permanent a rejection as a bad argument value. +const unknownCommandTriggerStderr = `{"Success":false,"Error":{"ErrorCode":"UNKNOWN_COMMAND",` + + `"Phase":"dispatch","Message":"Unknown command: simulate-keybord","Retryable":false,` + + `"SafeToRetry":false,"NextActions":["Run ` + "`uloop list`" + ` to see the available commands."]}}` + // pausePointArmedStatusResponse is an armed-but-never-hit marker with a lifetime long enough that // RemainingMilliseconds stays positive, so the abort response can be asserted to carry it. func pausePointArmedStatusResponse(id string) pausePointStatusResponse { @@ -192,6 +198,83 @@ func TestRunWaitForPausePointKeepsMarkerWhenTriggerRejectsArguments(t *testing.T } } +// Verifies a mistyped trigger command name aborts the wait and keeps the marker armed, the same as +// a rejected argument value: the command never reached Unity, so the marker cannot be hit by it. +func TestRunWaitForPausePointKeepsMarkerWhenTriggerCommandIsUnknown(t *testing.T) { + originalQuery := queryPausePointStatus + originalClear := clearPausePointStatus + originalDispatch := dispatchPausePointTriggerCommand + originalPoll := pausePointStatusPoll + pausePointStatusPoll = time.Millisecond + defer func() { + queryPausePointStatus = originalQuery + clearPausePointStatus = originalClear + dispatchPausePointTriggerCommand = originalDispatch + pausePointStatusPoll = originalPoll + }() + + queryPausePointStatus = func( + ctx context.Context, + connection unityipc.Connection, + id string, + ) (pausePointStatusResponse, error) { + return pausePointArmedStatusResponse(id), nil + } + clearPausePointStatus = func( + ctx context.Context, + connection unityipc.Connection, + id string, + ) (pausePointStatusResponse, error) { + t.Fatal("clearPausePointStatus must not be called when the trigger command name was unknown") + return pausePointStatusResponse{}, nil + } + dispatchPausePointTriggerCommand = func( + ctx context.Context, + connection unityipc.Connection, + command string, + commandArgs []string, + startPath string, + stdout io.Writer, + stderr io.Writer, + ) int { + _, _ = stderr.Write([]byte(unknownCommandTriggerStderr)) + return 1 + } + + var stdout bytes.Buffer + var stderr bytes.Buffer + startedAt := time.Now() + code := runWaitForPausePoint(context.Background(), unityipc.Connection{}, waitForPausePointOptions{ + id: "jump", + timeoutSeconds: 60, + timeout: 10 * time.Second, + triggerCommand: "simulate-keybord", + triggerArgs: []string{"--action", "Press", "--key", "Space"}, + }, &stdout, &stderr) + elapsed := time.Since(startedAt) + + if code != 1 { + t.Fatalf("expected failure exit code, got %d with stdout %s", code, stdout.String()) + } + if elapsed >= 5*time.Second { + t.Fatalf("expected an early abort, waited %v of a 10s timeout", elapsed) + } + envelope := parsePausePointErrorEnvelope(t, stderr.Bytes()) + if envelope.Error.ErrorCode != clierrors.ErrorCodePausePointTriggerFailed { + t.Fatalf("error code mismatch: %#v", envelope.Error) + } + if envelope.Error.Retryable || envelope.Error.SafeToRetry { + t.Fatalf("a mistyped command name is not fixed by retrying the same command: %#v", envelope.Error) + } + triggerResult, ok := envelope.Error.Details["TriggerResult"].(map[string]any) + if !ok { + t.Fatalf("TriggerResult detail missing or wrong shape: %#v", envelope.Error.Details) + } + if errorText, _ := triggerResult["Error"].(string); !strings.Contains(errorText, "UNKNOWN_COMMAND") { + t.Fatalf("TriggerResult must carry the trigger's own rejection: %#v", triggerResult) + } +} + // Verifies a trigger that completes normally mid-wait never aborts the wait and is still reported // once, so receiving it inside the poll loop does not regress the existing join behavior. func TestWaitForPausePointDoesNotAbortWhenTriggerSucceeds(t *testing.T) { @@ -487,6 +570,80 @@ func TestRunWaitForPausePointReportsRepauseFailure(t *testing.T) { } } +// Verifies --resume-play that found PlayMode already unpaused sends no Pause on abort: this command +// resumed nothing, so pausing a game the caller was running would be an unrequested side effect. +func TestWaitForPausePointDoesNotRepauseWhenResumeWasANoOp(t *testing.T) { + originalQuery := queryPausePointStatus + originalDispatch := dispatchPausePointTriggerCommand + originalResume := resumePlayModeForPausePoint + originalSend := sendControlPlayModeForPausePoint + originalPoll := pausePointStatusPoll + pausePointStatusPoll = time.Millisecond + defer func() { + queryPausePointStatus = originalQuery + dispatchPausePointTriggerCommand = originalDispatch + resumePlayModeForPausePoint = originalResume + sendControlPlayModeForPausePoint = originalSend + pausePointStatusPoll = originalPoll + }() + + queryPausePointStatus = func( + ctx context.Context, + connection unityipc.Connection, + id string, + ) (pausePointStatusResponse, error) { + return pausePointArmedStatusResponse(id), nil + } + resumePlayModeForPausePoint = func( + ctx context.Context, + connection unityipc.Connection, + ) pausePointResumePlayResult { + // Mirrors the real contract: Play is only sent when Status reports IsPaused=true. + return pausePointResumePlayResult{WasPaused: false, Resumed: false} + } + sendControlPlayModeForPausePoint = func( + ctx context.Context, + connection unityipc.Connection, + action string, + ) (controlPlayModeToolResponse, error) { + t.Fatalf("unexpected control-play-mode %q request after a no-op resume", action) + return controlPlayModeToolResponse{}, nil + } + dispatchPausePointTriggerCommand = func( + ctx context.Context, + connection unityipc.Connection, + command string, + commandArgs []string, + startPath string, + stdout io.Writer, + stderr io.Writer, + ) int { + _, _ = stderr.Write([]byte(argumentErrorTriggerStderr)) + return 1 + } + + _, state, _, resumeResult, err := waitForPausePoint(context.Background(), unityipc.Connection{}, waitForPausePointOptions{ + id: "jump", + timeoutSeconds: 60, + timeout: 10 * time.Second, + triggerCommand: "simulate-keyboard", + triggerArgs: []string{"--action", "Hold"}, + resumePlay: true, + }) + if err != nil { + t.Fatalf("waitForPausePoint failed: %v", err) + } + if state != pausePointWaitStateTriggerFailed { + t.Fatalf("expected trigger_failed state, got %q", state) + } + if resumeResult == nil || resumeResult.Resumed { + t.Fatalf("expected a no-op ResumePlayResult, got %#v", resumeResult) + } + if resumeResult.Repaused || resumeResult.RepauseError != "" { + t.Fatalf("expected no re-pause to be reported, got %#v", resumeResult) + } +} + // Verifies a wait that did not resume PlayMode itself never sends Pause on abort: pausing a game // this command did not resume would be an unrequested side effect. func TestWaitForPausePointDoesNotRepauseWhenItDidNotResume(t *testing.T) { @@ -548,9 +705,10 @@ func TestWaitForPausePointDoesNotRepauseWhenItDidNotResume(t *testing.T) { } } -// Verifies only an argument-parsing rejection aborts the wait: any other trigger failure (a -// connection drop, a disabled tool, unparseable output) must leave the hit wait running. -func TestPausePointTriggerFailedWithArgumentError(t *testing.T) { +// Verifies only a pre-execution rejection (bad arguments or an unknown command name) aborts the +// wait: any other trigger failure (a connection drop, a disabled tool, unparseable output) must +// leave the hit wait running. +func TestPausePointTriggerRejectedBeforeExecution(t *testing.T) { cases := []struct { name string result *pausePointTriggerResult @@ -562,17 +720,14 @@ func TestPausePointTriggerFailedWithArgumentError(t *testing.T) { want: false, }, { - name: "argument rejection on stderr", + name: "argument rejection", result: &pausePointTriggerResult{Completed: true, Error: argumentErrorTriggerStderr}, want: true, }, { - name: "argument rejection carried in the response", - result: &pausePointTriggerResult{ - Completed: true, - Response: []byte(argumentErrorTriggerStderr), - }, - want: true, + name: "unknown command name rejection", + result: &pausePointTriggerResult{Completed: true, Error: unknownCommandTriggerStderr}, + want: true, }, { name: "another error code does not abort", @@ -596,8 +751,8 @@ func TestPausePointTriggerFailedWithArgumentError(t *testing.T) { for _, testCase := range cases { t.Run(testCase.name, func(t *testing.T) { - if got := pausePointTriggerFailedWithArgumentError(testCase.result); got != testCase.want { - t.Fatalf("pausePointTriggerFailedWithArgumentError mismatch: got %v, want %v", got, testCase.want) + if got := pausePointTriggerRejectedBeforeExecution(testCase.result); got != testCase.want { + t.Fatalf("pausePointTriggerRejectedBeforeExecution mismatch: got %v, want %v", got, testCase.want) } }) } diff --git a/cli/project-runner/shared-inputs-stamp.json b/cli/project-runner/shared-inputs-stamp.json index 95962c26d1..b6cd063854 100644 --- a/cli/project-runner/shared-inputs-stamp.json +++ b/cli/project-runner/shared-inputs-stamp.json @@ -1,4 +1,4 @@ { "schemaVersion": 1, - "sharedInputsHash": "504ccc5a729153bc12482f4e8eab73330fdf3f36" + "sharedInputsHash": "962315b78bf904764fa1f75bbd7d6d2ca9ffe092" }