fix(mcp): remove writeEnabled from Start() to prevent readonly state desync

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

cmd/mcp_test.go

@@ -1,7 +1,6 @@
 package cmd
 
 import (
-	"context"
 	"testing"
 
 	fn "knative.dev/func/pkg/functions"
@@ -30,37 +29,32 @@ 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 that the FUNC_ENABLE_MCP_WRITE environment
+// variable is correctly parsed and the server starts in both default
+// (readonly) and write-enabled modes.
 func TestMCP_StartWriteable(t *testing.T) {
 	_ = FromTempDirectory(t)
 
-	// Ensure it defaults to off.
+	// Ensure it defaults to readonly (no env var set).
 	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)
 	}
+	if !server.StartInvoked {
+		t.Fatal("MCP server was not started in default mode")
+	}
 
-	// Ensure it is set to true on proper truthy value
+	// Ensure it starts successfully with write mode enabled.
 	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 {
 		t.Fatal(err)
 	}
+	if !server.StartInvoked {
+		t.Fatal("MCP server was not started with write mode enabled")
+	}
 }

pkg/functions/client.go

@@ -227,7 +227,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.
@@ -1266,8 +1266,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
@@ -1555,4 +1555,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 }

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)
 	}
 
@@ -1295,6 +1295,15 @@ func TestClient_StartMCPServer(t *testing.T) {
 	}
 }
 
+// TestClient_StartMCPServer_Default ensures the default noop MCP server
+// does not error when no server is explicitly configured.
+func TestClient_StartMCPServer_Default(t *testing.T) {
+	client := fn.New() // no WithMCPServer — uses noopMCPServer
+	if err := client.StartMCPServer(t.Context()); err != nil {
+		t.Fatal(err)
+	}
+}
+
 // TestClient_List_OutsideRoot ensures that a call to a function (in this case list)
 // that is not contextually dependent on being associated with a function,
 // can be run from anywhere, thus ensuring that the client itself makes

pkg/mcp/mcp.go

@@ -157,9 +157,10 @@ 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.Store(!writeEnabled)
+// Start the MCP server using the configured transport.
+// The server's readonly mode is determined at construction time via
+// WithReadonly; it cannot be changed after the server is created.
+func (s *Server) Start(ctx context.Context) error {
 	return s.impl.Run(ctx, s.transport)
 }
 

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

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

pkg/mock/mcp_server_test.go

@@ -0,0 +1,21 @@
+package mock
+
+import (
+	"context"
+	"testing"
+)
+
+func TestMCPServer_Start(t *testing.T) {
+	s := NewMCPServer()
+	if s.StartInvoked {
+		t.Fatal("StartInvoked should be false before calling Start")
+	}
+
+	err := s.Start(context.Background())
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if !s.StartInvoked {
+		t.Fatal("StartInvoked should be true after calling Start")
+	}
+}