From d4dbb84e4d50b98484539743c91e14b749531176 Mon Sep 17 00:00:00 2001 From: Ankitsinghsisodya Date: Fri, 7 Aug 2026 19:16:56 +0530 Subject: [PATCH 1/6] feat(mcp): add 'describe' tool and update documentation - Introduced the 'describe' tool with support for both path and name parameters. - Updated instructions to clarify the requirements for 'delete' and 'describe' tools. - Added help resources for the 'describe' tool in the MCP server. This enhances the functionality of the MCP by providing a way to describe functions in the cluster. --- pkg/mcp/instructions.md | 12 +- pkg/mcp/mcp.go | 2 + pkg/mcp/tools_describe.go | 96 ++++++++++++++++ pkg/mcp/tools_describe_test.go | 196 +++++++++++++++++++++++++++++++++ pkg/mcp/tools_test.go | 21 ++++ 5 files changed, 326 insertions(+), 1 deletion(-) create mode 100644 pkg/mcp/tools_describe.go create mode 100644 pkg/mcp/tools_describe_test.go diff --git a/pkg/mcp/instructions.md b/pkg/mcp/instructions.md index 45af704c81..fb4f1fefbc 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. @@ -131,6 +132,15 @@ A first-time deploy can be detected by checking the func.yaml for a value in the - Optional `namespace` parameter to list Functions in specific namespace - Returns list of deployed Functions in current/specified namespace +### describe + +- **FIRST:** Read `func://help/describe` for authoritative usage information +- Supports TWO modes (mutually exclusive): + 1. **Describe by PATH:** Provide 'path' parameter (reads function name from func.yaml at that path) + 2. **Describe by NAME:** Provide 'name' parameter (describes named function from cluster) +- Exactly ONE of 'path' or 'name' must be provided, not both +- Read-only; does not modify local files or cluster resources + ### delete - **FIRST:** Read `func://help/delete` for authoritative usage information diff --git a/pkg/mcp/mcp.go b/pkg/mcp/mcp.go index 02ea0f5d44..b29da06036 100644 --- a/pkg/mcp/mcp.go +++ b/pkg/mcp/mcp.go @@ -108,6 +108,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) @@ -138,6 +139,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")) diff --git a/pkg/mcp/tools_describe.go b/pkg/mcp/tools_describe.go new file mode 100644 index 0000000000..ae79f387bf --- /dev/null +++ b/pkg/mcp/tools_describe.go @@ -0,0 +1,96 @@ +package mcp + +import ( + "context" + "encoding/json" + "fmt" + + "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, image, namespace, labels, 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 + } + + out, err := s.executor.Execute(ctx, "describe", input.Args()...) + if err != nil { + err = fmt.Errorf("%w\n%s", err, string(out)) + return + } + + var instance fn.Instance + if err = json.Unmarshal(out, &instance); err != nil { + err = fmt.Errorf("failed to parse describe output: %w\n%s", err, string(out)) + return + } + + 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, + Revision: instance.Revision, + } + 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)"` + 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"` + Revision string `json:"revision,omitempty" jsonschema:"Source commit SHA embedded in the image"` +} diff --git a/pkg/mcp/tools_describe_test.go b/pkg/mcp/tools_describe_test.go new file mode 100644 index 0000000000..4b036885ec --- /dev/null +++ b/pkg/mcp/tools_describe_test.go @@ -0,0 +1,196 @@ +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.ExecuteFn = func(ctx context.Context, subcommand string, args ...string) ([]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"}, + }) + + 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"}], + "revision": "abc123", + "ready": "true" + }`), 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.ExecuteInvoked { + 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.Revision != "abc123" { + t.Errorf("expected revision %q, got %q", "abc123", output.Revision) + } +} + +// 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_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.ExecuteFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, error) { + return []byte("not json"), 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") + } +} 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) { From 1da4fd0e44cec30e69198be95975df431b83f218 Mon Sep 17 00:00:00 2001 From: Ankitsinghsisodya Date: Sat, 8 Aug 2026 11:53:28 +0530 Subject: [PATCH 2/6] feat(mcp): enhance describe tool to handle leading stderr warnings - Added a new test to verify that the describe handler correctly parses JSON output even when preceded by non-JSON warnings in stderr. - Introduced a new function, parseDescribeOutput, to extract and parse the JSON object from combined stdout+stderr output, ensuring robustness against leading noise. This improves the reliability of the describe tool in real-world scenarios where warnings may be emitted before the JSON payload. --- pkg/mcp/tools_describe.go | 29 +++++++++++++++++++-- pkg/mcp/tools_describe_test.go | 46 ++++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 2 deletions(-) diff --git a/pkg/mcp/tools_describe.go b/pkg/mcp/tools_describe.go index ae79f387bf..85eff454e1 100644 --- a/pkg/mcp/tools_describe.go +++ b/pkg/mcp/tools_describe.go @@ -1,6 +1,7 @@ package mcp import ( + "bytes" "context" "encoding/json" "fmt" @@ -33,8 +34,8 @@ func (s *Server) describeHandler(ctx context.Context, r *mcp.CallToolRequest, in return } - var instance fn.Instance - if err = json.Unmarshal(out, &instance); err != nil { + instance, err := parseDescribeOutput(out) + if err != nil { err = fmt.Errorf("failed to parse describe output: %w\n%s", err, string(out)) return } @@ -54,6 +55,30 @@ func (s *Server) describeHandler(ctx context.Context, r *mcp.CallToolRequest, in return } +// parseDescribeOutput extracts and parses the JSON object emitted by +// `func describe --output json`. The executor captures combined +// stdout+stderr (see defaultExecutor.Execute), so warnings written to +// stderr (e.g. cluster permission notices) may precede the JSON payload +// on success. This skips any leading non-JSON noise by scanning for the +// first '{' that begins a successfully-parseable JSON object. +func parseDescribeOutput(out []byte) (fn.Instance, error) { + var instance fn.Instance + rest := out + offset := 0 + for { + idx := bytes.IndexByte(rest, '{') + if idx == -1 { + return instance, fmt.Errorf("no JSON object found in output") + } + offset += idx + if err := json.Unmarshal(out[offset:], &instance); err == nil { + return instance, nil + } + offset++ + rest = out[offset:] + } +} + // DescribeInput defines the input parameters for the describe tool. // Exactly one of Path or Name must be provided. type DescribeInput struct { diff --git a/pkg/mcp/tools_describe_test.go b/pkg/mcp/tools_describe_test.go index 4b036885ec..835212ec30 100644 --- a/pkg/mcp/tools_describe_test.go +++ b/pkg/mcp/tools_describe_test.go @@ -194,3 +194,49 @@ func TestTool_Describe_MalformedJSON(t *testing.T) { t.Fatal("expected describe to return an error result for malformed JSON output") } } + +// TestTool_Describe_LeadingStderrWarning ensures the handler still parses +// the JSON payload when the executor's combined stdout+stderr output has +// leading non-JSON noise (e.g. a warning printed to stderr before the CLI +// writes its JSON payload to stdout). +func TestTool_Describe_LeadingStderrWarning(t *testing.T) { + executor := mock.NewExecutor() + executor.ExecuteFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, error) { + return []byte("Warning: unable to determine cluster permissions\n" + `{ + "name": "my-function", + "namespace": "prod", + "ready": "true" + }`), 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) + } +} From 309b216b970ea4f6828cfbd0e90b0c007a2ee936 Mon Sep 17 00:00:00 2001 From: Ankitsinghsisodya Date: Sat, 8 Aug 2026 12:15:50 +0530 Subject: [PATCH 3/6] feat(mcp): implement ExecuteSplit for improved stdout/stderr handling - Added ExecuteSplit method to the executor interface, allowing separate capture of stdout and stderr. - Updated the describe handler to utilize ExecuteSplit, ensuring JSON parsing is unaffected by stderr warnings. - Modified tests to validate the new execution method and its behavior with leading stderr content. This enhancement increases the robustness of the describe tool by preventing stderr noise from interfering with JSON output parsing. --- pkg/mcp/mcp.go | 20 +++++++++++++++ pkg/mcp/mock/executor.go | 15 ++++++++++++ pkg/mcp/tools_describe.go | 45 ++++++++++------------------------ pkg/mcp/tools_describe_test.go | 38 +++++++++++++++++----------- 4 files changed, 72 insertions(+), 46 deletions(-) diff --git a/pkg/mcp/mcp.go b/pkg/mcp/mcp.go index b29da06036..d9045ec97d 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) @@ -180,6 +189,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 index 85eff454e1..6e66e3b47f 100644 --- a/pkg/mcp/tools_describe.go +++ b/pkg/mcp/tools_describe.go @@ -1,7 +1,6 @@ package mcp import ( - "bytes" "context" "encoding/json" "fmt" @@ -13,7 +12,7 @@ import ( var describeTool = &mcp.Tool{ Name: "describe", Title: "Describe Function", - Description: "Describe a deployed Function: URL, image, namespace, labels, readiness, and event subscriptions.", + Description: "Describe a deployed Function: URL, routes, image, namespace, deployer, labels, revision, readiness, and event subscriptions.", Annotations: &mcp.ToolAnnotations{ Title: "Describe Function", ReadOnlyHint: true, @@ -28,15 +27,21 @@ func (s *Server) describeHandler(ctx context.Context, r *mcp.CallToolRequest, in return } - out, err := s.executor.Execute(ctx, "describe", input.Args()...) + // 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\n%s", err, string(out)) + err = fmt.Errorf("%w\nstdout: %s\nstderr: %s", err, string(stdout), string(stderr)) return } - instance, err := parseDescribeOutput(out) - if err != nil { - err = fmt.Errorf("failed to parse describe output: %w\n%s", err, string(out)) + 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 } @@ -55,30 +60,6 @@ func (s *Server) describeHandler(ctx context.Context, r *mcp.CallToolRequest, in return } -// parseDescribeOutput extracts and parses the JSON object emitted by -// `func describe --output json`. The executor captures combined -// stdout+stderr (see defaultExecutor.Execute), so warnings written to -// stderr (e.g. cluster permission notices) may precede the JSON payload -// on success. This skips any leading non-JSON noise by scanning for the -// first '{' that begins a successfully-parseable JSON object. -func parseDescribeOutput(out []byte) (fn.Instance, error) { - var instance fn.Instance - rest := out - offset := 0 - for { - idx := bytes.IndexByte(rest, '{') - if idx == -1 { - return instance, fmt.Errorf("no JSON object found in output") - } - offset += idx - if err := json.Unmarshal(out[offset:], &instance); err == nil { - return instance, nil - } - offset++ - rest = out[offset:] - } -} - // DescribeInput defines the input parameters for the describe tool. // Exactly one of Path or Name must be provided. type DescribeInput struct { @@ -117,5 +98,5 @@ type DescribeOutput struct { 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"` - Revision string `json:"revision,omitempty" jsonschema:"Source commit SHA embedded in the image"` + Revision string `json:"revision,omitempty" jsonschema:"Source commit SHA, read from the OCI revision label baked into the built image"` } diff --git a/pkg/mcp/tools_describe_test.go b/pkg/mcp/tools_describe_test.go index 835212ec30..500be17a33 100644 --- a/pkg/mcp/tools_describe_test.go +++ b/pkg/mcp/tools_describe_test.go @@ -27,7 +27,7 @@ func TestTool_Describe_Args(t *testing.T) { name := "my-function" 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 != "describe" { t.Fatalf("expected subcommand 'describe', got %q", subcommand) } @@ -53,10 +53,14 @@ func TestTool_Describe_Args(t *testing.T) { "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", + "Route": "https://my-function.prod.example.com", "routes": ["https://my-function.prod.example.com"], "image": "docker.io/alice/my-function:latest", "deployer": "knative", @@ -64,7 +68,7 @@ func TestTool_Describe_Args(t *testing.T) { "subscriptions": [{"source": "src", "type": "type", "broker": "default"}], "revision": "abc123", "ready": "true" - }`), nil + }`), nil, nil } client, server, err := newTestPair(t, WithExecutor(executor)) @@ -86,7 +90,7 @@ func TestTool_Describe_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") } @@ -174,8 +178,8 @@ func TestTool_Describe_RequiresPathOrName(t *testing.T) { // (rather than panicking) when the CLI output cannot be parsed as JSON. func TestTool_Describe_MalformedJSON(t *testing.T) { executor := mock.NewExecutor() - executor.ExecuteFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, error) { - return []byte("not json"), nil + 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)) @@ -195,18 +199,24 @@ func TestTool_Describe_MalformedJSON(t *testing.T) { } } -// TestTool_Describe_LeadingStderrWarning ensures the handler still parses -// the JSON payload when the executor's combined stdout+stderr output has -// leading non-JSON noise (e.g. a warning printed to stderr before the CLI -// writes its JSON payload to stdout). -func TestTool_Describe_LeadingStderrWarning(t *testing.T) { +// 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.ExecuteFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, error) { - return []byte("Warning: unable to determine cluster permissions\n" + `{ + executor.ExecuteSplitFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, []byte, error) { + stdout := []byte(`{ "name": "my-function", "namespace": "prod", "ready": "true" - }`), nil + }`) + stderr := []byte("Warning: cannot list eventing triggers (permission denied) - skipping\n") + return stdout, stderr, nil } client, server, err := newTestPair(t, WithExecutor(executor)) From ff1d4727ffa071a72017219a4df6890b85c60587 Mon Sep 17 00:00:00 2001 From: Ankitsinghsisodya Date: Mon, 10 Aug 2026 23:06:21 +0530 Subject: [PATCH 4/6] feat(mcp): enhance describe tool with middleware support and validation rules - Updated the describe tool to include middleware version information in the output. - Added validation to ensure 'namespace' is only valid when describing by 'name', rejecting cases where both 'path' and 'namespace' are provided. - Enhanced documentation to clarify usage and requirements for the describe command. - Added tests to verify the new middleware handling and validation logic. These changes improve the functionality and robustness of the describe tool, ensuring accurate representation of function instances in the cluster. --- pkg/mcp/instructions.md | 6 ++- pkg/mcp/tools_describe.go | 26 +++++++++++- pkg/mcp/tools_describe_test.go | 76 ++++++++++++++++++++++++++++++++++ 3 files changed, 105 insertions(+), 3 deletions(-) diff --git a/pkg/mcp/instructions.md b/pkg/mcp/instructions.md index ff7fc4c084..03146c3551 100644 --- a/pkg/mcp/instructions.md +++ b/pkg/mcp/instructions.md @@ -135,10 +135,12 @@ A first-time deploy can be detected by checking the func.yaml for a value in the ### 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 function name from func.yaml at that path) - 2. **Describe by NAME:** Provide 'name' parameter (describes named function from cluster) + 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/tools_describe.go b/pkg/mcp/tools_describe.go index 6e66e3b47f..270c5b995b 100644 --- a/pkg/mcp/tools_describe.go +++ b/pkg/mcp/tools_describe.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "strings" "github.com/modelcontextprotocol/go-sdk/mcp" fn "knative.dev/func/pkg/functions" @@ -27,6 +28,15 @@ func (s *Server) describeHandler(ctx context.Context, r *mcp.CallToolRequest, in 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 @@ -45,6 +55,11 @@ func (s *Server) describeHandler(ctx context.Context, r *mcp.CallToolRequest, in return } + var middleware *fn.Middleware + if instance.Middleware.Version != "" { + middleware = &instance.Middleware + } + output = DescribeOutput{ Name: instance.Name, Namespace: instance.Namespace, @@ -55,7 +70,14 @@ func (s *Server) describeHandler(ctx context.Context, r *mcp.CallToolRequest, in 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 } @@ -65,7 +87,7 @@ func (s *Server) describeHandler(ctx context.Context, r *mcp.CallToolRequest, in 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)"` + 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"` } @@ -98,5 +120,7 @@ type DescribeOutput struct { 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 index 500be17a33..83776d076d 100644 --- a/pkg/mcp/tools_describe_test.go +++ b/pkg/mcp/tools_describe_test.go @@ -66,6 +66,7 @@ func TestTool_Describe_Args(t *testing.T) { "deployer": "knative", "labels": {"app": "my-function"}, "subscriptions": [{"source": "src", "type": "type", "broker": "default"}], + "middleware": {"version": "1.2.3"}, "revision": "abc123", "ready": "true" }`), nil, nil @@ -126,9 +127,49 @@ func TestTool_Describe_Args(t *testing.T) { 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 @@ -154,6 +195,37 @@ func TestTool_Describe_PathAndNameMutuallyExclusive(t *testing.T) { } } +// 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) { @@ -249,4 +321,8 @@ func TestTool_Describe_StderrWarningDoesNotBreakParsing(t *testing.T) { 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) + } } From ca9ff86354aa8aa6a70b31f183d701e50ac959bc Mon Sep 17 00:00:00 2001 From: Ankitsinghsisodya Date: Tue, 11 Aug 2026 00:36:53 +0530 Subject: [PATCH 5/6] re-running-ci From 29e164d258877ec1af9898efddacffb9b27bcbbc Mon Sep 17 00:00:00 2001 From: Ankitsinghsisodya Date: Tue, 11 Aug 2026 01:35:16 +0530 Subject: [PATCH 6/6] re-running-ci