diff --git a/internal/agent/tools/mcp/init_test.go b/internal/agent/tools/mcp/init_test.go index 49ceb0410931fb6d20531f440960fb226ab2d768..5e906054f649aa90083fcbb120cff556d51dd06c 100644 --- a/internal/agent/tools/mcp/init_test.go +++ b/internal/agent/tools/mcp/init_test.go @@ -91,17 +91,40 @@ func TestCreateTransport_URLResolution(t *testing.T) { require.Equal(t, "https://mcp.example.com/events", sse.Endpoint) }) - t.Run("http unset var surfaces error, no transport created", func(t *testing.T) { + t.Run("http failing $(cmd) surfaces error, no transport created", func(t *testing.T) { t.Parallel() + // Under lenient nounset, unset $VAR expands to "" silently, + // so the only way a URL resolution *errors* is a failing + // $(cmd). Mirror the SSE subtest so both transports share + // coverage for the url-resolve-failure path. m := config.MCPConfig{ Type: config.MCPHttp, - URL: "https://$MCP_MISSING_HOST/api", + URL: "https://$(false)/api", } - tr, err := createTransport(t.Context(), m, shell) + tr, err := createTransport(t.Context(), m, shellResolverWithPath(t, nil)) require.Error(t, err) require.Nil(t, tr) require.Contains(t, err.Error(), "url:") - require.Contains(t, err.Error(), "$MCP_MISSING_HOST") + require.Contains(t, err.Error(), "$(false)") + }) + + t.Run("http unset var expands empty", func(t *testing.T) { + t.Parallel() + // Pinning test for the new lenient-nounset default: an + // unset bare $VAR in the URL is *not* an error. It + // expands to "" and, here, leaves a syntactically weird + // but non-empty URL that the existing non-empty guard + // still lets through. Guards against a future regression + // that flips strict-by-default back on. + m := config.MCPConfig{ + Type: config.MCPHttp, + URL: "https://$MCP_MISSING_HOST/api", + } + tr, err := createTransport(t.Context(), m, shell) + require.NoError(t, err) + sct, ok := tr.(*mcp.StreamableClientTransport) + require.True(t, ok) + require.Equal(t, "https:///api", sct.Endpoint) }) t.Run("sse failing $(cmd) surfaces error, no transport created", func(t *testing.T) { @@ -201,16 +224,18 @@ func TestCreateTransport_StdioResolution(t *testing.T) { require.Contains(t, err.Error(), "env TOKEN") }) - t.Run("unset env var is a hard error, not silent empty", func(t *testing.T) { + t.Run("failing env command is a hard error", 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. + // Under lenient nounset a bare $UNSET expands to "" + // silently — see the pinning subtest below. The remaining + // failure mode for env resolution is a $(cmd) that exits + // non-zero, which must still error out and prevent exec so + // we never hand a broken credential to the child process. r := shellResolverWithPath(t, nil) m := config.MCPConfig{ Type: config.MCPStdio, Command: "forgejo-mcp", - Env: map[string]string{"FORGEJO_ACCESS_TOKEN": "$FORGJO_TOKEN"}, + Env: map[string]string{"FORGEJO_ACCESS_TOKEN": "$(exit 5)"}, } tr, err := createTransport(t.Context(), m, r) require.Error(t, err) @@ -218,6 +243,28 @@ func TestCreateTransport_StdioResolution(t *testing.T) { require.Contains(t, err.Error(), "env FORGEJO_ACCESS_TOKEN") }) + t.Run("unset env var expands empty", func(t *testing.T) { + t.Parallel() + // Pinning test for the lenient-nounset default: a bare + // $UNSET in an env value expands to "" without error, and + // the empty entry is kept on the resulting exec.Cmd (env + // entries, unlike headers, are not dropped — see design + // decision #18). Guards against a regression that flips + // strict-by-default back on and silently breaks users + // with configs like FORGEJO_ACCESS_TOKEN=$FORGEJO_TOKEN. + r := shellResolverWithPath(t, nil) + m := config.MCPConfig{ + Type: config.MCPStdio, + Command: "forgejo-mcp", + Env: map[string]string{"FORGEJO_ACCESS_TOKEN": "$FORGEJO_TOKEN_UNSET"}, + } + tr, err := createTransport(t.Context(), m, r) + require.NoError(t, err) + ct, ok := tr.(*mcp.CommandTransport) + require.True(t, ok) + require.Contains(t, ct.Command.Env, "FORGEJO_ACCESS_TOKEN=") + }) + t.Run("args resolution failure surfaces error, no transport created", func(t *testing.T) { t.Parallel() r := shellResolverWithPath(t, nil) @@ -311,17 +358,43 @@ func TestCreateTransport_HeadersResolution(t *testing.T) { t.Run("sse failing header surfaces error, no transport", func(t *testing.T) { t.Parallel() + // Under lenient nounset a bare $MISSING expands to "", + // which ResolvedHeaders drops — no error. The failing + // $(cmd) path is the remaining way this can fail loudly; + // cover it on the SSE branch to mirror the HTTP subtest. r := shellResolverWithPath(t, nil) m := config.MCPConfig{ Type: config.MCPSSE, URL: "https://mcp.example.com/events", - Headers: map[string]string{"Authorization": "Bearer $MISSING_TOKEN"}, + 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") - require.Contains(t, err.Error(), "$MISSING_TOKEN") + }) + + t.Run("sse unset var header drops silently", func(t *testing.T) { + t.Parallel() + // Pinning test for design decision #18 + lenient nounset: + // a header whose value resolves to "" (here because the + // bare $VAR is unset) is omitted from the round tripper + // rather than sent as "X-Header:". Guards against a + // regression that either re-introduces strict-by-default + // or stops dropping empty headers. + r := shellResolverWithPath(t, nil) + m := config.MCPConfig{ + Type: config.MCPSSE, + URL: "https://mcp.example.com/events", + Headers: map[string]string{"Authorization": "$MISSING_TOKEN"}, + } + tr, err := createTransport(t.Context(), m, r) + require.NoError(t, err) + sse, ok := tr.(*mcp.SSEClientTransport) + require.True(t, ok) + rt, ok := sse.HTTPClient.Transport.(*headerRoundTripper) + require.True(t, ok) + require.NotContains(t, rt.headers, "Authorization") }) } @@ -356,24 +429,44 @@ func TestCreateSession_ResolutionFailureUpdatesState(t *testing.T) { wantErrContains: "env FORGEJO_ACCESS_TOKEN", }, { + // Args that reference an unset bare $VAR no longer + // error out under lenient nounset; the only remaining + // failure mode for arg resolution is a failing $(cmd). name: "stdio args failure", mcpName: "test-stdio-args-fail", cfg: config.MCPConfig{ Type: config.MCPStdio, Command: "echo", - Args: []string{"--token", "$MCP_UNSET_TOKEN"}, + Args: []string{"--token", "$(false)"}, }, wantErrContains: "arg 1", }, { + // Likewise for URL: bare $UNSET expands to "" + // silently, so we need a failing $(cmd) to exercise + // the "url:" wrap from ResolvedURL. name: "http url failure", mcpName: "test-http-url-fail", cfg: config.MCPConfig{ Type: config.MCPHttp, - URL: "https://$MCP_MISSING_HOST/api", + URL: "https://$(false)/api", }, wantErrContains: "url:", }, + { + // A URL whose shell expansion yields the empty + // string (here via ${VAR:-}) is not a ResolvedURL + // error, but the non-empty guard in createTransport + // must still reject it so the state card renders an + // error instead of spawning a transport against "". + name: "http empty-resolved url", + mcpName: "test-http-url-empty", + cfg: config.MCPConfig{ + Type: config.MCPHttp, + URL: "${MCP_URL_EMPTY:-}", + }, + wantErrContains: "non-empty 'url'", + }, { name: "http header failure", mcpName: "test-http-header-fail", @@ -394,12 +487,17 @@ func TestCreateSession_ResolutionFailureUpdatesState(t *testing.T) { wantErrContains: "url:", }, { + // Bare $MISSING in a header resolves to "" silently + // and is then dropped (design decision #18). The + // "header Authorization" wrap only surfaces on a + // $(cmd) failure; that is what this subtest now + // pins for the SSE path. 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"}, + Headers: map[string]string{"Authorization": "$(false)"}, }, wantErrContains: "header Authorization", },