diff --git a/internal/mcp/server.go b/internal/mcp/server.go index 1d0d41b..73e45f1 100644 --- a/internal/mcp/server.go +++ b/internal/mcp/server.go @@ -50,22 +50,19 @@ func RunServer(version string) { sctx.LiveFeed = true } - // Pin the real stdout for exclusive use by the JSON-RPC encoder, then - // repoint the process-level os.Stdout at stderr. Tool handlers — and - // the TUI helpers they call (progress bars, browser animations, - // banners in internal/tui) — freely fmt.Print to os.Stdout. In MCP - // mode those writes corrupt the newline-delimited JSON-RPC stream the - // client (Codex/CC rmcp) reads: an empty line from fmt.Println() - // deserializes to nothing (serde "line: 0, column: 0") and a progress - // bar deserializes to garbage, either of which kills the transport - // worker with "data did not match any variant of untagged enum - // JsonRpcMessage". Redirecting os.Stdout to stderr sends every stray - // write to diagnostics instead of the wire the parser reads. - realStdout := os.Stdout - os.Stdout = os.Stderr - defer func() { os.Stdout = realStdout }() - - serveMCP(os.Stdin, realStdout, sctx, version) + // Reserve the original stdout stream for JSON-RPC only, then route every + // other stdout write to stderr. Reassigning os.Stdout protects normal Go + // fmt.Print calls, but not lower-level fd 1 writes from subprocesses or + // libraries. redirectStdoutForMCP duplicates the original stdout for the + // encoder and, on Unix, also redirects the actual fd 1 to stderr. + jsonOut, restoreStdout, err := redirectStdoutForMCP() + if err != nil { + fmt.Fprintf(os.Stderr, "qmax-code MCP stdout isolation failed: %v\n", err) + return + } + defer restoreStdout() + + serveMCP(os.Stdin, jsonOut, sctx, version) } // serveMCP runs the newline-delimited JSON-RPC read/respond loop against the diff --git a/internal/mcp/stdout_other.go b/internal/mcp/stdout_other.go new file mode 100644 index 0000000..9bae044 --- /dev/null +++ b/internal/mcp/stdout_other.go @@ -0,0 +1,13 @@ +//go:build !darwin && !linux + +package mcp + +import "os" + +func redirectStdoutForMCP() (*os.File, func(), error) { + origStdout := os.Stdout + os.Stdout = os.Stderr + return origStdout, func() { + os.Stdout = origStdout + }, nil +} diff --git a/internal/mcp/stdout_unix.go b/internal/mcp/stdout_unix.go new file mode 100644 index 0000000..e23ba70 --- /dev/null +++ b/internal/mcp/stdout_unix.go @@ -0,0 +1,45 @@ +//go:build darwin || linux + +package mcp + +import ( + "fmt" + "os" + + "golang.org/x/sys/unix" +) + +func redirectStdoutForMCP() (*os.File, func(), error) { + origStdout := os.Stdout + + jsonFD, err := unix.Dup(int(origStdout.Fd())) + if err != nil { + return nil, func() {}, fmt.Errorf("duplicate stdout: %w", err) + } + jsonOut := os.NewFile(uintptr(jsonFD), "qmax-mcp-json-stdout") + if jsonOut == nil { + _ = unix.Close(jsonFD) + return nil, func() {}, fmt.Errorf("wrap duplicated stdout") + } + + restoreFD, err := unix.Dup(int(origStdout.Fd())) + if err != nil { + _ = jsonOut.Close() + return nil, func() {}, fmt.Errorf("duplicate stdout for restore: %w", err) + } + + if err := unix.Dup2(int(os.Stderr.Fd()), int(origStdout.Fd())); err != nil { + _ = jsonOut.Close() + _ = unix.Close(restoreFD) + return nil, func() {}, fmt.Errorf("redirect stdout to stderr: %w", err) + } + + os.Stdout = os.Stderr + + return jsonOut, func() { + _ = jsonOut.Close() + _ = unix.Dup2(restoreFD, int(origStdout.Fd())) + _ = unix.Close(restoreFD) + os.Stdout = origStdout + }, nil +} diff --git a/internal/mcp/stdout_unix_test.go b/internal/mcp/stdout_unix_test.go new file mode 100644 index 0000000..239ff8b --- /dev/null +++ b/internal/mcp/stdout_unix_test.go @@ -0,0 +1,131 @@ +//go:build darwin || linux + +package mcp + +import ( + "bufio" + "encoding/json" + "fmt" + "io" + "os" + "strings" + "testing" + "time" + + "golang.org/x/sys/unix" +) + +func TestRunServerRedirectsFDStdoutAwayFromJSONRPC(t *testing.T) { + origStdin := os.Stdin + origStdout := os.Stdout + origStderr := os.Stderr + + inR, inW, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + outR, outW, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + errR, errW, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + + stdoutFD := int(outW.Fd()) + + restore := func() { + os.Stdin = origStdin + os.Stdout = origStdout + os.Stderr = origStderr + } + defer func() { + restore() + _ = inR.Close() + _ = inW.Close() + _ = outR.Close() + _ = outW.Close() + _ = errR.Close() + _ = errW.Close() + }() + + os.Stdin = inR + os.Stdout = outW + os.Stderr = errW + + done := make(chan struct{}) + go func() { + RunServer("test") + close(done) + }() + + if _, err := fmt.Fprintln(inW, `{"jsonrpc":"2.0","id":1,"method":"initialize","params":{}}`); err != nil { + t.Fatal(err) + } + + stdoutReader := bufio.NewReader(outR) + lineCh := make(chan string, 1) + errCh := make(chan error, 1) + go func() { + line, err := stdoutReader.ReadString('\n') + if err != nil { + errCh <- err + return + } + lineCh <- line + }() + + var firstLine string + select { + case firstLine = <-lineCh: + case err := <-errCh: + t.Fatalf("reading initialize response: %v", err) + case <-time.After(2 * time.Second): + t.Fatal("timed out waiting for initialize response") + } + + var msg response + if err := json.Unmarshal([]byte(firstLine), &msg); err != nil { + t.Fatalf("initialize response is not JSON-RPC: %v: %q", err, firstLine) + } + + const stray = "raw fd1 write from lower-level dependency\n" + if _, err := unix.Write(stdoutFD, []byte(stray)); err != nil { + t.Fatal(err) + } + + if err := inW.Close(); err != nil { + t.Fatal(err) + } + + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatal("timed out waiting for RunServer to exit") + } + restore() + + if err := outW.Close(); err != nil { + t.Fatal(err) + } + if err := errW.Close(); err != nil { + t.Fatal(err) + } + + remainingStdout, err := io.ReadAll(stdoutReader) + if err != nil { + t.Fatal(err) + } + stderrBytes, err := io.ReadAll(errR) + if err != nil { + t.Fatal(err) + } + + if stdout := firstLine + string(remainingStdout); strings.Contains(stdout, stray) { + t.Fatalf("raw fd stdout leaked onto JSON-RPC stdout: %q", stdout) + } + if stderr := string(stderrBytes); !strings.Contains(stderr, strings.TrimSpace(stray)) { + t.Fatalf("raw fd stdout was not redirected to stderr; stderr = %q", stderr) + } +} diff --git a/internal/repl/repl.go b/internal/repl/repl.go index 44833f0..f57a04c 100644 --- a/internal/repl/repl.go +++ b/internal/repl/repl.go @@ -1500,7 +1500,7 @@ func handleSetCommand(input string, ag *agent.Agent, term *tui.Terminal) { return } - case "cloudsync": + case "cloud_sync", "cloudsync": switch strings.ToLower(value) { case "true", "1", "yes", "on": v := true diff --git a/internal/repl/repl_set_test.go b/internal/repl/repl_set_test.go new file mode 100644 index 0000000..f50be54 --- /dev/null +++ b/internal/repl/repl_set_test.go @@ -0,0 +1,56 @@ +package repl + +import ( + "testing" + + "github.com/qualitymax/qmax-code/internal/agent" + "github.com/qualitymax/qmax-code/internal/api" + "github.com/qualitymax/qmax-code/internal/tui" +) + +func TestHandleSetCommandCloudSyncDocumentedKey(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + cfg := api.DefaultConfig() + ag := &agent.Agent{ + AppConfig: cfg, + Cfg: agent.AgentConfig{Context: &api.SessionContext{}}, + } + + handleSetCommand("/set cloud_sync true", ag, &tui.Terminal{}) + + if cfg.CloudSync == nil || !*cfg.CloudSync { + t.Fatalf("CloudSync = %v, want true", cfg.CloudSync) + } + if loaded := api.LoadQMaxCodeConfig(); loaded.CloudSync == nil || !*loaded.CloudSync { + t.Fatalf("persisted CloudSync = %v, want true", loaded.CloudSync) + } +} + +func TestHandleSetCommandCloudSyncLegacyAlias(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + cfg := api.DefaultConfig() + ag := &agent.Agent{ + AppConfig: cfg, + Cfg: agent.AgentConfig{Context: &api.SessionContext{}}, + } + + handleSetCommand("/set cloudsync false", ag, &tui.Terminal{}) + + if cfg.CloudSync == nil || *cfg.CloudSync { + t.Fatalf("CloudSync = %v, want false", cfg.CloudSync) + } +} + +func TestHandleSetCommandCloudSyncRejectsInvalidValue(t *testing.T) { + cfg := api.DefaultConfig() + ag := &agent.Agent{ + AppConfig: cfg, + Cfg: agent.AgentConfig{Context: &api.SessionContext{}}, + } + + handleSetCommand("/set cloud_sync maybe", ag, &tui.Terminal{}) + + if cfg.CloudSync != nil { + t.Fatalf("CloudSync = %v after invalid value, want nil", cfg.CloudSync) + } +}