From e878082fcc2d7435ff296afeae262dde97c16405 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Fri, 4 Sep 2026 14:54:53 +0200 Subject: [PATCH 1/2] fix(parallel): give upstream MCPs the broker's environment The parallel broker spawned each upstream with cmd.Env set to only the spec's env block from mcp.json, so PATH, HOME and everything else were gone. Any upstream that shells out failed: the git MCP's analyze_change returned `exec: "git": executable file not found in $PATH` for every call routed through triagent-parallel__call, while the same tool invoked directly by Claude Code worked because Claude Code layers the env block onto its own environment. Build the upstream env the same way: os.Environ() with the spec's env layered on top, spec winning on conflict so each upstream keeps its own telemetry tool prefix. The broker's TRIAGENT_MCP_PARALLEL_UPSTREAMS blob is dropped since it carries every upstream's secrets and means nothing to a child. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01TnKDV9gdifLNNJQ8SbpSFV --- pkg/mcp/parallel/registry.go | 39 +++++++++++++++++++++++-------- pkg/mcp/parallel/registry_test.go | 31 ++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 10 deletions(-) diff --git a/pkg/mcp/parallel/registry.go b/pkg/mcp/parallel/registry.go index e2658639..3107944e 100644 --- a/pkg/mcp/parallel/registry.go +++ b/pkg/mcp/parallel/registry.go @@ -4,7 +4,9 @@ import ( "context" "encoding/json" "fmt" + "os" "os/exec" + "strings" "sync" sdkmcp "github.com/modelcontextprotocol/go-sdk/mcp" @@ -150,18 +152,35 @@ func prodSpawner(spec UpstreamSpec) (*sdkmcp.ClientSession, func() error, error) return sess, closeFn, nil } -// buildUpstreamCommand assembles the *exec.Cmd for a given upstream. We -// populate cmd.Env explicitly from the spec so the broker process's own -// env doesn't leak into upstreams (each upstream's env block in mcp.json -// is the full intended environment for that subprocess). +// buildUpstreamCommand assembles the *exec.Cmd for a given upstream. The +// subprocess gets the broker's own environment with the spec's env block +// layered on top, which is the same shape Claude Code gives an MCP it +// launches directly: PATH, HOME and the rest come from the parent so an +// upstream that shells out (git fetch, kubectl) can find its binaries, and +// the spec wins on conflict so each upstream keeps its own telemetry tool +// prefix instead of the broker's. The broker's upstream registry blob is +// dropped: it is large, carries every upstream's secrets, and means nothing +// to a child. func buildUpstreamCommand(spec UpstreamSpec) *exec.Cmd { cmd := exec.Command(spec.Command, spec.Args...) - if len(spec.Env) > 0 { - env := make([]string, 0, len(spec.Env)) - for k, v := range spec.Env { - env = append(env, k+"="+v) + cmd.Env = upstreamEnv(os.Environ(), spec.Env) + return cmd +} + +func upstreamEnv(parent []string, overrides map[string]string) []string { + env := make([]string, 0, len(parent)+len(overrides)) + for _, kv := range parent { + key, _, _ := strings.Cut(kv, "=") + if key == EnvUpstreams { + continue } - cmd.Env = env + if _, overridden := overrides[key]; overridden { + continue + } + env = append(env, kv) } - return cmd + for k, v := range overrides { + env = append(env, k+"="+v) + } + return env } diff --git a/pkg/mcp/parallel/registry_test.go b/pkg/mcp/parallel/registry_test.go index de7dcb8f..381948cc 100644 --- a/pkg/mcp/parallel/registry_test.go +++ b/pkg/mcp/parallel/registry_test.go @@ -3,9 +3,11 @@ package parallel import ( "context" "encoding/json" + "strings" "testing" sdkmcp "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -90,3 +92,32 @@ func TestBuildUpstreamCommand_PopulatesArgsAndEnv(t *testing.T) { require.Equal(t, []string{"/bin/echo", "a", "b"}, cmd.Args) require.Contains(t, cmd.Env, "K=V") } + +// Upstreams shell out (the git MCP runs `git fetch`), so they need the +// broker's PATH, HOME and friends exactly as they would when Claude Code +// launches them directly. The spec's env block layers on top and wins on +// conflict, so an upstream keeps its own telemetry tool prefix rather than +// inheriting the broker's. +func TestBuildUpstreamCommand_InheritsBrokerEnvWithSpecOverride(t *testing.T) { + t.Setenv("PATH", "/parallel-test-bin") + t.Setenv("TRIAGENT_MCP_TELEMETRY_TOOL_PREFIX", "mcp__triagent-parallel__") + t.Setenv("TRIAGENT_MCP_PARALLEL_UPSTREAMS", "{}") + + spec := UpstreamSpec{Command: "/bin/echo", Env: map[string]string{ + "TRIAGENT_MCP_TELEMETRY_TOOL_PREFIX": "mcp__triagent-git-alerts__", + }} + cmd := buildUpstreamCommand(spec) + + assert.Contains(t, cmd.Env, "PATH=/parallel-test-bin") + assert.Contains(t, cmd.Env, "TRIAGENT_MCP_TELEMETRY_TOOL_PREFIX=mcp__triagent-git-alerts__") + assert.NotContains(t, cmd.Env, "TRIAGENT_MCP_TELEMETRY_TOOL_PREFIX=mcp__triagent-parallel__") + for _, kv := range cmd.Env { + assert.False(t, strings.HasPrefix(kv, "TRIAGENT_MCP_PARALLEL_UPSTREAMS="), "upstream registry blob must not leak into upstreams: %s", kv) + } +} + +func TestBuildUpstreamCommand_EmptySpecEnvStillInheritsBrokerEnv(t *testing.T) { + t.Setenv("PATH", "/parallel-test-bin") + cmd := buildUpstreamCommand(UpstreamSpec{Command: "/bin/echo"}) + assert.Contains(t, cmd.Env, "PATH=/parallel-test-bin") +} From 102228f9158222297a584c5e135985938cfc5cd9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Fri, 4 Sep 2026 15:02:28 +0200 Subject: [PATCH 2/2] fix(parallel): drop the upstreams blob even when a spec env carries it upstreamEnv filtered TRIAGENT_MCP_PARALLEL_UPSTREAMS out of the broker's inherited environment but not out of the spec's override block, so the docstring's guarantee that the secrets-bearing registry never reaches a child held only for one of the two sources. Preflight never writes the key into an upstream's env block today, but the guarantee should not depend on that. Skip the key in both loops and extend the test to put it in the spec too. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01TnKDV9gdifLNNJQ8SbpSFV --- pkg/mcp/parallel/registry.go | 3 +++ pkg/mcp/parallel/registry_test.go | 3 ++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/pkg/mcp/parallel/registry.go b/pkg/mcp/parallel/registry.go index 3107944e..faae6365 100644 --- a/pkg/mcp/parallel/registry.go +++ b/pkg/mcp/parallel/registry.go @@ -180,6 +180,9 @@ func upstreamEnv(parent []string, overrides map[string]string) []string { env = append(env, kv) } for k, v := range overrides { + if k == EnvUpstreams { + continue + } env = append(env, k+"="+v) } return env diff --git a/pkg/mcp/parallel/registry_test.go b/pkg/mcp/parallel/registry_test.go index 381948cc..95a5a714 100644 --- a/pkg/mcp/parallel/registry_test.go +++ b/pkg/mcp/parallel/registry_test.go @@ -105,6 +105,7 @@ func TestBuildUpstreamCommand_InheritsBrokerEnvWithSpecOverride(t *testing.T) { spec := UpstreamSpec{Command: "/bin/echo", Env: map[string]string{ "TRIAGENT_MCP_TELEMETRY_TOOL_PREFIX": "mcp__triagent-git-alerts__", + EnvUpstreams: "{\"from-spec\":{}}", }} cmd := buildUpstreamCommand(spec) @@ -112,7 +113,7 @@ func TestBuildUpstreamCommand_InheritsBrokerEnvWithSpecOverride(t *testing.T) { assert.Contains(t, cmd.Env, "TRIAGENT_MCP_TELEMETRY_TOOL_PREFIX=mcp__triagent-git-alerts__") assert.NotContains(t, cmd.Env, "TRIAGENT_MCP_TELEMETRY_TOOL_PREFIX=mcp__triagent-parallel__") for _, kv := range cmd.Env { - assert.False(t, strings.HasPrefix(kv, "TRIAGENT_MCP_PARALLEL_UPSTREAMS="), "upstream registry blob must not leak into upstreams: %s", kv) + assert.False(t, strings.HasPrefix(kv, EnvUpstreams+"="), "upstream registry blob must not leak into upstreams from the broker env or the spec: %s", kv) } }