diff --git a/pkg/mcp/instructions.md b/pkg/mcp/instructions.md index 8f0e55025c..e2c6c0c5ad 100644 --- a/pkg/mcp/instructions.md +++ b/pkg/mcp/instructions.md @@ -39,7 +39,7 @@ This is essential because: **Exceptions:** - The `list` tool operates on the cluster, not local files, so it does NOT use a path parameter (it uses namespace instead) -- The `delete` tool requires exactly one of `path` or `name`; it does NOT support a no-argument CWD mode (the MCP server process has its own working directory unrelated to the Function being managed) +- The `delete` and `describe` tools each require exactly one of `path` or `name`; they do NOT support a no-argument CWD mode (the MCP server process has its own working directory unrelated to the Function being managed) ## Deployment Behavior @@ -57,6 +57,7 @@ This is essential because: - Before 'deploy' → Read `func://help/deploy` - Before 'build' → Read `func://help/build` - Before 'list' → Read `func://help/list` +- Before 'describe' → Read `func://help/describe` - Before 'delete' → Read `func://help/delete` The help text provides authoritative parameter information and usage context. @@ -129,7 +130,18 @@ A first-time deploy can be detected by checking the func.yaml for a value in the - **FIRST:** Read `func://help/list` for authoritative usage information - Does NOT use path parameter (operates on cluster, not local files) - Optional `namespace` parameter to list Functions in specific namespace -- Returns list of deployed Functions in current/specified namespace +- Returns structured JSON: an `items` array (name, namespace, runtime, url, ready, deployer for each deployed Function; empty if none found) plus `warnings` for any non-fatal issues encountered while listing (e.g. a deployer backend that couldn't be reached) + +### describe + +- **FIRST:** Read `func://help/describe` for authoritative usage information +- Describes a **deployed** Function instance on the cluster; it never just reads local `func.yaml`. This tool will fail if the Function has not yet been deployed (e.g. calling it right after 'create' but before 'deploy' is a usage error, not a tool bug) +- Supports TWO modes (mutually exclusive): + 1. **Describe by PATH:** Provide 'path' parameter (reads the Function's name/namespace from func.yaml at that path, then describes whatever is deployed for that Function) + 2. **Describe by NAME:** Provide 'name' parameter (describes the named Function on the cluster) +- Exactly ONE of 'path' or 'name' must be provided, not both +- 'namespace' is only valid together with 'name'; providing both 'path' and 'namespace' is rejected, since path mode already determines the namespace from the Function's own deploy identity +- Read-only; does not modify local files or cluster resources ### delete diff --git a/pkg/mcp/mcp.go b/pkg/mcp/mcp.go index 3d93e18d07..db73c27b40 100644 --- a/pkg/mcp/mcp.go +++ b/pkg/mcp/mcp.go @@ -1,6 +1,7 @@ package mcp import ( + "bytes" "context" "fmt" "os/exec" @@ -32,6 +33,14 @@ type Server struct { type executor interface { Execute(ctx context.Context, subcommand string, args ...string) ([]byte, error) + // ExecuteSplit runs the command and returns stdout and stderr captured + // into separate buffers. Unlike Execute (which uses CombinedOutput and + // therefore offers no guarantee about the relative ordering of stdout + // and stderr bytes - they're copied by two independently-scheduled + // goroutines), ExecuteSplit gives each stream its own buffer, so callers + // that need to parse structured output (e.g. JSON) from stdout can do so + // without risk of stderr content (warnings, etc.) corrupting the parse. + ExecuteSplit(ctx context.Context, subcommand string, args ...string) (stdout, stderr []byte, err error) } type Option func(*Server) @@ -108,6 +117,7 @@ func New(options ...Option) *Server { mcp.AddTool(i, buildTool, s.buildHandler) mcp.AddTool(i, deployTool, s.deployHandler) mcp.AddTool(i, listTool, s.listHandler) + mcp.AddTool(i, describeTool, s.describeHandler) mcp.AddTool(i, deleteTool, s.deleteHandler) mcp.AddTool(i, configVolumesListTool, s.configVolumesListHandler) mcp.AddTool(i, configVolumesAddTool, s.configVolumesAddHandler) @@ -139,6 +149,7 @@ func New(options ...Option) *Server { i.AddResource(newHelpResource(s, "Build Help", "help for 'build'", "build")) i.AddResource(newHelpResource(s, "Deploy Help", "help for 'deploy'", "deploy")) i.AddResource(newHelpResource(s, "List Help", "help for 'list'", "list")) + i.AddResource(newHelpResource(s, "Describe Help", "help for 'describe'", "describe")) i.AddResource(newHelpResource(s, "Delete Help", "help for delete", "delete")) i.AddResource(newHelpResource(s, "Volumes Help", "general help for volumes", "config", "volumes")) @@ -181,6 +192,17 @@ func (e defaultExecutor) Execute(ctx context.Context, subcommand string, args .. return cmd.CombinedOutput() } +func (e defaultExecutor) ExecuteSplit(ctx context.Context, subcommand string, args ...string) (stdout, stderr []byte, err error) { + 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 + var outBuf, errBuf bytes.Buffer + cmd.Stdout = &outBuf + cmd.Stderr = &errBuf + err = cmd.Run() + return outBuf.Bytes(), errBuf.Bytes(), err +} + // 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. diff --git a/pkg/mcp/mock/executor.go b/pkg/mcp/mock/executor.go index 1427df430c..859bdc4bb8 100644 --- a/pkg/mcp/mock/executor.go +++ b/pkg/mcp/mock/executor.go @@ -9,6 +9,9 @@ import ( type Executor struct { ExecuteInvoked bool ExecuteFn func(context.Context, string, ...string) ([]byte, error) + + ExecuteSplitInvoked bool + ExecuteSplitFn func(context.Context, string, ...string) (stdout, stderr []byte, err error) } // NewExecutor creates a new mock executor @@ -27,3 +30,15 @@ func (m *Executor) Execute(ctx context.Context, subcommand string, args ...strin return []byte(""), nil } + +// ExecuteSplit implements the executor interface, recording invocation +// details and delegating to ExecuteSplitFn if provided. +func (m *Executor) ExecuteSplit(ctx context.Context, subcommand string, args ...string) ([]byte, []byte, error) { + m.ExecuteSplitInvoked = true + + if m.ExecuteSplitFn != nil { + return m.ExecuteSplitFn(ctx, subcommand, args...) + } + + return []byte(""), []byte(""), nil +} diff --git a/pkg/mcp/tools_describe.go b/pkg/mcp/tools_describe.go new file mode 100644 index 0000000000..270c5b995b --- /dev/null +++ b/pkg/mcp/tools_describe.go @@ -0,0 +1,126 @@ +package mcp + +import ( + "context" + "encoding/json" + "fmt" + "strings" + + "github.com/modelcontextprotocol/go-sdk/mcp" + fn "knative.dev/func/pkg/functions" +) + +var describeTool = &mcp.Tool{ + Name: "describe", + Title: "Describe Function", + Description: "Describe a deployed Function: URL, routes, image, namespace, deployer, labels, revision, readiness, and event subscriptions.", + Annotations: &mcp.ToolAnnotations{ + Title: "Describe Function", + ReadOnlyHint: true, + IdempotentHint: true, // Describing the same function multiple times returns consistent results at any point in time. + }, +} + +func (s *Server) describeHandler(ctx context.Context, r *mcp.CallToolRequest, input DescribeInput) (result *mcp.CallToolResult, output DescribeOutput, err error) { + // Validate: exactly one of Path or Name must be provided + if (input.Path != nil && input.Name != nil) || (input.Path == nil && input.Name == nil) { + err = fmt.Errorf("exactly one of 'path' or 'name' must be provided") + return + } + + // Validate: namespace only makes sense alongside 'name'. When describing + // by 'path', the Function's name and namespace are read from its own + // deploy identity (func.yaml); the CLI rejects a separate --namespace in + // that mode ("must also specify a name when specifying namespace"). + if input.Path != nil && input.Namespace != nil { + err = fmt.Errorf("'namespace' is only valid with 'name'; when describing by 'path', the namespace is read from the Function's own deploy identity") + return + } + + // ExecuteSplit (rather than Execute/CombinedOutput) is required here: + // the CLI can write warnings to stderr on an otherwise-successful call + // (e.g. permission warnings from the knative describer), and stdout and + // stderr copied via CombinedOutput have no guaranteed relative ordering. + // Parsing JSON only ever out of a clean, unmixed stdout avoids that + // entirely rather than relying on any heuristic about stream ordering. + stdout, stderr, err := s.executor.ExecuteSplit(ctx, "describe", input.Args()...) + if err != nil { + err = fmt.Errorf("%w\nstdout: %s\nstderr: %s", err, string(stdout), string(stderr)) + return + } + + var instance fn.Instance + if err = json.Unmarshal(stdout, &instance); err != nil { + err = fmt.Errorf("failed to parse describe output: %w\n%s", err, string(stdout)) + return + } + + var middleware *fn.Middleware + if instance.Middleware.Version != "" { + middleware = &instance.Middleware + } + + output = DescribeOutput{ + Name: instance.Name, + Namespace: instance.Namespace, + URL: instance.Route, + Routes: instance.Routes, + Image: instance.Image, + Ready: instance.Ready, + Deployer: instance.Deployer, + Labels: instance.Labels, + Subscriptions: instance.Subscriptions, + Middleware: middleware, + Revision: instance.Revision, + // A non-fatal warning on stderr (e.g. RBAC denying the eventing + // trigger list) can accompany an otherwise-successful, but partial, + // JSON payload on stdout (e.g. an empty subscriptions list). + // Surface it so the agent doesn't mistake "no subscriptions" for + // "no permission to see them". + Warnings: strings.TrimSpace(string(stderr)), + } + return +} + +// DescribeInput defines the input parameters for the describe tool. +// Exactly one of Path or Name must be provided. +type DescribeInput struct { + Path *string `json:"path,omitempty" jsonschema:"Path to the function project directory (mutually exclusive with name)"` + Name *string `json:"name,omitempty" jsonschema:"Name of the function to describe (mutually exclusive with path)"` + Namespace *string `json:"namespace,omitempty" jsonschema:"Kubernetes namespace to describe from (default: current or active namespace). Only valid together with 'name'; when describing by 'path' the namespace is read from the Function's own deploy identity"` + Verbose *bool `json:"verbose,omitempty" jsonschema:"Enable verbose logging output"` +} + +func (i DescribeInput) Args() []string { + args := []string{} + + // Either path flag or positional name argument + if i.Path != nil { + args = append(args, "--path", *i.Path) + } else if i.Name != nil { + args = append(args, *i.Name) + } + + args = appendStringFlag(args, "--namespace", i.Namespace) + args = appendBoolFlag(args, "--verbose", i.Verbose) + + // The tool's contract is structured JSON, regardless of caller input. + args = append(args, "--output", "json") + return args +} + +// DescribeOutput defines the structured output returned by the describe tool. +type DescribeOutput struct { + Name string `json:"name" jsonschema:"Function name"` + Namespace string `json:"namespace,omitempty" jsonschema:"Kubernetes namespace"` + URL string `json:"url,omitempty" jsonschema:"Primary route URL"` + Routes []string `json:"routes,omitempty" jsonschema:"All route URLs"` + Image string `json:"image,omitempty" jsonschema:"Deployed container image"` + Ready string `json:"ready,omitempty" jsonschema:"Overall readiness (true/false/unknown)"` + Deployer string `json:"deployer,omitempty" jsonschema:"Deployer backend (knative, k8s, keda)"` + Labels map[string]string `json:"labels,omitempty" jsonschema:"Function labels"` + Subscriptions []fn.Subscription `json:"subscriptions,omitempty" jsonschema:"Active event subscriptions"` + Middleware *fn.Middleware `json:"middleware,omitempty" jsonschema:"Middleware backend (e.g. keda) applied at deploy time, if any"` + Revision string `json:"revision,omitempty" jsonschema:"Source commit SHA, read from the OCI revision label baked into the built image"` + Warnings string `json:"warnings,omitempty" jsonschema:"Non-fatal warnings emitted while gathering this Function's status (e.g. permission errors that caused partial data, such as an empty subscriptions list)"` +} diff --git a/pkg/mcp/tools_describe_test.go b/pkg/mcp/tools_describe_test.go new file mode 100644 index 0000000000..83776d076d --- /dev/null +++ b/pkg/mcp/tools_describe_test.go @@ -0,0 +1,328 @@ +package mcp + +import ( + "context" + "testing" + + "github.com/modelcontextprotocol/go-sdk/mcp" + "knative.dev/func/pkg/mcp/mock" +) + +// TestTool_Describe_Args ensures the describe tool executes with all arguments +// passed correctly, and that the structured JSON output from the CLI is parsed +// into DescribeOutput correctly. +func TestTool_Describe_Args(t *testing.T) { + stringFlags := map[string]struct { + jsonKey string + flag string + value string + }{ + "namespace": {"namespace", "--namespace", "prod"}, + } + + boolFlags := map[string]string{ + "verbose": "--verbose", + } + + name := "my-function" + + executor := mock.NewExecutor() + executor.ExecuteSplitFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, []byte, error) { + if subcommand != "describe" { + t.Fatalf("expected subcommand 'describe', got %q", subcommand) + } + + // Expected: 1 positional + 2 string flags (namespace, output) * 2 + 1 bool flag + // = 1 + 2*2 + 1 = 6 args + if len(args) != 1+2*2+1 { + t.Fatalf("expected %d args, got %d: %v", 1+2*2+1, len(args), args) + } + + if args[0] != name { + t.Fatalf("expected positional arg %q, got %q", name, args[0]) + } + + rest := args[1:] + validateStringFlags(t, rest, stringFlags) + validateBoolFlags(t, rest, boolFlags) + validateStringFlags(t, rest, map[string]struct { + jsonKey string + flag string + value string + }{ + "output": {"output", "--output", "json"}, + }) + + // NOTE: fn.Instance.Route has no json tag (unlike its sibling + // fields), so real `func describe --output json` output emits + // "Route" capitalized. Using that exact casing here (rather than + // "route") keeps this test honest about the real CLI wire format. + return []byte(`{ + "name": "my-function", + "namespace": "prod", + "Route": "https://my-function.prod.example.com", + "routes": ["https://my-function.prod.example.com"], + "image": "docker.io/alice/my-function:latest", + "deployer": "knative", + "labels": {"app": "my-function"}, + "subscriptions": [{"source": "src", "type": "type", "broker": "default"}], + "middleware": {"version": "1.2.3"}, + "revision": "abc123", + "ready": "true" + }`), nil, nil + } + + client, server, err := newTestPair(t, WithExecutor(executor)) + if err != nil { + t.Fatal(err) + } + server.readonly.Store(false) + + inputArgs := buildInputArgs(stringFlags, boolFlags) + inputArgs["name"] = name + + result, err := client.CallTool(t.Context(), &mcp.CallToolParams{ + Name: "describe", + Arguments: inputArgs, + }) + if err != nil { + t.Fatal(err) + } + if result.IsError { + t.Fatalf("unexpected error result: %v", result) + } + if !executor.ExecuteSplitInvoked { + t.Fatal("executor was not invoked") + } + + var output DescribeOutput + if err := unmarshalStructuredContent(result, &output); err != nil { + t.Fatal(err) + } + + if output.Name != "my-function" { + t.Errorf("expected name %q, got %q", "my-function", output.Name) + } + if output.Namespace != "prod" { + t.Errorf("expected namespace %q, got %q", "prod", output.Namespace) + } + if output.URL != "https://my-function.prod.example.com" { + t.Errorf("expected url %q, got %q", "https://my-function.prod.example.com", output.URL) + } + if len(output.Routes) != 1 || output.Routes[0] != "https://my-function.prod.example.com" { + t.Errorf("unexpected routes: %v", output.Routes) + } + if output.Image != "docker.io/alice/my-function:latest" { + t.Errorf("expected image %q, got %q", "docker.io/alice/my-function:latest", output.Image) + } + if output.Ready != "true" { + t.Errorf("expected ready %q, got %q", "true", output.Ready) + } + if output.Deployer != "knative" { + t.Errorf("expected deployer %q, got %q", "knative", output.Deployer) + } + if output.Labels["app"] != "my-function" { + t.Errorf("expected label app=my-function, got %v", output.Labels) + } + if len(output.Subscriptions) != 1 || output.Subscriptions[0].Broker != "default" { + t.Errorf("unexpected subscriptions: %v", output.Subscriptions) + } + if output.Middleware == nil || output.Middleware.Version != "1.2.3" { + t.Errorf("expected middleware version %q, got %v", "1.2.3", output.Middleware) + } + if output.Revision != "abc123" { + t.Errorf("expected revision %q, got %q", "abc123", output.Revision) + } + if output.Warnings != "" { + t.Errorf("expected no warnings, got %q", output.Warnings) + } +} + +// TestTool_Describe_NoMiddleware ensures the middleware field is omitted +// (left nil) when the CLI reports no middleware version, rather than +// surfacing a zero-value struct. +func TestTool_Describe_NoMiddleware(t *testing.T) { + executor := mock.NewExecutor() + executor.ExecuteSplitFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, []byte, error) { + return []byte(`{"name": "my-function"}`), nil, nil + } + + client, _, err := newTestPair(t, WithExecutor(executor)) + if err != nil { + t.Fatal(err) + } + + result, err := client.CallTool(t.Context(), &mcp.CallToolParams{ + Name: "describe", + Arguments: map[string]any{"name": "my-function"}, + }) + if err != nil { + t.Fatal(err) + } + if result.IsError { + t.Fatalf("unexpected error result: %v", result) + } + + var output DescribeOutput + if err := unmarshalStructuredContent(result, &output); err != nil { + t.Fatal(err) + } + if output.Middleware != nil { + t.Errorf("expected nil middleware, got %v", output.Middleware) + } +} + +// TestTool_Describe_PathAndNameMutuallyExclusive ensures providing both path +// and name is rejected. +func TestTool_Describe_PathAndNameMutuallyExclusive(t *testing.T) { + client, _, err := newTestPair(t) + if err != nil { + t.Fatal(err) + } + + result, err := client.CallTool(t.Context(), &mcp.CallToolParams{ + Name: "describe", + Arguments: map[string]any{ + "path": "/tmp/my-function", + "name": "my-function", + }, + }) + if err != nil { + t.Fatal(err) + } + if !result.IsError { + t.Fatal("expected describe to be rejected when both path and name are provided") + } +} + +// TestTool_Describe_PathAndNamespaceRejected ensures providing both 'path' +// and 'namespace' is rejected: path mode determines the namespace from the +// Function's own deploy identity (func.yaml), and the CLI itself rejects a +// separate --namespace in that mode. +func TestTool_Describe_PathAndNamespaceRejected(t *testing.T) { + executor := mock.NewExecutor() + executor.ExecuteSplitFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, []byte, error) { + t.Fatal("executor should not be invoked when path+namespace validation fails") + return nil, nil, nil + } + + client, _, err := newTestPair(t, WithExecutor(executor)) + if err != nil { + t.Fatal(err) + } + + result, err := client.CallTool(t.Context(), &mcp.CallToolParams{ + Name: "describe", + Arguments: map[string]any{ + "path": "/tmp/my-function", + "namespace": "prod", + }, + }) + if err != nil { + t.Fatal(err) + } + if !result.IsError { + t.Fatal("expected describe to be rejected when both path and namespace are provided") + } +} + +// TestTool_Describe_RequiresPathOrName ensures providing neither path nor name +// is rejected. +func TestTool_Describe_RequiresPathOrName(t *testing.T) { + client, _, err := newTestPair(t) + if err != nil { + t.Fatal(err) + } + + result, err := client.CallTool(t.Context(), &mcp.CallToolParams{ + Name: "describe", + Arguments: map[string]any{}, + }) + if err != nil { + t.Fatal(err) + } + if !result.IsError { + t.Fatal("expected describe to be rejected when neither path nor name is provided") + } +} + +// TestTool_Describe_MalformedJSON ensures the handler returns an error +// (rather than panicking) when the CLI output cannot be parsed as JSON. +func TestTool_Describe_MalformedJSON(t *testing.T) { + executor := mock.NewExecutor() + executor.ExecuteSplitFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, []byte, error) { + return []byte("not json"), nil, nil + } + + client, _, err := newTestPair(t, WithExecutor(executor)) + if err != nil { + t.Fatal(err) + } + + result, err := client.CallTool(t.Context(), &mcp.CallToolParams{ + Name: "describe", + Arguments: map[string]any{"name": "my-function"}, + }) + if err != nil { + t.Fatal(err) + } + if !result.IsError { + t.Fatal("expected describe to return an error result for malformed JSON output") + } +} + +// TestTool_Describe_StderrWarningDoesNotBreakParsing ensures the handler +// parses the JSON payload correctly even when the CLI writes a warning to +// stderr on an otherwise-successful call (e.g. the knative describer's +// permission warnings, see pkg/knative/describer.go). This is exactly the +// scenario ExecuteSplit exists for: stdout and stderr are captured into +// independent buffers by the executor, so stderr content can never corrupt +// the JSON parse regardless of how the two streams would have interleaved +// under CombinedOutput. +func TestTool_Describe_StderrWarningDoesNotBreakParsing(t *testing.T) { + executor := mock.NewExecutor() + executor.ExecuteSplitFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, []byte, error) { + stdout := []byte(`{ + "name": "my-function", + "namespace": "prod", + "ready": "true" + }`) + stderr := []byte("Warning: cannot list eventing triggers (permission denied) - skipping\n") + return stdout, stderr, nil + } + + client, server, err := newTestPair(t, WithExecutor(executor)) + if err != nil { + t.Fatal(err) + } + server.readonly.Store(false) + + result, err := client.CallTool(t.Context(), &mcp.CallToolParams{ + Name: "describe", + Arguments: map[string]any{"name": "my-function"}, + }) + if err != nil { + t.Fatal(err) + } + if result.IsError { + t.Fatalf("unexpected error result: %v", result) + } + + var output DescribeOutput + if err := unmarshalStructuredContent(result, &output); err != nil { + t.Fatal(err) + } + if output.Name != "my-function" { + t.Errorf("expected name %q, got %q", "my-function", output.Name) + } + if output.Namespace != "prod" { + t.Errorf("expected namespace %q, got %q", "prod", output.Namespace) + } + if output.Ready != "true" { + t.Errorf("expected ready %q, got %q", "true", output.Ready) + } + wantWarning := "Warning: cannot list eventing triggers (permission denied) - skipping" + if output.Warnings != wantWarning { + t.Errorf("expected warnings %q, got %q", wantWarning, output.Warnings) + } +} diff --git a/pkg/mcp/tools_list.go b/pkg/mcp/tools_list.go index 4f8b0ec6ba..88941c60cd 100644 --- a/pkg/mcp/tools_list.go +++ b/pkg/mcp/tools_list.go @@ -1,10 +1,14 @@ package mcp import ( + "bytes" "context" + "encoding/json" "fmt" + "strings" "github.com/modelcontextprotocol/go-sdk/mcp" + fn "knative.dev/func/pkg/functions" ) var listTool = &mcp.Tool{ @@ -18,14 +22,36 @@ var listTool = &mcp.Tool{ }, } +// noFunctionsFoundPrefix is the leading text of the human-readable message +// `func list` prints on stdout (see cmd/list.go printNoFunctionsFound) when +// there are zero results. This is printed even under `--output json` +// instead of an empty JSON array, so it must be special-cased below rather +// than treated as a JSON parse failure. +const noFunctionsFoundPrefix = "no functions found" + 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()...) + // ExecuteSplit (rather than Execute/CombinedOutput) is required here for + // the same reason as the describe tool: stdout must stay a clean, + // unmixed JSON payload so it can be parsed regardless of anything the + // CLI writes to stderr. + stdout, stderr, err := s.executor.ExecuteSplit(ctx, "list", input.Args()...) if err != nil { - err = fmt.Errorf("%w\n%s", err, string(out)) + err = fmt.Errorf("%w\nstdout: %s\nstderr: %s", err, string(stdout), string(stderr)) return } + + items := []fn.ListItem{} + trimmed := bytes.TrimSpace(stdout) + if len(trimmed) != 0 && !bytes.HasPrefix(trimmed, []byte(noFunctionsFoundPrefix)) { + if err = json.Unmarshal(trimmed, &items); err != nil { + err = fmt.Errorf("failed to parse list output: %w\n%s", err, string(stdout)) + return + } + } + output = ListOutput{ - Message: string(out), + Items: items, + Warnings: strings.TrimSpace(string(stderr)), } return } @@ -35,7 +61,6 @@ func (s *Server) listHandler(ctx context.Context, r *mcp.CallToolRequest, input 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"` } @@ -44,12 +69,19 @@ func (i ListInput) 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) + + // The tool's contract is structured JSON, regardless of caller input. + args = append(args, "--output", "json") return args } // ListOutput defines the structured output returned by the list tool. type ListOutput struct { - Message string `json:"message" jsonschema:"Output message"` + Items []fn.ListItem `json:"items" jsonschema:"Deployed Functions matching the query (empty if none found)"` + // A non-fatal warning on stderr (e.g. a cluster connectivity issue for + // one of several configured deployers) can accompany an otherwise + // successful, but partial, list. Surface it so the agent doesn't + // mistake a short list for the complete picture. + Warnings string `json:"warnings,omitempty" jsonschema:"Non-fatal warnings emitted while listing Functions"` } diff --git a/pkg/mcp/tools_list_test.go b/pkg/mcp/tools_list_test.go index 54caf014fe..fc1c32daa2 100644 --- a/pkg/mcp/tools_list_test.go +++ b/pkg/mcp/tools_list_test.go @@ -2,22 +2,23 @@ package mcp import ( "context" + "fmt" "testing" "github.com/modelcontextprotocol/go-sdk/mcp" "knative.dev/func/pkg/mcp/mock" ) -// TestTool_List_Args ensures the list tool executes with all arguments passed correctly. +// TestTool_List_Args ensures the list tool executes with all arguments passed +// correctly, always forcing --output json regardless of caller input, and +// that the structured JSON output from the CLI is parsed into ListOutput. 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"}, } boolFlags := map[string]string{ @@ -26,16 +27,24 @@ func TestTool_List_Args(t *testing.T) { } executor := mock.NewExecutor() - executor.ExecuteFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, error) { + executor.ExecuteSplitFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, []byte, error) { if subcommand != "list" { t.Fatalf("expected subcommand 'list', got %q", subcommand) } - validateArgLength(t, args, len(stringFlags), len(boolFlags)) + // len(stringFlags) + 1 for the always-appended --output json + validateArgLength(t, args, len(stringFlags)+1, len(boolFlags)) validateStringFlags(t, args, stringFlags) validateBoolFlags(t, args, boolFlags) + validateStringFlags(t, args, map[string]struct { + jsonKey string + flag string + value string + }{ + "output": {"output", "--output", "json"}, + }) - return []byte("NAME\tNAMESPACE\tRUNTIME\nmy-func\tprod\tgo\n"), nil + return []byte(`[{"name":"my-func","namespace":"prod","runtime":"go","url":"https://my-func.prod.example.com","ready":"true","deployer":"knative"}]`), nil, nil } client, _, err := newTestPair(t, WithExecutor(executor)) @@ -43,10 +52,8 @@ func TestTool_List_Args(t *testing.T) { 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, @@ -57,7 +64,198 @@ func TestTool_List_Args(t *testing.T) { if result.IsError { t.Fatalf("unexpected error result: %v", result) } - if !executor.ExecuteInvoked { + if !executor.ExecuteSplitInvoked { t.Fatal("executor was not invoked") } + + var output ListOutput + if err := unmarshalStructuredContent(result, &output); err != nil { + t.Fatal(err) + } + if len(output.Items) != 1 { + t.Fatalf("expected 1 item, got %d: %v", len(output.Items), output.Items) + } + item := output.Items[0] + if item.Name != "my-func" { + t.Errorf("expected name %q, got %q", "my-func", item.Name) + } + if item.Namespace != "prod" { + t.Errorf("expected namespace %q, got %q", "prod", item.Namespace) + } + if item.Runtime != "go" { + t.Errorf("expected runtime %q, got %q", "go", item.Runtime) + } + if item.URL != "https://my-func.prod.example.com" { + t.Errorf("expected url %q, got %q", "https://my-func.prod.example.com", item.URL) + } + if item.Ready != "true" { + t.Errorf("expected ready %q, got %q", "true", item.Ready) + } + if item.Deployer != "knative" { + t.Errorf("expected deployer %q, got %q", "knative", item.Deployer) + } + if output.Warnings != "" { + t.Errorf("expected no warnings, got %q", output.Warnings) + } +} + +// TestTool_List_NoFunctionsFound ensures the handler treats the CLI's +// human-readable "no functions found" message (which `func list` prints on +// stdout even under --output json, see cmd/list.go printNoFunctionsFound) as +// a valid empty result rather than a JSON parse failure. +func TestTool_List_NoFunctionsFound(t *testing.T) { + executor := mock.NewExecutor() + executor.ExecuteSplitFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, []byte, error) { + return []byte(`no functions found in namespace 'prod' + +'func list' shows functions that have been deployed to your cluster.`), nil, nil + } + + client, _, err := newTestPair(t, WithExecutor(executor)) + 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.Fatalf("unexpected error result: %v", result) + } + + var output ListOutput + if err := unmarshalStructuredContent(result, &output); err != nil { + t.Fatal(err) + } + if len(output.Items) != 0 { + t.Errorf("expected no items, got %v", output.Items) + } +} + +// TestTool_List_EmptyStdout ensures a handler that receives completely empty +// stdout (no output at all) also resolves to an empty, non-erroring result. +func TestTool_List_EmptyStdout(t *testing.T) { + executor := mock.NewExecutor() + executor.ExecuteSplitFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, []byte, error) { + return []byte(""), nil, nil + } + + client, _, err := newTestPair(t, WithExecutor(executor)) + 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.Fatalf("unexpected error result: %v", result) + } + + var output ListOutput + if err := unmarshalStructuredContent(result, &output); err != nil { + t.Fatal(err) + } + if len(output.Items) != 0 { + t.Errorf("expected no items, got %v", output.Items) + } +} + +// TestTool_List_MalformedJSON ensures the handler returns an error (rather +// than panicking) when the CLI output cannot be parsed as JSON and doesn't +// match the known "no functions found" message. +func TestTool_List_MalformedJSON(t *testing.T) { + executor := mock.NewExecutor() + executor.ExecuteSplitFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, []byte, error) { + return []byte("not json"), nil, nil + } + + client, _, err := newTestPair(t, WithExecutor(executor)) + 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 list to return an error result for malformed JSON output") + } +} + +// TestTool_List_StderrWarningDoesNotBreakParsing ensures the handler parses +// the JSON payload correctly even when the CLI writes a warning to stderr on +// an otherwise-successful call, and surfaces that warning in the output. +func TestTool_List_StderrWarningDoesNotBreakParsing(t *testing.T) { + executor := mock.NewExecutor() + executor.ExecuteSplitFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, []byte, error) { + stdout := []byte(`[{"name":"my-func","namespace":"prod","runtime":"go","url":"https://my-func.prod.example.com","ready":"true","deployer":"knative"}]`) + stderr := []byte("Warning: cannot connect to keda deployer - skipping\n") + return stdout, stderr, nil + } + + client, _, err := newTestPair(t, WithExecutor(executor)) + 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.Fatalf("unexpected error result: %v", result) + } + + var output ListOutput + if err := unmarshalStructuredContent(result, &output); err != nil { + t.Fatal(err) + } + if len(output.Items) != 1 { + t.Fatalf("expected 1 item, got %d: %v", len(output.Items), output.Items) + } + wantWarning := "Warning: cannot connect to keda deployer - skipping" + if output.Warnings != wantWarning { + t.Errorf("expected warnings %q, got %q", wantWarning, output.Warnings) + } +} + +// TestTool_List_CLIError ensures a CLI failure (non-zero exit) surfaces as +// an error result including stdout/stderr context. +func TestTool_List_CLIError(t *testing.T) { + executor := mock.NewExecutor() + executor.ExecuteSplitFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, []byte, error) { + return nil, []byte("Error: cannot connect to cluster"), fmt.Errorf("exit status 1") + } + + client, _, err := newTestPair(t, WithExecutor(executor)) + 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 list to return an error result when the CLI fails") + } } diff --git a/pkg/mcp/tools_test.go b/pkg/mcp/tools_test.go index a4931ecdbf..52e812e86d 100644 --- a/pkg/mcp/tools_test.go +++ b/pkg/mcp/tools_test.go @@ -1,10 +1,31 @@ package mcp import ( + "encoding/json" + "fmt" "strings" "testing" + + "github.com/modelcontextprotocol/go-sdk/mcp" ) +// unmarshalStructuredContent decodes a CallToolResult's StructuredContent +// (which may arrive as json.RawMessage or as an already-decoded map) into out. +func unmarshalStructuredContent(result *mcp.CallToolResult, out any) error { + switch sc := result.StructuredContent.(type) { + case json.RawMessage: + return json.Unmarshal(sc, out) + case nil: + return fmt.Errorf("result has no structured content") + default: + b, err := json.Marshal(sc) + if err != nil { + return err + } + return json.Unmarshal(b, out) + } +} + // validateArgLength validates that the args slice has the expected length based on // the number of string flags (2 args each: flag + value) and bool flags (1 arg each). func validateArgLength(t *testing.T, args []string, stringFlagsCount, boolFlagsCount int) {