diff --git a/internal/config/config.go b/internal/config/config.go index ce4dfa0d32969f6765f785919c1130096f614209..17fceeb660a090601c3f67136c2c5d4ed50cdf46 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -401,6 +401,77 @@ func (m MCPConfig) ResolvedHeaders(r VariableResolver) (map[string]string, error return out, nil } +// ResolvedArgs returns l.Args with every element expanded through the +// given resolver. A fresh slice is allocated; l.Args is never mutated. +// On the first resolution failure it returns nil and an error +// identifying the offending positional index; the inner resolver error +// is already sanitized by ResolveValue and is wrapped with %w so +// errors.Is/As continues to work. +// +// Empty resolved values are kept (a deliberate "empty positional arg" +// like --flag "" is sometimes valid), matching MCPConfig.ResolvedArgs; +// see PLAN.md Phase 2 design decision #18. +// +// The resolver choice matters: in server mode pass the shell resolver +// so $VAR / $(cmd) expand; in client mode pass IdentityResolver so the +// template is forwarded verbatim. See PLAN.md Phase 2 design decision +// #13. +func (l LSPConfig) ResolvedArgs(r VariableResolver) ([]string, error) { + if len(l.Args) == 0 { + return nil, nil + } + out := make([]string, len(l.Args)) + for i, a := range l.Args { + v, err := r.ResolveValue(a) + if err != nil { + return nil, fmt.Errorf("arg %d: %w", i, err) + } + out[i] = v + } + return out, nil +} + +// ResolvedEnv returns l.Env with every value expanded through the +// given resolver. A fresh map is allocated; l.Env is never mutated. +// On the first resolution failure it returns nil and an error that +// identifies the offending key; the inner resolver error is already +// sanitized by ResolveValue and is wrapped with %w so errors.Is/As +// continues to work. +// +// Empty resolved values are kept ("FOO=" is a legitimate request; +// opt out via ${VAR:+...}), matching MCPConfig.ResolvedEnv; see +// PLAN.md Phase 2 design decision #18. +// +// Shape note: this returns map[string]string rather than the []string +// shape MCPConfig.ResolvedEnv uses because the consumer +// (powernap.ClientConfig.Environment in internal/lsp/client.go) takes +// a map directly — returning a []string here would only force a +// round-trip back to a map at the call site. See PLAN.md Phase 2 +// design decision #13. +// +// See ResolvedArgs for guidance on picking a resolver. +func (l LSPConfig) ResolvedEnv(r VariableResolver) (map[string]string, error) { + if len(l.Env) == 0 { + return map[string]string{}, nil + } + out := make(map[string]string, len(l.Env)) + // Sort keys so failures are reported deterministically when more + // than one value would fail. + keys := make([]string, 0, len(l.Env)) + for k := range l.Env { + keys = append(keys, k) + } + slices.Sort(keys) + for _, k := range keys { + v, err := r.ResolveValue(l.Env[k]) + if err != nil { + return nil, fmt.Errorf("env %q: %w", k, err) + } + out[k] = v + } + return out, nil +} + type Agent struct { ID string `json:"id,omitempty"` Name string `json:"name,omitempty"` diff --git a/internal/config/resolve_real_test.go b/internal/config/resolve_real_test.go index bb7083346b8cffbe44e371e481d73be34b84c5b2..64cecc5769c6574975ae38ecdafd982eadbbe252 100644 --- a/internal/config/resolve_real_test.go +++ b/internal/config/resolve_real_test.go @@ -359,6 +359,118 @@ func TestResolvedArgs_RealShell(t *testing.T) { }) } +// TestLSPConfig_ResolvedArgs_RealShell exercises both success and +// failure for l.Args symmetrically with MCP args. Args are ordered so +// error messages must identify a positional index, not a key. +func TestLSPConfig_ResolvedArgs_RealShell(t *testing.T) { + t.Parallel() + + t.Run("success expands $VAR in each element", func(t *testing.T) { + t.Parallel() + l := LSPConfig{Args: []string{"--root", "$HOME", "--flag", "literal"}} + r := realShellResolver(map[string]string{"HOME": "/home/tester"}) + got, err := l.ResolvedArgs(r) + require.NoError(t, err) + require.Equal(t, []string{"--root", "/home/tester", "--flag", "literal"}, got) + }) + + t.Run("failure identifies offending index", func(t *testing.T) { + t.Parallel() + l := LSPConfig{Args: []string{"--root", "$(false)"}} + orig := slices.Clone(l.Args) + + got, err := l.ResolvedArgs(realShellResolver(nil)) + require.Error(t, err) + require.Nil(t, got) + require.Contains(t, err.Error(), "arg 1") + require.Equal(t, orig, l.Args, "receiver Args must be preserved") + }) + + t.Run("nil args returns nil, no error", func(t *testing.T) { + t.Parallel() + l := LSPConfig{} + got, err := l.ResolvedArgs(realShellResolver(nil)) + require.NoError(t, err) + require.Nil(t, got) + }) +} + +// TestLSPConfig_ResolvedEnv_RealShell pins the LSP env contract: +// success expands $VAR, failure wraps with the key name, and the +// receiver map is never mutated. The shape is map[string]string +// (not the MCP []string form) because powernap.ClientConfig.Environment +// takes a map directly. +func TestLSPConfig_ResolvedEnv_RealShell(t *testing.T) { + t.Parallel() + + t.Run("success expands $VAR", func(t *testing.T) { + t.Parallel() + l := LSPConfig{Env: map[string]string{"GOPATH": "$HOME/go"}} + r := realShellResolver(map[string]string{"HOME": "/home/tester"}) + got, err := l.ResolvedEnv(r) + require.NoError(t, err) + require.Equal(t, map[string]string{"GOPATH": "/home/tester/go"}, got) + }) + + t.Run("failure identifies offending key", func(t *testing.T) { + t.Parallel() + l := LSPConfig{Env: map[string]string{ + "GOPATH": "$(false)", + "OTHER": "literal", + }} + orig := maps.Clone(l.Env) + + got, err := l.ResolvedEnv(realShellResolver(nil)) + require.Error(t, err) + require.Nil(t, got) + require.Contains(t, err.Error(), `env "GOPATH"`) + require.Equal(t, orig, l.Env, "receiver Env must be preserved") + }) + + t.Run("idempotent and non-mutating", func(t *testing.T) { + t.Parallel() + l := LSPConfig{Env: map[string]string{ + "A": "$(echo one)", + "B": "literal", + }} + orig := maps.Clone(l.Env) + r := realShellResolver(nil) + + first, err := l.ResolvedEnv(r) + require.NoError(t, err) + second, err := l.ResolvedEnv(r) + require.NoError(t, err) + require.Equal(t, first, second) + require.Equal(t, orig, l.Env, "receiver Env must be preserved") + }) +} + +// TestLSPConfig_IdentityResolver pins the client-mode contract: both +// ResolvedArgs and ResolvedEnv round-trip the template verbatim under +// IdentityResolver and never error on unset variables. Local +// expansion would double-expand when the server does its own — this +// has to stay a pure pass-through. +func TestLSPConfig_IdentityResolver(t *testing.T) { + t.Parallel() + + l := LSPConfig{ + Args: []string{"--root", "$LSP_ROOT", "$(vault read -f lsp)"}, + Env: map[string]string{ + "GOPATH": "$HOME/go", + "TOKEN": "$(cat /run/secrets/x)", + }, + } + r := IdentityResolver() + + args, err := l.ResolvedArgs(r) + require.NoError(t, err) + require.Equal(t, l.Args, args) + + envs, err := l.ResolvedEnv(r) + require.NoError(t, err) + require.Equal(t, l.Env, envs) +} + // TestMCPConfig_IdentityResolver pins the client-mode contract: every // Resolved* method round-trips the template verbatim and never errors // on unset variables. Local expansion would double-expand when the diff --git a/internal/lsp/client.go b/internal/lsp/client.go index 57bb7ab968bf4e0bddc8a2bcc74ac4515e84f5c6..9f6f9885e4bcc6ff0ce303ed98e6c73d3502309f 100644 --- a/internal/lsp/client.go +++ b/internal/lsp/client.go @@ -5,7 +5,6 @@ import ( "encoding/json" "fmt" "log/slog" - "maps" "os" "path/filepath" "slices" @@ -169,11 +168,21 @@ func (c *Client) createPowernapClient() error { return fmt.Errorf("invalid lsp command: %w", err) } + args, err := c.config.ResolvedArgs(c.resolver) + if err != nil { + return fmt.Errorf("invalid lsp args: %w", err) + } + + envs, err := c.config.ResolvedEnv(c.resolver) + if err != nil { + return fmt.Errorf("invalid lsp env: %w", err) + } + clientConfig := powernap.ClientConfig{ Command: home.Long(command), - Args: c.config.Args, + Args: args, RootURI: rootURI, - Environment: maps.Clone(c.config.Env), + Environment: envs, Settings: c.config.Options, InitOptions: c.config.InitOptions, WorkspaceFolders: []protocol.WorkspaceFolder{ diff --git a/internal/lsp/client_test.go b/internal/lsp/client_test.go index 904a7640bc263acb3e41714de13fda637d061892..0337dc8b9ba506264e519e1c53a725ab9fec35b1 100644 --- a/internal/lsp/client_test.go +++ b/internal/lsp/client_test.go @@ -61,6 +61,42 @@ func TestClient(t *testing.T) { } } +// TestNew_ExpansionFailure_Args pins that a failing $(cmd) in LSP +// args surfaces as a load error prefixed "invalid lsp args:" and that +// no client is returned. Mirrors the MCP contract where expansion +// failure hard-stops transport creation rather than silently running +// with an empty or literal value. +func TestNew_ExpansionFailure_Args(t *testing.T) { + t.Parallel() + + cfg := config.LSPConfig{ + Command: "echo", + Args: []string{"--root", "$(false)"}, + } + resolver := config.NewShellVariableResolver(env.NewFromMap(map[string]string{})) + + client, err := New(t.Context(), "test-args-fail", cfg, resolver, ".", false) + require.Error(t, err) + require.Nil(t, client, "client must not start when args expansion fails") + require.Contains(t, err.Error(), "invalid lsp args") +} + +// TestNew_ExpansionFailure_Env pins the same contract for env values. +func TestNew_ExpansionFailure_Env(t *testing.T) { + t.Parallel() + + cfg := config.LSPConfig{ + Command: "echo", + Env: map[string]string{"BAD": "$(false)"}, + } + resolver := config.NewShellVariableResolver(env.NewFromMap(map[string]string{})) + + client, err := New(t.Context(), "test-env-fail", cfg, resolver, ".", false) + require.Error(t, err) + require.Nil(t, client, "client must not start when env expansion fails") + require.Contains(t, err.Error(), "invalid lsp env") +} + func TestNilClient(t *testing.T) { t.Parallel()