diff --git a/cmd/mcp.go b/cmd/mcp.go index 391817001c..3d5d23097b 100644 --- a/cmd/mcp.go +++ b/cmd/mcp.go @@ -110,11 +110,18 @@ func runMCPStart(cmd *cobra.Command, args []string, newClient ClientFactory) err rootCmd := cmd.Root() cmdPrefix := rootCmd.Use - // Instantiate - client, done := newClient(ClientConfig{}, - fn.WithMCPServer(mcp.New( - mcp.WithPrefix(cmdPrefix), - mcp.WithReadonly(!writeEnabled)))) + // Instantiate. The MCP server needs a *fn.Client for tool handlers that + // call pkg/functions directly (see issue #3771). We capture the client + // via a closure so the MCP server can look it up lazily after the client + // is constructed below. + var client *fn.Client + mcpServer := mcp.New( + mcp.WithPrefix(cmdPrefix), + mcp.WithReadonly(!writeEnabled), + mcp.WithClientProvider(func() *fn.Client { return client }), + ) + var done func() + client, done = newClient(ClientConfig{}, fn.WithMCPServer(mcpServer)) defer done() // Start diff --git a/pkg/mcp/mcp.go b/pkg/mcp/mcp.go index 02ea0f5d44..cb3bab1d6e 100644 --- a/pkg/mcp/mcp.go +++ b/pkg/mcp/mcp.go @@ -8,6 +8,7 @@ import ( "sync/atomic" "github.com/modelcontextprotocol/go-sdk/mcp" + fn "knative.dev/func/pkg/functions" ) const ( @@ -28,6 +29,11 @@ type Server struct { executor executor transport mcp.Transport // Transport to use (defaults to StdioTransport) impl *mcp.Server // implements the protocol + + // clientProvider returns a *fn.Client for direct library calls. + // Set via WithClientProvider; nil for tool handlers that still shell out. + // Tracks the migration in https://github.com/knative/func/issues/3771. + clientProvider func() *fn.Client } type executor interface { @@ -68,6 +74,16 @@ func WithTransport(transport mcp.Transport) Option { } } +// WithClientProvider sets a provider that returns the functions Client used +// for direct library calls (replacing the executor subprocess path). +// The provider is called lazily per tool invocation so it can capture a +// client that is constructed after the MCP server. +func WithClientProvider(p func() *fn.Client) Option { + return func(s *Server) { + s.clientProvider = p + } +} + // WithReadonly sets the server to readonly mode. func WithReadonly(readonly bool) Option { return func(s *Server) { diff --git a/pkg/mcp/tools_list.go b/pkg/mcp/tools_list.go index 4f8b0ec6ba..a9ae814bdf 100644 --- a/pkg/mcp/tools_list.go +++ b/pkg/mcp/tools_list.go @@ -3,8 +3,11 @@ package mcp import ( "context" "fmt" + "strings" + "text/tabwriter" "github.com/modelcontextprotocol/go-sdk/mcp" + fn "knative.dev/func/pkg/functions" ) var listTool = &mcp.Tool{ @@ -14,42 +17,71 @@ var listTool = &mcp.Tool{ Annotations: &mcp.ToolAnnotations{ Title: "List Functions", ReadOnlyHint: true, - IdempotentHint: true, // Listing functions with the same parameters multiple times returns consistent results at any point in time. + IdempotentHint: true, }, } +// listHandler calls pkg/functions.Client.List directly rather than shelling +// out to `func list`. Part of the migration tracked in +// https://github.com/knative/func/issues/3771. func (s *Server) listHandler(ctx context.Context, r *mcp.CallToolRequest, input ListInput) (result *mcp.CallToolResult, output ListOutput, err error) { - out, err := s.executor.Execute(ctx, "list", input.Args()...) + if s.clientProvider == nil { + err = fmt.Errorf("list tool requires a configured client provider") + return + } + client := s.clientProvider() + if client == nil { + err = fmt.Errorf("list tool: client provider returned nil") + return + } + + namespace := "" + if input.Namespace != nil { + namespace = *input.Namespace + } + if input.AllNamespaces != nil && *input.AllNamespaces { + namespace = "" + } + + items, err := client.List(ctx, namespace) if err != nil { - err = fmt.Errorf("%w\n%s", err, string(out)) return } + output = ListOutput{ - Message: string(out), + Functions: items, + Message: formatListItems(items, namespace), } return } // ListInput defines the input parameters for the list tool. -// All fields are optional since list can work without any parameters. type ListInput struct { AllNamespaces *bool `json:"allNamespaces,omitempty" jsonschema:"List functions in all namespaces (overrides namespace parameter)"` - Namespace *string `json:"namespace,omitempty" jsonschema:"Kubernetes namespace to list functions in (default: current namespace)"` - Output *string `json:"output,omitempty" jsonschema:"Output format: human, plain, json, xml, or yaml"` - Verbose *bool `json:"verbose,omitempty" jsonschema:"Enable verbose logging output"` -} - -func (i ListInput) Args() []string { - args := []string{} - - args = appendBoolFlag(args, "--all-namespaces", i.AllNamespaces) - args = appendStringFlag(args, "--namespace", i.Namespace) - args = appendStringFlag(args, "--output", i.Output) - args = appendBoolFlag(args, "--verbose", i.Verbose) - return args + Namespace *string `json:"namespace,omitempty" jsonschema:"Kubernetes namespace to list functions in (empty: all namespaces)"` } // ListOutput defines the structured output returned by the list tool. +// Functions is the canonical machine-readable result; Message is a human +// summary for fallback display. type ListOutput struct { - Message string `json:"message" jsonschema:"Output message"` + Functions []fn.ListItem `json:"functions" jsonschema:"Deployed functions"` + Message string `json:"message" jsonschema:"Human-readable summary"` +} + +func formatListItems(items []fn.ListItem, namespace string) string { + if len(items) == 0 { + if namespace != "" { + return fmt.Sprintf("no functions found in namespace %q", namespace) + } + return "no functions found" + } + var b strings.Builder + w := tabwriter.NewWriter(&b, 0, 8, 2, ' ', 0) + fmt.Fprintf(w, "%s\t%s\t%s\t%s\t%s\t%s\n", "NAME", "NAMESPACE", "RUNTIME", "DEPLOYER", "URL", "READY") + for _, it := range items { + fmt.Fprintf(w, "%s\t%s\t%s\t%s\t%s\t%s\n", it.Name, it.Namespace, it.Runtime, it.Deployer, it.URL, it.Ready) + } + w.Flush() + return b.String() } diff --git a/pkg/mcp/tools_list_test.go b/pkg/mcp/tools_list_test.go index 54caf014fe..202a95c9f7 100644 --- a/pkg/mcp/tools_list_test.go +++ b/pkg/mcp/tools_list_test.go @@ -2,54 +2,89 @@ package mcp import ( "context" + "encoding/json" "testing" "github.com/modelcontextprotocol/go-sdk/mcp" - "knative.dev/func/pkg/mcp/mock" + fn "knative.dev/func/pkg/functions" + "knative.dev/func/pkg/mock" ) -// TestTool_List_Args ensures the list tool executes with all arguments passed correctly. -func TestTool_List_Args(t *testing.T) { - // Test data - defined once and used for both input and validation - stringFlags := map[string]struct { - jsonKey string - flag string - value string - }{ - "namespace": {"namespace", "--namespace", "prod"}, - "output": {"output", "--output", "json"}, +// TestTool_List_DirectCall verifies the list tool calls pkg/functions.Client.List +// directly (no subprocess) and returns the items as structured output. +func TestTool_List_DirectCall(t *testing.T) { + lister := mock.NewLister() + lister.ListFn = func(_ context.Context, namespace string) ([]fn.ListItem, error) { + if namespace != "prod" { + t.Fatalf("expected namespace 'prod', got %q", namespace) + } + return []fn.ListItem{ + {Name: "my-func", Namespace: "prod", Runtime: "go", Deployer: "knative", URL: "http://my-func.prod.example.com", Ready: "True"}, + }, nil } - boolFlags := map[string]string{ - "allNamespaces": "--all-namespaces", - "verbose": "--verbose", + fnClient := fn.New(fn.WithListers(lister)) + client, _, err := newTestPair(t, WithClientProvider(func() *fn.Client { return fnClient })) + if err != nil { + t.Fatal(err) } - executor := mock.NewExecutor() - executor.ExecuteFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, error) { - if subcommand != "list" { - t.Fatalf("expected subcommand 'list', got %q", subcommand) - } + ns := "prod" + result, err := client.CallTool(t.Context(), &mcp.CallToolParams{ + Name: "list", + Arguments: map[string]any{"namespace": ns}, + }) + if err != nil { + t.Fatal(err) + } + if result.IsError { + t.Fatalf("unexpected error result: %v", result) + } + if !lister.ListInvoked { + t.Fatal("lister was not invoked — handler did not call pkg/functions directly") + } - validateArgLength(t, args, len(stringFlags), len(boolFlags)) - validateStringFlags(t, args, stringFlags) - validateBoolFlags(t, args, boolFlags) + // StructuredContent is the canonical channel for #3770/#3771. + if result.StructuredContent == nil { + t.Fatal("expected StructuredContent to be populated") + } + raw, err := json.Marshal(result.StructuredContent) + if err != nil { + t.Fatalf("marshal structured content: %v", err) + } + var out ListOutput + if err := json.Unmarshal(raw, &out); err != nil { + t.Fatalf("unmarshal ListOutput: %v", err) + } + if len(out.Functions) != 1 || out.Functions[0].Name != "my-func" { + t.Fatalf("unexpected functions in output: %+v", out.Functions) + } +} - return []byte("NAME\tNAMESPACE\tRUNTIME\nmy-func\tprod\tgo\n"), nil +// TestTool_List_AllNamespaces verifies that AllNamespaces overrides any +// supplied namespace and results in an empty namespace passed to the lister +// (which the lister contract treats as "all namespaces"). +func TestTool_List_AllNamespaces(t *testing.T) { + lister := mock.NewLister() + lister.ListFn = func(_ context.Context, namespace string) ([]fn.ListItem, error) { + if namespace != "" { + t.Fatalf("expected empty namespace when AllNamespaces=true, got %q", namespace) + } + return []fn.ListItem{}, nil } - client, _, err := newTestPair(t, WithExecutor(executor)) + fnClient := fn.New(fn.WithListers(lister)) + client, _, err := newTestPair(t, WithClientProvider(func() *fn.Client { return fnClient })) if err != nil { t.Fatal(err) } - // Build input arguments from test data - inputArgs := buildInputArgs(stringFlags, boolFlags) - - // Invoke tool with all optional arguments result, err := client.CallTool(t.Context(), &mcp.CallToolParams{ - Name: "list", - Arguments: inputArgs, + Name: "list", + Arguments: map[string]any{ + "allNamespaces": true, + "namespace": "ignored", + }, }) if err != nil { t.Fatal(err) @@ -57,7 +92,26 @@ func TestTool_List_Args(t *testing.T) { if result.IsError { t.Fatalf("unexpected error result: %v", result) } - if !executor.ExecuteInvoked { - t.Fatal("executor was not invoked") + if !lister.ListInvoked { + t.Fatal("lister was not invoked") + } +} + +// TestTool_List_RequiresClientProvider ensures the handler returns an error +// (rather than panicking) when no client provider was configured. +func TestTool_List_RequiresClientProvider(t *testing.T) { + client, _, err := newTestPair(t) + if err != nil { + t.Fatal(err) + } + result, err := client.CallTool(t.Context(), &mcp.CallToolParams{ + Name: "list", + Arguments: map[string]any{}, + }) + if err != nil { + t.Fatal(err) + } + if !result.IsError { + t.Fatal("expected IsError=true when no client provider is configured") } }