diff --git a/.agents/skills/uloop-pause-point/SKILL.md b/.agents/skills/uloop-pause-point/SKILL.md index 68702eb471..31e64add12 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. 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. +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 — the hit response then carries `TriggerFailed: true` at the top level and a `Warning` explaining that no input reached the game (the triggered command's own response stays in `TriggerResult`), so do not treat 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. @@ -53,8 +53,8 @@ Every hit response embeds `CapturedVariables`: the method's in-scope locals, its - The snapshot is taken **before** the resolved line executes, exactly like an IDE breakpoint on that line. To inspect a value after an assignment, place the pause point on the following line. - `Scope` is `Local`, `Parameter`, `InstanceField`, or `This`. The synthetic `this` entry identifies which instance or GameObject was hit via `UnityObjectPath` and `UnityObjectInstanceId`; `UnityEngine.Object` values carry the same handle fields for follow-up digs with `get-hierarchy`, `find-game-objects`, or `execute-dynamic-code`. - `--captured-variables names` on `await-pause-point`/`pause-point-status` drops every `Value` and keeps `Name`/`Scope`/`TypeName` — use it first on field-heavy classes, then fetch full values with a plain `pause-point-status` call. -- When the response would be dominated by variables you do not need, pass `--captured-variable-names velocity,this` (comma-separated, exact match on `Name`) to keep only those entries; it composes with `--captured-variables full|names`. `CapturedVariablesTruncated` in the response reports truncation at Unity-side capture time and is unrelated to this name filter — it can be `true` even when every requested name was found. -- Pass `--expect 'name=value'` (repeatable; on `await-pause-point` and `enable-pause-point --await`, not `pause-point-status`) to have the CLI compare captured variables against expected values; the response includes an `Expectations` array and `AllExpectationsPassed`, so you do not need to eyeball the JSON. Matching is string equality against the serialized value. +- When the response would be dominated by variables you do not need, pass `--captured-variable-names velocity,this` (comma-separated, exact match on `Name`) to keep only those entries; it composes with `--captured-variables full|names`. `CapturedVariablesTruncated` in the response reports truncation at Unity-side capture time and is unrelated to this name filter — it can be `true` even when every requested name was found. Requested names that matched nothing are listed in `CapturedVariableNamesNotFound`, so a partial match is visible without comparing the response against the request by hand. +- Pass `--expect 'name=value'` (repeatable; on `await-pause-point`, `enable-pause-point --await`, and `pause-point-status`) to have the CLI compare captured variables against expected values; the response includes an `Expectations` array and `AllExpectationsPassed`, so you do not need to eyeball the JSON. Matching is string equality against the serialized value. On `pause-point-status` a marker that has not been hit yet reports each expectation as not found, and the verdict never changes the exit code — a polling loop reads `AllExpectationsPassed`, not the exit status. - Collection values (arrays, `List`, dictionaries, plain objects) render as a JSON preview capped at 10 elements by default. When the elements you need sit past that cap (a 10x20 grid, a long list), re-enable with `--max-preview-elements ` (1–1000). The value set at enable time also caps the previews in every later `pause-point-status` response for that marker — status has no flag to change it. - While Unity is still paused, `UloopPausePoint.TryGetCapturedValue("name")` (and `"this"`) returns live captured references for `execute-dynamic-code`; the return is a `(bool Found, object Value)` tuple, and the holder clears on resume. (file:line marker hits only — id-only markers store no capture) These are **live objects in their frame-completed state, not snapshots** — use them only to dig further into objects that are still alive, never to reconstruct what a value was at the paused line. @@ -95,7 +95,7 @@ For "N frames after the input" (for example, three frames after a key press), ad A pause point hits only when control flow reaches the patched line (or the `Pause(id)` call). `simulate-keyboard` returning `PressEdgeObserved=true` means the input edge was observed, not that your target game logic has reached the pause line yet. -If a `simulate-*` command instead returns a failure whose message says PlayMode is paused, suspect a pause point hit rather than an unrelated failure: an active pause point can make PlayMode paused mid-simulation, and the `simulate-*` call surfaces that as a preflight failure. Check `uloop pause-point-status --id ` first to confirm the hit before treating it as a bug in the simulated action itself. +If a `simulate-*` command instead returns a failure whose message says PlayMode is paused, suspect a pause point hit rather than an unrelated failure: an active pause point can make PlayMode paused mid-simulation, and the `simulate-*` call surfaces that as a preflight failure. The failure response names the responsible marker in `RejectedByActivePausePointId`. Check `uloop pause-point-status --id ` first to confirm the hit before treating it as a bug in the simulated action itself. ## When To Use diff --git a/.claude/skills/uloop-pause-point/SKILL.md b/.claude/skills/uloop-pause-point/SKILL.md index 68702eb471..31e64add12 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. 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. +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 — the hit response then carries `TriggerFailed: true` at the top level and a `Warning` explaining that no input reached the game (the triggered command's own response stays in `TriggerResult`), so do not treat 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. @@ -53,8 +53,8 @@ Every hit response embeds `CapturedVariables`: the method's in-scope locals, its - The snapshot is taken **before** the resolved line executes, exactly like an IDE breakpoint on that line. To inspect a value after an assignment, place the pause point on the following line. - `Scope` is `Local`, `Parameter`, `InstanceField`, or `This`. The synthetic `this` entry identifies which instance or GameObject was hit via `UnityObjectPath` and `UnityObjectInstanceId`; `UnityEngine.Object` values carry the same handle fields for follow-up digs with `get-hierarchy`, `find-game-objects`, or `execute-dynamic-code`. - `--captured-variables names` on `await-pause-point`/`pause-point-status` drops every `Value` and keeps `Name`/`Scope`/`TypeName` — use it first on field-heavy classes, then fetch full values with a plain `pause-point-status` call. -- When the response would be dominated by variables you do not need, pass `--captured-variable-names velocity,this` (comma-separated, exact match on `Name`) to keep only those entries; it composes with `--captured-variables full|names`. `CapturedVariablesTruncated` in the response reports truncation at Unity-side capture time and is unrelated to this name filter — it can be `true` even when every requested name was found. -- Pass `--expect 'name=value'` (repeatable; on `await-pause-point` and `enable-pause-point --await`, not `pause-point-status`) to have the CLI compare captured variables against expected values; the response includes an `Expectations` array and `AllExpectationsPassed`, so you do not need to eyeball the JSON. Matching is string equality against the serialized value. +- When the response would be dominated by variables you do not need, pass `--captured-variable-names velocity,this` (comma-separated, exact match on `Name`) to keep only those entries; it composes with `--captured-variables full|names`. `CapturedVariablesTruncated` in the response reports truncation at Unity-side capture time and is unrelated to this name filter — it can be `true` even when every requested name was found. Requested names that matched nothing are listed in `CapturedVariableNamesNotFound`, so a partial match is visible without comparing the response against the request by hand. +- Pass `--expect 'name=value'` (repeatable; on `await-pause-point`, `enable-pause-point --await`, and `pause-point-status`) to have the CLI compare captured variables against expected values; the response includes an `Expectations` array and `AllExpectationsPassed`, so you do not need to eyeball the JSON. Matching is string equality against the serialized value. On `pause-point-status` a marker that has not been hit yet reports each expectation as not found, and the verdict never changes the exit code — a polling loop reads `AllExpectationsPassed`, not the exit status. - Collection values (arrays, `List`, dictionaries, plain objects) render as a JSON preview capped at 10 elements by default. When the elements you need sit past that cap (a 10x20 grid, a long list), re-enable with `--max-preview-elements ` (1–1000). The value set at enable time also caps the previews in every later `pause-point-status` response for that marker — status has no flag to change it. - While Unity is still paused, `UloopPausePoint.TryGetCapturedValue("name")` (and `"this"`) returns live captured references for `execute-dynamic-code`; the return is a `(bool Found, object Value)` tuple, and the holder clears on resume. (file:line marker hits only — id-only markers store no capture) These are **live objects in their frame-completed state, not snapshots** — use them only to dig further into objects that are still alive, never to reconstruct what a value was at the paused line. @@ -95,7 +95,7 @@ For "N frames after the input" (for example, three frames after a key press), ad A pause point hits only when control flow reaches the patched line (or the `Pause(id)` call). `simulate-keyboard` returning `PressEdgeObserved=true` means the input edge was observed, not that your target game logic has reached the pause line yet. -If a `simulate-*` command instead returns a failure whose message says PlayMode is paused, suspect a pause point hit rather than an unrelated failure: an active pause point can make PlayMode paused mid-simulation, and the `simulate-*` call surfaces that as a preflight failure. Check `uloop pause-point-status --id ` first to confirm the hit before treating it as a bug in the simulated action itself. +If a `simulate-*` command instead returns a failure whose message says PlayMode is paused, suspect a pause point hit rather than an unrelated failure: an active pause point can make PlayMode paused mid-simulation, and the `simulate-*` call surfaces that as a preflight failure. The failure response names the responsible marker in `RejectedByActivePausePointId`. Check `uloop pause-point-status --id ` first to confirm the hit before treating it as a bug in the simulated action itself. ## When To Use diff --git a/Assets/Tests/Editor/PausePointRejectionResponseFieldTests.cs b/Assets/Tests/Editor/PausePointRejectionResponseFieldTests.cs new file mode 100644 index 0000000000..f65719dadc --- /dev/null +++ b/Assets/Tests/Editor/PausePointRejectionResponseFieldTests.cs @@ -0,0 +1,67 @@ +using Newtonsoft.Json; +using NUnit.Framework; + +using io.github.hatayama.UnityCliLoop.FirstPartyTools; + +namespace io.github.hatayama.UnityCliLoop.Tests.Editor +{ + /// + /// Test fixture that pins the wire name of the pause-point rejection field on every tool response + /// a --trigger can dispatch, because the CLI matches on that exact name to detect a trigger that + /// was refused before it ran. + /// + public class PausePointRejectionResponseFieldTests + { + private const string ExpectedJsonFragment = "\"RejectedByActivePausePointId\":\"marker\""; + + [Test] + public void SimulateKeyboardResponse_WhenRejectedByPausePoint_SerializesTheRejectionField() + { + // Verifies simulate-keyboard's rejection response carries the field under the name the CLI reads. + string json = JsonConvert.SerializeObject( + new SimulateKeyboardResponse { Success = false, RejectedByActivePausePointId = "marker" }); + + Assert.That(json, Does.Contain(ExpectedJsonFragment)); + } + + [Test] + public void SimulateMouseInputResponse_WhenRejectedByPausePoint_SerializesTheRejectionField() + { + // Verifies simulate-mouse-input's rejection response carries the field under the name the CLI reads. + string json = JsonConvert.SerializeObject( + new SimulateMouseInputResponse { Success = false, RejectedByActivePausePointId = "marker" }); + + Assert.That(json, Does.Contain(ExpectedJsonFragment)); + } + + [Test] + public void SimulateMouseUiResponse_WhenRejectedByPausePoint_SerializesTheRejectionField() + { + // Verifies simulate-mouse-ui's rejection response carries the field under the name the CLI reads. + string json = JsonConvert.SerializeObject( + new SimulateMouseUiResponse { Success = false, RejectedByActivePausePointId = "marker" }); + + Assert.That(json, Does.Contain(ExpectedJsonFragment)); + } + + [Test] + public void RecordInputResponse_WhenRejectedByPausePoint_SerializesTheRejectionField() + { + // Verifies record-input reports the same structured rejection, since it shares the preflight that refuses it. + string json = JsonConvert.SerializeObject( + new RecordInputResponse { Success = false, RejectedByActivePausePointId = "marker" }); + + Assert.That(json, Does.Contain(ExpectedJsonFragment)); + } + + [Test] + public void ReplayInputResponse_WhenRejectedByPausePoint_SerializesTheRejectionField() + { + // Verifies replay-input reports the same structured rejection: it is a realistic --trigger target. + string json = JsonConvert.SerializeObject( + new ReplayInputResponse { Success = false, RejectedByActivePausePointId = "marker" }); + + Assert.That(json, Does.Contain(ExpectedJsonFragment)); + } + } +} diff --git a/Assets/Tests/Editor/PausePointRejectionResponseFieldTests.cs.meta b/Assets/Tests/Editor/PausePointRejectionResponseFieldTests.cs.meta new file mode 100644 index 0000000000..4ac8bac88b --- /dev/null +++ b/Assets/Tests/Editor/PausePointRejectionResponseFieldTests.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 48fc9d8273cd94ee585a666e36a3e4e9 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Assets/Tests/Editor/PlayModeToolPreflightResultTests.cs b/Assets/Tests/Editor/PlayModeToolPreflightResultTests.cs new file mode 100644 index 0000000000..3a3dc95d3c --- /dev/null +++ b/Assets/Tests/Editor/PlayModeToolPreflightResultTests.cs @@ -0,0 +1,79 @@ +using NUnit.Framework; + +using io.github.hatayama.UnityCliLoop.FirstPartyTools; + +namespace io.github.hatayama.UnityCliLoop.Tests.Editor +{ + /// + /// Test fixture that verifies the preflight result carries the active pause point's id as a + /// structured field, not only inside the human-readable rejection message. + /// + public class PlayModeToolPreflightResultTests + { + private const string PausedActionDescription = "simulating keyboard input"; + + [Test] + public void Evaluate_WhenPlayModeIsNotActive_ReportsNoPausePointId() + { + // Verifies a not-active rejection has nothing to do with a pause point, so the structured field stays empty. + PlayModeToolPreflightResult result = PlayModeToolPreflightService.Evaluate( + isPlaying: false, + isPaused: false, + activePausePointId: "marker", + pausedActionDescription: PausedActionDescription); + + Assert.That(result.IsValid, Is.False); + Assert.That(result.ErrorMessage, Is.EqualTo(PlayModeToolPreflightService.PlayModeNotActiveMessage)); + Assert.That(result.RejectedByActivePausePointId, Is.Null); + } + + [Test] + public void Evaluate_WhenPausedByPausePoint_ReportsThatPausePointId() + { + // Verifies a pause-point-owned pause reports the id in a field a caller can compare, since the message alone forces string matching. + PlayModeToolPreflightResult result = PlayModeToolPreflightService.Evaluate( + isPlaying: true, + isPaused: true, + activePausePointId: "marker", + pausedActionDescription: PausedActionDescription); + + Assert.That(result.IsValid, Is.False); + Assert.That(result.RejectedByActivePausePointId, Is.EqualTo("marker")); + Assert.That( + result.ErrorMessage, + Is.EqualTo(PlayModeToolPreflightService.FormatPausePointPausedMessage("marker", PausedActionDescription))); + } + + [Test] + public void Evaluate_WhenPausedWithoutPausePoint_ReportsNoPausePointId() + { + // Verifies a manual pause is not attributed to a pause point, so a caller cannot mistake it for one of its own markers. + PlayModeToolPreflightResult result = PlayModeToolPreflightService.Evaluate( + isPlaying: true, + isPaused: true, + activePausePointId: string.Empty, + pausedActionDescription: PausedActionDescription); + + Assert.That(result.IsValid, Is.False); + Assert.That(result.RejectedByActivePausePointId, Is.Null); + Assert.That( + result.ErrorMessage, + Is.EqualTo(PlayModeToolPreflightService.FormatPausedMessage(PausedActionDescription))); + } + + [Test] + public void Evaluate_WhenPlayingAndNotPaused_Succeeds() + { + // Verifies the success path stays a plain success with no rejection details attached. + PlayModeToolPreflightResult result = PlayModeToolPreflightService.Evaluate( + isPlaying: true, + isPaused: false, + activePausePointId: "marker", + pausedActionDescription: PausedActionDescription); + + Assert.That(result.IsValid, Is.True); + Assert.That(result.ErrorMessage, Is.Empty); + Assert.That(result.RejectedByActivePausePointId, Is.Null); + } + } +} diff --git a/Assets/Tests/Editor/PlayModeToolPreflightResultTests.cs.meta b/Assets/Tests/Editor/PlayModeToolPreflightResultTests.cs.meta new file mode 100644 index 0000000000..3fa024dc77 --- /dev/null +++ b/Assets/Tests/Editor/PlayModeToolPreflightResultTests.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: eb33fa476a1a34b47ba86f1626768877 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Assets/Tests/Editor/PlayModeToolPreflightServiceTests.cs b/Assets/Tests/Editor/PlayModeToolPreflightServiceTests.cs index b7b47dde9e..aa2b2e8599 100644 --- a/Assets/Tests/Editor/PlayModeToolPreflightServiceTests.cs +++ b/Assets/Tests/Editor/PlayModeToolPreflightServiceTests.cs @@ -1,7 +1,6 @@ using NUnit.Framework; using io.github.hatayama.UnityCliLoop.FirstPartyTools; -using io.github.hatayama.UnityCliLoop.ToolContracts; namespace io.github.hatayama.UnityCliLoop.Tests.Editor { @@ -17,7 +16,7 @@ public class PlayModeToolPreflightServiceTests public void RequireActive_WhenEditModeIsNotPlaying_ReturnsNotActiveFailure() { // Verifies the active-only preflight fails with the exact wire-visible not-active message. - ValidationResult result = PlayModeToolPreflightService.RequireActive(); + PlayModeToolPreflightResult result = PlayModeToolPreflightService.RequireActive(); Assert.That(result.IsValid, Is.False); Assert.That(result.ErrorMessage, Is.EqualTo(ExpectedNotActiveMessage)); @@ -27,7 +26,7 @@ public void RequireActive_WhenEditModeIsNotPlaying_ReturnsNotActiveFailure() public void RequireActiveAndNotPaused_WhenEditModeIsNotPlaying_ReturnsNotActiveFailure() { // Verifies the paused-aware preflight also fails with the exact not-active message when PlayMode is inactive. - ValidationResult result = PlayModeToolPreflightService.RequireActiveAndNotPaused(RecordInputUseCase.PausedActionDescription); + PlayModeToolPreflightResult result = PlayModeToolPreflightService.RequireActiveAndNotPaused(RecordInputUseCase.PausedActionDescription); Assert.That(result.IsValid, Is.False); Assert.That(result.ErrorMessage, Is.EqualTo(ExpectedNotActiveMessage)); diff --git a/Packages/src/Editor/CliOnlyTools~/PausePoint/Skill/SKILL.md b/Packages/src/Editor/CliOnlyTools~/PausePoint/Skill/SKILL.md index 68702eb471..31e64add12 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. 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. +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 — the hit response then carries `TriggerFailed: true` at the top level and a `Warning` explaining that no input reached the game (the triggered command's own response stays in `TriggerResult`), so do not treat 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. @@ -53,8 +53,8 @@ Every hit response embeds `CapturedVariables`: the method's in-scope locals, its - The snapshot is taken **before** the resolved line executes, exactly like an IDE breakpoint on that line. To inspect a value after an assignment, place the pause point on the following line. - `Scope` is `Local`, `Parameter`, `InstanceField`, or `This`. The synthetic `this` entry identifies which instance or GameObject was hit via `UnityObjectPath` and `UnityObjectInstanceId`; `UnityEngine.Object` values carry the same handle fields for follow-up digs with `get-hierarchy`, `find-game-objects`, or `execute-dynamic-code`. - `--captured-variables names` on `await-pause-point`/`pause-point-status` drops every `Value` and keeps `Name`/`Scope`/`TypeName` — use it first on field-heavy classes, then fetch full values with a plain `pause-point-status` call. -- When the response would be dominated by variables you do not need, pass `--captured-variable-names velocity,this` (comma-separated, exact match on `Name`) to keep only those entries; it composes with `--captured-variables full|names`. `CapturedVariablesTruncated` in the response reports truncation at Unity-side capture time and is unrelated to this name filter — it can be `true` even when every requested name was found. -- Pass `--expect 'name=value'` (repeatable; on `await-pause-point` and `enable-pause-point --await`, not `pause-point-status`) to have the CLI compare captured variables against expected values; the response includes an `Expectations` array and `AllExpectationsPassed`, so you do not need to eyeball the JSON. Matching is string equality against the serialized value. +- When the response would be dominated by variables you do not need, pass `--captured-variable-names velocity,this` (comma-separated, exact match on `Name`) to keep only those entries; it composes with `--captured-variables full|names`. `CapturedVariablesTruncated` in the response reports truncation at Unity-side capture time and is unrelated to this name filter — it can be `true` even when every requested name was found. Requested names that matched nothing are listed in `CapturedVariableNamesNotFound`, so a partial match is visible without comparing the response against the request by hand. +- Pass `--expect 'name=value'` (repeatable; on `await-pause-point`, `enable-pause-point --await`, and `pause-point-status`) to have the CLI compare captured variables against expected values; the response includes an `Expectations` array and `AllExpectationsPassed`, so you do not need to eyeball the JSON. Matching is string equality against the serialized value. On `pause-point-status` a marker that has not been hit yet reports each expectation as not found, and the verdict never changes the exit code — a polling loop reads `AllExpectationsPassed`, not the exit status. - Collection values (arrays, `List`, dictionaries, plain objects) render as a JSON preview capped at 10 elements by default. When the elements you need sit past that cap (a 10x20 grid, a long list), re-enable with `--max-preview-elements ` (1–1000). The value set at enable time also caps the previews in every later `pause-point-status` response for that marker — status has no flag to change it. - While Unity is still paused, `UloopPausePoint.TryGetCapturedValue("name")` (and `"this"`) returns live captured references for `execute-dynamic-code`; the return is a `(bool Found, object Value)` tuple, and the holder clears on resume. (file:line marker hits only — id-only markers store no capture) These are **live objects in their frame-completed state, not snapshots** — use them only to dig further into objects that are still alive, never to reconstruct what a value was at the paused line. @@ -95,7 +95,7 @@ For "N frames after the input" (for example, three frames after a key press), ad A pause point hits only when control flow reaches the patched line (or the `Pause(id)` call). `simulate-keyboard` returning `PressEdgeObserved=true` means the input edge was observed, not that your target game logic has reached the pause line yet. -If a `simulate-*` command instead returns a failure whose message says PlayMode is paused, suspect a pause point hit rather than an unrelated failure: an active pause point can make PlayMode paused mid-simulation, and the `simulate-*` call surfaces that as a preflight failure. Check `uloop pause-point-status --id ` first to confirm the hit before treating it as a bug in the simulated action itself. +If a `simulate-*` command instead returns a failure whose message says PlayMode is paused, suspect a pause point hit rather than an unrelated failure: an active pause point can make PlayMode paused mid-simulation, and the `simulate-*` call surfaces that as a preflight failure. The failure response names the responsible marker in `RejectedByActivePausePointId`. Check `uloop pause-point-status --id ` first to confirm the hit before treating it as a bug in the simulated action itself. ## When To Use diff --git a/Packages/src/Editor/FirstPartyTools/Common/Preflight/PlayModeToolPreflightResult.cs b/Packages/src/Editor/FirstPartyTools/Common/Preflight/PlayModeToolPreflightResult.cs new file mode 100644 index 0000000000..6a7e908832 --- /dev/null +++ b/Packages/src/Editor/FirstPartyTools/Common/Preflight/PlayModeToolPreflightResult.cs @@ -0,0 +1,54 @@ +#nullable enable +using UnityEngine; + +namespace io.github.hatayama.UnityCliLoop.FirstPartyTools +{ + /// + /// Outcome of a PlayMode preflight check: whether the tool may run, the wire-visible rejection + /// message, and — when a pause point is what refused the call — that pause point's id as a + /// separate field. The id exists on its own because callers (the CLI's --trigger diagnosis + /// above all) have to tell "refused by the marker I am waiting on" from "refused for some other + /// reason", and the message text is too brittle to match on. + /// + public class PlayModeToolPreflightResult + { + public bool IsValid { get; } + + /// Rejection message, empty on success. Never null, so callers can assign it to a + /// tool response's non-nullable Message without a null check. + public string ErrorMessage { get; } + + /// Id of the pause point holding PlayMode paused, null for every other outcome. + public string? RejectedByActivePausePointId { get; } + + private PlayModeToolPreflightResult(bool isValid, string errorMessage, string? rejectedByActivePausePointId) + { + IsValid = isValid; + ErrorMessage = errorMessage; + RejectedByActivePausePointId = rejectedByActivePausePointId; + } + + /// Creates the success outcome. + public static PlayModeToolPreflightResult Success() + { + return new PlayModeToolPreflightResult(true, string.Empty, null); + } + + /// Creates a rejection that has no pause point behind it. + public static PlayModeToolPreflightResult Failure(string errorMessage) + { + Debug.Assert(!string.IsNullOrEmpty(errorMessage), "errorMessage must not be null or empty"); + return new PlayModeToolPreflightResult(false, errorMessage, null); + } + + /// Creates a rejection caused by an active pause point, recording which one. + public static PlayModeToolPreflightResult FailureRejectedByPausePoint( + string errorMessage, + string pausePointId) + { + Debug.Assert(!string.IsNullOrEmpty(errorMessage), "errorMessage must not be null or empty"); + Debug.Assert(!string.IsNullOrEmpty(pausePointId), "pausePointId must not be null or empty"); + return new PlayModeToolPreflightResult(false, errorMessage, pausePointId); + } + } +} diff --git a/Packages/src/Editor/FirstPartyTools/Common/Preflight/PlayModeToolPreflightResult.cs.meta b/Packages/src/Editor/FirstPartyTools/Common/Preflight/PlayModeToolPreflightResult.cs.meta new file mode 100644 index 0000000000..1a1047e344 --- /dev/null +++ b/Packages/src/Editor/FirstPartyTools/Common/Preflight/PlayModeToolPreflightResult.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: b6c9bb51e0b3d4cd4b1d88aa86b681b2 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Packages/src/Editor/FirstPartyTools/Common/Preflight/PlayModeToolPreflightService.cs b/Packages/src/Editor/FirstPartyTools/Common/Preflight/PlayModeToolPreflightService.cs index c09fa2e183..fe05f3a5b1 100644 --- a/Packages/src/Editor/FirstPartyTools/Common/Preflight/PlayModeToolPreflightService.cs +++ b/Packages/src/Editor/FirstPartyTools/Common/Preflight/PlayModeToolPreflightService.cs @@ -3,7 +3,6 @@ using UnityEngine; using io.github.hatayama.UnityCliLoop.Runtime; -using io.github.hatayama.UnityCliLoop.ToolContracts; namespace io.github.hatayama.UnityCliLoop.FirstPartyTools { @@ -19,39 +18,60 @@ public static class PlayModeToolPreflightService /// /// Fails when PlayMode is not active. Use for tools that tolerate paused PlayMode. /// - public static ValidationResult RequireActive() + public static PlayModeToolPreflightResult RequireActive() { if (!EditorApplication.isPlaying) { - return ValidationResult.Failure(PlayModeNotActiveMessage); + return PlayModeToolPreflightResult.Failure(PlayModeNotActiveMessage); } - return ValidationResult.Success(); + return PlayModeToolPreflightResult.Success(); } /// /// Fails when PlayMode is not active, or is active but paused. The paused-message suffix /// describes the blocked action in the caller's vocabulary (for example "recording input"). /// - public static ValidationResult RequireActiveAndNotPaused(string pausedActionDescription) + public static PlayModeToolPreflightResult RequireActiveAndNotPaused(string pausedActionDescription) + { + return Evaluate( + EditorApplication.isPlaying, + EditorApplication.isPaused, + UloopPausePointRegistry.GetActivePausePointId(), + pausedActionDescription); + } + + /// + /// Decides a paused-aware preflight outcome from already-read editor state. Separated from + /// RequireActiveAndNotPaused so the paused branches — the only ones that produce a pause + /// point id — are reachable from EditMode tests, where PlayMode is never actually running. + /// + public static PlayModeToolPreflightResult Evaluate( + bool isPlaying, + bool isPaused, + string activePausePointId, + string pausedActionDescription) { Debug.Assert(!string.IsNullOrEmpty(pausedActionDescription), "pausedActionDescription must not be null or empty"); - if (!EditorApplication.isPlaying) + if (!isPlaying) + { + return PlayModeToolPreflightResult.Failure(PlayModeNotActiveMessage); + } + + if (!isPaused) { - return ValidationResult.Failure(PlayModeNotActiveMessage); + return PlayModeToolPreflightResult.Success(); } - if (EditorApplication.isPaused) + if (string.IsNullOrEmpty(activePausePointId)) { - string activePausePointId = UloopPausePointRegistry.GetActivePausePointId(); - string message = string.IsNullOrEmpty(activePausePointId) - ? FormatPausedMessage(pausedActionDescription) - : FormatPausePointPausedMessage(activePausePointId, pausedActionDescription); - return ValidationResult.Failure(message); + return PlayModeToolPreflightResult.Failure(FormatPausedMessage(pausedActionDescription)); } - return ValidationResult.Success(); + return PlayModeToolPreflightResult.FailureRejectedByPausePoint( + FormatPausePointPausedMessage(activePausePointId, pausedActionDescription), + activePausePointId); } /// diff --git a/Packages/src/Editor/FirstPartyTools/RecordInput/RecordInputResponse.cs b/Packages/src/Editor/FirstPartyTools/RecordInput/RecordInputResponse.cs index b0f770acab..ed1eee1ac0 100644 --- a/Packages/src/Editor/FirstPartyTools/RecordInput/RecordInputResponse.cs +++ b/Packages/src/Editor/FirstPartyTools/RecordInput/RecordInputResponse.cs @@ -14,5 +14,13 @@ public class RecordInputResponse : UnityCliLoopToolResponse public string? OutputPath { get; set; } public int? TotalFrames { get; set; } public float? DurationSeconds { get; set; } + + /// + /// Id of the pause point that refused this call before it ran anything, null otherwise. A + /// refusal means nothing was started, so a caller reading only Success would miss that the + /// action never happened. The CLI's --trigger diagnosis compares this against the marker it + /// awaits. + /// + public string? RejectedByActivePausePointId { get; set; } } } diff --git a/Packages/src/Editor/FirstPartyTools/RecordInput/RecordInputUseCase.cs b/Packages/src/Editor/FirstPartyTools/RecordInput/RecordInputUseCase.cs index 44b1b66f3b..2ca4bc94f9 100644 --- a/Packages/src/Editor/FirstPartyTools/RecordInput/RecordInputUseCase.cs +++ b/Packages/src/Editor/FirstPartyTools/RecordInput/RecordInputUseCase.cs @@ -98,14 +98,15 @@ private static async Task ExecuteStartAsync( RecordInputSchema request, CancellationToken ct) { - ValidationResult preflight = PlayModeToolPreflightService.RequireActiveAndNotPaused(PausedActionDescription); + PlayModeToolPreflightResult preflight = PlayModeToolPreflightService.RequireActiveAndNotPaused(PausedActionDescription); if (!preflight.IsValid) { return new RecordInputResponse { Success = false, Message = preflight.ErrorMessage, - Action = RecordInputAction.Start.ToString() + Action = RecordInputAction.Start.ToString(), + RejectedByActivePausePointId = preflight.RejectedByActivePausePointId }; } diff --git a/Packages/src/Editor/FirstPartyTools/ReplayInput/ReplayInputResponse.cs b/Packages/src/Editor/FirstPartyTools/ReplayInput/ReplayInputResponse.cs index 7b50abd38f..773aec61e8 100644 --- a/Packages/src/Editor/FirstPartyTools/ReplayInput/ReplayInputResponse.cs +++ b/Packages/src/Editor/FirstPartyTools/ReplayInput/ReplayInputResponse.cs @@ -16,5 +16,13 @@ public class ReplayInputResponse : UnityCliLoopToolResponse public int? TotalFrames { get; set; } public float? Progress { get; set; } public bool? IsReplaying { get; set; } + + /// + /// Id of the pause point that refused this call before it ran anything, null otherwise. A + /// refusal means nothing was started, so a caller reading only Success would miss that the + /// action never happened. The CLI's --trigger diagnosis compares this against the marker it + /// awaits. + /// + public string? RejectedByActivePausePointId { get; set; } } } diff --git a/Packages/src/Editor/FirstPartyTools/ReplayInput/ReplayInputUseCase.cs b/Packages/src/Editor/FirstPartyTools/ReplayInput/ReplayInputUseCase.cs index 9e3ce27179..d23f144e15 100644 --- a/Packages/src/Editor/FirstPartyTools/ReplayInput/ReplayInputUseCase.cs +++ b/Packages/src/Editor/FirstPartyTools/ReplayInput/ReplayInputUseCase.cs @@ -94,14 +94,15 @@ public async Task ReplayInputAsync( #if ULOOP_HAS_INPUT_SYSTEM private static ReplayInputResponse ExecuteStart(ReplayInputSchema request) { - ValidationResult preflight = PlayModeToolPreflightService.RequireActiveAndNotPaused(PausedActionDescription); + PlayModeToolPreflightResult preflight = PlayModeToolPreflightService.RequireActiveAndNotPaused(PausedActionDescription); if (!preflight.IsValid) { return new ReplayInputResponse { Success = false, Message = preflight.ErrorMessage, - Action = ReplayInputAction.Start.ToString() + Action = ReplayInputAction.Start.ToString(), + RejectedByActivePausePointId = preflight.RejectedByActivePausePointId }; } diff --git a/Packages/src/Editor/FirstPartyTools/SimulateKeyboard/SimulateKeyboardResponse.cs b/Packages/src/Editor/FirstPartyTools/SimulateKeyboard/SimulateKeyboardResponse.cs index 52d1151b42..b9ff6b3c7c 100644 --- a/Packages/src/Editor/FirstPartyTools/SimulateKeyboard/SimulateKeyboardResponse.cs +++ b/Packages/src/Editor/FirstPartyTools/SimulateKeyboard/SimulateKeyboardResponse.cs @@ -15,6 +15,14 @@ public class SimulateKeyboardResponse : UnityCliLoopToolResponse public string Action { get; set; } = ""; public string? KeyName { get; set; } public bool InterruptedByPausePoint { get; set; } + /// + /// Id of the pause point that refused this call before it ran anything, null otherwise. + /// Distinct from PausePointId, which reports a marker hit *during* the call: a refusal means + /// no input was injected at all, so a caller reading only Success would miss that the action + /// never happened. The CLI's --trigger diagnosis compares this against the marker it awaits. + /// + public string? RejectedByActivePausePointId { get; set; } + public string? PausePointId { get; set; } public int? PausePointHitCount { get; set; } public List? PausePointHits { get; set; } diff --git a/Packages/src/Editor/FirstPartyTools/SimulateKeyboard/SimulateKeyboardUseCase.cs b/Packages/src/Editor/FirstPartyTools/SimulateKeyboard/SimulateKeyboardUseCase.cs index 6b7601e988..3520090e25 100644 --- a/Packages/src/Editor/FirstPartyTools/SimulateKeyboard/SimulateKeyboardUseCase.cs +++ b/Packages/src/Editor/FirstPartyTools/SimulateKeyboard/SimulateKeyboardUseCase.cs @@ -49,7 +49,7 @@ public async Task ExecuteAsync( // ReleaseAll must work while paused so agents can recover stuck device state after a // pause-point interruption without first resuming PlayMode. - ValidationResult preflight = parameters.Action == UnityCliLoopKeyboardAction.ReleaseAll + PlayModeToolPreflightResult preflight = parameters.Action == UnityCliLoopKeyboardAction.ReleaseAll ? PlayModeToolPreflightService.RequireActive() : PlayModeToolPreflightService.RequireActiveAndNotPaused(PausedActionDescription); if (!preflight.IsValid) @@ -58,7 +58,8 @@ public async Task ExecuteAsync( { Success = false, Message = preflight.ErrorMessage, - Action = parameters.Action.ToString() + Action = parameters.Action.ToString(), + RejectedByActivePausePointId = preflight.RejectedByActivePausePointId }; } diff --git a/Packages/src/Editor/FirstPartyTools/SimulateMouseInput/SimulateMouseInputResponse.cs b/Packages/src/Editor/FirstPartyTools/SimulateMouseInput/SimulateMouseInputResponse.cs index 72982359eb..71cd0e8ab9 100644 --- a/Packages/src/Editor/FirstPartyTools/SimulateMouseInput/SimulateMouseInputResponse.cs +++ b/Packages/src/Editor/FirstPartyTools/SimulateMouseInput/SimulateMouseInputResponse.cs @@ -26,6 +26,14 @@ public class SimulateMouseInputResponse : UnityCliLoopToolResponse public float? InjectedUnityPositionY { get; set; } public string CoordinateConversionFormula { get; set; } = ""; public bool InterruptedByPausePoint { get; set; } + /// + /// Id of the pause point that refused this call before it ran anything, null otherwise. + /// Distinct from PausePointId, which reports a marker hit *during* the call: a refusal means + /// no input was injected at all, so a caller reading only Success would miss that the action + /// never happened. The CLI's --trigger diagnosis compares this against the marker it awaits. + /// + public string? RejectedByActivePausePointId { get; set; } + public string? PausePointId { get; set; } public int? PausePointHitCount { get; set; } public List? PausePointHits { get; set; } diff --git a/Packages/src/Editor/FirstPartyTools/SimulateMouseInput/SimulateMouseInputUseCase.cs b/Packages/src/Editor/FirstPartyTools/SimulateMouseInput/SimulateMouseInputUseCase.cs index 9bc0fa565d..ff007ac80f 100644 --- a/Packages/src/Editor/FirstPartyTools/SimulateMouseInput/SimulateMouseInputUseCase.cs +++ b/Packages/src/Editor/FirstPartyTools/SimulateMouseInput/SimulateMouseInputUseCase.cs @@ -46,14 +46,15 @@ public async Task ExecuteAsync( #else string correlationId = UnityCliLoopConstants.GenerateCorrelationId(); - ValidationResult preflight = PlayModeToolPreflightService.RequireActiveAndNotPaused(PausedActionDescription); + PlayModeToolPreflightResult preflight = PlayModeToolPreflightService.RequireActiveAndNotPaused(PausedActionDescription); if (!preflight.IsValid) { return new SimulateMouseInputResponse { Success = false, Message = preflight.ErrorMessage, - Action = parameters.Action.ToString() + Action = parameters.Action.ToString(), + RejectedByActivePausePointId = preflight.RejectedByActivePausePointId }; } diff --git a/Packages/src/Editor/FirstPartyTools/SimulateMouseUi/MouseUiSimulationResponseFactory.cs b/Packages/src/Editor/FirstPartyTools/SimulateMouseUi/MouseUiSimulationResponseFactory.cs index 6e739c7c0c..356eead17d 100644 --- a/Packages/src/Editor/FirstPartyTools/SimulateMouseUi/MouseUiSimulationResponseFactory.cs +++ b/Packages/src/Editor/FirstPartyTools/SimulateMouseUi/MouseUiSimulationResponseFactory.cs @@ -24,6 +24,25 @@ internal static SimulateMouseUiResponse CreateFailure( }; } + /// + /// Creates the failure response for a rejected PlayMode preflight, carrying the rejecting + /// pause point's id through to the wire. Separate from CreateFailure (rather than an optional + /// argument on it) so the other failure sites cannot accidentally claim a pause-point cause. + /// + internal static SimulateMouseUiResponse CreatePreflightFailure( + MouseUiSimulationCommand parameters, + PlayModeToolPreflightResult preflight) + { + Debug.Assert(!preflight.IsValid, "CreatePreflightFailure must only be called for a rejected preflight"); + return new SimulateMouseUiResponse + { + Success = false, + Message = preflight.ErrorMessage, + Action = parameters.Action.ToString(), + RejectedByActivePausePointId = preflight.RejectedByActivePausePointId + }; + } + internal static SimulateMouseUiResponse CreateFrameTimeoutResult( MouseAction action, Vector2 position, diff --git a/Packages/src/Editor/FirstPartyTools/SimulateMouseUi/MouseUiSimulationValidator.cs b/Packages/src/Editor/FirstPartyTools/SimulateMouseUi/MouseUiSimulationValidator.cs index e3d85abcec..239600f464 100644 --- a/Packages/src/Editor/FirstPartyTools/SimulateMouseUi/MouseUiSimulationValidator.cs +++ b/Packages/src/Editor/FirstPartyTools/SimulateMouseUi/MouseUiSimulationValidator.cs @@ -2,7 +2,6 @@ using UnityEngine.EventSystems; using io.github.hatayama.UnityCliLoop.Runtime; -using io.github.hatayama.UnityCliLoop.ToolContracts; namespace io.github.hatayama.UnityCliLoop.FirstPartyTools { @@ -16,11 +15,11 @@ internal static class MouseUiSimulationValidator EventSystem? eventSystem, string pausedActionDescription) { - ValidationResult playModeResult = + PlayModeToolPreflightResult playModeResult = PlayModeToolPreflightService.RequireActiveAndNotPaused(pausedActionDescription); if (!playModeResult.IsValid) { - return MouseUiSimulationResponseFactory.CreateFailure(parameters, playModeResult.ErrorMessage); + return MouseUiSimulationResponseFactory.CreatePreflightFailure(parameters, playModeResult); } if (eventSystem == null) diff --git a/Packages/src/Editor/FirstPartyTools/SimulateMouseUi/SimulateMouseUiResponse.cs b/Packages/src/Editor/FirstPartyTools/SimulateMouseUi/SimulateMouseUiResponse.cs index 66fd34b283..8da0c6270a 100644 --- a/Packages/src/Editor/FirstPartyTools/SimulateMouseUi/SimulateMouseUiResponse.cs +++ b/Packages/src/Editor/FirstPartyTools/SimulateMouseUi/SimulateMouseUiResponse.cs @@ -19,6 +19,14 @@ public class SimulateMouseUiResponse : UnityCliLoopToolResponse public float? EndPositionX { get; set; } public float? EndPositionY { get; set; } public bool InterruptedByPausePoint { get; set; } + /// + /// Id of the pause point that refused this call before it ran anything, null otherwise. + /// Distinct from PausePointId, which reports a marker hit *during* the call: a refusal means + /// no input was injected at all, so a caller reading only Success would miss that the action + /// never happened. The CLI's --trigger diagnosis compares this against the marker it awaits. + /// + public string? RejectedByActivePausePointId { get; set; } + public string? PausePointId { get; set; } public int? PausePointHitCount { get; set; } public List? PausePointHits { get; set; } diff --git a/cli/project-runner/internal/projectrunner/native_command_help.go b/cli/project-runner/internal/projectrunner/native_command_help.go index e2b83e0570..b35957333b 100644 --- a/cli/project-runner/internal/projectrunner/native_command_help.go +++ b/cli/project-runner/internal/projectrunner/native_command_help.go @@ -1,12 +1,9 @@ package projectrunner import ( - "fmt" "io" "sort" - clierrors "github.com/hatayama/unity-cli-loop/common/errors" - "github.com/hatayama/unity-cli-loop/common/clicore" "github.com/hatayama/unity-cli-loop/common/tooldocs" ) @@ -39,6 +36,7 @@ var runnerNativeCommandOptions = map[string][]string{ "--" + PausePointIDFlagName, "--" + tooldocs.PausePointCapturedVariablesFlagName, "--" + tooldocs.PausePointCapturedVariableNamesFlagName, + "--" + tooldocs.PausePointExpectFlagName, }, } @@ -87,22 +85,6 @@ func printNativeCommandHelp(command string, stdout io.Writer) { } } -// pausePointUnknownOptionError reports an unrecognized flag for a runner-owned native -// command. The hint calls out an outdated installed project runner as the likely cause when -// the flag is documented in the skill but this runner build predates it, rather than leaving -// the caller to guess between a typo and a stale binary. -func pausePointUnknownOptionError(command string, name string) *clierrors.ArgumentError { - return &clierrors.ArgumentError{ - Message: fmt.Sprintf( - "Unknown option %q for %s. If the skill documentation mentions this option, the installed "+ - "project runner may be older than the docs — check 'uloop --version' and update the CLI.", - "--"+name, command), - Option: "--" + name, - Command: command, - NextActions: []string{fmt.Sprintf("Run `uloop %s --help` to inspect supported options.", command)}, - } -} - func sortedNativeCommandOptions(options []string) []string { result := append([]string{}, options...) sort.Strings(result) diff --git a/cli/project-runner/internal/projectrunner/pause_point_captured_variable_names_filter.go b/cli/project-runner/internal/projectrunner/pause_point_captured_variable_names_filter.go index 5978bb45e5..52eaa599aa 100644 --- a/cli/project-runner/internal/projectrunner/pause_point_captured_variable_names_filter.go +++ b/cli/project-runner/internal/projectrunner/pause_point_captured_variable_names_filter.go @@ -1,6 +1,9 @@ package projectrunner -import "strings" +import ( + "slices" + "strings" +) // parsePausePointCapturedVariableNames splits the comma-separated --captured-variable-names // value into individual names, trimming surrounding whitespace and dropping empty entries. @@ -38,13 +41,17 @@ func filterPausePointCapturedVariablesByName( nameSet[name] = struct{}{} } + matchedNames := map[string]struct{}{} + filteredCurrent, currentMatchCount := filterCapturedVariablesByNameSet(response.CapturedVariables, nameSet) + collectCapturedVariableNames(filteredCurrent, matchedNames) response.CapturedVariables = filteredCurrent totalMatchCount := currentMatchCount history := make([]pausePointCapturedHistoryFrame, len(response.CapturedVariableHistory)) for index, frame := range response.CapturedVariableHistory { filteredFrame, frameMatchCount := filterCapturedVariablesByNameSet(frame.CapturedVariables, nameSet) + collectCapturedVariableNames(filteredFrame, matchedNames) frame.CapturedVariables = filteredFrame totalMatchCount += frameMatchCount history[index] = frame @@ -52,9 +59,38 @@ func filterPausePointCapturedVariablesByName( response.CapturedVariableHistory = history response.CapturedVariableNameFilterNoMatch = totalMatchCount == 0 + response.CapturedVariableNamesNotFound = unmatchedCapturedVariableNames(names, matchedNames) return response } +// unmatchedCapturedVariableNames lists the requested names that matched nothing, keeping the order +// they were requested in so the report reads back against the flag value the caller wrote. A name +// matched anywhere — current variables or any history frame — counts as found. A name requested +// twice is reported once: the list answers "which names have no value", not "how many times each +// was asked for". +func unmatchedCapturedVariableNames(names []string, matchedNames map[string]struct{}) []string { + notFound := make([]string, 0, len(names)) + for _, name := range names { + if _, ok := matchedNames[name]; ok { + continue + } + if slices.Contains(notFound, name) { + continue + } + notFound = append(notFound, name) + } + if len(notFound) == 0 { + return nil + } + return notFound +} + +func collectCapturedVariableNames(variables []pausePointCapturedVariable, names map[string]struct{}) { + for _, variable := range variables { + names[variable.Name] = struct{}{} + } +} + func filterCapturedVariablesByNameSet( variables []pausePointCapturedVariable, nameSet map[string]struct{}, diff --git a/cli/project-runner/internal/projectrunner/pause_point_captured_variable_names_filter_test.go b/cli/project-runner/internal/projectrunner/pause_point_captured_variable_names_filter_test.go index 6d5d5f7956..f1ea9cd846 100644 --- a/cli/project-runner/internal/projectrunner/pause_point_captured_variable_names_filter_test.go +++ b/cli/project-runner/internal/projectrunner/pause_point_captured_variable_names_filter_test.go @@ -2,28 +2,32 @@ package projectrunner import "testing" +// capturedVariableNamesFilterResponse is the fixture both --captured-variable-names test functions +// filter: three current variables, two of which also appear in one history frame. +func capturedVariableNamesFilterResponse() pausePointStatusResponse { + return pausePointStatusResponse{ + CapturedVariables: []pausePointCapturedVariable{ + {Name: "velocity", Scope: "Local", TypeName: "Vector3", Value: pausePointVariableValue("(1,0,0)")}, + {Name: "this", Scope: "This", TypeName: "PlayerController", Value: pausePointVariableValue("PlayerController")}, + {Name: "health", Scope: "Local", TypeName: "Int32", Value: pausePointVariableValue("100")}, + }, + CapturedVariableHistory: []pausePointCapturedHistoryFrame{ + { + HitSequence: 1, + CapturedVariables: []pausePointCapturedVariable{ + {Name: "velocity", Scope: "Local", TypeName: "Vector3", Value: pausePointVariableValue("(0,0,0)")}, + {Name: "health", Scope: "Local", TypeName: "Int32", Value: pausePointVariableValue("100")}, + }, + }, + }, + } +} + // TestFilterPausePointCapturedVariablesByName verifies the --captured-variable-names filter: // single-name selection, multi-name selection, a name with no match, and that it composes with // the --captured-variables mode (filter narrows first, then mode strips values). func TestFilterPausePointCapturedVariablesByName(t *testing.T) { - baseResponse := func() pausePointStatusResponse { - return pausePointStatusResponse{ - CapturedVariables: []pausePointCapturedVariable{ - {Name: "velocity", Scope: "Local", TypeName: "Vector3", Value: pausePointVariableValue("(1,0,0)")}, - {Name: "this", Scope: "This", TypeName: "PlayerController", Value: pausePointVariableValue("PlayerController")}, - {Name: "health", Scope: "Local", TypeName: "Int32", Value: pausePointVariableValue("100")}, - }, - CapturedVariableHistory: []pausePointCapturedHistoryFrame{ - { - HitSequence: 1, - CapturedVariables: []pausePointCapturedVariable{ - {Name: "velocity", Scope: "Local", TypeName: "Vector3", Value: pausePointVariableValue("(0,0,0)")}, - {Name: "health", Scope: "Local", TypeName: "Int32", Value: pausePointVariableValue("100")}, - }, - }, - }, - } - } + baseResponse := capturedVariableNamesFilterResponse t.Run("single name keeps only the matching variable", func(t *testing.T) { result := filterPausePointCapturedVariablesByName(baseResponse(), []string{"velocity"}) @@ -104,3 +108,57 @@ func TestParsePausePointCapturedVariableNames(t *testing.T) { } } } + +// TestFilterPausePointCapturedVariablesByNameReportsNotFound verifies which requested names are +// reported as matching nothing: request order is preserved, a history-only match counts as found, a +// repeat is reported once, and the all-or-nothing flag stays consistent with the list. +func TestFilterPausePointCapturedVariablesByNameReportsNotFound(t *testing.T) { + baseResponse := capturedVariableNamesFilterResponse + + t.Run("reports which requested names matched nothing, in the requested order", func(t *testing.T) { + result := filterPausePointCapturedVariablesByName( + baseResponse(), []string{"shield", "velocity", "armor"}) + if len(result.CapturedVariableNamesNotFound) != 2 || + result.CapturedVariableNamesNotFound[0] != "shield" || + result.CapturedVariableNamesNotFound[1] != "armor" { + t.Fatalf("expected the unmatched names in request order: %#v", result.CapturedVariableNamesNotFound) + } + if result.CapturedVariableNameFilterNoMatch { + t.Fatal("a partial match must not set CapturedVariableNameFilterNoMatch") + } + }) + + t.Run("a name matched only in history is not reported as missing", func(t *testing.T) { + response := baseResponse() + response.CapturedVariables = nil + result := filterPausePointCapturedVariablesByName(response, []string{"health"}) + if len(result.CapturedVariableNamesNotFound) != 0 { + t.Fatalf("a history-only match must count as found: %#v", result.CapturedVariableNamesNotFound) + } + }) + + t.Run("all names missing sets both the list and the no-match flag", func(t *testing.T) { + result := filterPausePointCapturedVariablesByName(baseResponse(), []string{"shield", "armor"}) + if len(result.CapturedVariableNamesNotFound) != 2 { + t.Fatalf("expected both names reported missing: %#v", result.CapturedVariableNamesNotFound) + } + if !result.CapturedVariableNameFilterNoMatch { + t.Fatal("expected CapturedVariableNameFilterNoMatch to stay true when nothing matches") + } + }) + + t.Run("a name requested twice is reported missing once", func(t *testing.T) { + result := filterPausePointCapturedVariablesByName( + baseResponse(), []string{"shield", "shield"}) + if len(result.CapturedVariableNamesNotFound) != 1 { + t.Fatalf("expected the repeated name once: %#v", result.CapturedVariableNamesNotFound) + } + }) + + t.Run("every name matching leaves the missing list empty", func(t *testing.T) { + result := filterPausePointCapturedVariablesByName(baseResponse(), []string{"velocity", "health"}) + if result.CapturedVariableNamesNotFound != nil { + t.Fatalf("expected no missing names: %#v", result.CapturedVariableNamesNotFound) + } + }) +} diff --git a/cli/project-runner/internal/projectrunner/pause_point_cli_options_contract_test.go b/cli/project-runner/internal/projectrunner/pause_point_cli_options_contract_test.go index 57f35940a4..2405418b28 100644 --- a/cli/project-runner/internal/projectrunner/pause_point_cli_options_contract_test.go +++ b/cli/project-runner/internal/projectrunner/pause_point_cli_options_contract_test.go @@ -3,6 +3,7 @@ package projectrunner import ( "testing" + "github.com/hatayama/unity-cli-loop/common/clicore" "github.com/hatayama/unity-cli-loop/common/tooldocs" ) @@ -118,3 +119,49 @@ func TestPausePointSharedHelpOptionsAreAcceptedByTheWaitParser(t *testing.T) { } } } + +// runnerNativeCommandSampleArgs supplies one accepted argv form per flag advertised by a +// runner-owned native command's --help. A flag added to runnerNativeCommandOptions without an entry +// here fails the contract test below rather than silently going unchecked. +var runnerNativeCommandSampleArgs = map[string][]string{ + "--" + PausePointIDFlagName: {"--id", "marker"}, + "--" + PausePointTimeoutFlagName: {"--timeout-seconds", "5"}, + "--" + PausePointLogsMaxCountFlagName: {"--matching-logs-max-count", "3"}, + "--" + tooldocs.PausePointCapturedVariablesFlagName: {"--captured-variables", "names"}, + "--" + tooldocs.PausePointCapturedVariableNamesFlagName: {"--captured-variable-names", "score"}, + "--" + tooldocs.PausePointExpectFlagName: {"--expect", "score=1"}, + "--" + tooldocs.PausePointTriggerFlagName: {"--trigger", "focus-window"}, + "--" + tooldocs.PausePointResumePlayFlagName: {"--resume-play"}, +} + +// Verifies every flag pause-point-status advertises in its --help output is actually accepted by its +// own parser. The status parser is a separate switch from the await one, so a flag added to the help +// table alone would be advertised and then rejected as unknown on use. +func TestPausePointStatusHelpOptionsAreAcceptedByTheStatusParser(t *testing.T) { + for _, option := range runnerNativeCommandOptions[clicore.PausePointStatusUserCommandName] { + args, ok := runnerNativeCommandSampleArgs[option] + if !ok { + t.Fatalf("no sample argv for %s: add one to runnerNativeCommandSampleArgs", option) + } + + statusArgs := append([]string{"--" + PausePointIDFlagName, "marker"}, args...) + if _, err := parsePausePointStatusOptions(statusArgs); err != nil { + t.Errorf("pause-point-status parser rejected advertised option %s: %v", option, err) + } + } +} + +// Verifies the same for await-pause-point, whose help table is the larger of the two. +func TestPausePointAwaitHelpOptionsAreAcceptedByTheWaitParser(t *testing.T) { + for _, option := range runnerNativeCommandOptions[clicore.PausePointAwaitCommandName] { + args, ok := runnerNativeCommandSampleArgs[option] + if !ok { + t.Fatalf("no sample argv for %s: add one to runnerNativeCommandSampleArgs", option) + } + + waitArgs := append([]string{"--" + PausePointIDFlagName, "marker"}, args...) + if _, err := parseWaitForPausePointOptions(waitArgs); err != nil { + t.Errorf("await-pause-point parser rejected advertised option %s: %v", option, err) + } + } +} diff --git a/cli/project-runner/internal/projectrunner/pause_point_enable.go b/cli/project-runner/internal/projectrunner/pause_point_enable.go index c9a7e61202..50c78da18d 100644 --- a/cli/project-runner/internal/projectrunner/pause_point_enable.go +++ b/cli/project-runner/internal/projectrunner/pause_point_enable.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "io" + "slices" "strings" "time" @@ -407,35 +408,18 @@ func runPausePointWaitAfterEnable( response = filterPausePointCapturedVariablesByName(response, options.capturedVariableNames) response = applyPausePointCapturedVariablesMode(response, options.capturedVariablesMode) - var payload any = response logs, logsErr := fetchMatchingLogs(ctx, connection, options.id, options.matchingLogsMaxCount) - switch { - case logsErr == nil: - payload = pausePointWaitResult{ - pausePointStatusResponse: response, - MatchingLogs: logs.Logs, - Warning: joinPausePointWarnings(enableFields.Warning, buildPausePointWarning(logs, response.HitCount)), - Expectations: expectations, - AllExpectationsPassed: pausePointAllExpectationsPassedPointer(expectations), - } - case enableFields.Warning != "" || len(expectations) > 0: - // Best-effort like the plain await path: a failed log fetch must not also drop the - // enable-time warning or --expect results, since those are the only evidence left in - // this branch. Uses an anonymous struct (not pausePointWaitResult) so MatchingLogs is - // omitted entirely rather than serialized as an empty array, preserving "empty array - // only means a successful fetch with no matches". - payload = struct { - pausePointStatusResponse - Warning string `json:"Warning,omitempty"` - Expectations []pausePointExpectationResult `json:"Expectations,omitempty"` - AllExpectationsPassed *bool `json:"AllExpectationsPassed,omitempty"` - }{ - pausePointStatusResponse: response, - Warning: enableFields.Warning, - Expectations: expectations, - AllExpectationsPassed: pausePointAllExpectationsPassedPointer(expectations), - } - } + // Unity's warning can come from either the enable response or the status poll that observed + // the hit, so both are passed; the join drops the repeat when they carry the same text. + payload := buildPausePointHitPayload(pausePointHitPayloadInputs{ + response: response, + logs: logs, + logsErr: logsErr, + unityWarning: joinPausePointWarnings(enableFields.Warning, response.Warning), + triggerResult: triggerResult, + awaitedPausePointID: options.id, + expectations: expectations, + }) result, marshalErr := json.Marshal(payload) if marshalErr != nil { clierrors.WriteClassifiedError(stderr, marshalErr, clierrors.ErrorContext{ @@ -477,12 +461,17 @@ func runPausePointWaitAfterEnable( return 1 } +// joinPausePointWarnings concatenates the warnings that apply to one response, dropping empty ones +// and repeats. Repeats are possible because the same text can reach a hit payload from two sources — +// the enable response and the status poll that observed the hit — and printing it twice reads as two +// separate problems. func joinPausePointWarnings(warnings ...string) string { - nonEmpty := make([]string, 0, len(warnings)) + unique := make([]string, 0, len(warnings)) for _, warning := range warnings { - if warning != "" { - nonEmpty = append(nonEmpty, warning) + if warning == "" || slices.Contains(unique, warning) { + continue } + unique = append(unique, warning) } - return strings.Join(nonEmpty, " ") + return strings.Join(unique, " ") } diff --git a/cli/project-runner/internal/projectrunner/pause_point_enable_await_diagnosis_test.go b/cli/project-runner/internal/projectrunner/pause_point_enable_await_diagnosis_test.go new file mode 100644 index 0000000000..ac876ab4aa --- /dev/null +++ b/cli/project-runner/internal/projectrunner/pause_point_enable_await_diagnosis_test.go @@ -0,0 +1,98 @@ +package projectrunner + +import ( + "bytes" + "context" + "errors" + "strings" + "testing" + "time" + + "github.com/hatayama/unity-cli-loop/common/unityipc" +) + +// runEnableAwaitWithStubbedTrigger drives the enable-pause-point --await hit path, the second hit +// payload builder, with the same stubs the plain await path's tests use. +func runEnableAwaitWithStubbedTrigger(t *testing.T, enableWarning string) (int, string) { + t.Helper() + + var stdout bytes.Buffer + var stderr bytes.Buffer + code := runPausePointWaitAfterEnable( + context.Background(), + unityipc.Connection{}, + waitForPausePointOptions{ + id: "jump", + timeoutSeconds: 1, + timeout: time.Second, + matchingLogsMaxCount: 5, + triggerCommand: "simulate-keyboard", + triggerArgs: []string{"--action", "Press", "--key", "W"}, + }, + enablePausePointPropagatedFields{Warning: enableWarning}, + &stdout, + &stderr, + ) + if stderr.Len() > 0 { + t.Logf("stderr: %s", stderr.String()) + } + return code, stdout.String() +} + +// Verifies enable-pause-point --await diagnoses a refused trigger exactly as await-pause-point does: +// the two commands build their hit payloads separately, so a diagnosis wired into only one is +// invisible to callers of the other, which is the form this project's own checklist exercises. +func TestRunPausePointWaitAfterEnableWarnsWhenTheTriggerWasRefusedByThisMarker(t *testing.T) { + stubPausePointHit(t, "") + stubPausePointMatchingLogs(t, nil) + stubPausePointTriggerDispatch(t, pausePointRejectedTriggerResponse("jump")) + + code, output := runEnableAwaitWithStubbedTrigger(t, "") + + if code != 0 { + t.Fatalf("expected the hit to stay a success, got %d: %s", code, output) + } + result := decodePausePointWaitResult(t, output) + if !strings.Contains(result.Warning, "refused") { + t.Errorf("expected a refusal warning: %q", result.Warning) + } + if result.TriggerFailed == nil || !*result.TriggerFailed { + t.Errorf("TriggerFailed must be promoted to the top level: %#v", result.TriggerFailed) + } +} + +// Verifies the enable-time warning survives next to the CLI's refusal warning, and that the +// refusal warning also survives a failed matching-log fetch. +func TestRunPausePointWaitAfterEnableKeepsEnableWarningWithTheRefusalWarning(t *testing.T) { + stubPausePointHit(t, "") + stubPausePointMatchingLogs(t, errors.New("unity busy")) + stubPausePointTriggerDispatch(t, pausePointRejectedTriggerResponse("jump")) + + _, output := runEnableAwaitWithStubbedTrigger(t, "Enable-time warning.") + + if strings.Contains(output, `"MatchingLogs"`) { + t.Errorf("a failed fetch must omit MatchingLogs entirely: %s", output) + } + result := decodePausePointWaitResult(t, output) + if !strings.Contains(result.Warning, "Enable-time warning.") { + t.Errorf("the enable-time warning was dropped: %q", result.Warning) + } + if !strings.Contains(result.Warning, "refused") { + t.Errorf("the refusal warning was dropped: %q", result.Warning) + } +} + +// Verifies a warning reported by both the enable response and the status poll is printed once: +// repeating identical text reads as two separate problems. +func TestRunPausePointWaitAfterEnableReportsARepeatedUnityWarningOnce(t *testing.T) { + stubPausePointHit(t, "Same Unity warning.") + stubPausePointMatchingLogs(t, nil) + stubPausePointTriggerDispatch(t, `{"Success":true}`) + + _, output := runEnableAwaitWithStubbedTrigger(t, "Same Unity warning.") + + result := decodePausePointWaitResult(t, output) + if strings.Count(result.Warning, "Same Unity warning.") != 1 { + t.Errorf("expected the repeated warning exactly once: %q", result.Warning) + } +} diff --git a/cli/project-runner/internal/projectrunner/pause_point_logs.go b/cli/project-runner/internal/projectrunner/pause_point_logs.go index 9a1206ba75..48f2919ef4 100644 --- a/cli/project-runner/internal/projectrunner/pause_point_logs.go +++ b/cli/project-runner/internal/projectrunner/pause_point_logs.go @@ -52,6 +52,62 @@ type pausePointWaitResult struct { AllExpectationsPassed *bool `json:"AllExpectationsPassed,omitempty"` } +// pausePointHitPayloadInputs gathers everything a hit payload is built from. Both hit paths +// (await-pause-point and enable-pause-point --await) share one builder because they had drifted +// apart before: a field added to one silently stayed missing from the other. +type pausePointHitPayloadInputs struct { + response pausePointStatusResponse + + // logs / logsErr come straight from fetchMatchingLogs. A failed fetch omits MatchingLogs + // entirely rather than emitting an empty array, so "empty array" keeps meaning "the fetch + // succeeded and nothing matched". + logs pausePointMatchingLogsResult + logsErr error + + // unityWarning is Unity's own warning for this hit: the status response's on the plain await + // path, the enable response's on the enable --await path. + unityWarning string + + triggerResult *pausePointTriggerResult + awaitedPausePointID string + expectations []pausePointExpectationResult +} + +// buildPausePointHitPayload assembles the JSON payload for a hit, folding the CLI-side diagnosis +// (trigger outcome, warnings, --expect verdicts) into the Unity response. +func buildPausePointHitPayload(inputs pausePointHitPayloadInputs) any { + response := inputs.response + response.TriggerFailed = pausePointTriggerFailedPointer(inputs.triggerResult) + triggerWarning := pausePointTriggerRefusalWarning(inputs.triggerResult, inputs.awaitedPausePointID) + + if inputs.logsErr != nil { + // Best-effort: a failed log fetch must not also drop the CLI-side evidence — the warnings + // or the --expect results, which are the only evidence left in this branch. + return struct { + pausePointStatusResponse + Warning string `json:"Warning,omitempty"` + Expectations []pausePointExpectationResult `json:"Expectations,omitempty"` + AllExpectationsPassed *bool `json:"AllExpectationsPassed,omitempty"` + }{ + pausePointStatusResponse: response, + Warning: joinPausePointWarnings(inputs.unityWarning, triggerWarning), + Expectations: inputs.expectations, + AllExpectationsPassed: pausePointAllExpectationsPassedPointer(inputs.expectations), + } + } + + return pausePointWaitResult{ + pausePointStatusResponse: response, + MatchingLogs: inputs.logs.Logs, + Warning: joinPausePointWarnings( + inputs.unityWarning, + buildPausePointWarning(inputs.logs, response.HitCount), + triggerWarning), + Expectations: inputs.expectations, + AllExpectationsPassed: pausePointAllExpectationsPassedPointer(inputs.expectations), + } +} + // pausePointAllExpectationsPassedPointer returns nil when no --expect was given, and otherwise // a pointer to whether every expectation passed. func pausePointAllExpectationsPassedPointer(results []pausePointExpectationResult) *bool { diff --git a/cli/project-runner/internal/projectrunner/pause_point_status_expect_test.go b/cli/project-runner/internal/projectrunner/pause_point_status_expect_test.go new file mode 100644 index 0000000000..84d3646d3a --- /dev/null +++ b/cli/project-runner/internal/projectrunner/pause_point_status_expect_test.go @@ -0,0 +1,224 @@ +package projectrunner + +import ( + "bytes" + "context" + "encoding/json" + "strings" + "testing" + + "github.com/hatayama/unity-cli-loop/common/clicore" + "github.com/hatayama/unity-cli-loop/common/unityipc" +) + +// pausePointStatusExpectPayload decodes only the expectation fields the CLI adds on top of the +// Unity status response, so these tests assert the wire names --expect callers actually read. +type pausePointStatusExpectPayload struct { + Status string `json:"Status"` + CapturedVariables []pausePointCapturedVariable `json:"CapturedVariables"` + Expectations []pausePointExpectationResult `json:"Expectations"` + AllExpectationsPassed *bool `json:"AllExpectationsPassed"` +} + +func stubPausePointStatusHitWithSpeed(t *testing.T, speed string) { + t.Helper() + + originalQuery := queryPausePointStatus + t.Cleanup(func() { + queryPausePointStatus = originalQuery + }) + + queryPausePointStatus = func( + ctx context.Context, + connection unityipc.Connection, + id string, + ) (pausePointStatusResponse, error) { + return pausePointStatusResponse{ + Success: true, + Id: id, + Status: pausePointStatusHit, + IsEnabled: true, + IsHit: true, + HitCount: 1, + CapturedVariables: []pausePointCapturedVariable{ + {Name: "speed", Scope: "Local", TypeName: "System.Int32", Value: pausePointVariableValue(speed)}, + }, + }, nil + } +} + +func runPausePointStatusForExpect(t *testing.T, args []string) (int, string) { + t.Helper() + + var stdout bytes.Buffer + var stderr bytes.Buffer + code := runPausePointStatusCommand( + context.Background(), + unityipc.Connection{ProjectRoot: "/tmp/MyProject"}, + args, + &stdout, + &stderr) + if stderr.Len() > 0 { + t.Logf("stderr: %s", stderr.String()) + } + return code, stdout.String() +} + +func decodePausePointStatusExpectPayload(t *testing.T, output string) pausePointStatusExpectPayload { + t.Helper() + + var payload pausePointStatusExpectPayload + if err := json.Unmarshal([]byte(output), &payload); err != nil { + t.Fatalf("stdout is not valid JSON: %v\n%s", err, output) + } + return payload +} + +// Verifies pause-point-status --expect reports each expectation and the aggregate verdict using the +// same field names await-pause-point already emits, so one query shape works for both commands. +func TestRunPausePointStatusEvaluatesExpectations(t *testing.T) { + stubPausePointStatusHitWithSpeed(t, "5") + + code, output := runPausePointStatusForExpect(t, []string{"--id", "jump", "--expect", "speed=5"}) + + if code != 0 { + t.Fatalf("expected success, got %d: %s", code, output) + } + payload := decodePausePointStatusExpectPayload(t, output) + if len(payload.Expectations) != 1 { + t.Fatalf("expected 1 expectation, got %#v", payload.Expectations) + } + expectation := payload.Expectations[0] + if expectation.Name != "speed" || expectation.Expected != "5" || expectation.Actual != "5" || + !expectation.Found || !expectation.Passed { + t.Fatalf("expectation mismatch: %#v", expectation) + } + if payload.AllExpectationsPassed == nil || !*payload.AllExpectationsPassed { + t.Fatalf("AllExpectationsPassed mismatch: %#v", payload.AllExpectationsPassed) + } +} + +// Verifies a failed expectation still exits 0: querying the hit succeeded, and the expectation +// verdict is reported in the payload rather than through the process exit code. +func TestRunPausePointStatusFailedExpectationKeepsExitCodeZero(t *testing.T) { + stubPausePointStatusHitWithSpeed(t, "5") + + code, output := runPausePointStatusForExpect(t, []string{"--id", "jump", "--expect", "speed=9"}) + + if code != 0 { + t.Fatalf("expected exit code 0 for a failed expectation, got %d: %s", code, output) + } + payload := decodePausePointStatusExpectPayload(t, output) + if len(payload.Expectations) != 1 || payload.Expectations[0].Passed { + t.Fatalf("expected a failing expectation, got %#v", payload.Expectations) + } + if payload.AllExpectationsPassed == nil || *payload.AllExpectationsPassed { + t.Fatalf("AllExpectationsPassed must be present and false: %#v", payload.AllExpectationsPassed) + } +} + +// Verifies expectations are evaluated before --captured-variables names strips values, so the +// requested value is still compared even though the response itself reports names only. +func TestRunPausePointStatusEvaluatesExpectationsBeforeNamesModeStripsValues(t *testing.T) { + stubPausePointStatusHitWithSpeed(t, "5") + + code, output := runPausePointStatusForExpect( + t, []string{"--id", "jump", "--captured-variables", "names", "--expect", "speed=5"}) + + if code != 0 { + t.Fatalf("expected success, got %d: %s", code, output) + } + payload := decodePausePointStatusExpectPayload(t, output) + if len(payload.Expectations) != 1 || !payload.Expectations[0].Passed || + payload.Expectations[0].Actual != "5" { + t.Fatalf("expectation must be evaluated against the unfiltered value: %#v", payload.Expectations) + } + if len(payload.CapturedVariables) != 1 || payload.CapturedVariables[0].Value != nil { + t.Fatalf("names mode must still strip Value: %#v", payload.CapturedVariables) + } +} + +// Verifies expectations are evaluated before the --captured-variable-names filter narrows the +// response, so an --expect target that was not also requested by name is not reported as missing. +func TestRunPausePointStatusEvaluatesExpectationsBeforeNameFilter(t *testing.T) { + stubPausePointStatusHitWithSpeed(t, "5") + + code, output := runPausePointStatusForExpect( + t, []string{"--id", "jump", "--captured-variable-names", "health", "--expect", "speed=5"}) + + if code != 0 { + t.Fatalf("expected success, got %d: %s", code, output) + } + payload := decodePausePointStatusExpectPayload(t, output) + if len(payload.Expectations) != 1 || !payload.Expectations[0].Found || + !payload.Expectations[0].Passed { + t.Fatalf("expectation must survive the name filter: %#v", payload.Expectations) + } +} + +// Verifies a status query without --expect emits neither expectation field, so callers that never +// asked for expectations see no schema change. +func TestRunPausePointStatusOmitsExpectationFieldsWithoutExpectFlag(t *testing.T) { + stubPausePointStatusHitWithSpeed(t, "5") + + code, output := runPausePointStatusForExpect(t, []string{"--id", "jump"}) + + if code != 0 { + t.Fatalf("expected success, got %d: %s", code, output) + } + if strings.Contains(output, "Expectations") || strings.Contains(output, "AllExpectationsPassed") { + t.Fatalf("expectation fields must be omitted without --expect: %s", output) + } +} + +// Verifies a marker that is armed but not yet hit reports its expectations as not found rather than +// omitting them, and that Status stays the field distinguishing "not hit yet" from "hit and wrong". +func TestRunPausePointStatusReportsExpectationsAsNotFoundBeforeAHit(t *testing.T) { + originalQuery := queryPausePointStatus + t.Cleanup(func() { + queryPausePointStatus = originalQuery + }) + queryPausePointStatus = func( + ctx context.Context, + connection unityipc.Connection, + id string, + ) (pausePointStatusResponse, error) { + return pausePointStatusResponse{Success: true, Id: id, Status: pausePointStatusEnabled, IsEnabled: true}, nil + } + + code, output := runPausePointStatusForExpect(t, []string{"--id", "jump", "--expect", "speed=5"}) + + if code != 0 { + t.Fatalf("expected success, got %d: %s", code, output) + } + payload := decodePausePointStatusExpectPayload(t, output) + if payload.Status != pausePointStatusEnabled { + t.Fatalf("Status must still report the marker is only armed: %q", payload.Status) + } + if len(payload.Expectations) != 1 || payload.Expectations[0].Found || payload.Expectations[0].Passed { + t.Fatalf("an unhit marker captured nothing, so the expectation is not found: %#v", payload.Expectations) + } +} + +// Verifies an invalid --expect value is rejected by pause-point-status the same way +// await-pause-point rejects it, instead of being reported as an unknown option. +func TestRunPausePointStatusRejectsInvalidExpectValue(t *testing.T) { + stubPausePointStatusHitWithSpeed(t, "5") + + code, output := runPausePointStatusForExpect(t, []string{"--id", "jump", "--expect", "speed"}) + + if code == 0 { + t.Fatalf("expected failure for an --expect value without '=': %s", output) + } +} + +// Verifies pause-point-status --help advertises --expect, so the flag is discoverable from the +// command that accepts it. +func TestPausePointStatusHelpAdvertisesExpect(t *testing.T) { + var stdout bytes.Buffer + printNativeCommandHelp(clicore.PausePointStatusUserCommandName, &stdout) + + if !strings.Contains(stdout.String(), "--expect") { + t.Fatalf("pause-point-status help must list --expect: %s", stdout.String()) + } +} diff --git a/cli/project-runner/internal/projectrunner/pause_point_trigger_diagnosis.go b/cli/project-runner/internal/projectrunner/pause_point_trigger_diagnosis.go new file mode 100644 index 0000000000..39861450bb --- /dev/null +++ b/cli/project-runner/internal/projectrunner/pause_point_trigger_diagnosis.go @@ -0,0 +1,75 @@ +package projectrunner + +import "encoding/json" + +// pausePointTriggerResponseView is the part of a triggered command's response this diagnosis reads. +// Success is a pointer so a response that omits it is treated as "unknown", not as a failure. +type pausePointTriggerResponseView struct { + Success *bool `json:"Success"` + RejectedByActivePausePointId string `json:"RejectedByActivePausePointId"` +} + +// pausePointTriggerRefusalWarning warns about the case where the marker was hit before the trigger +// ran, so Unity refused the trigger for being called while PlayMode was paused and no input reached +// the game at all. The hit still reports success, which makes this indistinguishable from a real +// input-driven hit unless it is called out. +// +// The refusing pause point's id must match the marker being awaited: a PlayMode paused by some other +// marker is a different problem, and the advice below would be wrong for it. InterruptedByPausePoint +// is deliberately not consulted — it marks the working case, where the marker was hit while the +// input was being applied. +func pausePointTriggerRefusalWarning(result *pausePointTriggerResult, awaitedPausePointID string) string { + response, ok := decodePausePointTriggerResponse(result) + if !ok || response.Success == nil || *response.Success { + return "" + } + if response.RejectedByActivePausePointId != awaitedPausePointID { + return "" + } + + return "The marker was hit before the trigger ran, so Unity refused the trigger for running " + + "while PlayMode was paused and no input reached the game. This hit is not evidence about the " + + "trigger's input: hold the key down with a separate simulate-keyboard KeyDown call before " + + "arming the marker, or move the marker to a line reached after the input is applied. A marker " + + "placed after the input would have paused with the input already in effect." +} + +// pausePointTriggerFailedPointer reports whether the trigger failed, or nil when there is nothing to +// report: no trigger ran, or it never finished so its outcome is unknown. +func pausePointTriggerFailedPointer(result *pausePointTriggerResult) *bool { + if !pausePointTriggerFailed(result) { + return nil + } + failed := true + return &failed +} + +// pausePointTriggerFailed reports whether a completed trigger is known to have failed, either +// because its dispatch failed outright (Error) or because it ran and reported Success:false. +func pausePointTriggerFailed(result *pausePointTriggerResult) bool { + if result == nil || !result.Completed { + return false + } + if result.Error != "" { + return true + } + + response, ok := decodePausePointTriggerResponse(result) + return ok && response.Success != nil && !*response.Success +} + +// decodePausePointTriggerResponse reads the fields this diagnosis needs out of the triggered +// command's raw response, leaving TriggerResult.Response itself untouched. An unfinished trigger or +// an unparseable response yields no view: neither can be diagnosed, and guessing either way is worse +// than reporting nothing. +func decodePausePointTriggerResponse(result *pausePointTriggerResult) (pausePointTriggerResponseView, bool) { + if result == nil || !result.Completed || len(result.Response) == 0 { + return pausePointTriggerResponseView{}, false + } + + view := pausePointTriggerResponseView{} + if err := json.Unmarshal(result.Response, &view); err != nil { + return pausePointTriggerResponseView{}, false + } + return view, true +} diff --git a/cli/project-runner/internal/projectrunner/pause_point_trigger_diagnosis_test.go b/cli/project-runner/internal/projectrunner/pause_point_trigger_diagnosis_test.go new file mode 100644 index 0000000000..178eb8a24a --- /dev/null +++ b/cli/project-runner/internal/projectrunner/pause_point_trigger_diagnosis_test.go @@ -0,0 +1,273 @@ +package projectrunner + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "io" + "strings" + "testing" + "time" + + "github.com/hatayama/unity-cli-loop/common/unityipc" +) + +// pausePointRejectedTriggerResponse is the response Unity produces when a simulate command is +// refused before it runs anything because a pause point is holding PlayMode paused. Only Success and +// RejectedByActivePausePointId matter to the diagnosis; the rest is kept for realism. +func pausePointRejectedTriggerResponse(rejectedByID string) string { + return `{"Success":false,` + + `"Message":"PlayMode is paused because pause point '` + rejectedByID + `' is active ...",` + + `"InterruptedByPausePoint":false,"PausePointId":null,` + + `"RejectedByActivePausePointId":"` + rejectedByID + `"}` +} + +func stubPausePointHit(t *testing.T, unityWarning string) { + t.Helper() + + originalQuery := queryPausePointStatus + t.Cleanup(func() { + queryPausePointStatus = originalQuery + }) + + queryPausePointStatus = func( + ctx context.Context, + connection unityipc.Connection, + id string, + ) (pausePointStatusResponse, error) { + return pausePointStatusResponse{ + Success: true, + Id: id, + Status: pausePointStatusHit, + IsHit: true, + HitCount: 1, + Warning: unityWarning, + EditorState: pausePointEditorState{IsPlaying: true, IsPaused: true, CapturedAt: "PausePointHit"}, + }, nil + } +} + +func stubPausePointMatchingLogs(t *testing.T, fetchError error) { + t.Helper() + + originalFetch := fetchMatchingLogs + t.Cleanup(func() { + fetchMatchingLogs = originalFetch + }) + + fetchMatchingLogs = func( + ctx context.Context, + connection unityipc.Connection, + searchText string, + maxCount int, + ) (pausePointMatchingLogsResult, error) { + if fetchError != nil { + return pausePointMatchingLogsResult{}, fetchError + } + return pausePointMatchingLogsResult{ + SearchText: searchText, + TotalCount: 1, + DisplayedCount: 1, + MaxCount: maxCount, + Logs: []pausePointMatchingLog{{Type: "Log", Message: "[jump] hit"}}, + }, nil + } +} + +// stubPausePointTriggerDispatch makes the triggered command produce triggerStdout, which the wait +// path turns into TriggerResult.Response exactly as a real dispatch would. +func stubPausePointTriggerDispatch(t *testing.T, triggerStdout string) { + t.Helper() + + originalDispatch := dispatchPausePointTriggerCommand + t.Cleanup(func() { + dispatchPausePointTriggerCommand = originalDispatch + }) + + dispatchPausePointTriggerCommand = func( + ctx context.Context, + connection unityipc.Connection, + command string, + commandArgs []string, + startPath string, + stdout io.Writer, + stderr io.Writer, + ) int { + _, _ = stdout.Write([]byte(triggerStdout)) + return 0 + } +} + +func runAwaitWithStubbedTrigger(t *testing.T) (int, string) { + t.Helper() + + var stdout bytes.Buffer + var stderr bytes.Buffer + code := runWaitForPausePoint(context.Background(), unityipc.Connection{}, waitForPausePointOptions{ + id: "jump", + timeoutSeconds: 1, + timeout: time.Second, + matchingLogsMaxCount: 5, + triggerCommand: "simulate-keyboard", + triggerArgs: []string{"--action", "Press", "--key", "W"}, + }, &stdout, &stderr) + if stderr.Len() > 0 { + t.Logf("stderr: %s", stderr.String()) + } + return code, stdout.String() +} + +func decodePausePointWaitResult(t *testing.T, output string) pausePointWaitResult { + t.Helper() + + result := pausePointWaitResult{} + if err := json.Unmarshal([]byte(output), &result); err != nil { + t.Fatalf("stdout parse failed: %v from %s", err, output) + } + return result +} + +// Verifies a trigger refused by the very marker being awaited is called out end to end: the hit +// still reads as a success, so without this the refusal stays buried in TriggerResult.Response. +func TestRunWaitForPausePointWarnsWhenTheTriggerWasRefusedByThisMarker(t *testing.T) { + stubPausePointHit(t, "") + stubPausePointMatchingLogs(t, nil) + stubPausePointTriggerDispatch(t, pausePointRejectedTriggerResponse("jump")) + + code, output := runAwaitWithStubbedTrigger(t) + + if code != 0 { + t.Fatalf("expected the hit to stay a success, got %d: %s", code, output) + } + // Asserted on the raw stdout, not only through the decoded struct: callers read this by its wire + // name, so a renamed json tag must fail here rather than pass a struct round trip. + if !strings.Contains(output, `"TriggerFailed": true`) { + t.Errorf("TriggerFailed must appear at the top level under that exact name: %s", output) + } + result := decodePausePointWaitResult(t, output) + if !strings.Contains(result.Warning, "refused") { + t.Errorf("expected a refusal warning: %q", result.Warning) + } + if result.TriggerFailed == nil || !*result.TriggerFailed { + t.Errorf("TriggerFailed must be promoted to the top level: %#v", result.TriggerFailed) + } +} + +// Verifies the refusal warning still reaches the caller when the matching-log fetch fails, the +// branch that used to build a payload with no Warning field at all. +func TestRunWaitForPausePointKeepsTheRefusalWarningWhenTheLogFetchFails(t *testing.T) { + stubPausePointHit(t, "") + stubPausePointMatchingLogs(t, errors.New("unity busy")) + stubPausePointTriggerDispatch(t, pausePointRejectedTriggerResponse("jump")) + + _, output := runAwaitWithStubbedTrigger(t) + + if strings.Contains(output, `"MatchingLogs"`) { + t.Errorf("a failed fetch must omit MatchingLogs entirely: %s", output) + } + result := decodePausePointWaitResult(t, output) + if !strings.Contains(result.Warning, "refused") { + t.Errorf("expected the refusal warning despite the failed log fetch: %q", result.Warning) + } + if result.TriggerFailed == nil || !*result.TriggerFailed { + t.Errorf("TriggerFailed must survive the failed log fetch: %#v", result.TriggerFailed) + } +} + +// Verifies Unity's own warning survives alongside the CLI's. Both use the JSON name "Warning", where +// the CLI's outer field shadows the embedded Unity one, so Unity's text is lost unless joined in. +func TestRunWaitForPausePointKeepsUnityWarningAlongsideCliWarnings(t *testing.T) { + stubPausePointHit(t, "Unity-side enable warning.") + stubPausePointMatchingLogs(t, nil) + stubPausePointTriggerDispatch(t, pausePointRejectedTriggerResponse("jump")) + + _, output := runAwaitWithStubbedTrigger(t) + + result := decodePausePointWaitResult(t, output) + if !strings.Contains(result.Warning, "Unity-side enable warning.") { + t.Errorf("Unity's warning was dropped: %q", result.Warning) + } + if !strings.Contains(result.Warning, "refused") { + t.Errorf("the CLI warning was dropped: %q", result.Warning) + } +} + +// Verifies the refusal is only blamed on this wait when the refusing marker is the one being +// awaited, so a pause owned by some other marker does not produce advice about this one. +func TestPausePointTriggerRefusalWarningRequiresTheAwaitedMarker(t *testing.T) { + refusedByThisMarker := &pausePointTriggerResult{ + Completed: true, + Response: json.RawMessage(pausePointRejectedTriggerResponse("jump")), + } + if pausePointTriggerRefusalWarning(refusedByThisMarker, "jump") == "" { + t.Error("expected a warning when this marker refused the trigger") + } + + refusedByAnotherMarker := &pausePointTriggerResult{ + Completed: true, + Response: json.RawMessage(pausePointRejectedTriggerResponse("other-marker")), + } + if warning := pausePointTriggerRefusalWarning(refusedByAnotherMarker, "jump"); warning != "" { + t.Errorf("a refusal by another marker must not be warned about here: %q", warning) + } +} + +// Verifies a marker hit while the trigger's input was being applied produces no warning: that is +// the normal, working case, and the earlier draft predicate warned on exactly it. +func TestPausePointTriggerRefusalWarningIgnoresAMidExecutionInterruption(t *testing.T) { + interrupted := &pausePointTriggerResult{ + Completed: true, + Response: json.RawMessage( + `{"Success":true,"InterruptedByPausePoint":true,"PausePointId":"jump"}`), + } + + if warning := pausePointTriggerRefusalWarning(interrupted, "jump"); warning != "" { + t.Errorf("a mid-execution interruption is the normal case: %q", warning) + } +} + +// Verifies a trigger that never reported back within the grace window is neither warned about nor +// called failed: its outcome is unknown, and claiming failure would be as wrong as claiming success. +func TestPausePointTriggerDiagnosisTreatsAnIncompleteTriggerAsUnknown(t *testing.T) { + incomplete := &pausePointTriggerResult{Completed: false} + + if warning := pausePointTriggerRefusalWarning(incomplete, "jump"); warning != "" { + t.Errorf("an unfinished trigger cannot be diagnosed: %q", warning) + } + if pausePointTriggerFailed(incomplete) { + t.Error("an unfinished trigger has no known outcome") + } +} + +// Verifies the two failure shapes a completed trigger can have are both promoted, and a plain +// success is not. +func TestPausePointTriggerFailedCoversBothFailureShapes(t *testing.T) { + failedResponse := &pausePointTriggerResult{ + Completed: true, + Response: json.RawMessage(`{"Success":false,"Message":"no keyboard device found"}`), + } + if !pausePointTriggerFailed(failedResponse) { + t.Error("a completed trigger reporting Success:false has failed") + } + + failedDispatch := &pausePointTriggerResult{ + Completed: true, + Error: `{"Error":{"ErrorCode":"UNITY_NOT_REACHABLE"}}`, + } + if !pausePointTriggerFailed(failedDispatch) { + t.Error("a trigger whose dispatch failed has failed") + } + + succeeded := &pausePointTriggerResult{ + Completed: true, + Response: json.RawMessage(`{"Success":true}`), + } + if pausePointTriggerFailed(succeeded) { + t.Error("a successful trigger must not be reported as failed") + } + + if pausePointTriggerFailed(nil) { + t.Error("no trigger at all cannot have failed") + } +} diff --git a/cli/project-runner/internal/projectrunner/pause_point_types.go b/cli/project-runner/internal/projectrunner/pause_point_types.go index 874ca1174b..4594e41f54 100644 --- a/cli/project-runner/internal/projectrunner/pause_point_types.go +++ b/cli/project-runner/internal/projectrunner/pause_point_types.go @@ -51,6 +51,13 @@ type pausePointStatusResponse struct { // CapturedVariables array for "nothing was captured at this hit". CapturedVariableNameFilterNoMatch bool `json:"CapturedVariableNameFilterNoMatch,omitempty"` + // CapturedVariableNamesNotFound is set by the CLI, not Unity: the requested + // --captured-variable-names that matched no captured variable, in the order they were + // requested. Without it a partial match is indistinguishable from a full one, since the + // response only carries the names that did match and CapturedVariableNameFilterNoMatch covers + // the all-or-nothing case. Both are emitted when nothing matched at all. + CapturedVariableNamesNotFound []string `json:"CapturedVariableNamesNotFound,omitempty"` + // TriggerResult is set by the CLI, not Unity, only when --trigger was passed. It is omitted // entirely otherwise, so callers that never use --trigger see no schema change at all. TriggerResult *pausePointTriggerResult `json:"TriggerResult,omitempty"` @@ -58,6 +65,26 @@ type pausePointStatusResponse struct { // ResumePlayResult is set by the CLI, not Unity, only when --resume-play was passed. It is // omitted entirely otherwise, matching TriggerResult's omit-when-unused contract. ResumePlayResult *pausePointResumePlayResult `json:"ResumePlayResult,omitempty"` + + // TriggerFailed is set by the CLI, not Unity, only when --trigger was passed and the trigger is + // known to have failed. It repeats at the top level what TriggerResult already carries three + // levels down, because the loss it guards against is a caller reading Success:true / Status:Hit + // and never opening TriggerResult at all. A pointer so the field is absent — rather than a + // misleading false — when no trigger ran or its outcome is unknown. + TriggerFailed *bool `json:"TriggerFailed,omitempty"` +} + +// pausePointStatusResult wraps a status response with the CLI-evaluated --expect verdicts. +// pause-point-status marshals the Unity response directly, so it needs this wrapper to carry the +// two extra fields; the names match pausePointWaitResult's so one query shape reads both commands. +type pausePointStatusResult struct { + pausePointStatusResponse + + // Both fields are omitted unless --expect was passed, and AllExpectationsPassed is a pointer + // for the same reason as on pausePointWaitResult: to distinguish "no --expect given" from + // "the given expectations failed". + Expectations []pausePointExpectationResult `json:"Expectations,omitempty"` + AllExpectationsPassed *bool `json:"AllExpectationsPassed,omitempty"` } type pausePointEditorState struct { diff --git a/cli/project-runner/internal/projectrunner/pause_point_unknown_option.go b/cli/project-runner/internal/projectrunner/pause_point_unknown_option.go new file mode 100644 index 0000000000..1ee7fc3c9a --- /dev/null +++ b/cli/project-runner/internal/projectrunner/pause_point_unknown_option.go @@ -0,0 +1,129 @@ +package projectrunner + +import ( + "fmt" + "strings" + + clierrors "github.com/hatayama/unity-cli-loop/common/errors" + + "github.com/hatayama/unity-cli-loop/common/clicore" + "github.com/hatayama/unity-cli-loop/common/tooldocs" +) + +// pausePointFlagOwnerSearchOrder fixes the order in which an unknown flag's owning command is +// resolved. Several pause-point commands accept the same flag name (--id and --timeout-seconds +// exist on more than one), so a map-order search would report a different owner from run to run. +// enable-pause-point comes first because it owns the largest flag set and is the command whose +// flags are most often reached for while inspecting an already-armed marker. +var pausePointFlagOwnerSearchOrder = []string{ + pausePointEnableCommandName, + clicore.PausePointAwaitCommandName, + clicore.PausePointStatusUserCommandName, +} + +// pausePointCarriedOverEnableFlagNames are the enable-pause-point flags whose values Unity reports +// back on every later status response (as Mode, MaxHistory, MaxPreviewElements and TimeoutSeconds). +// Passing one of these to a query command is not just misplaced, it is unnecessary — which is the +// part a caller cannot infer from "wrong command" alone. +var pausePointCarriedOverEnableFlagNames = []string{ + "mode", + "max-history", + "max-preview-elements", + PausePointTimeoutFlagName, +} + +// pausePointUnknownOptionError reports an unrecognized flag for a runner-owned native command. +// A flag that belongs to another pause-point command is reported as such, since naming the real +// owner (and, for enable-time settings, saying the value is already in this response) is what lets +// the caller recover without a second round trip. A flag that exists nowhere is reported as a plain +// unknown option: it cannot be a case of documentation running ahead of this build. +func pausePointUnknownOptionError(command string, name string) *clierrors.ArgumentError { + message := fmt.Sprintf("Unknown option %q for %s.", "--"+name, command) + if owner, ok := pausePointFlagOwnerCommand(name); ok && owner != command { + message = fmt.Sprintf("--%s is %s %s option, not %s %s one.", + name, + indefiniteArticleFor(owner), owner, + indefiniteArticleFor(command), command) + if owner == pausePointEnableCommandName && isPausePointCarriedOverEnableFlag(name) { + message += " The value passed to " + pausePointEnableCommandName + + " is already applied to the response of this command, so it does not need to be passed again here." + } + } + + return &clierrors.ArgumentError{ + Message: message, + Option: "--" + name, + Command: command, + NextActions: []string{fmt.Sprintf("Run `uloop %s --help` to list the accepted options.", command)}, + } +} + +// indefiniteArticleFor picks the article for a command name interpolated into a message. Command +// names are lower-case ASCII identifiers, so the initial letter decides it — "an await-pause-point +// option" rather than "a await-pause-point option". Both slots of the owner sentence go through +// this, so neither reads as broken English for a vowel-initial command. +func indefiniteArticleFor(commandName string) string { + if commandName == "" { + return "a" + } + if strings.ContainsRune("aeiou", rune(commandName[0])) { + return "an" + } + return "a" +} + +// pausePointFlagOwnerCommand reports which pause-point command accepts the flag, searching in a +// fixed order so the answer never depends on map iteration. +func pausePointFlagOwnerCommand(name string) (string, bool) { + for _, command := range pausePointFlagOwnerSearchOrder { + for _, flagName := range pausePointCommandFlagNames(command) { + if flagName == name { + return command, true + } + } + } + return "", false +} + +// pausePointCommandFlagNames lists the flag names a pause-point command accepts, without the "--" +// prefix. Both sources are the same tables the commands' own --help output is built from, so a flag +// added to a command becomes recognizable here without a second registration. +func pausePointCommandFlagNames(command string) []string { + if command == pausePointEnableCommandName { + return pausePointEnableFlagNames() + } + + names := make([]string, 0, len(runnerNativeCommandOptions[command])) + for _, option := range runnerNativeCommandOptions[command] { + names = append(names, strings.TrimPrefix(option, "--")) + } + return names +} + +// pausePointEnableFlagNames lists enable-pause-point's CLI-only flags plus the ones derived from its +// Unity schema, since the misuse this message exists for (--max-preview-elements on a query command) +// is a schema-derived flag. +func pausePointEnableFlagNames() []string { + names := make([]string, 0) + for _, option := range tooldocs.PausePointEnableCLIOnlyOptions() { + names = append(names, option.FlagName) + } + + tool, ok := clicore.FindTool(clicore.LoadDefaultTools(), pausePointEnableCommandName) + if !ok { + return names + } + for propertyName, property := range tool.EffectiveInputSchema().Properties { + names = append(names, tooldocs.OptionNameForProperty(tool.Name, propertyName, property)) + } + return names +} + +func isPausePointCarriedOverEnableFlag(name string) bool { + for _, flagName := range pausePointCarriedOverEnableFlagNames { + if flagName == name { + return true + } + } + return false +} diff --git a/cli/project-runner/internal/projectrunner/pause_point_unknown_option_test.go b/cli/project-runner/internal/projectrunner/pause_point_unknown_option_test.go new file mode 100644 index 0000000000..21289b5a5a --- /dev/null +++ b/cli/project-runner/internal/projectrunner/pause_point_unknown_option_test.go @@ -0,0 +1,144 @@ +package projectrunner + +import ( + "encoding/json" + "strings" + "testing" + + "github.com/hatayama/unity-cli-loop/common/clicore" +) + +// Verifies a flag that belongs to enable-pause-point names its real owner and states that the value +// given at enable time already shows up here, which is the round trip the original message cost: +// the flag exists, so "your runner may be outdated" was never the answer. +func TestParsePausePointStatusUnknownOptionNamesEnableAsTheOwner(t *testing.T) { + _, err := parsePausePointStatusOptions([]string{"--id", "jump", "--max-preview-elements", "5"}) + + if err == nil { + t.Fatal("expected error for an enable-pause-point flag passed to pause-point-status") + } + message := err.Error() + if !strings.Contains(message, "--max-preview-elements is an enable-pause-point option, not a pause-point-status one.") { + t.Errorf("owner sentence missing: %s", message) + } + if !strings.Contains( + message, + "The value passed to enable-pause-point is already applied to the response of this command, "+ + "so it does not need to be passed again here.") { + t.Errorf("carry-over sentence missing: %s", message) + } + if strings.Contains(message, "older than the docs") { + t.Errorf("stale-runner hint must be gone: %s", message) + } +} + +// Verifies an enable-pause-point flag whose value is not carried into this command's response names +// the owner without claiming a carry-over that does not happen. +func TestParsePausePointStatusUnknownOptionOmitsCarryOverForNonCarriedFlags(t *testing.T) { + _, err := parsePausePointStatusOptions([]string{"--id", "jump", "--line", "42"}) + + if err == nil { + t.Fatal("expected error for --line passed to pause-point-status") + } + message := err.Error() + if !strings.Contains(message, "--line is an enable-pause-point option, not a pause-point-status one.") { + t.Errorf("owner sentence missing: %s", message) + } + if strings.Contains(message, "already applied to the response") { + t.Errorf("carry-over sentence must not be claimed for --line: %s", message) + } +} + +// Verifies a flag owned by another runner-owned command names that command, so a flag borrowed from +// await-pause-point is not reported as an enable-pause-point one. +func TestParsePausePointStatusUnknownOptionNamesAwaitAsTheOwner(t *testing.T) { + _, err := parsePausePointStatusOptions( + []string{"--id", "jump", "--" + PausePointLogsMaxCountFlagName, "3"}) + + if err == nil { + t.Fatal("expected error for an await-pause-point flag passed to pause-point-status") + } + if !strings.Contains( + err.Error(), + "--"+PausePointLogsMaxCountFlagName+" is an await-pause-point option, not a pause-point-status one.") { + t.Errorf("owner sentence missing: %s", err.Error()) + } +} + +// Verifies the article agrees with the command name in both slots of the owner sentence, so a +// vowel-initial command such as await-pause-point does not produce "a await-pause-point one". +func TestPausePointUnknownOptionArticleAgreesWithTheCommandName(t *testing.T) { + _, err := parseWaitForPausePointOptions( + []string{"--id", "jump", "--max-preview-elements", "5"}) + + if err == nil { + t.Fatal("expected error for an enable-pause-point flag passed to await-pause-point") + } + if !strings.Contains( + err.Error(), + "--max-preview-elements is an enable-pause-point option, not an await-pause-point one.") { + t.Errorf("article mismatch in the owner sentence: %s", err.Error()) + } + + if article := indefiniteArticleFor(clicore.PausePointStatusUserCommandName); article != "a" { + t.Errorf("a consonant-initial command takes \"a\", got %q", article) + } +} + +// Verifies the owner reported for a flag several commands accept is fixed rather than dependent on +// map iteration order, so the same misuse always produces the same message. +func TestPausePointUnknownOptionOwnerIsDeterministic(t *testing.T) { + const flagName = PausePointTimeoutFlagName + + first, ok := pausePointFlagOwnerCommand(flagName) + if !ok { + t.Fatalf("--%s must resolve to an owning command", flagName) + } + if first != pausePointEnableCommandName { + t.Errorf("owner of --%s must be the first command in the fixed search order, got %q", flagName, first) + } + for attempt := 0; attempt < 20; attempt++ { + owner, _ := pausePointFlagOwnerCommand(flagName) + if owner != first { + t.Fatalf("owner of --%s changed between calls: %q then %q", flagName, first, owner) + } + } +} + +// Verifies the carry-over sentence is only claimed for the enable-time settings the status response +// actually reports back, so the message never promises evidence the response cannot show. +func TestPausePointCarriedOverEnableFlagsAreVisibleInTheStatusResponse(t *testing.T) { + response, err := json.Marshal(pausePointStatusResponse{ + Mode: "continuous", + MaxHistory: 20, + MaxPreviewElements: 5, + TimeoutSeconds: 30, + }) + if err != nil { + t.Fatalf("failed to marshal status response: %v", err) + } + + carriedOverFields := map[string]string{ + "mode": "Mode", + "max-history": "MaxHistory", + "max-preview-elements": "MaxPreviewElements", + "timeout-seconds": "TimeoutSeconds", + } + if len(carriedOverFields) != len(pausePointCarriedOverEnableFlagNames) { + t.Fatalf("carry-over flag list changed: %v", pausePointCarriedOverEnableFlagNames) + } + for _, flagName := range pausePointCarriedOverEnableFlagNames { + field, ok := carriedOverFields[flagName] + if !ok { + t.Errorf("--%s is described as carried over but has no known status response field", flagName) + continue + } + if !strings.Contains(string(response), `"`+field+`"`) { + t.Errorf("status response does not report %s for --%s", field, flagName) + } + if owner, ok := pausePointFlagOwnerCommand(flagName); !ok || owner != pausePointEnableCommandName { + t.Errorf("--%s is listed as a carried-over enable flag but resolves to owner %q (found=%v)", + flagName, owner, ok) + } + } +} diff --git a/cli/project-runner/internal/projectrunner/pause_point_wait.go b/cli/project-runner/internal/projectrunner/pause_point_wait.go index 2ed2c8ec31..e8f29efc75 100644 --- a/cli/project-runner/internal/projectrunner/pause_point_wait.go +++ b/cli/project-runner/internal/projectrunner/pause_point_wait.go @@ -61,6 +61,7 @@ type pausePointStatusOptions struct { id string capturedVariablesMode pausePointCapturedVariablesMode capturedVariableNames []string + expectations []pausePointExpectation } func normalizePausePointStatusResponse(response pausePointStatusResponse) pausePointStatusResponse { @@ -171,10 +172,21 @@ func runPausePointStatusCommand( } response = normalizePausePointStatusResponse(response) response = filterPausePointCapturedVariableHistory(response) + // Evaluated against the raw CapturedVariables, before the filters below can narrow or strip + // values, for the same reason as on the await path (runWaitForPausePoint): otherwise an --expect + // target not also requested via --captured-variable-names, or whose value names mode stripped, + // would be reported as missing or failing. + expectations := evaluatePausePointExpectations(response.CapturedVariables, options.expectations) response = filterPausePointCapturedVariablesByName(response, options.capturedVariableNames) response = applyPausePointCapturedVariablesMode(response, options.capturedVariablesMode) - result, err := json.Marshal(response) + // Expectation verdicts never change the exit code: whether the query succeeded and whether the + // captured state matched are separate questions, as on await-pause-point. + result, err := json.Marshal(pausePointStatusResult{ + pausePointStatusResponse: response, + Expectations: expectations, + AllExpectationsPassed: pausePointAllExpectationsPassedPointer(expectations), + }) if err != nil { clierrors.WriteClassifiedError(stderr, err, clierrors.ErrorContext{ ProjectRoot: connection.ProjectRoot, @@ -218,35 +230,16 @@ func runWaitForPausePoint( response = filterPausePointCapturedVariablesByName(response, options.capturedVariableNames) response = applyPausePointCapturedVariablesMode(response, options.capturedVariablesMode) // Best-effort: a hit must stay a success even if Unity is busy while paused. - // On fetch failure MatchingLogs is omitted entirely, so an empty array always - // means "the fetch succeeded and no matching log exists". - var payload any = response logs, logsErr := fetchMatchingLogs(ctx, connection, options.id, options.matchingLogsMaxCount) - switch { - case logsErr == nil: - payload = pausePointWaitResult{ - pausePointStatusResponse: response, - MatchingLogs: logs.Logs, - Warning: buildPausePointWarning(logs, response.HitCount), - Expectations: expectations, - AllExpectationsPassed: pausePointAllExpectationsPassedPointer(expectations), - } - case len(expectations) > 0: - // Best-effort: a failed log fetch must not also drop --expect results, since that is - // the only evidence a caller asked for by name in this branch. Uses an anonymous - // struct (not pausePointWaitResult) so MatchingLogs is omitted entirely rather than - // serialized as an empty array, preserving "empty array only means a successful - // fetch with no matches". - payload = struct { - pausePointStatusResponse - Expectations []pausePointExpectationResult `json:"Expectations,omitempty"` - AllExpectationsPassed *bool `json:"AllExpectationsPassed,omitempty"` - }{ - pausePointStatusResponse: response, - Expectations: expectations, - AllExpectationsPassed: pausePointAllExpectationsPassedPointer(expectations), - } - } + payload := buildPausePointHitPayload(pausePointHitPayloadInputs{ + response: response, + logs: logs, + logsErr: logsErr, + unityWarning: response.Warning, + triggerResult: triggerResult, + awaitedPausePointID: options.id, + expectations: expectations, + }) result, marshalErr := json.Marshal(payload) if marshalErr != nil { clierrors.WriteClassifiedError(stderr, marshalErr, clierrors.ErrorContext{ @@ -397,6 +390,12 @@ func parsePausePointStatusOptions(args []string) (pausePointStatusOptions, error options.capturedVariablesMode = mode case tooldocs.PausePointCapturedVariableNamesFlagName: options.capturedVariableNames = parsePausePointCapturedVariableNames(value) + case tooldocs.PausePointExpectFlagName: + expectation, parseErr := parsePausePointExpectFlagValue(value) + if parseErr != nil { + return pausePointStatusOptions{}, parseErr + } + options.expectations = append(options.expectations, expectation) default: return pausePointStatusOptions{}, pausePointUnknownOptionError(clicore.PausePointStatusUserCommandName, name) } diff --git a/cli/project-runner/internal/projectrunner/pause_point_wait_test.go b/cli/project-runner/internal/projectrunner/pause_point_wait_test.go index 55eb131e5a..275927d1e3 100644 --- a/cli/project-runner/internal/projectrunner/pause_point_wait_test.go +++ b/cli/project-runner/internal/projectrunner/pause_point_wait_test.go @@ -1126,27 +1126,27 @@ func TestRunProjectLocalPausePointStatusRespectsToolSettings(t *testing.T) { } } -// Verifies an unrecognized flag on await-pause-point/pause-point-status carries a hint -// that the installed project runner may be older than the skill docs. -func TestParseUnknownOptionErrorsIncludeOutdatedRunnerHint(t *testing.T) { - wantHint := "Unknown option \"--bogus-flag\" for await-pause-point. If the skill documentation mentions this option, the installed project runner may be older than the docs — check 'uloop --version' and update the CLI." +// Verifies a flag that exists nowhere is reported as a plain unknown option: no stale-runner hint, +// since the flag being absent from every command means the docs cannot be ahead of this build. +func TestParseUnknownOptionErrorsOmitStaleRunnerHint(t *testing.T) { + wantMessage := `Unknown option "--bogus-flag" for await-pause-point.` _, err := parseWaitForPausePointOptions([]string{"--id", "jump", "--bogus-flag", "value"}) if err == nil { t.Fatal("expected error for unknown flag") } - if err.Error() != wantHint { - t.Fatalf("await-pause-point hint mismatch: %v", err) + if err.Error() != wantMessage { + t.Fatalf("await-pause-point message mismatch: %v", err) } - wantStatusHint := "Unknown option \"--bogus-flag\" for pause-point-status. If the skill documentation mentions this option, the installed project runner may be older than the docs — check 'uloop --version' and update the CLI." + wantStatusMessage := `Unknown option "--bogus-flag" for pause-point-status.` _, statusErr := parsePausePointStatusOptions([]string{"--id", "jump", "--bogus-flag", "value"}) if statusErr == nil { t.Fatal("expected error for unknown flag") } - if statusErr.Error() != wantStatusHint { - t.Fatalf("pause-point-status hint mismatch: %v", statusErr) + if statusErr.Error() != wantStatusMessage { + t.Fatalf("pause-point-status message mismatch: %v", statusErr) } } diff --git a/cli/project-runner/internal/projectrunner/pause_point_warning_join_test.go b/cli/project-runner/internal/projectrunner/pause_point_warning_join_test.go new file mode 100644 index 0000000000..4f7dbb09f3 --- /dev/null +++ b/cli/project-runner/internal/projectrunner/pause_point_warning_join_test.go @@ -0,0 +1,55 @@ +package projectrunner + +import ( + "bytes" + "context" + "errors" + "strings" + "testing" + "time" + + "github.com/hatayama/unity-cli-loop/common/unityipc" +) + +func runAwaitWithoutTrigger(t *testing.T) string { + t.Helper() + + var stdout bytes.Buffer + var stderr bytes.Buffer + code := runWaitForPausePoint(context.Background(), unityipc.Connection{}, waitForPausePointOptions{ + id: "jump", + timeoutSeconds: 1, + timeout: time.Second, + matchingLogsMaxCount: 5, + }, &stdout, &stderr) + if code != 0 { + t.Fatalf("expected the hit to be a success, got %d with stderr %s", code, stderr.String()) + } + return stdout.String() +} + +// Verifies Unity's own warning reaches the caller on a hit. The CLI's Warning field shadows the +// embedded Unity one — both serialize as "Warning" — so Unity's text is dropped unless joined in. +func TestRunWaitForPausePointKeepsUnityWarningOnAHit(t *testing.T) { + stubPausePointHit(t, "Unity-side enable warning.") + stubPausePointMatchingLogs(t, nil) + + result := decodePausePointWaitResult(t, runAwaitWithoutTrigger(t)) + + if !strings.Contains(result.Warning, "Unity-side enable warning.") { + t.Errorf("Unity's warning was dropped: %q", result.Warning) + } +} + +// Verifies Unity's warning also survives the failed-log-fetch branch, which builds a different +// payload shape and previously had no Warning field at all. +func TestRunWaitForPausePointKeepsUnityWarningWhenTheLogFetchFails(t *testing.T) { + stubPausePointHit(t, "Unity-side enable warning.") + stubPausePointMatchingLogs(t, errors.New("unity busy")) + + result := decodePausePointWaitResult(t, runAwaitWithoutTrigger(t)) + + if !strings.Contains(result.Warning, "Unity-side enable warning.") { + t.Errorf("Unity's warning was dropped on the failed-fetch branch: %q", result.Warning) + } +}