From 12648f500209f98a9c96fdfb15edc56ba26f4eee Mon Sep 17 00:00:00 2001 From: Jahvon Dockery Date: Thu, 27 Aug 2026 00:22:14 -0400 Subject: [PATCH 1/2] 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") + } +} From 85734b9a670521589262aa95ef1e313af80206f3 Mon Sep 17 00:00:00 2001 From: Jahvon Dockery Date: Thu, 27 Aug 2026 00:22:24 -0400 Subject: [PATCH 2/2] chore(mcp): set an idle timeout for the flow MCP server MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bounds how long an MCP client waits on a silent flow tool call before giving up. This is a backstop, not the fix for the hang that prompted it — that was a panic in the exit-error path, fixed separately. Scoped per-server rather than set globally via CLAUDE_CODE_MCP_TOOL_IDLE_TIMEOUT so it does not loosen timeouts for every other MCP server a user has connected. Ten minutes matches the longest timeout in CI. It is an idle timeout — time without output or progress, not total runtime — so a legitimately slow run like `validate` on a cold cache is unaffected, while a genuine hang still ends. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01R328pa3FUUfga4gYah1iQi --- .mcp.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.mcp.json b/.mcp.json index 19fec29f..15fb2efb 100644 --- a/.mcp.json +++ b/.mcp.json @@ -3,7 +3,8 @@ "flow": { "type": "stdio", "command": "flow", - "args": ["mcp"] + "args": ["mcp"], + "timeout": 600000 } } }