From f3e61344f688fcd01f3a0aac64de245be6e079ea Mon Sep 17 00:00:00 2001 From: Alvaro Saurin Date: Mon, 20 Oct 2025 15:30:36 +0200 Subject: [PATCH 1/2] Agent fixes --- cmd/agent.go | 10 +-- cmd/exe.go | 2 +- docs/usage-agent.md | 139 ++++++++++----------------------- pkg/agent/agent.go | 88 ++++++++++++++------- pkg/agent/cagent_runtime.go | 24 ++++-- pkg/agent/model.go | 2 +- pkg/command/command.go | 4 +- pkg/command/command_exec.go | 2 +- pkg/command/runner.go | 2 +- pkg/command/runner_docker.go | 12 +-- pkg/command/runner_exec.go | 38 ++++----- pkg/command/runner_firejail.go | 50 ++++++------ pkg/command/runner_sandbox.go | 54 ++++++------- pkg/common/constraints.go | 20 ++--- pkg/common/logging.go | 11 ++- pkg/common/panic.go | 4 +- pkg/server/server.go | 2 +- 17 files changed, 227 insertions(+), 237 deletions(-) diff --git a/cmd/agent.go b/cmd/agent.go index 7b45e58..c1132dc 100644 --- a/cmd/agent.go +++ b/cmd/agent.go @@ -132,10 +132,7 @@ $ mcpshell agent --tools=examples/config.yaml \ If a model is configured as default in the agent configuration file, you can omit the --model flag: -$ mcpshell agent --tools=examples/config.yaml \ - --user-prompt "I am having trouble with my computer. It is slow and I think it is due to the CPU usage." - -You can also provide the initial user prompt as positional arguments: +You can provide initial user prompt as positional arguments: $ mcpshell agent I am having trouble with my computer. It is slow and I think it is due to the CPU usage. @@ -250,7 +247,10 @@ The agent will try to debug the issue with the given tools. go func() { defer wg.Done() if err := agentInstance.Run(ctx, userInput, agentOutput); err != nil { - logger.Error(color.HiRedString("Agent encountered an error: %v", err)) + // Don't log context cancellation as an error - it's an expected exit condition + if err != context.Canceled && err != context.DeadlineExceeded { + logger.Error(color.HiRedString("Agent encountered an error: %v", err)) + } // Cancel context to abort all goroutines on fatal errors cancel() } diff --git a/cmd/exe.go b/cmd/exe.go index 6b24fb8..aa981aa 100644 --- a/cmd/exe.go +++ b/cmd/exe.go @@ -67,7 +67,7 @@ will be reported. // Get the tool name toolName := args[0] - logger.Info("Executing tool: %s", toolName) + logger.Debug("Executing tool: %s", toolName) // Load the configuration file(s) (local or remote) localConfigPath, cleanup, err := config.ResolveMultipleConfigPaths(toolsFiles, logger) diff --git a/docs/usage-agent.md b/docs/usage-agent.md index 7086e67..0dc26d2 100644 --- a/docs/usage-agent.md +++ b/docs/usage-agent.md @@ -6,7 +6,7 @@ The MCPShell can be run in "agent mode" to establish a direct connection between In agent mode, MCPShell: -1. Connects directly to an LLM API (currently OpenAI-compatible APIs) +1. Connects directly to an LLM API 1. Makes your tools available to the LLM 1. Manages the conversation flow. 1. Handles tool execution requests @@ -51,105 +51,49 @@ MCPShell has agent-specific configuration that includes model definitions with p **See the [Agent Configuration Guide](usage-agent-conf.md)** -## Creating an Agent Script - -You can create a shell script to run MCPShell in agent mode with a specific configuration. This is useful for creating specialized agents for different tasks. - -Here's a practical example showing both agent configuration and tools configuration for a disk space analyzer agent: - -**Tools Configuration** (`disk-analyzer.yaml`): - -```yaml - description: "Tools for analyzing disk space usage and system performance" - run: - shell: "bash" - tools: - - name: "disk_usage" - description: "Check disk usage for a directory" - params: - directory: - type: string - description: "Directory to analyze" - required: true - max_depth: - type: number - description: "Maximum depth to analyze (1-3)" - default: 2 - constraints: - - "directory.startsWith('/')" # Must be absolute path - - "!directory.contains('..')" # Prevent directory traversal - - "max_depth >= 1 && max_depth <= 3" # Limit recursion depth - run: - command: | - du -h --max-depth={{ .max_depth }} {{ .directory }} | sort -hr | head -20 - output: - format: "text" - prefix: "Disk Usage Analysis (Top 20 largest directories):" - - - name: "filesystem_info" - description: "Show filesystem usage information" - run: - command: | - df -h - output: - format: "text" - prefix: "Filesystem Usage Information:" - - - name: "large_files" - description: "Find large files in a directory" - params: - directory: - type: string - description: "Directory to search in" - required: true - min_size: - type: string - description: "Minimum file size (e.g., '100M', '1G')" - default: "100M" - file_type: - type: string - description: "Filter by file extension (e.g., 'log', 'zip')" - required: false - constraints: - - "directory.startsWith('/')" # Must be absolute path - - "!directory.contains('..')" # Prevent directory traversal - run: - command: | - file_type="{{ .file_type }}" - if [ -n "$file_type" ]; then - find {{ .directory }} -type f -name "*.${file_type}" -size +{{ .min_size }} -exec ls -lh {} \; | sort -k5hr | head -20 - else - find {{ .directory }} -type f -size +{{ .min_size }} -exec ls -lh {} \; | sort -k5hr | head -20 - fi - output: - format: "text" - prefix: "Large Files (minimum size {{ .min_size }}):" -``` - ## Running the Agent -With the tools configuration saved as `disk-analyzer.yaml`, you can run: +With the tools degined in `disk-diagnostics-ro.yaml`, you can run: ```bash mcpshell agent \ - --tools disk-analyzer.yaml \ - --user-prompt "My root partition is running low on space. Can you help me find what's taking up space and how I might free some up?" + --tools disk-diagnostics-ro.yaml \ + "My root partition is running low on space. Can you help me find what's taking up space and how I might free some up?" ``` -If you have a default model configured in your agent config file, -you don't need to specify the model or API key: +The agent will: -```bash -mcpshell agent --tools disk-analyzer.yaml +- Load model and API settings from `~/.mcpshell/agent.yaml` (see [Agent Configuration Guide](usage-agent-conf.md)). It will look like this: + +```console +$ cat ~/.mcpshell/agent.yaml +agent: + models: + - model: "gpt-4o" + class: "openai" + name: "gpt-4o" + default: true + api-key: "your-openai-api-key" + api-url: "https://api.openai.com/v1" + + - name: "claude-sonnet-4" + class: "amazon-bedrock" + model: "us.anthropic.claude-sonnet-4-5-20250929-v1:0" + api-url: "https://bedrock-runtime.us-east-2.amazonaws.com" + + - model: "gemma3n" + class: "ollama" + name: "gemma3n" + + - model: "llama3.1:8b" + class: "ollama" + name: "llama3" ``` -The agent will: - -1. Load model and API settings from `~/.mcpshell/agent.yaml` (see [Agent Configuration Guide](usage-agent-conf.md)) -1. Load system prompts from the agent configuration -1. Load tools from `disk-analyzer.yaml` -1. Connect to the configured LLM API -1. Process the LLM's responses and execute tool calls as requested +- Load system prompts from the agent configuration (if any, or use teh default ones). +- Load tools from `disk-diagnostics-ro.yaml`. +- Connect to the configured LLM API. +- Process the LLM's responses and execute tool calls as requested. ## Interacting with the Agent @@ -157,15 +101,16 @@ In interactive mode (without the `--once` flag), the agent will: - Display the LLM's responses - Execute tool calls as requested by the LLM -- Prompt you for additional input -- Continue the conversation until you exit (Ctrl+C) or the LLM responds with "TERMINATE" +- Wait for you to provide additional input after the LLM completes its response +- Continue the conversation with the full conversation context preserved +- Loop until you exit (Ctrl+C) In one-shot mode (with the `--once` flag), the agent will: - Process the initial prompt - Execute any requested tools - Display the final response -- Exit automatically +- Exit automatically after the LLM completes ## Testing and Debugging @@ -178,9 +123,5 @@ When developing agents, you can: 1. Test with the `exe` command to verify individual tools: ```bash - mcpshell exe --tools disk-analyzer.yaml disk_usage directory="/" max_depth=2 - ``` - -## Conclusion - -The agent mode provides a powerful way to create specialized AI assistants that can perform specific tasks on your system using the tools you define. By combining well-defined tools with appropriate system and user prompts, you can create agents that solve real-world problems in a secure and controlled manner. + mcpshell exe --tools disk-diagnostics-ro.yaml disk_usage directory="/" max_depth=2 + ``` \ No newline at end of file diff --git a/pkg/agent/agent.go b/pkg/agent/agent.go index 9581e0f..7880f0a 100644 --- a/pkg/agent/agent.go +++ b/pkg/agent/agent.go @@ -102,6 +102,8 @@ func (a *Agent) Run(ctx context.Context, userInput chan string, agentOutput chan defer singleRunCancel() ctx = singleRunCtx a.logger.Info("Running in one-shot mode with 120s safety timeout") + } else { + a.logger.Info("Running in interactive mode (will wait for user input to continue)") } // Create cagent runtime with multi-agent system @@ -112,37 +114,65 @@ func (a *Agent) Run(ctx context.Context, userInput chan string, agentOutput chan return fmt.Errorf("failed to create cagent runtime: %w", err) } - // Start streaming events from cagent - a.logger.Info("Starting cagent event stream") - events := cagentRT.RunStream(ctx) - - // Process events and send output - eventCount := 0 - for event := range events { - eventCount++ - a.logger.Debug("Received event #%d: %T", eventCount, event) - - // Handle tool call confirmations - auto-approve tools - if _, ok := event.(*runtime.ToolCallConfirmationEvent); ok { - a.logger.Debug("Auto-approving tool execution") - cagentRT.Runtime().Resume(ctx, "approve-session") + // Conversation loop - run until Once mode or context cancellation + for { + // Start streaming events from cagent + a.logger.Debug("Starting cagent event stream") + events := cagentRT.RunStream(ctx) + + // Process events and send output + eventCount := 0 + for event := range events { + eventCount++ + a.logger.Debug("Received event #%d: %T", eventCount, event) + + // Handle tool call confirmations - auto-approve tools + if _, ok := event.(*runtime.ToolCallConfirmationEvent); ok { + a.logger.Debug("Auto-approving tool execution") + cagentRT.Runtime().Resume(ctx, "approve-session") + } + + if err := a.handleCagentEvent(event, agentOutput); err != nil { + a.logger.Error("Error handling event: %v", err) + // Continue processing other events + } } + a.logger.Debug("Event stream completed, processed %d events", eventCount) - if err := a.handleCagentEvent(event, agentOutput); err != nil { - a.logger.Error("Error handling event: %v", err) - // Continue processing other events + // In one-shot mode, exit after first response + if a.config.Once { + a.logger.Info("One-shot mode: exiting after first response") + return nil } - } - a.logger.Info("Event stream completed, processed %d events", eventCount) - - a.logger.Info("Cagent runtime completed") - // In one-shot mode, close the userInput channel - if a.config.Once { - close(userInput) + // In interactive mode, wait for user input to continue + a.logger.Debug("Waiting for user input to continue conversation...") + promptColor := color.New(color.Bold, color.FgHiCyan) + agentOutput <- fmt.Sprintf("\n%s", promptColor.Sprint("💬 Enter your next question (or Ctrl+C to exit): ")) + + select { + case <-ctx.Done(): + a.logger.Info("Context cancelled, exiting") + return ctx.Err() + case nextInput, ok := <-userInput: + if !ok { + a.logger.Info("User input channel closed, exiting") + return nil + } + if nextInput == "" { + continue // Skip empty input + } + + // Add the new user message to the session to continue the conversation + a.logger.Debug("Received user input: %s", nextInput) + if err := cagentRT.ContinueConversation(nextInput); err != nil { + a.logger.Error("Failed to continue conversation: %v", err) + agentOutput <- fmt.Sprintf("Error: %v\n", err) + return fmt.Errorf("failed to continue conversation: %w", err) + } + // Loop will continue with the updated session + } } - - return nil } // handleCagentEvent processes a single cagent event and sends appropriate output @@ -184,6 +214,8 @@ func (a *Agent) handleCagentEvent(event interface{}, agentOutput chan string) er case *runtime.ToolCallConfirmationEvent: // Tool is being confirmed/executed + // Add newline before logs to separate from agent output + agentOutput <- "\n" a.logger.Debug("Tool call confirmed for agent: %s", e.AgentName) case *runtime.ToolCallResponseEvent: @@ -192,9 +224,7 @@ func (a *Agent) handleCagentEvent(event interface{}, agentOutput chan string) er if len(response) > 1000 { response = response[:1000] + "... (truncated)" } - agentOutput <- fmt.Sprintf("%s\n%s\n", - blue.Sprint("✓ Tool result:"), - response) + agentOutput <- fmt.Sprintf("%s\n%s\n", blue.Sprint("✓ Tool result:"), response) case *runtime.StreamStartedEvent: // Agent started processing - use magenta for agent status diff --git a/pkg/agent/cagent_runtime.go b/pkg/agent/cagent_runtime.go index 34c42a3..c30d95e 100644 --- a/pkg/agent/cagent_runtime.go +++ b/pkg/agent/cagent_runtime.go @@ -39,7 +39,7 @@ func CreateCagentRuntime( userPrompt string, logger *common.Logger, ) (*CagentRuntime, error) { - logger.Info("Creating cagent single-agent runtime") + logger.Debug("Creating cagent single-agent runtime") // Use orchestrator config for the single agent agentLLM, err := initializeCagentModel(ctx, orchestratorConfig, logger) @@ -54,7 +54,7 @@ func CreateCagentRuntime( return nil, fmt.Errorf("failed to get MCP tools: %w", err) } - logger.Info("Creating single agent with %d MCP tools", len(tools)) + logger.Debug("Creating single agent with %d MCP tools", len(tools)) // Get system prompts - use tool-runner prompt since this agent will execute tools // Use config prompts if provided, otherwise use embedded default @@ -103,7 +103,7 @@ Remember: This is a multi-step investigation. Keep calling tools iteratively unt sess := session.New(session.WithUserMessage("", enhancedPrompt)) - logger.Info("Cagent single-agent runtime created successfully") + logger.Debug("Cagent single-agent runtime created successfully") return &CagentRuntime{ runtime: rt, @@ -114,7 +114,7 @@ Remember: This is a multi-step investigation. Keep calling tools iteratively unt // RunStream starts the streaming runtime and returns the event channel func (cr *CagentRuntime) RunStream(ctx context.Context) <-chan runtime.Event { - cr.logger.Info("Starting cagent runtime stream") + cr.logger.Debug("Starting cagent runtime stream") return cr.runtime.RunStream(ctx, cr.session) } @@ -123,6 +123,18 @@ func (cr *CagentRuntime) Runtime() runtime.Runtime { return cr.runtime } +// ContinueConversation adds a new user message to the session and continues the conversation +func (cr *CagentRuntime) ContinueConversation(userMessage string) error { + cr.logger.Debug("Adding user message to continue conversation") + + // Add the user message to the existing session + msg := session.UserMessage("", userMessage) + cr.session.AddMessage(msg) + + cr.logger.Debug("User message added to session, ready for next stream") + return nil +} + // initializeCagentModel creates a cagent-compatible model provider from our ModelConfig func initializeCagentModel(ctx context.Context, config ModelConfig, logger *common.Logger) (provider.Provider, error) { // Create cagent model configuration @@ -164,7 +176,7 @@ func initializeCagentModel(ctx context.Context, config ModelConfig, logger *comm logger.Debug("Setting base URL: %s", config.APIURL) } - logger.Info("Initializing cagent model: provider=%s, model=%s", + logger.Debug("Initializing cagent model: provider=%s, model=%s", cagentModelConfig.Provider, cagentModelConfig.Model) // Create environment provider for API keys @@ -182,7 +194,7 @@ func initializeCagentModel(ctx context.Context, config ModelConfig, logger *comm return nil, fmt.Errorf("failed to create model provider '%s': %w", cagentModelConfig.Provider, err) } - logger.Info("Successfully initialized %s provider for model %s", + logger.Debug("Successfully initialized %s provider for model %s", cagentModelConfig.Provider, cagentModelConfig.Model) return client, nil } diff --git a/pkg/agent/model.go b/pkg/agent/model.go index 4180a26..61628fc 100644 --- a/pkg/agent/model.go +++ b/pkg/agent/model.go @@ -149,7 +149,7 @@ type GenericProvider struct { } func (p *GenericProvider) InitializeClient(config ModelConfig, logger *common.Logger) (*openai.Client, error) { - logger.Info("Unknown model class '%s', treating as OpenAI-compatible", p.class) + logger.Warn("Unknown model class '%s', treating as OpenAI-compatible", p.class) apiKey := config.APIKey if apiKey == "" { diff --git a/pkg/command/command.go b/pkg/command/command.go index 0e6a598..f03cac5 100644 --- a/pkg/command/command.go +++ b/pkg/command/command.go @@ -84,7 +84,7 @@ func NewCommandHandler(tool config.Tool, params map[string]common.ParamConfig, s var err error if len(tool.Config.Constraints) > 0 { - logger.Info("Compiling %d constraints for tool '%s'", len(tool.Config.Constraints), tool.MCPTool.Name) + logger.Debug("Compiling %d constraints for tool '%s'", len(tool.Config.Constraints), tool.MCPTool.Name) compiled, err = common.NewCompiledConstraints(tool.Config.Constraints, params, logger) if err != nil { @@ -92,7 +92,7 @@ func NewCommandHandler(tool config.Tool, params map[string]common.ParamConfig, s return nil, fmt.Errorf("constraint compilation error: %w", err) } - logger.Info("Successfully compiled constraints for tool '%s'", tool.MCPTool.Name) + logger.Debug("Successfully compiled constraints for tool '%s'", tool.MCPTool.Name) } // Get the effective command, runner type, and options from the tool diff --git a/pkg/command/command_exec.go b/pkg/command/command_exec.go index 38b08af..f3f1986 100644 --- a/pkg/command/command_exec.go +++ b/pkg/command/command_exec.go @@ -184,7 +184,7 @@ func (h *CommandHandler) executeToolCommand(ctx context.Context, params map[stri h.logger.Debug("Final output with prefix:\n--------------------------------\n%s\n--------------------------------", finalOutput) } - h.logger.Info("Tool execution completed successfully") + h.logger.Debug("Tool execution completed successfully") return finalOutput, nil, nil } diff --git a/pkg/command/runner.go b/pkg/command/runner.go index 478b6cf..cb12016 100644 --- a/pkg/command/runner.go +++ b/pkg/command/runner.go @@ -72,7 +72,7 @@ func NewRunner(runnerType RunnerType, options RunnerOptions, logger *common.Logg // Check implicit requirements for the created runner if err := runner.CheckImplicitRequirements(); err != nil { if logger != nil { - logger.Printf("Runner %s failed implicit requirements check: %v", runnerType, err) + logger.Debug("Runner %s failed implicit requirements check: %v", runnerType, err) } return nil, err } diff --git a/pkg/command/runner_docker.go b/pkg/command/runner_docker.go index 765ac94..c79e95f 100644 --- a/pkg/command/runner_docker.go +++ b/pkg/command/runner_docker.go @@ -357,7 +357,7 @@ func (r *DockerRunner) Run(ctx context.Context, shell string, cmd string, env [] // Determine if we should run directly or via script if isSingleExecutableCommand(cmd) { - r.logger.Printf("Optimization: running single executable command directly in Docker: %s", cmd) + r.logger.Debug("Optimization: running single executable command directly in Docker: %s", cmd) // Build docker command to directly execute the command without a temp script dockerCmd = r.opts.GetDirectExecutionCommand(cmd, env) @@ -371,17 +371,17 @@ func (r *DockerRunner) Run(ctx context.Context, shell string, cmd string, env [] // Clean up the temporary script file when done defer func() { if err := os.Remove(scriptFile); err != nil { - r.logger.Printf("Warning: failed to remove temporary script file %s: %v", scriptFile, err) + r.logger.Debug("Warning: failed to remove temporary script file %s: %v", scriptFile, err) } }() - r.logger.Printf("Created temporary script file: %s", scriptFile) + r.logger.Debug("Created temporary script file: %s", scriptFile) // Construct the docker run command with the script file dockerCmd = r.opts.GetDockerCommand(scriptFile, env) } - r.logger.Printf("Running command in Docker: %s", dockerCmd) + r.logger.Debug("Running command in Docker: %s", dockerCmd) // Run the docker command - we set tmpfile to false because dockerCmd is already a full command output, err := execRunner.Run(ctx, "sh", dockerCmd, nil, params, false) @@ -420,7 +420,7 @@ func (r *DockerRunner) createScriptFile(shell string, cmd string, env []string) content.WriteString("\n# Preparation commands\n") content.WriteString(r.opts.PrepareCommand) content.WriteString("\n\n") - r.logger.Printf("Added preparation command to script: %s", r.opts.PrepareCommand) + r.logger.Debug("Added preparation command to script: %s", r.opts.PrepareCommand) } // Add the main command @@ -452,6 +452,6 @@ func (r *DockerRunner) createScriptFile(shell string, cmd string, env []string) return "", fmt.Errorf("failed to close temporary script file: %w", err) } - r.logger.Printf("Created temporary script file at: %s", scriptPath) + r.logger.Debug("Created temporary script file at: %s", scriptPath) return scriptPath, nil } diff --git a/pkg/command/runner_exec.go b/pkg/command/runner_exec.go index b08f2ee..a70c344 100644 --- a/pkg/command/runner_exec.go +++ b/pkg/command/runner_exec.go @@ -74,27 +74,27 @@ func (r *RunnerExec) Run(ctx context.Context, shell string, var tmpDir string if isSingleExecutableCommand(command) { - r.logger.Printf("Optimization: running single executable command directly: %s", command) + r.logger.Debug("Optimization: running single executable command directly: %s", command) execCmd = exec.CommandContext(ctx, command) if len(env) > 0 { - r.logger.Printf("Adding %d environment variables to command", len(env)) + r.logger.Debug("Adding %d environment variables to command", len(env)) for _, e := range env { - r.logger.Printf("... adding environment variable: %s", e) + r.logger.Debug("... adding environment variable: %s", e) } execCmd.Env = append(os.Environ(), env...) } - r.logger.Printf("Created command: %s", command) + r.logger.Debug("Created command: %s", command) } else if tmpfile { // Create a temporary file for the command var err error tmpDir, err = os.MkdirTemp("", "mcpshell") if err != nil { - r.logger.Printf("Failed to create temp directory: %v", err) + r.logger.Debug("Failed to create temp directory: %v", err) return "", err } defer func() { if err := os.RemoveAll(tmpDir); err != nil { - r.logger.Printf("Failed to remove temporary directory: %v", err) + r.logger.Debug("Failed to remove temporary directory: %v", err) } }() @@ -106,34 +106,34 @@ func (r *RunnerExec) Run(ctx context.Context, shell string, tmpFile := filepath.Join(tmpDir, "script.sh") err = os.WriteFile(tmpFile, []byte(scriptContent.String()), 0o700) if err != nil { - r.logger.Printf("Failed to write temporary file: %v", err) + r.logger.Debug("Failed to write temporary file: %v", err) return "", err } - r.logger.Printf("Created temporary script file at: %s", tmpFile) + r.logger.Debug("Created temporary script file at: %s", tmpFile) // Set up the command configShell := getShell(shell) - r.logger.Printf("Using shell: %s", configShell) + r.logger.Debug("Using shell: %s", configShell) // Create the command to execute the script file execCmd = exec.CommandContext(ctx, configShell, tmpFile) - r.logger.Printf("Created command: %s %s", configShell, tmpFile) + r.logger.Debug("Created command: %s %s", configShell, tmpFile) } else { // Execute the command directly without a temporary file configShell := getShell(shell) - r.logger.Printf("Using shell: %s", configShell) + r.logger.Debug("Using shell: %s", configShell) // Simple command without arguments execCmd = exec.CommandContext(ctx, configShell, "-c", command) - r.logger.Printf("Created command: %s -c %s", configShell, command) + r.logger.Debug("Created command: %s -c %s", configShell, command) } // Set environment variables if provided if len(env) > 0 { - r.logger.Printf("Adding %d environment variables to command", len(env)) + r.logger.Debug("Adding %d environment variables to command", len(env)) for _, e := range env { - r.logger.Printf("... adding environment variable: %s", e) + r.logger.Debug("... adding environment variable: %s", e) } execCmd.Env = append(os.Environ(), env...) } @@ -144,26 +144,26 @@ func (r *RunnerExec) Run(ctx context.Context, shell string, execCmd.Stderr = &stderr // Run the command - r.logger.Printf("Executing command") + r.logger.Debug("Executing command") err := execCmd.Run() if err != nil { // If there's error output, include it in the error if stderr.Len() > 0 { errMsg := strings.TrimSpace(stderr.String()) - r.logger.Printf("Command failed with stderr: %s", errMsg) + r.logger.Debug("Command failed with stderr: %s", errMsg) return "", errors.New(errMsg) } - r.logger.Printf("Command failed with error: %v", err) + r.logger.Debug("Command failed with error: %v", err) return "", err } // Get the output output := strings.TrimSpace(stdout.String()) - r.logger.Printf("Command executed successfully, output length: %d bytes", len(output)) + r.logger.Debug("Command executed successfully, output length: %d bytes", len(output)) if stderr.Len() > 0 { - r.logger.Printf("Command generated stderr (but no error): %s", strings.TrimSpace(stderr.String())) + r.logger.Debug("Command generated stderr (but no error): %s", strings.TrimSpace(stderr.String())) } // Return the stdout output diff --git a/pkg/command/runner_firejail.go b/pkg/command/runner_firejail.go index 0f247f7..78346f5 100644 --- a/pkg/command/runner_firejail.go +++ b/pkg/command/runner_firejail.go @@ -61,14 +61,14 @@ func NewRunnerFirejail(options RunnerOptions, logger *common.Logger) (*RunnerFir // Parse the firejail profile template profileTpl, err := template.New("firejail-profile").Parse(firejailProfileTemplate) if err != nil { - logger.Printf("Failed to parse firejail profile template: %v", err) + logger.Debug("Failed to parse firejail profile template: %v", err) return nil, err } // Parse firejail-specific options firejailOpts, err := NewRunnerFirejailOptions(options) if err != nil { - logger.Printf("Failed to parse firejail options: %v", err) + logger.Debug("Failed to parse firejail options: %v", err) return nil, fmt.Errorf("failed to parse firejail options: %w", err) } @@ -114,39 +114,39 @@ func (r *RunnerFirejail) Run(ctx context.Context, // Generate the profile by rendering the template var profileBuf bytes.Buffer if err := r.profileTpl.Execute(&profileBuf, r.options); err != nil { - r.logger.Printf("Failed to render firejail profile template: %v", err) + r.logger.Debug("Failed to render firejail profile template: %v", err) return "", fmt.Errorf("failed to render firejail profile: %w", err) } profile := profileBuf.String() - r.logger.Printf("Firejail options: %+v", r.options) - r.logger.Printf("Generated firejail profile: %s", profile) + r.logger.Debug("Firejail options: %+v", r.options) + r.logger.Debug("Generated firejail profile: %s", profile) // Create a temporary file for the firejail profile profileFile, err := os.CreateTemp("", "firejail-profile-*.profile") if err != nil { - r.logger.Printf("Failed to create temporary profile file: %v", err) + r.logger.Debug("Failed to create temporary profile file: %v", err) return "", fmt.Errorf("failed to create temporary profile file: %w", err) } defer func() { profileFilePath := profileFile.Name() if err := profileFile.Close(); err != nil { - r.logger.Printf("Warning: failed to close profile file: %v", err) + r.logger.Debug("Warning: failed to close profile file: %v", err) } if err := os.Remove(profileFilePath); err != nil { - r.logger.Printf("Warning: failed to remove temporary profile file: %v", err) + r.logger.Debug("Warning: failed to remove temporary profile file: %v", err) } }() // Write the profile to the temporary file if _, err := profileFile.WriteString(profile); err != nil { - r.logger.Printf("Failed to write profile to temporary file: %v", err) + r.logger.Debug("Failed to write profile to temporary file: %v", err) return "", fmt.Errorf("failed to write profile to temporary file: %w", err) } // Flush data to ensure it's written to disk if err := profileFile.Sync(); err != nil { - r.logger.Printf("Failed to sync profile file: %v", err) + r.logger.Debug("Failed to sync profile file: %v", err) return "", fmt.Errorf("failed to sync profile file: %w", err) } @@ -154,41 +154,41 @@ func (r *RunnerFirejail) Run(ctx context.Context, // Check if we can optimize by running a single executable directly if isSingleExecutableCommand(fullCmd) { - r.logger.Printf("Optimization: running single executable command directly: %s", fullCmd) + r.logger.Debug("Optimization: running single executable command directly: %s", fullCmd) execCmd = exec.CommandContext(ctx, "firejail", "--profile="+profileFile.Name(), fullCmd) } else { // Create a temporary file for the command tmpScript, err := os.CreateTemp("", "firejail-command-*.sh") if err != nil { - r.logger.Printf("Failed to create temporary command file: %v", err) + r.logger.Debug("Failed to create temporary command file: %v", err) return "", fmt.Errorf("failed to create temporary command file: %w", err) } // Ensure temporary file is deleted when this function exits defer func() { tmpScriptPath := tmpScript.Name() if err := tmpScript.Close(); err != nil { - r.logger.Printf("Warning: failed to close script file: %v", err) + r.logger.Debug("Warning: failed to close script file: %v", err) } if err := os.Remove(tmpScriptPath); err != nil { - r.logger.Printf("Warning: failed to remove temporary script file: %v", err) + r.logger.Debug("Warning: failed to remove temporary script file: %v", err) } }() // Write the command to the temporary file if _, err := tmpScript.WriteString(fullCmd); err != nil { - r.logger.Printf("Failed to write command to temporary file: %v", err) + r.logger.Debug("Failed to write command to temporary file: %v", err) return "", fmt.Errorf("failed to write command to temporary file: %w", err) } // Flush data to ensure it's written to disk if err := tmpScript.Sync(); err != nil { - r.logger.Printf("Failed to sync script file: %v", err) + r.logger.Debug("Failed to sync script file: %v", err) return "", fmt.Errorf("failed to sync script file: %w", err) } // Make the temporary file executable if err := os.Chmod(tmpScript.Name(), 0o700); err != nil { - r.logger.Printf("Failed to make temporary file executable: %v", err) + r.logger.Debug("Failed to make temporary file executable: %v", err) return "", fmt.Errorf("failed to make temporary file executable: %w", err) } @@ -203,13 +203,13 @@ func (r *RunnerFirejail) Run(ctx context.Context, // Continue execution } - r.logger.Printf("Created command: %s", execCmd.String()) + r.logger.Debug("Created command: %s", execCmd.String()) // Set environment variables if provided if len(env) > 0 { - r.logger.Printf("Adding %d environment variables to command", len(env)) + r.logger.Debug("Adding %d environment variables to command", len(env)) for _, e := range env { - r.logger.Printf("... adding environment variable: %s", e) + r.logger.Debug("... adding environment variable: %s", e) } execCmd.Env = append(os.Environ(), env...) } @@ -220,25 +220,25 @@ func (r *RunnerFirejail) Run(ctx context.Context, execCmd.Stderr = &stderr // Run the command - r.logger.Printf("Executing command") + r.logger.Debug("Executing command") if err := execCmd.Run(); err != nil { // If there's error output, include it in the error if stderr.Len() > 0 { errMsg := strings.TrimSpace(stderr.String()) - r.logger.Printf("Command failed with stderr: %s", errMsg) + r.logger.Debug("Command failed with stderr: %s", errMsg) return "", errors.New(errMsg) } - r.logger.Printf("Command failed with error: %v", err) + r.logger.Debug("Command failed with error: %v", err) return "", err } // Get the output outputStr := strings.TrimSpace(stdout.String()) - r.logger.Printf("Command executed successfully, output length: %d bytes", len(outputStr)) + r.logger.Debug("Command executed successfully, output length: %d bytes", len(outputStr)) if stderr.Len() > 0 { - r.logger.Printf("Command generated stderr (but no error): %s", strings.TrimSpace(stderr.String())) + r.logger.Debug("Command generated stderr (but no error): %s", strings.TrimSpace(stderr.String())) } // Return the stdout output diff --git a/pkg/command/runner_sandbox.go b/pkg/command/runner_sandbox.go index 8bacad7..47a4bca 100644 --- a/pkg/command/runner_sandbox.go +++ b/pkg/command/runner_sandbox.go @@ -62,14 +62,14 @@ func NewRunnerSandboxExec(options RunnerOptions, logger *common.Logger) (*Runner // Parse the sandbox profile template profileTpl, err := template.New("sandbox-profile").Parse(sandboxProfileTemplate) if err != nil { - logger.Printf("Failed to parse sandbox profile template: %v", err) + logger.Debug("Failed to parse sandbox profile template: %v", err) return nil, err } // Parse sandbox-specific options sandboxOpts, err := NewRunnerSandboxExecOptions(options) if err != nil { - logger.Printf("Failed to parse sandbox options: %v", err) + logger.Debug("Failed to parse sandbox options: %v", err) return nil, fmt.Errorf("failed to parse sandbox options: %w", err) } @@ -112,7 +112,7 @@ func (r *RunnerSandboxExec) Run(ctx context.Context, shell string, command strin // Add parent directory if not already in the list if !contains(r.options.AllowReadFolders, dir) { r.options.AllowReadFolders = append(r.options.AllowReadFolders, dir) - r.logger.Printf("[DEBUG] Added parent directory to allow list: %s", dir) + r.logger.Debug("[DEBUG] Added parent directory to allow list: %s", dir) } } } @@ -126,7 +126,7 @@ func (r *RunnerSandboxExec) Run(ctx context.Context, shell string, command strin // Add parent directory if not already in the list if !contains(r.options.AllowWriteFolders, dir) { r.options.AllowWriteFolders = append(r.options.AllowWriteFolders, dir) - r.logger.Printf("[DEBUG] Added parent directory to allow list: %s", dir) + r.logger.Debug("[DEBUG] Added parent directory to allow list: %s", dir) } } } @@ -134,39 +134,39 @@ func (r *RunnerSandboxExec) Run(ctx context.Context, shell string, command strin // Generate the profile by rendering the template var profileBuf bytes.Buffer if err := r.profileTpl.Execute(&profileBuf, r.options); err != nil { - r.logger.Printf("Failed to render sandbox profile template: %v", err) + r.logger.Debug("Failed to render sandbox profile template: %v", err) return "", fmt.Errorf("failed to render sandbox profile: %w", err) } profile := profileBuf.String() - r.logger.Printf("Sandbox options: %+v", r.options) - r.logger.Printf("Generated sandbox profile:\n%s", profile) + r.logger.Debug("Sandbox options: %+v", r.options) + r.logger.Debug("Generated sandbox profile:\n%s", profile) // Create a temporary file for the sandbox profile profileFile, err := os.CreateTemp("", "sandbox-profile-*.sb") if err != nil { - r.logger.Printf("Failed to create temporary profile file: %v", err) + r.logger.Debug("Failed to create temporary profile file: %v", err) return "", fmt.Errorf("failed to create temporary profile file: %w", err) } defer func() { profileFilePath := profileFile.Name() if err := profileFile.Close(); err != nil { - r.logger.Printf("Warning: failed to close profile file: %v", err) + r.logger.Debug("Warning: failed to close profile file: %v", err) } if err := os.Remove(profileFilePath); err != nil { - r.logger.Printf("Warning: failed to remove temporary profile file: %v", err) + r.logger.Debug("Warning: failed to remove temporary profile file: %v", err) } }() // Write the profile to the temporary file if _, err := profileFile.WriteString(profile); err != nil { - r.logger.Printf("Failed to write profile to temporary file: %v", err) + r.logger.Debug("Failed to write profile to temporary file: %v", err) return "", fmt.Errorf("failed to write profile to temporary file: %w", err) } // Flush data to ensure it's written to disk if err := profileFile.Sync(); err != nil { - r.logger.Printf("Failed to sync profile file: %v", err) + r.logger.Debug("Failed to sync profile file: %v", err) return "", fmt.Errorf("failed to sync profile file: %w", err) } @@ -174,54 +174,54 @@ func (r *RunnerSandboxExec) Run(ctx context.Context, shell string, command strin // Check if we can optimize by running a single executable directly if isSingleExecutableCommand(fullCmd) { - r.logger.Printf("Optimization: running single executable command directly: %s", fullCmd) + r.logger.Debug("Optimization: running single executable command directly: %s", fullCmd) execCmd = exec.CommandContext(ctx, "sandbox-exec", "-f", profileFile.Name(), fullCmd) } else { // Create a temporary file for the command tmpScript, err := os.CreateTemp("", "sandbox-script-*.sh") if err != nil { - r.logger.Printf("Failed to create temporary command file: %v", err) + r.logger.Debug("Failed to create temporary command file: %v", err) return "", fmt.Errorf("failed to create temporary command file: %w", err) } // Ensure temporary file is deleted when this function exits defer func() { tmpScriptPath := tmpScript.Name() if err := tmpScript.Close(); err != nil { - r.logger.Printf("Warning: failed to close script file: %v", err) + r.logger.Debug("Warning: failed to close script file: %v", err) } if err := os.Remove(tmpScriptPath); err != nil { - r.logger.Printf("Warning: failed to remove temporary script file: %v", err) + r.logger.Debug("Warning: failed to remove temporary script file: %v", err) } }() // Write the command to the temporary file if _, err := tmpScript.WriteString(fullCmd); err != nil { - r.logger.Printf("Failed to write command to temporary file: %v", err) + r.logger.Debug("Failed to write command to temporary file: %v", err) return "", fmt.Errorf("failed to write command to temporary file: %w", err) } // Flush data to ensure it's written to disk if err := tmpScript.Sync(); err != nil { - r.logger.Printf("Failed to sync script file: %v", err) + r.logger.Debug("Failed to sync script file: %v", err) return "", fmt.Errorf("failed to sync script file: %w", err) } // Make the temporary file executable if err := os.Chmod(tmpScript.Name(), 0o700); err != nil { - r.logger.Printf("Failed to make temporary file executable: %v", err) + r.logger.Debug("Failed to make temporary file executable: %v", err) return "", fmt.Errorf("failed to make temporary file executable: %w", err) } execCmd = exec.CommandContext(ctx, "sandbox-exec", "-f", profileFile.Name(), tmpScript.Name()) } - r.logger.Printf("Created command: %s", execCmd.String()) + r.logger.Debug("Created command: %s", execCmd.String()) // Set environment variables if provided if len(env) > 0 { - r.logger.Printf("Adding %d environment variables to command", len(env)) + r.logger.Debug("Adding %d environment variables to command", len(env)) for _, e := range env { - r.logger.Printf("... adding environment variable: %s", e) + r.logger.Debug("... adding environment variable: %s", e) } execCmd.Env = append(os.Environ(), env...) } @@ -232,25 +232,25 @@ func (r *RunnerSandboxExec) Run(ctx context.Context, shell string, command strin execCmd.Stderr = &stderr // Run the command - r.logger.Printf("Executing command") + r.logger.Debug("Executing command") if err := execCmd.Run(); err != nil { // If there's error output, include it in the error if stderr.Len() > 0 { errMsg := strings.TrimSpace(stderr.String()) - r.logger.Printf("Command failed with stderr: %s", errMsg) + r.logger.Debug("Command failed with stderr: %s", errMsg) return "", errors.New(errMsg) } - r.logger.Printf("Command failed with error: %v", err) + r.logger.Debug("Command failed with error: %v", err) return "", err } // Get the output outputStr := strings.TrimSpace(stdout.String()) - r.logger.Printf("Command executed successfully, output length: %d bytes", len(outputStr)) + r.logger.Debug("Command executed successfully, output length: %d bytes", len(outputStr)) if stderr.Len() > 0 { - r.logger.Printf("Command generated stderr (but no error): %s", strings.TrimSpace(stderr.String())) + r.logger.Debug("Command generated stderr (but no error): %s", strings.TrimSpace(stderr.String())) } // Return the stdout output diff --git a/pkg/common/constraints.go b/pkg/common/constraints.go index 803bc46..fc3c39c 100644 --- a/pkg/common/constraints.go +++ b/pkg/common/constraints.go @@ -107,7 +107,7 @@ func (cc *CompiledConstraints) Evaluate(args map[string]interface{}, params map[ evalArgs := make(map[string]interface{}) for k, v := range args { evalArgs[k] = v - cc.logger.Printf("Argument provided: %s = %v", k, v) + cc.logger.Debug("Argument provided: %s = %v", k, v) } // Ensure all parameters have at least empty values if not provided @@ -117,13 +117,13 @@ func (cc *CompiledConstraints) Evaluate(args map[string]interface{}, params map[ switch param.Type { case "string", "": evalArgs[name] = "" - cc.logger.Printf("Adding default empty string for missing parameter: %s", name) + cc.logger.Debug("Adding default empty string for missing parameter: %s", name) case "number", "integer": evalArgs[name] = 0.0 - cc.logger.Printf("Adding default zero value for missing parameter: %s", name) + cc.logger.Debug("Adding default zero value for missing parameter: %s", name) case "boolean": evalArgs[name] = false - cc.logger.Printf("Adding default false value for missing parameter: %s", name) + cc.logger.Debug("Adding default false value for missing parameter: %s", name) } } } @@ -133,17 +133,17 @@ func (cc *CompiledConstraints) Evaluate(args map[string]interface{}, params map[ // Evaluate each constraint program for i, prg := range cc.programs { // Execute the program - cc.logger.Printf("Evaluating constraint #%d: %s", i+1, cc.expressions[i]) + cc.logger.Debug("Evaluating constraint #%d: %s", i+1, cc.expressions[i]) val, _, err := prg.Eval(evalArgs) if err != nil { - cc.logger.Printf("Constraint #%d evaluation error: %v", i+1, err) + cc.logger.Debug("Constraint #%d evaluation error: %v", i+1, err) return false, nil, fmt.Errorf("constraint evaluation error: %w", err) } // Check if the result is a boolean and is true boolVal, ok := val.Value().(bool) if !ok { - cc.logger.Printf("Constraint #%d did not evaluate to a boolean", i+1) + cc.logger.Debug("Constraint #%d did not evaluate to a boolean", i+1) return false, nil, fmt.Errorf("constraint did not evaluate to a boolean") } @@ -151,15 +151,15 @@ func (cc *CompiledConstraints) Evaluate(args map[string]interface{}, params map[ // If any constraint fails, add it to the failed constraints list failureMsg := fmt.Sprintf("%s (with values: %s)", cc.expressions[i], formatArgValues(evalArgs)) failedConstraints = append(failedConstraints, failureMsg) - cc.logger.Printf("Constraint #%d failed evaluation: %s", i+1, failureMsg) + cc.logger.Debug("Constraint #%d failed evaluation: %s", i+1, failureMsg) } else { - cc.logger.Printf("Constraint #%d passed evaluation", i+1) + cc.logger.Debug("Constraint #%d passed evaluation", i+1) } } // Return failure if any constraints failed if len(failedConstraints) > 0 { - cc.logger.Printf("%d constraints failed evaluation", len(failedConstraints)) + cc.logger.Debug("%d constraints failed evaluation", len(failedConstraints)) return false, failedConstraints, nil } diff --git a/pkg/common/logging.go b/pkg/common/logging.go index 7ebf092..42f64f3 100644 --- a/pkg/common/logging.go +++ b/pkg/common/logging.go @@ -104,8 +104,8 @@ func NewLogger(prefix string, filePath string, level LogLevel, truncate bool) (* // Log the initialization if filePath != "" && level >= LogLevelInfo { - logger.Printf("----------------------------") - logger.Printf("Logging initialized to file: %s", filePath) + logger.Debug("----------------------------") + logger.Debug("Logging initialized to file: %s", filePath) } return logger, nil @@ -133,6 +133,13 @@ func (l *Logger) Info(format string, v ...interface{}) { } } +// Warn logs a warning message +func (l *Logger) Warn(format string, v ...interface{}) { + if l.level >= LogLevelInfo { + l.Printf("[WARN] "+format, v...) + } +} + // Error logs a message at error level func (l *Logger) Error(format string, v ...interface{}) { if l.level >= LogLevelError { diff --git a/pkg/common/panic.go b/pkg/common/panic.go index 4a4330a..6f2071d 100644 --- a/pkg/common/panic.go +++ b/pkg/common/panic.go @@ -19,8 +19,8 @@ func RecoverPanic() bool { // Log panic information to the logger if provided if logger != nil { - logger.Printf("PANIC RECOVERED: %v", r) - logger.Printf("Stack trace:\n%s", stackTrace) + logger.Debug("PANIC RECOVERED: %v", r) + logger.Debug("Stack trace:\n%s", stackTrace) } // Always log to stderr for immediate visibility diff --git a/pkg/server/server.go b/pkg/server/server.go index 30073a7..476c8ab 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -460,7 +460,7 @@ func (s *Server) ExecuteTool(ctx context.Context, toolName string, args map[stri s.logger.Debug("Sending JSON-RPC request: %s", string(jsonBytes)) // Execute the tool through the MCP server - s.logger.Info("Executing tool: %s", toolName) + s.logger.Debug("Executing tool: %s", toolName) // We need to handle the request manually since we don't have direct access to tool handlers jsonMsg := s.mcpServer.HandleMessage(ctx, mustMarshalJSON(jsonRpcRequest)) From 759d45250f85d0ef22aa293fe33cb46ccd884b2e Mon Sep 17 00:00:00 2001 From: Alvaro <1841612+inercia@users.noreply.github.com> Date: Mon, 20 Oct 2025 15:40:12 +0200 Subject: [PATCH 2/2] Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- docs/usage-agent.md | 4 ++-- pkg/common/panic.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/usage-agent.md b/docs/usage-agent.md index 0dc26d2..512ab3f 100644 --- a/docs/usage-agent.md +++ b/docs/usage-agent.md @@ -53,7 +53,7 @@ MCPShell has agent-specific configuration that includes model definitions with p ## Running the Agent -With the tools degined in `disk-diagnostics-ro.yaml`, you can run: +With the tools defined in `disk-diagnostics-ro.yaml`, you can run: ```bash mcpshell agent \ @@ -90,7 +90,7 @@ agent: name: "llama3" ``` -- Load system prompts from the agent configuration (if any, or use teh default ones). +- Load system prompts from the agent configuration (if any, or use the default ones). - Load tools from `disk-diagnostics-ro.yaml`. - Connect to the configured LLM API. - Process the LLM's responses and execute tool calls as requested. diff --git a/pkg/common/panic.go b/pkg/common/panic.go index 6f2071d..c38062a 100644 --- a/pkg/common/panic.go +++ b/pkg/common/panic.go @@ -19,8 +19,8 @@ func RecoverPanic() bool { // Log panic information to the logger if provided if logger != nil { - logger.Debug("PANIC RECOVERED: %v", r) - logger.Debug("Stack trace:\n%s", stackTrace) + logger.Error("PANIC RECOVERED: %v", r) + logger.Error("Stack trace:\n%s", stackTrace) } // Always log to stderr for immediate visibility