@@ -2,6 +2,8 @@ package mcp
import (
"context"
+ "maps"
+ "os"
"testing"
"github.com/charmbracelet/crush/internal/config"
@@ -11,6 +13,17 @@ import (
"go.uber.org/goleak"
)
+// shellResolverWithPath builds a shell resolver whose env carries PATH
+// plus any caller-supplied overrides. Without PATH, $(cat), $(echo),
+// etc. can't find their binaries in a test process where the shell env
+// is otherwise empty.
+func shellResolverWithPath(t *testing.T, overrides map[string]string) config.VariableResolver {
+ t.Helper()
+ m := map[string]string{"PATH": os.Getenv("PATH")}
+ maps.Copy(m, overrides)
+ return config.NewShellVariableResolver(env.NewFromMap(m))
+}
+
func TestMCPSession_CancelOnClose(t *testing.T) {
defer goleak.VerifyNone(t)
@@ -132,3 +145,285 @@ func TestCreateTransport_URLResolution(t *testing.T) {
require.Equal(t, tmpl, sct.Endpoint)
})
}
+
+// TestCreateTransport_StdioResolution pins that command, args, and env
+// for stdio MCPs go through the same resolver seam as the other
+// transports. Covers both success (expansion produced the expected
+// exec.Cmd) and failure (any one field erroring prevents transport
+// creation).
+func TestCreateTransport_StdioResolution(t *testing.T) {
+ t.Parallel()
+
+ t.Run("success expands command, args, and env", func(t *testing.T) {
+ t.Parallel()
+ r := shellResolverWithPath(t, map[string]string{
+ "MY_TOKEN": "hunter2",
+ })
+ m := config.MCPConfig{
+ Type: config.MCPStdio,
+ Command: "forgejo-mcp",
+ Args: []string{"--token", "$MY_TOKEN", "--host", "$(echo example.com)"},
+ Env: map[string]string{
+ "SECRET": "$(echo shh)",
+ "PLAIN": "literal",
+ "REFERENCE": "$MY_TOKEN",
+ },
+ }
+ tr, err := createTransport(t.Context(), m, r)
+ require.NoError(t, err)
+ require.NotNil(t, tr)
+
+ ct, ok := tr.(*mcp.CommandTransport)
+ require.True(t, ok, "expected CommandTransport, got %T", tr)
+
+ // exec.Cmd.Args[0] is the command name; the rest are positional
+ // args as passed.
+ require.Equal(t, []string{"forgejo-mcp", "--token", "hunter2", "--host", "example.com"}, ct.Command.Args)
+
+ // Env is os.Environ() + resolved entries (sorted). Check the
+ // resolved entries are present with their expanded values.
+ require.Contains(t, ct.Command.Env, "SECRET=shh")
+ require.Contains(t, ct.Command.Env, "PLAIN=literal")
+ require.Contains(t, ct.Command.Env, "REFERENCE=hunter2")
+ })
+
+ t.Run("env resolution failure surfaces error, no transport created", func(t *testing.T) {
+ t.Parallel()
+ r := shellResolverWithPath(t, nil)
+ m := config.MCPConfig{
+ Type: config.MCPStdio,
+ Command: "forgejo-mcp",
+ Env: map[string]string{"TOKEN": "$(false)"},
+ }
+ tr, err := createTransport(t.Context(), m, r)
+ require.Error(t, err)
+ require.Nil(t, tr)
+ require.Contains(t, err.Error(), "env TOKEN")
+ })
+
+ t.Run("unset env var is a hard error, not silent empty", func(t *testing.T) {
+ t.Parallel()
+ // The regression at the heart of PLAN.md: unset vars used to
+ // silently substitute "" and hand an empty credential to the
+ // child process. Now they must error out before exec.
+ r := shellResolverWithPath(t, nil)
+ m := config.MCPConfig{
+ Type: config.MCPStdio,
+ Command: "forgejo-mcp",
+ Env: map[string]string{"FORGEJO_ACCESS_TOKEN": "$FORGJO_TOKEN"},
+ }
+ tr, err := createTransport(t.Context(), m, r)
+ require.Error(t, err)
+ require.Nil(t, tr)
+ require.Contains(t, err.Error(), "env FORGEJO_ACCESS_TOKEN")
+ })
+
+ t.Run("args resolution failure surfaces error, no transport created", func(t *testing.T) {
+ t.Parallel()
+ r := shellResolverWithPath(t, nil)
+ m := config.MCPConfig{
+ Type: config.MCPStdio,
+ Command: "forgejo-mcp",
+ Args: []string{"--token", "$(false)"},
+ }
+ tr, err := createTransport(t.Context(), m, r)
+ require.Error(t, err)
+ require.Nil(t, tr)
+ require.Contains(t, err.Error(), "arg 1")
+ })
+
+ t.Run("command resolution failure surfaces error, no transport created", func(t *testing.T) {
+ t.Parallel()
+ r := shellResolverWithPath(t, nil)
+ m := config.MCPConfig{
+ Type: config.MCPStdio,
+ Command: "$(false)",
+ }
+ tr, err := createTransport(t.Context(), m, r)
+ require.Error(t, err)
+ require.Nil(t, tr)
+ require.Contains(t, err.Error(), "invalid mcp command")
+ })
+
+ t.Run("identity resolver round-trips templates verbatim", func(t *testing.T) {
+ t.Parallel()
+ // Client mode: no local expansion, no error on unset vars.
+ m := config.MCPConfig{
+ Type: config.MCPStdio,
+ Command: "forgejo-mcp",
+ Args: []string{"--token", "$MCP_MISSING"},
+ Env: map[string]string{"TOKEN": "$(vault read -f token)"},
+ }
+ tr, err := createTransport(t.Context(), m, config.IdentityResolver())
+ require.NoError(t, err)
+ ct, ok := tr.(*mcp.CommandTransport)
+ require.True(t, ok)
+ require.Equal(t, []string{"forgejo-mcp", "--token", "$MCP_MISSING"}, ct.Command.Args)
+ require.Contains(t, ct.Command.Env, "TOKEN=$(vault read -f token)")
+ })
+}
+
+// TestCreateTransport_HeadersResolution pins that a single failing
+// header aborts HTTP/SSE transport creation and that the successful
+// resolver passes every expanded header through to the round tripper.
+func TestCreateTransport_HeadersResolution(t *testing.T) {
+ t.Parallel()
+
+ t.Run("http headers success expands $(cmd)", func(t *testing.T) {
+ t.Parallel()
+ r := shellResolverWithPath(t, map[string]string{
+ "GITHUB_TOKEN": "gh-secret",
+ })
+ m := config.MCPConfig{
+ Type: config.MCPHttp,
+ URL: "https://mcp.example.com/api",
+ Headers: map[string]string{
+ "Authorization": "$(echo Bearer $GITHUB_TOKEN)",
+ "X-Static": "kept",
+ },
+ }
+ tr, err := createTransport(t.Context(), m, r)
+ require.NoError(t, err)
+
+ sct, ok := tr.(*mcp.StreamableClientTransport)
+ require.True(t, ok)
+ rt, ok := sct.HTTPClient.Transport.(*headerRoundTripper)
+ require.True(t, ok, "expected headerRoundTripper, got %T", sct.HTTPClient.Transport)
+ require.Equal(t, map[string]string{
+ "Authorization": "Bearer gh-secret",
+ "X-Static": "kept",
+ }, rt.headers)
+ })
+
+ t.Run("http failing header surfaces error, no transport", func(t *testing.T) {
+ t.Parallel()
+ r := shellResolverWithPath(t, nil)
+ m := config.MCPConfig{
+ Type: config.MCPHttp,
+ URL: "https://mcp.example.com/api",
+ Headers: map[string]string{"Authorization": "$(false)"},
+ }
+ tr, err := createTransport(t.Context(), m, r)
+ require.Error(t, err)
+ require.Nil(t, tr)
+ require.Contains(t, err.Error(), "header Authorization")
+ })
+
+ t.Run("sse failing header surfaces error, no transport", func(t *testing.T) {
+ t.Parallel()
+ r := shellResolverWithPath(t, nil)
+ m := config.MCPConfig{
+ Type: config.MCPSSE,
+ URL: "https://mcp.example.com/events",
+ Headers: map[string]string{"Authorization": "Bearer $MISSING_TOKEN"},
+ }
+ tr, err := createTransport(t.Context(), m, r)
+ require.Error(t, err)
+ require.Nil(t, tr)
+ require.Contains(t, err.Error(), "header Authorization")
+ require.Contains(t, err.Error(), "$MISSING_TOKEN")
+ })
+}
+
+// TestCreateSession_ResolutionFailureUpdatesState pins the user-visible
+// half of the regression fix: when any of command/args/env/headers/url
+// fails to resolve, createSession must publish StateError to the state
+// map so crush_info and the TUI's MCP status card can render a real
+// error instead of the MCP silently sitting in "starting" or being
+// spawned with an empty credential.
+//
+// These subtests cannot run in parallel: `states` is a package-level
+// csync.Map and each assertion reads the entry written by the call
+// under test. They do use unique MCP names per subtest to keep them
+// independent regardless of ordering.
+func TestCreateSession_ResolutionFailureUpdatesState(t *testing.T) {
+ r := shellResolverWithPath(t, nil)
+
+ tests := []struct {
+ name string
+ mcpName string
+ cfg config.MCPConfig
+ wantErrContains string
+ }{
+ {
+ name: "stdio env failure",
+ mcpName: "test-stdio-env-fail",
+ cfg: config.MCPConfig{
+ Type: config.MCPStdio,
+ Command: "echo",
+ Env: map[string]string{"FORGEJO_ACCESS_TOKEN": "$(false)"},
+ },
+ wantErrContains: "env FORGEJO_ACCESS_TOKEN",
+ },
+ {
+ name: "stdio args failure",
+ mcpName: "test-stdio-args-fail",
+ cfg: config.MCPConfig{
+ Type: config.MCPStdio,
+ Command: "echo",
+ Args: []string{"--token", "$MCP_UNSET_TOKEN"},
+ },
+ wantErrContains: "arg 1",
+ },
+ {
+ name: "http url failure",
+ mcpName: "test-http-url-fail",
+ cfg: config.MCPConfig{
+ Type: config.MCPHttp,
+ URL: "https://$MCP_MISSING_HOST/api",
+ },
+ wantErrContains: "url:",
+ },
+ {
+ name: "http header failure",
+ mcpName: "test-http-header-fail",
+ cfg: config.MCPConfig{
+ Type: config.MCPHttp,
+ URL: "https://mcp.example.com/api",
+ Headers: map[string]string{"Authorization": "$(false)"},
+ },
+ wantErrContains: "header Authorization",
+ },
+ {
+ name: "sse url failure",
+ mcpName: "test-sse-url-fail",
+ cfg: config.MCPConfig{
+ Type: config.MCPSSE,
+ URL: "https://$(false)/events",
+ },
+ wantErrContains: "url:",
+ },
+ {
+ name: "sse header failure",
+ mcpName: "test-sse-header-fail",
+ cfg: config.MCPConfig{
+ Type: config.MCPSSE,
+ URL: "https://mcp.example.com/events",
+ Headers: map[string]string{"Authorization": "Bearer $MISSING_SSE_TOKEN"},
+ },
+ wantErrContains: "header Authorization",
+ },
+ }
+
+ for _, tc := range tests {
+ t.Run(tc.name, func(t *testing.T) {
+ // Guarantee a clean slate on the shared state map so a
+ // stale entry from another test can't satisfy the
+ // assertion.
+ states.Del(tc.mcpName)
+ t.Cleanup(func() { states.Del(tc.mcpName) })
+
+ sess, err := createSession(t.Context(), tc.mcpName, tc.cfg, r)
+ require.Error(t, err)
+ require.Nil(t, sess)
+ require.Contains(t, err.Error(), tc.wantErrContains)
+
+ info, ok := GetState(tc.mcpName)
+ require.True(t, ok, "state entry must be written for %q", tc.mcpName)
+ require.Equal(t, StateError, info.State, "expected StateError, got %s", info.State)
+ require.Error(t, info.Error, "state must carry the failure error")
+ require.Contains(t, info.Error.Error(), tc.wantErrContains)
+ require.Nil(t, info.Client, "no client session on failure")
+ })
+ }
+}
@@ -0,0 +1,294 @@
+package config
+
+import (
+ "fmt"
+ "maps"
+ "os"
+ "path/filepath"
+ "slices"
+ "testing"
+
+ "github.com/charmbracelet/crush/internal/env"
+ "github.com/stretchr/testify/require"
+)
+
+// These tests exercise the full shell-expansion path (no mocks,
+// no injected Expander) to catch regressions that only surface when
+// internal/shell actually runs the value. Table-level unit tests with
+// fake expanders live in resolve_test.go.
+
+// realShellResolver builds a resolver backed by a shell env that
+// contains PATH + the caller-supplied overrides. Production callers
+// get PATH for free via env.New(); these tests need it so $(cat ...)
+// and similar inner commands can resolve.
+func realShellResolver(vars map[string]string) VariableResolver {
+ m := map[string]string{"PATH": os.Getenv("PATH")}
+ maps.Copy(m, vars)
+ return NewShellVariableResolver(env.NewFromMap(m))
+}
+
+func writeTempFile(t *testing.T, content string) string {
+ t.Helper()
+ dir := t.TempDir()
+ p := filepath.Join(dir, "secret")
+ require.NoError(t, os.WriteFile(p, []byte(content), 0o600))
+ return p
+}
+
+// TestResolvedEnv_RealShell_Success covers the shell constructs the
+// PLAN calls out: $(cat tempfile) with and without trailing newline,
+// ${VAR:-default} for unset vars, literal-space preservation around
+// $(...), nested parens, quoted args inside $(echo ...), and a
+// glob-like literal round-tripping unchanged.
+func TestResolvedEnv_RealShell_Success(t *testing.T) {
+ t.Parallel()
+
+ withNL := writeTempFile(t, "token-with-nl\n")
+ noNL := writeTempFile(t, "token-no-nl")
+
+ m := MCPConfig{
+ Env: map[string]string{
+ // POSIX strips exactly one trailing newline from $(...)
+ // output, so both forms land on the same value.
+ "TOK_NL": fmt.Sprintf("$(cat %s)", withNL),
+ "TOK_NO": fmt.Sprintf("$(cat %s)", noNL),
+
+ // ${VAR:-default} must not error on unset: this is the
+ // opt-in escape hatch for "empty is fine".
+ "FALLBACK": "${MCP_MISSING:-fallback}",
+
+ // Leading/trailing literal spaces around $(...) must be
+ // preserved — single-value contract, no field splitting.
+ "PADDED": " $(echo v) ",
+
+ // ")" inside a quoted arg to echo is a regression guard
+ // for the old hand-rolled paren matcher.
+ "PAREN": `$(echo ")")`,
+
+ // Embedded space inside a quoted arg must survive
+ // verbatim; no word-splitting side effect.
+ "SPACEY": `$(echo "a b")`,
+
+ // Glob-like literals must not expand.
+ "GLOB": "*.go",
+ },
+ }
+
+ got, err := m.ResolvedEnv(realShellResolver(nil))
+ require.NoError(t, err)
+
+ // ResolvedEnv returns "KEY=value" sorted by key.
+ want := []string{
+ "FALLBACK=fallback",
+ "GLOB=*.go",
+ "PADDED= v ",
+ "PAREN=)",
+ "SPACEY=a b",
+ "TOK_NL=token-with-nl",
+ "TOK_NO=token-no-nl",
+ }
+ require.Equal(t, want, got)
+}
+
+// TestResolvedEnv_RealShell_DoesNotMutate pins that both success and
+// failure paths leave m.Env untouched. Prior behaviour rewrote the
+// value in place on error; that was the exact mechanism that shipped
+// empty credentials to MCP servers.
+func TestResolvedEnv_RealShell_DoesNotMutate(t *testing.T) {
+ t.Parallel()
+
+ t.Run("success path leaves Env untouched", func(t *testing.T) {
+ t.Parallel()
+ m := MCPConfig{Env: map[string]string{"TOKEN": "$(echo shh)"}}
+ orig := maps.Clone(m.Env)
+
+ _, err := m.ResolvedEnv(realShellResolver(nil))
+ require.NoError(t, err)
+ require.Equal(t, orig, m.Env)
+ })
+
+ t.Run("failure path leaves Env untouched", func(t *testing.T) {
+ t.Parallel()
+ m := MCPConfig{Env: map[string]string{"BROKEN": "$(false)"}}
+ orig := maps.Clone(m.Env)
+
+ _, err := m.ResolvedEnv(realShellResolver(nil))
+ require.Error(t, err)
+ require.Equal(t, orig, m.Env, "map must be preserved on error")
+ })
+}
+
+// TestResolvedEnv_RealShell_Idempotent pins the pure-function contract:
+// two calls on the same config return deeply-equal slices.
+func TestResolvedEnv_RealShell_Idempotent(t *testing.T) {
+ t.Parallel()
+
+ m := MCPConfig{
+ Env: map[string]string{
+ "A": "$(echo one)",
+ "B": "$(echo two)",
+ "C": "literal",
+ },
+ }
+ r := realShellResolver(nil)
+
+ first, err := m.ResolvedEnv(r)
+ require.NoError(t, err)
+ second, err := m.ResolvedEnv(r)
+ require.NoError(t, err)
+ require.Equal(t, first, second)
+}
+
+// TestResolvedEnv_RealShell_Deterministic guards against Go's
+// randomized map iteration leaking into the returned slice order.
+func TestResolvedEnv_RealShell_Deterministic(t *testing.T) {
+ t.Parallel()
+
+ m := MCPConfig{Env: map[string]string{
+ "Z": "z",
+ "A": "a",
+ "M": "m",
+ }}
+
+ got, err := m.ResolvedEnv(realShellResolver(nil))
+ require.NoError(t, err)
+ require.True(t, slices.IsSorted(got), "env slice must be sorted; got %v", got)
+}
+
+// TestResolvedEnv_RealShell_NounsetRegression is the single most
+// important assertion in this file: an unset variable is an error, not
+// an empty expansion. Swapping the hand-rolled parser for mvdan's
+// default-expansion behaviour without nounset would re-introduce
+// Defect 1 via a typo'd variable name.
+func TestResolvedEnv_RealShell_NounsetRegression(t *testing.T) {
+ t.Parallel()
+
+ m := MCPConfig{Env: map[string]string{
+ // Intentional typo: user meant $FORGEJO_TOKEN.
+ "FORGEJO_ACCESS_TOKEN": "$FORGJO_TOKEN",
+ }}
+ got, err := m.ResolvedEnv(realShellResolver(nil))
+ require.Error(t, err, "unset var must not silently expand to empty")
+ require.Nil(t, got)
+ require.Contains(t, err.Error(), "FORGEJO_ACCESS_TOKEN")
+ require.Contains(t, err.Error(), "$FORGJO_TOKEN")
+}
+
+// TestResolvedEnv_RealShell_FailureDetail pins that a failing inner
+// command surfaces enough detail (exit code + stderr) to diagnose
+// without forcing the user to re-run the command by hand. Also
+// verifies the template is included so they know which Env entry
+// blew up.
+func TestResolvedEnv_RealShell_FailureDetail(t *testing.T) {
+ t.Parallel()
+
+ missing := filepath.Join(t.TempDir(), "definitely-not-here")
+ m := MCPConfig{Env: map[string]string{
+ "FORGEJO_ACCESS_TOKEN": fmt.Sprintf("$(cat %s)", missing),
+ }}
+
+ _, err := m.ResolvedEnv(realShellResolver(nil))
+ require.Error(t, err)
+ msg := err.Error()
+ require.Contains(t, msg, "FORGEJO_ACCESS_TOKEN", "must identify the failing env var")
+ require.Contains(t, msg, missing, "must include the template so users see what failed")
+ require.Contains(t, msg, "exit status", "must surface the inner exit code")
+}
+
+// TestResolvedHeaders_RealShell_FailurePreservesOriginal pins two
+// invariants simultaneously: on failure the returned map is nil (not
+// a partially-populated map) and the receiver's Headers map is
+// unchanged. A test that only asserted on the returned value could
+// hide an in-place mutation regression.
+func TestResolvedHeaders_RealShell_FailurePreservesOriginal(t *testing.T) {
+ t.Parallel()
+
+ m := MCPConfig{Headers: map[string]string{
+ "Authorization": "Bearer $(false)",
+ "X-Static": "kept",
+ }}
+ orig := maps.Clone(m.Headers)
+
+ got, err := m.ResolvedHeaders(realShellResolver(nil))
+ require.Error(t, err)
+ require.Nil(t, got, "headers map must be nil on failure")
+ require.Contains(t, err.Error(), "header Authorization")
+ require.Equal(t, orig, m.Headers, "receiver Headers must be preserved")
+}
+
+// TestResolvedArgs_RealShell exercises both success and failure for
+// m.Args symmetrically with Env. Args are ordered so error messages
+// must identify a positional index, not a key.
+func TestResolvedArgs_RealShell(t *testing.T) {
+ t.Parallel()
+
+ t.Run("success expands each element", func(t *testing.T) {
+ t.Parallel()
+ m := MCPConfig{Args: []string{"--token", "$(echo shh)", "--host", "example.com"}}
+ got, err := m.ResolvedArgs(realShellResolver(nil))
+ require.NoError(t, err)
+ require.Equal(t, []string{"--token", "shh", "--host", "example.com"}, got)
+ })
+
+ t.Run("failure identifies offending index", func(t *testing.T) {
+ t.Parallel()
+ m := MCPConfig{Args: []string{"--token", "$(false)"}}
+ orig := slices.Clone(m.Args)
+
+ got, err := m.ResolvedArgs(realShellResolver(nil))
+ require.Error(t, err)
+ require.Nil(t, got)
+ require.Contains(t, err.Error(), "arg 1")
+ require.Equal(t, orig, m.Args, "receiver Args must be preserved")
+ })
+
+ t.Run("nil args returns nil, no error", func(t *testing.T) {
+ t.Parallel()
+ m := MCPConfig{}
+ got, err := m.ResolvedArgs(realShellResolver(nil))
+ require.NoError(t, err)
+ require.Nil(t, got)
+ })
+}
+
+// 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
+// server does its own — this has to stay a pure pass-through.
+func TestMCPConfig_IdentityResolver(t *testing.T) {
+ t.Parallel()
+
+ m := MCPConfig{
+ Command: "$CMD",
+ Args: []string{"--token", "$MCP_MISSING_TOKEN", "$(vault read -f secret)"},
+ Env: map[string]string{
+ "TOKEN": "$(cat /run/secrets/x)",
+ "HOST": "$MCP_MISSING_HOST",
+ },
+ Headers: map[string]string{
+ "Authorization": "Bearer $(vault read -f token)",
+ },
+ URL: "https://$MCP_HOST/$(vault read -f path)",
+ }
+ r := IdentityResolver()
+
+ args, err := m.ResolvedArgs(r)
+ require.NoError(t, err)
+ require.Equal(t, m.Args, args)
+
+ envs, err := m.ResolvedEnv(r)
+ require.NoError(t, err)
+ // Sorted "KEY=value".
+ require.Equal(t, []string{
+ "HOST=$MCP_MISSING_HOST",
+ "TOKEN=$(cat /run/secrets/x)",
+ }, envs)
+
+ headers, err := m.ResolvedHeaders(r)
+ require.NoError(t, err)
+ require.Equal(t, m.Headers, headers)
+
+ u, err := m.ResolvedURL(r)
+ require.NoError(t, err)
+ require.Equal(t, m.URL, u)
+}