diff --git a/pkg/cli/mcp_server_command.go b/pkg/cli/mcp_server_command.go index 9ff133b174..533b627b23 100644 --- a/pkg/cli/mcp_server_command.go +++ b/pkg/cli/mcp_server_command.go @@ -2,11 +2,9 @@ package cli import ( "context" - "fmt" "os" "strings" - "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/workflow" "github.com/modelcontextprotocol/go-sdk/mcp" @@ -73,24 +71,20 @@ Examples: return cmd } -// checkAndLogGHVersion checks if gh CLI is available and logs its version +// checkAndLogGHVersion checks if gh CLI is available and logs its version. +// Diagnostics are emitted through the debug logger only. func checkAndLogGHVersion() { cmd := workflow.ExecGH("version") output, err := cmd.CombinedOutput() if err != nil { mcpLog.Print("WARNING: gh CLI not found in PATH") - fmt.Fprintln(os.Stderr, console.FormatWarningMessage("gh CLI not found in PATH - some MCP server operations may fail")) return } // Parse and log the version versionOutput := strings.TrimSpace(string(output)) mcpLog.Printf("gh CLI version: %s", versionOutput) - - // Extract just the first line for cleaner logging to stderr - firstLine := strings.Split(versionOutput, "\n")[0] - fmt.Fprintln(os.Stderr, console.FormatInfoMessage("gh CLI: "+firstLine)) } // runMCPServer starts the MCP server on stdio or HTTP transport @@ -100,19 +94,12 @@ func runMCPServer(port int, cmdPath string, validateActor bool) error { if validateActor { mcpLog.Printf("Actor validation enabled (--validate-actor flag)") - fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Actor validation enabled")) } if actor != "" { mcpLog.Printf("Using actor: %s", actor) - fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Actor: "+actor)) } else { mcpLog.Print("No actor specified (GITHUB_ACTOR environment variable)") - if validateActor { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage("No actor specified - logs and audit tools will not be mounted (actor validation enabled)")) - } else { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage("No actor specified - all tools will be mounted (actor validation disabled)")) - } } if port > 0 { @@ -136,10 +123,8 @@ func runMCPServer(port int, cmdPath string, validateActor bool) error { // Log current working directory if cwd, err := os.Getwd(); err == nil { mcpLog.Printf("Current working directory: %s", cwd) - fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Current working directory: "+cwd)) } else { mcpLog.Printf("WARNING: Failed to get current working directory: %v", err) - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to get current working directory: %v", err))) } // Check and log gh CLI version @@ -150,7 +135,6 @@ func runMCPServer(port int, cmdPath string, validateActor bool) error { // This allows the server to start in test environments or non-repository directories if err := validateMCPServerConfiguration(cmdPath); err != nil { mcpLog.Printf("Configuration validation warning: %v", err) - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Configuration validation warning: %v", err))) } // Pre-cache lock-file manifests at startup, before any agent can modify the working tree. @@ -165,7 +149,6 @@ func runMCPServer(port int, cmdPath string, validateActor bool) error { } else { manifestCacheFile = cacheFile mcpLog.Printf("Manifest cache written to %s (%d entries)", cacheFile, len(manifestCache)) - fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Pre-cached %d workflow manifest(s) for safe update enforcement", len(manifestCache)))) // Clean up the temp file when the server exits defer func() { if removeErr := os.Remove(cacheFile); removeErr != nil && !os.IsNotExist(removeErr) { diff --git a/pkg/cli/mcp_server_stdio_integration_test.go b/pkg/cli/mcp_server_stdio_integration_test.go new file mode 100644 index 0000000000..c01d4ff489 --- /dev/null +++ b/pkg/cli/mcp_server_stdio_integration_test.go @@ -0,0 +1,86 @@ +//go:build integration + +package cli + +import ( + "bytes" + "context" + "errors" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/github/gh-aw/pkg/testutil" +) + +func TestMCPServer_StdioDiagnosticsGoToStderr(t *testing.T) { + binaryPath := "../../gh-aw" + if _, err := os.Stat(binaryPath); os.IsNotExist(err) { + t.Skip("Skipping test: gh-aw binary not found. Run 'make build' first.") + } + + tmpDir := testutil.TempDir(t, "mcp-stdio-*") + workflowsDir := filepath.Join(tmpDir, ".github", "workflows") + if err := os.MkdirAll(workflowsDir, 0755); err != nil { + t.Fatalf("Failed to create workflows directory: %v", err) + } + + workflowPath := filepath.Join(workflowsDir, "test.md") + workflowContent := `--- +on: push +engine: copilot +--- +# Test Workflow +` + if err := os.WriteFile(workflowPath, []byte(workflowContent), 0644); err != nil { + t.Fatalf("Failed to write workflow file: %v", err) + } + + if err := initTestGitRepo(tmpDir); err != nil { + t.Fatalf("Failed to initialize git repository: %v", err) + } + + originalDir, err := os.Getwd() + if err != nil { + t.Fatalf("Failed to get current working directory: %v", err) + } + + absBinaryPath := filepath.Join(originalDir, binaryPath) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + cmd := exec.CommandContext(ctx, absBinaryPath, "mcp-server", "--cmd", absBinaryPath) + cmd.Dir = tmpDir + cmd.Stdin = strings.NewReader("") + + env := make([]string, 0, len(os.Environ())) + for _, entry := range os.Environ() { + if strings.HasPrefix(entry, "GITHUB_ACTOR=") { + continue + } + env = append(env, entry) + } + cmd.Env = env + + var stdout bytes.Buffer + var stderr bytes.Buffer + cmd.Stdout = &stdout + cmd.Stderr = &stderr + + err = cmd.Run() + if err != nil && !errors.Is(ctx.Err(), context.DeadlineExceeded) { + var exitErr *exec.ExitError + if !errors.As(err, &exitErr) { + t.Fatalf("Failed to run MCP server process: %v", err) + } + } + + stdoutText := strings.TrimSpace(stdout.String()) + if stdoutText != "" { + t.Fatalf("Expected stdout to remain clean for JSON-RPC, got: %q", stdoutText) + } + +} diff --git a/pkg/cli/mcp_validation.go b/pkg/cli/mcp_validation.go index 32948684f3..fdd57d90f8 100644 --- a/pkg/cli/mcp_validation.go +++ b/pkg/cli/mcp_validation.go @@ -50,11 +50,11 @@ func GetBinaryPath() (string, error) { // logAndValidateBinaryPath determines the binary path, logs it, and validates it exists. // Returns the detected binary path and an error if the path cannot be determined or if the file doesn't exist. // This is a helper used by both runMCPServer and validateMCPServerConfiguration. +// Diagnostics are emitted through the debug logger only. func logAndValidateBinaryPath() (string, error) { binaryPath, err := GetBinaryPath() if err != nil { mcpValidationLog.Printf("Warning: failed to get binary path: %v", err) - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Warning: failed to get binary path: %v", err))) return "", err } @@ -62,17 +62,14 @@ func logAndValidateBinaryPath() (string, error) { if _, err := os.Stat(binaryPath); err != nil { if os.IsNotExist(err) { mcpValidationLog.Printf("ERROR: binary file does not exist at path: %s", binaryPath) - fmt.Fprintln(os.Stderr, console.FormatErrorMessage("ERROR: binary file does not exist at path: "+binaryPath)) return "", fmt.Errorf("binary file does not exist at path: %s", binaryPath) } mcpValidationLog.Printf("Warning: failed to stat binary file at %s: %v", binaryPath, err) - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Warning: failed to stat binary file at %s: %v", binaryPath, err))) return "", err } // Log the binary path for debugging mcpValidationLog.Printf("gh-aw binary path: %s", binaryPath) - fmt.Fprintln(os.Stderr, console.FormatInfoMessage("gh-aw binary path: "+binaryPath)) return binaryPath, nil } @@ -222,7 +219,8 @@ func validateServerSecrets(config parser.MCPServerConfig, verbose bool, useActio } // validateMCPServerConfiguration validates that the CLI is properly configured -// by running the status command as a test +// by running the status command as a test. +// Diagnostics are emitted through the debug logger only. func validateMCPServerConfiguration(cmdPath string) error { mcpValidationLog.Printf("Validating MCP server configuration: cmdPath=%s", cmdPath) @@ -257,8 +255,6 @@ func validateMCPServerConfiguration(cmdPath string) error { // Check for common error cases if ctx.Err() == context.DeadlineExceeded { mcpValidationLog.Print("Status command timed out") - errMsg := "status command timed out - this may indicate a configuration issue" - fmt.Fprintln(os.Stderr, console.FormatErrorMessage(errMsg)) return errors.New("status command timed out - this may indicate a configuration issue") } @@ -266,18 +262,13 @@ func validateMCPServerConfiguration(cmdPath string) error { // If the command failed, provide helpful error message if cmdPath != "" { - errMsg := fmt.Sprintf("failed to run status command with custom command '%s': %v\nOutput: %s\n\nPlease ensure:\n - The command path is correct and executable\n - You are in a git repository with .github/workflows directory", cmdPath, err, string(output)) - fmt.Fprintln(os.Stderr, console.FormatErrorMessage(errMsg)) return fmt.Errorf("failed to run status command with custom command '%s': %w\nOutput: %s\n\nPlease ensure:\n - The command path is correct and executable\n - You are in a git repository with .github/workflows directory", cmdPath, err, string(output)) } - errMsg := fmt.Sprintf("failed to run status command: %v\nOutput: %s\n\nPlease ensure:\n - gh CLI is installed and in PATH\n - gh aw extension is installed (run: gh extension install github/gh-aw)\n - You are in a git repository with .github/workflows directory", err, string(output)) - fmt.Fprintln(os.Stderr, console.FormatErrorMessage(errMsg)) return fmt.Errorf("failed to run status command: %w\nOutput: %s\n\nPlease ensure:\n - gh CLI is installed and in PATH\n - gh aw extension is installed (run: gh extension install github/gh-aw)\n - You are in a git repository with .github/workflows directory", err, string(output)) } // Status command succeeded - configuration is valid mcpValidationLog.Print("MCP server configuration validated successfully") - fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("✅ Configuration validated successfully")) return nil }