From 23aed7bf6ba873b99c96ae8062280f7ac0ed6a77 Mon Sep 17 00:00:00 2001 From: Ankitsinghsisodya Date: Sat, 16 May 2026 20:14:19 +0530 Subject: [PATCH] refactor(mcp): remove writeEnabled parameter from StartMCPServer and related functions This change simplifies the MCP server start process by removing the writeEnabled boolean parameter. The server now defaults to readonly mode unless explicitly configured otherwise. Updated related tests and comments to reflect this change in behavior. --- cmd/mcp.go | 2 +- cmd/mcp_test.go | 23 ++++++----------------- pkg/functions/client.go | 8 ++++---- pkg/functions/client_test.go | 2 +- pkg/mcp/mcp.go | 6 +++--- pkg/mcp/mcp_test.go | 2 +- pkg/mock/mcp_server.go | 8 ++++---- 7 files changed, 20 insertions(+), 31 deletions(-) diff --git a/cmd/mcp.go b/cmd/mcp.go index 821cf874d4..391817001c 100644 --- a/cmd/mcp.go +++ b/cmd/mcp.go @@ -118,6 +118,6 @@ func runMCPStart(cmd *cobra.Command, args []string, newClient ClientFactory) err defer done() // Start - return client.StartMCPServer(cmd.Context(), writeEnabled) + return client.StartMCPServer(cmd.Context()) } diff --git a/cmd/mcp_test.go b/cmd/mcp_test.go index 7ee8ef2608..0d07ea0c09 100644 --- a/cmd/mcp_test.go +++ b/cmd/mcp_test.go @@ -1,7 +1,6 @@ package cmd import ( - "context" "testing" fn "knative.dev/func/pkg/functions" @@ -30,34 +29,24 @@ func TestMCP_Start(t *testing.T) { } } -// TestMCP_StartWriteable ensures that the server is started with write -// enabled only when the environment variable FUNC_ENABLE_MCP_WRITE is set. +// TestMCP_StartWriteable ensures the "mcp start" command executes without +// error in both default (readonly) and write-enabled modes. +// Readonly correctness (instructions reflecting the correct mode) is covered +// by TestInstructions in pkg/mcp. func TestMCP_StartWriteable(t *testing.T) { _ = FromTempDirectory(t) - // Ensure it defaults to off. + // Default: FUNC_ENABLE_MCP_WRITE unset — should start without error. server := mock.NewMCPServer() - server.StartFn = func(_ context.Context, writeEnabled bool) error { - if writeEnabled { - t.Fatal("MCP server started write-enabled by default") - } - return nil - } cmd := NewMCPCmd(NewTestClient(fn.WithMCPServer(server))) cmd.SetArgs([]string{"start"}) if err := cmd.Execute(); err != nil { t.Fatal(err) } - // Ensure it is set to true on proper truthy value + // Explicit write mode enabled — should also start without error. t.Setenv("FUNC_ENABLE_MCP_WRITE", "true") server = mock.NewMCPServer() - server.StartFn = func(_ context.Context, writeEnabled bool) error { - if !writeEnabled { - t.Fatal("MCP server was not enabled") - } - return nil - } cmd = NewMCPCmd(NewTestClient(fn.WithMCPServer(server))) cmd.SetArgs([]string{"start"}) if err := cmd.Execute(); err != nil { diff --git a/pkg/functions/client.go b/pkg/functions/client.go index 54b9c1d115..c373e92b30 100644 --- a/pkg/functions/client.go +++ b/pkg/functions/client.go @@ -221,7 +221,7 @@ type PipelinesProvider interface { // MCPServer for a given client instance which performs bidirectional // communication with a client agent. type MCPServer interface { - Start(context.Context, bool) error + Start(context.Context) error } // New client for function management. @@ -1247,8 +1247,8 @@ func (c *Client) Push(ctx context.Context, f Function) (Function, bool, error) { // StartMCPServer is currently a passthrough to the configured MCP Server // instance. -func (c *Client) StartMCPServer(ctx context.Context, writeEnabled bool) error { - return c.mcpServer.Start(ctx, writeEnabled) +func (c *Client) StartMCPServer(ctx context.Context) error { + return c.mcpServer.Start(ctx) } // ensureRunDataDir creates a .func directory at the given path, and @@ -1536,4 +1536,4 @@ func (n *noopDNSProvider) Provide(_ Function) error { return nil } // MCPServer type noopMCPServer struct{} -func (n *noopMCPServer) Start(_ context.Context, _ bool) error { return nil } +func (n *noopMCPServer) Start(_ context.Context) error { return nil } diff --git a/pkg/functions/client_test.go b/pkg/functions/client_test.go index c950ec4f86..85b1fb4450 100644 --- a/pkg/functions/client_test.go +++ b/pkg/functions/client_test.go @@ -1286,7 +1286,7 @@ func TestClient_StartMCPServer(t *testing.T) { client := fn.New(fn.WithMCPServer(server)) - if err := client.StartMCPServer(t.Context(), true); err != nil { + if err := client.StartMCPServer(t.Context()); err != nil { t.Fatal(err) } diff --git a/pkg/mcp/mcp.go b/pkg/mcp/mcp.go index a0c14496e2..59143cb967 100644 --- a/pkg/mcp/mcp.go +++ b/pkg/mcp/mcp.go @@ -144,9 +144,9 @@ func New(options ...Option) *Server { return s } -// Start the MCP server using the configured transport -func (s *Server) Start(ctx context.Context, writeEnabled bool) error { - s.readonly = !writeEnabled +// Start the MCP server using the configured transport. +// Readonly mode must be configured before calling Start via WithReadonly in New. +func (s *Server) Start(ctx context.Context) error { return s.impl.Run(ctx, s.transport) } diff --git a/pkg/mcp/mcp_test.go b/pkg/mcp/mcp_test.go index 67a4fd22c2..ac7afe178b 100644 --- a/pkg/mcp/mcp_test.go +++ b/pkg/mcp/mcp_test.go @@ -84,7 +84,7 @@ func newTestPairCore(t *testing.T, readonly bool, options ...Option) (session *m // Start the Server go func() { - errCh <- server.Start(t.Context(), !readonly) + errCh <- server.Start(t.Context()) }() // Connect a client to trigger initialization diff --git a/pkg/mock/mcp_server.go b/pkg/mock/mcp_server.go index f1fe6ce3c0..fe6839e137 100644 --- a/pkg/mock/mcp_server.go +++ b/pkg/mock/mcp_server.go @@ -6,16 +6,16 @@ import ( type MCPServer struct { StartInvoked bool - StartFn func(context.Context, bool) error + StartFn func(context.Context) error } func NewMCPServer() *MCPServer { return &MCPServer{ - StartFn: func(context.Context, bool) error { return nil }, + StartFn: func(context.Context) error { return nil }, } } -func (s *MCPServer) Start(ctx context.Context, writeEnabled bool) error { +func (s *MCPServer) Start(ctx context.Context) error { s.StartInvoked = true - return s.StartFn(ctx, writeEnabled) + return s.StartFn(ctx) }