From 05e0f90c3c5eaba529ddcf2ceafbefa720bb1736 Mon Sep 17 00:00:00 2001 From: ChrisJBurns <29541485+ChrisJBurns@users.noreply.github.com> Date: Mon, 14 Apr 2025 23:02:47 +0100 Subject: [PATCH] feat: adds new json editor layer This commit adds a new editor layer that is more robust and less invasive when editing client config files. We now use a combination of json patches and targetting segment retrievals to avoid the failings of the JSON-CC format causing errors with the standard json marhshalling functions. Signed-off-by: ChrisJBurns <29541485+ChrisJBurns@users.noreply.github.com> --- cmd/thv/app/config.go | 5 +- cmd/thv/app/rm.go | 3 +- go.mod | 5 + go.sum | 10 + pkg/client/config.go | 281 ++++++---------------------- pkg/client/config_editor.go | 197 ++++++++++++++++++++ pkg/client/config_editor_test.go | 226 +++++++++++++++++++++++ pkg/client/config_test.go | 307 +------------------------------ pkg/client/editor.go | 181 ------------------ pkg/runner/runner.go | 5 +- 10 files changed, 501 insertions(+), 719 deletions(-) create mode 100644 pkg/client/config_editor.go create mode 100644 pkg/client/config_editor_test.go delete mode 100644 pkg/client/editor.go diff --git a/cmd/thv/app/config.go b/cmd/thv/app/config.go index 4d6f7954ad..a379686d08 100644 --- a/cmd/thv/app/config.go +++ b/cmd/thv/app/config.go @@ -11,6 +11,7 @@ import ( "github.com/StacklokLabs/toolhive/pkg/container" rt "github.com/StacklokLabs/toolhive/pkg/container/runtime" "github.com/StacklokLabs/toolhive/pkg/labels" + "github.com/StacklokLabs/toolhive/pkg/logger" "github.com/StacklokLabs/toolhive/pkg/secrets" ) @@ -297,8 +298,8 @@ func addRunningMCPsToClient(clientName string) error { // Update each configuration file for _, clientConfig := range clientConfigs { // Update the MCP server configuration with locking - if err := clientConfig.SaveWithLock(name, url, clientConfig.Editor); err != nil { - fmt.Printf("Warning: Failed to update MCP server configuration in %s: %v\n", clientConfig.Path, err) + if err := client.Upsert(clientConfig, name, url); err != nil { + logger.Log.Warn(fmt.Sprintf("Warning: Failed to update MCP server configuration in %s: %v", clientConfig.Path, err)) continue } diff --git a/cmd/thv/app/rm.go b/cmd/thv/app/rm.go index 57171dbd4c..31fdff0255 100644 --- a/cmd/thv/app/rm.go +++ b/cmd/thv/app/rm.go @@ -137,8 +137,7 @@ func removeClientConfigurations(containerName string) error { for _, c := range configs { logger.Log.Info(fmt.Sprintf("Removing MCP server from client configuration: %s", c.Path)) - // Remove the MCP server configuration with locking - if err := c.DeleteConfigWithLock(containerName, c.Editor); err != nil { + if err := c.ConfigUpdater.Remove(containerName); err != nil { logger.Log.Warn(fmt.Sprintf("Warning: Failed to remove MCP server from client configurationn %s: %v", c.Path, err)) continue } diff --git a/go.mod b/go.mod index 974bb39705..cd83683fbb 100644 --- a/go.mod +++ b/go.mod @@ -12,12 +12,15 @@ require ( github.com/google/uuid v1.6.0 github.com/lmittmann/tint v1.0.7 github.com/stretchr/testify v1.10.0 + github.com/tailscale/hujson v0.0.0-20250226034555-ec1d1c113d33 + github.com/tidwall/gjson v1.18.0 github.com/zalando/go-keyring v0.2.6 golang.org/x/exp/jsonrpc2 v0.0.0-20250305212735-054e65f0b394 golang.org/x/mod v0.24.0 golang.org/x/sync v0.13.0 golang.org/x/term v0.31.0 gopkg.in/yaml.v3 v3.0.1 + gotest.tools v2.2.0+incompatible k8s.io/api v0.32.3 k8s.io/apimachinery v0.32.3 ) @@ -48,6 +51,8 @@ require ( github.com/mxk/go-flowrate v0.0.0-20140419014527-cca7078d478f // indirect github.com/russross/blackfriday/v2 v2.1.0 // indirect github.com/stretchr/objx v0.5.2 // indirect + github.com/tidwall/match v1.1.1 // indirect + github.com/tidwall/pretty v1.2.0 // indirect github.com/x448/float16 v0.8.4 // indirect go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.35.0 // indirect golang.org/x/exp/event v0.0.0-20220217172124-1812c5b45e43 // indirect diff --git a/go.sum b/go.sum index e84225d68b..acaabb65cb 100644 --- a/go.sum +++ b/go.sum @@ -171,6 +171,14 @@ github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO github.com/stretchr/testify v1.8.1/go.mod h1:w2LPCIKwWwSfY2zedu0+kehJoqGctiVI29o6fzry7u4= github.com/stretchr/testify v1.10.0 h1:Xv5erBjTwe/5IxqUQTdXv5kgmIvbHo3QQyRwhJsOfJA= github.com/stretchr/testify v1.10.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY= +github.com/tailscale/hujson v0.0.0-20250226034555-ec1d1c113d33 h1:idh63uw+gsG05HwjZsAENCG4KZfyvjK03bpjxa5qRRk= +github.com/tailscale/hujson v0.0.0-20250226034555-ec1d1c113d33/go.mod h1:EbW0wDK/qEUYI0A5bqq0C2kF8JTQwWONmGDBbzsxxHo= +github.com/tidwall/gjson v1.18.0 h1:FIDeeyB800efLX89e5a8Y0BNH+LOngJyGrIWxG2FKQY= +github.com/tidwall/gjson v1.18.0/go.mod h1:/wbyibRr2FHMks5tjHJ5F8dMZh3AcwJEMf5vlfC0lxk= +github.com/tidwall/match v1.1.1 h1:+Ho715JplO36QYgwN9PGYNhgZvoUSc9X2c80KVTi+GA= +github.com/tidwall/match v1.1.1/go.mod h1:eRSPERbgtNPcGhD8UCthc6PmLEQXEWd3PRB5JTxsfmM= +github.com/tidwall/pretty v1.2.0 h1:RWIZEg2iJ8/g6fDDYzMpobmaoGh5OLl4AXtGUGPcqCs= +github.com/tidwall/pretty v1.2.0/go.mod h1:ITEVvHYasfjBbM0u2Pg8T2nJnzm8xPwvNhhsoaGGjNU= github.com/x448/float16 v0.8.4 h1:qLwI1I70+NjRFUR3zs1JPUCgaCXSh3SW62uAKT1mSBM= github.com/x448/float16 v0.8.4/go.mod h1:14CWIYCyZA/cWjXOioeEpHeN/83MdbZDRQHoFcYsOfg= github.com/yuin/goldmark v1.1.27/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74= @@ -267,6 +275,8 @@ gopkg.in/inf.v0 v0.9.1/go.mod h1:cWUDdTG/fYaXco+Dcufb5Vnc6Gp2YChqWtbxRZE0mXw= gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= +gotest.tools v2.2.0+incompatible h1:VsBPFP1AI068pPrMxtb/S8Zkgf9xEmTLJjfM+P5UIEo= +gotest.tools v2.2.0+incompatible/go.mod h1:DsYFclhRJ6vuDpmuTbkuFWG+y2sxOXAzmJt81HFBacw= gotest.tools/v3 v3.5.2 h1:7koQfIKdy+I8UTetycgUqXWSDwpgv193Ka+qRsmBY8Q= gotest.tools/v3 v3.5.2/go.mod h1:LtdLGcnqToBH83WByAAi/wiwSFCArdFIUV/xxN4pcjA= k8s.io/api v0.32.3 h1:Hw7KqxRusq+6QSplE3NYG4MBxZw1BZnq4aP4cJVINls= diff --git a/pkg/client/config.go b/pkg/client/config.go index 778cca9ce2..8eca5f203f 100644 --- a/pkg/client/config.go +++ b/pkg/client/config.go @@ -3,8 +3,6 @@ package client import ( - "context" - "encoding/json" "fmt" "os" "path/filepath" @@ -13,7 +11,7 @@ import ( "strings" "time" - "github.com/gofrs/flock" + "github.com/tailscale/hujson" "gopkg.in/yaml.v3" "github.com/StacklokLabs/toolhive/pkg/config" @@ -37,8 +35,9 @@ func IsYAML(ext string) bool { // TODO: This type could be removed with more refactoring. type pathAndEditor struct { - Path string - Editor ConfigEditor + Path string + MCPServersPathPrefix string + ClientType MCPClient } // MCPClient is an enum of supported MCP clients. @@ -57,11 +56,11 @@ const ( // mcpClientConfig represents a configuration path for a supported MCP client. type mcpClientConfig struct { - ClientType MCPClient - Description string - RelPath []string - PlatformPrefix map[string][]string - Editor ConfigEditor + ClientType MCPClient + Description string + RelPath []string + PlatformPrefix map[string][]string + MCPServersPathPrefix string } var supportedClientIntegrations = []mcpClientConfig{ @@ -75,7 +74,7 @@ var supportedClientIntegrations = []mcpClientConfig{ "linux": {".config"}, "darwin": {"Library", "Application Support"}, }, - Editor: &StandardConfigEditor{}, + MCPServersPathPrefix: "/mcpServers", }, { ClientType: VSCodeInsider, @@ -87,7 +86,7 @@ var supportedClientIntegrations = []mcpClientConfig{ "linux": {".config"}, "darwin": {"Library", "Application Support"}, }, - Editor: &VSCodeConfigEditor{}, + MCPServersPathPrefix: "/mcp/servers", }, { ClientType: VSCode, @@ -95,25 +94,27 @@ var supportedClientIntegrations = []mcpClientConfig{ RelPath: []string{ "Code", "User", "settings.json", }, + MCPServersPathPrefix: "/mcp/servers", PlatformPrefix: map[string][]string{ "linux": {".config"}, "darwin": {"Library", "Application Support"}, }, - Editor: &VSCodeConfigEditor{}, }, { - ClientType: Cursor, - Description: "Cursor editor", - RelPath: []string{".cursor", "mcp.json"}, - Editor: &StandardConfigEditor{}, + ClientType: Cursor, + Description: "Cursor editor", + MCPServersPathPrefix: "/mcpServers", + RelPath: []string{".cursor", "mcp.json"}, }, } // ConfigFile represents a client configuration file type ConfigFile struct { - Path string - Contents map[string]interface{} - Editor ConfigEditor + Path string + ClientType MCPClient + Contents map[string]interface{} + ConfigUpdater ConfigUpdater + MCPServersPathPrefix string } // MCPServerConfig represents an MCP server configuration in a client config file @@ -148,12 +149,16 @@ func FindClientConfigs() ([]ConfigFile, error) { } var configs []ConfigFile + // Check each path for _, pe := range configPaths { - clientConfig, err := readConfigFile(pe.Path) + // TODO: This is a bit of a hack to get the client type into the ConfigFile + // object. We should probably refactor this to be more elegant. + // We can also rename the `readConfigFile` function so that it expresses that it + // only retrieves client config file metadata, not the contents. + clientConfig, err := readConfigFile(pe.Path, pe.MCPServersPathPrefix) + clientConfig.ClientType = pe.ClientType if err == nil { - // ugly hack, refactor away in future. - clientConfig.Editor = pe.Editor configs = append(configs, clientConfig) } @@ -165,8 +170,23 @@ func FindClientConfigs() ([]ConfigFile, error) { return configs, nil } +// Upsert updates/inserts an MCP server in a client configuration file +// It is a wrapper around the ConfigUpdater.Upsert method. Because the +// ConfigUpdater is different for each client type, we need to handle +// the different types of McpServer objects. For example, VSCode allows +// for a `type` field, but Cursor and others do not. This allows us to +// build up more complex MCP server configurations for different clients +// without leaking them into the CMD layer. +func Upsert(cf ConfigFile, name string, url string) error { + if cf.ClientType == VSCode || cf.ClientType == VSCodeInsider { + return cf.ConfigUpdater.Upsert(name, MCPServer{Url: url, Type: "sse"}) + } + + return cf.ConfigUpdater.Upsert(name, MCPServer{Url: url}) +} + // readConfigFile reads and parses a client configuration file -func readConfigFile(path string) (ConfigFile, error) { +func readConfigFile(path, mcpServersPathPrefix string) (ConfigFile, error) { // Check if file exists if _, err := os.Stat(path); os.IsNotExist(err) { return ConfigFile{}, fmt.Errorf("file does not exist: %s", path) @@ -182,6 +202,7 @@ func readConfigFile(path string) (ConfigFile, error) { // Determine format based on file extension var contents map[string]interface{} ext := strings.ToLower(filepath.Ext(path)) + var configUpdater ConfigUpdater if IsYAML(ext) { // Parse YAML @@ -190,213 +211,20 @@ func readConfigFile(path string) (ConfigFile, error) { } } else { // Default to JSON - if err := json.Unmarshal(data, &contents); err != nil { + _, err := hujson.Parse(data) + if err != nil { return ConfigFile{}, fmt.Errorf("failed to parse JSON: %w", err) } + configUpdater = &JSONConfigUpdater{Path: cleanpath, MCPServersPathPrefix: mcpServersPathPrefix} } return ConfigFile{ - Path: path, - Contents: contents, + Path: path, + Contents: contents, + ConfigUpdater: configUpdater, }, nil } -// UpdateMCPServerConfig updates the MCP server configuration in memory -// This does not save the changes to the file -func (c *ConfigFile) UpdateMCPServerConfig(serverName, url string) error { - // Get mcpServers object - mcpServers, ok := c.Contents["mcpServers"] - if !ok { - // Create mcpServers object if it doesn't exist - c.Contents["mcpServers"] = make(map[string]interface{}) - mcpServers = c.Contents["mcpServers"] - } - - // Convert to map - mcpServersMap, ok := mcpServers.(map[string]interface{}) - if !ok { - return fmt.Errorf("mcpServers is not a map") - } - - // Check if the server already exists - existingConfig, exists := mcpServersMap[serverName] - if exists { - // Update only the URL field and preserve all other fields - existingConfigMap, ok := existingConfig.(map[string]interface{}) - if ok { - // Update the URL field - existingConfigMap["url"] = url - // Keep the existing config - mcpServersMap[serverName] = existingConfigMap - } else { - // If the existing config is not a map, replace it - mcpServersMap[serverName] = map[string]interface{}{ - "url": url, - } - } - } else { - // Create a new server config - mcpServersMap[serverName] = map[string]interface{}{ - "url": url, - } - } - - return nil -} - -// Save writes the updated configuration back to the file without locking -// This is unsafe for concurrent access and should only be used in tests -func (c *ConfigFile) Save() error { - // Determine format based on file extension - ext := strings.ToLower(filepath.Ext(c.Path)) - - var data []byte - var err error - - if IsYAML(ext) { - // Marshal YAML - data, err = yaml.Marshal(c.Contents) - if err != nil { - return fmt.Errorf("failed to marshal YAML: %w", err) - } - } else { - // Default to JSON - data, err = json.MarshalIndent(c.Contents, "", " ") - if err != nil { - return fmt.Errorf("failed to marshal JSON: %w", err) - } - } - - // Write file - if err := os.WriteFile(c.Path, data, 0600); err != nil { - return fmt.Errorf("failed to write file: %w", err) - } - - return nil -} - -// SaveWithLock safely updates the MCP server configuration in the file -// It acquires a lock, reads the latest content, applies the change, and saves the file -func (c *ConfigFile) SaveWithLock(serverName, url string, editor ConfigEditor) error { - // Create a lock file - fileLock := flock.New(c.Path + ".lock") - - // Create a context with timeout - ctx, cancel := context.WithTimeout(context.Background(), lockTimeout) - defer cancel() - - // Try to acquire the lock with a timeout - locked, err := fileLock.TryLockContext(ctx, 100*time.Millisecond) - if err != nil { - return fmt.Errorf("failed to acquire lock: %w", err) - } - if !locked { - return fmt.Errorf("failed to acquire lock: timeout after %v", lockTimeout) - } - defer fileLock.Unlock() - - // Read the latest content from the file - latestConfig, err := readConfigFile(c.Path) - if err != nil { - return fmt.Errorf("failed to read latest config: %w", err) - } - - // Apply our change to the latest content - if err := editor.AddServer(&latestConfig, serverName, url); err != nil { - return fmt.Errorf("failed to update latest config: %w", err) - } - - // Determine format based on file extension - ext := strings.ToLower(filepath.Ext(c.Path)) - - var data []byte - - if IsYAML(ext) { - // Marshal YAML - data, err = yaml.Marshal(latestConfig.Contents) - if err != nil { - return fmt.Errorf("failed to marshal YAML: %w", err) - } - } else { - // Default to JSON - data, err = json.MarshalIndent(latestConfig.Contents, "", " ") - if err != nil { - return fmt.Errorf("failed to marshal JSON: %w", err) - } - } - - // Write file - if err := os.WriteFile(c.Path, data, 0600); err != nil { - return fmt.Errorf("failed to write file: %w", err) - } - - // Update our in-memory representation to match the file - c.Contents = latestConfig.Contents - - return nil -} - -// DeleteConfigWithLock safely removes the MCP server configuration in the file -// It acquires a lock, reads the latest content, applies the change, and saves the file -func (c *ConfigFile) DeleteConfigWithLock(serverName string, editor ConfigEditor) error { - // Create a lock file - fileLock := flock.New(c.Path + ".lock") - - // Create a context with timeout - ctx, cancel := context.WithTimeout(context.Background(), lockTimeout) - defer cancel() - - // Try to acquire the lock with a timeout - locked, err := fileLock.TryLockContext(ctx, 100*time.Millisecond) - if err != nil { - return fmt.Errorf("failed to acquire lock: %w", err) - } - if !locked { - return fmt.Errorf("failed to acquire lock: timeout after %v", lockTimeout) - } - defer fileLock.Unlock() - - // Read the latest content from the file - latestConfig, err := readConfigFile(c.Path) - if err != nil { - return fmt.Errorf("failed to read latest config: %w", err) - } - - // Apply our change to the latest content - if err := editor.RemoveServer(&latestConfig, serverName); err != nil { - return fmt.Errorf("failed to update latest config: %w", err) - } - - // Determine format based on file extension - ext := strings.ToLower(filepath.Ext(c.Path)) - - var data []byte - - if IsYAML(ext) { - // Marshal YAML - data, err = yaml.Marshal(latestConfig.Contents) - if err != nil { - return fmt.Errorf("failed to marshal YAML: %w", err) - } - } else { - // Default to JSON - data, err = json.MarshalIndent(latestConfig.Contents, "", " ") - if err != nil { - return fmt.Errorf("failed to marshal JSON: %w", err) - } - } - - // Write file - if err := os.WriteFile(c.Path, data, 0600); err != nil { - return fmt.Errorf("failed to write file: %w", err) - } - - // Update our in-memory representation to match the file - c.Contents = latestConfig.Contents - - return nil -} - // GenerateMCPServerURL generates the URL for an MCP server func GenerateMCPServerURL(host string, port int, containerName string) string { // The URL format is: http://host:port/sse#container-name @@ -423,9 +251,12 @@ func getSupportedPaths(filters []MCPClient) ([]pathAndEditor, error) { path = append(path, prefix...) } path = append(path, cfg.RelPath...) + // TODO: This is a bit of a hack to get the client type into the pathAndEditor + // object. We should probably refactor this to be more elegant. paths = append(paths, pathAndEditor{ - Path: filepath.Join(path...), - Editor: cfg.Editor, + Path: filepath.Join(path...), + MCPServersPathPrefix: cfg.MCPServersPathPrefix, + ClientType: cfg.ClientType, }) } diff --git a/pkg/client/config_editor.go b/pkg/client/config_editor.go new file mode 100644 index 0000000000..cefc8d79c8 --- /dev/null +++ b/pkg/client/config_editor.go @@ -0,0 +1,197 @@ +package client + +import ( + "context" + "encoding/json" + "fmt" + "os" + "strings" + "time" + + "github.com/gofrs/flock" + "github.com/tailscale/hujson" + "github.com/tidwall/gjson" + + "github.com/StacklokLabs/toolhive/pkg/logger" +) + +// ConfigUpdater defines the interface for types which can edit MCP client config files. +type ConfigUpdater interface { + Upsert(serverName string, data MCPServer) error + Remove(serverName string) error +} + +// MCPServer represents an MCP server in a MCP client config file +type MCPServer struct { + Url string `json:"url,omitempty"` + Type string `json:"type,omitempty"` +} + +// JSONConfigUpdater is a ConfigUpdater that is responsible for updating +// JSON config files. +type JSONConfigUpdater struct { + Path string + MCPServersPathPrefix string +} + +// Upsert inserts or updates an MCP server in the MCP client config file +func (jcu *JSONConfigUpdater) Upsert(serverName string, data MCPServer) error { + // Create a lock file + fileLock := flock.New(jcu.Path + ".lock") + + // Create a context with timeout + ctx, cancel := context.WithTimeout(context.Background(), lockTimeout) + defer cancel() + + // Try to acquire the lock with a timeout + locked, err := fileLock.TryLockContext(ctx, 100*time.Millisecond) + if err != nil { + return fmt.Errorf("failed to acquire lock: %w", err) + } + if !locked { + return fmt.Errorf("failed to acquire lock: timeout after %v", lockTimeout) + } + defer fileLock.Unlock() + + content, err := os.ReadFile(jcu.Path) + if err != nil { + logger.Log.Error("Failed to read file: %v", err) + } + + content = ensurePathExists(content, jcu.MCPServersPathPrefix) + + v, _ := hujson.Parse(content) + + dataJSON, err := json.Marshal(data) + if err != nil { + logger.Log.Error("Unable to marshal the MCPServer into JSON: %v", err) + } + + patch := fmt.Sprintf(`[{ "op": "add", "path": "%s/%s", "value": %s } ]`, jcu.MCPServersPathPrefix, serverName, dataJSON) + err = v.Patch([]byte(patch)) + if err != nil { + logger.Log.Error("Failed to patch file: %v", err) + } + + formatted, _ := hujson.Format(v.Pack()) + if err != nil { + logger.Log.Error("Failed to format the patched file: %v", err) + } + + // Write back to the file + if err := os.WriteFile(jcu.Path, formatted, 0600); err != nil { + logger.Log.Error("Failed to write file: %v", err) + } + + logger.Log.Info("Successfully updated the client config file for MCPServer %s", serverName) + + return nil +} + +// Remove removes an MCP server from the MCP client config file +func (jcu *JSONConfigUpdater) Remove(serverName string) error { + // Create a lock file + fileLock := flock.New(jcu.Path + ".lock") + + // Create a context with timeout + ctx, cancel := context.WithTimeout(context.Background(), lockTimeout) + defer cancel() + + // Try to acquire the lock with a timeout + locked, err := fileLock.TryLockContext(ctx, 100*time.Millisecond) + if err != nil { + return fmt.Errorf("failed to acquire lock: %w", err) + } + if !locked { + return fmt.Errorf("failed to acquire lock: timeout after %v", lockTimeout) + } + defer fileLock.Unlock() + + content, err := os.ReadFile(jcu.Path) + if err != nil { + logger.Log.Error("Failed to read file: %v", err) + } + + v, _ := hujson.Parse(content) + + patch := fmt.Sprintf(`[{ "op": "remove", "path": "%s/%s" } ]`, jcu.MCPServersPathPrefix, serverName) + err = v.Patch([]byte(patch)) + if err != nil { + logger.Log.Error("Failed to patch file: %v", err) + } + + formatted, _ := hujson.Format(v.Pack()) + + // Write back to the file + if err := os.WriteFile(jcu.Path, formatted, 0600); err != nil { + logger.Log.Error("Failed to write file: %v", err) + } + + logger.Log.Info("Successfully removed the MCPServer %s from the client config file", serverName) + + return nil +} + +// ensurePathExists ensures that the path exists in the JSON content +// and returns the updated content. +// For example: +// - if the path is "/mcp/servers", +// the function will ensure that the path "/mcp/servers" exists +// and returns the updated content. +// - if the path is "/mcpServers", +// the function will ensure that the path "/mcpServers" exists +// and returns the updated content. +// +// This is necessary because the MCP client config file is a JSON object, +// and we need to ensure that the path exists before we can add a new key to it. +func ensurePathExists(content []byte, path string) []byte { + segments := strings.Split(path, "/") + + // Navigate through the JSON structure + var pathSoFarForPatch string + var pathSoFarForRetrieval string + for i, segment := range segments[:] { + // we want to skip the first segments because it is the root + if path[0] == '/' && (i == 0) { + continue + } + + // We build the path up to this segment so that we can check if it exists + // and if it doesn't, we can create it as an empty object. + // The "/" is added to the path for the patch operation because the path + // is a JSON pointer, and JSON pointers are prefixed with "/". + // The "." is added to the path for the retrieval operation. + if len(pathSoFarForPatch) == 0 { + pathSoFarForPatch = "/" + segment + pathSoFarForRetrieval = segment + } else { + pathSoFarForPatch = pathSoFarForPatch + "/" + segment + pathSoFarForRetrieval = pathSoFarForRetrieval + "." + segment + } + + // We retrieve the segment from the content so that we can check if it exists + // and if it doesn't, we can create it as an empty object. If it does exist, + // we can skip the patch operation onto the next segment. + segmentPath := gjson.GetBytes(content, pathSoFarForRetrieval).Raw + if segmentPath != "" { + continue + } + + // Create a JSON patch to add an empty object at this path + patch := fmt.Sprintf(`[{ "op": "add", "path": "%s", "value": {} }]`, pathSoFarForPatch) + + // Parse the current content and apply the patch + v, _ := hujson.Parse(content) + err := v.Patch([]byte(patch)) + if err != nil { + logger.Log.Error("Failed to patch file: %v", err) + } + + // Update the content with the patched version + content = v.Pack() + } + // Parse the updated content with hujson to maintain formatting + v, _ := hujson.Parse(content) + formatted, _ := hujson.Format(v.Pack()) + return formatted +} diff --git a/pkg/client/config_editor_test.go b/pkg/client/config_editor_test.go new file mode 100644 index 0000000000..20832d6988 --- /dev/null +++ b/pkg/client/config_editor_test.go @@ -0,0 +1,226 @@ +package client + +import ( + "encoding/json" + "fmt" + "log" + "os" + "path/filepath" + "testing" + + "github.com/google/uuid" + "github.com/tidwall/gjson" + "gotest.tools/assert" + + "github.com/StacklokLabs/toolhive/pkg/logger" +) + +func TestUpsertMCPServerConfig(t *testing.T) { + t.Parallel() + + logger.Initialize() + + tests := []struct { + mcpServerPatchPath string // the path used by the patch operation + mcpServerKeyPath string // the path used to retrieve the value from the config file (for testing purposes) + mcpServerName string // the name of the MCP server to remove + }{ + {mcpServerPatchPath: "/mcp/servers", mcpServerKeyPath: "mcp.servers", mcpServerName: "testMcpServerUpdate"}, + {mcpServerPatchPath: "/mcpServers", mcpServerKeyPath: "mcpServers", mcpServerName: "testMcpServerUpdate"}, + } + + for _, tt := range tests { + + t.Run("AddNewMCPServer", func(t *testing.T) { + t.Parallel() + + uniqueId := uuid.New().String() + tempDir, configPath := setupEmptyTestConfig(t, uniqueId) + + jsu := JSONConfigUpdater{ + Path: configPath, + MCPServersPathPrefix: tt.mcpServerPatchPath, + } + + mcpServer := MCPServer{ + Url: fmt.Sprintf("test-url-%s", uniqueId), + } + + err := jsu.Upsert(tt.mcpServerName, mcpServer) + if err != nil { + t.Fatalf("Failed to update config: %v", err) + } + + testMcpServer := getMCPServerFromFile(t, configPath, tt.mcpServerKeyPath+"."+tt.mcpServerName) + + assert.Equal(t, mcpServer.Url, testMcpServer.Url, "The retrieved value should match the set value") + + t.Cleanup(func() { + if err := os.RemoveAll(tempDir); err != nil { + t.Logf("Failed to remove temp dir: %v", err) + } + }) + }) + } + + // Run subtests + + for _, tt := range tests { + + t.Run("UpdateExistingMCPServer", func(t *testing.T) { + t.Parallel() + + uniqueId := uuid.New().String() + tempDir, configPath := setupEmptyTestConfig(t, uniqueId) + + jsu := JSONConfigUpdater{ + Path: configPath, + MCPServersPathPrefix: tt.mcpServerPatchPath, + } + + // add an MCP server so we can update it + mcpServer := MCPServer{ + Url: fmt.Sprintf("test-url-%s-before-update", uniqueId), + } + err := jsu.Upsert(tt.mcpServerName, mcpServer) + if err != nil { + t.Fatalf("Failed to add mcp server to config: %v", err) + } + testMcpServer := getMCPServerFromFile(t, configPath, tt.mcpServerKeyPath+"."+tt.mcpServerName) + assert.Equal(t, mcpServer.Url, testMcpServer.Url, "The retrieved value should match the set value") + + // now we update the mcp server + mcpServerUpdated := MCPServer{ + Url: fmt.Sprintf("test-url-%s-after-update", uniqueId), + } + err = jsu.Upsert(tt.mcpServerName, mcpServerUpdated) + if err != nil { + t.Fatalf("Failed to update mcp server inconfig: %v", err) + } + // we make sure to get the same mcp server that we created and then updated + testMcpServerUpdate := getMCPServerFromFile(t, configPath, tt.mcpServerKeyPath+"."+tt.mcpServerName) + assert.Equal(t, mcpServerUpdated.Url, testMcpServerUpdate.Url, "The retrieved value should match the set value") + + if err != nil { + t.Fatalf("Failed to update config: %v", err) + } + + t.Cleanup(func() { + if err := os.RemoveAll(tempDir); err != nil { + t.Logf("Failed to remove temp dir: %v", err) + } + }) + }) + } +} + +func TestRemoveMCPServerConfigNew(t *testing.T) { + t.Parallel() + + logger.Initialize() + + tests := []struct { + mcpServerPatchPath string // the path used by the patch operation + mcpServerKeyPath string // the path used to retrieve the value from the config file (for testing purposes) + mcpServerName string // the name of the MCP server to remove + }{ + {mcpServerPatchPath: "/mcp/servers", mcpServerKeyPath: "mcp.servers", mcpServerName: "testMcpServerRemove"}, + {mcpServerPatchPath: "/mcpServers", mcpServerKeyPath: "mcpServers", mcpServerName: "testMcpServerRemove"}, + } + + for _, tt := range tests { + t.Run("DeleteMCPServer", func(t *testing.T) { + t.Parallel() + + uniqueId := uuid.New().String() + tempDir, configPath := setupEmptyTestConfig(t, uniqueId) + + jsu := JSONConfigUpdater{ + Path: configPath, + MCPServersPathPrefix: tt.mcpServerPatchPath, + } + + // add an MCP server so we can remove it + mcpServer := MCPServer{ + Url: fmt.Sprintf("test-url-%s-before-removal", uniqueId), + } + err := jsu.Upsert(tt.mcpServerName, mcpServer) + if err != nil { + t.Fatalf("Failed to add mcp server to config: %v", err) + } + testMcpServer := getMCPServerFromFile(t, configPath, tt.mcpServerKeyPath+"."+tt.mcpServerName) + assert.Equal(t, mcpServer.Url, testMcpServer.Url, "The retrieved value should match the set value") + + // remove both mcp servers + err = jsu.Remove(tt.mcpServerName) + if err != nil { + t.Fatalf("Failed to remove mcp server testMcpServer from config: %v", err) + } + + // read the config file and check that the mcp servers are removed + content, err := os.ReadFile(configPath) + if err != nil { + log.Fatalf("Failed to read file: %v", err) + } + + testMcpServerJson := gjson.GetBytes(content, tt.mcpServerKeyPath+"."+tt.mcpServerName).Raw + if testMcpServerJson != "" { + t.Fatalf("Failed to remove mcp server testMcpServer from config: %v", testMcpServerJson) + } + + t.Cleanup(func() { + if err := os.RemoveAll(tempDir); err != nil { + t.Logf("Failed to remove temp dir: %v", err) + } + }) + }) + } +} + +// setupEmptyTestConfig creates a temporary directory and an empty config file for testing +// It returns the temp directory path, config file path, and the loaded config +// The logs are created in "/var/folders/2k/jvn73p4d2nn_j6tvc40vj4r00000gn/T/toolhive-test4175700918/config-9f74ab6d-0b4e-4956-b818-315bf16aa803.json" +func setupEmptyTestConfig(t *testing.T, testName string) (string, string) { + t.Helper() + + // Create a temporary file + tempDir, err := os.MkdirTemp("", "toolhive-test") + if err != nil { + t.Fatalf("Failed to create temp dir: %v", err) + } + + // Create a test config file with existing MCP servers + configPath := filepath.Join(tempDir, fmt.Sprintf("config-%s.json", testName)) + testConfig := map[string]interface{}{} + + // // Write the test config to the file + data, err := json.MarshalIndent(testConfig, "", " ") + if err != nil { + t.Fatalf("Failed to marshal JSON: %v", err) + } + if err := os.WriteFile(configPath, data, 0600); err != nil { + t.Fatalf("Failed to write file: %v", err) + } + + return tempDir, configPath +} + +// getMCPServerFromFile reads the config file and returns a mcpServer object +func getMCPServerFromFile(t *testing.T, configPath string, key string) MCPServer { + t.Helper() + + content, err := os.ReadFile(configPath) + if err != nil { + t.Fatalf("Failed to read file: %v", err) + } + + testMcpServerJson := gjson.GetBytes(content, key).Raw + + var testMcpServer MCPServer + err = json.Unmarshal([]byte(testMcpServerJson), &testMcpServer) + if err != nil { + t.Fatalf("Failed to unmarshal JSON: %v", err) + } + + return testMcpServer +} diff --git a/pkg/client/config_test.go b/pkg/client/config_test.go index 1f9ec1ab70..2f5e0434f6 100644 --- a/pkg/client/config_test.go +++ b/pkg/client/config_test.go @@ -3,317 +3,12 @@ package client import ( - "encoding/json" - "fmt" - "os" - "path/filepath" "testing" - "github.com/StacklokLabs/toolhive/pkg/logger" "github.com/StacklokLabs/toolhive/pkg/transport/ssecommon" ) -// setupTestConfig creates a temporary directory and config file for testing -// It returns the temp directory path, config file path, and the loaded config -func setupTestConfig(t *testing.T, testName string) (string, string, ConfigFile) { - t.Helper() - - // Create a temporary file - tempDir, err := os.MkdirTemp("", "toolhive-test") - if err != nil { - t.Fatalf("Failed to create temp dir: %v", err) - } - - // Create a test config file with existing MCP servers - configPath := filepath.Join(tempDir, fmt.Sprintf("config-%s.json", testName)) - testConfig := map[string]interface{}{ - "mcpServers": map[string]interface{}{ - "existing-server": map[string]interface{}{ - "url": "http://localhost:12345/sse", - }, - "postgres": map[string]interface{}{ - "command": "node", - "args": []interface{}{ - "F://node_modules//node_modules//@modelcontextprotocol//server-postgres//dist//index.js", - "postgresql://postgres:postgres@localhost/novel", - }, - "alwaysAllow": []interface{}{ - "query", - }, - }, - }, - } - - // Write the test config to the file - data, err := json.MarshalIndent(testConfig, "", " ") - if err != nil { - t.Fatalf("Failed to marshal JSON: %v", err) - } - if err := os.WriteFile(configPath, data, 0600); err != nil { - t.Fatalf("Failed to write file: %v", err) - } - - // Read the config file - config, err := readConfigFile(configPath) - if err != nil { - t.Fatalf("Failed to read config file: %v", err) - } - - return tempDir, configPath, config -} - -// getMCPServers reads the config file and returns the mcpServers map -func getMCPServers(t *testing.T, configPath string, editor ConfigEditor) map[string]interface{} { - t.Helper() - - // Read the config file - updatedConfig, err := readConfigFile(configPath) - if err != nil { - t.Fatalf("Failed to read updated config file: %v", err) - } - - _, vsOk := editor.(*VSCodeConfigEditor) - if vsOk { - updatedConfig.Editor = editor - mcpMap, ok := updatedConfig.Contents["mcp"].(map[string]interface{}) - if !ok { - t.Fatalf("mcp is not a map") - } - - // Get servers child object - mcpServers, ok := mcpMap["servers"] - if !ok { - t.Fatalf("mcpServers is not a map") - } - return mcpServers.(map[string]interface{}) - } - - _, standardOk := editor.(*StandardConfigEditor) - if standardOk { - mcpServers, ok := updatedConfig.Contents["mcpServers"].(map[string]interface{}) - if !ok { - t.Fatalf("mcpServers is not a map") - } - return mcpServers - } - - return nil -} - -// testUpdateExistingServer tests updating an existing server -func testUpdateExistingServer(t *testing.T, config ConfigFile, configPath string) { - t.Helper() - // Test updating an existing server with lock - expectedURL := "http://localhost:54321" + ssecommon.HTTPSSEEndpoint + "#test-container" - editor := &StandardConfigEditor{} - - err := config.SaveWithLock("existing-server", expectedURL, editor) - if err != nil { - t.Fatalf("Failed to update MCP server config: %v", err) - } - - // Get the updated servers - mcpServers := getMCPServers(t, configPath, editor) - - // Check existing server - existingServer, ok := mcpServers["existing-server"].(map[string]interface{}) - if !ok { - t.Fatalf("existing-server is not a map") - } - existingURL, ok := existingServer["url"].(string) - if !ok { - t.Fatalf("url is not a string") - } - if existingURL != expectedURL { - t.Fatalf("Unexpected URL for existing-server: %s, expected: %s", existingURL, expectedURL) - } -} - -// testAddNewServer tests adding a new server -func testAddNewServer(t *testing.T, config ConfigFile, configPath string) { - t.Helper() - // Test adding a new server with lock - expectedURL := "http://localhost:9876" + ssecommon.HTTPSSEEndpoint + "#new-container" - editor := &StandardConfigEditor{} - err := config.SaveWithLock("new-server", expectedURL, editor) - if err != nil { - t.Fatalf("Failed to add new MCP server config: %v", err) - } - - // Get the updated servers - mcpServers := getMCPServers(t, configPath, editor) - - // Check new server - newServer, ok := mcpServers["new-server"].(map[string]interface{}) - if !ok { - t.Fatalf("new-server is not a map") - } - newURL, ok := newServer["url"].(string) - if !ok { - t.Fatalf("url is not a string") - } - if newURL != expectedURL { - t.Fatalf("Unexpected URL for new-server: %s, expected: %s", newURL, expectedURL) - } -} - -// testAddNewServer tests adding a new server -func testRemovingServer(t *testing.T, config ConfigFile, configPath string, editor ConfigEditor) { - t.Helper() - // Test adding a new server with lock - expectedURL := "http://localhost:9876" + ssecommon.HTTPSSEEndpoint + "#new-container" - err := config.SaveWithLock("new-server", expectedURL, editor) - if err != nil { - t.Fatalf("Failed to add new MCP server config: %v", err) - } - - // Get the updated servers - mcpServers := getMCPServers(t, configPath, editor) - - // Check new server - newServer, ok := mcpServers["new-server"].(map[string]interface{}) - if !ok { - t.Fatalf("new-server is not a map") - } - newURL, ok := newServer["url"].(string) - if !ok { - t.Fatalf("url is not a string") - } - if newURL != expectedURL { - t.Fatalf("Unexpected URL for new-server: %s, expected: %s", newURL, expectedURL) - } - - // Remove the server - err = config.DeleteConfigWithLock("new-server", editor) - if err != nil { - t.Fatalf("Failed to remove MCP server config: %v", err) - } - - mcpServersNew := getMCPServers(t, configPath, editor) - - // Check that the server was removed - _, ok = mcpServersNew["new-server"] - if ok { - t.Fatalf("new-server is still in the config") - } -} - -// testPreserveExistingConfig tests that existing configurations are preserved -func testPreserveExistingConfig(t *testing.T, configPath string) { - t.Helper() - // Get the updated servers - editor := &StandardConfigEditor{} - mcpServers := getMCPServers(t, configPath, editor) - - // Check postgres server (should be unchanged) - postgresServer, ok := mcpServers["postgres"].(map[string]interface{}) - if !ok { - t.Fatalf("postgres is not a map") - } - command, ok := postgresServer["command"].(string) - if !ok { - t.Fatalf("command is not a string") - } - if command != "node" { - t.Fatalf("Unexpected command for postgres: %s", command) - } - args, ok := postgresServer["args"].([]interface{}) - if !ok { - t.Fatalf("args is not a slice") - } - if len(args) != 2 { - t.Fatalf("Unexpected args length for postgres: %d", len(args)) - } - alwaysAllow, ok := postgresServer["alwaysAllow"].([]interface{}) - if !ok { - t.Fatalf("alwaysAllow is not a slice") - } - if len(alwaysAllow) != 1 || alwaysAllow[0].(string) != "query" { - t.Fatalf("Unexpected alwaysAllow for postgres: %v", alwaysAllow) - } -} - -func TestUpdateMCPServerConfig(t *testing.T) { - t.Parallel() - - // Run subtests - t.Run("UpdateExistingServer", func(t *testing.T) { - t.Parallel() - - // Setup test environment for this subtest - tempDir, configPath, config := setupTestConfig(t, "update") - t.Cleanup(func() { - if err := os.RemoveAll(tempDir); err != nil { - t.Logf("Failed to remove temp dir: %v", err) - } - }) - - testUpdateExistingServer(t, config, configPath) - }) - - t.Run("AddNewServer", func(t *testing.T) { - t.Parallel() - - // Setup test environment for this subtest - tempDir, configPath, config := setupTestConfig(t, "add") - t.Cleanup(func() { - if err := os.RemoveAll(tempDir); err != nil { - t.Logf("Failed to remove temp dir: %v", err) - } - }) - - testAddNewServer(t, config, configPath) - }) - - t.Run("PreserveExistingConfig", func(t *testing.T) { - t.Parallel() - - // Setup test environment for this subtest - tempDir, configPath, _ := setupTestConfig(t, "preserve") - t.Cleanup(func() { - if err := os.RemoveAll(tempDir); err != nil { - t.Logf("Failed to remove temp dir: %v", err) - } - }) - - testPreserveExistingConfig(t, configPath) - }) - -} - -func TestRemoveMCPServerConfig(t *testing.T) { - t.Parallel() - - logger.Initialize() - - t.Run("RemoveExistingServerStandardEditor", func(t *testing.T) { - t.Parallel() - - // Setup test environment for this subtest - tempDir, configPath, config := setupTestConfig(t, "remove") - t.Cleanup(func() { - if err := os.RemoveAll(tempDir); err != nil { - t.Logf("Failed to remove temp dir: %v", err) - } - }) - - testRemovingServer(t, config, configPath, &StandardConfigEditor{}) - }) - - t.Run("RemoveExistingServerVSCodeEditor", func(t *testing.T) { - t.Parallel() - - // Setup test environment for this subtest - tempDir, configPath, config := setupTestConfig(t, "remove") - t.Cleanup(func() { - if err := os.RemoveAll(tempDir); err != nil { - t.Logf("Failed to remove temp dir: %v", err) - } - }) - - testRemovingServer(t, config, configPath, &VSCodeConfigEditor{}) - }) -} +// TODO: Chris, add betters tests for config layer. func TestGenerateMCPServerURL(t *testing.T) { t.Parallel() diff --git a/pkg/client/editor.go b/pkg/client/editor.go deleted file mode 100644 index 9038a27688..0000000000 --- a/pkg/client/editor.go +++ /dev/null @@ -1,181 +0,0 @@ -package client - -import ( - "fmt" - - "github.com/StacklokLabs/toolhive/pkg/logger" -) - -// ConfigEditor defines the interface for types which can edit MCP client config files. -type ConfigEditor interface { - AddServer(config *ConfigFile, serverName, url string) error - RemoveServer(config *ConfigFile, serverName string) error -} - -// StandardConfigEditor edits the MCP client config format used by most clients. -type StandardConfigEditor struct{} - -// AddServer inserts or updates a server in the MCP client config file. -func (*StandardConfigEditor) AddServer(config *ConfigFile, serverName, url string) error { - // Get mcpServers object - mcpServers, ok := config.Contents["mcpServers"] - if !ok { - // Create mcpServers object if it doesn't exist - config.Contents["mcpServers"] = make(map[string]any) - mcpServers = config.Contents["mcpServers"] - } - - // Convert to map - mcpServersMap, ok := mcpServers.(map[string]any) - if !ok { - return fmt.Errorf("mcpServers is not a map") - } - - // Check if the server already exists - existingConfig, exists := mcpServersMap[serverName] - if exists { - // Update only the URL field and preserve all other fields - existingConfigMap, ok := existingConfig.(map[string]any) - if ok { - // Update the URL field - existingConfigMap["url"] = url - // Keep the existing config - mcpServersMap[serverName] = existingConfigMap - } else { - // If the existing config is not a map, replace it - mcpServersMap[serverName] = map[string]any{ - "url": url, - } - } - } else { - // Create a new server config - mcpServersMap[serverName] = map[string]any{ - "url": url, - } - } - - return nil -} - -// RemoveServer removes the specified MCP server from the Client config file. -func (*StandardConfigEditor) RemoveServer(config *ConfigFile, serverName string) error { - // Get mcpServers object - mcpServers, ok := config.Contents["mcpServers"] - if !ok { - // Create mcpServers object if it doesn't exist - config.Contents["mcpServers"] = make(map[string]any) - mcpServers = config.Contents["mcpServers"] - } - - // Convert to map - mcpServersMap, ok := mcpServers.(map[string]any) - if !ok { - return fmt.Errorf("mcpServers is not a map") - } - - // Check if the server already exists - _, exists := mcpServersMap[serverName] - if exists { - delete(mcpServersMap, serverName) - logger.Log.Info(fmt.Sprintf("Removed MCP server %s from config file %s", serverName, config.Path)) - } else { - logger.Log.Info(fmt.Sprintf("Nothing to do, MCP server %s was not found in config file %s", serverName, config.Path)) - } - - return nil -} - -// VSCodeConfigEditor edits the MCP client config format used by VS Code. -type VSCodeConfigEditor struct{} - -// AddServer inserts or updates a server in the MCP client config file. -func (*VSCodeConfigEditor) AddServer(config *ConfigFile, serverName, url string) error { - // TODO: This pattern of "descend through JSON and apply a diff" can be generalized. - // Get mcp object - mcp, ok := config.Contents["mcp"] - if !ok { - // Create mcp object if it doesn't exist - config.Contents["mcp"] = make(map[string]any) - mcp = config.Contents["mcp"] - } - mcpMap := mcp.(map[string]any) - - // Get servers child object - mcpServers, ok := mcpMap["servers"] - if !ok { - // Create servers object if it doesn't exist - (config.Contents["mcp"].(map[string]any))["servers"] = make(map[string]any) - mcpServers = (config.Contents["mcp"].(map[string]any))["servers"] - } - - // Convert to map - mcpServersMap, ok := mcpServers.(map[string]any) - if !ok { - return fmt.Errorf("mcpServers is not a map") - } - - // Check if the server already exists - existingConfig, exists := mcpServersMap[serverName] - if exists { - // Update only the URL field and preserve all other fields - existingConfigMap, ok := existingConfig.(map[string]any) - if ok { - // Update the URL field - existingConfigMap["url"] = url - // Keep the existing config - mcpServersMap[serverName] = existingConfigMap - } else { - // If the existing config is not a map, replace it - mcpServersMap[serverName] = map[string]any{ - "url": url, - "type": "sse", - } - } - } else { - // Create a new server config - mcpServersMap[serverName] = map[string]any{ - "url": url, - "type": "sse", - } - } - - return nil -} - -// RemoveServer removes the specified MCP server from the Client config file (VSCode) -func (*VSCodeConfigEditor) RemoveServer(config *ConfigFile, serverName string) error { - // TODO: This pattern of "descend through JSON and apply a diff" can be generalized. - // Get mcp object - mcp, ok := config.Contents["mcp"] - if !ok { - // Create mcp object if it doesn't exist - config.Contents["mcp"] = make(map[string]any) - mcp = config.Contents["mcp"] - } - mcpMap := mcp.(map[string]any) - - // Get servers child object - mcpServers, ok := mcpMap["servers"] - if !ok { - // Create servers object if it doesn't exist - (config.Contents["mcp"].(map[string]any))["servers"] = make(map[string]any) - mcpServers = (config.Contents["mcp"].(map[string]any))["servers"] - } - - // Convert to map - mcpServersMap, ok := mcpServers.(map[string]any) - if !ok { - return fmt.Errorf("mcpServers is not a map") - } - - // Check if the server already exists - _, exists := mcpServersMap[serverName] - if exists { - delete(mcpServersMap, serverName) - logger.Log.Info(fmt.Sprintf("Removed MCP server %s from config file %s", serverName, config.Path)) - } else { - logger.Log.Info(fmt.Sprintf("Nothing to do, MCP server %s was not found in config file %s", serverName, config.Path)) - } - - return nil -} diff --git a/pkg/runner/runner.go b/pkg/runner/runner.go index 392a96c195..992be3fc81 100644 --- a/pkg/runner/runner.go +++ b/pkg/runner/runner.go @@ -217,9 +217,8 @@ func updateClientConfigurations(containerName, host string, port int) error { for _, config := range configs { logger.Log.Info(fmt.Sprintf("Updating client configuration: %s", config.Path)) - // Update the MCP server configuration with locking - if err := config.SaveWithLock(containerName, url, config.Editor); err != nil { - logger.Log.Warn(fmt.Sprintf("Warning: Failed to update MCP server configuration in %s: %v", config.Path, err)) + if err := client.Upsert(config, containerName, url); err != nil { + fmt.Printf("Warning: Failed to update MCP server configuration in %s: %v\n", config.Path, err) continue }