git.iko.soy/crush
on main help sign in
lsp: expand shell variables in args and env

LSP args and env values used to be passed to the language server as raw strings, so "$HOME/go" was sent literally. They now go through the same shell expansion already used elsewhere in the config. If expansion fails, that LSP fails to load with a message that points at the specific arg or env key. Co-Authored-By: Charm Crush <crush@charm.land>

commit1f83559440dda9ef8654c1fe3cc8e48f98c143dd authorChristian Rocha <christian@rocha.is> dateSun 3 May 2026, 17:02 (6 months ago) parentf23e998
4 files changed+231−3
compare with main
unifiedsplit
internal/config/config.go+71 internal/config/resolve_real_test.go+112 internal/lsp/client.go+12−3 internal/lsp/client_test.go+36
internal/config/config.go+71
@@ -401,6 +401,77 @@ func (m MCPConfig) ResolvedHeaders(r VariableResolver) (map[string]string, error
401401 return out, nil
402402}
403403
404+// ResolvedArgs returns l.Args with every element expanded through the
405+// given resolver. A fresh slice is allocated; l.Args is never mutated.
406+// On the first resolution failure it returns nil and an error
407+// identifying the offending positional index; the inner resolver error
408+// is already sanitized by ResolveValue and is wrapped with %w so
409+// errors.Is/As continues to work.
410+//
411+// Empty resolved values are kept (a deliberate "empty positional arg"
412+// like --flag "" is sometimes valid), matching MCPConfig.ResolvedArgs;
413+// see PLAN.md Phase 2 design decision #18.
414+//
415+// The resolver choice matters: in server mode pass the shell resolver
416+// so $VAR / $(cmd) expand; in client mode pass IdentityResolver so the
417+// template is forwarded verbatim. See PLAN.md Phase 2 design decision
418+// #13.
419+func (l LSPConfig) ResolvedArgs(r VariableResolver) ([]string, error) {
420+ if len(l.Args) == 0 {
421+ return nil, nil
422+ }
423+ out := make([]string, len(l.Args))
424+ for i, a := range l.Args {
425+ v, err := r.ResolveValue(a)
426+ if err != nil {
427+ return nil, fmt.Errorf("arg %d: %w", i, err)
428+ }
429+ out[i] = v
430+ }
431+ return out, nil
432+}
433+
434+// ResolvedEnv returns l.Env with every value expanded through the
435+// given resolver. A fresh map is allocated; l.Env is never mutated.
436+// On the first resolution failure it returns nil and an error that
437+// identifies the offending key; the inner resolver error is already
438+// sanitized by ResolveValue and is wrapped with %w so errors.Is/As
439+// continues to work.
440+//
441+// Empty resolved values are kept ("FOO=" is a legitimate request;
442+// opt out via ${VAR:+...}), matching MCPConfig.ResolvedEnv; see
443+// PLAN.md Phase 2 design decision #18.
444+//
445+// Shape note: this returns map[string]string rather than the []string
446+// shape MCPConfig.ResolvedEnv uses because the consumer
447+// (powernap.ClientConfig.Environment in internal/lsp/client.go) takes
448+// a map directly — returning a []string here would only force a
449+// round-trip back to a map at the call site. See PLAN.md Phase 2
450+// design decision #13.
451+//
452+// See ResolvedArgs for guidance on picking a resolver.
453+func (l LSPConfig) ResolvedEnv(r VariableResolver) (map[string]string, error) {
454+ if len(l.Env) == 0 {
455+ return map[string]string{}, nil
456+ }
457+ out := make(map[string]string, len(l.Env))
458+ // Sort keys so failures are reported deterministically when more
459+ // than one value would fail.
460+ keys := make([]string, 0, len(l.Env))
461+ for k := range l.Env {
462+ keys = append(keys, k)
463+ }
464+ slices.Sort(keys)
465+ for _, k := range keys {
466+ v, err := r.ResolveValue(l.Env[k])
467+ if err != nil {
468+ return nil, fmt.Errorf("env %q: %w", k, err)
469+ }
470+ out[k] = v
471+ }
472+ return out, nil
473+}
474+
404475type Agent struct {
405476 ID string `json:"id,omitempty"`
406477 Name string `json:"name,omitempty"`
internal/config/resolve_real_test.go+112
@@ -359,6 +359,118 @@ func TestResolvedArgs_RealShell(t *testing.T) {
359359 })
360360}
361361
362+// TestLSPConfig_ResolvedArgs_RealShell exercises both success and
363+// failure for l.Args symmetrically with MCP args. Args are ordered so
364+// error messages must identify a positional index, not a key.
365+func TestLSPConfig_ResolvedArgs_RealShell(t *testing.T) {
366+ t.Parallel()
367+
368+ t.Run("success expands $VAR in each element", func(t *testing.T) {
369+ t.Parallel()
370+ l := LSPConfig{Args: []string{"--root", "$HOME", "--flag", "literal"}}
371+ r := realShellResolver(map[string]string{"HOME": "/home/tester"})
372+ got, err := l.ResolvedArgs(r)
373+ require.NoError(t, err)
374+ require.Equal(t, []string{"--root", "/home/tester", "--flag", "literal"}, got)
375+ })
376+
377+ t.Run("failure identifies offending index", func(t *testing.T) {
378+ t.Parallel()
379+ l := LSPConfig{Args: []string{"--root", "$(false)"}}
380+ orig := slices.Clone(l.Args)
381+
382+ got, err := l.ResolvedArgs(realShellResolver(nil))
383+ require.Error(t, err)
384+ require.Nil(t, got)
385+ require.Contains(t, err.Error(), "arg 1")
386+ require.Equal(t, orig, l.Args, "receiver Args must be preserved")
387+ })
388+
389+ t.Run("nil args returns nil, no error", func(t *testing.T) {
390+ t.Parallel()
391+ l := LSPConfig{}
392+ got, err := l.ResolvedArgs(realShellResolver(nil))
393+ require.NoError(t, err)
394+ require.Nil(t, got)
395+ })
396+}
397+
398+// TestLSPConfig_ResolvedEnv_RealShell pins the LSP env contract:
399+// success expands $VAR, failure wraps with the key name, and the
400+// receiver map is never mutated. The shape is map[string]string
401+// (not the MCP []string form) because powernap.ClientConfig.Environment
402+// takes a map directly.
403+func TestLSPConfig_ResolvedEnv_RealShell(t *testing.T) {
404+ t.Parallel()
405+
406+ t.Run("success expands $VAR", func(t *testing.T) {
407+ t.Parallel()
408+ l := LSPConfig{Env: map[string]string{"GOPATH": "$HOME/go"}}
409+ r := realShellResolver(map[string]string{"HOME": "/home/tester"})
410+ got, err := l.ResolvedEnv(r)
411+ require.NoError(t, err)
412+ require.Equal(t, map[string]string{"GOPATH": "/home/tester/go"}, got)
413+ })
414+
415+ t.Run("failure identifies offending key", func(t *testing.T) {
416+ t.Parallel()
417+ l := LSPConfig{Env: map[string]string{
418+ "GOPATH": "$(false)",
419+ "OTHER": "literal",
420+ }}
421+ orig := maps.Clone(l.Env)
422+
423+ got, err := l.ResolvedEnv(realShellResolver(nil))
424+ require.Error(t, err)
425+ require.Nil(t, got)
426+ require.Contains(t, err.Error(), `env "GOPATH"`)
427+ require.Equal(t, orig, l.Env, "receiver Env must be preserved")
428+ })
429+
430+ t.Run("idempotent and non-mutating", func(t *testing.T) {
431+ t.Parallel()
432+ l := LSPConfig{Env: map[string]string{
433+ "A": "$(echo one)",
434+ "B": "literal",
435+ }}
436+ orig := maps.Clone(l.Env)
437+ r := realShellResolver(nil)
438+
439+ first, err := l.ResolvedEnv(r)
440+ require.NoError(t, err)
441+ second, err := l.ResolvedEnv(r)
442+ require.NoError(t, err)
443+ require.Equal(t, first, second)
444+ require.Equal(t, orig, l.Env, "receiver Env must be preserved")
445+ })
446+}
447+
448+// TestLSPConfig_IdentityResolver pins the client-mode contract: both
449+// ResolvedArgs and ResolvedEnv round-trip the template verbatim under
450+// IdentityResolver and never error on unset variables. Local
451+// expansion would double-expand when the server does its own — this
452+// has to stay a pure pass-through.
453+func TestLSPConfig_IdentityResolver(t *testing.T) {
454+ t.Parallel()
455+
456+ l := LSPConfig{
457+ Args: []string{"--root", "$LSP_ROOT", "$(vault read -f lsp)"},
458+ Env: map[string]string{
459+ "GOPATH": "$HOME/go",
460+ "TOKEN": "$(cat /run/secrets/x)",
461+ },
462+ }
463+ r := IdentityResolver()
464+
465+ args, err := l.ResolvedArgs(r)
466+ require.NoError(t, err)
467+ require.Equal(t, l.Args, args)
468+
469+ envs, err := l.ResolvedEnv(r)
470+ require.NoError(t, err)
471+ require.Equal(t, l.Env, envs)
472+}
473+
362474// TestMCPConfig_IdentityResolver pins the client-mode contract: every
363475// Resolved* method round-trips the template verbatim and never errors
364476// on unset variables. Local expansion would double-expand when the
internal/lsp/client.go+12−3
@@ -5,7 +5,6 @@ import (
55 "encoding/json"
66 "fmt"
77 "log/slog"
8− "maps"
98 "os"
109 "path/filepath"
1110 "slices"
@@ -169,11 +168,21 @@ func (c *Client) createPowernapClient() error {
169168 return fmt.Errorf("invalid lsp command: %w", err)
170169 }
171170
171+ args, err := c.config.ResolvedArgs(c.resolver)
172+ if err != nil {
173+ return fmt.Errorf("invalid lsp args: %w", err)
174+ }
175+
176+ envs, err := c.config.ResolvedEnv(c.resolver)
177+ if err != nil {
178+ return fmt.Errorf("invalid lsp env: %w", err)
179+ }
180+
172181 clientConfig := powernap.ClientConfig{
173182 Command: home.Long(command),
174− Args: c.config.Args,
183+ Args: args,
175184 RootURI: rootURI,
176− Environment: maps.Clone(c.config.Env),
185+ Environment: envs,
177186 Settings: c.config.Options,
178187 InitOptions: c.config.InitOptions,
179188 WorkspaceFolders: []protocol.WorkspaceFolder{
internal/lsp/client_test.go+36
@@ -61,6 +61,42 @@ func TestClient(t *testing.T) {
6161 }
6262}
6363
64+// TestNew_ExpansionFailure_Args pins that a failing $(cmd) in LSP
65+// args surfaces as a load error prefixed "invalid lsp args:" and that
66+// no client is returned. Mirrors the MCP contract where expansion
67+// failure hard-stops transport creation rather than silently running
68+// with an empty or literal value.
69+func TestNew_ExpansionFailure_Args(t *testing.T) {
70+ t.Parallel()
71+
72+ cfg := config.LSPConfig{
73+ Command: "echo",
74+ Args: []string{"--root", "$(false)"},
75+ }
76+ resolver := config.NewShellVariableResolver(env.NewFromMap(map[string]string{}))
77+
78+ client, err := New(t.Context(), "test-args-fail", cfg, resolver, ".", false)
79+ require.Error(t, err)
80+ require.Nil(t, client, "client must not start when args expansion fails")
81+ require.Contains(t, err.Error(), "invalid lsp args")
82+}
83+
84+// TestNew_ExpansionFailure_Env pins the same contract for env values.
85+func TestNew_ExpansionFailure_Env(t *testing.T) {
86+ t.Parallel()
87+
88+ cfg := config.LSPConfig{
89+ Command: "echo",
90+ Env: map[string]string{"BAD": "$(false)"},
91+ }
92+ resolver := config.NewShellVariableResolver(env.NewFromMap(map[string]string{}))
93+
94+ client, err := New(t.Context(), "test-env-fail", cfg, resolver, ".", false)
95+ require.Error(t, err)
96+ require.Nil(t, client, "client must not start when env expansion fails")
97+ require.Contains(t, err.Error(), "invalid lsp env")
98+}
99+
64100func TestNilClient(t *testing.T) {
65101 t.Parallel()
66102