MCP-Client

[msafarik]

MCP client implementation with Claude agent (simple terminal version that is temporary until GUI will be made)

[Copilot]

Pull Request Overview

This PR introduces an MCP (Model Context Protocol) client that integrates with the Anthropic Claude API to provide an AI agent interface for managing Keylime infrastructure. The client acts as a terminal-based interface that connects to the existing MCP server, retrieves available tools, and enables Claude to autonomously execute multi-step operations through natural language queries. Additionally, defensive nil-checking is added to the backend to prevent potential issues when API responses contain nil values.

Key changes:

• New MCP client implementation with Claude 3 Haiku integration for autonomous agent operations
• Agentic loop supporting up to 5 turns of tool execution and reasoning
• Defensive nil checks in backend for handling empty collections from Keylime API responses

Reviewed Changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
mcp-client/main.go	New MCP client with Claude agent loop, tool conversion, and MCP server connection handling
mcp-client/go.mod	Go module definition with dependencies for Anthropic SDK, MCP SDK, and godotenv
mcp-client/go.sum	Dependency checksums for the MCP client module
backend/tools.go	Added nil-safe initialization for FailedAgents slice to prevent JSON serialization issues
backend/helpers.go	Added nil checks for UUID and TPM algorithm slices to return empty slices instead of nil

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

[nitpick] The maxTokens value of 1024 might be insufficient for complex tool execution responses, especially when dealing with multiple agents or detailed policy information. Consider increasing this value or making it configurable based on the expected response complexity.

[Copilot]

The error message "Tool execution failed" is too generic and doesn't provide information about what went wrong. Consider including the tool name and any error details from result.Content to help with debugging.

Suggested change

	log.Printf("[Error] Tool execution failed")
	return anthropic.NewToolResultBlock(
	toolUse.ID,
	"Tool execution failed",
	errorDetails := extractTextContent(result.Content)
	log.Printf("[Error] Tool execution failed for tool '%s': %s", toolUse.Name, errorDetails)
	return anthropic.NewToolResultBlock(
	toolUse.ID,
	fmt.Sprintf("Tool '%s' execution failed: %s", toolUse.Name, errorDetails),

[Copilot]

[nitpick] The type assertion for required doesn't handle the case where the assertion fails. If inputSchemaMap["required"] is not a []string, required will be nil, which could be the intended behavior. However, for consistency with the properties handling, consider explicitly checking the assertion result or add a comment explaining that nil is acceptable.

Suggested change

	required, _ := inputSchemaMap["required"].([]string)
	// Explicitly check type assertion for required; nil is acceptable if not present or not a []string
	var required []string
	if r, ok := inputSchemaMap["required"].([]string); ok {
	required = r
	}

[Copilot]

[nitpick] The agent loop can reach maxAgentTurns (5) and exit without logging or indicating that the maximum number of turns was reached. This could leave the user unaware of why the conversation ended. Consider logging a message or returning a specific status when the loop exits due to reaching the turn limit.

[Copilot]

[nitpick] Missing documentation comment for the extractTextContent function. Go convention recommends documenting all exported functions, and for consistency, internal helper functions should also have brief documentation explaining their purpose.

Suggested change

	// extractTextContent returns the concatenated text from all mcp.TextContent elements in the given slice.

[Copilot]

[nitpick] The function returns an empty string when content is empty or contains no text content. Consider handling this case explicitly by logging a warning or checking before calling this function, as an empty result might indicate an unexpected response from the MCP server.

[Copilot]

[nitpick] If the MCP server process (started via exec.Command(serverPath)) fails or exits unexpectedly during operation, there's no explicit handling to detect or recover from this state. Consider adding health checks or monitoring for the server process, especially given the multi-turn conversation loop.

[Copilot]

The function only breaks the loop when hasToolUse is false, meaning when Claude doesn't request any tool calls. However, if Claude continues requesting tools for all maxAgentTurns, the loop exits silently without providing any final response to the user. Consider adding a final message request without tools after the loop completes to get Claude's summary of the work done.

Suggested change

	// After the loop, request a final assistant message without tool use
	finalMessage, err := client.Messages.New(ctx, anthropic.MessageNewParams{
	Model: claudeModel,
	MaxTokens: maxTokens,
	System: []anthropic.TextBlockParam{{Type: "text", Text: systemPrompt}},
	Messages: messages,
	Tools: tools,
	})
	if err != nil {
	return fmt.Errorf("claude API error (final message): %w", err)
	}
	// Print the final assistant message (summary)
	for _, block := range finalMessage.Content {
	if textBlock, ok := block.AsAny().(anthropic.TextBlock); ok {
	fmt.Println(textBlock.Text)
	}
	}

[Copilot]

[nitpick] The API key is retrieved from environment variables without any validation of its format or length. Consider adding validation to ensure the API key meets minimum requirements (e.g., non-empty after trimming whitespace) before attempting to use it.

Suggested change

	apiKey := os.Getenv("ANTHROPIC_API_KEY")
	if apiKey == "" {
	log.Fatal("ANTHROPIC_API_KEY environment variable not set")
	apiKey := strings.TrimSpace(os.Getenv("ANTHROPIC_API_KEY"))
	if apiKey == "" {
	log.Fatal("ANTHROPIC_API_KEY environment variable not set or is empty after trimming whitespace")

[Copilot]

[nitpick] The model anthropic.ModelClaude_3_Haiku_20240307 is from March 2024. Consider verifying if a newer model version is available in the anthropic-sdk-go v1.18.0 that might offer improved performance or capabilities for this use case.

Suggested change

	claudeModel = anthropic.ModelClaude_3_Haiku_20240307
	claudeModel = anthropic.ModelClaude_3_Haiku_20240610

[Copilot]

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated 9 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

[nitpick] Unused loop variable. The blank identifier _ is used for the loop variable, but using a named variable (e.g., turn) would improve code readability and allow for better debugging/logging of which iteration is currently executing.

Consider:

for turn := range maxAgentTurns {
    // optionally log: log.Printf("Agent turn %d/%d", turn+1, maxAgentTurns)
    ...
}

Suggested change

	for _ = range maxAgentTurns {
	for turn := 0; turn < maxAgentTurns; turn++ {

[Copilot]

[nitpick] The goroutine monitoring the server process has a potential race condition. The goroutine calls cmd.Process.Wait() after the connection is established, but session.Close() in the main function's defer might close the connection before the monitoring goroutine completes. This could lead to misleading log messages.

Consider using a context or WaitGroup to ensure proper cleanup coordination.

[Copilot]

Missing input validation for user query. The code accepts any command-line arguments as user input without validation. Empty strings or extremely long inputs could cause issues with the Claude API or lead to unexpected behavior.

Consider adding validation:

userQuery := strings.Join(os.Args[1:], " ")
userQuery = strings.TrimSpace(userQuery)
if userQuery == "" {
    log.Fatal("Error: user query cannot be empty")
}

Suggested change

	userQuery := strings.Join(os.Args[1:], " ")
	userQuery := strings.Join(os.Args[1:], " ")
	userQuery = strings.TrimSpace(userQuery)
	if userQuery == "" {
	log.Fatal("Error: user query cannot be empty")
	}

[Copilot]

[nitpick] Inconsistent indentation in multi-line string. The backtick string uses inconsistent indentation with \n characters and leading tabs. This will result in the string containing literal tabs and newlines, which may not render as intended.

Consider using consistent formatting:

summaryPrompt := `I've reached the maximum number of allowed turns. Please provide a summary of:
1. What you accomplished so far
2. What still needs to be done
3. Any issues or blockers encountered`

Suggested change

	summaryPrompt := `I've reached the maximum number of allowed turns. Please provide a summary of:\n
	1. What you accomplished so far
	2. What still needs to be done
	3. Any issues or blockers encountered`
	summaryPrompt := `I've reached the maximum number of allowed turns. Please provide a summary of:
	1. What you accomplished so far
	2. What still needs to be done
	3. Any issues or blockers encountered`

[Copilot]

The properties field extracted from inputSchemaMap is assigned directly without type checking. If the MCP tool's InputSchema doesn't contain a "properties" field or if it's not in the expected format, this could cause issues with the Claude API. Consider adding validation:

properties := inputSchemaMap["properties"]
if properties == nil {
    properties = map[string]any{}
}

Suggested change

	properties := inputSchemaMap["properties"]
	var properties map[string]any
	if p, ok := inputSchemaMap["properties"].(map[string]any); ok && p != nil {
	properties = p
	} else {
	properties = map[string]any{}
	}

[Copilot]

The type assertion for required field is incorrect. The required field in JSON Schema is typically a []interface{} when unmarshaled from JSON, not []string. This will always fail the type assertion, resulting in required being an empty slice.

This should be:

var required []string
if r, ok := inputSchemaMap["required"].([]interface{}); ok {
    for _, v := range r {
        if s, ok := v.(string); ok {
            required = append(required, s)
        }
    }
}

Suggested change

	if r, ok := inputSchemaMap["required"].([]string); ok {
	required = r
	if r, ok := inputSchemaMap["required"].([]interface{}); ok {
	for _, v := range r {
	if s, ok := v.(string); ok {
	required = append(required, s)
	}
	}

[Copilot]

[nitpick] The hardcoded server path ../backend/server assumes a specific directory structure. This makes the client brittle and difficult to use from different locations. Consider making the server path configurable via an environment variable or command-line flag:

serverPath := os.Getenv("MCP_SERVER_PATH")
if serverPath == "" {
    serverPath = "../backend/server"
}

[Copilot]

[nitpick] Inconsistent error message format. Some error messages use capitalized first words (e.g., "Failed to connect", "Warning: .env file not loaded") while others don't (e.g., "failed to list tools").

For consistency, consider using lowercase for error wrapping:

return nil, fmt.Errorf("failed to connect: %w", err)

And reserve capitalized messages for top-level logs:

log.Printf("Warning: .env file not loaded: %v", err)

[Copilot]

[nitpick] The system prompt contains instructions that might conflict with Claude's behavior. The phrase "You are an autonomous agent" suggests the agent should act independently, but the agentic loop is controlled by the client code with a hard limit of maxAgentTurns. Consider clarifying the agent's autonomy level in the prompt, or explicitly mention the turn limitation to set proper expectations.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The executeToolCall function always returns a ContentBlockParamUnion even when an error occurs, making it impossible for the caller to distinguish between a successful tool call with error content and a failed API call. Consider redesigning to return (anthropic.ContentBlockParamUnion, error) so the agent loop can implement proper retry logic or error recovery for transient failures.

[msafarik]

Yes, because there is no ResultBlock for error and this way I can send this error back to the LLM and LLM will be aware of this problem, in system prompt I defined when error occurs LLM have to investigate that problem.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

[nitpick] The loop variable _ is assigned but never used. While this is syntactically correct, it's clearer to use an explicit counter or the more idiomatic pattern when the index isn't needed:

for range maxAgentTurns {

This removes the unnecessary blank identifier assignment.

Suggested change

	for _ = range maxAgentTurns {
	for i := 0; i < maxAgentTurns; i++ {

[sarroutbi]

The PR has 2 critical issues that should be addressed before merging:

1. Missing Graceful Shutdown Handling

Location: mcp-client/main.go

Problem: When users press Ctrl+C or the process receives SIGTERM, the MCP server subprocess (spawned at line 92) may be left running as an orphaned/zombie process.

Fix: Add signal handling to ensure clean shutdown:

func main() {
    ctx, cancel := context.WithCancel(context.Background())
    defer cancel()

    // Handle shutdown signals
    sigChan := make(chan os.Signal, 1)
    signal.Notify(sigChan, os.Interrupt, syscall.SIGTERM)
    go func() {
        <-sigChan
        log.Println("\nShutting down gracefully...")
        cancel()
    }()

    // ... rest of main
}

2. Process Cleanup in Monitoring Goroutine

Location: mcp-client/main.go:100-114

Problem: The goroutine that monitors the MCP server process may not properly clean up the process. If context is cancelled, cmd.Process.Wait() is never called, potentially leaking zombie processes.

Fix: Ensure process cleanup happens regardless of how the program exits:

  // In connectToMCPServer, add cleanup function
  if cmd.Process != nil {
      go func() {
          cmd.Process.Wait() // Always wait for process
      }()
  }

  // In main(), ensure process is killed on exit
  defer func() {
      if cmd.Process != nil {
          cmd.Process.Kill()
      }
  }()

[Copilot]

Pull request overview

Copilot reviewed 4 out of 7 changed files in this pull request and generated 7 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The runAgentLoop should check for context cancellation to support graceful shutdown. The loop continues even if the context is cancelled. Consider adding a context check:

for _ = range maxAgentTurns {
    select {
    case <-ctx.Done():
        return ctx.Err()
    default:
    }
    
    message, err := client.Messages.New(ctx, anthropic.MessageNewParams{
        // ...
    })
    // ...
}

[Copilot]

The error message uses inconsistent capitalization. Error messages should start with lowercase unless they begin with a proper noun. Change:

log.Fatal("Error: user query cannot be empty")

to:

log.Fatal("error: user query cannot be empty")

Suggested change

	log.Fatal("Error: user query cannot be empty")
	log.Fatal("error: user query cannot be empty")

[Copilot]

[nitpick] Using _ as the loop variable is unconventional here since range is being used with an integer. Consider using a named variable for clarity:

for turn := range maxAgentTurns {

or use a traditional for loop:

for i := 0; i < maxAgentTurns; i++ {

Suggested change

	for _ = range maxAgentTurns {
	for i := 0; i < maxAgentTurns; i++ {

[Copilot]

[nitpick] The convertMCPToolToClaudeTool function lacks documentation explaining the conversion logic and why certain fallbacks (empty maps/slices) are necessary. Consider adding a more detailed comment explaining the schema transformation:

// convertMCPToolToClaudeTool converts a single MCP tool to Claude format.
// It extracts the input schema properties and required fields from the MCP tool definition,
// handling cases where the schema may be nil or missing fields by providing empty defaults.
// This ensures the Claude API receives a valid tool definition even if the MCP tool schema is incomplete.

[Copilot]

The tools.Tools slice could potentially be nil if the MCP server returns an unexpected response. Consider adding a nil check:

tools, err := session.ListTools(ctx, &mcp.ListToolsParams{})
if err != nil {
    return nil, fmt.Errorf("failed to list tools: %w", err)
}
if tools == nil || tools.Tools == nil {
    return nil, fmt.Errorf("no tools returned from MCP server")
}

[Copilot]

[nitpick] The finalMessage function silently returns on error without providing feedback to the user. Since this is called when the maximum turns are reached, the user should be informed if the summary cannot be generated. Consider:

if err != nil {
    log.Printf("Warning: failed to get final summary: %v", err)
    fmt.Println("\nNote: Maximum turns reached but unable to generate summary.")
    return
}

Suggested change

	log.Printf("failed to get final summary: %v", err)
	log.Printf("failed to get final summary: %v", err)
	fmt.Println("\nNote: Maximum turns reached but unable to generate summary.")

[Copilot]

The cmd.Process.Kill() call should check and handle potential errors. The Kill() method returns an error that should be logged if it fails. Consider:

defer func() {
    if cmd != nil && cmd.Process != nil {
        if err := cmd.Process.Kill(); err != nil {
            log.Printf("Warning: failed to kill MCP server process: %v", err)
        }
    }
}()

Suggested change

	cmd.Process.Kill()
	if err := cmd.Process.Kill(); err != nil {
	log.Printf("Warning: failed to kill MCP server process: %v", err)
	}

[sarroutbi]

LGTM