From 12648f500209f98a9c96fdfb15edc56ba26f4eee Mon Sep 17 00:00:00 2001 From: Jahvon Dockery Date: Thu, 27 Aug 2026 00:22:14 -0400 Subject: [PATCH] fix(mcp): reply instead of hanging when a command exits non-zero MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Any MCP tool call whose flow subprocess exited non-zero left the client waiting on a request that never got a response, until it hit an idle timeout. The target passed to errors.As was exec.ExitError, a value. ExitError's Error method has a pointer receiver, so the value type does not implement error, and errors.As panics on such a target ("*target must be interface or implement error"). That panic killed the tool handler's goroutine before it could reply. Nothing about the failure was visible from outside: the subprocess had already run and recorded its terminal status in flow's history, and the server kept serving other requests normally — only the one request was lost. The one-character fix is *exec.ExitError, matching tests/utils/runner.go, which had it right. This also restores the behavior the comment always described: a non-zero exit is a normal outcome whose detail is already in the captured output, so it returns as output with a nil error. Affects every tool that shells out — execute, run_command, run_executable, get_execution_logs, sync_executables — for any failing command. The regression test drives the real FlowCLIExecutor against a stand-in binary via FLOW_CLI_BINARY; it fails with the original panic before the fix. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01R328pa3FUUfga4gYah1iQi --- internal/mcp/command_executor.go | 11 ++- .../mcp/command_executor_internal_test.go | 72 +++++++++++++++++++ 2 files changed, 81 insertions(+), 2 deletions(-) create mode 100644 internal/mcp/command_executor_internal_test.go diff --git a/internal/mcp/command_executor.go b/internal/mcp/command_executor.go index 59db1526..029f2fd9 100644 --- a/internal/mcp/command_executor.go +++ b/internal/mcp/command_executor.go @@ -134,8 +134,15 @@ func (c *FlowCLIExecutor) ExecuteContext(ctx context.Context, args ...string) (s } output, err := cmd.CombinedOutput() if err != nil { - // Only return an error if it's not an exit error. - var exitErr exec.ExitError + // Only return an error if it's not an exit error: a non-zero exit is a + // normal outcome whose detail is already in the captured output. + // + // The target must be *exec.ExitError, not exec.ExitError. ExitError's + // Error method has a pointer receiver, so the value type does not + // implement error and errors.As panics on it - which killed the tool + // handler's goroutine before it could reply, leaving the MCP client + // waiting on a request that never got a response. + var exitErr *exec.ExitError if !errors.As(err, &exitErr) { return string(output), err } diff --git a/internal/mcp/command_executor_internal_test.go b/internal/mcp/command_executor_internal_test.go new file mode 100644 index 00000000..caf26175 --- /dev/null +++ b/internal/mcp/command_executor_internal_test.go @@ -0,0 +1,72 @@ +package mcp + +import ( + "context" + "os" + "path/filepath" + "runtime" + "strings" + "testing" +) + +// fakeFlowBinary writes a script that echoes to both streams and exits with the +// given code, then points FLOW_CLI_BINARY at it so FlowCLIExecutor runs it in +// place of the real CLI. +func fakeFlowBinary(t *testing.T, exitCode int) { + t.Helper() + if runtime.GOOS == "windows" { + t.Skip("shell script stand-in is not portable to Windows") + } + + path := filepath.Join(t.TempDir(), "fake-flow") + script := "#!/bin/sh\necho stdout-line\necho stderr-line >&2\nexit " + + strings.TrimSpace(string(rune('0'+exitCode))) + "\n" + if err := os.WriteFile(path, []byte(script), 0700); err != nil { + t.Fatalf("writing fake binary: %v", err) + } + t.Setenv(cliBinaryEnvKey, path) +} + +// A non-zero exit is a normal outcome for a flow command (a failing test, a +// script that raises). It must come back as output with a nil error, not as a +// panic: a panic here kills the tool handler's goroutine before it replies, and +// the MCP client then waits on a request that never gets a response. +func TestExecuteContext_NonZeroExitReturnsOutputNotPanic(t *testing.T) { + fakeFlowBinary(t, 1) + + defer func() { + if r := recover(); r != nil { + t.Fatalf("ExecuteContext panicked on a non-zero exit: %v", r) + } + }() + + out, err := (&FlowCLIExecutor{}).ExecuteContext(context.Background(), "anything") + if err != nil { + t.Errorf("err = %v, want nil for a non-zero exit", err) + } + if !strings.Contains(out, "stdout-line") || !strings.Contains(out, "stderr-line") { + t.Errorf("output = %q, want both streams captured", out) + } +} + +func TestExecuteContext_SuccessReturnsOutput(t *testing.T) { + fakeFlowBinary(t, 0) + + out, err := (&FlowCLIExecutor{}).ExecuteContext(context.Background(), "anything") + if err != nil { + t.Fatalf("err = %v, want nil", err) + } + if !strings.Contains(out, "stdout-line") { + t.Errorf("output = %q, want stdout captured", out) + } +} + +// A missing binary is a real failure, not an exit status, so it must surface as +// an error rather than being swallowed alongside exit codes. +func TestExecuteContext_MissingBinaryReturnsError(t *testing.T) { + t.Setenv(cliBinaryEnvKey, filepath.Join(t.TempDir(), "does-not-exist")) + + if _, err := (&FlowCLIExecutor{}).ExecuteContext(context.Background(), "anything"); err == nil { + t.Error("err = nil, want an error when the binary cannot be run") + } +}