From d254a1971b03705e787578c0085568fa51af5dc8 Mon Sep 17 00:00:00 2001 From: Nolan Date: Tue, 25 Aug 2026 18:36:42 +0800 Subject: [PATCH] fix(cli): keep legacy hidden selectors valid for advertise-required inputs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since #2004, Registry.Invoke validates builtin commands against their advertised schema before the handler runs. Three agentcontext commands mark agent-id as advertise-required, so the schema lists it in required while the daemon binder deliberately treats it as optional: resolveAgentSelector accepts exactly one of agent-id or the hidden deprecated provider. The invocation validator therefore rejected the supported legacy spelling outright, and passing both selectors tripped the binder's exactly-one guard — no valid spelling was left. AdvertisedRequired keeps the advertised contract strict while allowing a hidden compatibility selector to satisfy runtime validation, so say so in the schema itself: a new advertise-alt tag names the hidden fields that can stand in, and Schema renders one anyOf branch per accepted spelling (typed object branches, which the invocation validator already supports) instead of a hard required entry. One branch per name matters: a single branch listing both names would require both at once. Fields without an alternate keep rendering exactly as before. The advertised capability contract for the three commands now states the real constraint — one of agent-id or the deprecated provider — which is also what external schema consumers should see. Fixes #2193 Signed-off-by: Nolan --- .../service/cli/framework/framework_test.go | 52 ++++++++++++ .../tuttid/service/cli/framework/input.go | 38 +++++++-- services/tuttid/service/cli/framework/spec.go | 17 ++-- .../agentcontext/composer_options.go | 2 +- .../providers/agentcontext/provider_test.go | 80 ++++++++++++++++++- .../agentcontext/session_commands.go | 2 +- .../providers/agentcontext/skill_bundle.go | 2 +- services/tuttid/service/cli/registry_test.go | 51 ++++++++++++ 8 files changed, 225 insertions(+), 19 deletions(-) diff --git a/services/tuttid/service/cli/framework/framework_test.go b/services/tuttid/service/cli/framework/framework_test.go index eb3c23ed5f..f63f5768a0 100644 --- a/services/tuttid/service/cli/framework/framework_test.go +++ b/services/tuttid/service/cli/framework/framework_test.go @@ -360,3 +360,55 @@ func (f fakeWorkspaceCatalog) Startup(context.Context) (*workspacebiz.Summary, e func (fakeWorkspaceCatalog) Get(_ context.Context, workspaceID string) (workspacebiz.Summary, error) { return workspacebiz.Summary{ID: workspaceID}, nil } + +// Regression test for #2193: an advertise-required field with a hidden +// alternate must render as an anyOf branch instead of a hard required entry. +func TestSchemaRendersAnyOfForAdvertisedRequiredWithAlternate(t *testing.T) { + type input struct { + AgentID string `cli:"agent-id" advertise-required:"true" advertise-alt:"provider" hint:"Use agent list --json to discover available agents."` + Prompt string `cli:"prompt" validate:"required"` + Provider string `cli:"provider" hidden:"true"` + } + schema := Schema(FromStruct[input]()) + + required, _ := schema["required"].([]string) + if len(required) != 1 || required[0] != "prompt" { + t.Fatalf("schema required = %#v, want only prompt", required) + } + branches, ok := schema["anyOf"].([]map[string]any) + if !ok || len(branches) != 2 { + t.Fatalf("schema anyOf = %#v, want one branch per accepted spelling", schema["anyOf"]) + } + first, _ := branches[0]["required"].([]string) + second, _ := branches[1]["required"].([]string) + if len(first) != 1 || first[0] != "agent-id" || len(second) != 1 || second[0] != "provider" { + t.Fatalf("anyOf branches required = [%#v %#v], want [[agent-id] [provider]]", first, second) + } + for _, branch := range branches { + if branch["type"] != "object" { + t.Fatalf("anyOf branch missing object type: %#v", branch) + } + } + properties := schema["properties"].(map[string]any) + if _, exists := properties["provider"]; exists { + t.Fatal("hidden provider field leaked into advertised properties") + } +} + +// Without an alternate, an advertise-required field keeps rendering as a hard +// required entry (existing advertised contracts unchanged). +func TestSchemaKeepsHardRequiredForAdvertisedRequiredWithoutAlternate(t *testing.T) { + type input struct { + AgentID string `cli:"agent-id" advertise-required:"true"` + Prompt string `cli:"prompt"` + } + schema := Schema(FromStruct[input]()) + + required, _ := schema["required"].([]string) + if len(required) != 1 || required[0] != "agent-id" { + t.Fatalf("schema required = %#v, want [agent-id]", required) + } + if _, exists := schema["anyOf"]; exists { + t.Fatalf("schema anyOf = %#v, want absent", schema["anyOf"]) + } +} diff --git a/services/tuttid/service/cli/framework/input.go b/services/tuttid/service/cli/framework/input.go index d67f43dc07..68fb7710d3 100644 --- a/services/tuttid/service/cli/framework/input.go +++ b/services/tuttid/service/cli/framework/input.go @@ -35,13 +35,14 @@ func FromStruct[T any]() InputSpec { name = kebabCase(field.Name) } fieldSpec := FieldSpec{ - Name: name, - Type: schemaType(field.Type), - Description: strings.TrimSpace(field.Tag.Get("description")), - Hidden: field.Tag.Get("hidden") == "true", - AdvertisedRequired: field.Tag.Get("advertise-required") == "true", - Hint: strings.TrimSpace(field.Tag.Get("hint")), - Default: typedDefault(field.Type, field.Tag.Get("default")), + Name: name, + Type: schemaType(field.Type), + Description: strings.TrimSpace(field.Tag.Get("description")), + Hidden: field.Tag.Get("hidden") == "true", + AdvertisedRequired: field.Tag.Get("advertise-required") == "true", + AdvertiseAlternates: parseCSVTag(field.Tag.Get("advertise-alt")), + Hint: strings.TrimSpace(field.Tag.Get("hint")), + Default: typedDefault(field.Type, field.Tag.Get("default")), } applyValidateTag(&fieldSpec, field.Tag.Get("validate")) fieldSpec.Enum = parseCSVTag(field.Tag.Get("enum")) @@ -58,6 +59,7 @@ func Schema(input InputSpec) map[string]any { } properties := schema["properties"].(map[string]any) required := []string{} + var alternatives []map[string]any for _, field := range input.Fields { if field.Hidden { continue @@ -86,13 +88,33 @@ func Schema(input InputSpec) map[string]any { property["default"] = field.Default } properties[field.Name] = property - if field.Required || field.AdvertisedRequired { + switch { + case field.Required: + required = append(required, field.Name) + case field.AdvertisedRequired && len(field.AdvertiseAlternates) > 0: + // The advertised contract keeps this field required, but a hidden + // compatibility selector can satisfy the handler instead. Emit one + // anyOf branch per accepted spelling so the invocation validator + // accepts any one of them (issue #2193). A single branch listing + // every name would require all of them at once. + for _, name := range append([]string{field.Name}, field.AdvertiseAlternates...) { + alternatives = append(alternatives, map[string]any{ + // The invocation validator only applies "required" to + // branches that declare an object type. + "type": "object", + "required": []string{name}, + }) + } + case field.AdvertisedRequired: required = append(required, field.Name) } } if len(required) > 0 { schema["required"] = required } + if len(alternatives) > 0 { + schema["anyOf"] = alternatives + } return schema } diff --git a/services/tuttid/service/cli/framework/spec.go b/services/tuttid/service/cli/framework/spec.go index 5999572f4d..fcc2fb1e0b 100644 --- a/services/tuttid/service/cli/framework/spec.go +++ b/services/tuttid/service/cli/framework/spec.go @@ -58,12 +58,17 @@ type FieldSpec struct { // AdvertisedRequired keeps the canonical capability contract strict while // allowing a hidden compatibility selector to satisfy runtime validation. AdvertisedRequired bool - Required bool - Hint string - Min *int64 - Max *int64 - Enum []string - Default any + // AdvertiseAlternates names hidden input fields that can satisfy an + // AdvertisedRequired field at runtime (e.g. the deprecated --provider + // selector standing in for --agent-id). Schema() expresses this as an + // anyOf branch so invocation validation accepts either spelling. + AdvertiseAlternates []string + Required bool + Hint string + Min *int64 + Max *int64 + Enum []string + Default any } type InputSpec struct { diff --git a/services/tuttid/service/cli/providers/agentcontext/composer_options.go b/services/tuttid/service/cli/providers/agentcontext/composer_options.go index bbeeb8fca7..cd11ccff1c 100644 --- a/services/tuttid/service/cli/providers/agentcontext/composer_options.go +++ b/services/tuttid/service/cli/providers/agentcontext/composer_options.go @@ -9,7 +9,7 @@ import ( ) type composerOptionsInput struct { - AgentID string `cli:"agent-id" advertise-required:"true" hint:"Use agent list --json to discover available agents."` + AgentID string `cli:"agent-id" advertise-required:"true" advertise-alt:"provider" hint:"Use agent list --json to discover available agents."` Cwd string `cli:"cwd"` Locale string `cli:"locale"` Model string `cli:"model"` diff --git a/services/tuttid/service/cli/providers/agentcontext/provider_test.go b/services/tuttid/service/cli/providers/agentcontext/provider_test.go index b2a9ad4dfe..9bec750a45 100644 --- a/services/tuttid/service/cli/providers/agentcontext/provider_test.go +++ b/services/tuttid/service/cli/providers/agentcontext/provider_test.go @@ -1099,9 +1099,21 @@ func TestStartCommandRequiresOneSelectorAndPrompt(t *testing.T) { if !ok { t.Fatalf("required schema = %#v", command.Capability.InputSchema["required"]) } - if len(required) != 2 || required[0] != "agent-id" || required[1] != "prompt" { + if len(required) != 1 || required[0] != "prompt" { t.Fatalf("required = %#v", required) } + // agent-id is advertise-required with the hidden legacy --provider as its + // alternate, so it renders as an anyOf branch rather than a hard required + // entry (issue #2193). + branches, ok := command.Capability.InputSchema["anyOf"].([]map[string]any) + if !ok || len(branches) != 2 { + t.Fatalf("anyOf schema = %#v, want one branch per accepted spelling", command.Capability.InputSchema["anyOf"]) + } + first, _ := branches[0]["required"].([]string) + second, _ := branches[1]["required"].([]string) + if len(first) != 1 || first[0] != "agent-id" || len(second) != 1 || second[0] != "provider" { + t.Fatalf("anyOf branches required = [%#v %#v], want [[agent-id] [provider]]", first, second) + } for name, input := range map[string]map[string]any{ "missing agent id": {"model": "gpt-5", "prompt": "do work"}, @@ -1943,9 +1955,21 @@ func TestAgentStartCommandAllowsOmittedModel(t *testing.T) { if !ok { t.Fatalf("required schema = %#v", command.Capability.InputSchema["required"]) } - if len(required) != 2 || required[0] != "agent-id" || required[1] != "prompt" { + if len(required) != 1 || required[0] != "prompt" { t.Fatalf("required = %#v", required) } + // agent-id is advertise-required with the hidden legacy --provider as its + // alternate, so it renders as an anyOf branch rather than a hard required + // entry (issue #2193). + branches, ok := command.Capability.InputSchema["anyOf"].([]map[string]any) + if !ok || len(branches) != 2 { + t.Fatalf("anyOf schema = %#v, want one branch per accepted spelling", command.Capability.InputSchema["anyOf"]) + } + first, _ := branches[0]["required"].([]string) + second, _ := branches[1]["required"].([]string) + if len(first) != 1 || first[0] != "agent-id" || len(second) != 1 || second[0] != "provider" { + t.Fatalf("anyOf branches required = [%#v %#v], want [[agent-id] [provider]]", first, second) + } _, err := command.Handler(context.Background(), cliservice.InvokeRequest{Input: map[string]any{"model": "gpt-5"}}) if !errors.Is(err, cliservice.ErrInvalidInput) { t.Fatalf("err = %v, want ErrInvalidInput", err) @@ -2605,3 +2629,55 @@ func TestActivePeersReturnsServiceProjection(t *testing.T) { t.Fatalf("output = %#v", output.Value) } } + +// End-to-end regression for #2193: the schema the framework generates for the +// real start command must let the legacy hidden --provider selector through +// the daemon's invocation validator. This exercises the generator output, not +// a hand-written schema. +func TestStartCommandSchemaPassesInvocationValidationForLegacySelector(t *testing.T) { + command := newTestProvider(fakeWorkspaceCatalog{startup: workspacebiz.Summary{ID: "workspace-1"}}, &fakeAgentSessions{}).newStartCommand() + + registry, err := cliservice.NewRegistryFromProviders(singleCommandProvider{command: command}) + if err != nil { + t.Fatalf("NewRegistryFromProviders: %v", err) + } + + // The pre-binding validator must accept both spellings and reject neither. + cases := []struct { + name string + input map[string]any + wantErr bool + }{ + {"canonical agent-id", map[string]any{"agent-id": agenttargetbiz.IDLocalCodex, "prompt": "hi"}, false}, + {"legacy provider", map[string]any{"provider": "codex", "prompt": "hi"}, false}, + {"no selector", map[string]any{"prompt": "hi"}, true}, + } + for _, tc := range cases { + _, err := registry.Invoke(context.Background(), cliservice.InvokeRequest{ + CommandID: command.Capability.ID, + Input: tc.input, + }) + if tc.wantErr && err == nil { + t.Fatalf("%s: accepted, want rejection", tc.name) + } + if !tc.wantErr { + // The handler may still fail (fake catalog lookups); what matters + // is that the invocation validator did not reject the input. + if err != nil && strings.Contains(err.Error(), "does not match any allowed schema") { + t.Fatalf("%s: rejected by invocation validator: %v", tc.name, err) + } + } + } +} + +type singleCommandProvider struct { + command cliservice.Command +} + +func (p singleCommandProvider) AppID() string { + return "agentcontext-test" +} + +func (p singleCommandProvider) Commands() []cliservice.Command { + return []cliservice.Command{p.command} +} diff --git a/services/tuttid/service/cli/providers/agentcontext/session_commands.go b/services/tuttid/service/cli/providers/agentcontext/session_commands.go index 7c078b5a9d..ec265f761c 100644 --- a/services/tuttid/service/cli/providers/agentcontext/session_commands.go +++ b/services/tuttid/service/cli/providers/agentcontext/session_commands.go @@ -28,7 +28,7 @@ var sessionActionColumns = []cliservice.TableColumn{ } type startInput struct { - AgentID string `cli:"agent-id" advertise-required:"true" hint:"Use agent list --json to discover available agents."` + AgentID string `cli:"agent-id" advertise-required:"true" advertise-alt:"provider" hint:"Use agent list --json to discover available agents."` Cwd string `cli:"cwd"` DisplayPrompt string `cli:"display-prompt"` Hidden bool `cli:"hidden"` diff --git a/services/tuttid/service/cli/providers/agentcontext/skill_bundle.go b/services/tuttid/service/cli/providers/agentcontext/skill_bundle.go index e3b98af7a9..278df29fcb 100644 --- a/services/tuttid/service/cli/providers/agentcontext/skill_bundle.go +++ b/services/tuttid/service/cli/providers/agentcontext/skill_bundle.go @@ -9,7 +9,7 @@ import ( ) type skillBundleInput struct { - AgentID string `cli:"agent-id" advertise-required:"true" hint:"Use agent list --json to discover available agents."` + AgentID string `cli:"agent-id" advertise-required:"true" advertise-alt:"provider" hint:"Use agent list --json to discover available agents."` AgentSessionID string `cli:"agent-session-id"` BrowserUse bool `cli:"browser-use"` ComputerUse bool `cli:"computer-use"` diff --git a/services/tuttid/service/cli/registry_test.go b/services/tuttid/service/cli/registry_test.go index 1cb452dc1c..e496e8fb95 100644 --- a/services/tuttid/service/cli/registry_test.go +++ b/services/tuttid/service/cli/registry_test.go @@ -411,3 +411,54 @@ func testCommandWithPath(id string, path []string) Command { }, } } + +// Regression test for #2193: an AdvertisedRequired field whose runtime +// requirement can be satisfied by a hidden compatibility selector must not +// reject the legacy spelling at invocation. Schema() expresses the selector as +// an anyOf branch, so validation accepts either --agent-id or --provider. +func TestRegistryInvokeAcceptsLegacyAlternateForAdvertisedRequired(t *testing.T) { + invoked := false + command := testCommand("agentcontext.agent.start") + command.Capability.InputSchema = map[string]any{ + "type": "object", + "properties": map[string]any{ + "agent-id": map[string]any{"type": "string"}, + "prompt": map[string]any{"type": "string"}, + }, + "required": []string{"prompt"}, + "anyOf": []any{ + map[string]any{"type": "object", "required": []string{"agent-id"}}, + map[string]any{"type": "object", "required": []string{"provider"}}, + }, + } + command.Handler = func(context.Context, InvokeRequest) (CommandOutput, error) { + invoked = true + return CommandOutput{Kind: OutputModePlain, Text: "started"}, nil + } + registry := newTestRegistry(t, command) + + // Canonical spelling. + if _, err := registry.Invoke(context.Background(), InvokeRequest{ + CommandID: "agentcontext.agent.start", + Input: map[string]any{"agent-id": "agent-1", "prompt": "hi"}, + }); err != nil { + t.Fatalf("Invoke(agent-id): %v", err) + } + // Legacy hidden selector, the path broken since #2004. + if _, err := registry.Invoke(context.Background(), InvokeRequest{ + CommandID: "agentcontext.agent.start", + Input: map[string]any{"provider": "claude", "prompt": "hi"}, + }); err != nil { + t.Fatalf("Invoke(provider): %v", err) + } + // Neither spelling present is still rejected. + if _, err := registry.Invoke(context.Background(), InvokeRequest{ + CommandID: "agentcontext.agent.start", + Input: map[string]any{"prompt": "hi"}, + }); err == nil { + t.Fatal("Invoke(no selector): want error, got nil") + } + if !invoked { + t.Fatal("handler was not invoked for valid inputs") + } +}