Skip to content

fix: redirect raw fd1 writes in MCP mode, add cloud_sync alias for /set#143

Merged
Desperado merged 1 commit into
mainfrom
chore/mcp-stdout-isolation-and-cloud-sync-alias
Jul 12, 2026
Merged

fix: redirect raw fd1 writes in MCP mode, add cloud_sync alias for /set#143
Desperado merged 1 commit into
mainfrom
chore/mcp-stdout-isolation-and-cloud-sync-alias

Conversation

@Desperado

Copy link
Copy Markdown
Contributor

Summary

  • MCP mode (qmax-code serve --mcp) previously only reassigned Go's os.Stdout variable to os.Stderr, which protects normal fmt.Print calls but not raw fd 1 writes made by subprocesses or lower-level libraries. Those stray writes corrupt the newline-delimited JSON-RPC stream the client (Codex/CC rmcp) reads, killing the transport worker. redirectStdoutForMCP additionally dup2s the real fd 1 to stderr on Unix (Linux/macOS), with a plain variable-swap fallback on other platforms.
  • /set cloudsync only matched the undocumented key name cloudsync; cloud_sync (the documented key) now works as the primary spelling, with cloudsync kept as a legacy alias.

Test plan

  • go build ./...
  • go vet ./...
  • go test -count=1 ./internal/mcp/... ./internal/repl/... — includes new TestRunServerRedirectsFDStdoutAwayFromJSONRPC (verifies raw fd1 writes land on stderr, not the JSON-RPC stdout stream) and new /set cloud_sync coverage in repl_set_test.go

- MCP mode previously only reassigned Go's os.Stdout to stderr, which
  protects fmt.Print calls but not lower-level fd 1 writes from
  subprocesses/libraries. redirectStdoutForMCP additionally dup2's the
  real fd 1 to stderr on Unix, so stray writes can't corrupt the
  newline-delimited JSON-RPC stream the client reads.
- /set cloudsync only matched the undocumented key name; add the
  documented cloud_sync key as the primary spelling, keeping cloudsync
  as a legacy alias.
@sigilix

sigilix Bot commented Jul 12, 2026

Copy link
Copy Markdown

Sigilix Overview

Effort: 4/5 (large)

Quality gates

  • ✅ PR title follows convention
  • ✅ PR description is complete
  • ℹ️ PR is linked to an issue — No Closes #N / Closes SIG-N keyword found in PR body or commit messages.

Summary — latest push

Fixes a transport-corruption bug in MCP mode where raw fd 1 writes from subprocesses bypassed Go's os.Stdout variable swap and poisoned the newline-delimited JSON-RPC stream. The fix introduces a platform-aware redirectStdoutForMCP that dup2s fd 1 to stderr on Unix while preserving the original fd for the JSON-RPC encoder, with a variable-swap fallback on other platforms. Also adds cloud_sync as the documented primary key for /set, keeping cloudsync as a legacy alias.

Important files

File Score Notes Next step
internal/mcp/stdout_unix.go 5/5 Implements the core Unix fd 1 dup2 redirection logic that prevents raw stdout writes from corrupting the JSON-RPC stream. Verify that the restore closure correctly handles edge cases where the original fd 1 was already closed or where Dup2 fails mid-restore, potentially leaving the process in a broken fd state.
internal/mcp/stdout_unix_test.go 4/5 Integration test that verifies a raw unix.Write to fd 1 is redirected to stderr and does not leak into the JSON-RPC stream. Add a targeted unit test for the restore closure to ensure fd 1 is correctly reinstated after the MCP server exits, preventing fd leaks in the parent process.
internal/mcp/server.go 3/5 Replaces the simple os.Stdout swap with a call to redirectStdoutForMCP, passing the preserved jsonOut fd to the JSON-RPC encoder. Confirm that the fatal error path when redirectStdoutForMCP fails correctly cleans up any partially duplicated file descriptors before returning.
internal/mcp/stdout_other.go 2/5 Provides the non-Unix fallback for redirectStdoutForMCP using the original os.Stdout variable swap. Document the inherent limitation on non-Unix platforms that raw fd 1 writes from subprocesses will still corrupt the JSON-RPC stream.
internal/repl/repl.go 2/5 Adds cloud_sync as a recognized key alongside cloudsync in the /set command handler. Consider deprecating the cloudsync alias with a warning log in a future iteration to migrate users to the documented key.

Sequence diagram

sequenceDiagram
    participant Client
    participant RunServer
    participant redirectStdoutForMCP
    participant serveMCP
    Client->>RunServer: Start MCP mode
    RunServer->>redirectStdoutForMCP: Call
    redirectStdoutForMCP->>redirectStdoutForMCP: Dup original fd 1 (jsonOut)
    redirectStdoutForMCP->>redirectStdoutForMCP: Dup2 stderr -> fd 1
    redirectStdoutForMCP-->>RunServer: Return jsonOut & restore func
    RunServer->>serveMCP: Pass jsonOut for JSON-RPC
    Note over Client: Raw fd 1 writes now go to stderr
    serveMCP-->>Client: JSON-RPC responses on jsonOut
    RunServer->>RunServer: defer restoreStdout()
    Note over RunServer: Dup2 restore fd -> fd 1 & close
Loading

Confidence: 4/5

The core fd redirection logic is well-isolated with a targeted integration test verifying the exact corruption scenario, though the Unix dup2 manipulation warrants careful review of fd lifecycle and restore edge cases.

  • In stdout_unix.go lines 30-32, verify the error path closes both jsonOut and restoreFD to prevent fd leaks if Dup2 fails.
  • In stdout_unix.go lines 38-42, confirm the restore closure safely handles a scenario where Dup2 fails during restoration, as ignoring that error could leave the process with a corrupted stdout state.
  • In stdout_other.go, acknowledge that on non-Unix platforms, raw fd 1 writes from spawned subprocesses will still corrupt the JSON-RPC stream since only the Go variable is swapped.
  • In server.go line 62, ensure that if redirectStdoutForMCP returns an error, the deferred restoreStdout() call is safe to execute even though it wasn't assigned a valid restore function in the error path.

Suggested labels: bug


Posted · af3a375 · 0 findings — View review
Dismiss @sigilix dismiss <reason> (not-a-bug | bad-anchor | already-covered | too-minor | wrong-context) · Re-run /sigilix review

@sigilix sigilix Bot added the bug Something isn't working label Jul 12, 2026
@qualitymaxapp

qualitymaxapp Bot commented Jul 12, 2026

Copy link
Copy Markdown

✅ QualityMax Pipeline

Gate Result
🔍 AI Review ✅ Clean
🧪 Repo Tests ✅ 421/421 passed (go)
🤖 AI Tests ✅ 47/51 passed

Powered by QualityMax — AI-Powered Test Automation

@Desperado Desperado merged commit 93f6cd0 into main Jul 12, 2026
7 checks passed
@Desperado Desperado deleted the chore/mcp-stdout-isolation-and-cloud-sync-alias branch July 12, 2026 16:45
@Desperado Desperado mentioned this pull request Jul 12, 2026
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant