diff --git a/docs/adr/38544-cap-mcp-server-child-process-concurrency.md b/docs/adr/38544-cap-mcp-server-child-process-concurrency.md new file mode 100644 index 00000000000..fd4bf8b00f9 --- /dev/null +++ b/docs/adr/38544-cap-mcp-server-child-process-concurrency.md @@ -0,0 +1,40 @@ +# ADR-38544: Cap MCP Server Child Process Concurrency with a Shared Semaphore Guardrail + +**Date**: 2026-06-11 +**Status**: Accepted — the Go MCP server now enforces a shared 4-process subprocess cap. +**Deciders**: pelikhan, Copilot + +## Context + +The Go MCP server shells out to `gh` and `gh aw` child processes to service tool calls (workflow add/update/fix, logs, audit, diff, compile, inspect), as well as for supporting operations such as repository lookup, actor permission checks, config validation, and the `gh version` startup probe. Under concurrent tool usage these subprocess invocations were unbounded: every in-flight request that needed a child process spawned one immediately, so a burst of concurrent requests could fan out an arbitrary number of `gh`/`gh aw` processes and exhaust host resources (file descriptors, memory, process table). There was no central place enforcing a ceiling, and each call site managed its own `cmd.Output()` / `cmd.CombinedOutput()` independently. This change is preventive hardening: the issue was to add an explicit server-side guardrail before resource pressure showed up as flaky or host-specific failures. + +## Decision + +We will cap the number of simultaneously-active server-managed child processes at **4** using a single shared, context-aware guardrail. The guardrail (`mcpSubprocessGuardrail`) is a buffered-channel semaphore: each subprocess acquires one slot before executing and releases it when done. Acquisition is context-aware, so a cancelled request stops waiting rather than blocking behind queued subprocesses. All subprocess call sites route through the shared `defaultMCPSubprocessGuardrail` via the helpers `runMCPSubprocessOutput`, `runMCPSubprocessCombinedOutput`, `runMCPExecOutput`, and `runMCPExecCombinedOutput`, replacing direct `cmd.Output()`/`cmd.CombinedOutput()` calls. Output contracts and tool behavior are unchanged; only concurrency is bounded. + +## Alternatives Considered + +### Alternative 1: Leave subprocess spawning unbounded +Keep relying on the operating system and `gh`'s own behavior to absorb concurrent load. Rejected because the failure mode (resource exhaustion under a burst of concurrent tool calls) is silent and hard to diagnose, and the server has no backpressure mechanism of its own once limits are hit. + +### Alternative 2: Per-call-site or per-tool limits +Give each tool or call site its own concurrency limit rather than one shared cap. Rejected because the resource pressure is global — total live child processes is what matters — so independent per-tool counters could still sum well past a safe ceiling, and would duplicate limiting logic across many files. + +### Alternative 3: Explicit worker pool / job queue +Introduce a fixed pool of worker goroutines that own subprocess execution, with requests submitting jobs to a queue. Rejected as heavier than needed: it changes the execution model and call-site ergonomics, whereas a buffered-channel semaphore achieves the same bound with a minimal, drop-in wrapper around existing `exec.Cmd` calls. + +## Consequences + +### Positive +- Total concurrent server-managed child processes is bounded at 4, preventing unbounded fan-out and the associated resource exhaustion. +- A single chokepoint (`defaultMCPSubprocessGuardrail`) centralizes the limit; future call sites that use the helpers are covered automatically. +- Slot acquisition respects `context` cancellation, so cancelled or timed-out requests do not hang waiting for a slot. + +### Negative +- The limit `4` is a hardcoded constant (`maxActiveMCPChildProcesses`) and is not configurable per host or deployment; tuning requires a code change. That is intentional for this first guardrail because the goal is to add one deterministic ceiling without introducing new user-facing configuration surface. +- Under high concurrency, requests block while waiting for a free slot, which can increase tail latency for tool calls that previously ran immediately. +- Correctness depends on every subprocess call site using the guardrail helpers; a direct `cmd.Output()`/`cmd.CombinedOutput()` added later would silently bypass the cap. + +### Neutral +- The guardrail is process-global state (a package-level `defaultMCPSubprocessGuardrail`), shared across all MCP requests in the server process. +- Existing stdout/stderr separation (using `Output()` vs `CombinedOutput()` for JSON-producing commands) is preserved through distinct helper variants. diff --git a/pkg/cli/mcp_helpers.go b/pkg/cli/mcp_helpers.go index 7b8e13bd1bb..64f68e22a50 100644 --- a/pkg/cli/mcp_helpers.go +++ b/pkg/cli/mcp_helpers.go @@ -7,7 +7,9 @@ package cli import ( "fmt" "os" + "os/exec" "path/filepath" + "strings" "github.com/github/gh-aw/pkg/logger" ) @@ -67,3 +69,27 @@ func logAndValidateBinaryPath() (string, error) { mcpHelpersLog.Printf("gh-aw binary path: %s", binaryPath) return binaryPath, nil } + +func withNonInteractiveCIEnv(env []string) []string { + if env == nil { + env = os.Environ() + } + + env = append([]string(nil), env...) + for i, entry := range env { + if strings.HasPrefix(entry, "CI=") { + env[i] = "CI=1" + return env + } + } + + return append(env, "CI=1") +} + +func setNonInteractiveCIEnv(cmd *exec.Cmd) { + if cmd == nil { + return + } + + cmd.Env = withNonInteractiveCIEnv(cmd.Env) +} diff --git a/pkg/cli/mcp_permissions.go b/pkg/cli/mcp_permissions.go index 93222341968..a1b3dfbe0bc 100644 --- a/pkg/cli/mcp_permissions.go +++ b/pkg/cli/mcp_permissions.go @@ -34,7 +34,7 @@ func queryActorRole(ctx context.Context, actor string, repo string) (string, err mcpLog.Printf("Querying GitHub API for %s's permission in %s", actor, repo) cmd := workflow.ExecGHContext(ctx, "api", apiPath, "--jq", ".permission") - output, err := cmd.Output() + output, err := runMCPSubprocessOutput(ctx, cmd) if err != nil { mcpLog.Printf("Failed to query actor permission: %v", err) return "", fmt.Errorf("failed to query actor permission: %w", err) @@ -83,7 +83,7 @@ func checkActorPermission(ctx context.Context, actor string, validateActor bool, } // Get repository using cached lookup - repo, err := getRepository() + repo, err := getRepository(ctx) if err != nil { mcpLog.Printf("Tool %s: failed to get repository context, denying access: %v", toolName, err) return newMCPError(jsonrpc.CodeInternalError, "permission check failed", map[string]any{ diff --git a/pkg/cli/mcp_repository.go b/pkg/cli/mcp_repository.go index 79812ceb99e..aacc3a8eb63 100644 --- a/pkg/cli/mcp_repository.go +++ b/pkg/cli/mcp_repository.go @@ -1,6 +1,7 @@ package cli import ( + "context" "errors" "fmt" "os" @@ -12,7 +13,7 @@ import ( // getRepository retrieves the current repository name (owner/repo format). // Results are cached for 1 hour to avoid repeated queries. // Checks GITHUB_REPOSITORY environment variable first, then falls back to gh repo view. -func getRepository() (string, error) { +func getRepository(ctx context.Context) (string, error) { // Check cache first if repo, ok := mcpCache.GetRepo(); ok { mcpLog.Printf("Using cached repository: %s", repo) @@ -29,8 +30,8 @@ func getRepository() (string, error) { // Fall back to gh repo view mcpLog.Print("Querying repository using gh repo view") - cmd := workflow.ExecGH("repo", "view", "--json", "nameWithOwner", "--jq", ".nameWithOwner") - output, err := cmd.Output() + cmd := workflow.ExecGHContext(ctx, "repo", "view", "--json", "nameWithOwner", "--jq", ".nameWithOwner") + output, err := runMCPSubprocessOutput(ctx, cmd) if err != nil { mcpLog.Printf("Failed to get repository: %v", err) return "", fmt.Errorf("failed to get repository: %w", err) diff --git a/pkg/cli/mcp_server.go b/pkg/cli/mcp_server.go index 090d1264c60..ff1409cc31d 100644 --- a/pkg/cli/mcp_server.go +++ b/pkg/cli/mcp_server.go @@ -16,15 +16,21 @@ type execCmdFunc func(ctx context.Context, args ...string) *exec.Cmd // The second return value is a reserved SDK extension slot and must be nil for now. // createMCPServer creates and configures the MCP server with all tools -func createMCPServer(cmdPath string, actor string, validateActor bool, manifestCacheFile string) *mcp.Server { +func createMCPServer(cmdPath string, actor string, validateActor bool, manifestCacheFile string, env []string) *mcp.Server { // Helper function to execute command with proper path execCmd := func(ctx context.Context, args ...string) *exec.Cmd { + var cmd *exec.Cmd if cmdPath != "" { // Use custom command path - return exec.CommandContext(ctx, cmdPath, args...) + cmd = exec.CommandContext(ctx, cmdPath, args...) + } else { + // Use default gh aw command with proper token handling + cmd = workflow.ExecGHContext(ctx, append([]string{"aw"}, args...)...) + } + if env != nil { + cmd.Env = append([]string(nil), env...) } - // Use default gh aw command with proper token handling - return workflow.ExecGHContext(ctx, append([]string{"aw"}, args...)...) + return cmd } // Log actor and validation settings diff --git a/pkg/cli/mcp_server_command.go b/pkg/cli/mcp_server_command.go index e28909fd21e..e805acc0c0a 100644 --- a/pkg/cli/mcp_server_command.go +++ b/pkg/cli/mcp_server_command.go @@ -3,10 +3,8 @@ package cli import ( "context" "os" - "strings" "github.com/github/gh-aw/pkg/logger" - "github.com/github/gh-aw/pkg/workflow" "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/spf13/cobra" ) @@ -60,7 +58,7 @@ Examples: GITHUB_ACTOR=octocat gh aw mcp-server # Set actor via environment variable for access control DEBUG=mcp:* GITHUB_ACTOR=octocat gh aw mcp-server # Run with verbose debug logging and actor set via environment variable`, RunE: func(cmd *cobra.Command, args []string) error { - return runMCPServer(port, cmdPath, validateActor) + return runMCPServer(cmd.Context(), port, cmdPath, validateActor) }, } @@ -71,24 +69,10 @@ Examples: return cmd } -// 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") - return - } - - // Parse and log the version - versionOutput := strings.TrimSpace(string(output)) - mcpLog.Printf("gh CLI version: %s", versionOutput) -} - // runMCPServer starts the MCP server on stdio or HTTP transport -func runMCPServer(port int, cmdPath string, validateActor bool) error { +func runMCPServer(ctx context.Context, port int, cmdPath string, validateActor bool) error { + mcpServerEnv := withNonInteractiveCIEnv(nil) + // Get actor from environment variable actor := os.Getenv("GITHUB_ACTOR") @@ -127,13 +111,10 @@ func runMCPServer(port int, cmdPath string, validateActor bool) error { mcpLog.Printf("WARNING: Failed to get current working directory: %v", err) } - // Check and log gh CLI version - 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); err != nil { + if err := validateMCPServerConfiguration(ctx, cmdPath, mcpServerEnv); err != nil { mcpLog.Printf("Configuration validation warning: %v", err) } @@ -159,7 +140,7 @@ func runMCPServer(port int, cmdPath string, validateActor bool) error { } // Create the server configuration - server := createMCPServer(cmdPath, actor, validateActor, manifestCacheFile) + server := createMCPServer(cmdPath, actor, validateActor, manifestCacheFile, mcpServerEnv) if port > 0 { // Run HTTP server with SSE transport @@ -168,5 +149,5 @@ func runMCPServer(port int, cmdPath string, validateActor bool) error { // Run stdio transport mcpLog.Print("MCP server ready on stdio") - return server.Run(context.Background(), &mcp.StdioTransport{}) + return server.Run(ctx, &mcp.StdioTransport{}) } diff --git a/pkg/cli/mcp_server_unit_test.go b/pkg/cli/mcp_server_unit_test.go index 6d596166bfd..48eaacb4bc1 100644 --- a/pkg/cli/mcp_server_unit_test.go +++ b/pkg/cli/mcp_server_unit_test.go @@ -19,7 +19,7 @@ import ( // TestMCPServerUnit_ListTools verifies that the MCP server exposes exactly the // expected set of tools without spawning a subprocess. func TestMCPServerUnit_ListTools(t *testing.T) { - server := createMCPServer("", "", false, "") + server := createMCPServer("", "", false, "", nil) session := connectInMemory(t, server) ctx := context.Background() @@ -43,7 +43,7 @@ func TestMCPServerUnit_ListTools(t *testing.T) { // TestMCPServerUnit_ServerCapabilities verifies that the server advertises the // Tools capability with ListChanged=false (tools are static, no notifications needed). func TestMCPServerUnit_ServerCapabilities(t *testing.T) { - server := createMCPServer("", "", false, "") + server := createMCPServer("", "", false, "", nil) session := connectInMemory(t, server) initResult := session.InitializeResult() @@ -66,7 +66,7 @@ func TestMCPServerUnit_StatusTool(t *testing.T) { require.NoError(t, os.Chdir(tmpDir), "should change to temp dir") t.Cleanup(func() { _ = os.Chdir(origDir) }) - server := createMCPServer("", "", false, "") + server := createMCPServer("", "", false, "", nil) session := connectInMemory(t, server) ctx := context.Background() diff --git a/pkg/cli/mcp_subprocess_guardrail.go b/pkg/cli/mcp_subprocess_guardrail.go new file mode 100644 index 00000000000..84d158b2b25 --- /dev/null +++ b/pkg/cli/mcp_subprocess_guardrail.go @@ -0,0 +1,81 @@ +package cli + +import ( + "context" + "os/exec" +) + +const maxActiveMCPChildProcesses = 4 + +type mcpSubprocessGuardrail struct { + slots chan struct{} +} + +var defaultMCPSubprocessGuardrail = newMCPSubprocessGuardrail(maxActiveMCPChildProcesses) + +func newMCPSubprocessGuardrail(limit int) *mcpSubprocessGuardrail { + return &mcpSubprocessGuardrail{ + slots: make(chan struct{}, limit), + } +} + +func (g *mcpSubprocessGuardrail) acquire(ctx context.Context) error { + if err := ctx.Err(); err != nil { + return err + } + + select { + case g.slots <- struct{}{}: + if err := ctx.Err(); err != nil { + g.release() + return err + } + return nil + case <-ctx.Done(): + return ctx.Err() + } +} + +func (g *mcpSubprocessGuardrail) release() { + <-g.slots +} + +func (g *mcpSubprocessGuardrail) output(ctx context.Context, cmd *exec.Cmd) ([]byte, error) { + if err := g.acquire(ctx); err != nil { + return nil, err + } + defer g.release() + + return cmd.Output() +} + +func (g *mcpSubprocessGuardrail) combinedOutput(ctx context.Context, cmd *exec.Cmd) ([]byte, error) { + if err := g.acquire(ctx); err != nil { + return nil, err + } + defer g.release() + + return cmd.CombinedOutput() +} + +// runMCPSubprocessOutput executes cmd under the shared MCP subprocess guardrail. +// ctx governs slot acquisition and any subprocess cancellation only when cmd was +// created with the same context (for example via exec.CommandContext or ExecGHContext). +func runMCPSubprocessOutput(ctx context.Context, cmd *exec.Cmd) ([]byte, error) { + return defaultMCPSubprocessGuardrail.output(ctx, cmd) +} + +// runMCPSubprocessCombinedOutput executes cmd under the shared MCP subprocess +// guardrail. ctx governs slot acquisition and any subprocess cancellation only +// when cmd was created with the same context. +func runMCPSubprocessCombinedOutput(ctx context.Context, cmd *exec.Cmd) ([]byte, error) { + return defaultMCPSubprocessGuardrail.combinedOutput(ctx, cmd) +} + +func runMCPExecOutput(ctx context.Context, execCmd execCmdFunc, args ...string) ([]byte, error) { + return runMCPSubprocessOutput(ctx, execCmd(ctx, args...)) +} + +func runMCPExecCombinedOutput(ctx context.Context, execCmd execCmdFunc, args ...string) ([]byte, error) { + return runMCPSubprocessCombinedOutput(ctx, execCmd(ctx, args...)) +} diff --git a/pkg/cli/mcp_subprocess_guardrail_test.go b/pkg/cli/mcp_subprocess_guardrail_test.go new file mode 100644 index 00000000000..396c4a4cd05 --- /dev/null +++ b/pkg/cli/mcp_subprocess_guardrail_test.go @@ -0,0 +1,125 @@ +//go:build !integration + +package cli + +import ( + "context" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestMCPSubprocessGuardrailLimitsConcurrentAcquisitions(t *testing.T) { + guardrail := newMCPSubprocessGuardrail(maxActiveMCPChildProcesses) + + var current atomic.Int32 + var maxObserved atomic.Int32 + var wg sync.WaitGroup + errCh := make(chan error, maxActiveMCPChildProcesses+2) + acquiredCh := make(chan struct{}, maxActiveMCPChildProcesses+2) + releaseCh := make(chan struct{}) + + for range maxActiveMCPChildProcesses + 2 { + wg.Go(func() { + if err := guardrail.acquire(context.Background()); err != nil { + errCh <- err + return + } + defer guardrail.release() + + active := current.Add(1) + defer current.Add(-1) + + for { + previousMax := maxObserved.Load() + if active <= previousMax || maxObserved.CompareAndSwap(previousMax, active) { + break + } + } + + acquiredCh <- struct{}{} + <-releaseCh + }) + } + + for range maxActiveMCPChildProcesses { + select { + case <-acquiredCh: + case err := <-errCh: + t.Fatalf("unexpected acquisition error: %v", err) + case <-time.After(time.Second): + t.Fatal("timed out waiting for initial guardrail acquisitions") + } + } + + select { + case <-acquiredCh: + t.Fatal("guardrail allowed more than the configured number of active subprocesses") + case err := <-errCh: + t.Fatalf("unexpected acquisition error: %v", err) + case <-time.After(100 * time.Millisecond): + } + + close(releaseCh) + wg.Wait() + close(errCh) + + for err := range errCh { + require.NoError(t, err, "goroutine should not receive an acquisition error") + } + + assert.Equal(t, int32(maxActiveMCPChildProcesses), maxObserved.Load(), "peak concurrent acquisitions should not exceed the guardrail limit") +} + +func TestMCPSubprocessGuardrailAcquireHonorsContextCancellation(t *testing.T) { + guardrail := newMCPSubprocessGuardrail(1) + require.NoError(t, guardrail.acquire(context.Background()), "initial acquire on empty guardrail should succeed") + defer guardrail.release() + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + err := guardrail.acquire(ctx) + require.ErrorIs(t, err, context.Canceled, "acquire with a cancelled context should return context.Canceled") +} + +func TestMCPSubprocessGuardrailAcquireHonorsCanceledContextWithAvailableSlot(t *testing.T) { + guardrail := newMCPSubprocessGuardrail(1) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + err := guardrail.acquire(ctx) + require.ErrorIs(t, err, context.Canceled, "acquire with a cancelled context should fail even when a slot is available") +} + +func TestMCPSubprocessGuardrailAcquireCancelsWhileBlocked(t *testing.T) { + guardrail := newMCPSubprocessGuardrail(1) + require.NoError(t, guardrail.acquire(context.Background()), "initial acquire on empty guardrail should succeed") + defer guardrail.release() + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + startedCh := make(chan struct{}) + errCh := make(chan error, 1) + go func() { + close(startedCh) + errCh <- guardrail.acquire(ctx) + }() + + <-startedCh + time.Sleep(10 * time.Millisecond) + cancel() + + select { + case err := <-errCh: + require.ErrorIs(t, err, context.Canceled, "blocked acquire should return context.Canceled after cancellation") + case <-time.After(time.Second): + t.Fatal("blocked acquire did not unblock after context cancellation") + } +} diff --git a/pkg/cli/mcp_tools_management.go b/pkg/cli/mcp_tools_management.go index 668b09b53b2..5f84b34c206 100644 --- a/pkg/cli/mcp_tools_management.go +++ b/pkg/cli/mcp_tools_management.go @@ -68,8 +68,7 @@ func registerAddTool(server *mcp.Server, execCmd execCmdFunc) { } // Execute the CLI command - cmd := execCmd(ctx, cmdArgs...) - output, err := cmd.CombinedOutput() + output, err := runMCPExecCombinedOutput(ctx, execCmd, cmdArgs...) if err != nil { return nil, nil, newMCPError(jsonrpc.CodeInternalError, "failed to add workflows", map[string]any{"error": err.Error(), "output": string(output)}) @@ -152,8 +151,7 @@ Returns formatted text output showing: } // Execute the CLI command - cmd := execCmd(ctx, cmdArgs...) - output, err := cmd.CombinedOutput() + output, err := runMCPExecCombinedOutput(ctx, execCmd, cmdArgs...) if err != nil { return nil, nil, newMCPError(jsonrpc.CodeInternalError, "failed to update workflows", map[string]any{"error": err.Error(), "output": string(output)}) @@ -234,8 +232,7 @@ Returns formatted text output showing: } // Execute the CLI command - cmd := execCmd(ctx, cmdArgs...) - output, err := cmd.CombinedOutput() + output, err := runMCPExecCombinedOutput(ctx, execCmd, cmdArgs...) if err != nil { return nil, nil, newMCPError(jsonrpc.CodeInternalError, "failed to fix workflows", map[string]any{"error": err.Error(), "output": string(output)}) diff --git a/pkg/cli/mcp_tools_privileged.go b/pkg/cli/mcp_tools_privileged.go index 053250cbf72..6009af0695d 100644 --- a/pkg/cli/mcp_tools_privileged.go +++ b/pkg/cli/mcp_tools_privileged.go @@ -193,8 +193,7 @@ from where the previous request stopped due to timeout.`, // Use separate stdout/stderr capture instead of CombinedOutput because: // - Stdout contains JSON output (--json flag) // - Stderr contains console messages and error details - cmd := execCmd(ctx, cmdArgs...) - stdout, err := cmd.Output() + stdout, err := runMCPExecOutput(ctx, execCmd, cmdArgs...) // The logs command outputs JSON to stdout when --json flag is used. // If the command fails, we need to provide detailed error information. @@ -404,8 +403,7 @@ Multi-run diff returns JSON describing changes between the base and each compari // Use separate stdout/stderr capture instead of CombinedOutput because: // - Stdout contains JSON output (--json flag) // - Stderr contains console messages and debug logs that shouldn't be mixed with JSON - cmd := execCmd(ctx, cmdArgs...) - stdout, err := cmd.Output() + stdout, err := runMCPExecOutput(ctx, execCmd, cmdArgs...) // The audit command outputs JSON to stdout when --json flag is used. // If the command fails, we need to provide detailed error information. @@ -536,8 +534,7 @@ Returns JSON describing the differences between the base run and each comparison notifyProgress(ctx, req, 0, 100, "Downloading artifacts for diff...") - cmd := execCmd(ctx, cmdArgs...) - stdout, err := cmd.Output() + stdout, err := runMCPExecOutput(ctx, execCmd, cmdArgs...) outputStr := string(stdout) if err != nil { diff --git a/pkg/cli/mcp_tools_readonly.go b/pkg/cli/mcp_tools_readonly.go index 62ac96b25c6..e17311c5219 100644 --- a/pkg/cli/mcp_tools_readonly.go +++ b/pkg/cli/mcp_tools_readonly.go @@ -221,8 +221,7 @@ Returns JSON array with validation results for each workflow: // Use separate stdout/stderr capture instead of CombinedOutput because: // - Stdout contains JSON output (--json flag) // - Stderr contains console messages that shouldn't be mixed with JSON - cmd := execCmd(ctx, cmdArgs...) - stdout, err := cmd.Output() + stdout, err := runMCPExecOutput(ctx, execCmd, cmdArgs...) // The compile command always outputs JSON to stdout when --json flag is used, even on error. // We should return the JSON output to the LLM so it can see validation errors. @@ -329,8 +328,7 @@ Returns formatted text output showing: cmdArgs = append(cmdArgs, "--check-secrets") // Execute the CLI command - cmd := execCmd(ctx, cmdArgs...) - output, err := cmd.CombinedOutput() + output, err := runMCPExecCombinedOutput(ctx, execCmd, cmdArgs...) if err != nil { return nil, nil, newMCPError(jsonrpc.CodeInternalError, "failed to inspect MCP servers", map[string]any{"error": err.Error(), "output": string(output)}) diff --git a/pkg/cli/mcp_validation.go b/pkg/cli/mcp_validation.go index 534d6a87615..25de3791d5e 100644 --- a/pkg/cli/mcp_validation.go +++ b/pkg/cli/mcp_validation.go @@ -170,7 +170,7 @@ func validateServerSecrets(config parser.RegistryMCPServerConfig, verbose bool, // 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 { +func validateMCPServerConfiguration(ctx context.Context, cmdPath string, env []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 @@ -185,7 +185,7 @@ func validateMCPServerConfiguration(cmdPath string) error { } // Try to run the status command to verify CLI is working - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + ctx, cancel := context.WithTimeout(ctx, 5*time.Second) defer cancel() var cmd *exec.Cmd @@ -198,7 +198,10 @@ func validateMCPServerConfiguration(cmdPath string) error { // Use default gh aw command with proper token handling cmd = workflow.ExecGHContext(ctx, "aw", "status") } - output, err := cmd.CombinedOutput() + if env != nil { + cmd.Env = append([]string(nil), env...) + } + output, err := runMCPSubprocessCombinedOutput(ctx, cmd) if err != nil { // Check for common error cases diff --git a/pkg/cli/mcp_validation_test.go b/pkg/cli/mcp_validation_test.go index e30daf724b1..4d789072717 100644 --- a/pkg/cli/mcp_validation_test.go +++ b/pkg/cli/mcp_validation_test.go @@ -4,6 +4,7 @@ package cli import ( "os" + "os/exec" "path/filepath" "testing" @@ -59,3 +60,37 @@ func TestGetBinaryPath(t *testing.T) { // If EvalSymlinks fails, that's OK - the original path is still valid }) } + +func TestSetNonInteractiveCIEnv(t *testing.T) { + t.Run("returns copied env with CI forced on", func(t *testing.T) { + input := []string{"CI=false", "HOME=/tmp/test-home"} + + output := withNonInteractiveCIEnv(input) + + assert.Equal(t, []string{"CI=false", "HOME=/tmp/test-home"}, input) + assert.Contains(t, output, "CI=1") + assert.NotContains(t, output, "CI=false") + assert.Contains(t, output, "HOME=/tmp/test-home") + }) + + t.Run("adds CI when missing", func(t *testing.T) { + cmd := exec.Command("echo") + cmd.Env = []string{"HOME=/tmp/test-home"} + + setNonInteractiveCIEnv(cmd) + + assert.Contains(t, cmd.Env, "CI=1") + assert.Contains(t, cmd.Env, "HOME=/tmp/test-home") + }) + + t.Run("overrides existing CI value", func(t *testing.T) { + cmd := exec.Command("echo") + cmd.Env = []string{"CI=false", "HOME=/tmp/test-home"} + + setNonInteractiveCIEnv(cmd) + + assert.Contains(t, cmd.Env, "CI=1") + assert.NotContains(t, cmd.Env, "CI=false") + assert.Contains(t, cmd.Env, "HOME=/tmp/test-home") + }) +}