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/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/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/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/skill_guidance.go b/cli/common/tooldocs/skill_guidance.go new file mode 100644 index 0000000000..6c01f9cbdb --- /dev/null +++ b/cli/common/tooldocs/skill_guidance.go @@ -0,0 +1,55 @@ +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", + + "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 +// 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. 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 for workflow rules and response fields that --help does not cover.", 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..f818aa06a6 --- /dev/null +++ b/cli/common/tooldocs/skill_guidance_test.go @@ -0,0 +1,126 @@ +package tooldocs + +import ( + "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() + + content, err := os.ReadFile(path) + if err != nil { + t.Fatalf("failed to read %s: %v", path, err) + } + + for _, line := range strings.Split(string(content), "\n") { + if strings.HasPrefix(line, "name:") { + return strings.TrimSpace(strings.TrimPrefix(line, "name:")) + } + } + + 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/common/tooldocs/tool_option_help.go b/cli/common/tooldocs/tool_option_help.go index e72f80a9ff..a1a34e6fcf 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 }) @@ -89,22 +93,27 @@ 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 { - parts = append(parts, "default: "+defaultValueText(propertyDefault)) + // 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 { - parts = append(parts, "values: "+strings.Join(property.Enum, "|")) + parts = append(parts, "values: "+strings.Join(property.Enum, optionValuesSeparator)) } 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) } @@ -142,10 +151,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/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/dispatcher/internal/dispatcher/command_help.go b/cli/dispatcher/internal/dispatcher/command_help.go index 8d6367b938..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,12 +102,27 @@ 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) } } 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..77f2b1a57b 100644 --- a/cli/dispatcher/internal/dispatcher/help_test.go +++ b/cli/dispatcher/internal/dispatcher/help_test.go @@ -253,6 +253,106 @@ 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 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) { + 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/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/dispatcher/shared-inputs-stamp.json b/cli/dispatcher/shared-inputs-stamp.json index e665d3dbb9..aa7345dc75 100644 --- a/cli/dispatcher/shared-inputs-stamp.json +++ b/cli/dispatcher/shared-inputs-stamp.json @@ -1,4 +1,4 @@ { "schemaVersion": 1, - "sharedInputsHash": "8a863e4239e16efaf67ef4b7a2dfa7ed17a36b65" + "sharedInputsHash": "1f2aea4c3b64942bc6b25492861c6935620ccd84" } diff --git a/cli/project-runner/internal/projectrunner/list_output.go b/cli/project-runner/internal/projectrunner/list_output.go index 12525a2fbb..d9978c9756 100644 --- a/cli/project-runner/internal/projectrunner/list_output.go +++ b/cli/project-runner/internal/projectrunner/list_output.go @@ -37,6 +37,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 = clicore.ApplyEmbeddedDescriptionFallback(cache) + content, err := json.Marshal(newListCatalog(cache)) if err != nil { panic(err) @@ -45,9 +50,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 +61,7 @@ func newListCatalog(cache clicore.ToolsCache) listCatalog { Version: clicontract.ProjectRunnerVersion(), ServerVersion: cache.ServerVersion, UpdatedAt: cache.UpdatedAt, - Tools: tools, + Tools: listTools, } } @@ -101,36 +106,16 @@ 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 + // 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 values[index], true + return defaultValue } func appendDynamicCodeFileListOption(tool clicore.ToolDefinition, options []listOption) []listOption { @@ -149,51 +134,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..c6d6d50628 100644 --- a/cli/project-runner/internal/projectrunner/list_output_test.go +++ b/cli/project-runner/internal/projectrunner/list_output_test.go @@ -143,12 +143,57 @@ 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) +} + +// 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) + } +} + +// 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 { diff --git a/cli/project-runner/internal/projectrunner/native_command_help.go b/cli/project-runner/internal/projectrunner/native_command_help.go index fa058fb94b..e2b83e0570 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, }, } @@ -80,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) { 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..57f35940a4 --- /dev/null +++ b/cli/project-runner/internal/projectrunner/pause_point_cli_options_contract_test.go @@ -0,0 +1,120 @@ +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 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) { + 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..c9a7e61202 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,168 +23,186 @@ import ( // extractDynamicCodeFileFlag intercepts --code-file for execute-dynamic-code. const pausePointEnableCommandName = "enable-pause-point" -const pausePointEnableAwaitFlagName = "await" +// 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 == "--"+pausePointEnableAwaitFlagName { - await = true + flagName, handler, ok := findPausePointEnableFlagHandler(arg) + if !ok { + remaining = append(remaining, arg) continue } - if arg == "--"+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, 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 { - 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)") - } - resumePlay = true - if consumedNext { - index++ - } + if handler.applyValue == nil { + remaining = append(remaining, arg) continue } - if isPausePointFlag(arg, 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 { - 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, 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 { - 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, 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 { - 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, 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 { - 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 := "--" + PausePointCapturedVariablesFlagName - switch { - case resumePlay: - option = "--" + PausePointResumePlayFlagName - case triggerSet: - option = "--" + PausePointTriggerFlagName - case len(expectations) > 0: - option = "--" + PausePointExpectFlagName - case namesSet: - option = "--" + 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 { 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) diff --git a/cli/project-runner/shared-inputs-stamp.json b/cli/project-runner/shared-inputs-stamp.json index b6cd063854..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": "962315b78bf904764fa1f75bbd7d6d2ca9ffe092" + "sharedInputsHash": "cbfdc9ea86e2166a5be8ca466799820ef2aed727" }