From 85bacf3728ac38ba39c4d93bb0ef7ccf6831ce9b Mon Sep 17 00:00:00 2001 From: Allison Durham Date: Wed, 14 May 2025 15:01:10 -0700 Subject: [PATCH 1/3] delete and status response --- acp/internal/server/agent_status_test.go | 188 +++++++++++++++++++++++ acp/internal/server/server.go | 120 +++++++++++++++ acp/internal/server/server_test.go | 178 +++++++++++++++++++++ 3 files changed, 486 insertions(+) create mode 100644 acp/internal/server/agent_status_test.go diff --git a/acp/internal/server/agent_status_test.go b/acp/internal/server/agent_status_test.go new file mode 100644 index 0000000..1b26aac --- /dev/null +++ b/acp/internal/server/agent_status_test.go @@ -0,0 +1,188 @@ +package server + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/gin-gonic/gin" + acp "github.com/humanlayer/agentcontrolplane/acp/api/v1alpha1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +func TestAgentEndpointStatus(t *testing.T) { + // Create a scheme with our API types registered + scheme := runtime.NewScheme() + if err := corev1.AddToScheme(scheme); err != nil { + t.Fatalf("Failed to add corev1 to scheme: %v", err) + } + if err := acp.AddToScheme(scheme); err != nil { + t.Fatalf("Failed to add acp to scheme: %v", err) + } + + // Create a fake client with the agent pre-loaded + agent := &acp.Agent{ + ObjectMeta: metav1.ObjectMeta{ + Name: "status-test-agent", + Namespace: "status-test-namespace", + }, + Spec: acp.AgentSpec{ + LLMRef: acp.LocalObjectReference{Name: "status-test-llm"}, + System: "Agent with status", + }, + Status: acp.AgentStatus{ + Ready: true, + Status: acp.AgentStatusReady, + StatusDetail: "Everything is working", + }, + } + + llm := &acp.LLM{ + ObjectMeta: metav1.ObjectMeta{ + Name: "status-test-llm", + Namespace: "status-test-namespace", + }, + Spec: acp.LLMSpec{ + Provider: "test-provider", + Parameters: acp.BaseConfig{ + Model: "test-model", + }, + }, + } + + mcpServer := &acp.MCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "status-test-agent-mcp1", + Namespace: "status-test-namespace", + }, + Spec: acp.MCPServerSpec{ + Transport: "stdio", + Command: "python", + Args: []string{"-m", "script.py"}, + }, + Status: acp.MCPServerStatus{ + Connected: true, + Status: "Ready", + StatusDetail: "Connected to MCP server", + }, + } + + // Create namespace + namespace := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: "status-test-namespace"}, + } + + // Add these objects to our client + k8sClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(namespace, llm, agent, mcpServer). + Build() + + agent.Spec.MCPServers = []acp.LocalObjectReference{ + {Name: "status-test-agent-mcp1"}, + } + ctx := context.Background() + if err := k8sClient.Update(ctx, agent); err != nil { + t.Fatalf("Failed to update agent: %v", err) + } + + // Create an API server with the client + apiServer := NewAPIServer(k8sClient, ":8080") + gin.SetMode(gin.TestMode) + + // Test GET /agents/:name + t.Run("GET /agents/:name", func(t *testing.T) { + recorder := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/v1/agents/status-test-agent?namespace=status-test-namespace", nil) + apiServer.Router().ServeHTTP(recorder, req) + + if recorder.Code != http.StatusOK { + t.Errorf("Expected status code %d, got %d", http.StatusOK, recorder.Code) + t.Logf("Response body: %s", recorder.Body.String()) + return + } + + var response AgentResponse + if err := json.Unmarshal(recorder.Body.Bytes(), &response); err != nil { + t.Fatalf("Failed to unmarshal response: %v", err) + } + + // Verify status fields are included and correct + if response.Status != string(acp.AgentStatusReady) { + t.Errorf("Expected status %s, got %s", string(acp.AgentStatusReady), response.Status) + } + if response.StatusDetail != "Everything is working" { + t.Errorf("Expected status detail %q, got %q", "Everything is working", response.StatusDetail) + } + if !response.Ready { + t.Error("Expected ready to be true") + } + + mcpStatus, ok := response.MCPStatus["mcp1"] + if !ok { + t.Error("Expected MCPStatus to have key 'mcp1'") + } else if mcpStatus != "Ready" { + t.Errorf("Expected MCP status %q, got %q", "Ready", mcpStatus) + } + }) + + // Test GET /agents (list) + t.Run("GET /agents (list)", func(t *testing.T) { + recorder := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/v1/agents?namespace=status-test-namespace", nil) + apiServer.Router().ServeHTTP(recorder, req) + + if recorder.Code != http.StatusOK { + t.Errorf("Expected status code %d, got %d", http.StatusOK, recorder.Code) + t.Logf("Response body: %s", recorder.Body.String()) + return + } + + var response []AgentResponse + if err := json.Unmarshal(recorder.Body.Bytes(), &response); err != nil { + t.Fatalf("Failed to unmarshal response: %v", err) + } + + if len(response) == 0 { + t.Fatal("Expected at least one agent in response") + } + + // Find our test agent in the response + var testAgentResponse AgentResponse + found := false + for _, agentResp := range response { + if agentResp.Name == "status-test-agent" { + testAgentResponse = agentResp + found = true + break + } + } + + if !found { + t.Fatal("Test agent not found in response") + } + + // Verify status fields are included and correct + if testAgentResponse.Status != string(acp.AgentStatusReady) { + t.Errorf("Expected status %s, got %s", string(acp.AgentStatusReady), testAgentResponse.Status) + } + if testAgentResponse.StatusDetail != "Everything is working" { + t.Errorf("Expected status detail %q, got %q", "Everything is working", testAgentResponse.StatusDetail) + } + if !testAgentResponse.Ready { + t.Error("Expected ready to be true") + } + + mcpStatus, ok := testAgentResponse.MCPStatus["mcp1"] + if !ok { + t.Error("Expected MCPStatus to have key 'mcp1'") + } else if mcpStatus != "Ready" { + t.Errorf("Expected MCP status %q, got %q", "Ready", mcpStatus) + } + }) +} diff --git a/acp/internal/server/server.go b/acp/internal/server/server.go index 833317e..d174554 100644 --- a/acp/internal/server/server.go +++ b/acp/internal/server/server.go @@ -64,6 +64,10 @@ type AgentResponse struct { LLM string `json:"llm"` SystemPrompt string `json:"systemPrompt"` MCPServers map[string]MCPServerConfig `json:"mcpServers,omitempty"` + Status string `json:"status,omitempty"` // e.g., "Ready", "Error", "Pending" + StatusDetail string `json:"statusDetail,omitempty"` // Additional status details + Ready bool `json:"ready,omitempty"` // Indicates if agent is ready + MCPStatus map[string]string `json:"mcpStatus,omitempty"` // Status of each MCP server } // APIServer represents the REST API server @@ -111,6 +115,7 @@ func (s *APIServer) registerRoutes() { agents.GET("/:name", s.getAgent) agents.POST("", s.createAgent) agents.PUT("/:name", s.updateAgent) + agents.DELETE("/:name", s.deleteAgent) } // processMCPServers creates MCP servers and their secrets based on the given configuration @@ -318,12 +323,28 @@ func (s *APIServer) listAgents(c *gin.Context) { return } + // Fetch MCP server statuses + mcpStatus := make(map[string]string) + for _, mcpRef := range agent.Spec.MCPServers { + var mcpServer acp.MCPServer + if err := s.client.Get(ctx, client.ObjectKey{Namespace: namespace, Name: mcpRef.Name}, &mcpServer); err == nil { + // Extract key from MCP server name (assuming it follows the pattern: {agent-name}-{key}) + parts := strings.Split(mcpRef.Name, "-") + key := parts[len(parts)-1] + mcpStatus[key] = mcpServer.Status.Status + } + } + response = append(response, AgentResponse{ Namespace: namespace, Name: agent.Name, LLM: agent.Spec.LLMRef.Name, SystemPrompt: agent.Spec.System, MCPServers: mcpServers, + Status: string(agent.Status.Status), + StatusDetail: agent.Status.StatusDetail, + Ready: agent.Status.Ready, + MCPStatus: mcpStatus, }) } @@ -369,6 +390,18 @@ func (s *APIServer) getAgent(c *gin.Context) { return } + // Fetch MCP server statuses + mcpStatus := make(map[string]string) + for _, mcpRef := range agent.Spec.MCPServers { + var mcpServer acp.MCPServer + if err := s.client.Get(ctx, client.ObjectKey{Namespace: namespace, Name: mcpRef.Name}, &mcpServer); err == nil { + // Extract key from MCP server name (assuming it follows the pattern: {agent-name}-{key}) + parts := strings.Split(mcpRef.Name, "-") + key := parts[len(parts)-1] + mcpStatus[key] = mcpServer.Status.Status + } + } + // Return the response c.JSON(http.StatusOK, AgentResponse{ Namespace: namespace, @@ -376,9 +409,18 @@ func (s *APIServer) getAgent(c *gin.Context) { LLM: agent.Spec.LLMRef.Name, SystemPrompt: agent.Spec.System, MCPServers: mcpServers, + Status: string(agent.Status.Status), + StatusDetail: agent.Status.StatusDetail, + Ready: agent.Status.Ready, + MCPStatus: mcpStatus, }) } +// Router returns the gin router for testing +func (s *APIServer) Router() *gin.Engine { + return s.router +} + // Start begins listening for requests in a goroutine func (s *APIServer) Start(ctx context.Context) error { errChan := make(chan error, 1) @@ -626,6 +668,84 @@ func defaultIfEmpty(val, defaultVal string) string { return val } +// deleteAgent handles the DELETE /agents/:name endpoint to delete an agent and its associated resources +func (s *APIServer) deleteAgent(c *gin.Context) { + ctx := c.Request.Context() + logger := log.FromContext(ctx) + + // Get namespace from query parameter (required) + namespace := c.Query("namespace") + if namespace == "" { + c.JSON(http.StatusBadRequest, gin.H{"error": "namespace query parameter is required"}) + return + } + + // Get agent name from path parameter + name := c.Param("name") + if name == "" { + c.JSON(http.StatusBadRequest, gin.H{"error": "agent name is required"}) + return + } + + // Get the agent + var agent acp.Agent + if err := s.client.Get(ctx, client.ObjectKey{Namespace: namespace, Name: name}, &agent); err != nil { + if apierrors.IsNotFound(err) { + c.JSON(http.StatusNotFound, gin.H{"error": "Agent not found"}) + return + } + logger.Error(err, "Failed to get agent", "name", name, "namespace", namespace) + c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to get agent: " + err.Error()}) + return + } + + // Delete MCP servers and secrets + for _, mcpRef := range agent.Spec.MCPServers { + mcpName := mcpRef.Name + secretName := fmt.Sprintf("%s-secrets", mcpName) + + // Delete MCP server + var mcp acp.MCPServer + if err := s.client.Get(ctx, client.ObjectKey{Namespace: namespace, Name: mcpName}, &mcp); err == nil { + if err := s.client.Delete(ctx, &mcp); err != nil { + logger.Error(err, "Failed to delete MCP server", "name", mcpName) + c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("Failed to delete MCP server %s: %s", mcpName, err.Error())}) + return + } + } else if !apierrors.IsNotFound(err) { + // Only return error if it's not a NotFound error (we don't care if the MCP server doesn't exist) + logger.Error(err, "Failed to get MCP server", "name", mcpName) + c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("Failed to get MCP server %s: %s", mcpName, err.Error())}) + return + } + + // Delete secret + var secret corev1.Secret + if err := s.client.Get(ctx, client.ObjectKey{Namespace: namespace, Name: secretName}, &secret); err == nil { + if err := s.client.Delete(ctx, &secret); err != nil { + logger.Error(err, "Failed to delete secret", "name", secretName) + c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("Failed to delete secret %s: %s", secretName, err.Error())}) + return + } + } else if !apierrors.IsNotFound(err) { + // Only return error if it's not a NotFound error (we don't care if the secret doesn't exist) + logger.Error(err, "Failed to get secret", "name", secretName) + c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("Failed to get secret %s: %s", secretName, err.Error())}) + return + } + } + + // Delete the agent + if err := s.client.Delete(ctx, &agent); err != nil { + logger.Error(err, "Failed to delete agent", "name", name) + c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("Failed to delete agent: %s", err.Error())}) + return + } + + // Return success with no content + c.Status(http.StatusNoContent) +} + // updateAgent handles updating an existing agent and its associated MCP servers func (s *APIServer) updateAgent(c *gin.Context) { ctx := c.Request.Context() diff --git a/acp/internal/server/server_test.go b/acp/internal/server/server_test.go index d4c7c20..eb63a00 100644 --- a/acp/internal/server/server_test.go +++ b/acp/internal/server/server_test.go @@ -1176,6 +1176,184 @@ var _ = Describe("Agent API", func() { Expect(err).NotTo(HaveOccurred()) Expect(errorResponse["error"]).To(Equal("namespace query parameter is required")) }) + + Describe("DELETE /v1/agents/:name", func() { + It("should delete an agent and its associated resources", func() { + // Create namespace + namespace := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: "delete-namespace"}, + } + Expect(k8sClient.Create(ctx, namespace)).To(Succeed()) + + // Create an LLM + createTestLLM("delete-llm", "delete-namespace") + + // Create an agent with MCP servers + agent := &acp.Agent{ + ObjectMeta: metav1.ObjectMeta{ + Name: "delete-agent", + Namespace: "delete-namespace", + }, + Spec: acp.AgentSpec{ + LLMRef: acp.LocalObjectReference{Name: "delete-llm"}, + System: "Agent to be deleted", + }, + } + Expect(k8sClient.Create(ctx, agent)).To(Succeed()) + + // Create MCP servers and secrets for this agent + mcpServer1 := &acp.MCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "delete-agent-mcp1", + Namespace: "delete-namespace", + }, + Spec: acp.MCPServerSpec{ + Transport: "stdio", + Command: "python", + Args: []string{"-m", "script.py"}, + }, + } + Expect(k8sClient.Create(ctx, mcpServer1)).To(Succeed()) + + mcpServer2 := &acp.MCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "delete-agent-mcp2", + Namespace: "delete-namespace", + }, + Spec: acp.MCPServerSpec{ + Transport: "http", + URL: "http://localhost:8000", + }, + } + Expect(k8sClient.Create(ctx, mcpServer2)).To(Succeed()) + + // Create secrets + secret1 := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "delete-agent-mcp1-secrets", + Namespace: "delete-namespace", + }, + Data: map[string][]byte{ + "API_KEY": []byte("test-secret"), + }, + } + Expect(k8sClient.Create(ctx, secret1)).To(Succeed()) + + // Update agent to reference the MCP servers + agent.Spec.MCPServers = []acp.LocalObjectReference{ + {Name: "delete-agent-mcp1"}, + {Name: "delete-agent-mcp2"}, + } + Expect(k8sClient.Update(ctx, agent)).To(Succeed()) + + // Make the delete request + req := httptest.NewRequest(http.MethodDelete, "/v1/agents/delete-agent?namespace=delete-namespace", nil) + recorder = httptest.NewRecorder() + router.ServeHTTP(recorder, req) + + // Verify the response + Expect(recorder.Code).To(Equal(http.StatusNoContent)) + Expect(recorder.Body.String()).To(BeEmpty()) + + // Verify agent was deleted + var deletedAgent acp.Agent + err := k8sClient.Get(ctx, types.NamespacedName{ + Name: "delete-agent", + Namespace: "delete-namespace", + }, &deletedAgent) + Expect(apierrors.IsNotFound(err)).To(BeTrue()) + + // Verify MCP servers were deleted + var deletedMCP1 acp.MCPServer + err = k8sClient.Get(ctx, types.NamespacedName{ + Name: "delete-agent-mcp1", + Namespace: "delete-namespace", + }, &deletedMCP1) + Expect(apierrors.IsNotFound(err)).To(BeTrue()) + + var deletedMCP2 acp.MCPServer + err = k8sClient.Get(ctx, types.NamespacedName{ + Name: "delete-agent-mcp2", + Namespace: "delete-namespace", + }, &deletedMCP2) + Expect(apierrors.IsNotFound(err)).To(BeTrue()) + + // Verify secrets were deleted + var deletedSecret corev1.Secret + err = k8sClient.Get(ctx, types.NamespacedName{ + Name: "delete-agent-mcp1-secrets", + Namespace: "delete-namespace", + }, &deletedSecret) + Expect(apierrors.IsNotFound(err)).To(BeTrue()) + }) + + It("should return 404 if agent doesn't exist", func() { + req := httptest.NewRequest(http.MethodDelete, "/v1/agents/non-existent-agent?namespace=default", nil) + recorder = httptest.NewRecorder() + router.ServeHTTP(recorder, req) + + Expect(recorder.Code).To(Equal(http.StatusNotFound)) + var errorResponse map[string]string + err := json.Unmarshal(recorder.Body.Bytes(), &errorResponse) + Expect(err).NotTo(HaveOccurred()) + Expect(errorResponse["error"]).To(Equal("Agent not found")) + }) + + It("should require a namespace parameter", func() { + req := httptest.NewRequest(http.MethodDelete, "/v1/agents/some-agent", nil) + recorder = httptest.NewRecorder() + router.ServeHTTP(recorder, req) + + Expect(recorder.Code).To(Equal(http.StatusBadRequest)) + var errorResponse map[string]string + err := json.Unmarshal(recorder.Body.Bytes(), &errorResponse) + Expect(err).NotTo(HaveOccurred()) + Expect(errorResponse["error"]).To(Equal("namespace query parameter is required")) + }) + + It("should be idempotent when MCP servers or secrets are already deleted", func() { + // Create namespace + namespace := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: "idempotent-namespace"}, + } + Expect(k8sClient.Create(ctx, namespace)).To(Succeed()) + + // Create an LLM + createTestLLM("idempotent-llm", "idempotent-namespace") + + // Create an agent with MCP server references + agent := &acp.Agent{ + ObjectMeta: metav1.ObjectMeta{ + Name: "idempotent-agent", + Namespace: "idempotent-namespace", + }, + Spec: acp.AgentSpec{ + LLMRef: acp.LocalObjectReference{Name: "idempotent-llm"}, + System: "Agent for idempotent test", + MCPServers: []acp.LocalObjectReference{ + {Name: "idempotent-agent-mcp1"}, // This MCP server doesn't actually exist + }, + }, + } + Expect(k8sClient.Create(ctx, agent)).To(Succeed()) + + // Make the delete request + req := httptest.NewRequest(http.MethodDelete, "/v1/agents/idempotent-agent?namespace=idempotent-namespace", nil) + recorder = httptest.NewRecorder() + router.ServeHTTP(recorder, req) + + // Should succeed even though MCP servers don't exist + Expect(recorder.Code).To(Equal(http.StatusNoContent)) + + // Verify agent was deleted + var deletedAgent acp.Agent + err := k8sClient.Get(ctx, types.NamespacedName{ + Name: "idempotent-agent", + Namespace: "idempotent-namespace", + }, &deletedAgent) + Expect(apierrors.IsNotFound(err)).To(BeTrue()) + }) + }) }) }) }) From 09a9aa799cf6f80fa4b14e1ec32aabe7d369b81e Mon Sep 17 00:00:00 2001 From: Allison Durham Date: Wed, 14 May 2025 15:21:51 -0700 Subject: [PATCH 2/3] update status to be better --- acp/internal/server/server.go | 189 ++++++++++++++++++---------------- 1 file changed, 98 insertions(+), 91 deletions(-) diff --git a/acp/internal/server/server.go b/acp/internal/server/server.go index d174554..afec3da 100644 --- a/acp/internal/server/server.go +++ b/acp/internal/server/server.go @@ -49,12 +49,15 @@ type UpdateAgentRequest struct { // MCPServerConfig defines the configuration for an MCP server type MCPServerConfig struct { - Transport string `json:"transport"` // Required: "stdio" or "http" - Command string `json:"command,omitempty"` // Required for stdio transport - Args []string `json:"args,omitempty"` // Required for stdio transport - URL string `json:"url,omitempty"` // Required for http transport - Env map[string]string `json:"env,omitempty"` // Optional environment variables - Secrets map[string]string `json:"secrets,omitempty"` // Optional secrets + Transport string `json:"transport"` // Required: "stdio" or "http" + Command string `json:"command,omitempty"` // Required for stdio transport + Args []string `json:"args,omitempty"` // Required for stdio transport + URL string `json:"url,omitempty"` // Required for http transport + Env map[string]string `json:"env,omitempty"` // Optional environment variables + Secrets map[string]string `json:"secrets,omitempty"` // Optional secrets + Status string `json:"status,omitempty"` // e.g., "Ready", "Error", "Pending" + StatusDetail string `json:"statusDetail,omitempty"`// Additional status details + Ready bool `json:"ready,omitempty"` // Indicates if MCP server is ready/connected } // AgentResponse defines the structure of the response body for agent endpoints @@ -67,7 +70,6 @@ type AgentResponse struct { Status string `json:"status,omitempty"` // e.g., "Ready", "Error", "Pending" StatusDetail string `json:"statusDetail,omitempty"` // Additional status details Ready bool `json:"ready,omitempty"` // Indicates if agent is ready - MCPStatus map[string]string `json:"mcpStatus,omitempty"` // Status of each MCP server } // APIServer represents the REST API server @@ -323,15 +325,21 @@ func (s *APIServer) listAgents(c *gin.Context) { return } - // Fetch MCP server statuses - mcpStatus := make(map[string]string) + // Update MCP servers with status information for _, mcpRef := range agent.Spec.MCPServers { var mcpServer acp.MCPServer if err := s.client.Get(ctx, client.ObjectKey{Namespace: namespace, Name: mcpRef.Name}, &mcpServer); err == nil { // Extract key from MCP server name (assuming it follows the pattern: {agent-name}-{key}) parts := strings.Split(mcpRef.Name, "-") key := parts[len(parts)-1] - mcpStatus[key] = mcpServer.Status.Status + + // Update the config with status information + if config, ok := mcpServers[key]; ok { + config.Status = mcpServer.Status.Status + config.StatusDetail = mcpServer.Status.StatusDetail + config.Ready = mcpServer.Status.Connected + mcpServers[key] = config + } } } @@ -344,7 +352,6 @@ func (s *APIServer) listAgents(c *gin.Context) { Status: string(agent.Status.Status), StatusDetail: agent.Status.StatusDetail, Ready: agent.Status.Ready, - MCPStatus: mcpStatus, }) } @@ -390,15 +397,21 @@ func (s *APIServer) getAgent(c *gin.Context) { return } - // Fetch MCP server statuses - mcpStatus := make(map[string]string) + // Update MCP servers with status information for _, mcpRef := range agent.Spec.MCPServers { var mcpServer acp.MCPServer if err := s.client.Get(ctx, client.ObjectKey{Namespace: namespace, Name: mcpRef.Name}, &mcpServer); err == nil { // Extract key from MCP server name (assuming it follows the pattern: {agent-name}-{key}) parts := strings.Split(mcpRef.Name, "-") key := parts[len(parts)-1] - mcpStatus[key] = mcpServer.Status.Status + + // Update the config with status information + if config, ok := mcpServers[key]; ok { + config.Status = mcpServer.Status.Status + config.StatusDetail = mcpServer.Status.StatusDetail + config.Ready = mcpServer.Status.Connected + mcpServers[key] = config + } } } @@ -412,7 +425,6 @@ func (s *APIServer) getAgent(c *gin.Context) { Status: string(agent.Status.Status), StatusDetail: agent.Status.StatusDetail, Ready: agent.Status.Ready, - MCPStatus: mcpStatus, }) } @@ -498,20 +510,25 @@ func (s *APIServer) getTask(c *gin.Context) { // Get the task if err := s.client.Get(ctx, client.ObjectKey{Namespace: namespace, Name: id}, &task); err != nil { - logger.Error(err, "Failed to get task", "name", id, "namespace", namespace) - c.JSON(http.StatusNotFound, gin.H{ - "error": "Task not found: " + err.Error(), - }) + if apierrors.IsNotFound(err) { + c.JSON(http.StatusNotFound, gin.H{ + "error": "Task not found", + }) + } else { + logger.Error(err, "Failed to get task") + c.JSON(http.StatusInternalServerError, gin.H{ + "error": "Failed to get task: " + err.Error(), + }) + } return } c.JSON(http.StatusOK, task) } -// Helper functions for Agent API -// resourceExists checks if a resource with the given name exists in the specified namespace func (s *APIServer) resourceExists(ctx context.Context, obj client.Object, namespace, name string) (bool, error) { - err := s.client.Get(ctx, client.ObjectKey{Namespace: namespace, Name: name}, obj) + key := client.ObjectKey{Namespace: namespace, Name: name} + err := s.client.Get(ctx, key, obj) if err != nil { if apierrors.IsNotFound(err) { return false, nil @@ -521,37 +538,36 @@ func (s *APIServer) resourceExists(ctx context.Context, obj client.Object, names return true, nil } -// validateMCPConfig validates an MCP server configuration -// Defaults to "stdio" transport if not specified func validateMCPConfig(config MCPServerConfig) error { - // Default to stdio transport if not specified - transport := config.Transport - if transport == "" { - transport = "stdio" + // If transport is empty, default to stdio + if config.Transport == "" { + config.Transport = "stdio" } - // Validate the transport type - if transport != "stdio" && transport != "http" { - return fmt.Errorf("invalid transport: %s", transport) + // Check transport type is valid + if config.Transport != "stdio" && config.Transport != "http" { + return fmt.Errorf("invalid transport type '%s', must be 'stdio' or 'http'", config.Transport) } - // Validate transport-specific requirements - if transport == "stdio" && (config.Command == "" || len(config.Args) == 0) { + // Validate stdio requirements + if config.Transport == "stdio" && (config.Command == "" || len(config.Args) == 0) { return fmt.Errorf("command and args required for stdio transport") } - if transport == "http" && config.URL == "" { + + // Validate http requirements + if config.Transport == "http" && config.URL == "" { return fmt.Errorf("url required for http transport") } return nil } -// createSecret creates a Kubernetes Secret from a map of secret values -func createSecret(name, namespace string, secrets map[string]string) *corev1.Secret { +func createSecret(name, namespace string, secretData map[string]string) *corev1.Secret { data := make(map[string][]byte) - for k, v := range secrets { + for k, v := range secretData { data[k] = []byte(v) } + return &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: name, @@ -561,54 +577,48 @@ func createSecret(name, namespace string, secrets map[string]string) *corev1.Sec } } -// createMCPServer creates an MCPServer from a configuration -// Defaults to "stdio" transport if not specified func createMCPServer(name, namespace string, config MCPServerConfig, secretName string) *acp.MCPServer { - env := []acp.EnvVar{} - - // Add regular environment variables - for k, v := range config.Env { - env = append(env, acp.EnvVar{ - Name: k, - Value: v, - }) - } - - // Add secret references - for k := range config.Secrets { - env = append(env, acp.EnvVar{ - Name: k, - ValueFrom: &acp.EnvVarSource{ - SecretKeyRef: &acp.SecretKeyRef{ - Name: secretName, - Key: k, - }, - }, - }) - } - - // Default to stdio transport if not specified - transport := config.Transport - if transport == "" { - transport = "stdio" - } - - return &acp.MCPServer{ + mcpServer := &acp.MCPServer{ ObjectMeta: metav1.ObjectMeta{ Name: name, Namespace: namespace, }, Spec: acp.MCPServerSpec{ - Transport: transport, + Transport: config.Transport, Command: config.Command, Args: config.Args, URL: config.URL, - Env: env, }, } + + // Add environment variables + envVars := []acp.EnvVar{} + for k, v := range config.Env { + envVars = append(envVars, acp.EnvVar{ + Name: k, + Value: v, + }) + } + + // Add secrets as environment variables + if len(config.Secrets) > 0 { + for k := range config.Secrets { + envVars = append(envVars, acp.EnvVar{ + Name: k, + ValueFrom: &acp.EnvVarSource{ + SecretKeyRef: &acp.SecretKeyRef{ + Name: secretName, + Key: k, + }, + }, + }) + } + } + + mcpServer.Spec.Env = envVars + return mcpServer } -// fetchMCPServers retrieves MCP servers and their configurations func (s *APIServer) fetchMCPServers(ctx context.Context, namespace string, refs []acp.LocalObjectReference) (map[string]MCPServerConfig, error) { result := make(map[string]MCPServerConfig) @@ -1012,52 +1022,49 @@ func (s *APIServer) createTask(c *gin.Context) { responseURL := req.ResponseURL if responseURL == "" && req.ResponseUrl != "" { responseURL = req.ResponseUrl - logger.Info("Using deprecated 'responseUrl' field, please use 'responseURL' instead", - "responseUrl", req.ResponseUrl) } // Check if agent exists var agent acp.Agent - err := s.client.Get(ctx, client.ObjectKey{Namespace: namespace, Name: req.AgentName}, &agent) - if err != nil { + if err := s.client.Get(ctx, client.ObjectKey{Namespace: namespace, Name: req.AgentName}, &agent); err != nil { if apierrors.IsNotFound(err) { c.JSON(http.StatusNotFound, gin.H{"error": "Agent not found"}) - return + } else { + logger.Error(err, "Failed to check agent existence") + c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to check agent existence: " + err.Error()}) } - logger.Error(err, "Failed to check agent existence") - c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to check agent existence: " + err.Error()}) return } - // Generate (mostly) unique task name - generatedName := req.AgentName + "-task-" + uuid.New().String()[:8] + // Generate task name with agent name prefix for easier tracking + taskName := fmt.Sprintf("%s-task-%s", req.AgentName, uuid.New().String()[:8]) + // Create task task := &acp.Task{ ObjectMeta: metav1.ObjectMeta{ - Name: generatedName, + Name: taskName, Namespace: namespace, Labels: map[string]string{ "acp.humanlayer.dev/agent": req.AgentName, }, }, Spec: acp.TaskSpec{ - AgentRef: acp.LocalObjectReference{Name: req.AgentName}, + AgentRef: acp.LocalObjectReference{ + Name: req.AgentName, + }, UserMessage: req.UserMessage, ContextWindow: req.ContextWindow, ResponseURL: responseURL, }, } + // Create the task in Kubernetes if err := s.client.Create(ctx, task); err != nil { - if statusErr := new(apierrors.StatusError); errors.As(err, &statusErr) { - status := statusErr.Status() - c.JSON(int(status.Code), gin.H{"error": status.Message}) - } else { - logger.Error(err, "Failed to create task") - c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to create task: " + err.Error()}) - } + logger.Error(err, "Failed to create task") + c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to create task: " + err.Error()}) return } + // Return the created task c.JSON(http.StatusCreated, task) -} +} \ No newline at end of file From ad1dfb4f126ad31b93ea9a1629c14f61ac4e5e7e Mon Sep 17 00:00:00 2001 From: Allison Durham Date: Wed, 14 May 2025 15:39:42 -0700 Subject: [PATCH 3/3] formatting --- acp/internal/server/server.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acp/internal/server/server.go b/acp/internal/server/server.go index 90837e8..f742de8 100644 --- a/acp/internal/server/server.go +++ b/acp/internal/server/server.go @@ -594,7 +594,7 @@ func createMCPServer(name, namespace string, config MCPServerConfig, secretName if transport == "" { transport = transportTypeStdio } - + mcpServer := &acp.MCPServer{ ObjectMeta: metav1.ObjectMeta{ Name: name,