From 7ab1c179e1644db3ee4e4c7e1a70837cc732ae1b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 17 Apr 2026 17:20:58 +0000 Subject: [PATCH 01/11] Initial plan From 18d15f3f4269c73720160f343c5421dabeffb10f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 17 Apr 2026 18:38:41 +0000 Subject: [PATCH 02/11] test: add integration coverage for mcp-server stdio stream cleanliness Agent-Logs-Url: https://github.com/github/gh-aw/sessions/c706a8e3-1a8b-422e-90e9-b564187e75fe Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/mcp_server_stdio_integration_test.go | 70 ++++++++++++++++++++ 1 file changed, 70 insertions(+) create mode 100644 pkg/cli/mcp_server_stdio_integration_test.go 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 00000000000..c3983d86154 --- /dev/null +++ b/pkg/cli/mcp_server_stdio_integration_test.go @@ -0,0 +1,70 @@ +//go:build integration + +package cli + +import ( + "bytes" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "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) + cmd := exec.Command(absBinaryPath, "mcp-server", "--cmd", absBinaryPath) + cmd.Dir = tmpDir + cmd.Stdin = strings.NewReader("") + cmd.Env = append(os.Environ(), "GITHUB_ACTOR=") + + var stdout bytes.Buffer + var stderr bytes.Buffer + cmd.Stdout = &stdout + cmd.Stderr = &stderr + + _ = cmd.Run() + + stdoutText := strings.TrimSpace(stdout.String()) + if stdoutText != "" { + t.Fatalf("Expected stdout to remain clean for JSON-RPC, got: %q", stdoutText) + } + + stderrText := strings.TrimSpace(stderr.String()) + if stderrText == "" { + t.Fatal("Expected MCP diagnostics on stderr, got empty output") + } +} From c6c3c3ffcbab997b59210c3a4934d982686a12bd Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 17 Apr 2026 19:03:54 +0000 Subject: [PATCH 03/11] test: harden mcp stdio integration test execution Agent-Logs-Url: https://github.com/github/gh-aw/sessions/ced7bfd5-256b-47a5-b224-452c6cf309eb Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/mcp_server_stdio_integration_test.go | 31 +++++++++++++++----- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/pkg/cli/mcp_server_stdio_integration_test.go b/pkg/cli/mcp_server_stdio_integration_test.go index c3983d86154..7989fa44269 100644 --- a/pkg/cli/mcp_server_stdio_integration_test.go +++ b/pkg/cli/mcp_server_stdio_integration_test.go @@ -4,11 +4,14 @@ package cli import ( "bytes" + "context" + "errors" "os" "os/exec" "path/filepath" "strings" "testing" + "time" "github.com/github/gh-aw/pkg/testutil" ) @@ -46,25 +49,39 @@ engine: copilot } absBinaryPath := filepath.Join(originalDir, binaryPath) - cmd := exec.Command(absBinaryPath, "mcp-server", "--cmd", absBinaryPath) + 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("") - cmd.Env = append(os.Environ(), "GITHUB_ACTOR=") + + 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 - _ = cmd.Run() + 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) } - stderrText := strings.TrimSpace(stderr.String()) - if stderrText == "" { - t.Fatal("Expected MCP diagnostics on stderr, got empty output") - } + _ = strings.TrimSpace(stderr.String()) } From cdd16dccc850a0e5a7aa5bdda8efeee20aa4638e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 17 Apr 2026 19:04:52 +0000 Subject: [PATCH 04/11] test: keep stderr validation while avoiding stdio hang risk Agent-Logs-Url: https://github.com/github/gh-aw/sessions/ced7bfd5-256b-47a5-b224-452c6cf309eb Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/mcp_server_stdio_integration_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/cli/mcp_server_stdio_integration_test.go b/pkg/cli/mcp_server_stdio_integration_test.go index 7989fa44269..2d96addd14a 100644 --- a/pkg/cli/mcp_server_stdio_integration_test.go +++ b/pkg/cli/mcp_server_stdio_integration_test.go @@ -83,5 +83,8 @@ engine: copilot t.Fatalf("Expected stdout to remain clean for JSON-RPC, got: %q", stdoutText) } - _ = strings.TrimSpace(stderr.String()) + stderrText := strings.TrimSpace(stderr.String()) + if !errors.Is(ctx.Err(), context.DeadlineExceeded) && stderrText == "" { + t.Fatal("Expected MCP server diagnostics or shutdown details on stderr") + } } From 169717f615ddbd26e429f4c2ce132d85e12bf9f9 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 17 Apr 2026 19:39:41 +0000 Subject: [PATCH 05/11] fix: silence stdio diagnostics for mcp-server startup Agent-Logs-Url: https://github.com/github/gh-aw/sessions/7f800e19-520a-428f-b3d0-f2579decdb10 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/mcp_server_command.go | 56 ++++++++++++++------ pkg/cli/mcp_server_stdio_integration_test.go | 5 +- pkg/cli/mcp_validation.go | 39 ++++++++++---- 3 files changed, 69 insertions(+), 31 deletions(-) diff --git a/pkg/cli/mcp_server_command.go b/pkg/cli/mcp_server_command.go index 9ff133b1749..13fc6f21634 100644 --- a/pkg/cli/mcp_server_command.go +++ b/pkg/cli/mcp_server_command.go @@ -3,6 +3,7 @@ package cli import ( "context" "fmt" + "io" "os" "strings" @@ -74,13 +75,15 @@ Examples: } // checkAndLogGHVersion checks if gh CLI is available and logs its version -func checkAndLogGHVersion() { +func checkAndLogGHVersion(diagnosticOutput io.Writer) { 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")) + if diagnosticOutput != nil { + fmt.Fprintln(diagnosticOutput, console.FormatWarningMessage("gh CLI not found in PATH - some MCP server operations may fail")) + } return } @@ -90,28 +93,41 @@ func checkAndLogGHVersion() { // 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)) + if diagnosticOutput != nil { + fmt.Fprintln(diagnosticOutput, console.FormatInfoMessage("gh CLI: "+firstLine)) + } } // runMCPServer starts the MCP server on stdio or HTTP transport func runMCPServer(port int, cmdPath string, validateActor bool) error { + var diagnosticOutput io.Writer + if port > 0 { + diagnosticOutput = os.Stderr + } + // Get actor from environment variable actor := os.Getenv("GITHUB_ACTOR") if validateActor { mcpLog.Printf("Actor validation enabled (--validate-actor flag)") - fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Actor validation enabled")) + if diagnosticOutput != nil { + fmt.Fprintln(diagnosticOutput, console.FormatInfoMessage("Actor validation enabled")) + } } if actor != "" { mcpLog.Printf("Using actor: %s", actor) - fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Actor: "+actor)) + if diagnosticOutput != nil { + fmt.Fprintln(diagnosticOutput, 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 diagnosticOutput != nil { + if validateActor { + fmt.Fprintln(diagnosticOutput, console.FormatWarningMessage("No actor specified - logs and audit tools will not be mounted (actor validation enabled)")) + } else { + fmt.Fprintln(diagnosticOutput, console.FormatWarningMessage("No actor specified - all tools will be mounted (actor validation disabled)")) + } } } @@ -126,7 +142,7 @@ func runMCPServer(port int, cmdPath string, validateActor bool) error { if cmdPath == "" { // Attempt to detect the binary path and assign it to cmdPath // This ensures createMCPServer receives the actual binary path instead of falling back to "gh aw" - detectedPath, err := logAndValidateBinaryPath() + detectedPath, err := logAndValidateBinaryPath(diagnosticOutput) if err == nil && detectedPath != "" { cmdPath = detectedPath mcpLog.Printf("Using detected binary path: %s", cmdPath) @@ -136,21 +152,27 @@ 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)) + if diagnosticOutput != nil { + fmt.Fprintln(diagnosticOutput, 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))) + if diagnosticOutput != nil { + fmt.Fprintln(diagnosticOutput, console.FormatWarningMessage(fmt.Sprintf("Failed to get current working directory: %v", err))) + } } // Check and log gh CLI version - checkAndLogGHVersion() + checkAndLogGHVersion(diagnosticOutput) // Validate that the CLI and secrets are properly configured // Note: Validation failures are logged as warnings but don't prevent server startup // This allows the server to start in test environments or non-repository directories - if err := validateMCPServerConfiguration(cmdPath); err != nil { + if err := validateMCPServerConfiguration(cmdPath, diagnosticOutput); err != nil { mcpLog.Printf("Configuration validation warning: %v", err) - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Configuration validation warning: %v", err))) + if diagnosticOutput != nil { + fmt.Fprintln(diagnosticOutput, 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 +187,9 @@ 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)))) + if diagnosticOutput != nil { + fmt.Fprintln(diagnosticOutput, 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 index 2d96addd14a..8cd596cd5c7 100644 --- a/pkg/cli/mcp_server_stdio_integration_test.go +++ b/pkg/cli/mcp_server_stdio_integration_test.go @@ -83,8 +83,5 @@ engine: copilot t.Fatalf("Expected stdout to remain clean for JSON-RPC, got: %q", stdoutText) } - stderrText := strings.TrimSpace(stderr.String()) - if !errors.Is(ctx.Err(), context.DeadlineExceeded) && stderrText == "" { - t.Fatal("Expected MCP server diagnostics or shutdown details on stderr") - } + _ = stderr.String() } diff --git a/pkg/cli/mcp_validation.go b/pkg/cli/mcp_validation.go index 32948684f3e..e690514e61a 100644 --- a/pkg/cli/mcp_validation.go +++ b/pkg/cli/mcp_validation.go @@ -7,6 +7,7 @@ import ( "context" "errors" "fmt" + "io" "os" "os/exec" "path/filepath" @@ -50,11 +51,13 @@ 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. -func logAndValidateBinaryPath() (string, error) { +func logAndValidateBinaryPath(diagnosticOutput io.Writer) (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))) + if diagnosticOutput != nil { + fmt.Fprintln(diagnosticOutput, console.FormatWarningMessage(fmt.Sprintf("Warning: failed to get binary path: %v", err))) + } return "", err } @@ -62,17 +65,23 @@ 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)) + if diagnosticOutput != nil { + fmt.Fprintln(diagnosticOutput, 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))) + if diagnosticOutput != nil { + fmt.Fprintln(diagnosticOutput, 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)) + if diagnosticOutput != nil { + fmt.Fprintln(diagnosticOutput, console.FormatInfoMessage("gh-aw binary path: "+binaryPath)) + } return binaryPath, nil } @@ -223,7 +232,7 @@ func validateServerSecrets(config parser.MCPServerConfig, verbose bool, useActio // validateMCPServerConfiguration validates that the CLI is properly configured // by running the status command as a test -func validateMCPServerConfiguration(cmdPath string) error { +func validateMCPServerConfiguration(cmdPath string, diagnosticOutput io.Writer) error { mcpValidationLog.Printf("Validating MCP server configuration: cmdPath=%s", cmdPath) // Determine, log, and validate the binary path only if --cmd flag is not provided @@ -231,7 +240,7 @@ func validateMCPServerConfiguration(cmdPath string) error { if cmdPath == "" { // Attempt to detect the binary path and assign it to cmdPath // This ensures the validation uses the actual binary path instead of falling back to "gh aw" - detectedPath, err := logAndValidateBinaryPath() + detectedPath, err := logAndValidateBinaryPath(diagnosticOutput) if err == nil && detectedPath != "" { cmdPath = detectedPath } @@ -258,7 +267,9 @@ func validateMCPServerConfiguration(cmdPath string) error { 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)) + if diagnosticOutput != nil { + fmt.Fprintln(diagnosticOutput, console.FormatErrorMessage(errMsg)) + } return errors.New("status command timed out - this may indicate a configuration issue") } @@ -267,17 +278,23 @@ 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)) + if diagnosticOutput != nil { + fmt.Fprintln(diagnosticOutput, 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)) + if diagnosticOutput != nil { + fmt.Fprintln(diagnosticOutput, 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")) + if diagnosticOutput != nil { + fmt.Fprintln(diagnosticOutput, console.FormatSuccessMessage("✅ Configuration validated successfully")) + } return nil } From e636d8be38c051c8a7cfce119065a808591b78f9 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 17 Apr 2026 19:41:00 +0000 Subject: [PATCH 06/11] docs: clarify diagnostic output behavior in mcp helpers Agent-Logs-Url: https://github.com/github/gh-aw/sessions/7f800e19-520a-428f-b3d0-f2579decdb10 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/mcp_server_command.go | 3 ++- pkg/cli/mcp_validation.go | 4 +++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/pkg/cli/mcp_server_command.go b/pkg/cli/mcp_server_command.go index 13fc6f21634..4823da787df 100644 --- a/pkg/cli/mcp_server_command.go +++ b/pkg/cli/mcp_server_command.go @@ -74,7 +74,8 @@ 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. +// If diagnosticOutput is nil, diagnostic messages are suppressed. func checkAndLogGHVersion(diagnosticOutput io.Writer) { cmd := workflow.ExecGH("version") output, err := cmd.CombinedOutput() diff --git a/pkg/cli/mcp_validation.go b/pkg/cli/mcp_validation.go index e690514e61a..b33cbb0acc5 100644 --- a/pkg/cli/mcp_validation.go +++ b/pkg/cli/mcp_validation.go @@ -51,6 +51,7 @@ 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. +// If diagnosticOutput is nil, diagnostic messages are suppressed. func logAndValidateBinaryPath(diagnosticOutput io.Writer) (string, error) { binaryPath, err := GetBinaryPath() if err != nil { @@ -231,7 +232,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. +// If diagnosticOutput is nil, diagnostic messages are suppressed. func validateMCPServerConfiguration(cmdPath string, diagnosticOutput io.Writer) error { mcpValidationLog.Printf("Validating MCP server configuration: cmdPath=%s", cmdPath) From 3d30f1b4c4b83b2df87285327e1347ebf098a215 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 17 Apr 2026 19:42:14 +0000 Subject: [PATCH 07/11] test: finalize stdio stream-safety adjustments Agent-Logs-Url: https://github.com/github/gh-aw/sessions/7f800e19-520a-428f-b3d0-f2579decdb10 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/mcp_server_command.go | 2 ++ pkg/cli/mcp_server_stdio_integration_test.go | 1 - 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/cli/mcp_server_command.go b/pkg/cli/mcp_server_command.go index 4823da787df..8eecd00e176 100644 --- a/pkg/cli/mcp_server_command.go +++ b/pkg/cli/mcp_server_command.go @@ -102,6 +102,8 @@ func checkAndLogGHVersion(diagnosticOutput io.Writer) { // runMCPServer starts the MCP server on stdio or HTTP transport func runMCPServer(port int, cmdPath string, validateActor bool) error { var diagnosticOutput io.Writer + // Only emit startup diagnostics in HTTP mode. + // In stdio mode, any non-JSON output risks polluting the JSON-RPC stream. if port > 0 { diagnosticOutput = os.Stderr } diff --git a/pkg/cli/mcp_server_stdio_integration_test.go b/pkg/cli/mcp_server_stdio_integration_test.go index 8cd596cd5c7..c01d4ff4891 100644 --- a/pkg/cli/mcp_server_stdio_integration_test.go +++ b/pkg/cli/mcp_server_stdio_integration_test.go @@ -83,5 +83,4 @@ engine: copilot t.Fatalf("Expected stdout to remain clean for JSON-RPC, got: %q", stdoutText) } - _ = stderr.String() } From fd5f56ec181e78bd55f382d1a155e23e998530c6 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 17 Apr 2026 19:43:28 +0000 Subject: [PATCH 08/11] docs: clarify stdio diagnostic suppression semantics Agent-Logs-Url: https://github.com/github/gh-aw/sessions/7f800e19-520a-428f-b3d0-f2579decdb10 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/mcp_server_command.go | 7 ++++--- pkg/cli/mcp_validation.go | 4 ++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/pkg/cli/mcp_server_command.go b/pkg/cli/mcp_server_command.go index 8eecd00e176..437fbfee6ee 100644 --- a/pkg/cli/mcp_server_command.go +++ b/pkg/cli/mcp_server_command.go @@ -75,7 +75,7 @@ Examples: } // checkAndLogGHVersion checks if gh CLI is available and logs its version. -// If diagnosticOutput is nil, diagnostic messages are suppressed. +// Pass nil for diagnosticOutput in stdio mode to avoid contaminating the JSON-RPC stream. func checkAndLogGHVersion(diagnosticOutput io.Writer) { cmd := workflow.ExecGH("version") output, err := cmd.CombinedOutput() @@ -102,8 +102,9 @@ func checkAndLogGHVersion(diagnosticOutput io.Writer) { // runMCPServer starts the MCP server on stdio or HTTP transport func runMCPServer(port int, cmdPath string, validateActor bool) error { var diagnosticOutput io.Writer - // Only emit startup diagnostics in HTTP mode. - // In stdio mode, any non-JSON output risks polluting the JSON-RPC stream. + // port > 0 indicates HTTP mode; port == 0 indicates stdio mode. + // Only emit startup diagnostics in HTTP mode, because in stdio mode any non-JSON + // output risks polluting the JSON-RPC stream. if port > 0 { diagnosticOutput = os.Stderr } diff --git a/pkg/cli/mcp_validation.go b/pkg/cli/mcp_validation.go index b33cbb0acc5..33ef422f8f6 100644 --- a/pkg/cli/mcp_validation.go +++ b/pkg/cli/mcp_validation.go @@ -51,7 +51,7 @@ 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. -// If diagnosticOutput is nil, diagnostic messages are suppressed. +// Pass nil for diagnosticOutput in stdio mode to avoid contaminating the JSON-RPC stream. func logAndValidateBinaryPath(diagnosticOutput io.Writer) (string, error) { binaryPath, err := GetBinaryPath() if err != nil { @@ -233,7 +233,7 @@ func validateServerSecrets(config parser.MCPServerConfig, verbose bool, useActio // validateMCPServerConfiguration validates that the CLI is properly configured // by running the status command as a test. -// If diagnosticOutput is nil, diagnostic messages are suppressed. +// Pass nil for diagnosticOutput in stdio mode to avoid contaminating the JSON-RPC stream. func validateMCPServerConfiguration(cmdPath string, diagnosticOutput io.Writer) error { mcpValidationLog.Printf("Validating MCP server configuration: cmdPath=%s", cmdPath) From 1923df061f5edacc48c2f01d4427348d9a68db7e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 17 Apr 2026 19:44:43 +0000 Subject: [PATCH 09/11] docs: document diagnostic stream behavior for mcp modes Agent-Logs-Url: https://github.com/github/gh-aw/sessions/7f800e19-520a-428f-b3d0-f2579decdb10 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/mcp_server_command.go | 3 ++- pkg/cli/mcp_validation.go | 6 ++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/pkg/cli/mcp_server_command.go b/pkg/cli/mcp_server_command.go index 437fbfee6ee..ebca4542287 100644 --- a/pkg/cli/mcp_server_command.go +++ b/pkg/cli/mcp_server_command.go @@ -75,7 +75,8 @@ Examples: } // checkAndLogGHVersion checks if gh CLI is available and logs its version. -// Pass nil for diagnosticOutput in stdio mode to avoid contaminating the JSON-RPC stream. +// Pass os.Stderr for diagnosticOutput in HTTP mode for visible diagnostics. +// Pass nil in stdio mode to avoid contaminating the JSON-RPC stream. func checkAndLogGHVersion(diagnosticOutput io.Writer) { cmd := workflow.ExecGH("version") output, err := cmd.CombinedOutput() diff --git a/pkg/cli/mcp_validation.go b/pkg/cli/mcp_validation.go index 33ef422f8f6..b1d3e32d2ab 100644 --- a/pkg/cli/mcp_validation.go +++ b/pkg/cli/mcp_validation.go @@ -51,7 +51,8 @@ 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. -// Pass nil for diagnosticOutput in stdio mode to avoid contaminating the JSON-RPC stream. +// Pass os.Stderr for diagnosticOutput in HTTP mode for visible diagnostics. +// Pass nil in stdio mode to avoid contaminating the JSON-RPC stream. func logAndValidateBinaryPath(diagnosticOutput io.Writer) (string, error) { binaryPath, err := GetBinaryPath() if err != nil { @@ -233,7 +234,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. -// Pass nil for diagnosticOutput in stdio mode to avoid contaminating the JSON-RPC stream. +// Pass os.Stderr for diagnosticOutput in HTTP mode for visible diagnostics. +// Pass nil in stdio mode to avoid contaminating the JSON-RPC stream. func validateMCPServerConfiguration(cmdPath string, diagnosticOutput io.Writer) error { mcpValidationLog.Printf("Validating MCP server configuration: cmdPath=%s", cmdPath) From a8345cc541d87aa34e81eb4caaba8f3debd5dba5 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 17 Apr 2026 21:53:22 +0000 Subject: [PATCH 10/11] refactor: use logger for mcp startup diagnostics Agent-Logs-Url: https://github.com/github/gh-aw/sessions/699fb236-6542-4357-9393-fd8bf353a63e Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/mcp_server_command.go | 55 +++-------------------------------- pkg/cli/mcp_validation.go | 38 ++---------------------- 2 files changed, 7 insertions(+), 86 deletions(-) diff --git a/pkg/cli/mcp_server_command.go b/pkg/cli/mcp_server_command.go index ebca4542287..d12dd9bd707 100644 --- a/pkg/cli/mcp_server_command.go +++ b/pkg/cli/mcp_server_command.go @@ -2,12 +2,9 @@ package cli import ( "context" - "fmt" - "io" "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" @@ -75,65 +72,33 @@ Examples: } // checkAndLogGHVersion checks if gh CLI is available and logs its version. -// Pass os.Stderr for diagnosticOutput in HTTP mode for visible diagnostics. -// Pass nil in stdio mode to avoid contaminating the JSON-RPC stream. -func checkAndLogGHVersion(diagnosticOutput io.Writer) { +func checkAndLogGHVersion() { cmd := workflow.ExecGH("version") output, err := cmd.CombinedOutput() if err != nil { mcpLog.Print("WARNING: gh CLI not found in PATH") - if diagnosticOutput != nil { - fmt.Fprintln(diagnosticOutput, 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] - if diagnosticOutput != nil { - fmt.Fprintln(diagnosticOutput, console.FormatInfoMessage("gh CLI: "+firstLine)) - } } // runMCPServer starts the MCP server on stdio or HTTP transport func runMCPServer(port int, cmdPath string, validateActor bool) error { - var diagnosticOutput io.Writer - // port > 0 indicates HTTP mode; port == 0 indicates stdio mode. - // Only emit startup diagnostics in HTTP mode, because in stdio mode any non-JSON - // output risks polluting the JSON-RPC stream. - if port > 0 { - diagnosticOutput = os.Stderr - } - // Get actor from environment variable actor := os.Getenv("GITHUB_ACTOR") if validateActor { mcpLog.Printf("Actor validation enabled (--validate-actor flag)") - if diagnosticOutput != nil { - fmt.Fprintln(diagnosticOutput, console.FormatInfoMessage("Actor validation enabled")) - } } if actor != "" { mcpLog.Printf("Using actor: %s", actor) - if diagnosticOutput != nil { - fmt.Fprintln(diagnosticOutput, console.FormatInfoMessage("Actor: "+actor)) - } } else { mcpLog.Print("No actor specified (GITHUB_ACTOR environment variable)") - if diagnosticOutput != nil { - if validateActor { - fmt.Fprintln(diagnosticOutput, console.FormatWarningMessage("No actor specified - logs and audit tools will not be mounted (actor validation enabled)")) - } else { - fmt.Fprintln(diagnosticOutput, console.FormatWarningMessage("No actor specified - all tools will be mounted (actor validation disabled)")) - } - } } if port > 0 { @@ -147,7 +112,7 @@ func runMCPServer(port int, cmdPath string, validateActor bool) error { if cmdPath == "" { // Attempt to detect the binary path and assign it to cmdPath // This ensures createMCPServer receives the actual binary path instead of falling back to "gh aw" - detectedPath, err := logAndValidateBinaryPath(diagnosticOutput) + detectedPath, err := logAndValidateBinaryPath() if err == nil && detectedPath != "" { cmdPath = detectedPath mcpLog.Printf("Using detected binary path: %s", cmdPath) @@ -157,27 +122,18 @@ 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) - if diagnosticOutput != nil { - fmt.Fprintln(diagnosticOutput, console.FormatInfoMessage("Current working directory: "+cwd)) - } } else { mcpLog.Printf("WARNING: Failed to get current working directory: %v", err) - if diagnosticOutput != nil { - fmt.Fprintln(diagnosticOutput, console.FormatWarningMessage(fmt.Sprintf("Failed to get current working directory: %v", err))) - } } // Check and log gh CLI version - checkAndLogGHVersion(diagnosticOutput) + checkAndLogGHVersion() // Validate that the CLI and secrets are properly configured // Note: Validation failures are logged as warnings but don't prevent server startup // This allows the server to start in test environments or non-repository directories - if err := validateMCPServerConfiguration(cmdPath, diagnosticOutput); err != nil { + if err := validateMCPServerConfiguration(cmdPath); err != nil { mcpLog.Printf("Configuration validation warning: %v", err) - if diagnosticOutput != nil { - fmt.Fprintln(diagnosticOutput, console.FormatWarningMessage(fmt.Sprintf("Configuration validation warning: %v", err))) - } } // Pre-cache lock-file manifests at startup, before any agent can modify the working tree. @@ -192,9 +148,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)) - if diagnosticOutput != nil { - fmt.Fprintln(diagnosticOutput, 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_validation.go b/pkg/cli/mcp_validation.go index b1d3e32d2ab..00eca715f4c 100644 --- a/pkg/cli/mcp_validation.go +++ b/pkg/cli/mcp_validation.go @@ -7,7 +7,6 @@ import ( "context" "errors" "fmt" - "io" "os" "os/exec" "path/filepath" @@ -51,15 +50,10 @@ 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. -// Pass os.Stderr for diagnosticOutput in HTTP mode for visible diagnostics. -// Pass nil in stdio mode to avoid contaminating the JSON-RPC stream. -func logAndValidateBinaryPath(diagnosticOutput io.Writer) (string, error) { +func logAndValidateBinaryPath() (string, error) { binaryPath, err := GetBinaryPath() if err != nil { mcpValidationLog.Printf("Warning: failed to get binary path: %v", err) - if diagnosticOutput != nil { - fmt.Fprintln(diagnosticOutput, console.FormatWarningMessage(fmt.Sprintf("Warning: failed to get binary path: %v", err))) - } return "", err } @@ -67,23 +61,14 @@ func logAndValidateBinaryPath(diagnosticOutput io.Writer) (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) - if diagnosticOutput != nil { - fmt.Fprintln(diagnosticOutput, 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) - if diagnosticOutput != nil { - fmt.Fprintln(diagnosticOutput, 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) - if diagnosticOutput != nil { - fmt.Fprintln(diagnosticOutput, console.FormatInfoMessage("gh-aw binary path: "+binaryPath)) - } return binaryPath, nil } @@ -234,9 +219,7 @@ func validateServerSecrets(config parser.MCPServerConfig, verbose bool, useActio // validateMCPServerConfiguration validates that the CLI is properly configured // by running the status command as a test. -// Pass os.Stderr for diagnosticOutput in HTTP mode for visible diagnostics. -// Pass nil in stdio mode to avoid contaminating the JSON-RPC stream. -func validateMCPServerConfiguration(cmdPath string, diagnosticOutput io.Writer) error { +func validateMCPServerConfiguration(cmdPath string) error { mcpValidationLog.Printf("Validating MCP server configuration: cmdPath=%s", cmdPath) // Determine, log, and validate the binary path only if --cmd flag is not provided @@ -244,7 +227,7 @@ func validateMCPServerConfiguration(cmdPath string, diagnosticOutput io.Writer) if cmdPath == "" { // Attempt to detect the binary path and assign it to cmdPath // This ensures the validation uses the actual binary path instead of falling back to "gh aw" - detectedPath, err := logAndValidateBinaryPath(diagnosticOutput) + detectedPath, err := logAndValidateBinaryPath() if err == nil && detectedPath != "" { cmdPath = detectedPath } @@ -270,10 +253,6 @@ func validateMCPServerConfiguration(cmdPath string, diagnosticOutput io.Writer) // 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" - if diagnosticOutput != nil { - fmt.Fprintln(diagnosticOutput, console.FormatErrorMessage(errMsg)) - } return errors.New("status command timed out - this may indicate a configuration issue") } @@ -281,24 +260,13 @@ func validateMCPServerConfiguration(cmdPath string, diagnosticOutput io.Writer) // 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)) - if diagnosticOutput != nil { - fmt.Fprintln(diagnosticOutput, 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)) - if diagnosticOutput != nil { - fmt.Fprintln(diagnosticOutput, 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") - if diagnosticOutput != nil { - fmt.Fprintln(diagnosticOutput, console.FormatSuccessMessage("✅ Configuration validated successfully")) - } return nil } From be474a25992684f5bf6d27002b3e39b9793f864b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 17 Apr 2026 22:05:59 +0000 Subject: [PATCH 11/11] docs: clarify logger-only diagnostics in mcp helpers Agent-Logs-Url: https://github.com/github/gh-aw/sessions/699fb236-6542-4357-9393-fd8bf353a63e Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/mcp_server_command.go | 1 + pkg/cli/mcp_validation.go | 2 ++ 2 files changed, 3 insertions(+) diff --git a/pkg/cli/mcp_server_command.go b/pkg/cli/mcp_server_command.go index d12dd9bd707..533b627b230 100644 --- a/pkg/cli/mcp_server_command.go +++ b/pkg/cli/mcp_server_command.go @@ -72,6 +72,7 @@ Examples: } // 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() diff --git a/pkg/cli/mcp_validation.go b/pkg/cli/mcp_validation.go index 00eca715f4c..fdd57d90f87 100644 --- a/pkg/cli/mcp_validation.go +++ b/pkg/cli/mcp_validation.go @@ -50,6 +50,7 @@ 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 { @@ -219,6 +220,7 @@ func validateServerSecrets(config parser.MCPServerConfig, verbose bool, useActio // validateMCPServerConfiguration validates that the CLI is properly configured // 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)