fix(mcp/lsp): expand variable in commands (#1116)

Carlos Alexandro Becker and taigr created 1 month ago

closes #806

Signed-off-by: Carlos Alexandro Becker <[e-mail]>
Co-authored-by: taigr <[e-mail]>

Change summary

internal/app/lsp.go             |  2 +-
internal/llm/agent/mcp-tools.go | 21 +++++++++++++--------
internal/lsp/client.go          |  9 +++++++--
internal/lsp/client_test.go     |  9 ++++++---
4 files changed, 27 insertions(+), 14 deletions(-)

Detailed changes

internal/app/lsp.go

@@ -36,7 +36,7 @@ func (app *App) createAndStartLSPClient(ctx context.Context, name string, config
 	updateLSPState(name, lsp.StateStarting, nil, nil, 0)
 
 	// Create LSP client.
-	lspClient, err := lsp.New(ctx, name, config)
+	lspClient, err := lsp.New(ctx, name, config, app.config.Resolver())
 	if err != nil {
 		slog.Error("Failed to create LSP client for", name, err)
 		updateLSPState(name, lsp.StateError, err, nil, 0)

internal/llm/agent/mcp-tools.go

@@ -149,7 +149,8 @@ func getOrRenewClient(ctx context.Context, name string) (*client.Client, error)
 		return nil, fmt.Errorf("mcp '%s' not available", name)
 	}
 
-	m := config.Get().MCP[name]
+	cfg := config.Get()
+	m := cfg.MCP[name]
 	state, _ := mcpStates.Get(name)
 
 	timeout := mcpTimeout(m)
@@ -161,7 +162,7 @@ func getOrRenewClient(ctx context.Context, name string) (*client.Client, error)
 	}
 	updateMCPState(name, MCPStateError, maybeTimeoutErr(err, timeout), nil, state.ToolCount)
 
-	c, err = createAndInitializeClient(ctx, name, m)
+	c, err = createAndInitializeClient(ctx, name, m, cfg.Resolver())
 	if err != nil {
 		return nil, err
 	}
@@ -313,7 +314,7 @@ func doGetMCPTools(ctx context.Context, permissions permission.Service, cfg *con
 
 			ctx, cancel := context.WithTimeout(ctx, mcpTimeout(m))
 			defer cancel()
-			c, err := createAndInitializeClient(ctx, name, m)
+			c, err := createAndInitializeClient(ctx, name, m, cfg.Resolver())
 			if err != nil {
 				return
 			}
@@ -328,8 +329,8 @@ func doGetMCPTools(ctx context.Context, permissions permission.Service, cfg *con
 	return slices.Collect(result.Seq())
 }
 
-func createAndInitializeClient(ctx context.Context, name string, m config.MCPConfig) (*client.Client, error) {
-	c, err := createMcpClient(name, m)
+func createAndInitializeClient(ctx context.Context, name string, m config.MCPConfig, resolver config.VariableResolver) (*client.Client, error) {
+	c, err := createMcpClient(name, m, resolver)
 	if err != nil {
 		updateMCPState(name, MCPStateError, err, nil, 0)
 		slog.Error("error creating mcp client", "error", err, "name", name)
@@ -367,14 +368,18 @@ func maybeTimeoutErr(err error, timeout time.Duration) error {
 	return err
 }
 
-func createMcpClient(name string, m config.MCPConfig) (*client.Client, error) {
+func createMcpClient(name string, m config.MCPConfig, resolver config.VariableResolver) (*client.Client, error) {
 	switch m.Type {
 	case config.MCPStdio:
-		if strings.TrimSpace(m.Command) == "" {
+		command, err := resolver.ResolveValue(m.Command)
+		if err != nil {
+			return nil, fmt.Errorf("invalid mcp command: %w", err)
+		}
+		if strings.TrimSpace(command) == "" {
 			return nil, fmt.Errorf("mcp stdio config requires a non-empty 'command' field")
 		}
 		return client.NewStdioMCPClientWithOptions(
-			home.Long(m.Command),
+			home.Long(command),
 			m.ResolvedEnv(),
 			m.Args,
 			transport.WithCommandLogger(mcpLogger{name: name}),

internal/lsp/client.go

@@ -45,7 +45,7 @@ type Client struct {
 }
 
 // New creates a new LSP client using the powernap implementation.
-func New(ctx context.Context, name string, config config.LSPConfig) (*Client, error) {
+func New(ctx context.Context, name string, config config.LSPConfig, resolver config.VariableResolver) (*Client, error) {
 	// Convert working directory to file URI
 	workDir, err := os.Getwd()
 	if err != nil {
@@ -54,9 +54,14 @@ func New(ctx context.Context, name string, config config.LSPConfig) (*Client, er
 
 	rootURI := string(protocol.URIFromPath(workDir))
 
+	command, err := resolver.ResolveValue(config.Command)
+	if err != nil {
+		return nil, fmt.Errorf("invalid lsp command: %w", err)
+	}
+
 	// Create powernap client config
 	clientConfig := powernap.ClientConfig{
-		Command: home.Long(config.Command),
+		Command: home.Long(command),
 		Args:    config.Args,
 		RootURI: rootURI,
 		Environment: func() map[string]string {

internal/lsp/client_test.go

@@ -5,14 +5,15 @@ import (
 	"testing"
 
 	"github.com/charmbracelet/crush/internal/config"
+	"github.com/charmbracelet/crush/internal/env"
 )
 
-func TestPowernapClient(t *testing.T) {
+func TestClient(t *testing.T) {
 	ctx := context.Background()
 
 	// Create a simple config for testing
 	cfg := config.LSPConfig{
-		Command:   "echo", // Use echo as a dummy command that won't fail
+		Command:   "$THE_CMD", // Use echo as a dummy command that won't fail
 		Args:      []string{"hello"},
 		FileTypes: []string{"go"},
 		Env:       map[string]string{},
@@ -20,7 +21,9 @@ func TestPowernapClient(t *testing.T) {
 
 	// Test creating a powernap client - this will likely fail with echo
 	// but we can still test the basic structure
-	client, err := New(ctx, "test", cfg)
+	client, err := New(ctx, "test", cfg, config.NewEnvironmentVariableResolver(env.NewFromMap(map[string]string{
+		"THE_CMD": "echo",
+	})))
 	if err != nil {
 		// Expected to fail with echo command, skip the rest
 		t.Skipf("Powernap client creation failed as expected with dummy command: %v", err)