From 240e5c2ccb9ed4853f1fc9e220aa02ee20600b3d Mon Sep 17 00:00:00 2001 From: Christian Rocha Date: Sat, 2 May 2026 14:57:00 -0400 Subject: [PATCH] test: cover shell expansion in MCP config resolution --- internal/agent/tools/mcp/init_test.go | 295 ++++++++++++++++++++++++++ internal/config/load.go | 6 + internal/config/load_test.go | 58 +++++ internal/config/resolve_real_test.go | 294 +++++++++++++++++++++++++ 4 files changed, 653 insertions(+) create mode 100644 internal/config/resolve_real_test.go diff --git a/internal/agent/tools/mcp/init_test.go b/internal/agent/tools/mcp/init_test.go index a2d04484c2c7dd6d8a8f1cef7a25ce1ac9079fe1..49ceb0410931fb6d20531f440960fb226ab2d768 100644 --- a/internal/agent/tools/mcp/init_test.go +++ b/internal/agent/tools/mcp/init_test.go @@ -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") + }) + } +} diff --git a/internal/config/load.go b/internal/config/load.go index 967af2de8cd6bffd6a8bd08e77b562998a8d1913..b10d6fa643a54ed0ad64174cea92bebd6284a7d8 100644 --- a/internal/config/load.go +++ b/internal/config/load.go @@ -222,6 +222,12 @@ func (c *Config) configureProviders(store *ConfigStore, env env.Env, resolver Va if len(config.ExtraHeaders) > 0 { maps.Copy(headers, config.ExtraHeaders) } + // Intentional divergence from MCP env/headers/args/url resolution: + // provider headers that fail to resolve log and keep their literal + // template so providers with optional, env-gated headers still + // load on hosts where those vars are unset. Changing this to a + // hard error would break those configs. See PLAN.md "Design + // decisions" item 4 for the full rationale. for k, v := range headers { resolved, err := resolver.ResolveValue(v) if err != nil { diff --git a/internal/config/load_test.go b/internal/config/load_test.go index 68d52da39fae5000433a47dea2401fd46c193ba3..80551a7749014b6f3262d4b0f2b70c26aabde34d 100644 --- a/internal/config/load_test.go +++ b/internal/config/load_test.go @@ -1589,3 +1589,61 @@ func TestConfig_configureProviders_HyperAPIKeyFromConfigOverrides(t *testing.T) require.True(t, ok, "Hyper provider should be configured") require.Equal(t, "env-api-key", pc.APIKey) } + +// TestConfig_configureProviders_ProviderHeaderResolveFailure pins the +// intentional divergence at load.go:225: a provider header whose +// expansion fails must NOT fail the whole config load. It logs an +// error, keeps the literal template in the resolved header map, and +// moves on. The MCP contract (hard error on failed expansion) does not +// apply here because many providers ship DefaultHeaders that reference +// env vars which are legitimately unset on most hosts. +// +// If this test ever flips to requiring an error, read PLAN.md "Design +// decisions" item 4 before changing the production code — the +// divergence is deliberate. +func TestConfig_configureProviders_ProviderHeaderResolveFailure(t *testing.T) { + knownProviders := []catwalk.Provider{ + { + ID: "openai", + APIKey: "$OPENAI_API_KEY", + APIEndpoint: "https://api.openai.com/v1", + Models: []catwalk.Model{{ID: "test-model"}}, + }, + } + + cfg := &Config{ + Providers: csync.NewMapFrom(map[string]ProviderConfig{ + "openai": { + ExtraHeaders: map[string]string{ + // Failing $(...) — inner command exits 1, no stdout. + "X-Broken": "$(false)", + // Unset var — nounset makes this an error too. + "X-Missing": "$PROVIDER_HEADER_NEVER_SET", + // Happy path: must still be resolved, proving the + // failure in the other two did not abort the loop. + "X-Static": "kept-literal", + }, + }, + }), + } + cfg.setDefaults("/tmp", "") + + testEnv := env.NewFromMap(map[string]string{ + "OPENAI_API_KEY": "test-key", + "PATH": os.Getenv("PATH"), + }) + resolver := NewShellVariableResolver(testEnv) + + err := cfg.configureProviders(testStore(cfg), testEnv, resolver, knownProviders) + require.NoError(t, err, "provider load must tolerate failing header expansion") + + pc, ok := cfg.Providers.Get("openai") + require.True(t, ok, "openai provider must still be configured") + + // Literal template preserved for the two failing headers; the + // happy-path header is resolved through the shell (pass-through + // for a literal value). + require.Equal(t, "$(false)", pc.ExtraHeaders["X-Broken"]) + require.Equal(t, "$PROVIDER_HEADER_NEVER_SET", pc.ExtraHeaders["X-Missing"]) + require.Equal(t, "kept-literal", pc.ExtraHeaders["X-Static"]) +} diff --git a/internal/config/resolve_real_test.go b/internal/config/resolve_real_test.go new file mode 100644 index 0000000000000000000000000000000000000000..c840bfc38143eb876c972a756b1cee6c9c8578aa --- /dev/null +++ b/internal/config/resolve_real_test.go @@ -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) +}