From 913e15289c278fb9124cfc1fb49978a46646ac9c Mon Sep 17 00:00:00 2001 From: hatayama Date: Mon, 27 Jul 2026 00:39:33 +0900 Subject: [PATCH 1/8] Share pause-point CLI-only flag definitions between --help and list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit enable-pause-point accepts six orchestration flags that exist only in the CLI, so both option listings had to add them by hand and they drifted: `uloop list` documented all six while `--help` documented none of them. An agent reading `enable-pause-point --help` could not discover --await, --trigger, or --resume-play at all. Define the flags once in tooldocs and have both listings read that table, following the --code-file precedent. The dispatcher self-updates while the project runner stays pinned, so a new contract test asserts every flag the help table advertises is actually consumed by the runner's parsers — a drifted table would otherwise advertise options that fail on use. --- .../tooldocs/pause_point_cli_options.go | 130 ++++++++++++++++++ .../tooldocs/pause_point_cli_options_test.go | 80 +++++++++++ cli/common/tooldocs/tool_option_help.go | 12 +- .../internal/projectrunner/list_output.go | 58 +++----- .../projectrunner/list_output_test.go | 12 +- .../projectrunner/native_command_help.go | 28 ++-- .../pause_point_captured_variables_mode.go | 10 +- .../pause_point_cli_options_contract_test.go | 65 +++++++++ .../projectrunner/pause_point_enable.go | 39 +++--- .../projectrunner/pause_point_expect.go | 4 +- .../projectrunner/pause_point_trigger.go | 8 +- .../projectrunner/pause_point_wait.go | 19 +-- 12 files changed, 362 insertions(+), 103 deletions(-) create mode 100644 cli/common/tooldocs/pause_point_cli_options.go create mode 100644 cli/common/tooldocs/pause_point_cli_options_test.go create mode 100644 cli/project-runner/internal/projectrunner/pause_point_cli_options_contract_test.go diff --git a/cli/common/tooldocs/pause_point_cli_options.go b/cli/common/tooldocs/pause_point_cli_options.go new file mode 100644 index 0000000000..e45cfd76a3 --- /dev/null +++ b/cli/common/tooldocs/pause_point_cli_options.go @@ -0,0 +1,130 @@ +package tooldocs + +import "strings" + +// enable-pause-point accepts six orchestration flags that exist only in the CLI: they are parsed +// out of the argv before the Unity-side EnablePausePointSchema is consulted, so nothing in the +// tool schema describes them. Both listings that document a tool's options — the dispatcher's +// `--help` table and the project runner's `uloop list` output — therefore have to add them by +// hand, and they drifted: `uloop list` documented all six while `--help` documented none. +// Defining them once here makes both listings read the same table. +const ( + PausePointEnableAwaitFlagName = "await" + PausePointCapturedVariablesFlagName = "captured-variables" + PausePointCapturedVariableNamesFlagName = "captured-variable-names" + PausePointExpectFlagName = "expect" + PausePointTriggerFlagName = "trigger" + PausePointResumePlayFlagName = "resume-play" +) + +// Accepted --captured-variables values. Declared here because they appear in the option listings; +// the runner's own mode type is defined from these constants so the two cannot drift. +const ( + PausePointCapturedVariablesModeFull = "full" + PausePointCapturedVariablesModeNames = "names" +) + +// pausePointEnableCommandName is private to this package: importing the clicore package that owns +// the command-name constants would be an import cycle, the same reason +// executeDynamicCodeCommandName is declared locally. +const pausePointEnableCommandName = "enable-pause-point" + +// PausePointCLIOnlyOption describes one CLI-only pause-point flag in the shape both listings need: +// `--help` renders FlagName/Type/Description, and `uloop list` additionally reports Type and Values +// as structured fields. +type PausePointCLIOnlyOption struct { + FlagName string + Type string + Description string + Values []string +} + +// PausePointEnableCLIOnlyOptions returns enable-pause-point's CLI-only flags. A fresh slice is +// built per call so a caller that sorts or appends cannot mutate the shared table. +func PausePointEnableCLIOnlyOptions() []PausePointCLIOnlyOption { + return []PausePointCLIOnlyOption{ + { + FlagName: PausePointEnableAwaitFlagName, + Type: "boolean", + Description: "Wait for the marker to be hit (or time out) after enabling, in a single call, " + + "instead of a separate await-pause-point call", + }, + { + FlagName: PausePointCapturedVariablesFlagName, + Type: "string", + Description: "Requires --await. Same as await-pause-point's --captured-variables", + Values: []string{ + PausePointCapturedVariablesModeFull, + PausePointCapturedVariablesModeNames, + }, + }, + { + FlagName: PausePointCapturedVariableNamesFlagName, + Type: "string", + Description: "Requires --await. Same as await-pause-point's --captured-variable-names", + }, + { + FlagName: PausePointExpectFlagName, + Type: "string", + Description: "Requires --await. Same as await-pause-point's --expect (repeatable)", + }, + { + FlagName: PausePointTriggerFlagName, + Type: "string", + Description: "Requires --await. Same as await-pause-point's --trigger", + }, + { + FlagName: PausePointResumePlayFlagName, + Type: "boolean", + Description: "Requires --await. After confirming the marker is armed, resume PlayMode if " + + "paused (before --trigger), so a paused-arm workflow can fire input in one call", + }, + } +} + +func appendPausePointEnableCLIOnlyOptionHelpEntries( + toolName string, + entries []OptionHelpEntry, +) []OptionHelpEntry { + if toolName != pausePointEnableCommandName { + return entries + } + + for _, option := range PausePointEnableCLIOnlyOptions() { + optionName := "--" + option.FlagName + if hasOptionHelpEntry(entries, optionName) { + continue + } + entries = append(entries, OptionHelpEntry{ + Name: optionName, + Usage: pausePointCLIOnlyOptionUsage(optionName, option), + Description: pausePointCLIOnlyOptionDescription(option), + }) + } + return entries +} + +func pausePointCLIOnlyOptionUsage(optionName string, option PausePointCLIOnlyOption) string { + if option.Type == "boolean" { + return optionName + } + return optionName + " " +} + +// pausePointCLIOnlyOptionDescription appends the accepted values the same way the schema-driven +// rows do, so a CLI-only row is indistinguishable in shape from a schema-derived one. +func pausePointCLIOnlyOptionDescription(option PausePointCLIOnlyOption) string { + if len(option.Values) == 0 { + return option.Description + } + return option.Description + "; values: " + strings.Join(option.Values, optionValuesSeparator) +} + +func hasOptionHelpEntry(entries []OptionHelpEntry, name string) bool { + for _, entry := range entries { + if entry.Name == name { + return true + } + } + return false +} diff --git a/cli/common/tooldocs/pause_point_cli_options_test.go b/cli/common/tooldocs/pause_point_cli_options_test.go new file mode 100644 index 0000000000..fbbfab204b --- /dev/null +++ b/cli/common/tooldocs/pause_point_cli_options_test.go @@ -0,0 +1,80 @@ +package tooldocs + +import ( + "strings" + "testing" + + "github.com/hatayama/unity-cli-loop/common/tools" +) + +// Verifies enable-pause-point's --help lists every CLI-only pause-point flag. The schema-driven +// loop cannot produce them because none of them exist in the Unity-side EnablePausePointSchema, +// so --help documented none of the six while `uloop list` documented all six. +func TestVisibleOptionHelpEntriesIncludePausePointEnableCLIOnlyOptions(t *testing.T) { + tool, ok := tools.Find(tools.LoadDefault(), pausePointEnableCommandName) + if !ok { + t.Fatalf("embedded catalog has no %q tool", pausePointEnableCommandName) + } + + entries := VisibleOptionHelpEntriesForTool(tool) + for _, option := range PausePointEnableCLIOnlyOptions() { + optionName := "--" + option.FlagName + entry, found := findOptionHelpEntry(entries, optionName) + if !found { + t.Fatalf("enable-pause-point --help is missing CLI-only option %s", optionName) + } + if entry.Description == "" { + t.Errorf("option %s has no description in --help", optionName) + } + } +} + +// Verifies boolean CLI-only flags render without a value placeholder and valued ones render with +// one, so the help usage column matches how the flags are actually passed. +func TestPausePointEnableCLIOnlyOptionHelpUsage(t *testing.T) { + tool, ok := tools.Find(tools.LoadDefault(), pausePointEnableCommandName) + if !ok { + t.Fatalf("embedded catalog has no %q tool", pausePointEnableCommandName) + } + + entries := VisibleOptionHelpEntriesForTool(tool) + + awaitEntry, found := findOptionHelpEntry(entries, "--"+PausePointEnableAwaitFlagName) + if !found { + t.Fatalf("enable-pause-point --help is missing --%s", PausePointEnableAwaitFlagName) + } + if awaitEntry.Usage != "--"+PausePointEnableAwaitFlagName { + t.Errorf("boolean flag usage = %q, want %q", awaitEntry.Usage, "--"+PausePointEnableAwaitFlagName) + } + + triggerEntry, found := findOptionHelpEntry(entries, "--"+PausePointTriggerFlagName) + if !found { + t.Fatalf("enable-pause-point --help is missing --%s", PausePointTriggerFlagName) + } + if !strings.HasSuffix(triggerEntry.Usage, " ") { + t.Errorf("valued flag usage = %q, want a value placeholder", triggerEntry.Usage) + } +} + +// Verifies the CLI-only option table is not applied to unrelated tools, which would advertise +// pause-point orchestration flags on commands that reject them. +func TestVisibleOptionHelpEntriesOmitPausePointCLIOnlyOptionsForOtherTools(t *testing.T) { + tool, ok := tools.Find(tools.LoadDefault(), compileCommandName) + if !ok { + t.Fatalf("embedded catalog has no %q tool", compileCommandName) + } + + entries := VisibleOptionHelpEntriesForTool(tool) + if _, found := findOptionHelpEntry(entries, "--"+PausePointEnableAwaitFlagName); found { + t.Errorf("%s --help advertises --%s", compileCommandName, PausePointEnableAwaitFlagName) + } +} + +func findOptionHelpEntry(entries []OptionHelpEntry, name string) (OptionHelpEntry, bool) { + for _, entry := range entries { + if entry.Name == name { + return entry, true + } + } + return OptionHelpEntry{}, false +} diff --git a/cli/common/tooldocs/tool_option_help.go b/cli/common/tooldocs/tool_option_help.go index e72f80a9ff..e6d61c4ba0 100644 --- a/cli/common/tooldocs/tool_option_help.go +++ b/cli/common/tooldocs/tool_option_help.go @@ -19,6 +19,9 @@ const ( const DynamicCodeFileOptionDescription = "Read C# code from a file instead of --code when shell quoting would alter multiline code" +// optionValuesSeparator joins the accepted values of an option in help output. +const optionValuesSeparator = "|" + const ( compileCommandName = "compile" executeDynamicCodeCommandName = "execute-dynamic-code" @@ -49,6 +52,7 @@ func VisibleOptionHelpEntriesForTool(tool tools.ToolDefinition) []OptionHelpEntr } entries = appendDynamicCodeFileOptionHelpEntry(tool, entries) + entries = appendPausePointEnableCLIOnlyOptionHelpEntries(tool.Name, entries) sort.Slice(entries, func(i int, j int) bool { return entries[i].Name < entries[j].Name }) @@ -93,7 +97,7 @@ func optionDescription(toolName string, propertyName string, property tools.Tool parts = append(parts, "default: "+defaultValueText(propertyDefault)) } if len(property.Enum) > 0 { - parts = append(parts, "values: "+strings.Join(property.Enum, "|")) + parts = append(parts, "values: "+strings.Join(property.Enum, optionValuesSeparator)) } return strings.Join(parts, "; ") } @@ -142,10 +146,8 @@ func appendDynamicCodeFileOptionHelpEntry(tool tools.ToolDefinition, entries []O if tool.Name != executeDynamicCodeCommandName { return entries } - for _, entry := range entries { - if entry.Name == DynamicCodeFileOptionName { - return entries - } + if hasOptionHelpEntry(entries, DynamicCodeFileOptionName) { + return entries } return append(entries, OptionHelpEntry{ Name: DynamicCodeFileOptionName, diff --git a/cli/project-runner/internal/projectrunner/list_output.go b/cli/project-runner/internal/projectrunner/list_output.go index 12525a2fbb..49994f637a 100644 --- a/cli/project-runner/internal/projectrunner/list_output.go +++ b/cli/project-runner/internal/projectrunner/list_output.go @@ -149,51 +149,29 @@ func appendDynamicCodeFileListOption(tool clicore.ToolDefinition, options []list }) } -// appendPausePointEnableAwaitListOptions documents --await/--captured-variables/ -// --captured-variable-names on enable-pause-point's catalog entry, mirroring -// appendDynamicCodeFileListOption: these are CLI-only orchestration flags (pause_point_enable.go) -// that are not part of the Unity-side EnablePausePointSchema, so they never appear in -// listOptionsForTool's schema-driven loop above. +// appendPausePointEnableAwaitListOptions documents enable-pause-point's CLI-only orchestration +// flags on its catalog entry, mirroring appendDynamicCodeFileListOption: they are not part of the +// Unity-side EnablePausePointSchema, so they never appear in listOptionsForTool's schema-driven +// loop above. The flag table itself is shared with the dispatcher's `--help` renderer +// (tooldocs.PausePointEnableCLIOnlyOptions) so the two listings cannot drift apart. func appendPausePointEnableAwaitListOptions(tool clicore.ToolDefinition, options []listOption) []listOption { if tool.Name != pausePointEnableCommandName { return options } - if hasListOption(options, "--"+pausePointEnableAwaitFlagName) { - return options + + for _, option := range tooldocs.PausePointEnableCLIOnlyOptions() { + optionName := "--" + option.FlagName + if hasListOption(options, optionName) { + continue + } + options = append(options, listOption{ + Name: optionName, + Type: option.Type, + Description: option.Description, + Values: option.Values, + }) } - return append(options, - listOption{ - Name: "--" + pausePointEnableAwaitFlagName, - Type: "boolean", - Description: "Wait for the marker to be hit (or time out) after enabling, in a single call, instead of a separate await-pause-point call", - }, - listOption{ - Name: "--" + PausePointCapturedVariablesFlagName, - Type: "string", - Description: "Requires --await. Same as await-pause-point's --captured-variables", - Values: []string{string(pausePointCapturedVariablesModeFull), string(pausePointCapturedVariablesModeNames)}, - }, - listOption{ - Name: "--" + PausePointCapturedVariableNamesFlagName, - Type: "string", - Description: "Requires --await. Same as await-pause-point's --captured-variable-names", - }, - listOption{ - Name: "--" + PausePointExpectFlagName, - Type: "string", - Description: "Requires --await. Same as await-pause-point's --expect (repeatable)", - }, - listOption{ - Name: "--" + PausePointTriggerFlagName, - Type: "string", - Description: "Requires --await. Same as await-pause-point's --trigger", - }, - listOption{ - Name: "--" + PausePointResumePlayFlagName, - Type: "boolean", - Description: "Requires --await. After confirming the marker is armed, resume PlayMode if paused (before --trigger), so a paused-arm workflow can fire input in one call", - }, - ) + return options } func hasListOption(options []listOption, name string) bool { diff --git a/cli/project-runner/internal/projectrunner/list_output_test.go b/cli/project-runner/internal/projectrunner/list_output_test.go index 372f3fe09c..21b26b8739 100644 --- a/cli/project-runner/internal/projectrunner/list_output_test.go +++ b/cli/project-runner/internal/projectrunner/list_output_test.go @@ -143,12 +143,12 @@ func TestNewListCatalogIncludesEnablePausePointAwaitOptions(t *testing.T) { catalog := newListCatalog(clicore.ToolsCache{Tools: []clicore.ToolDefinition{tool}}) enablePausePoint := findListTool(t, catalog, pausePointEnableCommandName) - findListOption(t, enablePausePoint, "--"+pausePointEnableAwaitFlagName) - findListOption(t, enablePausePoint, "--"+PausePointCapturedVariablesFlagName) - findListOption(t, enablePausePoint, "--"+PausePointCapturedVariableNamesFlagName) - findListOption(t, enablePausePoint, "--"+PausePointExpectFlagName) - findListOption(t, enablePausePoint, "--"+PausePointTriggerFlagName) - findListOption(t, enablePausePoint, "--"+PausePointResumePlayFlagName) + findListOption(t, enablePausePoint, "--"+tooldocs.PausePointEnableAwaitFlagName) + findListOption(t, enablePausePoint, "--"+tooldocs.PausePointCapturedVariablesFlagName) + findListOption(t, enablePausePoint, "--"+tooldocs.PausePointCapturedVariableNamesFlagName) + findListOption(t, enablePausePoint, "--"+tooldocs.PausePointExpectFlagName) + findListOption(t, enablePausePoint, "--"+tooldocs.PausePointTriggerFlagName) + findListOption(t, enablePausePoint, "--"+tooldocs.PausePointResumePlayFlagName) } func decodeListCatalog(t *testing.T, content []byte) listCatalog { diff --git a/cli/project-runner/internal/projectrunner/native_command_help.go b/cli/project-runner/internal/projectrunner/native_command_help.go index fa058fb94b..eec18faba8 100644 --- a/cli/project-runner/internal/projectrunner/native_command_help.go +++ b/cli/project-runner/internal/projectrunner/native_command_help.go @@ -11,15 +11,13 @@ import ( "github.com/hatayama/unity-cli-loop/common/tooldocs" ) +// Runner-specific pause-point flags: --id and --timeout-seconds mirror the Unity-side schema, and +// --matching-logs-max-count only exists on the wait commands. The CLI-only flags shared with +// enable-pause-point's --help listing live in tooldocs instead (pause_point_cli_options.go). const ( - PausePointIDFlagName = "id" - PausePointTimeoutFlagName = "timeout-seconds" - PausePointLogsMaxCountFlagName = "matching-logs-max-count" - PausePointCapturedVariablesFlagName = "captured-variables" - PausePointCapturedVariableNamesFlagName = "captured-variable-names" - PausePointExpectFlagName = "expect" - PausePointTriggerFlagName = "trigger" - PausePointResumePlayFlagName = "resume-play" + PausePointIDFlagName = "id" + PausePointTimeoutFlagName = "timeout-seconds" + PausePointLogsMaxCountFlagName = "matching-logs-max-count" ) // runnerNativeCommandOptions lists the flags accepted by each runner-owned @@ -31,16 +29,16 @@ var runnerNativeCommandOptions = map[string][]string{ "--" + PausePointIDFlagName, "--" + PausePointTimeoutFlagName, "--" + PausePointLogsMaxCountFlagName, - "--" + PausePointCapturedVariablesFlagName, - "--" + PausePointCapturedVariableNamesFlagName, - "--" + PausePointExpectFlagName, - "--" + PausePointTriggerFlagName, - "--" + PausePointResumePlayFlagName, + "--" + tooldocs.PausePointCapturedVariablesFlagName, + "--" + tooldocs.PausePointCapturedVariableNamesFlagName, + "--" + tooldocs.PausePointExpectFlagName, + "--" + tooldocs.PausePointTriggerFlagName, + "--" + tooldocs.PausePointResumePlayFlagName, }, clicore.PausePointStatusUserCommandName: { "--" + PausePointIDFlagName, - "--" + PausePointCapturedVariablesFlagName, - "--" + PausePointCapturedVariableNamesFlagName, + "--" + tooldocs.PausePointCapturedVariablesFlagName, + "--" + tooldocs.PausePointCapturedVariableNamesFlagName, }, } diff --git a/cli/project-runner/internal/projectrunner/pause_point_captured_variables_mode.go b/cli/project-runner/internal/projectrunner/pause_point_captured_variables_mode.go index 3212cb0639..aac0c0612a 100644 --- a/cli/project-runner/internal/projectrunner/pause_point_captured_variables_mode.go +++ b/cli/project-runner/internal/projectrunner/pause_point_captured_variables_mode.go @@ -2,6 +2,8 @@ package projectrunner import ( clierrors "github.com/hatayama/unity-cli-loop/common/errors" + + "github.com/hatayama/unity-cli-loop/common/tooldocs" ) // pausePointCapturedVariablesMode controls how much of each captured variable's data the CLI @@ -10,9 +12,11 @@ import ( // (via TryGetCapturedValue or pause-point-status) instead of paying for every value up front. type pausePointCapturedVariablesMode string +// Defined from the tooldocs values rather than repeating the literals: the same two strings are +// rendered as --captured-variables' accepted values in both option listings. const ( - pausePointCapturedVariablesModeFull pausePointCapturedVariablesMode = "full" - pausePointCapturedVariablesModeNames pausePointCapturedVariablesMode = "names" + pausePointCapturedVariablesModeFull pausePointCapturedVariablesMode = tooldocs.PausePointCapturedVariablesModeFull + pausePointCapturedVariablesModeNames pausePointCapturedVariablesMode = tooldocs.PausePointCapturedVariablesModeNames ) func parsePausePointCapturedVariablesMode(value string) (pausePointCapturedVariablesMode, error) { @@ -23,7 +27,7 @@ func parsePausePointCapturedVariablesMode(value string) (pausePointCapturedVaria return pausePointCapturedVariablesModeNames, nil default: return "", clierrors.InvalidValueArgumentError( - "--"+PausePointCapturedVariablesFlagName, value, "full or names") + "--"+tooldocs.PausePointCapturedVariablesFlagName, value, "full or names") } } 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 new file mode 100644 index 0000000000..f7b441ea83 --- /dev/null +++ b/cli/project-runner/internal/projectrunner/pause_point_cli_options_contract_test.go @@ -0,0 +1,65 @@ +package projectrunner + +import ( + "testing" + + "github.com/hatayama/unity-cli-loop/common/tooldocs" +) + +// pausePointCLIOnlySampleArgs supplies one accepted argv form per CLI-only pause-point flag. A flag +// added to the shared table without an entry here fails the contract tests below rather than +// silently going unchecked. +var pausePointCLIOnlySampleArgs = map[string][]string{ + tooldocs.PausePointEnableAwaitFlagName: {"--await"}, + tooldocs.PausePointCapturedVariablesFlagName: {"--captured-variables", "names"}, + tooldocs.PausePointCapturedVariableNamesFlagName: {"--captured-variable-names", "score"}, + tooldocs.PausePointExpectFlagName: {"--expect", "score=1"}, + tooldocs.PausePointTriggerFlagName: {"--trigger", "simulate-keyboard --action Press --key Space"}, + tooldocs.PausePointResumePlayFlagName: {"--resume-play"}, +} + +// Verifies every CLI-only flag that enable-pause-point's --help advertises is actually consumed by +// the runner's enable-pause-point argument parser. The dispatcher self-updates while the project +// runner stays pinned, so a dispatcher-side table that grows a flag the pinned runner does not +// accept would advertise an option that fails on use; this contract catches that drift. +func TestPausePointEnableHelpOptionsAreAcceptedByTheEnableParser(t *testing.T) { + for _, option := range tooldocs.PausePointEnableCLIOnlyOptions() { + args, ok := pausePointCLIOnlySampleArgs[option.FlagName] + if !ok { + t.Fatalf("no sample argv for --%s: add one to pausePointCLIOnlySampleArgs", option.FlagName) + } + + // Every flag but --await itself requires --await, so it is always passed alongside. + enableArgs := append([]string{"--" + tooldocs.PausePointEnableAwaitFlagName}, args...) + remaining, _, _, _, _, _, _, _, err := extractPausePointEnableAwaitFlags(enableArgs) + if err != nil { + t.Errorf("enable-pause-point parser rejected advertised option --%s: %v", option.FlagName, err) + continue + } + if len(remaining) != 0 { + t.Errorf("enable-pause-point parser did not consume advertised option --%s: remaining %v", + option.FlagName, remaining) + } + } +} + +// Verifies the CLI-only flags shared with await-pause-point are accepted by the wait parser as +// well, since enable-pause-point --help documents them as "same as await-pause-point's --x". +func TestPausePointSharedHelpOptionsAreAcceptedByTheWaitParser(t *testing.T) { + for _, option := range tooldocs.PausePointEnableCLIOnlyOptions() { + if option.FlagName == tooldocs.PausePointEnableAwaitFlagName { + // --await only exists on enable-pause-point: await-pause-point is the wait itself. + continue + } + + args, ok := pausePointCLIOnlySampleArgs[option.FlagName] + if !ok { + t.Fatalf("no sample argv for --%s: add one to pausePointCLIOnlySampleArgs", option.FlagName) + } + + waitArgs := append([]string{"--id", "marker"}, args...) + if _, err := parseWaitForPausePointOptions(waitArgs); err != nil { + t.Errorf("await-pause-point parser rejected shared option --%s: %v", option.FlagName, err) + } + } +} diff --git a/cli/project-runner/internal/projectrunner/pause_point_enable.go b/cli/project-runner/internal/projectrunner/pause_point_enable.go index 31d786d6ff..e31f8a78fa 100644 --- a/cli/project-runner/internal/projectrunner/pause_point_enable.go +++ b/cli/project-runner/internal/projectrunner/pause_point_enable.go @@ -11,6 +11,7 @@ import ( "github.com/hatayama/unity-cli-loop/common/ui" "github.com/hatayama/unity-cli-loop/common/clicore" + "github.com/hatayama/unity-cli-loop/common/tooldocs" "github.com/hatayama/unity-cli-loop/common/unityipc" ) @@ -22,8 +23,6 @@ import ( // extractDynamicCodeFileFlag intercepts --code-file for execute-dynamic-code. const pausePointEnableCommandName = "enable-pause-point" -const pausePointEnableAwaitFlagName = "await" - // extractPausePointEnableAwaitFlags pulls the CLI-only --await/--captured-variables/ // --captured-variable-names/--expect/--trigger/--resume-play flags out of enable-pause-point args // before generic schema parsing, because none of them are part of the Unity-side @@ -46,30 +45,30 @@ func extractPausePointEnableAwaitFlags( for index := 0; index < len(args); index++ { arg := args[index] - if arg == "--"+pausePointEnableAwaitFlagName { + if arg == "--"+tooldocs.PausePointEnableAwaitFlagName { await = true continue } - if arg == "--"+PausePointResumePlayFlagName { + if arg == "--"+tooldocs.PausePointResumePlayFlagName { resumePlay = true continue } // --resume-play=true|1 must be accepted here too: otherwise the =value form falls through // to Unity schema parsing and becomes a confusing unrelated error. - if isPausePointFlag(arg, PausePointResumePlayFlagName) { + if isPausePointFlag(arg, tooldocs.PausePointResumePlayFlagName) { name, value, consumedNext, err := clicore.ParseFlagValue(arg, args, index) if err != nil { return nil, false, mode, nil, nil, "", nil, false, err } - if name != PausePointResumePlayFlagName { + if name != tooldocs.PausePointResumePlayFlagName { remaining = append(remaining, arg) continue } if value != "true" && value != "1" { return nil, false, mode, nil, nil, "", nil, false, clierrors.InvalidValueArgumentError( - "--"+PausePointResumePlayFlagName, value, "boolean flag (pass with no value, or =true)") + "--"+tooldocs.PausePointResumePlayFlagName, value, "boolean flag (pass with no value, or =true)") } resumePlay = true if consumedNext { @@ -78,12 +77,12 @@ func extractPausePointEnableAwaitFlags( continue } - if isPausePointFlag(arg, PausePointTriggerFlagName) { + if isPausePointFlag(arg, tooldocs.PausePointTriggerFlagName) { name, value, consumedNext, err := clicore.ParseFlagValue(arg, args, index) if err != nil { return nil, false, mode, nil, nil, "", nil, false, err } - if name != PausePointTriggerFlagName { + if name != tooldocs.PausePointTriggerFlagName { remaining = append(remaining, arg) continue } @@ -100,12 +99,12 @@ func extractPausePointEnableAwaitFlags( continue } - if isPausePointFlag(arg, PausePointCapturedVariablesFlagName) { + if isPausePointFlag(arg, tooldocs.PausePointCapturedVariablesFlagName) { name, value, consumedNext, err := clicore.ParseFlagValue(arg, args, index) if err != nil { return nil, false, mode, nil, nil, "", nil, false, err } - if name != PausePointCapturedVariablesFlagName { + if name != tooldocs.PausePointCapturedVariablesFlagName { remaining = append(remaining, arg) continue } @@ -121,12 +120,12 @@ func extractPausePointEnableAwaitFlags( continue } - if isPausePointFlag(arg, PausePointCapturedVariableNamesFlagName) { + if isPausePointFlag(arg, tooldocs.PausePointCapturedVariableNamesFlagName) { name, value, consumedNext, err := clicore.ParseFlagValue(arg, args, index) if err != nil { return nil, false, mode, nil, nil, "", nil, false, err } - if name != PausePointCapturedVariableNamesFlagName { + if name != tooldocs.PausePointCapturedVariableNamesFlagName { remaining = append(remaining, arg) continue } @@ -138,12 +137,12 @@ func extractPausePointEnableAwaitFlags( continue } - if isPausePointFlag(arg, PausePointExpectFlagName) { + if isPausePointFlag(arg, tooldocs.PausePointExpectFlagName) { name, value, consumedNext, err := clicore.ParseFlagValue(arg, args, index) if err != nil { return nil, false, mode, nil, nil, "", nil, false, err } - if name != PausePointExpectFlagName { + if name != tooldocs.PausePointExpectFlagName { remaining = append(remaining, arg) continue } @@ -162,16 +161,16 @@ func extractPausePointEnableAwaitFlags( } if !await && (modeSet || namesSet || len(expectations) > 0 || triggerSet || resumePlay) { - option := "--" + PausePointCapturedVariablesFlagName + option := "--" + tooldocs.PausePointCapturedVariablesFlagName switch { case resumePlay: - option = "--" + PausePointResumePlayFlagName + option = "--" + tooldocs.PausePointResumePlayFlagName case triggerSet: - option = "--" + PausePointTriggerFlagName + option = "--" + tooldocs.PausePointTriggerFlagName case len(expectations) > 0: - option = "--" + PausePointExpectFlagName + option = "--" + tooldocs.PausePointExpectFlagName case namesSet: - option = "--" + PausePointCapturedVariableNamesFlagName + option = "--" + tooldocs.PausePointCapturedVariableNamesFlagName } return nil, false, mode, nil, nil, "", nil, false, &clierrors.ArgumentError{ Message: "--captured-variables, --captured-variable-names, --expect, --trigger, and --resume-play require --await", diff --git a/cli/project-runner/internal/projectrunner/pause_point_expect.go b/cli/project-runner/internal/projectrunner/pause_point_expect.go index f1087c4510..652a664fd4 100644 --- a/cli/project-runner/internal/projectrunner/pause_point_expect.go +++ b/cli/project-runner/internal/projectrunner/pause_point_expect.go @@ -4,6 +4,8 @@ import ( "strings" clierrors "github.com/hatayama/unity-cli-loop/common/errors" + + "github.com/hatayama/unity-cli-loop/common/tooldocs" ) // pausePointExpectation is one --expect 'Name=value' assertion parsed from the CLI args. @@ -30,7 +32,7 @@ func parsePausePointExpectFlagValue(value string) (pausePointExpectation, error) if !found || name == "" { return pausePointExpectation{}, &clierrors.ArgumentError{ Message: "Invalid --expect value: " + value, - Option: "--" + PausePointExpectFlagName, + Option: "--" + tooldocs.PausePointExpectFlagName, ExpectedType: "Name=value", NextActions: []string{"Pass `--expect 'Name=value'`, for example `--expect 'Health=100'`."}, } diff --git a/cli/project-runner/internal/projectrunner/pause_point_trigger.go b/cli/project-runner/internal/projectrunner/pause_point_trigger.go index 87a4cbdcab..26d9ebc579 100644 --- a/cli/project-runner/internal/projectrunner/pause_point_trigger.go +++ b/cli/project-runner/internal/projectrunner/pause_point_trigger.go @@ -82,7 +82,7 @@ func parsePausePointTriggerCommand(command string, value string) (string, []stri if err != nil { return "", nil, &clierrors.ArgumentError{ Message: err.Error(), - Option: "--" + PausePointTriggerFlagName, + Option: "--" + tooldocs.PausePointTriggerFlagName, Command: command, NextActions: []string{ "Quote the trigger command consistently, e.g. --trigger \"simulate-keyboard --action Press --key Space --duration 5\".", @@ -90,7 +90,7 @@ func parsePausePointTriggerCommand(command string, value string) (string, []stri } } if len(tokens) == 0 { - return "", nil, clierrors.MissingValueArgumentError("--" + PausePointTriggerFlagName) + return "", nil, clierrors.MissingValueArgumentError("--" + tooldocs.PausePointTriggerFlagName) } triggerCommand := tokens[0] @@ -103,7 +103,7 @@ func parsePausePointTriggerCommand(command string, value string) (string, []stri Message: fmt.Sprintf( "--trigger cannot target %q: waiting for a pause point from inside another pause-point wait cannot make progress.", triggerCommand), - Option: "--" + PausePointTriggerFlagName, + Option: "--" + tooldocs.PausePointTriggerFlagName, Command: command, NextActions: []string{ "Pass a command that performs an action (for example simulate-keyboard), not another pause-point wait.", @@ -116,7 +116,7 @@ func parsePausePointTriggerCommand(command string, value string) (string, []stri return "", nil, &clierrors.ArgumentError{ Message: "--trigger cannot include --project-path: the triggered command always runs " + "against the same project as the parent command.", - Option: "--" + PausePointTriggerFlagName, + Option: "--" + tooldocs.PausePointTriggerFlagName, Command: command, NextActions: []string{ "Remove --project-path from the trigger command string.", diff --git a/cli/project-runner/internal/projectrunner/pause_point_wait.go b/cli/project-runner/internal/projectrunner/pause_point_wait.go index e1f055ec7a..2ed2c8ec31 100644 --- a/cli/project-runner/internal/projectrunner/pause_point_wait.go +++ b/cli/project-runner/internal/projectrunner/pause_point_wait.go @@ -11,6 +11,7 @@ import ( 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" "github.com/hatayama/unity-cli-loop/common/unityipc" ) @@ -297,7 +298,7 @@ func parseWaitForPausePointOptions(args []string) (waitForPausePointOptions, err arg := args[index] // --resume-play is a boolean flag (no value). ParseFlagValue would otherwise demand one. - if arg == "--"+PausePointResumePlayFlagName { + if arg == "--"+tooldocs.PausePointResumePlayFlagName { options.resumePlay = true continue } @@ -324,33 +325,33 @@ func parseWaitForPausePointOptions(args []string) (waitForPausePointOptions, err "--"+PausePointLogsMaxCountFlagName, value, "positive integer") } options.matchingLogsMaxCount = maxCount - case PausePointCapturedVariablesFlagName: + case tooldocs.PausePointCapturedVariablesFlagName: mode, parseErr := parsePausePointCapturedVariablesMode(value) if parseErr != nil { return waitForPausePointOptions{}, parseErr } options.capturedVariablesMode = mode - case PausePointCapturedVariableNamesFlagName: + case tooldocs.PausePointCapturedVariableNamesFlagName: options.capturedVariableNames = parsePausePointCapturedVariableNames(value) - case PausePointExpectFlagName: + case tooldocs.PausePointExpectFlagName: expectation, parseErr := parsePausePointExpectFlagValue(value) if parseErr != nil { return waitForPausePointOptions{}, parseErr } options.expectations = append(options.expectations, expectation) - case PausePointTriggerFlagName: + case tooldocs.PausePointTriggerFlagName: triggerCommand, triggerArgs, parseErr := parsePausePointTriggerCommand(clicore.PausePointAwaitCommandName, value) if parseErr != nil { return waitForPausePointOptions{}, parseErr } options.triggerCommand = triggerCommand options.triggerArgs = triggerArgs - case PausePointResumePlayFlagName: + case tooldocs.PausePointResumePlayFlagName: // --resume-play=true style is accepted; any other value is rejected so a typo cannot // silently disable the resume step. if value != "true" && value != "1" { return waitForPausePointOptions{}, clierrors.InvalidValueArgumentError( - "--"+PausePointResumePlayFlagName, value, "boolean flag (pass with no value)") + "--"+tooldocs.PausePointResumePlayFlagName, value, "boolean flag (pass with no value)") } options.resumePlay = true default: @@ -388,13 +389,13 @@ func parsePausePointStatusOptions(args []string) (pausePointStatusOptions, error switch name { case PausePointIDFlagName: options.id = value - case PausePointCapturedVariablesFlagName: + case tooldocs.PausePointCapturedVariablesFlagName: mode, parseErr := parsePausePointCapturedVariablesMode(value) if parseErr != nil { return pausePointStatusOptions{}, parseErr } options.capturedVariablesMode = mode - case PausePointCapturedVariableNamesFlagName: + case tooldocs.PausePointCapturedVariableNamesFlagName: options.capturedVariableNames = parsePausePointCapturedVariableNames(value) default: return pausePointStatusOptions{}, pausePointUnknownOptionError(clicore.PausePointStatusUserCommandName, name) From 603f39b3a34694b7780fd1341aaf8fbbfdf0e6b6 Mon Sep 17 00:00:00 2001 From: hatayama Date: Mon, 27 Jul 2026 00:43:56 +0900 Subject: [PATCH 2/8] Show enum option defaults by name instead of by ordinal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Unity's schema cache serializes a C# enum default as its ordinal while listing the members by name, so `simulate-keyboard --help` reported "default: 0" for a parameter that only accepts "Press". `uloop list` already converted the ordinal back to a name; --help did not. Move that conversion into tooldocs so both listings share it. Indexing the name list by ordinal only works while the enum is zero-based and contiguous, so a Unity-side guard test now asserts that of every enum a first-party schema exposes — an explicit member value or a [Flags] enum would otherwise make --help name the wrong default. --- .../FirstPartyToolSchemaMetadataTests.cs | 69 ++++++++++++++++-- cli/common/tooldocs/enum_defaults.go | 35 +++++++++ cli/common/tooldocs/enum_defaults_test.go | 71 +++++++++++++++++++ cli/common/tooldocs/tool_option_help.go | 7 +- .../internal/projectrunner/list_output.go | 28 +------- 5 files changed, 176 insertions(+), 34 deletions(-) create mode 100644 cli/common/tooldocs/enum_defaults.go create mode 100644 cli/common/tooldocs/enum_defaults_test.go diff --git a/Assets/Tests/Editor/DynamicCodeToolTests/FirstPartyToolSchemaMetadataTests.cs b/Assets/Tests/Editor/DynamicCodeToolTests/FirstPartyToolSchemaMetadataTests.cs index 776ef8c127..a5a87472b0 100644 --- a/Assets/Tests/Editor/DynamicCodeToolTests/FirstPartyToolSchemaMetadataTests.cs +++ b/Assets/Tests/Editor/DynamicCodeToolTests/FirstPartyToolSchemaMetadataTests.cs @@ -20,11 +20,7 @@ public class FirstPartyToolSchemaMetadataTests public void FirstPartySchemaProperties_WhenLoaded_ShouldNotExposeDescriptionAttributes() { // Tests that long-form agent guidance stays in skill files instead of runtime schema metadata. - Type[] schemaTypes = TypeCache.GetTypesDerivedFrom() - .Where(type => type.Assembly.GetName().Name.StartsWith( - "UnityCLILoop.FirstPartyTools", - StringComparison.Ordinal)) - .ToArray(); + Type[] schemaTypes = FirstPartySchemaTypes(); Assert.That(schemaTypes, Is.Not.Empty); @@ -45,6 +41,69 @@ public void FirstPartySchemaProperties_WhenLoaded_ShouldNotExposeDescriptionAttr } } + [Test] + public void FirstPartySchemaEnumProperties_WhenLoaded_ShouldBeZeroBasedAndContiguous() + { + // Tests that every enum a first-party schema exposes can be resolved by its ordinal. + // The schema cache stores an enum default as a number while listing the members by name, + // so the CLI recovers the name shown in `--help` by indexing the name list with that + // number. A member with an explicit value or a [Flags] enum would make the CLI print a + // different member's name as the default. + Type[] schemaTypes = FirstPartySchemaTypes(); + + Assert.That(schemaTypes, Is.Not.Empty); + + int checkedEnumPropertyCount = 0; + foreach (Type schemaType in schemaTypes) + { + PropertyInfo[] properties = schemaType.GetProperties( + BindingFlags.Instance | + BindingFlags.Public | + BindingFlags.DeclaredOnly); + + foreach (PropertyInfo property in properties) + { + Type propertyType = Nullable.GetUnderlyingType(property.PropertyType) ?? property.PropertyType; + if (!propertyType.IsEnum) + { + continue; + } + + string location = $"{schemaType.FullName}.{property.Name} ({propertyType.FullName})"; + + Assert.That( + propertyType.GetCustomAttribute(), + Is.Null, + $"{location} is a [Flags] enum, which cannot be resolved by ordinal"); + + Array members = Enum.GetValues(propertyType); + for (int index = 0; index < members.Length; index++) + { + long value = Convert.ToInt64(members.GetValue(index)); + Assert.That( + value, + Is.EqualTo((long)index), + $"{location} is not zero-based and contiguous at index {index}"); + } + + checkedEnumPropertyCount++; + } + } + + // Guards the guard: if schemas stop exposing enums the assertions above go unreached, + // and this test would keep passing while checking nothing. + Assert.That(checkedEnumPropertyCount, Is.GreaterThan(0)); + } + + private static Type[] FirstPartySchemaTypes() + { + return TypeCache.GetTypesDerivedFrom() + .Where(type => type.Assembly.GetName().Name.StartsWith( + "UnityCLILoop.FirstPartyTools", + StringComparison.Ordinal)) + .ToArray(); + } + [Test] public void ExecuteDynamicCodeSchema_WhenCreated_ShouldNotWaitForDomainReloadByDefault() { diff --git a/cli/common/tooldocs/enum_defaults.go b/cli/common/tooldocs/enum_defaults.go new file mode 100644 index 0000000000..7899adfcb2 --- /dev/null +++ b/cli/common/tooldocs/enum_defaults.go @@ -0,0 +1,35 @@ +package tooldocs + +// EnumValueForNumericDefault converts a numeric default on an enum property into the member name +// that has to be typed on the command line. Unity's schema generator serializes a C# enum default +// as its ordinal, so both option listings would otherwise report "default: 0" for a parameter that +// only accepts names such as "Press". +// +// The conversion assumes the enum is zero-based and contiguous, which is what a name lookup by +// ordinal requires. A value outside the listed range yields no conversion so the raw number is +// shown instead of a wrong member name. +func EnumValueForNumericDefault(defaultValue any, values []string) (string, bool) { + if len(values) == 0 || defaultValue == nil { + return "", false + } + + switch value := defaultValue.(type) { + case int: + return enumValueAtIndex(value, values) + case float64: + index := int(value) + if value != float64(index) { + return "", false + } + return enumValueAtIndex(index, values) + default: + return "", false + } +} + +func enumValueAtIndex(index int, values []string) (string, bool) { + if index < 0 || index >= len(values) { + return "", false + } + return values[index], true +} diff --git a/cli/common/tooldocs/enum_defaults_test.go b/cli/common/tooldocs/enum_defaults_test.go new file mode 100644 index 0000000000..ae302a0d55 --- /dev/null +++ b/cli/common/tooldocs/enum_defaults_test.go @@ -0,0 +1,71 @@ +package tooldocs + +import ( + "strings" + "testing" + + "github.com/hatayama/unity-cli-loop/common/tools" +) + +// Verifies an enum property whose cached default is the C# enum's ordinal renders the member name +// in --help. Unity's schema cache serializes enum defaults as numbers, so --help showed +// "default: 0" for a value that has to be passed as "Press". +func TestOptionDescriptionRendersEnumDefaultByName(t *testing.T) { + property := tools.ToolProperty{ + Type: "string", + Description: "Keyboard action", + DefaultValue: float64(0), + Enum: []string{"Press", "KeyDown", "KeyUp", "ReleaseAll"}, + } + + description := optionDescription("simulate-keyboard", "Action", property) + if !strings.Contains(description, "default: Press") { + t.Errorf("description = %q, want it to contain %q", description, "default: Press") + } +} + +// Verifies a string default that already names an enum member is passed through untouched. +func TestOptionDescriptionKeepsNamedEnumDefault(t *testing.T) { + property := tools.ToolProperty{ + Type: "string", + Description: "Pause point mode", + DefaultValue: "single-shot", + Enum: []string{"single-shot", "repeat"}, + } + + description := optionDescription("enable-pause-point", "Mode", property) + if !strings.Contains(description, "default: single-shot") { + t.Errorf("description = %q, want it to contain %q", description, "default: single-shot") + } +} + +// Verifies a numeric default on a property with no enum stays numeric, so --timeout-seconds does +// not get mistaken for an enum ordinal. +func TestOptionDescriptionKeepsNumericDefaultWithoutEnum(t *testing.T) { + property := tools.ToolProperty{ + Type: "number", + Description: "Timeout", + DefaultValue: float64(30), + Enum: nil, + } + + description := optionDescription("await-pause-point", "TimeoutSeconds", property) + if !strings.Contains(description, "default: 30") { + t.Errorf("description = %q, want it to contain %q", description, "default: 30") + } +} + +// Verifies an ordinal outside the enum's range is left as-is rather than reported as some other +// member: guessing a name there would be worse than showing the raw value. +func TestEnumValueForNumericDefaultRejectsOutOfRangeOrdinal(t *testing.T) { + if value, ok := EnumValueForNumericDefault(float64(7), []string{"Press", "KeyDown"}); ok { + t.Errorf("out-of-range ordinal resolved to %q, want no conversion", value) + } +} + +// Verifies a fractional default is not treated as an ordinal, since no enum member can match it. +func TestEnumValueForNumericDefaultRejectsFractionalOrdinal(t *testing.T) { + if value, ok := EnumValueForNumericDefault(0.5, []string{"Press", "KeyDown"}); ok { + t.Errorf("fractional ordinal resolved to %q, want no conversion", value) + } +} diff --git a/cli/common/tooldocs/tool_option_help.go b/cli/common/tooldocs/tool_option_help.go index e6d61c4ba0..5a1bffe65f 100644 --- a/cli/common/tooldocs/tool_option_help.go +++ b/cli/common/tooldocs/tool_option_help.go @@ -94,7 +94,7 @@ func optionDescription(toolName string, propertyName string, property tools.Tool parts = append(parts, description) } if propertyDefault := property.EffectiveDefault(); propertyDefault != nil { - parts = append(parts, "default: "+defaultValueText(propertyDefault)) + parts = append(parts, "default: "+defaultValueText(propertyDefault, property.Enum)) } if len(property.Enum) > 0 { parts = append(parts, "values: "+strings.Join(property.Enum, optionValuesSeparator)) @@ -102,13 +102,16 @@ func optionDescription(toolName string, propertyName string, property tools.Tool return strings.Join(parts, "; ") } -func defaultValueText(value any) string { +func defaultValueText(value any, enumValues []string) string { if boolValue, ok := value.(bool); ok { if boolValue { return "enabled" } return "disabled" } + if enumValue, ok := EnumValueForNumericDefault(value, enumValues); ok { + return enumValue + } return fmt.Sprint(value) } diff --git a/cli/project-runner/internal/projectrunner/list_output.go b/cli/project-runner/internal/projectrunner/list_output.go index 49994f637a..03ccee6da6 100644 --- a/cli/project-runner/internal/projectrunner/list_output.go +++ b/cli/project-runner/internal/projectrunner/list_output.go @@ -101,38 +101,12 @@ func listOptionDefault(property clicore.ToolProperty) any { return false } defaultValue := property.EffectiveDefault() - if enumValue, ok := enumValueForNumericDefault(defaultValue, property.Enum); ok { + if enumValue, ok := tooldocs.EnumValueForNumericDefault(defaultValue, property.Enum); ok { return enumValue } return defaultValue } -func enumValueForNumericDefault(defaultValue any, values []string) (string, bool) { - if len(values) == 0 || defaultValue == nil { - return "", false - } - - switch value := defaultValue.(type) { - case int: - return enumValueAtIndex(value, values) - case float64: - index := int(value) - if value != float64(index) { - return "", false - } - return enumValueAtIndex(index, values) - default: - return "", false - } -} - -func enumValueAtIndex(index int, values []string) (string, bool) { - if index < 0 || index >= len(values) { - return "", false - } - return values[index], true -} - func appendDynamicCodeFileListOption(tool clicore.ToolDefinition, options []listOption) []listOption { if tool.Name != clicore.ExecuteDynamicCodeCommandName { return options From 03a8cb7df76c8833effb9f7959313f0c6bc4cad3 Mon Sep 17 00:00:00 2001 From: hatayama Date: Mon, 27 Jul 2026 00:48:00 +0900 Subject: [PATCH 3/8] Point --help at the skill that documents each command `--help` can only list option names and one-line summaries, so an agent that discovered a command there had no way to learn that a skill with the workflow rules, response shapes, and failure diagnoses exists. Close each command's help with an instruction to load its skill. The mapping is a static table rather than a name derived from the command: the four pause-point commands are all documented by uloop-pause-point, and a uloop-enable-pause-point skill does not exist. Commands absent from the table (custom commands) get no line at all. A guard test asserts every skill named in the table is declared by a real SKILL.md. --- cli/common/tooldocs/skill_guidance.go | 47 +++++++ cli/common/tooldocs/skill_guidance_test.go | 133 ++++++++++++++++++ .../internal/dispatcher/command_help.go | 13 ++ .../internal/dispatcher/help_test.go | 47 +++++++ .../projectrunner/native_command_help.go | 7 + .../projectrunner/native_command_help_test.go | 22 +++ 6 files changed, 269 insertions(+) create mode 100644 cli/common/tooldocs/skill_guidance.go create mode 100644 cli/common/tooldocs/skill_guidance_test.go diff --git a/cli/common/tooldocs/skill_guidance.go b/cli/common/tooldocs/skill_guidance.go new file mode 100644 index 0000000000..fb415f6436 --- /dev/null +++ b/cli/common/tooldocs/skill_guidance.go @@ -0,0 +1,47 @@ +package tooldocs + +// commandSkillNames maps a command to the agent skill that documents it. A static table rather than +// a name derived from the command: `enable-pause-point` is documented by `uloop-pause-point`, not by +// a `uloop-enable-pause-point` skill that does not exist, and four commands share that one skill. +// A command absent from this table gets no guidance line at all, so custom commands are never +// pointed at a skill nobody installed. +var commandSkillNames = map[string]string{ + "clear-console": "uloop-clear-console", + "compile": "uloop-compile", + "control-play-mode": "uloop-control-play-mode", + "execute-dynamic-code": "uloop-execute-dynamic-code", + "find-game-objects": "uloop-find-game-objects", + "focus-window": "uloop-focus-window", + "get-hierarchy": "uloop-get-hierarchy", + "get-logs": "uloop-get-logs", + "raycast": "uloop-raycast", + "record-input": "uloop-record-input", + "replay-input": "uloop-replay-input", + "run-tests": "uloop-run-tests", + "screenshot": "uloop-screenshot", + "set-game-view-size": "uloop-set-game-view-size", + "simulate-keyboard": "uloop-simulate-keyboard", + "simulate-mouse-input": "uloop-simulate-mouse-input", + "simulate-mouse-ui": "uloop-simulate-mouse-ui", + + // One skill, four commands. + "enable-pause-point": "uloop-pause-point", + "clear-pause-point": "uloop-pause-point", + "await-pause-point": "uloop-pause-point", + "pause-point-status": "uloop-pause-point", +} + +// SkillGuidanceLine returns the closing line of a command's --help output: an instruction to load +// the skill that documents it. `--help` can only list option names and one-line summaries, so the +// workflow rules, response shapes, and failure diagnoses live in the skill; without this line an +// agent that found the command through --help has no way to learn the skill exists. +// +// Phrased as an instruction rather than a cross-reference, because a line that merely mentions a +// document is easy to read past. +func SkillGuidanceLine(command string) (string, bool) { + skillName, ok := commandSkillNames[command] + if !ok { + return "", false + } + return "Load the " + skillName + " skill before using options not listed above.", true +} diff --git a/cli/common/tooldocs/skill_guidance_test.go b/cli/common/tooldocs/skill_guidance_test.go new file mode 100644 index 0000000000..9a0f163da4 --- /dev/null +++ b/cli/common/tooldocs/skill_guidance_test.go @@ -0,0 +1,133 @@ +package tooldocs + +import ( + "bufio" + "os" + "path/filepath" + "strings" + "testing" +) + +// Verifies a command with a matching skill gets an instruction to load it, and that the instruction +// names the skill exactly. +func TestSkillGuidanceLineNamesTheMatchingSkill(t *testing.T) { + line, ok := SkillGuidanceLine("simulate-keyboard") + if !ok { + t.Fatalf("simulate-keyboard has no skill guidance line") + } + if !strings.Contains(line, "uloop-simulate-keyboard") { + t.Errorf("guidance line = %q, want it to name uloop-simulate-keyboard", line) + } +} + +// Verifies all four pause-point commands point at the single uloop-pause-point skill rather than a +// per-command skill name derived from the command, which would not exist. +func TestSkillGuidanceLineMapsEveryPausePointCommandToOneSkill(t *testing.T) { + commands := []string{ + "enable-pause-point", + "clear-pause-point", + "await-pause-point", + "pause-point-status", + } + + for _, command := range commands { + line, ok := SkillGuidanceLine(command) + if !ok { + t.Errorf("%s has no skill guidance line", command) + continue + } + if !strings.Contains(line, "uloop-pause-point") { + t.Errorf("%s guidance line = %q, want it to name uloop-pause-point", command, line) + } + } +} + +// Verifies a command with no skill gets no guidance line, so custom commands are not pointed at a +// skill that does not exist. +func TestSkillGuidanceLineOmittedForUnmappedCommands(t *testing.T) { + if line, ok := SkillGuidanceLine("my-custom-command"); ok { + t.Errorf("unmapped command produced guidance line %q", line) + } +} + +// Verifies every skill named in the guidance map exists as a SKILL.md frontmatter name, so the +// instruction can never tell an agent to load a skill that was renamed or removed. +func TestSkillGuidanceMapNamesExistingSkills(t *testing.T) { + existing := installedSkillNames(t) + + for command, skillName := range commandSkillNames { + if !existing[skillName] { + t.Errorf("command %s points at skill %q, which no SKILL.md declares", command, skillName) + } + } +} + +func installedSkillNames(t *testing.T) map[string]bool { + t.Helper() + + names := map[string]bool{} + for _, pattern := range []string{ + filepath.Join("Packages", "src", "Editor", "FirstPartyTools", "*", "Skill", "SKILL.md"), + filepath.Join("Packages", "src", "Editor", "CliOnlyTools~", "*", "Skill", "SKILL.md"), + } { + matches, err := filepath.Glob(filepath.Join(repositoryRoot(t), pattern)) + if err != nil { + t.Fatalf("failed to glob %s: %v", pattern, err) + } + for _, match := range matches { + names[skillFrontmatterName(t, match)] = true + } + } + + if len(names) == 0 { + t.Fatalf("found no SKILL.md files to validate against") + } + return names +} + +func skillFrontmatterName(t *testing.T, path string) string { + t.Helper() + + file, err := os.Open(path) + if err != nil { + t.Fatalf("failed to open %s: %v", path, err) + } + defer file.Close() + + scanner := bufio.NewScanner(file) + for scanner.Scan() { + line := scanner.Text() + if strings.HasPrefix(line, "name:") { + return strings.TrimSpace(strings.TrimPrefix(line, "name:")) + } + } + if err := scanner.Err(); err != nil { + t.Fatalf("failed to read %s: %v", path, err) + } + + t.Fatalf("%s has no frontmatter name", path) + return "" +} + +// repositoryRoot walks up from the test's working directory until the repository layout is visible, +// because this module's tests run from their own package directory. +func repositoryRoot(t *testing.T) string { + t.Helper() + + directory, err := os.Getwd() + if err != nil { + t.Fatalf("failed to resolve current directory: %v", err) + } + + for { + if _, err := os.Stat(filepath.Join(directory, "Packages", "src", "Editor")); err == nil { + return directory + } + + parent := filepath.Dir(directory) + if parent == directory { + t.Fatalf("failed to find the repository root from %s", directory) + } + directory = parent + } +} diff --git a/cli/dispatcher/internal/dispatcher/command_help.go b/cli/dispatcher/internal/dispatcher/command_help.go index 8d6367b938..5855645bd7 100644 --- a/cli/dispatcher/internal/dispatcher/command_help.go +++ b/cli/dispatcher/internal/dispatcher/command_help.go @@ -106,6 +106,19 @@ func printToolHelp(tool clicore.ToolDefinition, stdout io.Writer) { clicore.WriteLine(stdout, "") printGlobalOptionsHelp(stdout) + printSkillGuidanceHelp(tool.Name, stdout) +} + +// printSkillGuidanceHelp closes a command's help with the instruction to load its skill. Nothing is +// printed for a command with no skill (custom commands), so the output never names a skill that +// cannot be loaded. +func printSkillGuidanceHelp(command string, stdout io.Writer) { + guidance, ok := tooldocs.SkillGuidanceLine(command) + if !ok { + return + } + clicore.WriteLine(stdout, "") + clicore.WriteLine(stdout, guidance) } func nativeCommandDescription(command string) (string, bool) { diff --git a/cli/dispatcher/internal/dispatcher/help_test.go b/cli/dispatcher/internal/dispatcher/help_test.go index 70108404cc..366a4b6ef5 100644 --- a/cli/dispatcher/internal/dispatcher/help_test.go +++ b/cli/dispatcher/internal/dispatcher/help_test.go @@ -253,6 +253,53 @@ func TestCommandHelpPrefersProjectCacheForDefaultToolNames(t *testing.T) { } } +// Verifies a tool's help closes with the instruction to load its skill, which is the only pointer +// from --help to the workflow rules and response shapes that --help itself cannot carry. +func TestCommandHelpPointsAtTheToolSkill(t *testing.T) { + var stdout bytes.Buffer + var stderr bytes.Buffer + + handled, code := tryHandleCommandHelp("simulate-keyboard", "", "", &stdout, &stderr) + + if !handled || code != 0 { + t.Fatalf("simulate-keyboard help was not handled: handled=%v code=%d stderr=%s", handled, code, stderr.String()) + } + if !strings.Contains(stdout.String(), "Load the uloop-simulate-keyboard skill") { + t.Fatalf("simulate-keyboard help does not point at its skill:\n%s", stdout.String()) + } +} + +// Verifies a command with no matching skill gets no guidance line, so a custom command is never +// told to load a skill nobody installed. +func TestCommandHelpOmitsSkillLineForCustomCommands(t *testing.T) { + projectRoot := createLaunchTestProject(t) + writeToolCache(t, projectRoot, `{ + "tools": [ + { + "name": "my-custom-command", + "description": "A project-local custom command", + "inputSchema": { + "type": "object", + "properties": { + "Value": {"type": "string", "description": "Some value"} + } + } + } + ] +}`) + var stdout bytes.Buffer + var stderr bytes.Buffer + + handled, code := tryHandleCommandHelp("my-custom-command", projectRoot, projectRoot, &stdout, &stderr) + + if !handled || code != 0 { + t.Fatalf("custom command help was not handled: handled=%v code=%d stderr=%s", handled, code, stderr.String()) + } + if strings.Contains(stdout.String(), "skill") { + t.Fatalf("custom command help mentions a skill:\n%s", stdout.String()) + } +} + // Verifies enable-watch stays a plain default tool and still exposes schema-driven option help. func TestCommandHelpUsesWatchToolSchemaForDefaultWatchCommands(t *testing.T) { var stdout bytes.Buffer diff --git a/cli/project-runner/internal/projectrunner/native_command_help.go b/cli/project-runner/internal/projectrunner/native_command_help.go index eec18faba8..e2b83e0570 100644 --- a/cli/project-runner/internal/projectrunner/native_command_help.go +++ b/cli/project-runner/internal/projectrunner/native_command_help.go @@ -78,6 +78,13 @@ func printNativeCommandHelp(command string, stdout io.Writer) { clicore.WriteLine(stdout, "") clicore.WriteLine(stdout, "Global options:") clicore.WriteFormat(stdout, " --%s Run against a Unity project outside the current directory\n", tooldocs.ProjectPathFlagName) + + // Runner-owned commands print their own help, so the dispatcher's closing skill line never + // reaches them: await-pause-point and pause-point-status need it added here. + if guidance, ok := tooldocs.SkillGuidanceLine(command); ok { + clicore.WriteLine(stdout, "") + clicore.WriteLine(stdout, guidance) + } } // pausePointUnknownOptionError reports an unrecognized flag for a runner-owned native 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 dcbe321e03..143f577770 100644 --- a/cli/project-runner/internal/projectrunner/native_command_help_test.go +++ b/cli/project-runner/internal/projectrunner/native_command_help_test.go @@ -51,6 +51,28 @@ func TestRunProjectLocalPausePointStatusHelpListsExpectedFlags(t *testing.T) { } } +// Verifies runner-owned pause-point commands close their help with the instruction to load the +// pause-point skill. The dispatcher never renders these commands' help, so its own closing line +// cannot reach them. +func TestRunProjectLocalPausePointHelpPointsAtTheSkill(t *testing.T) { + for _, command := range []string{"await-pause-point", "pause-point-status"} { + t.Run(command, func(t *testing.T) { + t.Chdir(t.TempDir()) + + var stdout bytes.Buffer + var stderr bytes.Buffer + code := RunProjectLocal(context.Background(), []string{command, "--help"}, &stdout, &stderr) + + if code != 0 { + t.Fatalf("%s --help failed: code=%d stderr=%s", command, code, stderr.String()) + } + if !strings.Contains(stdout.String(), "uloop-pause-point skill") { + t.Fatalf("%s --help must point at the pause-point skill: %s", command, stdout.String()) + } + }) + } +} + // Verifies list/sync/focus-window --help print only the global --project-path // option, since these commands take no command-specific flags. func TestRunProjectLocalNoOptionCommandsHelpListsOnlyGlobalOption(t *testing.T) { From 193f4ea110f19d0682a669efc80f6a498f1f7d71 Mon Sep 17 00:00:00 2001 From: hatayama Date: Mon, 27 Jul 2026 00:52:20 +0900 Subject: [PATCH 4/8] Restore option help text inside a synced Unity project Unity's schema generator emits "Parameter: " for every property with no [Description] attribute, and the generated cache has no tool-level description field at all. Inside a synced project that made `--help` and `uloop list` strictly worse than having no cache: 99 of 102 cached options carried no information, and the tool summary line disappeared entirely. Fall back to the descriptions embedded in this binary when the cache's text is a placeholder. Only description text is taken; type, enum, default, and required always stay as the cache reported them, since Unity is the authority on what the running Editor accepts. Authored descriptions and tools absent from the embedded catalog (custom commands) are left untouched. Both readers are wired up, because `list` formats the raw response and never passes through the project-cache loader. Also document that simulate-keyboard rejects bare digit keys, and that enable-pause-point's --max-preview-elements caps later pause-point-status previews too. --- cli/common/tools/catalog.go | 4 +- cli/common/tools/default-tools.json | 4 +- cli/common/tools/description_fallback.go | 59 +++++ cli/common/tools/description_fallback_test.go | 205 ++++++++++++++++++ .../internal/projectrunner/list_output.go | 12 +- .../projectrunner/list_output_test.go | 29 +++ 6 files changed, 307 insertions(+), 6 deletions(-) create mode 100644 cli/common/tools/description_fallback.go create mode 100644 cli/common/tools/description_fallback_test.go diff --git a/cli/common/tools/catalog.go b/cli/common/tools/catalog.go index 426a772a6f..53ee3ab19c 100644 --- a/cli/common/tools/catalog.go +++ b/cli/common/tools/catalog.go @@ -44,7 +44,9 @@ func LoadProjectCache(projectRoot string, internalToolNames map[string]bool) (To if json.Unmarshal(content, &cache) != nil { return ToolCatalog{}, false } - return FilterInternalTools(cache, internalToolNames), true + // Unity's generator produces placeholder descriptions, so a synced cache alone would strip every + // option's help text; the embedded catalog fills those gaps in. + return ApplyEmbeddedDescriptionFallback(FilterInternalTools(cache, internalToolNames)), true } func LoadDefault() ToolCatalog { diff --git a/cli/common/tools/default-tools.json b/cli/common/tools/default-tools.json index 10ee3bc3e2..3dc202fadd 100644 --- a/cli/common/tools/default-tools.json +++ b/cli/common/tools/default-tools.json @@ -385,7 +385,7 @@ }, "MaxPreviewElements": { "type": "integer", - "description": "Maximum number of elements to include in a captured collection's preview (1-1000)", + "description": "Maximum number of elements to include in a captured collection's preview (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.", "default": 10 } } @@ -622,7 +622,7 @@ }, "Key": { "type": "string", - "description": "Key name matching Input System Key enum (e.g. \"W\", \"Space\", \"LeftShift\", \"A\", \"Return\"). Case-insensitive. Not required for ReleaseAll." + "description": "Key name matching Input System Key enum (e.g. \"W\", \"Space\", \"LeftShift\", \"A\", \"Enter\", \"Digit3\"). Case-insensitive. Digit keys use Digit0-Digit9 or Numpad0-Numpad9, not bare 0-9. Not required for ReleaseAll." }, "Duration": { "type": "number", diff --git a/cli/common/tools/description_fallback.go b/cli/common/tools/description_fallback.go new file mode 100644 index 0000000000..60b1dc17f1 --- /dev/null +++ b/cli/common/tools/description_fallback.go @@ -0,0 +1,59 @@ +package tools + +// unityPlaceholderDescriptionPrefix is what Unity's schema generator emits for a property that +// carries no [Description] attribute (UnityCliLoopToolParameterSchemaGenerator.GetDescription +// returns "Parameter: "). The generated schema cache is therefore almost entirely +// placeholders, and the cache has no tool-level description field at all. +const unityPlaceholderDescriptionPrefix = "Parameter: " + +// ApplyEmbeddedDescriptionFallback fills in the descriptions a synced project cache does not carry, +// using the catalog embedded in this binary. Without it, every option of every tool reads +// "Parameter: " inside a synced project while the same command outside a project shows real +// help — the cache was strictly worse than having no cache. +// +// Only description text is taken from the embedded catalog. Type, enum, default, required, and +// hidden always stay as the cache reported them, because Unity is the authority on what the running +// Editor actually accepts. The embedded text may come from an older or newer generation than the +// installed package, which is why it is a fallback and not a replacement. +func ApplyEmbeddedDescriptionFallback(catalog ToolCatalog) ToolCatalog { + embedded := LoadDefault() + + for index, tool := range catalog.Tools { + embeddedTool, ok := Find(embedded, tool.Name) + if !ok { + // A tool the embedded catalog does not know is a project-local custom command: its + // author's own [Description] text is all there is, so it passes through untouched. + continue + } + + if tool.Description == "" { + tool.Description = embeddedTool.Description + } + fillPlaceholderPropertyDescriptions(tool.EffectiveInputSchema(), embeddedTool.EffectiveInputSchema()) + catalog.Tools[index] = tool + } + + return catalog +} + +func fillPlaceholderPropertyDescriptions(schema ToolInputSchema, embeddedSchema ToolInputSchema) { + for propertyName, property := range schema.Properties { + if !isPlaceholderDescription(property.Description, propertyName) { + // A real description means the schema author wrote one; overwriting it would discard + // the more specific text in favor of this binary's generation. + continue + } + + embeddedProperty, ok := embeddedSchema.Properties[propertyName] + if !ok || isPlaceholderDescription(embeddedProperty.Description, propertyName) { + continue + } + + property.Description = embeddedProperty.Description + schema.Properties[propertyName] = property + } +} + +func isPlaceholderDescription(description string, propertyName string) bool { + return description == "" || description == unityPlaceholderDescriptionPrefix+propertyName +} diff --git a/cli/common/tools/description_fallback_test.go b/cli/common/tools/description_fallback_test.go new file mode 100644 index 0000000000..9727ff9aae --- /dev/null +++ b/cli/common/tools/description_fallback_test.go @@ -0,0 +1,205 @@ +package tools + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// Verifies a cached property whose description is Unity's generated placeholder is replaced with the +// embedded catalog's real description, since the placeholder carries no information at all. +func TestApplyEmbeddedDescriptionFallbackReplacesPlaceholders(t *testing.T) { + catalog := ToolCatalog{Tools: []ToolDefinition{{ + Name: "simulate-keyboard", + ParameterSchema: ToolInputSchema{Properties: map[string]ToolProperty{ + "Duration": {Type: "number", Description: "Parameter: Duration"}, + }}, + }}} + + result := ApplyEmbeddedDescriptionFallback(catalog) + + description := result.Tools[0].EffectiveInputSchema().Properties["Duration"].Description + if description == "Parameter: Duration" || description == "" { + t.Fatalf("placeholder description was not replaced: %q", description) + } +} + +// Verifies an empty description is treated as a placeholder too, since it is equally uninformative. +func TestApplyEmbeddedDescriptionFallbackReplacesEmptyDescriptions(t *testing.T) { + catalog := ToolCatalog{Tools: []ToolDefinition{{ + Name: "simulate-keyboard", + ParameterSchema: ToolInputSchema{Properties: map[string]ToolProperty{ + "Duration": {Type: "number", Description: ""}, + }}, + }}} + + result := ApplyEmbeddedDescriptionFallback(catalog) + + if result.Tools[0].EffectiveInputSchema().Properties["Duration"].Description == "" { + t.Fatal("empty description was not replaced") + } +} + +// Verifies a description the schema author actually wrote is never overwritten: the cache is +// authoritative whenever it carries real information. +func TestApplyEmbeddedDescriptionFallbackKeepsAuthoredDescriptions(t *testing.T) { + catalog := ToolCatalog{Tools: []ToolDefinition{{ + Name: "simulate-keyboard", + Description: "Cached tool description", + ParameterSchema: ToolInputSchema{Properties: map[string]ToolProperty{ + "Duration": {Type: "number", Description: "Authored duration description"}, + }}, + }}} + + result := ApplyEmbeddedDescriptionFallback(catalog) + + if result.Tools[0].Description != "Cached tool description" { + t.Errorf("tool description was overwritten: %q", result.Tools[0].Description) + } + if got := result.Tools[0].EffectiveInputSchema().Properties["Duration"].Description; got != "Authored duration description" { + t.Errorf("authored property description was overwritten: %q", got) + } +} + +// Verifies a tool absent from the embedded catalog passes through untouched, so a project's custom +// commands are not silently emptied or matched against an unrelated tool. +func TestApplyEmbeddedDescriptionFallbackLeavesCustomToolsAlone(t *testing.T) { + catalog := ToolCatalog{Tools: []ToolDefinition{{ + Name: "my-custom-command", + ParameterSchema: ToolInputSchema{Properties: map[string]ToolProperty{ + "Value": {Type: "string", Description: "Parameter: Value"}, + }}, + }}} + + result := ApplyEmbeddedDescriptionFallback(catalog) + + if result.Tools[0].Description != "" { + t.Errorf("custom tool gained a description: %q", result.Tools[0].Description) + } + if got := result.Tools[0].EffectiveInputSchema().Properties["Value"].Description; got != "Parameter: Value" { + t.Errorf("custom tool property description changed: %q", got) + } +} + +// Verifies a property the embedded catalog does not know keeps its placeholder rather than picking +// up an unrelated description. +func TestApplyEmbeddedDescriptionFallbackKeepsUnknownProperties(t *testing.T) { + catalog := ToolCatalog{Tools: []ToolDefinition{{ + Name: "simulate-keyboard", + ParameterSchema: ToolInputSchema{Properties: map[string]ToolProperty{ + "NewlyAddedOption": {Type: "string", Description: "Parameter: NewlyAddedOption"}, + }}, + }}} + + result := ApplyEmbeddedDescriptionFallback(catalog) + + if got := result.Tools[0].EffectiveInputSchema().Properties["NewlyAddedOption"].Description; got != "Parameter: NewlyAddedOption" { + t.Errorf("unknown property description changed: %q", got) + } +} + +// Verifies enum, default, type, and required stay as the cache reported them: Unity is the truth for +// everything except the description text. +func TestApplyEmbeddedDescriptionFallbackKeepsCachedSchemaFacts(t *testing.T) { + catalog := ToolCatalog{Tools: []ToolDefinition{{ + Name: "simulate-keyboard", + ParameterSchema: ToolInputSchema{ + Properties: map[string]ToolProperty{ + "Action": { + Type: "string", + Description: "Parameter: Action", + DefaultValue: float64(0), + Enum: []string{"Press", "OnlyCachedMember"}, + }, + }, + Required: []string{"Action"}, + }, + }}} + + result := ApplyEmbeddedDescriptionFallback(catalog) + + property := result.Tools[0].EffectiveInputSchema().Properties["Action"] + if len(property.Enum) != 2 || property.Enum[1] != "OnlyCachedMember" { + t.Errorf("cached enum was replaced: %v", property.Enum) + } + if property.EffectiveDefault() != float64(0) { + t.Errorf("cached default was replaced: %v", property.EffectiveDefault()) + } + if required := result.Tools[0].EffectiveInputSchema().Required; len(required) != 1 || required[0] != "Action" { + t.Errorf("cached required list was replaced: %v", required) + } +} + +// Verifies the fallback is applied when a project cache is loaded, which is the path both `--help` +// and `uloop list` read in a synced project. +func TestLoadProjectCacheAppliesDescriptionFallback(t *testing.T) { + projectRoot := t.TempDir() + cacheDirectory := filepath.Join(projectRoot, CacheDirectoryName) + if err := os.MkdirAll(cacheDirectory, 0o755); err != nil { + t.Fatalf("failed to create cache directory: %v", err) + } + content := `{"Tools":[{"name":"simulate-keyboard","parameterSchema":{"Properties":{"Duration":{"Type":"number","Description":"Parameter: Duration"}}}}]}` + if err := os.WriteFile(filepath.Join(cacheDirectory, CacheFileName), []byte(content), 0o644); err != nil { + t.Fatalf("failed to write cache: %v", err) + } + + catalog, ok := LoadProjectCache(projectRoot, nil) + if !ok { + t.Fatal("project cache was not loaded") + } + if catalog.Tools[0].Description == "" { + t.Error("tool description was not filled from the embedded catalog") + } + if got := catalog.Tools[0].EffectiveInputSchema().Properties["Duration"].Description; got == "Parameter: Duration" { + t.Errorf("property placeholder was not replaced: %q", got) + } +} + +// Verifies the embedded catalog itself has no placeholder descriptions, since it is the source the +// fallback reads from. +func TestEmbeddedCatalogHasNoPlaceholderDescriptions(t *testing.T) { + for _, tool := range LoadDefault().Tools { + if tool.Description == "" { + t.Errorf("embedded tool %s has no description", tool.Name) + } + for propertyName, property := range tool.EffectiveInputSchema().Properties { + if isPlaceholderDescription(property.Description, propertyName) { + t.Errorf("embedded tool %s property %s has a placeholder description", tool.Name, propertyName) + } + } + } +} + +// Verifies the documented digit-key rule reaches simulate-keyboard's --key description, so a caller +// reading only `--help` learns that bare digits are rejected. +func TestEmbeddedSimulateKeyboardKeyDescriptionDocumentsDigitKeys(t *testing.T) { + tool, ok := Find(LoadDefault(), "simulate-keyboard") + if !ok { + t.Fatal("embedded catalog has no simulate-keyboard tool") + } + + description := tool.EffectiveInputSchema().Properties["Key"].Description + for _, expected := range []string{"Digit3", "Numpad0"} { + if !strings.Contains(description, expected) { + t.Errorf("--key description does not mention %q: %q", expected, description) + } + } + if strings.Contains(description, "\"Return\"") { + t.Errorf("--key description still offers the rejected Return example: %q", description) + } +} + +// Verifies enable-pause-point documents that --max-preview-elements also shapes later +// pause-point-status responses, which is not discoverable from the option name. +func TestEmbeddedEnablePausePointDocumentsMaxPreviewElementsCarryOver(t *testing.T) { + tool, ok := Find(LoadDefault(), "enable-pause-point") + if !ok { + t.Fatal("embedded catalog has no enable-pause-point tool") + } + + description := tool.EffectiveInputSchema().Properties["MaxPreviewElements"].Description + if !strings.Contains(description, "pause-point-status") { + t.Errorf("--max-preview-elements description does not mention pause-point-status: %q", description) + } +} diff --git a/cli/project-runner/internal/projectrunner/list_output.go b/cli/project-runner/internal/projectrunner/list_output.go index 03ccee6da6..cb24b901d0 100644 --- a/cli/project-runner/internal/projectrunner/list_output.go +++ b/cli/project-runner/internal/projectrunner/list_output.go @@ -8,6 +8,7 @@ import ( "github.com/hatayama/unity-cli-loop/common/clicontract" "github.com/hatayama/unity-cli-loop/common/clicore" + "github.com/hatayama/unity-cli-loop/common/tools" ) type listCatalog struct { @@ -37,6 +38,11 @@ func formatToolListResult(result json.RawMessage) json.RawMessage { return result } + // list formats get-tool-details' raw response and never goes through the project-cache loader, + // so the placeholder fallback has to be applied here too. Without it `--help` would show real + // descriptions while `list` kept reporting "Parameter: ". + cache = tools.ApplyEmbeddedDescriptionFallback(cache) + content, err := json.Marshal(newListCatalog(cache)) if err != nil { panic(err) @@ -45,9 +51,9 @@ func formatToolListResult(result json.RawMessage) json.RawMessage { } func newListCatalog(cache clicore.ToolsCache) listCatalog { - tools := make([]listTool, 0, len(cache.Tools)) + listTools := make([]listTool, 0, len(cache.Tools)) for _, tool := range cache.Tools { - tools = append(tools, newListTool(tool)) + listTools = append(listTools, newListTool(tool)) } // Sourced from the embedded CLI contract because the tool catalog no longer @@ -56,7 +62,7 @@ func newListCatalog(cache clicore.ToolsCache) listCatalog { Version: clicontract.ProjectRunnerVersion(), ServerVersion: cache.ServerVersion, UpdatedAt: cache.UpdatedAt, - Tools: tools, + Tools: listTools, } } diff --git a/cli/project-runner/internal/projectrunner/list_output_test.go b/cli/project-runner/internal/projectrunner/list_output_test.go index 21b26b8739..4828368c45 100644 --- a/cli/project-runner/internal/projectrunner/list_output_test.go +++ b/cli/project-runner/internal/projectrunner/list_output_test.go @@ -151,6 +151,35 @@ func TestNewListCatalogIncludesEnablePausePointAwaitOptions(t *testing.T) { findListOption(t, enablePausePoint, "--"+tooldocs.PausePointResumePlayFlagName) } +// Tests that list replaces Unity's generated placeholder descriptions with the embedded catalog's +// real text. list formats the raw get-tool-details response, so it does not inherit the fallback the +// project-cache loader applies for `--help`. +func TestFormatToolListResultFillsPlaceholderDescriptions(t *testing.T) { + content := formatToolListResult([]byte(`{ + "tools": [ + { + "name": "simulate-keyboard", + "parameterSchema": { + "Properties": { + "Duration": {"Type": "number", "Description": "Parameter: Duration"} + } + } + } + ] +}`)) + + catalog := decodeListCatalog(t, content) + simulateKeyboard := findListTool(t, catalog, "simulate-keyboard") + + if simulateKeyboard.Description == "" { + t.Error("tool description was not filled from the embedded catalog") + } + option := findListOption(t, simulateKeyboard, "--duration") + if option.Description == "Parameter: Duration" || option.Description == "" { + t.Errorf("option placeholder description was not replaced: %q", option.Description) + } +} + func decodeListCatalog(t *testing.T, content []byte) listCatalog { t.Helper() From 6fde213e17461fa100296fdd2ed8bb177342e7b6 Mon Sep 17 00:00:00 2001 From: hatayama Date: Mon, 27 Jul 2026 00:54:06 +0900 Subject: [PATCH 5/8] Reconcile shared release inputs for the option help changes cli/common carries non-test source changes in this branch, so the dispatcher and project-runner release triggers have to be restamped in the same PR or check-release-triggers fails. --- cli/common/tooldocs/skill_guidance_test.go | 13 +++---------- cli/dispatcher/shared-inputs-stamp.json | 2 +- cli/project-runner/shared-inputs-stamp.json | 2 +- 3 files changed, 5 insertions(+), 12 deletions(-) diff --git a/cli/common/tooldocs/skill_guidance_test.go b/cli/common/tooldocs/skill_guidance_test.go index 9a0f163da4..f818aa06a6 100644 --- a/cli/common/tooldocs/skill_guidance_test.go +++ b/cli/common/tooldocs/skill_guidance_test.go @@ -1,7 +1,6 @@ package tooldocs import ( - "bufio" "os" "path/filepath" "strings" @@ -88,22 +87,16 @@ func installedSkillNames(t *testing.T) map[string]bool { func skillFrontmatterName(t *testing.T, path string) string { t.Helper() - file, err := os.Open(path) + content, err := os.ReadFile(path) if err != nil { - t.Fatalf("failed to open %s: %v", path, err) + t.Fatalf("failed to read %s: %v", path, err) } - defer file.Close() - scanner := bufio.NewScanner(file) - for scanner.Scan() { - line := scanner.Text() + for _, line := range strings.Split(string(content), "\n") { if strings.HasPrefix(line, "name:") { return strings.TrimSpace(strings.TrimPrefix(line, "name:")) } } - if err := scanner.Err(); err != nil { - t.Fatalf("failed to read %s: %v", path, err) - } t.Fatalf("%s has no frontmatter name", path) return "" diff --git a/cli/dispatcher/shared-inputs-stamp.json b/cli/dispatcher/shared-inputs-stamp.json index e665d3dbb9..ad234b6b29 100644 --- a/cli/dispatcher/shared-inputs-stamp.json +++ b/cli/dispatcher/shared-inputs-stamp.json @@ -1,4 +1,4 @@ { "schemaVersion": 1, - "sharedInputsHash": "8a863e4239e16efaf67ef4b7a2dfa7ed17a36b65" + "sharedInputsHash": "e98f01c7ee2474c3cf7d215d66805e63aa7877d6" } diff --git a/cli/project-runner/shared-inputs-stamp.json b/cli/project-runner/shared-inputs-stamp.json index b6cd063854..e8ee8d8193 100644 --- a/cli/project-runner/shared-inputs-stamp.json +++ b/cli/project-runner/shared-inputs-stamp.json @@ -1,4 +1,4 @@ { "schemaVersion": 1, - "sharedInputsHash": "962315b78bf904764fa1f75bbd7d6d2ca9ffe092" + "sharedInputsHash": "b601b60304696ae68084ae9d47bd375ee9c7c09a" } From 0b8e57d3e5b808eb77226ff0068a6a710f05e317 Mon Sep 17 00:00:00 2001 From: hatayama Date: Mon, 27 Jul 2026 01:16:59 +0900 Subject: [PATCH 6/8] Extend and correct the --help skill guidance Review follow-ups on the skill guidance line and option help formatting: - The watch commands and `launch` now get the line too. Watch expressions are documented by the pause-point skill, and launch renders its own help, so it was the one command with a real skill that never mentioned it. - The wording no longer refers to "options not listed above": commands such as focus-window have no options for that phrasing to point at. It now names what the skill adds instead. - An empty-string default is no longer rendered, since Unity reports one for every unset string parameter and it printed a bare "default: " with nothing after it. `uloop list` already omitted it. - Widen the option column so --captured-variable-names does not push its own description out of alignment. - Route list's fallback call through the clicore facade like every other tool catalog access in that package. - Build Unity's placeholder description from the JSON property name. The CLI recognizes the placeholder by comparing against the JSON name it received, so a [JsonProperty] rename would otherwise leave the placeholder in help output as if it were a real description. --- ...nityCliLoopToolParameterSchemaGenerator.cs | 7 ++- cli/common/clicore/tool_catalog.go | 4 ++ cli/common/tooldocs/skill_guidance.go | 14 +++-- cli/common/tooldocs/tool_option_help.go | 4 +- .../internal/dispatcher/command_help.go | 6 ++- .../internal/dispatcher/help_test.go | 53 +++++++++++++++++++ cli/dispatcher/internal/dispatcher/launch.go | 3 ++ .../internal/projectrunner/list_output.go | 3 +- 8 files changed, 85 insertions(+), 9 deletions(-) diff --git a/Packages/src/Editor/ToolContracts/UnityCliLoopToolParameterSchemaGenerator.cs b/Packages/src/Editor/ToolContracts/UnityCliLoopToolParameterSchemaGenerator.cs index 3ffce80653..cc25aefa66 100644 --- a/Packages/src/Editor/ToolContracts/UnityCliLoopToolParameterSchemaGenerator.cs +++ b/Packages/src/Editor/ToolContracts/UnityCliLoopToolParameterSchemaGenerator.cs @@ -132,8 +132,11 @@ private static string GetDescription(PropertyInfo property) if (descAttr != null) return descAttr.Description; - // Generate default description - return $"Parameter: {property.Name}"; + // Generate default description. Built from the JSON property name, not the C# one, + // because the CLI recognizes this placeholder by comparing it against the JSON name it + // received: a [JsonProperty] rename would otherwise make the placeholder unrecognizable + // and leave it in --help output as if it were a real description. + return $"Parameter: {GetJsonPropertyName(property)}"; } /// diff --git a/cli/common/clicore/tool_catalog.go b/cli/common/clicore/tool_catalog.go index 48a825766f..d681745e16 100644 --- a/cli/common/clicore/tool_catalog.go +++ b/cli/common/clicore/tool_catalog.go @@ -29,6 +29,10 @@ func LoadDefaultTools() ToolsCache { return tools.LoadDefault() } +func ApplyEmbeddedDescriptionFallback(cache ToolsCache) ToolsCache { + return tools.ApplyEmbeddedDescriptionFallback(cache) +} + func FindTool(cache ToolsCache, name string) (ToolDefinition, bool) { return tools.Find(cache, name) } diff --git a/cli/common/tooldocs/skill_guidance.go b/cli/common/tooldocs/skill_guidance.go index fb415f6436..6c01f9cbdb 100644 --- a/cli/common/tooldocs/skill_guidance.go +++ b/cli/common/tooldocs/skill_guidance.go @@ -24,11 +24,17 @@ var commandSkillNames = map[string]string{ "simulate-mouse-input": "uloop-simulate-mouse-input", "simulate-mouse-ui": "uloop-simulate-mouse-ui", - // One skill, four commands. + "launch": "uloop-launch", + + // One skill covers the four pause-point commands and the three watch commands: watch + // expressions are documented by the pause-point skill's references/watch-expressions.md. "enable-pause-point": "uloop-pause-point", "clear-pause-point": "uloop-pause-point", "await-pause-point": "uloop-pause-point", "pause-point-status": "uloop-pause-point", + "enable-watch": "uloop-pause-point", + "clear-watch": "uloop-pause-point", + "get-watch-values": "uloop-pause-point", } // SkillGuidanceLine returns the closing line of a command's --help output: an instruction to load @@ -37,11 +43,13 @@ var commandSkillNames = map[string]string{ // agent that found the command through --help has no way to learn the skill exists. // // Phrased as an instruction rather than a cross-reference, because a line that merely mentions a -// document is easy to read past. +// document is easy to read past. It names what the skill adds rather than referring to the options +// above, because commands such as focus-window have no options for that phrasing to point at. func SkillGuidanceLine(command string) (string, bool) { skillName, ok := commandSkillNames[command] if !ok { return "", false } - return "Load the " + skillName + " skill before using options not listed above.", true + return "Load the " + skillName + + " skill for workflow rules and response fields that --help does not cover.", true } diff --git a/cli/common/tooldocs/tool_option_help.go b/cli/common/tooldocs/tool_option_help.go index 5a1bffe65f..a1a34e6fcf 100644 --- a/cli/common/tooldocs/tool_option_help.go +++ b/cli/common/tooldocs/tool_option_help.go @@ -93,7 +93,9 @@ func optionDescription(toolName string, propertyName string, property tools.Tool if description := OptionSummary(toolName, propertyName, property); description != "" { parts = append(parts, description) } - if propertyDefault := property.EffectiveDefault(); propertyDefault != nil { + // An empty-string default is what Unity reports for any unset string parameter, so rendering it + // would print a bare "default: " with nothing after it. + if propertyDefault := property.EffectiveDefault(); propertyDefault != nil && propertyDefault != "" { parts = append(parts, "default: "+defaultValueText(propertyDefault, property.Enum)) } if len(property.Enum) > 0 { diff --git a/cli/dispatcher/internal/dispatcher/command_help.go b/cli/dispatcher/internal/dispatcher/command_help.go index 5855645bd7..56d9172c43 100644 --- a/cli/dispatcher/internal/dispatcher/command_help.go +++ b/cli/dispatcher/internal/dispatcher/command_help.go @@ -67,6 +67,7 @@ func printNativeSingleCommandHelp(command string, stdout io.Writer) { clicore.WriteLine(stdout, "") printGlobalOptionsHelp(stdout) } + printSkillGuidanceHelp(command, stdout) return } @@ -79,6 +80,7 @@ func printNativeSingleCommandHelp(command string, stdout io.Writer) { clicore.WriteLine(stdout, "") printGlobalOptionsHelp(stdout) } + printSkillGuidanceHelp(command, stdout) } func printToolHelp(tool clicore.ToolDefinition, stdout io.Writer) { @@ -100,7 +102,9 @@ func printToolHelp(tool clicore.ToolDefinition, stdout io.Writer) { clicore.WriteLine(stdout, "") clicore.WriteLine(stdout, "Options:") for _, entry := range entries { - clicore.WriteFormat(stdout, " %-32s %s\n", entry.Usage, entry.Description) + // Wide enough for the longest usage string (--captured-variable-names ), so no + // single row pushes its description out of the column. + clicore.WriteFormat(stdout, " %-34s %s\n", entry.Usage, entry.Description) } } diff --git a/cli/dispatcher/internal/dispatcher/help_test.go b/cli/dispatcher/internal/dispatcher/help_test.go index 366a4b6ef5..77f2b1a57b 100644 --- a/cli/dispatcher/internal/dispatcher/help_test.go +++ b/cli/dispatcher/internal/dispatcher/help_test.go @@ -269,6 +269,59 @@ func TestCommandHelpPointsAtTheToolSkill(t *testing.T) { } } +// Verifies the watch commands point at the pause-point skill, which is where watch expressions are +// documented, rather than at a per-command skill that does not exist. +func TestCommandHelpPointsWatchCommandsAtThePausePointSkill(t *testing.T) { + for _, command := range []string{"enable-watch", "clear-watch", "get-watch-values"} { + t.Run(command, func(t *testing.T) { + var stdout bytes.Buffer + var stderr bytes.Buffer + + handled, code := tryHandleCommandHelp(command, "", "", &stdout, &stderr) + + if !handled || code != 0 { + t.Fatalf("%s help was not handled: handled=%v code=%d stderr=%s", command, handled, code, stderr.String()) + } + if !strings.Contains(stdout.String(), "Load the uloop-pause-point skill") { + t.Fatalf("%s help does not point at the pause-point skill:\n%s", command, stdout.String()) + } + }) + } +} + +// Verifies dispatcher-owned native commands get the guidance line too: launch renders its own help, +// so it would otherwise be the only command with a skill that never mentions it. +func TestLaunchHelpPointsAtTheLaunchSkill(t *testing.T) { + t.Chdir(t.TempDir()) + var stdout bytes.Buffer + var stderr bytes.Buffer + + code := RunDispatcher(context.Background(), []string{clicore.LaunchCommandName, "--help"}, &stdout, &stderr) + + if code != 0 { + t.Fatalf("launch help failed: code=%d stderr=%s", code, stderr.String()) + } + if !strings.Contains(stdout.String(), "Load the uloop-launch skill") { + t.Fatalf("launch help does not point at its skill:\n%s", stdout.String()) + } +} + +// Verifies a native command whose skill is internal-only (or absent) gets no guidance line. +func TestVersionHelpOmitsSkillLine(t *testing.T) { + t.Chdir(t.TempDir()) + var stdout bytes.Buffer + var stderr bytes.Buffer + + code := RunDispatcher(context.Background(), []string{clicore.VersionCommandName, "--help"}, &stdout, &stderr) + + if code != 0 { + t.Fatalf("version help failed: code=%d stderr=%s", code, stderr.String()) + } + if strings.Contains(stdout.String(), "skill") { + t.Fatalf("version help mentions a skill:\n%s", stdout.String()) + } +} + // Verifies a command with no matching skill gets no guidance line, so a custom command is never // told to load a skill nobody installed. func TestCommandHelpOmitsSkillLineForCustomCommands(t *testing.T) { diff --git a/cli/dispatcher/internal/dispatcher/launch.go b/cli/dispatcher/internal/dispatcher/launch.go index b024e8b214..ea2d938abb 100644 --- a/cli/dispatcher/internal/dispatcher/launch.go +++ b/cli/dispatcher/internal/dispatcher/launch.go @@ -473,4 +473,7 @@ func printLaunchHelp(stdout io.Writer) { clicore.WriteLine(stdout, "Compiler errors are ignored by default during Unity startup.") clicore.WriteLine(stdout, "") printGlobalOptionsHelp(stdout) + // launch renders its own help instead of going through printNativeSingleCommandHelp, so the + // closing skill line has to be printed here as well. + printSkillGuidanceHelp(clicore.LaunchCommandName, stdout) } diff --git a/cli/project-runner/internal/projectrunner/list_output.go b/cli/project-runner/internal/projectrunner/list_output.go index cb24b901d0..ede76b35cd 100644 --- a/cli/project-runner/internal/projectrunner/list_output.go +++ b/cli/project-runner/internal/projectrunner/list_output.go @@ -8,7 +8,6 @@ import ( "github.com/hatayama/unity-cli-loop/common/clicontract" "github.com/hatayama/unity-cli-loop/common/clicore" - "github.com/hatayama/unity-cli-loop/common/tools" ) type listCatalog struct { @@ -41,7 +40,7 @@ func formatToolListResult(result json.RawMessage) json.RawMessage { // list formats get-tool-details' raw response and never goes through the project-cache loader, // so the placeholder fallback has to be applied here too. Without it `--help` would show real // descriptions while `list` kept reporting "Parameter: ". - cache = tools.ApplyEmbeddedDescriptionFallback(cache) + cache = clicore.ApplyEmbeddedDescriptionFallback(cache) content, err := json.Marshal(newListCatalog(cache)) if err != nil { From 5a4fcfd49674cc1ed984641dfe03a6e9735f5b1c Mon Sep 17 00:00:00 2001 From: hatayama Date: Mon, 27 Jul 2026 01:19:35 +0900 Subject: [PATCH 7/8] Fold enable-pause-point's flag parsing into a handler table MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The six CLI-only flags were parsed by six hand-written same-shaped branches, pushing extractPausePointEnableAwaitFlags to 33 branches over 157 lines — past the repository's complexity limit — right as this branch introduced a shared table for the very same flags. Keying the handlers by that table's flag names makes the two sets comparable, so a flag can no longer be documented without being parsed or parsed without being documented; a contract test now asserts the two key sets are equal. That is the direction the previous subset check could not see, and it is how all six flags came to be missing from --help. Behavior-preserving: no existing test changed, and new tests pin the two edges that the branch order encoded implicitly — --await has no =value form and unrelated arguments pass through to schema parsing. --- .../pause_point_cli_options_contract_test.go | 55 ++++ .../projectrunner/pause_point_enable.go | 280 ++++++++++-------- 2 files changed, 205 insertions(+), 130 deletions(-) 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 f7b441ea83..57f35940a4 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 @@ -43,6 +43,61 @@ func TestPausePointEnableHelpOptionsAreAcceptedByTheEnableParser(t *testing.T) { } } +// Verifies the documented flag set and the parsed flag set are exactly the same. The one-directional +// subset checks below cannot see the original defect from the other side: a flag the parser accepts +// but no listing documents is undiscoverable, which is how all six of these flags came to be missing +// from --help in the first place. +func TestPausePointEnableDocumentedAndParsedFlagsMatch(t *testing.T) { + documented := map[string]bool{} + for _, option := range tooldocs.PausePointEnableCLIOnlyOptions() { + documented[option.FlagName] = true + } + + for flagName := range pausePointEnableFlagHandlers { + if !documented[flagName] { + t.Errorf("the enable-pause-point parser accepts --%s but no option listing documents it", flagName) + } + } + for flagName := range documented { + if _, ok := pausePointEnableFlagHandlers[flagName]; !ok { + t.Errorf("--%s is documented but the enable-pause-point parser does not accept it", flagName) + } + } +} + +// Verifies --await has no =value form: it is passed through to Unity schema parsing, which is the +// behavior the parser had before the handler table replaced its open-coded branches. +func TestPausePointEnableAwaitRejectsValueForm(t *testing.T) { + remaining, await, _, _, _, _, _, _, err := extractPausePointEnableAwaitFlags([]string{"--await=true"}) + if err != nil { + t.Fatalf("--await=true should be passed through, not rejected here: %v", err) + } + if await { + t.Error("--await=true must not enable the wait") + } + if len(remaining) != 1 || remaining[0] != "--await=true" { + t.Errorf("--await=true was not passed through: %v", remaining) + } +} + +// Verifies an unrelated argument is passed through untouched for the schema pipeline. +func TestPausePointEnableLeavesSchemaArgumentsAlone(t *testing.T) { + remaining, _, _, _, _, _, _, _, err := extractPausePointEnableAwaitFlags( + []string{"--id", "marker", "--await", "--line", "42"}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + expected := []string{"--id", "marker", "--line", "42"} + if len(remaining) != len(expected) { + t.Fatalf("remaining = %v, want %v", remaining, expected) + } + for index, argument := range expected { + if remaining[index] != argument { + t.Fatalf("remaining = %v, want %v", remaining, expected) + } + } +} + // Verifies the CLI-only flags shared with await-pause-point are accepted by the wait parser as // well, since enable-pause-point --help documents them as "same as await-pause-point's --x". func TestPausePointSharedHelpOptionsAreAcceptedByTheWaitParser(t *testing.T) { diff --git a/cli/project-runner/internal/projectrunner/pause_point_enable.go b/cli/project-runner/internal/projectrunner/pause_point_enable.go index e31f8a78fa..c9a7e61202 100644 --- a/cli/project-runner/internal/projectrunner/pause_point_enable.go +++ b/cli/project-runner/internal/projectrunner/pause_point_enable.go @@ -23,166 +23,186 @@ import ( // extractDynamicCodeFileFlag intercepts --code-file for execute-dynamic-code. const pausePointEnableCommandName = "enable-pause-point" +// pausePointEnableFlagState accumulates the CLI-only flags pulled out of enable-pause-point's argv. +type pausePointEnableFlagState struct { + await bool + mode pausePointCapturedVariablesMode + modeSet bool + capturedVariableNames []string + namesSet bool + expectations []pausePointExpectation + triggerCommand string + triggerArgs []string + triggerSet bool + resumePlay bool +} + +// pausePointEnableFlagHandler consumes one CLI-only flag. applyBare is set for a flag that may be +// passed with no value, applyValue for a flag that accepts one; --await has no value form at all, so +// `--await=x` is deliberately left for Unity schema parsing exactly as before. +type pausePointEnableFlagHandler struct { + applyBare func(state *pausePointEnableFlagState) + applyValue func(state *pausePointEnableFlagState, value string) error +} + +// pausePointEnableFlagHandlers is keyed by the same flag names the option listings advertise +// (tooldocs.PausePointEnableCLIOnlyOptions), so a flag can no longer be documented without being +// parsed or parsed without being documented — a contract test compares the two key sets. +var pausePointEnableFlagHandlers = map[string]pausePointEnableFlagHandler{ + tooldocs.PausePointEnableAwaitFlagName: { + applyBare: func(state *pausePointEnableFlagState) { state.await = true }, + }, + tooldocs.PausePointResumePlayFlagName: { + applyBare: func(state *pausePointEnableFlagState) { state.resumePlay = true }, + // --resume-play=true|1 must be accepted here too: otherwise the =value form falls through + // to Unity schema parsing and becomes a confusing unrelated error. + applyValue: func(state *pausePointEnableFlagState, value string) error { + if value != "true" && value != "1" { + return clierrors.InvalidValueArgumentError( + "--"+tooldocs.PausePointResumePlayFlagName, value, "boolean flag (pass with no value, or =true)") + } + state.resumePlay = true + return nil + }, + }, + tooldocs.PausePointTriggerFlagName: { + applyValue: func(state *pausePointEnableFlagState, value string) error { + triggerCommand, triggerArgs, err := parsePausePointTriggerCommand(pausePointEnableCommandName, value) + if err != nil { + return err + } + state.triggerCommand = triggerCommand + state.triggerArgs = triggerArgs + state.triggerSet = true + return nil + }, + }, + tooldocs.PausePointCapturedVariablesFlagName: { + applyValue: func(state *pausePointEnableFlagState, value string) error { + mode, err := parsePausePointCapturedVariablesMode(value) + if err != nil { + return err + } + state.mode = mode + state.modeSet = true + return nil + }, + }, + tooldocs.PausePointCapturedVariableNamesFlagName: { + applyValue: func(state *pausePointEnableFlagState, value string) error { + state.capturedVariableNames = parsePausePointCapturedVariableNames(value) + state.namesSet = true + return nil + }, + }, + tooldocs.PausePointExpectFlagName: { + applyValue: func(state *pausePointEnableFlagState, value string) error { + expectation, err := parsePausePointExpectFlagValue(value) + if err != nil { + return err + } + state.expectations = append(state.expectations, expectation) + return nil + }, + }, +} + // extractPausePointEnableAwaitFlags pulls the CLI-only --await/--captured-variables/ // --captured-variable-names/--expect/--trigger/--resume-play flags out of enable-pause-point args // before generic schema parsing, because none of them are part of the Unity-side -// EnablePausePointSchema. +// EnablePausePointSchema. Anything this function does not recognize is passed through untouched for +// the generic schema pipeline to handle. func extractPausePointEnableAwaitFlags( args []string, ) ([]string, bool, pausePointCapturedVariablesMode, []string, []pausePointExpectation, string, []string, bool, error) { remaining := make([]string, 0, len(args)) - await := false - mode := pausePointCapturedVariablesModeFull - modeSet := false - var capturedVariableNames []string - namesSet := false - var expectations []pausePointExpectation - var triggerCommand string - var triggerArgs []string - triggerSet := false - resumePlay := false + state := pausePointEnableFlagState{mode: pausePointCapturedVariablesModeFull} for index := 0; index < len(args); index++ { arg := args[index] - if arg == "--"+tooldocs.PausePointEnableAwaitFlagName { - await = true + flagName, handler, ok := findPausePointEnableFlagHandler(arg) + if !ok { + remaining = append(remaining, arg) continue } - if arg == "--"+tooldocs.PausePointResumePlayFlagName { - resumePlay = true + if arg == "--"+flagName && handler.applyBare != nil { + handler.applyBare(&state) continue } - - // --resume-play=true|1 must be accepted here too: otherwise the =value form falls through - // to Unity schema parsing and becomes a confusing unrelated error. - if isPausePointFlag(arg, tooldocs.PausePointResumePlayFlagName) { - name, value, consumedNext, err := clicore.ParseFlagValue(arg, args, index) - if err != nil { - return nil, false, mode, nil, nil, "", nil, false, err - } - if name != tooldocs.PausePointResumePlayFlagName { - remaining = append(remaining, arg) - continue - } - if value != "true" && value != "1" { - return nil, false, mode, nil, nil, "", nil, false, clierrors.InvalidValueArgumentError( - "--"+tooldocs.PausePointResumePlayFlagName, value, "boolean flag (pass with no value, or =true)") - } - resumePlay = true - if consumedNext { - index++ - } + if handler.applyValue == nil { + remaining = append(remaining, arg) continue } - if isPausePointFlag(arg, tooldocs.PausePointTriggerFlagName) { - name, value, consumedNext, err := clicore.ParseFlagValue(arg, args, index) - if err != nil { - return nil, false, mode, nil, nil, "", nil, false, err - } - if name != tooldocs.PausePointTriggerFlagName { - remaining = append(remaining, arg) - continue - } - parsedCommand, parsedArgs, parseErr := parsePausePointTriggerCommand(pausePointEnableCommandName, value) - if parseErr != nil { - return nil, false, mode, nil, nil, "", nil, false, parseErr - } - triggerCommand = parsedCommand - triggerArgs = parsedArgs - triggerSet = true - if consumedNext { - index++ - } - continue + name, value, consumedNext, err := clicore.ParseFlagValue(arg, args, index) + if err != nil { + return nil, false, state.mode, nil, nil, "", nil, false, err } - - if isPausePointFlag(arg, tooldocs.PausePointCapturedVariablesFlagName) { - name, value, consumedNext, err := clicore.ParseFlagValue(arg, args, index) - if err != nil { - return nil, false, mode, nil, nil, "", nil, false, err - } - if name != tooldocs.PausePointCapturedVariablesFlagName { - remaining = append(remaining, arg) - continue - } - parsedMode, err := parsePausePointCapturedVariablesMode(value) - if err != nil { - return nil, false, mode, nil, nil, "", nil, false, err - } - mode = parsedMode - modeSet = true - if consumedNext { - index++ - } + if name != flagName { + remaining = append(remaining, arg) continue } - - if isPausePointFlag(arg, tooldocs.PausePointCapturedVariableNamesFlagName) { - name, value, consumedNext, err := clicore.ParseFlagValue(arg, args, index) - if err != nil { - return nil, false, mode, nil, nil, "", nil, false, err - } - if name != tooldocs.PausePointCapturedVariableNamesFlagName { - remaining = append(remaining, arg) - continue - } - capturedVariableNames = parsePausePointCapturedVariableNames(value) - namesSet = true - if consumedNext { - index++ - } - continue + if err := handler.applyValue(&state, value); err != nil { + return nil, false, state.mode, nil, nil, "", nil, false, err } - - if isPausePointFlag(arg, tooldocs.PausePointExpectFlagName) { - name, value, consumedNext, err := clicore.ParseFlagValue(arg, args, index) - if err != nil { - return nil, false, mode, nil, nil, "", nil, false, err - } - if name != tooldocs.PausePointExpectFlagName { - remaining = append(remaining, arg) - continue - } - expectation, parseErr := parsePausePointExpectFlagValue(value) - if parseErr != nil { - return nil, false, mode, nil, nil, "", nil, false, parseErr - } - expectations = append(expectations, expectation) - if consumedNext { - index++ - } - continue + if consumedNext { + index++ } + } - remaining = append(remaining, arg) + if err := pausePointEnableAwaitRequirementError(state); err != nil { + return nil, false, state.mode, nil, nil, "", nil, false, err } - if !await && (modeSet || namesSet || len(expectations) > 0 || triggerSet || resumePlay) { - option := "--" + tooldocs.PausePointCapturedVariablesFlagName - switch { - case resumePlay: - option = "--" + tooldocs.PausePointResumePlayFlagName - case triggerSet: - option = "--" + tooldocs.PausePointTriggerFlagName - case len(expectations) > 0: - option = "--" + tooldocs.PausePointExpectFlagName - case namesSet: - option = "--" + tooldocs.PausePointCapturedVariableNamesFlagName - } - return nil, false, mode, nil, nil, "", nil, false, &clierrors.ArgumentError{ - Message: "--captured-variables, --captured-variable-names, --expect, --trigger, and --resume-play require --await", - Option: option, - Command: pausePointEnableCommandName, - NextActions: []string{ - "Pass `--await` to wait for the marker after enabling, or drop these options.", - }, + return remaining, state.await, state.mode, state.capturedVariableNames, state.expectations, + state.triggerCommand, state.triggerArgs, state.resumePlay, nil +} + +// findPausePointEnableFlagHandler resolves an argv token to the CLI-only flag it names. The match is +// exact or `--flag=`-prefixed, and the flag names share no such prefix, so at most one handler can +// match regardless of map iteration order. +func findPausePointEnableFlagHandler(arg string) (string, pausePointEnableFlagHandler, bool) { + for flagName, handler := range pausePointEnableFlagHandlers { + if isPausePointFlag(arg, flagName) { + return flagName, handler, true } } + return "", pausePointEnableFlagHandler{}, false +} + +// pausePointEnableAwaitRequirementError rejects the orchestration flags when --await was not passed: +// without the wait there is nothing for them to configure. The reported Option follows a fixed +// priority so the message names one concrete flag instead of whichever the parser saw last. +func pausePointEnableAwaitRequirementError(state pausePointEnableFlagState) error { + if state.await { + return nil + } + if !state.modeSet && !state.namesSet && len(state.expectations) == 0 && !state.triggerSet && !state.resumePlay { + return nil + } - return remaining, await, mode, capturedVariableNames, expectations, triggerCommand, triggerArgs, resumePlay, nil + option := "--" + tooldocs.PausePointCapturedVariablesFlagName + switch { + case state.resumePlay: + option = "--" + tooldocs.PausePointResumePlayFlagName + case state.triggerSet: + option = "--" + tooldocs.PausePointTriggerFlagName + case len(state.expectations) > 0: + option = "--" + tooldocs.PausePointExpectFlagName + case state.namesSet: + option = "--" + tooldocs.PausePointCapturedVariableNamesFlagName + } + + return &clierrors.ArgumentError{ + Message: "--captured-variables, --captured-variable-names, --expect, --trigger, and --resume-play require --await", + Option: option, + Command: pausePointEnableCommandName, + NextActions: []string{ + "Pass `--await` to wait for the marker after enabling, or drop these options.", + }, + } } func isPausePointFlag(arg string, flagName string) bool { From c89a3684cf47eb769c6b09b94987f4c78ccaeca3 Mon Sep 17 00:00:00 2001 From: hatayama Date: Mon, 27 Jul 2026 01:24:10 +0900 Subject: [PATCH 8/8] Omit empty-string option defaults from uloop list Unity reports an empty string as the default of every unset string parameter. `--help` already suppresses that value, but list still emitted "Default": "" for 20 options because encoding/json's omitempty cannot drop an empty string held in an interface field. Reporting nil restores the help/list symmetry this change set exists to guarantee. Re-stamps the shared release inputs for the cli/common-adjacent change. --- cli/dispatcher/shared-inputs-stamp.json | 2 +- .../internal/projectrunner/list_output.go | 6 ++++++ .../internal/projectrunner/list_output_test.go | 16 ++++++++++++++++ cli/project-runner/shared-inputs-stamp.json | 2 +- 4 files changed, 24 insertions(+), 2 deletions(-) diff --git a/cli/dispatcher/shared-inputs-stamp.json b/cli/dispatcher/shared-inputs-stamp.json index ad234b6b29..aa7345dc75 100644 --- a/cli/dispatcher/shared-inputs-stamp.json +++ b/cli/dispatcher/shared-inputs-stamp.json @@ -1,4 +1,4 @@ { "schemaVersion": 1, - "sharedInputsHash": "e98f01c7ee2474c3cf7d215d66805e63aa7877d6" + "sharedInputsHash": "1f2aea4c3b64942bc6b25492861c6935620ccd84" } diff --git a/cli/project-runner/internal/projectrunner/list_output.go b/cli/project-runner/internal/projectrunner/list_output.go index ede76b35cd..d9978c9756 100644 --- a/cli/project-runner/internal/projectrunner/list_output.go +++ b/cli/project-runner/internal/projectrunner/list_output.go @@ -109,6 +109,12 @@ func listOptionDefault(property clicore.ToolProperty) any { if enumValue, ok := tooldocs.EnumValueForNumericDefault(defaultValue, property.Enum); ok { return enumValue } + // Unity reports an empty string as the default of every unset string parameter. Reporting it + // would claim the option has a default of "", and omitempty cannot drop it on its own because the + // field is an interface. Nil keeps list symmetric with --help, which omits the same value. + if defaultValue == "" { + return nil + } return defaultValue } diff --git a/cli/project-runner/internal/projectrunner/list_output_test.go b/cli/project-runner/internal/projectrunner/list_output_test.go index 4828368c45..c6d6d50628 100644 --- a/cli/project-runner/internal/projectrunner/list_output_test.go +++ b/cli/project-runner/internal/projectrunner/list_output_test.go @@ -180,6 +180,22 @@ func TestFormatToolListResultFillsPlaceholderDescriptions(t *testing.T) { } } +// Tests that an unset string parameter reports no default at all, matching --help. Unity reports an +// empty string for these, which would otherwise claim the option defaults to "". +func TestNewListCatalogOmitsEmptyStringDefaults(t *testing.T) { + catalog := newListCatalog(clicore.ToolsCache{Tools: []clicore.ToolDefinition{{ + Name: "screenshot", + ParameterSchema: clicore.InputSchema{Properties: map[string]clicore.ToolProperty{ + "OutputPath": {Type: "string", Description: "Where to write the file", DefaultValue: ""}, + }}, + }}}) + + option := findListOption(t, findListTool(t, catalog, "screenshot"), "--output-path") + if option.Default != nil { + t.Errorf("empty-string default was reported: %#v", option.Default) + } +} + func decodeListCatalog(t *testing.T, content []byte) listCatalog { t.Helper() diff --git a/cli/project-runner/shared-inputs-stamp.json b/cli/project-runner/shared-inputs-stamp.json index e8ee8d8193..7025b36f3f 100644 --- a/cli/project-runner/shared-inputs-stamp.json +++ b/cli/project-runner/shared-inputs-stamp.json @@ -1,4 +1,4 @@ { "schemaVersion": 1, - "sharedInputsHash": "b601b60304696ae68084ae9d47bd375ee9c7c09a" + "sharedInputsHash": "cbfdc9ea86e2166a5be8ca466799820ef2aed727" }