diff --git a/pkg/mcp/mcp.go b/pkg/mcp/mcp.go index 10dcc7a7da..313c96691d 100644 --- a/pkg/mcp/mcp.go +++ b/pkg/mcp/mcp.go @@ -114,7 +114,7 @@ func New(options ...Option) *Server { // Help // A resource for each command which returns its help // eg. "config volumes add" -> "func://help/config/volumes/add") - i.AddResource(newHelpResource(s, "Help", "help for the command root", "")) + i.AddResource(newHelpResource(s, "Help", "help for the command root")) i.AddResource(newHelpResource(s, "Create Help", "help for 'create'", "create")) i.AddResource(newHelpResource(s, "Build Help", "help for 'build'", "build")) i.AddResource(newHelpResource(s, "Deploy Help", "help for 'deploy'", "deploy")) @@ -151,12 +151,19 @@ type defaultExecutor struct { } func (e defaultExecutor) Execute(ctx context.Context, subcommand string, args ...string) ([]byte, error) { - // Parse prefix: "func" or "kn func" -> ["func"] or ["kn", "func"] - cmdParts := strings.Fields(e.s.prefix) - cmdParts = append(cmdParts, subcommand) - cmdParts = append(cmdParts, args...) - + cmdParts := buildArgs(e.s.prefix, subcommand, args) cmd := exec.CommandContext(ctx, cmdParts[0], cmdParts[1:]...) // cmd.Dir not set - inherits process working directory which is the current working directory return cmd.CombinedOutput() } + +// buildArgs constructs the ordered argument list for execution. +// An empty subcommand is omitted so that commands like "func --help" are +// built correctly rather than "func --help" with a spurious empty argument. +func buildArgs(prefix, subcommand string, args []string) []string { + parts := strings.Fields(prefix) + if subcommand != "" { + parts = append(parts, subcommand) + } + return append(parts, args...) +} diff --git a/pkg/mcp/mcp_test.go b/pkg/mcp/mcp_test.go index 95a79d9ad5..67a4fd22c2 100644 --- a/pkg/mcp/mcp_test.go +++ b/pkg/mcp/mcp_test.go @@ -118,3 +118,65 @@ func newTestPairWithReadonly(t *testing.T, readonly bool) (*mcp.ClientSession, * func newTestPair(t *testing.T, options ...Option) (session *mcp.ClientSession, server *Server, err error) { return newTestPairCore(t, false, options...) } + +// TestBuildArgs verifies that buildArgs constructs the command argument list +// correctly, and in particular that an empty subcommand is never injected. +func TestBuildArgs(t *testing.T) { + cases := []struct { + name string + prefix string + subcommand string + args []string + want []string + }{ + { + name: "empty subcommand is omitted", + prefix: "func", + subcommand: "", + args: []string{"create", "--help"}, + want: []string{"func", "create", "--help"}, + }, + { + name: "non-empty subcommand is included", + prefix: "func", + subcommand: "deploy", + args: []string{"--verbose"}, + want: []string{"func", "deploy", "--verbose"}, + }, + { + name: "multi-word prefix is split correctly", + prefix: "kn func", + subcommand: "build", + args: []string{"--push"}, + want: []string{"kn", "func", "build", "--push"}, + }, + { + name: "multi-word prefix with empty subcommand", + prefix: "kn func", + subcommand: "", + args: []string{"list", "--help"}, + want: []string{"kn", "func", "list", "--help"}, + }, + { + name: "no args", + prefix: "func", + subcommand: "list", + args: nil, + want: []string{"func", "list"}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := buildArgs(tc.prefix, tc.subcommand, tc.args) + if len(got) != len(tc.want) { + t.Fatalf("buildArgs(%q, %q, %v) = %v, want %v", tc.prefix, tc.subcommand, tc.args, got, tc.want) + } + for i := range got { + if got[i] != tc.want[i] { + t.Errorf("buildArgs result[%d] = %q, want %q (full: %v)", i, got[i], tc.want[i], got) + } + } + }) + } +} diff --git a/pkg/mcp/resources_test.go b/pkg/mcp/resources_test.go index f3850f0980..d792967c0a 100644 --- a/pkg/mcp/resources_test.go +++ b/pkg/mcp/resources_test.go @@ -84,6 +84,125 @@ func TestResource_FunctionState(t *testing.T) { } } +// TestResource_Help verifies that every help resource handler invokes the +// executor with an empty subcommand and the correct command arguments ending +// with "--help". This is the regression test for the bug where the empty +// subcommand was unconditionally appended, injecting a spurious "" argument +// into every help command. +func TestResource_Help(t *testing.T) { + cases := []struct { + name string + uri string + wantArgs []string // arguments expected after the empty subcommand + }{ + { + name: "root help", + uri: "func://help", + wantArgs: []string{"--help"}, + }, + { + name: "create help", + uri: "func://help/create", + wantArgs: []string{"create", "--help"}, + }, + { + name: "build help", + uri: "func://help/build", + wantArgs: []string{"build", "--help"}, + }, + { + name: "deploy help", + uri: "func://help/deploy", + wantArgs: []string{"deploy", "--help"}, + }, + { + name: "list help", + uri: "func://help/list", + wantArgs: []string{"list", "--help"}, + }, + { + name: "config volumes help", + uri: "func://help/config/volumes", + wantArgs: []string{"config", "volumes", "--help"}, + }, + { + name: "config volumes add help", + uri: "func://help/config/volumes/add", + wantArgs: []string{"config", "volumes", "add", "--help"}, + }, + { + name: "config labels help", + uri: "func://help/config/labels", + wantArgs: []string{"config", "labels", "--help"}, + }, + { + name: "config labels add help", + uri: "func://help/config/labels/add", + wantArgs: []string{"config", "labels", "add", "--help"}, + }, + { + name: "config envs help", + uri: "func://help/config/envs", + wantArgs: []string{"config", "envs", "--help"}, + }, + { + name: "config envs add help", + uri: "func://help/config/envs/add", + wantArgs: []string{"config", "envs", "add", "--help"}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + const helpOutput = "Usage: func ...\n" + + executor := mock.NewExecutor() + executor.ExecuteFn = func(_ context.Context, subcommand string, args ...string) ([]byte, error) { + // subcommand must always be empty: help resources encode the + // command path entirely in args, not in the subcommand field. + if subcommand != "" { + t.Errorf("expected empty subcommand, got %q", subcommand) + } + // No arg may be an empty string — that was the original bug. + for i, a := range args { + if a == "" { + t.Errorf("args[%d] is an empty string (full args: %v)", i, args) + } + } + if len(args) != len(tc.wantArgs) { + t.Fatalf("expected args %v, got %v", tc.wantArgs, args) + } + for i := range args { + if args[i] != tc.wantArgs[i] { + t.Errorf("args[%d] = %q, want %q", i, args[i], tc.wantArgs[i]) + } + } + return []byte(helpOutput), nil + } + + client, _, err := newTestPair(t, WithExecutor(executor)) + if err != nil { + t.Fatal(err) + } + + result, err := client.ReadResource(t.Context(), &mcp.ReadResourceParams{URI: tc.uri}) + if err != nil { + t.Fatalf("ReadResource(%q): %v", tc.uri, err) + } + + if len(result.Contents) != 1 { + t.Fatalf("expected 1 content, got %d", len(result.Contents)) + } + if result.Contents[0].Text != helpOutput { + t.Errorf("expected output %q, got %q", helpOutput, result.Contents[0].Text) + } + if !executor.ExecuteInvoked { + t.Fatal("executor was not invoked") + } + }) + } +} + func TestResource_Listings(t *testing.T) { cases := []struct { name string