refactor(mcp): call pkg/functions directly for list tool

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

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) {

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()
 }

pkg/mcp/tools_list_test.go

@@ -2,62 +2,116 @@ 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)
 	}
 	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")
 	}
 }