diff --git a/cli/common/tooldocs/pause_point_cli_options.go b/cli/common/tooldocs/pause_point_cli_options.go index 4c7c8e037..75be927fa 100644 --- a/cli/common/tooldocs/pause_point_cli_options.go +++ b/cli/common/tooldocs/pause_point_cli_options.go @@ -74,7 +74,7 @@ func PausePointEnableCLIOnlyOptions() []PausePointCLIOnlyOption { { FlagName: PausePointTriggerFlagName, Type: "string", - Description: "Requires --await. Same as await-pause-point's --trigger", + Description: "Requires --await. Same as await-pause-point's --trigger: a uloop subcommand without the leading 'uloop', e.g. \"simulate-keyboard --action Press --key Space\"", }, { FlagName: PausePointResumePlayFlagName, @@ -91,7 +91,7 @@ const ( pausePointCapturedVariablesDescription = "How much of each captured variable to include in the response" pausePointCapturedVariableNamesDescription = "Restrict CapturedVariables to these comma-separated names" pausePointExpectDescription = "Compare a captured variable against an expected value (repeatable; name=value)" - pausePointTriggerDescription = "Runs a single uloop subcommand in-process right after arming/registration" + pausePointTriggerDescription = "Runs a single uloop subcommand in-process right after arming/registration. Pass the subcommand without the leading 'uloop', e.g. \"simulate-keyboard --action Press --key Space\"" pausePointResumePlayDescription = "After confirming the marker is armed, resume PlayMode if paused " + "(before --trigger), so a paused-arm workflow can fire input in one call" ) diff --git a/cli/common/tooldocs/pause_point_cli_options_test.go b/cli/common/tooldocs/pause_point_cli_options_test.go index a9b5bac8f..440878164 100644 --- a/cli/common/tooldocs/pause_point_cli_options_test.go +++ b/cli/common/tooldocs/pause_point_cli_options_test.go @@ -100,6 +100,37 @@ func TestVisibleOptionHelpEntriesOmitPausePointCLIOnlyOptionsForOtherTools(t *te } } +const ( + wantPausePointAwaitTriggerDescription = "Runs a single uloop subcommand in-process right after arming/registration. Pass the subcommand without the leading 'uloop', e.g. \"simulate-keyboard --action Press --key Space\"" + wantPausePointEnableTriggerDescription = "Requires --await. Same as await-pause-point's --trigger: a uloop subcommand without the leading 'uloop', e.g. \"simulate-keyboard --action Press --key Space\"" +) + +// Verifies both --trigger help rows state the in-process subcommand form and include an +// example without a leading uloop token. +func TestPausePointTriggerDescriptionsDocumentSubcommandForm(t *testing.T) { + var awaitDescription string + for _, option := range PausePointAwaitCLIOnlyOptions() { + if option.FlagName == PausePointTriggerFlagName { + awaitDescription = option.Description + break + } + } + if awaitDescription != wantPausePointAwaitTriggerDescription { + t.Fatalf("await --trigger description mismatch:\nwant %q\ngot %q", wantPausePointAwaitTriggerDescription, awaitDescription) + } + + var enableDescription string + for _, option := range PausePointEnableCLIOnlyOptions() { + if option.FlagName == PausePointTriggerFlagName { + enableDescription = option.Description + break + } + } + if enableDescription != wantPausePointEnableTriggerDescription { + t.Fatalf("enable --trigger description mismatch:\nwant %q\ngot %q", wantPausePointEnableTriggerDescription, enableDescription) + } +} + func findOptionHelpEntry(entries []OptionHelpEntry, name string) (OptionHelpEntry, bool) { for _, entry := range entries { if entry.Name == name { diff --git a/cli/dispatcher/shared-inputs-stamp.json b/cli/dispatcher/shared-inputs-stamp.json index c2fd13c77..5f285a12a 100644 --- a/cli/dispatcher/shared-inputs-stamp.json +++ b/cli/dispatcher/shared-inputs-stamp.json @@ -1,4 +1,4 @@ { "schemaVersion": 1, - "sharedInputsHash": "057220b1f49759a881862f4fcc38916b3d2bbcff" + "sharedInputsHash": "8db2b4ce9e3caa87c04ca9f5c44d803940c83309" } diff --git a/cli/project-runner/internal/projectrunner/native_command_help_test.go b/cli/project-runner/internal/projectrunner/native_command_help_test.go index de2f16c16..c068cd14e 100644 --- a/cli/project-runner/internal/projectrunner/native_command_help_test.go +++ b/cli/project-runner/internal/projectrunner/native_command_help_test.go @@ -42,7 +42,7 @@ func TestRunProjectLocalAwaitPausePointHelpOptionsSection(t *testing.T) { --matching-logs-max-count Maximum Console logs matching the marker id to include on a hit --resume-play After confirming the marker is armed, resume PlayMode if paused (before --trigger), so a paused-arm workflow can fire input in one call --timeout-seconds Seconds to wait for a hit before timing out - --trigger Runs a single uloop subcommand in-process right after arming/registration + --trigger Runs a single uloop subcommand in-process right after arming/registration. Pass the subcommand without the leading 'uloop', e.g. "simulate-keyboard --action Press --key Space" ` assertNativeCommandHelpOptionsSection(t, "await-pause-point", expectedOptions) } diff --git a/cli/project-runner/internal/projectrunner/pause_point_errors.go b/cli/project-runner/internal/projectrunner/pause_point_errors.go index a3bf4b828..2d885ca67 100644 --- a/cli/project-runner/internal/projectrunner/pause_point_errors.go +++ b/cli/project-runner/internal/projectrunner/pause_point_errors.go @@ -112,8 +112,7 @@ func pausePointTriggerFailedNextActions(id string) []string { fmt.Sprintf( "The marker is still armed, so you can also wait on it directly: "+ "uloop await-pause-point --id %q --trigger \"\"", id), - "Check the rejected value against the triggered command's own `--help` before retrying, so the " + - "same value is not retried twice.", + "For an INVALID_ARGUMENT rejection, check the rejected value against the triggered command's own --help; for UNKNOWN_COMMAND, the first token must be a uloop subcommand name written without the leading 'uloop'.", } } diff --git a/cli/project-runner/internal/projectrunner/pause_point_errors_test.go b/cli/project-runner/internal/projectrunner/pause_point_errors_test.go index f5e49cda0..947777094 100644 --- a/cli/project-runner/internal/projectrunner/pause_point_errors_test.go +++ b/cli/project-runner/internal/projectrunner/pause_point_errors_test.go @@ -233,6 +233,25 @@ func TestPausePointStateNextActionsKeepsCodeMarkerGuidanceWhenIdContainsColon(t } } +const wantPausePointTriggerFailedUnknownCommandNextAction = "For an INVALID_ARGUMENT rejection, check the rejected value against the triggered command's own --help; for UNKNOWN_COMMAND, the first token must be a uloop subcommand name written without the leading 'uloop'." + +// Verifies the trigger-failed third NextAction distinguishes INVALID_ARGUMENT help-checking +// from UNKNOWN_COMMAND's leading-uloop format mistake, so a prefixed value is not sent to +// the triggered command's --help. +func TestPausePointTriggerFailedNextActionsDiagnosesUnknownCommandPrefix(t *testing.T) { + got := pausePointTriggerFailedNextActions("jump") + want := []string{ + "Fix the --trigger value in the command you just ran and run that command again. Re-running " + + "`enable-pause-point --await` is safe and is the cleanest reset: it restarts the marker's " + + "HitCount and --timeout-seconds countdown, and re-patching an already patched id is a no-op.", + `The marker is still armed, so you can also wait on it directly: uloop await-pause-point --id "jump" --trigger ""`, + wantPausePointTriggerFailedUnknownCommandNextAction, + } + if !reflect.DeepEqual(got, want) { + t.Fatalf("NextActions mismatch:\n got: %#v\nwant: %#v", got, want) + } +} + // Verifies a `.cs:0` suffix is not treated as a file:line id, because enable-pause-point // rejects --line 0 and C# never emits that id. func TestPausePointStateNextActionsKeepsCodeMarkerGuidanceForZeroLine(t *testing.T) { diff --git a/cli/project-runner/internal/projectrunner/pause_point_trigger.go b/cli/project-runner/internal/projectrunner/pause_point_trigger.go index 70992d745..fa2115ea5 100644 --- a/cli/project-runner/internal/projectrunner/pause_point_trigger.go +++ b/cli/project-runner/internal/projectrunner/pause_point_trigger.go @@ -102,6 +102,12 @@ func parsePausePointTriggerCommand(command string, value string) (string, []stri triggerCommand := tokens[0] triggerArgs := tokens[1:] + // The value is dispatched in-process as argv, not through a shell. A leading "uloop" is + // therefore a command name, not a prefix, and becomes UNKNOWN_COMMAND after arming. + if triggerCommand == pausePointTriggerLeadingDispatcherToken { + return "", nil, rejectLeadingUloopTrigger(command, triggerArgs) + } + // A pause-point wait cannot make progress from inside another pause-point wait: there is no // legitimate use case, only a wasted goroutine that outlives its parent's own timeout. if triggerCommand == clicore.PausePointAwaitCommandName || triggerCommand == pausePointEnableCommandName { @@ -134,6 +140,89 @@ func parsePausePointTriggerCommand(command string, value string) (string, []stri return triggerCommand, triggerArgs, nil } +const ( + pausePointTriggerLeadingDispatcherToken = "uloop" + pausePointTriggerLeadingUloopMessage = `--trigger must name the uloop subcommand without the leading "uloop": the value runs in-process, not through a shell.` + pausePointTriggerExampleWithoutDispatcher = `simulate-keyboard --action Press --key Space` +) + +// rejectLeadingUloopTrigger reports a parse-time ArgumentError so a prefixed --trigger never +// reaches dispatch. Why not reuse pausePointTriggerCommandString: that helper joins tokens +// without quoting, which would flatten a whitespace-bearing argument such as "10 20". +func rejectLeadingUloopTrigger(command string, triggerArgs []string) error { + corrected := pausePointTriggerExampleWithoutDispatcher + if len(triggerArgs) > 0 { + candidate := formatPausePointTriggerTokens(triggerArgs) + if pausePointTriggerCorrectionIsReusable(command, triggerArgs, candidate) { + corrected = candidate + } + } + return &clierrors.ArgumentError{ + Message: pausePointTriggerLeadingUloopMessage, + Option: "--" + tooldocs.PausePointTriggerFlagName, + Command: command, + NextActions: []string{ + "Re-run with --trigger " + quotePausePointTriggerFlagValue(corrected), + }, + } +} + +// pausePointTriggerCorrectionIsReusable reports whether a reconstructed --trigger value can be +// pasted back as-is. Why not present every remainder: empty or quote-bearing tokens do not +// round-trip through the tokenizer, and nested-wait / --project-path remainders are rejected +// on the next parse. Why refuse a remainder that still starts with uloop: re-parsing it would +// re-enter rejectLeadingUloopTrigger; a leftover prefix is already an unusable correction. +func pausePointTriggerCorrectionIsReusable(command string, original []string, corrected string) bool { + tokens, err := tokenizePausePointTriggerValue(corrected) + if err != nil { + return false + } + if !pausePointTriggerTokensEqual(tokens, original) { + return false + } + if tokens[0] == pausePointTriggerLeadingDispatcherToken { + return false + } + _, _, err = parsePausePointTriggerCommand(command, corrected) + return err == nil +} + +func pausePointTriggerTokensEqual(left []string, right []string) bool { + if len(left) != len(right) { + return false + } + for index, token := range left { + if token != right[index] { + return false + } + } + return true +} + +// formatPausePointTriggerTokens joins tokens and re-quotes any token that contains whitespace +// so the reconstructed --trigger value can be pasted back unchanged. +func formatPausePointTriggerTokens(tokens []string) string { + formatted := make([]string, len(tokens)) + for index, token := range tokens { + formatted[index] = quotePausePointTriggerToken(token) + } + return strings.Join(formatted, " ") +} + +func quotePausePointTriggerToken(token string) string { + if strings.ContainsAny(token, " \t") { + return `"` + token + `"` + } + return token +} + +func quotePausePointTriggerFlagValue(value string) string { + if strings.Contains(value, `"`) { + return "'" + value + "'" + } + return `"` + value + `"` +} + // tokenizePausePointTriggerValue splits a --trigger value into argv-style tokens, honoring single // and double quotes so an argument value (for example a key name) can contain a space. This is // intentionally not a full shell parser (no escape sequences, no nested quoting) — the trigger diff --git a/cli/project-runner/internal/projectrunner/pause_point_trigger_test.go b/cli/project-runner/internal/projectrunner/pause_point_trigger_test.go index ec220d361..8d9c2b4db 100644 --- a/cli/project-runner/internal/projectrunner/pause_point_trigger_test.go +++ b/cli/project-runner/internal/projectrunner/pause_point_trigger_test.go @@ -110,6 +110,12 @@ func TestParsePausePointTriggerCommand(t *testing.T) { value: "simulate-keyboard --action Press --project-path=/tmp/OtherProject", wantErrText: "cannot include --project-path", }, + { + name: "accepts a trigger without a leading uloop token", + value: "simulate-keyboard --action Press --key space", + wantCommand: "simulate-keyboard", + wantArgs: []string{"--action", "Press", "--key", "space"}, + }, } for _, testCase := range cases { @@ -142,6 +148,95 @@ func TestParsePausePointTriggerCommand(t *testing.T) { } } +const ( + wantPausePointTriggerLeadingUloopMessage = `--trigger must name the uloop subcommand without the leading "uloop": the value runs in-process, not through a shell.` + wantPausePointTriggerLeadingUloopCorrection = `Re-run with --trigger "simulate-keyboard --action Press --key space"` + wantPausePointTriggerLeadingUloopExample = `Re-run with --trigger "simulate-keyboard --action Press --key Space"` + wantPausePointTriggerLeadingUloopQuotedCorrection = `Re-run with --trigger 'simulate-mouse-input --action Move --position "10 20"'` +) + +// Verifies a leading uloop token is rejected at parse time, before dispatch, and NextActions +// carries the corrected command — including re-quoting tokens that contain whitespace. +func TestParsePausePointTriggerCommandRejectsLeadingUloop(t *testing.T) { + cases := []struct { + name string + value string + wantNextAction string + }{ + { + name: "strips a leading uloop token and restates the remaining command", + value: "uloop simulate-keyboard --action Press --key space", + wantNextAction: wantPausePointTriggerLeadingUloopCorrection, + }, + { + name: "uloop alone points at the subcommand format example", + value: "uloop", + wantNextAction: wantPausePointTriggerLeadingUloopExample, + }, + { + name: "re-quotes a remaining token that contains whitespace", + value: `uloop simulate-mouse-input --action Move --position "10 20"`, + wantNextAction: wantPausePointTriggerLeadingUloopQuotedCorrection, + }, + { + name: "empty quoted token falls back to the format example", + value: "uloop compile --flag ''", + wantNextAction: wantPausePointTriggerLeadingUloopExample, + }, + { + name: "quote-bearing token falls back to the format example", + value: `uloop compile --value '{"name":"x"}'`, + wantNextAction: wantPausePointTriggerLeadingUloopExample, + }, + { + name: "nested pause-point wait after uloop falls back to the format example", + value: "uloop await-pause-point --id other", + wantNextAction: wantPausePointTriggerLeadingUloopExample, + }, + { + name: "project-path after uloop falls back to the format example", + value: "uloop simulate-keyboard --project-path /x", + wantNextAction: wantPausePointTriggerLeadingUloopExample, + }, + } + + for _, testCase := range cases { + t.Run(testCase.name, func(t *testing.T) { + _, _, err := parsePausePointTriggerCommand("await-pause-point", testCase.value) + argumentError := requireArgumentError(t, err) + if argumentError.Message != wantPausePointTriggerLeadingUloopMessage { + t.Fatalf("Message mismatch:\nwant %q\ngot %q", wantPausePointTriggerLeadingUloopMessage, argumentError.Message) + } + if argumentError.Option != "--trigger" { + t.Fatalf("Option mismatch: got %q, want %q", argumentError.Option, "--trigger") + } + if argumentError.Command != "await-pause-point" { + t.Fatalf("Command mismatch: got %q, want %q", argumentError.Command, "await-pause-point") + } + requireNextActions(t, err, []string{testCase.wantNextAction}) + }) + } +} + +// Verifies formatPausePointTriggerTokens round-trips through the tokenizer for a safe argv +// that only needs whitespace quoting, so a presented correction can be pasted back unchanged. +func TestFormatPausePointTriggerTokensRoundTripsSafeTokens(t *testing.T) { + tokens := []string{"simulate-mouse-input", "--action", "Move", "--position", "10 20"} + formatted := formatPausePointTriggerTokens(tokens) + got, err := tokenizePausePointTriggerValue(formatted) + if err != nil { + t.Fatalf("tokenize(%q) failed: %v", formatted, err) + } + if len(got) != len(tokens) { + t.Fatalf("token count mismatch: got %#v, want %#v", got, tokens) + } + for index, token := range tokens { + if got[index] != token { + t.Fatalf("token[%d] mismatch: got %q, want %q", index, got[index], token) + } + } +} + // Verifies runPausePointTriggerSync passes a successfully dispatched trigger command's raw JSON // response through untouched, and reports a dispatch-level failure (unparseable output) as Error // instead, falling back to a synthesized message when the dispatched command wrote nothing to diff --git a/cli/project-runner/shared-inputs-stamp.json b/cli/project-runner/shared-inputs-stamp.json index 736bf00f1..1537601c7 100644 --- a/cli/project-runner/shared-inputs-stamp.json +++ b/cli/project-runner/shared-inputs-stamp.json @@ -1,4 +1,4 @@ { "schemaVersion": 1, - "sharedInputsHash": "9ac17f7fa800a193619ef4e1c5da9cd33f7f2259" + "sharedInputsHash": "74ab06f066c5b89f53e4039d2eaf91878ad68da1" }