git.iko.soy/crush
on main help sign in
Merge pull request #2782 from charmbracelet/resolve-env-vars
2814e40Christian Rocha
README.md+11−4
compare with main
unifiedsplit
@@ -293,10 +293,17 @@ like you would. LSPs can be added manually like so:
293
293
294### MCPs
294### MCPs
295
295
296−Crush also supports Model Context Protocol (MCP) servers through three
296+Crush also supports Model Context Protocol (MCP) servers through three transport
297−transport types: `stdio` for command-line servers, `http` for HTTP endpoints,
297+types: `stdio` for command-line servers, `http` for HTTP endpoints, and `sse`
298−and `sse` for Server-Sent Events. Environment variable expansion is supported
298+for Server-Sent Events.
299−using `$(echo $VAR)` syntax.
299+
300+Shell-style value expansion (`$VAR`, `${VAR:-default}`, `$(command)`, quoting,
301+and nesting (works in `command`, `args`, `env`, `headers`, and `url`, so
302+file-based secrets like work out of the box, so you can use values like
303+"$TOKEN"` and `"$(cat /path/to/secret/token)"``. Expansion runs through Crush's
304+embedded shell, so the same syntax works on all supported systems, including
305+Windows. Unset variables are an error; use `${VAR:-fallback}` to opt in to
306+a default.
300
307
301```json
308```json
302{
309{
internal/agent/tools/mcp/init.go+32−8
@@ -447,35 +447,59 @@ func createTransport(ctx context.Context, m config.MCPConfig, resolver config.Va
447 if strings.TrimSpace(command) == "" {
447 if strings.TrimSpace(command) == "" {
448 return nil, fmt.Errorf("mcp stdio config requires a non-empty 'command' field")
448 return nil, fmt.Errorf("mcp stdio config requires a non-empty 'command' field")
449 }
449 }
450+ args, err := m.ResolvedArgs(resolver)
451+ if err != nil {
452+ return nil, err
453+ }
454+ envs, err := m.ResolvedEnv(resolver)
455+ if err != nil {
456+ return nil, err
457+ }
450− cmd := exec.CommandContext(ctx, home.Long(command), m.Args...)
458+ cmd := exec.CommandContext(ctx, home.Long(command), args...)
451− cmd.Env = append(os.Environ(), m.ResolvedEnv()...)
459+ cmd.Env = append(os.Environ(), envs...)
452 return &mcp.CommandTransport{
460 return &mcp.CommandTransport{
453 Command: cmd,
461 Command: cmd,
454 }, nil
462 }, nil
455 case config.MCPHttp:
463 case config.MCPHttp:
464+ url, err := m.ResolvedURL(resolver)
465+ if err != nil {
466+ return nil, err
467+ }
456− if strings.TrimSpace(m.URL) == "" {
468+ if strings.TrimSpace(url) == "" {
457 return nil, fmt.Errorf("mcp http config requires a non-empty 'url' field")
469 return nil, fmt.Errorf("mcp http config requires a non-empty 'url' field")
458 }
470 }
471+ headers, err := m.ResolvedHeaders(resolver)
472+ if err != nil {
473+ return nil, err
474+ }
459 client := &http.Client{
475 client := &http.Client{
460 Transport: &headerRoundTripper{
476 Transport: &headerRoundTripper{
461− headers: m.ResolvedHeaders(),
477+ headers: headers,
462 },
478 },
463 }
479 }
464 return &mcp.StreamableClientTransport{
480 return &mcp.StreamableClientTransport{
465− Endpoint: m.URL,
481+ Endpoint: url,
466 HTTPClient: client,
482 HTTPClient: client,
467 }, nil
483 }, nil
468 case config.MCPSSE:
484 case config.MCPSSE:
485+ url, err := m.ResolvedURL(resolver)
486+ if err != nil {
487+ return nil, err
488+ }
469− if strings.TrimSpace(m.URL) == "" {
489+ if strings.TrimSpace(url) == "" {
470 return nil, fmt.Errorf("mcp sse config requires a non-empty 'url' field")
490 return nil, fmt.Errorf("mcp sse config requires a non-empty 'url' field")
471 }
491 }
492+ headers, err := m.ResolvedHeaders(resolver)
493+ if err != nil {
494+ return nil, err
495+ }
472 client := &http.Client{
496 client := &http.Client{
473 Transport: &headerRoundTripper{
497 Transport: &headerRoundTripper{
474− headers: m.ResolvedHeaders(),
498+ headers: headers,
475 },
499 },
476 }
500 }
477 return &mcp.SSEClientTransport{
501 return &mcp.SSEClientTransport{
478− Endpoint: m.URL,
502+ Endpoint: url,
479 HTTPClient: client,
503 HTTPClient: client,
480 }, nil
504 }, nil
481 default:
505 default:
internal/agent/tools/mcp/init_test.go+391
@@ -2,13 +2,28 @@ package mcp
2
2
3import (
3import (
4 "context"
4 "context"
5+ "maps"
6+ "os"
5 "testing"
7 "testing"
6
8
9+ "github.com/charmbracelet/crush/internal/config"
10+ "github.com/charmbracelet/crush/internal/env"
7 "github.com/modelcontextprotocol/go-sdk/mcp"
11 "github.com/modelcontextprotocol/go-sdk/mcp"
8 "github.com/stretchr/testify/require"
12 "github.com/stretchr/testify/require"
9 "go.uber.org/goleak"
13 "go.uber.org/goleak"
10)
14)
11
15
16+// shellResolverWithPath builds a shell resolver whose env carries PATH
17+// plus any caller-supplied overrides. Without PATH, $(cat), $(echo),
18+// etc. can't find their binaries in a test process where the shell env
19+// is otherwise empty.
20+func shellResolverWithPath(t *testing.T, overrides map[string]string) config.VariableResolver {
21+ t.Helper()
22+ m := map[string]string{"PATH": os.Getenv("PATH")}
23+ maps.Copy(m, overrides)
24+ return config.NewShellVariableResolver(env.NewFromMap(m))
25+}
26+
12func TestMCPSession_CancelOnClose(t *testing.T) {
27func TestMCPSession_CancelOnClose(t *testing.T) {
13 defer goleak.VerifyNone(t)
28 defer goleak.VerifyNone(t)
14
29
@@ -36,3 +51,379 @@ func TestMCPSession_CancelOnClose(t *testing.T) {
36 // After Close, the context must be cancelled.
51 // After Close, the context must be cancelled.
37 require.ErrorIs(t, ctx.Err(), context.Canceled)
52 require.ErrorIs(t, ctx.Err(), context.Canceled)
38}
53}
54+
55+// TestCreateTransport_URLResolution pins that m.URL goes through the
56+// same resolver seam as command, args, env, and headers. Covers both
57+// the HTTP and SSE branches, success and failure, so a regression in
58+// ResolvedURL wiring is caught at the transport layer rather than only
59+// at the config layer.
60+func TestCreateTransport_URLResolution(t *testing.T) {
61+ t.Parallel()
62+
63+ shell := config.NewShellVariableResolver(env.NewFromMap(map[string]string{
64+ "MCP_HOST": "mcp.example.com",
65+ }))
66+
67+ t.Run("http success expands $VAR", func(t *testing.T) {
68+ t.Parallel()
69+ m := config.MCPConfig{
70+ Type: config.MCPHttp,
71+ URL: "https://$MCP_HOST/api",
72+ }
73+ tr, err := createTransport(t.Context(), m, shell)
74+ require.NoError(t, err)
75+ require.NotNil(t, tr)
76+ sct, ok := tr.(*mcp.StreamableClientTransport)
77+ require.True(t, ok, "expected StreamableClientTransport, got %T", tr)
78+ require.Equal(t, "https://mcp.example.com/api", sct.Endpoint)
79+ })
80+
81+ t.Run("sse success expands $(cmd)", func(t *testing.T) {
82+ t.Parallel()
83+ m := config.MCPConfig{
84+ Type: config.MCPSSE,
85+ URL: "https://$(echo mcp.example.com)/events",
86+ }
87+ tr, err := createTransport(t.Context(), m, shell)
88+ require.NoError(t, err)
89+ sse, ok := tr.(*mcp.SSEClientTransport)
90+ require.True(t, ok, "expected SSEClientTransport, got %T", tr)
91+ require.Equal(t, "https://mcp.example.com/events", sse.Endpoint)
92+ })
93+
94+ t.Run("http unset var surfaces error, no transport created", func(t *testing.T) {
95+ t.Parallel()
96+ m := config.MCPConfig{
97+ Type: config.MCPHttp,
98+ URL: "https://$MCP_MISSING_HOST/api",
99+ }
100+ tr, err := createTransport(t.Context(), m, shell)
101+ require.Error(t, err)
102+ require.Nil(t, tr)
103+ require.Contains(t, err.Error(), "url:")
104+ require.Contains(t, err.Error(), "$MCP_MISSING_HOST")
105+ })
106+
107+ t.Run("sse failing $(cmd) surfaces error, no transport created", func(t *testing.T) {
108+ t.Parallel()
109+ m := config.MCPConfig{
110+ Type: config.MCPSSE,
111+ URL: "https://$(false)/events",
112+ }
113+ tr, err := createTransport(t.Context(), m, shell)
114+ require.Error(t, err)
115+ require.Nil(t, tr)
116+ require.Contains(t, err.Error(), "url:")
117+ require.Contains(t, err.Error(), "$(false)")
118+ })
119+
120+ t.Run("http empty-after-resolve still fails the non-empty guard", func(t *testing.T) {
121+ t.Parallel()
122+ // ${MCP_EMPTY:-} resolves to the empty string (no error),
123+ // then the existing TrimSpace guard in createTransport must
124+ // reject it so we never spawn a transport against "".
125+ m := config.MCPConfig{
126+ Type: config.MCPHttp,
127+ URL: "${MCP_EMPTY:-}",
128+ }
129+ tr, err := createTransport(t.Context(), m, shell)
130+ require.Error(t, err)
131+ require.Nil(t, tr)
132+ require.Contains(t, err.Error(), "non-empty 'url'")
133+ })
134+
135+ t.Run("identity resolver round-trips template verbatim", func(t *testing.T) {
136+ t.Parallel()
137+ // Client mode forwards the template to the server; no local
138+ // expansion, no error on unset vars.
139+ tmpl := "https://$MCP_MISSING_HOST/api"
140+ m := config.MCPConfig{Type: config.MCPHttp, URL: tmpl}
141+ tr, err := createTransport(t.Context(), m, config.IdentityResolver())
142+ require.NoError(t, err)
143+ sct, ok := tr.(*mcp.StreamableClientTransport)
144+ require.True(t, ok)
145+ require.Equal(t, tmpl, sct.Endpoint)
146+ })
147+}
148+
149+// TestCreateTransport_StdioResolution pins that command, args, and env
150+// for stdio MCPs go through the same resolver seam as the other
151+// transports. Covers both success (expansion produced the expected
152+// exec.Cmd) and failure (any one field erroring prevents transport
153+// creation).
154+func TestCreateTransport_StdioResolution(t *testing.T) {
155+ t.Parallel()
156+
157+ t.Run("success expands command, args, and env", func(t *testing.T) {
158+ t.Parallel()
159+ r := shellResolverWithPath(t, map[string]string{
160+ "MY_TOKEN": "hunter2",
161+ })
162+ m := config.MCPConfig{
163+ Type: config.MCPStdio,
164+ Command: "forgejo-mcp",
165+ Args: []string{"--token", "$MY_TOKEN", "--host", "$(echo example.com)"},
166+ Env: map[string]string{
167+ "SECRET": "$(echo shh)",
168+ "PLAIN": "literal",
169+ "REFERENCE": "$MY_TOKEN",
170+ },
171+ }
172+ tr, err := createTransport(t.Context(), m, r)
173+ require.NoError(t, err)
174+ require.NotNil(t, tr)
175+
176+ ct, ok := tr.(*mcp.CommandTransport)
177+ require.True(t, ok, "expected CommandTransport, got %T", tr)
178+
179+ // exec.Cmd.Args[0] is the command name; the rest are positional
180+ // args as passed.
181+ require.Equal(t, []string{"forgejo-mcp", "--token", "hunter2", "--host", "example.com"}, ct.Command.Args)
182+
183+ // Env is os.Environ() + resolved entries (sorted). Check the
184+ // resolved entries are present with their expanded values.
185+ require.Contains(t, ct.Command.Env, "SECRET=shh")
186+ require.Contains(t, ct.Command.Env, "PLAIN=literal")
187+ require.Contains(t, ct.Command.Env, "REFERENCE=hunter2")
188+ })
189+
190+ t.Run("env resolution failure surfaces error, no transport created", func(t *testing.T) {
191+ t.Parallel()
192+ r := shellResolverWithPath(t, nil)
193+ m := config.MCPConfig{
194+ Type: config.MCPStdio,
195+ Command: "forgejo-mcp",
196+ Env: map[string]string{"TOKEN": "$(false)"},
197+ }
198+ tr, err := createTransport(t.Context(), m, r)
199+ require.Error(t, err)
200+ require.Nil(t, tr)
201+ require.Contains(t, err.Error(), "env TOKEN")
202+ })
203+
204+ t.Run("unset env var is a hard error, not silent empty", func(t *testing.T) {
205+ t.Parallel()
206+ // The regression at the heart of PLAN.md: unset vars used to
207+ // silently substitute "" and hand an empty credential to the
208+ // child process. Now they must error out before exec.
209+ r := shellResolverWithPath(t, nil)
210+ m := config.MCPConfig{
211+ Type: config.MCPStdio,
212+ Command: "forgejo-mcp",
213+ Env: map[string]string{"FORGEJO_ACCESS_TOKEN": "$FORGJO_TOKEN"},
214+ }
215+ tr, err := createTransport(t.Context(), m, r)
216+ require.Error(t, err)
217+ require.Nil(t, tr)
218+ require.Contains(t, err.Error(), "env FORGEJO_ACCESS_TOKEN")
219+ })
220+
221+ t.Run("args resolution failure surfaces error, no transport created", func(t *testing.T) {
222+ t.Parallel()
223+ r := shellResolverWithPath(t, nil)
224+ m := config.MCPConfig{
225+ Type: config.MCPStdio,
226+ Command: "forgejo-mcp",
227+ Args: []string{"--token", "$(false)"},
228+ }
229+ tr, err := createTransport(t.Context(), m, r)
230+ require.Error(t, err)
231+ require.Nil(t, tr)
232+ require.Contains(t, err.Error(), "arg 1")
233+ })
234+
235+ t.Run("command resolution failure surfaces error, no transport created", func(t *testing.T) {
236+ t.Parallel()
237+ r := shellResolverWithPath(t, nil)
238+ m := config.MCPConfig{
239+ Type: config.MCPStdio,
240+ Command: "$(false)",
241+ }
242+ tr, err := createTransport(t.Context(), m, r)
243+ require.Error(t, err)
244+ require.Nil(t, tr)
245+ require.Contains(t, err.Error(), "invalid mcp command")
246+ })
247+
248+ t.Run("identity resolver round-trips templates verbatim", func(t *testing.T) {
249+ t.Parallel()
250+ // Client mode: no local expansion, no error on unset vars.
251+ m := config.MCPConfig{
252+ Type: config.MCPStdio,
253+ Command: "forgejo-mcp",
254+ Args: []string{"--token", "$MCP_MISSING"},
255+ Env: map[string]string{"TOKEN": "$(vault read -f token)"},
256+ }
257+ tr, err := createTransport(t.Context(), m, config.IdentityResolver())
258+ require.NoError(t, err)
259+ ct, ok := tr.(*mcp.CommandTransport)
260+ require.True(t, ok)
261+ require.Equal(t, []string{"forgejo-mcp", "--token", "$MCP_MISSING"}, ct.Command.Args)
262+ require.Contains(t, ct.Command.Env, "TOKEN=$(vault read -f token)")
263+ })
264+}
265+
266+// TestCreateTransport_HeadersResolution pins that a single failing
267+// header aborts HTTP/SSE transport creation and that the successful
268+// resolver passes every expanded header through to the round tripper.
269+func TestCreateTransport_HeadersResolution(t *testing.T) {
270+ t.Parallel()
271+
272+ t.Run("http headers success expands $(cmd)", func(t *testing.T) {
273+ t.Parallel()
274+ r := shellResolverWithPath(t, map[string]string{
275+ "GITHUB_TOKEN": "gh-secret",
276+ })
277+ m := config.MCPConfig{
278+ Type: config.MCPHttp,
279+ URL: "https://mcp.example.com/api",
280+ Headers: map[string]string{
281+ "Authorization": "$(echo Bearer $GITHUB_TOKEN)",
282+ "X-Static": "kept",
283+ },
284+ }
285+ tr, err := createTransport(t.Context(), m, r)
286+ require.NoError(t, err)
287+
288+ sct, ok := tr.(*mcp.StreamableClientTransport)
289+ require.True(t, ok)
290+ rt, ok := sct.HTTPClient.Transport.(*headerRoundTripper)
291+ require.True(t, ok, "expected headerRoundTripper, got %T", sct.HTTPClient.Transport)
292+ require.Equal(t, map[string]string{
293+ "Authorization": "Bearer gh-secret",
294+ "X-Static": "kept",
295+ }, rt.headers)
296+ })
297+
298+ t.Run("http failing header surfaces error, no transport", func(t *testing.T) {
299+ t.Parallel()
300+ r := shellResolverWithPath(t, nil)
301+ m := config.MCPConfig{
302+ Type: config.MCPHttp,
303+ URL: "https://mcp.example.com/api",
304+ Headers: map[string]string{"Authorization": "$(false)"},
305+ }
306+ tr, err := createTransport(t.Context(), m, r)
307+ require.Error(t, err)
308+ require.Nil(t, tr)
309+ require.Contains(t, err.Error(), "header Authorization")
310+ })
311+
312+ t.Run("sse failing header surfaces error, no transport", func(t *testing.T) {
313+ t.Parallel()
314+ r := shellResolverWithPath(t, nil)
315+ m := config.MCPConfig{
316+ Type: config.MCPSSE,
317+ URL: "https://mcp.example.com/events",
318+ Headers: map[string]string{"Authorization": "Bearer $MISSING_TOKEN"},
319+ }
320+ tr, err := createTransport(t.Context(), m, r)
321+ require.Error(t, err)
322+ require.Nil(t, tr)
323+ require.Contains(t, err.Error(), "header Authorization")
324+ require.Contains(t, err.Error(), "$MISSING_TOKEN")
325+ })
326+}
327+
328+// TestCreateSession_ResolutionFailureUpdatesState pins the user-visible
329+// half of the regression fix: when any of command/args/env/headers/url
330+// fails to resolve, createSession must publish StateError to the state
331+// map so crush_info and the TUI's MCP status card can render a real
332+// error instead of the MCP silently sitting in "starting" or being
333+// spawned with an empty credential.
334+//
335+// These subtests cannot run in parallel: `states` is a package-level
336+// csync.Map and each assertion reads the entry written by the call
337+// under test. They do use unique MCP names per subtest to keep them
338+// independent regardless of ordering.
339+func TestCreateSession_ResolutionFailureUpdatesState(t *testing.T) {
340+ r := shellResolverWithPath(t, nil)
341+
342+ tests := []struct {
343+ name string
344+ mcpName string
345+ cfg config.MCPConfig
346+ wantErrContains string
347+ }{
348+ {
349+ name: "stdio env failure",
350+ mcpName: "test-stdio-env-fail",
351+ cfg: config.MCPConfig{
352+ Type: config.MCPStdio,
353+ Command: "echo",
354+ Env: map[string]string{"FORGEJO_ACCESS_TOKEN": "$(false)"},
355+ },
356+ wantErrContains: "env FORGEJO_ACCESS_TOKEN",
357+ },
358+ {
359+ name: "stdio args failure",
360+ mcpName: "test-stdio-args-fail",
361+ cfg: config.MCPConfig{
362+ Type: config.MCPStdio,
363+ Command: "echo",
364+ Args: []string{"--token", "$MCP_UNSET_TOKEN"},
365+ },
366+ wantErrContains: "arg 1",
367+ },
368+ {
369+ name: "http url failure",
370+ mcpName: "test-http-url-fail",
371+ cfg: config.MCPConfig{
372+ Type: config.MCPHttp,
373+ URL: "https://$MCP_MISSING_HOST/api",
374+ },
375+ wantErrContains: "url:",
376+ },
377+ {
378+ name: "http header failure",
379+ mcpName: "test-http-header-fail",
380+ cfg: config.MCPConfig{
381+ Type: config.MCPHttp,
382+ URL: "https://mcp.example.com/api",
383+ Headers: map[string]string{"Authorization": "$(false)"},
384+ },
385+ wantErrContains: "header Authorization",
386+ },
387+ {
388+ name: "sse url failure",
389+ mcpName: "test-sse-url-fail",
390+ cfg: config.MCPConfig{
391+ Type: config.MCPSSE,
392+ URL: "https://$(false)/events",
393+ },
394+ wantErrContains: "url:",
395+ },
396+ {
397+ name: "sse header failure",
398+ mcpName: "test-sse-header-fail",
399+ cfg: config.MCPConfig{
400+ Type: config.MCPSSE,
401+ URL: "https://mcp.example.com/events",
402+ Headers: map[string]string{"Authorization": "Bearer $MISSING_SSE_TOKEN"},
403+ },
404+ wantErrContains: "header Authorization",
405+ },
406+ }
407+
408+ for _, tc := range tests {
409+ t.Run(tc.name, func(t *testing.T) {
410+ // Guarantee a clean slate on the shared state map so a
411+ // stale entry from another test can't satisfy the
412+ // assertion.
413+ states.Del(tc.mcpName)
414+ t.Cleanup(func() { states.Del(tc.mcpName) })
415+
416+ sess, err := createSession(t.Context(), tc.mcpName, tc.cfg, r)
417+ require.Error(t, err)
418+ require.Nil(t, sess)
419+ require.Contains(t, err.Error(), tc.wantErrContains)
420+
421+ info, ok := GetState(tc.mcpName)
422+ require.True(t, ok, "state entry must be written for %q", tc.mcpName)
423+ require.Equal(t, StateError, info.State, "expected StateError, got %s", info.State)
424+ require.Error(t, info.Error, "state must carry the failure error")
425+ require.Contains(t, info.Error.Error(), tc.wantErrContains)
426+ require.Nil(t, info.Client, "no client session on failure")
427+ })
428+ }
429+}
internal/config/config.go+106−30
@@ -5,7 +5,6 @@ import (
5 "context"
5 "context"
6 "errors"
6 "errors"
7 "fmt"
7 "fmt"
8− "log/slog"
9 "maps"
8 "maps"
10 "net/http"
9 "net/http"
11 "net/url"
10 "net/url"
@@ -15,7 +14,6 @@ import (
15
14
16 "charm.land/catwalk/pkg/catwalk"
15 "charm.land/catwalk/pkg/catwalk"
17 "github.com/charmbracelet/crush/internal/csync"
16 "github.com/charmbracelet/crush/internal/csync"
18− "github.com/charmbracelet/crush/internal/env"
19 "github.com/charmbracelet/crush/internal/oauth"
17 "github.com/charmbracelet/crush/internal/oauth"
20 "github.com/charmbracelet/crush/internal/oauth/copilot"
18 "github.com/charmbracelet/crush/internal/oauth/copilot"
21 "github.com/invopop/jsonschema"
19 "github.com/invopop/jsonschema"
@@ -304,25 +302,96 @@ func (l LSPs) Sorted() []LSP {
304 return sorted
302 return sorted
305}
303}
306
304
307−func (l LSPConfig) ResolvedEnv() []string {
305+// ResolvedEnv returns m.Env with every value expanded through the
308− return resolveEnvs(l.Env)
306+// given resolver. The returned slice is of the form "KEY=value" sorted
309−}
307+// by key so callers get deterministic output; the receiver's Env map is
310−
308+// not mutated. On the first resolution failure it returns nil and an
311−func (m MCPConfig) ResolvedEnv() []string {
309+// error that identifies the offending key; the inner resolver error is
312− return resolveEnvs(m.Env)
310+// already sanitized by ResolveValue and is wrapped with %w so
313−}
311+// errors.Is/As continues to work. Callers are expected to surface it
314−
312+// (for MCP, via StateError on the status card) rather than silently
315−func (m MCPConfig) ResolvedHeaders() map[string]string {
313+// spawn the server with an empty credential.
316− resolver := NewShellVariableResolver(env.New())
314+//
315+// The resolver choice matters: in server mode pass the shell resolver
316+// so $VAR / $(cmd) expand; in client mode pass IdentityResolver so the
317+// template is forwarded verbatim and expansion happens on the server.
318+func (m MCPConfig) ResolvedEnv(r VariableResolver) ([]string, error) {
319+ return resolveEnvs(m.Env, r)
320+}
321+
322+// ResolvedArgs returns m.Args with every element expanded through the
323+// given resolver. A fresh slice is allocated; m.Args is never mutated.
324+// On the first resolution failure it returns nil and an error
325+// identifying the offending positional index; the inner resolver error
326+// is already sanitized by ResolveValue and is wrapped with %w so
327+// errors.Is/As continues to work.
328+//
329+// See ResolvedEnv for guidance on picking a resolver.
330+func (m MCPConfig) ResolvedArgs(r VariableResolver) ([]string, error) {
331+ if len(m.Args) == 0 {
332+ return nil, nil
333+ }
334+ out := make([]string, len(m.Args))
317− for e, v := range m.Headers {
335+ for i, a := range m.Args {
318− var err error
319− m.Headers[e], err = resolver.ResolveValue(v)
336+ v, err := r.ResolveValue(a)
320 if err != nil {
337 if err != nil {
321− slog.Error("Error resolving header variable", "error", err, "variable", e, "value", v)
338+ return nil, fmt.Errorf("arg %d: %w", i, err)
322− continue
323 }
339 }
340+ out[i] = v
341+ }
342+ return out, nil
343+}
344+
345+// ResolvedURL returns m.URL expanded through the given resolver. The
346+// receiver is not mutated. Errors from the resolver are already
347+// sanitized by ResolveValue and are wrapped with %w for errors.Is/As.
348+//
349+// URLs run through the same shell-expansion pipeline as the other
350+// fields, so a literal '$' (e.g. OData query strings containing
351+// $filter/$select) must be escaped as '\$' or '${DOLLAR:-$}' to avoid
352+// being interpreted as a variable reference. Same constraint already
353+// applies to command, args, env, and headers.
354+//
355+// See ResolvedEnv for guidance on picking a resolver.
356+func (m MCPConfig) ResolvedURL(r VariableResolver) (string, error) {
357+ if m.URL == "" {
358+ return "", nil
359+ }
360+ v, err := r.ResolveValue(m.URL)
361+ if err != nil {
362+ return "", fmt.Errorf("url: %w", err)
363+ }
364+ return v, nil
365+}
366+
367+// ResolvedHeaders returns m.Headers with every value expanded through
368+// the given resolver. A fresh map is allocated; m.Headers is never
369+// mutated. On the first resolution failure it returns nil and an error
370+// identifying the offending header name; the inner resolver error is
371+// already sanitized by ResolveValue and is wrapped with %w so
372+// errors.Is/As continues to work.
373+//
374+// See ResolvedEnv for guidance on picking a resolver.
375+func (m MCPConfig) ResolvedHeaders(r VariableResolver) (map[string]string, error) {
376+ if len(m.Headers) == 0 {
377+ return map[string]string{}, nil
378+ }
379+ out := make(map[string]string, len(m.Headers))
380+ // Sort keys so failures are reported deterministically when more
381+ // than one header would fail.
382+ keys := make([]string, 0, len(m.Headers))
383+ for k := range m.Headers {
384+ keys = append(keys, k)
324 }
385 }
386+ slices.Sort(keys)
387+ for _, k := range keys {
325− return m.Headers
388+ v, err := r.ResolveValue(m.Headers[k])
389+ if err != nil {
390+ return nil, fmt.Errorf("header %s: %w", k, err)
391+ }
392+ out[k] = v
393+ }
394+ return out, nil
326}
395}
327
396
328type Agent struct {
397type Agent struct {
@@ -662,22 +731,29 @@ func (c *ProviderConfig) TestConnection(resolver VariableResolver) error {
662 return nil
731 return nil
663}
732}
664
733
665−func resolveEnvs(envs map[string]string) []string {
734+// resolveEnvs expands every value in envs through the given resolver
666− resolver := NewShellVariableResolver(env.New())
735+// and returns a fresh "KEY=value" slice sorted by key. The input map is
667− for e, v := range envs {
736+// not mutated. On the first resolution failure it returns nil and an
668− var err error
737+// error identifying the offending variable; the inner resolver error is
669− envs[e], err = resolver.ResolveValue(v)
738+// already sanitized by ResolveValue and is wrapped with %w.
670− if err != nil {
739+func resolveEnvs(envs map[string]string, r VariableResolver) ([]string, error) {
671− slog.Error("Error resolving environment variable", "error", err, "variable", e, "value", v)
740+ if len(envs) == 0 {
672− continue
741+ return nil, nil
673− }
674 }
742 }
675−
743+ keys := make([]string, 0, len(envs))
744+ for k := range envs {
745+ keys = append(keys, k)
746+ }
747+ slices.Sort(keys)
676 res := make([]string, 0, len(envs))
748 res := make([]string, 0, len(envs))
677− for k, v := range envs {
749+ for _, k := range keys {
750+ v, err := r.ResolveValue(envs[k])
751+ if err != nil {
752+ return nil, fmt.Errorf("env %s: %w", k, err)
753+ }
678 res = append(res, fmt.Sprintf("%s=%s", k, v))
754 res = append(res, fmt.Sprintf("%s=%s", k, v))
679 }
755 }
680− return res
756+ return res, nil
681}
757}
682
758
683func ptrValOr[T any](t *T, el T) T {
759func ptrValOr[T any](t *T, el T) T {
internal/config/load.go+6
@@ -222,6 +222,12 @@ func (c *Config) configureProviders(store *ConfigStore, env env.Env, resolver Va
222 if len(config.ExtraHeaders) > 0 {
222 if len(config.ExtraHeaders) > 0 {
223 maps.Copy(headers, config.ExtraHeaders)
223 maps.Copy(headers, config.ExtraHeaders)
224 }
224 }
225+ // Intentional divergence from MCP env/headers/args/url resolution:
226+ // provider headers that fail to resolve log and keep their literal
227+ // template so providers with optional, env-gated headers still
228+ // load on hosts where those vars are unset. Changing this to a
229+ // hard error would break those configs. See PLAN.md "Design
230+ // decisions" item 4 for the full rationale.
225 for k, v := range headers {
231 for k, v := range headers {
226 resolved, err := resolver.ResolveValue(v)
232 resolved, err := resolver.ResolveValue(v)
227 if err != nil {
233 if err != nil {
internal/config/load_test.go+58
@@ -1589,3 +1589,61 @@ func TestConfig_configureProviders_HyperAPIKeyFromConfigOverrides(t *testing.T)
1589 require.True(t, ok, "Hyper provider should be configured")
1589 require.True(t, ok, "Hyper provider should be configured")
1590 require.Equal(t, "env-api-key", pc.APIKey)
1590 require.Equal(t, "env-api-key", pc.APIKey)
1591}
1591}
1592+
1593+// TestConfig_configureProviders_ProviderHeaderResolveFailure pins the
1594+// intentional divergence at load.go:225: a provider header whose
1595+// expansion fails must NOT fail the whole config load. It logs an
1596+// error, keeps the literal template in the resolved header map, and
1597+// moves on. The MCP contract (hard error on failed expansion) does not
1598+// apply here because many providers ship DefaultHeaders that reference
1599+// env vars which are legitimately unset on most hosts.
1600+//
1601+// If this test ever flips to requiring an error, read PLAN.md "Design
1602+// decisions" item 4 before changing the production code — the
1603+// divergence is deliberate.
1604+func TestConfig_configureProviders_ProviderHeaderResolveFailure(t *testing.T) {
1605+ knownProviders := []catwalk.Provider{
1606+ {
1607+ ID: "openai",
1608+ APIKey: "$OPENAI_API_KEY",
1609+ APIEndpoint: "https://api.openai.com/v1",
1610+ Models: []catwalk.Model{{ID: "test-model"}},
1611+ },
1612+ }
1613+
1614+ cfg := &Config{
1615+ Providers: csync.NewMapFrom(map[string]ProviderConfig{
1616+ "openai": {
1617+ ExtraHeaders: map[string]string{
1618+ // Failing $(...) — inner command exits 1, no stdout.
1619+ "X-Broken": "$(false)",
1620+ // Unset var — nounset makes this an error too.
1621+ "X-Missing": "$PROVIDER_HEADER_NEVER_SET",
1622+ // Happy path: must still be resolved, proving the
1623+ // failure in the other two did not abort the loop.
1624+ "X-Static": "kept-literal",
1625+ },
1626+ },
1627+ }),
1628+ }
1629+ cfg.setDefaults("/tmp", "")
1630+
1631+ testEnv := env.NewFromMap(map[string]string{
1632+ "OPENAI_API_KEY": "test-key",
1633+ "PATH": os.Getenv("PATH"),
1634+ })
1635+ resolver := NewShellVariableResolver(testEnv)
1636+
1637+ err := cfg.configureProviders(testStore(cfg), testEnv, resolver, knownProviders)
1638+ require.NoError(t, err, "provider load must tolerate failing header expansion")
1639+
1640+ pc, ok := cfg.Providers.Get("openai")
1641+ require.True(t, ok, "openai provider must still be configured")
1642+
1643+ // Literal template preserved for the two failing headers; the
1644+ // happy-path header is resolved through the shell (pass-through
1645+ // for a literal value).
1646+ require.Equal(t, "$(false)", pc.ExtraHeaders["X-Broken"])
1647+ require.Equal(t, "$PROVIDER_HEADER_NEVER_SET", pc.ExtraHeaders["X-Missing"])
1648+ require.Equal(t, "kept-literal", pc.ExtraHeaders["X-Static"])
1649+}
internal/config/mcp_resolved_url_test.gonew file+88
@@ -0,0 +1,88 @@
1+package config
2+
3+import (
4+ "errors"
5+ "testing"
6+
7+ "github.com/charmbracelet/crush/internal/env"
8+ "github.com/stretchr/testify/require"
9+)
10+
11+func TestMCPConfig_ResolvedURL(t *testing.T) {
12+ t.Parallel()
13+
14+ t.Run("empty url short-circuits without calling resolver", func(t *testing.T) {
15+ t.Parallel()
16+ m := MCPConfig{Type: MCPHttp}
17+ got, err := m.ResolvedURL(stubResolver{err: errors.New("should not be called")})
18+ require.NoError(t, err)
19+ require.Empty(t, got)
20+ })
21+
22+ t.Run("literal url passes through unchanged", func(t *testing.T) {
23+ t.Parallel()
24+ m := MCPConfig{Type: MCPHttp, URL: "https://mcp.example.com/api"}
25+ got, err := m.ResolvedURL(NewShellVariableResolver(env.NewFromMap(nil)))
26+ require.NoError(t, err)
27+ require.Equal(t, "https://mcp.example.com/api", got)
28+ })
29+
30+ t.Run("expands $VAR with shell resolver", func(t *testing.T) {
31+ t.Parallel()
32+ m := MCPConfig{Type: MCPHttp, URL: "https://$MCP_HOST/api"}
33+ r := NewShellVariableResolver(env.NewFromMap(map[string]string{"MCP_HOST": "mcp.example.com"}))
34+ got, err := m.ResolvedURL(r)
35+ require.NoError(t, err)
36+ require.Equal(t, "https://mcp.example.com/api", got)
37+ })
38+
39+ t.Run("expands $(cmd) with shell resolver", func(t *testing.T) {
40+ t.Parallel()
41+ m := MCPConfig{Type: MCPSSE, URL: "https://$(echo mcp.example.com)/events"}
42+ got, err := m.ResolvedURL(NewShellVariableResolver(env.NewFromMap(nil)))
43+ require.NoError(t, err)
44+ require.Equal(t, "https://mcp.example.com/events", got)
45+ })
46+
47+ t.Run("unset var is an error wrapping the template", func(t *testing.T) {
48+ t.Parallel()
49+ m := MCPConfig{Type: MCPHttp, URL: "https://$MCP_MISSING_HOST/api"}
50+ _, err := m.ResolvedURL(NewShellVariableResolver(env.NewFromMap(nil)))
51+ require.Error(t, err)
52+ require.Contains(t, err.Error(), "url:")
53+ require.Contains(t, err.Error(), "$MCP_MISSING_HOST")
54+ require.Contains(t, err.Error(), "unbound")
55+ })
56+
57+ t.Run("failing command substitution is an error", func(t *testing.T) {
58+ t.Parallel()
59+ m := MCPConfig{Type: MCPHttp, URL: "https://$(false)/api"}
60+ _, err := m.ResolvedURL(NewShellVariableResolver(env.NewFromMap(nil)))
61+ require.Error(t, err)
62+ require.Contains(t, err.Error(), "url:")
63+ require.Contains(t, err.Error(), "$(false)")
64+ })
65+
66+ t.Run("identity resolver round-trips template verbatim", func(t *testing.T) {
67+ t.Parallel()
68+ // In client mode expansion happens server-side; the client must
69+ // forward the template without touching it and without erroring
70+ // on unset vars.
71+ tmpl := "https://$MCP_HOST/$(vault read -f url)"
72+ m := MCPConfig{Type: MCPHttp, URL: tmpl}
73+ got, err := m.ResolvedURL(IdentityResolver())
74+ require.NoError(t, err)
75+ require.Equal(t, tmpl, got)
76+ })
77+}
78+
79+// stubResolver returns ("", err) for every call. Paired with a non-nil
80+// err the empty-URL test asserts ResolvedURL short-circuits before
81+// reaching ResolveValue: if it didn't, the test would fail with err.
82+type stubResolver struct {
83+ err error
84+}
85+
86+func (s stubResolver) ResolveValue(v string) (string, error) {
87+ return "", s.err
88+}
internal/config/resolve.go+118−115
@@ -3,13 +3,17 @@ package config
3import (
3import (
4 "context"
4 "context"
5 "fmt"
5 "fmt"
6− "strings"
7 "time"
6 "time"
8
7
9 "github.com/charmbracelet/crush/internal/env"
8 "github.com/charmbracelet/crush/internal/env"
10 "github.com/charmbracelet/crush/internal/shell"
9 "github.com/charmbracelet/crush/internal/shell"
11)
10)
12
11
12+// resolveTimeout bounds how long a single ResolveValue call may spend
13+// inside shell expansion (including any command substitution). Matches
14+// the timeout used by the previous hand-rolled parser.
15+const resolveTimeout = 5 * time.Minute
16+
13type VariableResolver interface {
17type VariableResolver interface {
14 ResolveValue(value string) (string, error)
18 ResolveValue(value string) (string, error)
15}
19}
@@ -28,141 +32,140 @@ func IdentityResolver() VariableResolver {
28 return identityResolver{}
32 return identityResolver{}
29}
33}
30
34
31−type Shell interface {
35+// Expander is the single-value shell expansion seam used by
36+// shellVariableResolver. Production wires it to shell.ExpandValue; tests
37+// can inject a fake via WithExpander.
32− Exec(ctx context.Context, command string) (stdout, stderr string, err error)
38+type Expander func(ctx context.Context, value string, env []string) (string, error)
39+
40+// ShellResolverOption customizes shell variable resolver construction.
41+type ShellResolverOption func(*shellVariableResolver)
42+
43+// WithExpander overrides the expansion function used by the resolver.
44+// Primarily intended for tests; production callers should not need this.
45+func WithExpander(e Expander) ShellResolverOption {
46+ return func(r *shellVariableResolver) {
47+ if e != nil {
48+ r.expand = e
49+ }
50+ }
33}
51}
34
52
35type shellVariableResolver struct {
53type shellVariableResolver struct {
36− shell Shell
37− env env.Env
54+ env env.Env
55+ expand Expander
38}
56}
39
57
58+// NewShellVariableResolver returns a VariableResolver that delegates to
59+// the embedded shell (the same interpreter used by the bash tool and
60+// hooks). Supported constructs match shell.ExpandValue: $VAR, ${VAR},
61+// ${VAR:-default}, $(command), quoting, and escapes. Unset variables are
62+// an error; use ${VAR:-} to opt in to an empty fallback.
40−func NewShellVariableResolver(env env.Env) VariableResolver {
63+func NewShellVariableResolver(e env.Env, opts ...ShellResolverOption) VariableResolver {
41− return &shellVariableResolver{
64+ r := &shellVariableResolver{
42− env: env,
65+ env: e,
43− shell: shell.NewShell(
66+ expand: shell.ExpandValue,
44− &shell.Options{
45− Env: env.Env(),
46− },
47− ),
48 }
67 }
68+ for _, opt := range opts {
69+ opt(r)
70+ }
71+ return r
49}
72}
50
73
51−// ResolveValue is a method for resolving values, such as environment variables.
52−// it will resolve shell-like variable substitution anywhere in the string, including:
74+// ResolveValue resolves shell-style substitution anywhere in the string:
75+//
53−// - $(command) for command substitution
76+// - $(command) for command substitution, with full quoting and nesting.
54−// - $VAR or ${VAR} for environment variables
77+// - $VAR and ${VAR} for environment variables.
78+// - ${VAR:-default} / ${VAR:+alt} / ${VAR:?msg} for defaulting.
79+//
80+// Unset variables are a hard error (nounset), mirroring the historical
81+// behaviour of this resolver: silently expanding an unset variable to the
82+// empty string is exactly how broken credentials reach MCP servers.
55func (r *shellVariableResolver) ResolveValue(value string) (string, error) {
83func (r *shellVariableResolver) ResolveValue(value string) (string, error) {
56− // Special case: lone $ is an error (backward compatibility)
84+ // Preserve the historical backward-compat contract: a lone "$" is a
85+ // malformed config value, not a legal literal. The underlying shell
86+ // parser would accept it as a literal; we reject it here so existing
87+ // configs that relied on this validation still fail early.
57 if value == "$" {
88 if value == "$" {
58 return "", fmt.Errorf("invalid value format: %s", value)
89 return "", fmt.Errorf("invalid value format: %s", value)
59 }
90 }
60
91
61− // If no $ found, return as-is
92+ ctx, cancel := context.WithTimeout(context.Background(), resolveTimeout)
62− if !strings.Contains(value, "$") {
93+ defer cancel()
63− return value, nil
64− }
65−
66− result := value
67
94
68− // Handle command substitution: $(command)
95+ out, err := r.expand(ctx, value, r.env.Env())
69− for {
96+ if err != nil {
70− start := strings.Index(result, "$(")
97+ return "", sanitizeResolveError(value, err)
71− if start == -1 {
98+ }
72− break
99+ return out, nil
73− }
100+}
74
101
75− // Find matching closing parenthesis
102+// maxResolveErrBytes bounds the size of the inner error message surfaced
76− depth := 0
103+// from a resolution failure. Defense-in-depth on top of shell.ExpandValue's
77− end := -1
104+// own stderr budget: a custom Expander injected via WithExpander, or any
78− for i := start + 2; i < len(result); i++ {
105+// future non-shell error path, must still produce a user-safe message.
79− if result[i] == '(' {
106+const maxResolveErrBytes = 512
80− depth++
107+
81− } else if result[i] == ')' {
108+// sanitizeResolveError wraps an expansion error with the user-written
82− if depth == 0 {
109+// template (the pre-expansion string — it is what they typed, safe to
83− end = i
110+// surface) and a bounded, scrubbed rendering of the inner error message.
84− break
111+// Contract:
85− }
112+//
86− depth--
113+// - Never includes the resolved (post-expansion) value. This helper
114+// only receives the template and err, so a successful expansion
115+// result cannot reach it.
116+// - May include the template verbatim.
117+// - Truncates the inner error's message to maxResolveErrBytes and
118+// replaces embedded NULs and other non-printables (except tab and
119+// newline) with '?'.
120+//
121+// The returned error still unwraps to the original for errors.Is/As so
122+// callers can inspect typed sentinels; only the rendered message is
123+// scrubbed.
124+func sanitizeResolveError(template string, err error) error {
125+ if err == nil {
126+ return nil
87− }
127+ }
128+ return &resolveError{
129+ template: template,
130+ msg: scrubErrorMessage(err.Error()),
131+ inner: err,
132+ }
88− }
133+}
89
134
90− if end == -1 {
135+// resolveError is the concrete type returned by sanitizeResolveError.
91− return "", fmt.Errorf("unmatched $( in value: %s", value)
136+// Its Error() method returns the template + scrubbed inner message;
137+// Unwrap exposes the original error so errors.Is/As continue to work.
138+type resolveError struct {
139+ template string
140+ msg string
141+ inner error
92− }
142+}
93
143
94− command := result[start+2 : end]
144+func (e *resolveError) Error() string {
95− ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute)
145+ return fmt.Sprintf("resolving %q: %s", e.template, e.msg)
146+}
96
147
97− stdout, _, err := r.shell.Exec(ctx, command)
148+func (e *resolveError) Unwrap() error { return e.inner }
98− cancel()
99− if err != nil {
100− return "", fmt.Errorf("command execution failed for '%s': %w", command, err)
101− }
102
149
103− // Replace the $(command) with the output
150+// scrubErrorMessage bounds the message to maxResolveErrBytes bytes and
104− replacement := strings.TrimSpace(stdout)
151+// replaces non-printable bytes (anything outside ASCII printable, tab, or
105− result = result[:start] + replacement + result[end+1:]
152+// newline) with '?'. Mirrors shell.sanitizeStderr but operates on a
153+// string rather than raw command stderr and runs at the config layer,
154+// so arbitrary Expander error text is also sanitized.
155+func scrubErrorMessage(s string) string {
156+ if len(s) > maxResolveErrBytes {
157+ s = s[:maxResolveErrBytes]
106 }
158 }
107−
159+ out := make([]byte, len(s))
108− // Handle environment variables: $VAR and ${VAR}
160+ for i := 0; i < len(s); i++ {
109− searchStart := 0
161+ c := s[i]
110− for {
162+ if c == '\t' || c == '\n' || (c >= 0x20 && c < 0x7f) {
111− start := strings.Index(result[searchStart:], "$")
163+ out[i] = c
112− if start == -1 {
113− break
114− }
115− start += searchStart // Adjust for the offset
116−
117− // Skip if this is part of $( which we already handled
118− if start+1 < len(result) && result[start+1] == '(' {
119− // Skip past this $(...)
120− searchStart = start + 1
121 continue
164 continue
122 }
165 }
123− var varName string
166+ out[i] = '?'
124− var end int
125−
126− if start+1 < len(result) && result[start+1] == '{' {
127− // Handle ${VAR} format
128− closeIdx := strings.Index(result[start+2:], "}")
129− if closeIdx == -1 {
130− return "", fmt.Errorf("unmatched ${ in value: %s", value)
131− }
132− varName = result[start+2 : start+2+closeIdx]
133− end = start + 2 + closeIdx + 1
134− } else {
135− // Handle $VAR format - variable names must start with letter or underscore
136− if start+1 >= len(result) {
137− return "", fmt.Errorf("incomplete variable reference at end of string: %s", value)
138− }
139−
140− if result[start+1] != '_' &&
141− (result[start+1] < 'a' || result[start+1] > 'z') &&
142− (result[start+1] < 'A' || result[start+1] > 'Z') {
143− return "", fmt.Errorf("invalid variable name starting with '%c' in: %s", result[start+1], value)
144− }
145−
146− end = start + 1
147− for end < len(result) && (result[end] == '_' ||
148− (result[end] >= 'a' && result[end] <= 'z') ||
149− (result[end] >= 'A' && result[end] <= 'Z') ||
150− (result[end] >= '0' && result[end] <= '9')) {
151− end++
152− }
153− varName = result[start+1 : end]
154− }
155−
156− envValue := r.env.Get(varName)
157− if envValue == "" {
158− return "", fmt.Errorf("environment variable %q not set", varName)
159− }
160−
161− result = result[:start] + envValue + result[end:]
162− searchStart = start + len(envValue) // Continue searching after the replacement
163 }
167 }
164−
165− return result, nil
168+ return string(out)
166}
169}
167
170
168type environmentVariableResolver struct {
171type environmentVariableResolver struct {
@@ -177,11 +180,11 @@ func NewEnvironmentVariableResolver(env env.Env) VariableResolver {
177
180
178// ResolveValue resolves environment variables from the provided env.Env.
181// ResolveValue resolves environment variables from the provided env.Env.
179func (r *environmentVariableResolver) ResolveValue(value string) (string, error) {
182func (r *environmentVariableResolver) ResolveValue(value string) (string, error) {
180− if !strings.HasPrefix(value, "$") {
183+ if len(value) == 0 || value[0] != '$' {
181 return value, nil
184 return value, nil
182 }
185 }
183
186
184− varName := strings.TrimPrefix(value, "$")
187+ varName := value[1:]
185 resolvedValue := r.env.Get(varName)
188 resolvedValue := r.env.Get(varName)
186 if resolvedValue == "" {
189 if resolvedValue == "" {
187 return "", fmt.Errorf("environment variable %q not set", varName)
190 return "", fmt.Errorf("environment variable %q not set", varName)
internal/config/resolve_real_test.gonew file+310
@@ -0,0 +1,310 @@
1+package config
2+
3+import (
4+ "fmt"
5+ "maps"
6+ "os"
7+ "path/filepath"
8+ "slices"
9+ "strings"
10+ "testing"
11+
12+ "github.com/charmbracelet/crush/internal/env"
13+ "github.com/stretchr/testify/require"
14+)
15+
16+// These tests exercise the full shell-expansion path (no mocks,
17+// no injected Expander) to catch regressions that only surface when
18+// internal/shell actually runs the value. Table-level unit tests with
19+// fake expanders live in resolve_test.go.
20+
21+// realShellResolver builds a resolver backed by a shell env that
22+// contains PATH + the caller-supplied overrides. Production callers
23+// get PATH for free via env.New(); these tests need it so $(cat ...)
24+// and similar inner commands can resolve.
25+func realShellResolver(vars map[string]string) VariableResolver {
26+ m := map[string]string{"PATH": os.Getenv("PATH")}
27+ maps.Copy(m, vars)
28+ return NewShellVariableResolver(env.NewFromMap(m))
29+}
30+
31+func writeTempFile(t *testing.T, content string) string {
32+ t.Helper()
33+ dir := t.TempDir()
34+ p := filepath.Join(dir, "secret")
35+ require.NoError(t, os.WriteFile(p, []byte(content), 0o600))
36+ return p
37+}
38+
39+// TestResolvedEnv_RealShell_Success covers the shell constructs the
40+// PLAN calls out: $(cat tempfile) with and without trailing newline,
41+// ${VAR:-default} for unset vars, literal-space preservation around
42+// $(...), nested parens, quoted args inside $(echo ...), and a
43+// glob-like literal round-tripping unchanged.
44+func TestResolvedEnv_RealShell_Success(t *testing.T) {
45+ t.Parallel()
46+
47+ // filepath.ToSlash so Windows temp paths (C:\Users\...) survive
48+ // being injected into a shell command string — the embedded shell
49+ // treats backslashes as escapes, forward slashes work on every OS.
50+ withNL := filepath.ToSlash(writeTempFile(t, "token-with-nl\n"))
51+ noNL := filepath.ToSlash(writeTempFile(t, "token-no-nl"))
52+
53+ m := MCPConfig{
54+ Env: map[string]string{
55+ // POSIX strips exactly one trailing newline from $(...)
56+ // output, so both forms land on the same value.
57+ "TOK_NL": fmt.Sprintf("$(cat %s)", withNL),
58+ "TOK_NO": fmt.Sprintf("$(cat %s)", noNL),
59+
60+ // ${VAR:-default} must not error on unset: this is the
61+ // opt-in escape hatch for "empty is fine".
62+ "FALLBACK": "${MCP_MISSING:-fallback}",
63+
64+ // Leading/trailing literal spaces around $(...) must be
65+ // preserved — single-value contract, no field splitting.
66+ "PADDED": " $(echo v) ",
67+
68+ // ")" inside a quoted arg to echo is a regression guard
69+ // for the old hand-rolled paren matcher.
70+ "PAREN": `$(echo ")")`,
71+
72+ // Embedded space inside a quoted arg must survive
73+ // verbatim; no word-splitting side effect.
74+ "SPACEY": `$(echo "a b")`,
75+
76+ // Glob-like literals must not expand.
77+ "GLOB": "*.go",
78+ },
79+ }
80+
81+ got, err := m.ResolvedEnv(realShellResolver(nil))
82+ require.NoError(t, err)
83+
84+ // ResolvedEnv returns "KEY=value" sorted by key.
85+ want := []string{
86+ "FALLBACK=fallback",
87+ "GLOB=*.go",
88+ "PADDED= v ",
89+ "PAREN=)",
90+ "SPACEY=a b",
91+ "TOK_NL=token-with-nl",
92+ "TOK_NO=token-no-nl",
93+ }
94+ require.Equal(t, want, got)
95+}
96+
97+// TestResolvedEnv_RealShell_DoesNotMutate pins that both success and
98+// failure paths leave m.Env untouched. Prior behaviour rewrote the
99+// value in place on error; that was the exact mechanism that shipped
100+// empty credentials to MCP servers.
101+func TestResolvedEnv_RealShell_DoesNotMutate(t *testing.T) {
102+ t.Parallel()
103+
104+ t.Run("success path leaves Env untouched", func(t *testing.T) {
105+ t.Parallel()
106+ m := MCPConfig{Env: map[string]string{"TOKEN": "$(echo shh)"}}
107+ orig := maps.Clone(m.Env)
108+
109+ _, err := m.ResolvedEnv(realShellResolver(nil))
110+ require.NoError(t, err)
111+ require.Equal(t, orig, m.Env)
112+ })
113+
114+ t.Run("failure path leaves Env untouched", func(t *testing.T) {
115+ t.Parallel()
116+ m := MCPConfig{Env: map[string]string{"BROKEN": "$(false)"}}
117+ orig := maps.Clone(m.Env)
118+
119+ _, err := m.ResolvedEnv(realShellResolver(nil))
120+ require.Error(t, err)
121+ require.Equal(t, orig, m.Env, "map must be preserved on error")
122+ })
123+}
124+
125+// TestResolvedEnv_RealShell_Idempotent pins the pure-function contract:
126+// two calls on the same config return deeply-equal slices.
127+func TestResolvedEnv_RealShell_Idempotent(t *testing.T) {
128+ t.Parallel()
129+
130+ m := MCPConfig{
131+ Env: map[string]string{
132+ "A": "$(echo one)",
133+ "B": "$(echo two)",
134+ "C": "literal",
135+ },
136+ }
137+ r := realShellResolver(nil)
138+
139+ first, err := m.ResolvedEnv(r)
140+ require.NoError(t, err)
141+ second, err := m.ResolvedEnv(r)
142+ require.NoError(t, err)
143+ require.Equal(t, first, second)
144+}
145+
146+// TestResolvedEnv_RealShell_Deterministic guards against Go's
147+// randomized map iteration leaking into the returned slice order.
148+func TestResolvedEnv_RealShell_Deterministic(t *testing.T) {
149+ t.Parallel()
150+
151+ m := MCPConfig{Env: map[string]string{
152+ "Z": "z",
153+ "A": "a",
154+ "M": "m",
155+ }}
156+
157+ got, err := m.ResolvedEnv(realShellResolver(nil))
158+ require.NoError(t, err)
159+ require.True(t, slices.IsSorted(got), "env slice must be sorted; got %v", got)
160+}
161+
162+// TestResolvedEnv_RealShell_NounsetRegression is the single most
163+// important assertion in this file: an unset variable is an error, not
164+// an empty expansion. Swapping the hand-rolled parser for mvdan's
165+// default-expansion behaviour without nounset would re-introduce
166+// Defect 1 via a typo'd variable name.
167+func TestResolvedEnv_RealShell_NounsetRegression(t *testing.T) {
168+ t.Parallel()
169+
170+ m := MCPConfig{Env: map[string]string{
171+ // Intentional typo: user meant $FORGEJO_TOKEN.
172+ "FORGEJO_ACCESS_TOKEN": "$FORGJO_TOKEN",
173+ }}
174+ got, err := m.ResolvedEnv(realShellResolver(nil))
175+ require.Error(t, err, "unset var must not silently expand to empty")
176+ require.Nil(t, got)
177+ require.Contains(t, err.Error(), "FORGEJO_ACCESS_TOKEN")
178+ require.Contains(t, err.Error(), "$FORGJO_TOKEN")
179+}
180+
181+// TestResolvedEnv_RealShell_FailureDetail pins that a failing inner
182+// command surfaces enough detail (exit code + stderr on POSIX, the
183+// underlying OS error on Windows where coreutils runs in-process) to
184+// diagnose without forcing the user to re-run the command by hand.
185+// Also verifies the template is included so they know which Env
186+// entry blew up.
187+func TestResolvedEnv_RealShell_FailureDetail(t *testing.T) {
188+ t.Parallel()
189+
190+ // Forward slashes so the path survives shell-string injection on
191+ // Windows; see TestResolvedEnv_RealShell_Success for the same note.
192+ missing := filepath.ToSlash(filepath.Join(t.TempDir(), "definitely-not-here"))
193+ m := MCPConfig{Env: map[string]string{
194+ "FORGEJO_ACCESS_TOKEN": fmt.Sprintf("$(cat %s)", missing),
195+ }}
196+
197+ _, err := m.ResolvedEnv(realShellResolver(nil))
198+ require.Error(t, err)
199+ msg := err.Error()
200+ require.Contains(t, msg, "FORGEJO_ACCESS_TOKEN", "must identify the failing env var")
201+ require.Contains(t, msg, missing, "must include the template so users see what failed")
202+
203+ // Inner diagnostic detail must survive. POSIX surfaces "exit
204+ // status N" + stderr; Windows' in-process coreutils surfaces the
205+ // Go OS error instead. Accept either shape so the test is
206+ // portable without weakening the intent.
207+ lower := strings.ToLower(msg)
208+ hasDetail := strings.Contains(lower, "exit status") ||
209+ strings.Contains(lower, "no such file") ||
210+ strings.Contains(lower, "cannot find")
211+ require.True(t, hasDetail, "must surface inner error detail: %s", msg)
212+}
213+
214+// TestResolvedHeaders_RealShell_FailurePreservesOriginal pins two
215+// invariants simultaneously: on failure the returned map is nil (not
216+// a partially-populated map) and the receiver's Headers map is
217+// unchanged. A test that only asserted on the returned value could
218+// hide an in-place mutation regression.
219+func TestResolvedHeaders_RealShell_FailurePreservesOriginal(t *testing.T) {
220+ t.Parallel()
221+
222+ m := MCPConfig{Headers: map[string]string{
223+ "Authorization": "Bearer $(false)",
224+ "X-Static": "kept",
225+ }}
226+ orig := maps.Clone(m.Headers)
227+
228+ got, err := m.ResolvedHeaders(realShellResolver(nil))
229+ require.Error(t, err)
230+ require.Nil(t, got, "headers map must be nil on failure")
231+ require.Contains(t, err.Error(), "header Authorization")
232+ require.Equal(t, orig, m.Headers, "receiver Headers must be preserved")
233+}
234+
235+// TestResolvedArgs_RealShell exercises both success and failure for
236+// m.Args symmetrically with Env. Args are ordered so error messages
237+// must identify a positional index, not a key.
238+func TestResolvedArgs_RealShell(t *testing.T) {
239+ t.Parallel()
240+
241+ t.Run("success expands each element", func(t *testing.T) {
242+ t.Parallel()
243+ m := MCPConfig{Args: []string{"--token", "$(echo shh)", "--host", "example.com"}}
244+ got, err := m.ResolvedArgs(realShellResolver(nil))
245+ require.NoError(t, err)
246+ require.Equal(t, []string{"--token", "shh", "--host", "example.com"}, got)
247+ })
248+
249+ t.Run("failure identifies offending index", func(t *testing.T) {
250+ t.Parallel()
251+ m := MCPConfig{Args: []string{"--token", "$(false)"}}
252+ orig := slices.Clone(m.Args)
253+
254+ got, err := m.ResolvedArgs(realShellResolver(nil))
255+ require.Error(t, err)
256+ require.Nil(t, got)
257+ require.Contains(t, err.Error(), "arg 1")
258+ require.Equal(t, orig, m.Args, "receiver Args must be preserved")
259+ })
260+
261+ t.Run("nil args returns nil, no error", func(t *testing.T) {
262+ t.Parallel()
263+ m := MCPConfig{}
264+ got, err := m.ResolvedArgs(realShellResolver(nil))
265+ require.NoError(t, err)
266+ require.Nil(t, got)
267+ })
268+}
269+
270+// TestMCPConfig_IdentityResolver pins the client-mode contract: every
271+// Resolved* method round-trips the template verbatim and never errors
272+// on unset variables. Local expansion would double-expand when the
273+// server does its own — this has to stay a pure pass-through.
274+func TestMCPConfig_IdentityResolver(t *testing.T) {
275+ t.Parallel()
276+
277+ m := MCPConfig{
278+ Command: "$CMD",
279+ Args: []string{"--token", "$MCP_MISSING_TOKEN", "$(vault read -f secret)"},
280+ Env: map[string]string{
281+ "TOKEN": "$(cat /run/secrets/x)",
282+ "HOST": "$MCP_MISSING_HOST",
283+ },
284+ Headers: map[string]string{
285+ "Authorization": "Bearer $(vault read -f token)",
286+ },
287+ URL: "https://$MCP_HOST/$(vault read -f path)",
288+ }
289+ r := IdentityResolver()
290+
291+ args, err := m.ResolvedArgs(r)
292+ require.NoError(t, err)
293+ require.Equal(t, m.Args, args)
294+
295+ envs, err := m.ResolvedEnv(r)
296+ require.NoError(t, err)
297+ // Sorted "KEY=value".
298+ require.Equal(t, []string{
299+ "HOST=$MCP_MISSING_HOST",
300+ "TOKEN=$(cat /run/secrets/x)",
301+ }, envs)
302+
303+ headers, err := m.ResolvedHeaders(r)
304+ require.NoError(t, err)
305+ require.Equal(t, m.Headers, headers)
306+
307+ u, err := m.ResolvedURL(r)
308+ require.NoError(t, err)
309+ require.Equal(t, m.URL, u)
310+}
internal/config/resolve_test.go+190−229
@@ -3,260 +3,221 @@ package config
3import (
3import (
4 "context"
4 "context"
5 "errors"
5 "errors"
6+ "strings"
6 "testing"
7 "testing"
7
8
8 "github.com/charmbracelet/crush/internal/env"
9 "github.com/charmbracelet/crush/internal/env"
9 "github.com/stretchr/testify/require"
10 "github.com/stretchr/testify/require"
10)
11)
11
12
12−// mockShell implements the Shell interface for testing
13+// fakeExpander returns a canned value/error for the last passed value and
14+// records the context, raw value, and env slice it was called with. It
15+// lets the config-layer tests assert on delegation behaviour without
16+// spinning up a real interpreter — real-shell coverage lives in
17+// internal/shell/expand_test.go and resolve_real_test.go.
13−type mockShell struct {
18+type fakeExpander struct {
14− execFunc func(ctx context.Context, command string) (stdout, stderr string, err error)
19+ expand func(ctx context.Context, value string, env []string) (string, error)
20+ lastValue string
21+ lastEnv []string
22+ calls int
15}
23}
16
24
17−func (m *mockShell) Exec(ctx context.Context, command string) (stdout, stderr string, err error) {
25+func (f *fakeExpander) Expand(ctx context.Context, value string, env []string) (string, error) {
18− if m.execFunc != nil {
26+ f.calls++
19− return m.execFunc(ctx, command)
27+ f.lastValue = value
28+ f.lastEnv = env
29+ if f.expand == nil {
30+ return value, nil
20 }
31 }
21− return "", "", nil
32+ return f.expand(ctx, value, env)
22}
33}
23
34
24−func TestShellVariableResolver_ResolveValue(t *testing.T) {
35+func TestShellVariableResolver_DelegatesToExpander(t *testing.T) {
25− tests := []struct {
36+ t.Parallel()
26− name string
27− value string
28− envVars map[string]string
29− shellFunc func(ctx context.Context, command string) (stdout, stderr string, err error)
30− expected string
31− expectError bool
32− }{
33− {
34− name: "non-variable string returns as-is",
35− value: "plain-string",
36− expected: "plain-string",
37− },
38− {
39− name: "environment variable resolution",
40− value: "$HOME",
41− envVars: map[string]string{"HOME": "/home/user"},
42− expected: "/home/user",
43− },
44− {
45− name: "missing environment variable returns error",
46− value: "$MISSING_VAR",
47− envVars: map[string]string{},
48− expectError: true,
49− },
50
37
51− {
38+ fe := &fakeExpander{
52− name: "shell command with whitespace trimming",
53− value: "$(echo ' spaced ')",
54− shellFunc: func(ctx context.Context, command string) (stdout, stderr string, err error) {
39+ expand: func(_ context.Context, value string, _ []string) (string, error) {
55− if command == "echo ' spaced '" {
40+ if value == "hello $FOO" {
56− return " spaced \n", "", nil
41+ return "hello bar", nil
57− }
58− return "", "", errors.New("unexpected command")
59− },
60− expected: "spaced",
61− },
62− {
63− name: "shell command execution error",
64− value: "$(false)",
65− shellFunc: func(ctx context.Context, command string) (stdout, stderr string, err error) {
66− return "", "", errors.New("command failed")
67− },
68− expectError: true,
69− },
42+ }
70− {
71− name: "invalid format returns error",
72− value: "$",
43+ return value, nil
73− expectError: true,
74 },
44 },
75 }
45 }
76
46
77− for _, tt := range tests {
78− t.Run(tt.name, func(t *testing.T) {
79− testEnv := env.NewFromMap(tt.envVars)
47+ e := env.NewFromMap(map[string]string{"FOO": "bar"})
80− resolver := &shellVariableResolver{
48+ r := NewShellVariableResolver(e, WithExpander(fe.Expand))
81− shell: &mockShell{execFunc: tt.shellFunc},
82− env: testEnv,
83− }
84
49
85− result, err := resolver.ResolveValue(tt.value)
50+ got, err := r.ResolveValue("hello $FOO")
51+ require.NoError(t, err)
52+ require.Equal(t, "hello bar", got)
53+ require.Equal(t, 1, fe.calls)
54+ require.Equal(t, "hello $FOO", fe.lastValue)
55+ require.Contains(t, fe.lastEnv, "FOO=bar")
56+}
86
57
87− if tt.expectError {
58+func TestShellVariableResolver_LoneDollarIsError(t *testing.T) {
59+ t.Parallel()
60+
61+ // Lone "$" must short-circuit before reaching the expander: the
62+ // underlying shell parser would accept it as a literal, but this
63+ // resolver has historically rejected it and callers depend on
64+ // that early-fail behaviour.
65+ fe := &fakeExpander{}
66+ r := NewShellVariableResolver(env.NewFromMap(nil), WithExpander(fe.Expand))
67+
68+ _, err := r.ResolveValue("$")
88− require.Error(t, err)
69+ require.Error(t, err)
89− } else {
70+ require.Equal(t, 0, fe.calls, "expander must not be called for lone $")
90− require.NoError(t, err)
91− require.Equal(t, tt.expected, result)
92− }
93− })
94− }
95}
71}
96
72
97−func TestShellVariableResolver_EnhancedResolveValue(t *testing.T) {
73+func TestShellVariableResolver_PassesThroughLiterals(t *testing.T) {
98− tests := []struct {
74+ t.Parallel()
99− name string
75+
100− value string
76+ fe := &fakeExpander{
101− envVars map[string]string
77+ expand: func(_ context.Context, value string, _ []string) (string, error) {
102− shellFunc func(ctx context.Context, command string) (stdout, stderr string, err error)
103− expected string
104− expectError bool
105− }{
106− {
107− name: "command substitution within string",
108− value: "Bearer $(echo token123)",
109− shellFunc: func(ctx context.Context, command string) (stdout, stderr string, err error) {
110− if command == "echo token123" {
111− return "token123\n", "", nil
112− }
113− return "", "", errors.New("unexpected command")
114− },
115− expected: "Bearer token123",
116− },
117− {
118− name: "environment variable within string",
119− value: "Bearer $TOKEN",
120− envVars: map[string]string{"TOKEN": "sk-ant-123"},
121− expected: "Bearer sk-ant-123",
122− },
123− {
124− name: "environment variable with braces within string",
125− value: "Bearer ${TOKEN}",
126− envVars: map[string]string{"TOKEN": "sk-ant-456"},
127− expected: "Bearer sk-ant-456",
128− },
129− {
130− name: "mixed command and environment substitution",
131− value: "$USER-$(date +%Y)-$HOST",
132− envVars: map[string]string{
133− "USER": "testuser",
134− "HOST": "localhost",
135− },
136− shellFunc: func(ctx context.Context, command string) (stdout, stderr string, err error) {
137− if command == "date +%Y" {
138− return "2024\n", "", nil
139− }
140− return "", "", errors.New("unexpected command")
141− },
142− expected: "testuser-2024-localhost",
143− },
144− {
145− name: "multiple command substitutions",
146− value: "$(echo hello) $(echo world)",
147− shellFunc: func(ctx context.Context, command string) (stdout, stderr string, err error) {
148− switch command {
149− case "echo hello":
150− return "hello\n", "", nil
78+ return value, nil
151− case "echo world":
152− return "world\n", "", nil
153− }
154− return "", "", errors.New("unexpected command")
155− },
156− expected: "hello world",
157− },
158− {
159− name: "nested parentheses in command",
160− value: "$(echo $(echo inner))",
161− shellFunc: func(ctx context.Context, command string) (stdout, stderr string, err error) {
162− if command == "echo $(echo inner)" {
163− return "nested\n", "", nil
164− }
165− return "", "", errors.New("unexpected command")
166− },
167− expected: "nested",
168− },
169− {
170− name: "lone dollar with non-variable chars",
171− value: "prefix$123suffix", // Numbers can't start variable names
172− expectError: true,
173− },
174− {
175− name: "dollar with special chars",
176− value: "a$@b$#c", // Special chars aren't valid in variable names
177− expectError: true,
178− },
179− {
180− name: "empty environment variable substitution",
181− value: "Bearer $EMPTY_VAR",
182− envVars: map[string]string{},
183− expectError: true,
184− },
185− {
186− name: "unmatched command substitution opening",
187− value: "Bearer $(echo test",
188− expectError: true,
189− },
190− {
191− name: "unmatched environment variable braces",
192− value: "Bearer ${TOKEN",
193− expectError: true,
194− },
195− {
196− name: "command substitution with error",
197− value: "Bearer $(false)",
198− shellFunc: func(ctx context.Context, command string) (stdout, stderr string, err error) {
199− return "", "", errors.New("command failed")
200− },
201− expectError: true,
202− },
203− {
204− name: "complex real-world example",
205− value: "Bearer $(cat /tmp/token.txt | base64 -w 0)",
206− shellFunc: func(ctx context.Context, command string) (stdout, stderr string, err error) {
207− if command == "cat /tmp/token.txt | base64 -w 0" {
208− return "c2stYW50LXRlc3Q=\n", "", nil
209− }
210− return "", "", errors.New("unexpected command")
211− },
212− expected: "Bearer c2stYW50LXRlc3Q=",
213− },
214− {
215− name: "environment variable with underscores and numbers",
216− value: "Bearer $API_KEY_V2",
217− envVars: map[string]string{"API_KEY_V2": "sk-test-123"},
218− expected: "Bearer sk-test-123",
219− },
220− {
221− name: "no substitution needed",
222− value: "Bearer sk-ant-static-token",
223− expected: "Bearer sk-ant-static-token",
224− },
225− {
226− name: "incomplete variable at end",
227− value: "Bearer $",
228− expectError: true,
229− },
230− {
231− name: "variable with invalid character",
232− value: "Bearer $VAR-NAME", // Hyphen not allowed in variable names
233− expectError: true,
234− },
235− {
236− name: "multiple invalid variables",
237− value: "$1$2$3",
238− expectError: true,
239 },
79 },
240 }
80 }
81+ r := NewShellVariableResolver(env.NewFromMap(nil), WithExpander(fe.Expand))
241
82
242− for _, tt := range tests {
83+ got, err := r.ResolveValue("plain-string")
243− t.Run(tt.name, func(t *testing.T) {
84+ require.NoError(t, err)
244− testEnv := env.NewFromMap(tt.envVars)
85+ require.Equal(t, "plain-string", got)
245− resolver := &shellVariableResolver{
246− shell: &mockShell{execFunc: tt.shellFunc},
247− env: testEnv,
248− }
86+}
249
87
250− result, err := resolver.ResolveValue(tt.value)
88+func TestShellVariableResolver_WrapsErrorsWithTemplate(t *testing.T) {
89+ t.Parallel()
251
90
252− if tt.expectError {
91+ inner := errors.New("cat: /run/secrets/x: permission denied")
253− require.Error(t, err)
92+ fe := &fakeExpander{
254− } else {
93+ expand: func(_ context.Context, _ string, _ []string) (string, error) {
255− require.NoError(t, err)
94+ return "", inner
256− require.Equal(t, tt.expected, result)
257− }
258− })
95+ },
259 }
96 }
97+ r := NewShellVariableResolver(env.NewFromMap(nil), WithExpander(fe.Expand))
98+
99+ _, err := r.ResolveValue("$(cat /run/secrets/x)")
100+ require.Error(t, err)
101+ require.ErrorIs(t, err, inner)
102+ require.Contains(t, err.Error(), "$(cat /run/secrets/x)")
103+ require.Contains(t, err.Error(), "permission denied")
104+}
105+
106+func TestSanitizeResolveError(t *testing.T) {
107+ t.Parallel()
108+
109+ t.Run("nil passes through", func(t *testing.T) {
110+ t.Parallel()
111+ require.NoError(t, sanitizeResolveError("anything", nil))
112+ })
113+
114+ t.Run("includes template and wraps inner", func(t *testing.T) {
115+ t.Parallel()
116+ inner := errors.New("cat: /run/secrets/x: permission denied")
117+ got := sanitizeResolveError("$(cat /run/secrets/x)", inner)
118+ require.Error(t, got)
119+ require.ErrorIs(t, got, inner)
120+ require.Contains(t, got.Error(), "$(cat /run/secrets/x)")
121+ require.Contains(t, got.Error(), "permission denied")
122+ })
123+
124+ t.Run("unwrap preserves original for errors.Is", func(t *testing.T) {
125+ t.Parallel()
126+ inner := errors.New("sentinel")
127+ got := sanitizeResolveError("$FOO", inner)
128+ require.ErrorIs(t, got, inner)
129+ })
130+
131+ t.Run("truncates over-budget inner message", func(t *testing.T) {
132+ t.Parallel()
133+ // Inner message holds far more than the budget. After
134+ // sanitization the rendered inner portion must not exceed
135+ // maxResolveErrBytes, and the characters beyond the budget
136+ // (marked by a distinct tail sentinel) must be gone.
137+ const tailSentinel = "TAIL_SENTINEL_BEYOND_BUDGET"
138+ body := strings.Repeat("x", maxResolveErrBytes)
139+ inner := errors.New(body + tailSentinel)
140+
141+ got := sanitizeResolveError("$TEMPLATE", inner)
142+ require.Error(t, got)
143+
144+ prefix := `resolving "$TEMPLATE": `
145+ rendered := got.Error()
146+ require.True(
147+ t,
148+ strings.HasPrefix(rendered, prefix),
149+ "rendered error must start with template prefix",
150+ )
151+ innerRendered := strings.TrimPrefix(rendered, prefix)
152+ require.LessOrEqual(
153+ t,
154+ len(innerRendered),
155+ maxResolveErrBytes,
156+ "inner message must be bounded to maxResolveErrBytes",
157+ )
158+ require.NotContains(
159+ t,
160+ rendered,
161+ tailSentinel,
162+ "content past the budget must not leak",
163+ )
164+ })
165+
166+ t.Run("replaces non-printable bytes", func(t *testing.T) {
167+ t.Parallel()
168+ // NUL, BEL, ESC, DEL, and a UTF-8 high byte should all be
169+ // scrubbed to '?'. Tab and newline are preserved because
170+ // they show up legitimately in command stderr.
171+ inner := errors.New("ok\x00bad\x07\x1b\x7f\xffend\ttab\nline")
172+ got := sanitizeResolveError("$T", inner)
173+ rendered := got.Error()
174+
175+ require.NotContains(t, rendered, "\x00")
176+ require.NotContains(t, rendered, "\x07")
177+ require.NotContains(t, rendered, "\x1b")
178+ require.NotContains(t, rendered, "\x7f")
179+ require.NotContains(t, rendered, "\xff")
180+ require.Contains(t, rendered, "ok?bad????end\ttab\nline")
181+ })
182+
183+ t.Run("scrubbing does not depend on shell.ExpandValue upstream", func(t *testing.T) {
184+ t.Parallel()
185+ // A custom Expander can inject arbitrary error text. The
186+ // config-layer helper is the single chokepoint; it must
187+ // bound + scrub regardless of the error source.
188+ nasty := strings.Repeat("A", maxResolveErrBytes+64) + "\x00BEYOND"
189+ fe := &fakeExpander{
190+ expand: func(_ context.Context, _ string, _ []string) (string, error) {
191+ return "", errors.New(nasty)
192+ },
193+ }
194+ r := NewShellVariableResolver(env.NewFromMap(nil), WithExpander(fe.Expand))
195+
196+ _, err := r.ResolveValue("$T")
197+ require.Error(t, err)
198+ require.NotContains(t, err.Error(), "BEYOND", "over-budget tail must not leak")
199+ require.NotContains(t, err.Error(), "\x00", "non-printables must be scrubbed")
200+ })
201+}
202+
203+func TestScrubErrorMessage(t *testing.T) {
204+ t.Parallel()
205+
206+ t.Run("bounds output to maxResolveErrBytes", func(t *testing.T) {
207+ t.Parallel()
208+ got := scrubErrorMessage(strings.Repeat("a", maxResolveErrBytes*3))
209+ require.Len(t, got, maxResolveErrBytes)
210+ })
211+
212+ t.Run("preserves printable ASCII tab and newline", func(t *testing.T) {
213+ t.Parallel()
214+ require.Equal(t, "a\tb\nc d!", scrubErrorMessage("a\tb\nc d!"))
215+ })
216+
217+ t.Run("replaces control and non-ASCII bytes", func(t *testing.T) {
218+ t.Parallel()
219+ require.Equal(t, "a?b??c", scrubErrorMessage("a\x01b\x1b\xe2c"))
220+ })
260}
221}
261
222
262func TestEnvironmentVariableResolver_ResolveValue(t *testing.T) {
223func TestEnvironmentVariableResolver_ResolveValue(t *testing.T) {
internal/shell/expand.gonew file+123
@@ -0,0 +1,123 @@
1+package shell
2+
3+import (
4+ "bytes"
5+ "context"
6+ "fmt"
7+ "io"
8+ "os"
9+ "strings"
10+
11+ "mvdan.cc/sh/v3/expand"
12+ "mvdan.cc/sh/v3/interp"
13+ "mvdan.cc/sh/v3/syntax"
14+)
15+
16+// maxInnerStderrBytes bounds how much stderr from a failing $(...) is
17+// surfaced in the returned error, to avoid leaking a secret that happened
18+// to be embedded in a failing inner command.
19+const maxInnerStderrBytes = 512
20+
21+// ExpandValue expands shell-style substitutions in a single config value.
22+//
23+// Supported constructs match the bash tool:
24+//
25+// - $VAR and ${VAR} (unset is an error; see nounset below).
26+// - ${VAR:-default} / ${VAR:+alt} / ${VAR:?msg}.
27+// - $(command) with full quoting and nesting.
28+// - escaped and quoted strings ("...", '...').
29+//
30+// Contract:
31+//
32+// - Returns exactly one string. No field splitting, no globbing, no
33+// pathname generation. Multi-word command output is preserved
34+// verbatim; it is never split into multiple values.
35+// - Nounset is on: unset variables produce an error instead of
36+// expanding to the empty string. Use ${VAR:-default} to opt in to
37+// an empty fallback.
38+// - Embedded whitespace and newlines in the input are preserved
39+// verbatim. Command substitution strips trailing newlines only
40+// (POSIX), never leading or internal whitespace.
41+// - Errors wrap the failing inner command's exit code and a bounded
42+// prefix of its stderr. Callers that surface the error to users
43+// should additionally scrub it for the original template text.
44+func ExpandValue(ctx context.Context, value string, env []string) (string, error) {
45+ // Parse the value as a here-doc style word: no word splitting, no
46+ // globbing, but full support for $VAR, ${VAR...}, $(...), and
47+ // quoted/escaped strings.
48+ word, err := syntax.NewParser().Document(strings.NewReader(value))
49+ if err != nil {
50+ return "", fmt.Errorf("parse: %w", err)
51+ }
52+
53+ // Build a minimal Shell value purely to reuse its handler chain
54+ // (builtins, block funcs, optional Go coreutils) inside $(...).
55+ // We deliberately skip NewShell so the passed-in env is used
56+ // verbatim, with no CRUSH/AGENT/AI_AGENT injection: callers of
57+ // ExpandValue control the env, and nounset must treat any name
58+ // not in env as unset.
59+ cwd, _ := os.Getwd()
60+ s := &Shell{
61+ cwd: cwd,
62+ env: env,
63+ logger: noopLogger{},
64+ }
65+
66+ var stderrBuf bytes.Buffer
67+ cfg := &expand.Config{
68+ Env: expand.ListEnviron(env...),
69+ NoUnset: true,
70+ CmdSubst: func(w io.Writer, cs *syntax.CmdSubst) error {
71+ stderrBuf.Reset()
72+ runner, rerr := interp.New(
73+ interp.StdIO(nil, w, &stderrBuf),
74+ interp.Interactive(false),
75+ interp.Env(expand.ListEnviron(env...)),
76+ interp.Dir(s.cwd),
77+ interp.ExecHandlers(s.execHandlers()...),
78+ // Match the outer NoUnset: an unset $VAR inside
79+ // $(...) is also an error, not a silent empty.
80+ interp.Params("-u"),
81+ )
82+ if rerr != nil {
83+ return rerr
84+ }
85+ if rerr := runner.Run(ctx, &syntax.File{Stmts: cs.Stmts}); rerr != nil {
86+ return wrapCmdSubstErr(rerr, stderrBuf.Bytes())
87+ }
88+ return nil
89+ },
90+ // ReadDir / ReadDir2 left nil: globbing is disabled.
91+ }
92+
93+ return expand.Document(cfg, word)
94+}
95+
96+// wrapCmdSubstErr attaches a bounded prefix of the inner command's stderr
97+// to the original error, if any.
98+func wrapCmdSubstErr(err error, stderrBytes []byte) error {
99+ msg := sanitizeStderr(stderrBytes)
100+ if msg == "" {
101+ return err
102+ }
103+ return fmt.Errorf("%w: %s", err, msg)
104+}
105+
106+// sanitizeStderr trims, bounds, and scrubs non-printable bytes from the
107+// stderr of a failing command so the result is safe to include in an
108+// error message shown to the user.
109+func sanitizeStderr(b []byte) string {
110+ b = bytes.TrimRight(b, "\n")
111+ if len(b) > maxInnerStderrBytes {
112+ b = b[:maxInnerStderrBytes]
113+ }
114+ out := make([]byte, len(b))
115+ for i, c := range b {
116+ if c == '\t' || c == '\n' || (c >= 0x20 && c < 0x7f) {
117+ out[i] = c
118+ } else {
119+ out[i] = '?'
120+ }
121+ }
122+ return string(out)
123+}
internal/shell/expand_test.gonew file+199
@@ -0,0 +1,199 @@
1+package shell
2+
3+import (
4+ "strings"
5+ "testing"
6+
7+ "github.com/stretchr/testify/require"
8+)
9+
10+func TestExpandValue_Success(t *testing.T) {
11+ t.Parallel()
12+
13+ tests := []struct {
14+ name string
15+ value string
16+ env []string
17+ want string
18+ }{
19+ {
20+ name: "plain string round trip",
21+ value: "hello world",
22+ want: "hello world",
23+ },
24+ {
25+ name: "plain var from env",
26+ value: "$FOO",
27+ env: []string{"FOO=bar"},
28+ want: "bar",
29+ },
30+ {
31+ name: "braced var from env",
32+ value: "pre-${FOO}-post",
33+ env: []string{"FOO=bar"},
34+ want: "pre-bar-post",
35+ },
36+ {
37+ name: "default syntax on unset",
38+ value: "${MISSING:-fallback}",
39+ want: "fallback",
40+ },
41+ {
42+ name: "default syntax on set preserves value",
43+ value: "${SET:-fallback}",
44+ env: []string{"SET=used"},
45+ want: "used",
46+ },
47+ {
48+ name: "command substitution",
49+ value: "$(echo hi)",
50+ want: "hi",
51+ },
52+ {
53+ name: "command substitution preserves internal spaces",
54+ value: `$(echo "a b")`,
55+ want: "a b",
56+ },
57+ {
58+ name: "command substitution strips only trailing newline",
59+ value: "$(printf 'a\\nb\\n')",
60+ want: "a\nb",
61+ },
62+ {
63+ name: "literal spaces around cmdsubst are preserved",
64+ value: " $(echo v) ",
65+ want: " v ",
66+ },
67+ {
68+ name: "paren inside quoted arg to echo",
69+ value: `$(echo ")")`,
70+ want: ")",
71+ },
72+ {
73+ name: "nested command substitution",
74+ value: "$(echo $(echo hi))",
75+ want: "hi",
76+ },
77+ {
78+ name: "glob-like input round trips unchanged",
79+ value: "*.go",
80+ want: "*.go",
81+ },
82+ {
83+ name: "tilde round trips unchanged",
84+ value: "~",
85+ want: "~",
86+ },
87+ {
88+ name: "env var inside cmdsubst",
89+ value: `$(printf '%s' "$FOO")`,
90+ env: []string{"FOO=bar"},
91+ want: "bar",
92+ },
93+ }
94+
95+ for _, tc := range tests {
96+ t.Run(tc.name, func(t *testing.T) {
97+ t.Parallel()
98+ got, err := ExpandValue(t.Context(), tc.value, tc.env)
99+ require.NoError(t, err)
100+ require.Equal(t, tc.want, got)
101+ })
102+ }
103+}
104+
105+func TestExpandValue_Errors(t *testing.T) {
106+ t.Parallel()
107+
108+ t.Run("unset var is an error, not empty", func(t *testing.T) {
109+ t.Parallel()
110+ _, err := ExpandValue(t.Context(), "$MISSING", nil)
111+ require.Error(t, err)
112+ })
113+
114+ t.Run("unset var inside braces is an error", func(t *testing.T) {
115+ t.Parallel()
116+ _, err := ExpandValue(t.Context(), "${MISSING}", nil)
117+ require.Error(t, err)
118+ })
119+
120+ t.Run("unset var inside cmdsubst is an error", func(t *testing.T) {
121+ t.Parallel()
122+ _, err := ExpandValue(t.Context(), `$(printf '%s' "$MISSING")`, nil)
123+ require.Error(t, err)
124+ })
125+
126+ t.Run("bad syntax returns error", func(t *testing.T) {
127+ t.Parallel()
128+ _, err := ExpandValue(t.Context(), "$(", nil)
129+ require.Error(t, err)
130+ })
131+
132+ t.Run("inner non-zero exit returns error with exit code", func(t *testing.T) {
133+ t.Parallel()
134+ _, err := ExpandValue(t.Context(), "$(false)", nil)
135+ require.Error(t, err)
136+ require.Contains(t, err.Error(), "exit status 1")
137+ })
138+
139+ t.Run("inner explicit exit code is surfaced", func(t *testing.T) {
140+ t.Parallel()
141+ _, err := ExpandValue(t.Context(), "$(exit 7)", nil)
142+ require.Error(t, err)
143+ require.Contains(t, err.Error(), "exit status 7")
144+ })
145+
146+ t.Run("inner stderr is surfaced", func(t *testing.T) {
147+ t.Parallel()
148+ _, err := ExpandValue(
149+ t.Context(),
150+ `$(printf 'boom\n' 1>&2; exit 1)`,
151+ nil,
152+ )
153+ require.Error(t, err)
154+ require.Contains(t, err.Error(), "boom")
155+ })
156+
157+ t.Run("inner stderr is truncated to byte budget", func(t *testing.T) {
158+ t.Parallel()
159+ // Emit more than maxInnerStderrBytes bytes of 'X' on stderr.
160+ long := strings.Repeat("X", maxInnerStderrBytes*2)
161+ _, err := ExpandValue(
162+ t.Context(),
163+ `$(printf '`+long+`' 1>&2; exit 1)`,
164+ nil,
165+ )
166+ require.Error(t, err)
167+ require.NotContains(
168+ t,
169+ err.Error(),
170+ strings.Repeat("X", maxInnerStderrBytes+1),
171+ "stderr should be bounded",
172+ )
173+ })
174+}
175+
176+func TestSanitizeStderr(t *testing.T) {
177+ t.Parallel()
178+
179+ t.Run("trims trailing newlines", func(t *testing.T) {
180+ t.Parallel()
181+ require.Equal(t, "hi", sanitizeStderr([]byte("hi\n\n")))
182+ })
183+
184+ t.Run("preserves tabs and embedded newlines", func(t *testing.T) {
185+ t.Parallel()
186+ require.Equal(t, "a\tb\nc", sanitizeStderr([]byte("a\tb\nc")))
187+ })
188+
189+ t.Run("replaces control characters", func(t *testing.T) {
190+ t.Parallel()
191+ require.Equal(t, "a?b", sanitizeStderr([]byte{'a', 0x01, 'b'}))
192+ })
193+
194+ t.Run("bounds output", func(t *testing.T) {
195+ t.Parallel()
196+ got := sanitizeStderr([]byte(strings.Repeat("x", maxInnerStderrBytes*2)))
197+ require.Len(t, got, maxInnerStderrBytes)
198+ })
199+}