Let MCP registry env override inherited parent keys

CHANGELOG.md

@@ -7,41 +7,25 @@ loosely while pre-1.0 (breaking changes can land on minor bumps).
 
 ## [Unreleased]
 
-### Added
-- **Intent reconcile Phase B (JIT ΔS waves).** `POST /v1/reconcile` `mode=ensure`
-  runs a wave loop: observe → spawn ephemeral agent clones for residual
-  covers → execute in the bound workspace → teardown → re-observe, until ΔS
-  is empty or a budget/cancel/rollback fires. Success is runtime re-observe
-  plus required verification — not model self-claim or advisor
-  CONTINUE/REDIRECT/HALT. `agent_map` (or `StateClause.agent` / capability
-  fallback) is required to resolve ensure residuals; no LLM mapper.
-  Budgets: `max_waves`, `max_wall_ms`, `max_agents_per_wave`,
-  `max_retries_per_clause`. SSE: `agent.spawned` / `agent.teardown` plus
-  `reconcile.delta` each wave. Observe mode and `POST /v1/orchestrate` are
-  unchanged. See [`docs/concepts/reconcile.md`](docs/concepts/reconcile.md)
-  and [#208](https://github.com/tylerreckart/arbiter/issues/208).
 ### Fixed
+- **MCP registry `env` overrides parent keys.** Subprocess spawn skipped
+  secret-shaped parent keys then appended `env_extra`, leaving duplicate
+  `KEY=` entries. glibc/macOS `getenv` (and `execvp` PATH search) use the
+  first match, so registry `PATH` / `NODE_ENV` / `HTTP_PROXY` / non-secret
+  keys were ignored. Inherited keys that `env` sets are now omitted so
+  the registry value is the only copy.
+- **Reconcile rollback no longer wipes the workspace on a failed restore.**
+- **LaTeX math recursion depth.** `latex_math_to_plain` now stops converting
+- **Secret key/token writes do not follow a planted dest symlink.**
+- **Remote `--connect` base URL query/userinfo.** `normalize_api_base_url`
+- **Intent reconcile Phase B (JIT ΔS waves).** `POST /v1/reconcile` `mode=ensure`
 - **A2A unary HTTP errors stay bounded.** `Client::rpc` no longer concatenates
-  the remote response body into `err_out`. `/a2a call` copies that string into
-  the calling agent's tool envelope, so a verbose or hostile remote could dump
-  unbounded HTML/JSON (and any secrets it echoed) into conversation history.
-  Non-200 responses now report `HTTP <status>` plus a 200-byte, single-line
-  JSON-RPC `error.message` when present. JSON-RPC errors on HTTP 200 are
-  clipped the same way.
 - **Remote `--connect` DELETE/PATCH body cap.** Conversation delete and
-  title-rename used a one-shot libcurl write callback that appended the
-  entire response with no limit. A `--connect` peer that omitted
-  `Content-Length` could grow the TUI heap without bound; GET/POST already
-  go through `a2a::http`. Cap is 16 MiB (`kSseMaxEventBytes`).
 - **Intent LLM prompt text cap.** `build_llm_user_prompt` now truncates
 - **Remote TUI: recoverable SSE `error` is not a failed turn.** `RemoteSseTurnConsumer::finish` copied accumulated `error` event text even when the terminal `done` event had `ok: true` (e.g. catalog skip of a stored agent whose JSON failed validation). `done` is authoritative: success clears the result error; failure still prefers `done.error` and falls back to prior `error` events when that field is empty.
 - **Unreadable TUI sessions are not empty.** `session_json_is_empty` no
 - **Advise-gate cancel is not a bad bearer.** `POST /v1/advise/gate`
 - **MCP string JSON-RPC ids.** `parse_response` now accepts a decimal-string
-- **Reconcile rollback no longer wipes the workspace on a failed restore.**
-- **LaTeX math recursion depth.** `latex_math_to_plain` now stops converting
-- **Secret key/token writes do not follow a planted dest symlink.**
-- **Remote `--connect` base URL query/userinfo.** `normalize_api_base_url`
 
 ## [0.13.6] — 2026-09-21
 

docs/concepts/mcp.md

@@ -34,7 +34,7 @@ Configure available servers via `~/.arbiter/mcp_servers.json` (path comes from `
 |-------------------|----------------|----------|-------------|
 | `command`         | string         | yes      | Executable to run. PATH-resolved via `execvp`. |
 | `args`            | array<string>  | no       | Arguments after `command`. |
-| `env`             | object         | no       | Extra `KEY: "VALUE"` strings appended to the parent environment. |
+| `env`             | object         | no       | Extra `KEY: "VALUE"` strings merged into the child environment. Same-name keys override the parent (secret-shaped parent keys are stripped; registry secrets are still passed through). |
 | `init_timeout_ms` | int            | no       | Wall-clock budget for the JSON-RPC `initialize` handshake. Defaults to 60s — first-run `npx` may install. |
 | `call_timeout_ms` | int            | no       | Per-`tools/call` timeout. Defaults to 30s. Playwright snapshots/navigation routinely take 5–15s. |
 

include/mcp/subprocess.h

@@ -29,9 +29,11 @@ constexpr size_t kReadBufMaxBytes = 256 * 1024;
 class Subprocess {
 public:
     // Spawn `argv[0]` as the executable, with the remaining args.  `env_extra`
-    // is appended after a scrubbed copy of the parent environment (secret-
-    // shaped keys like *_API_KEY / ARBITER_* are stripped).  Useful for
-    // passing PLAYWRIGHT_BROWSERS_PATH etc.  Throws std::runtime_error on
+    // is applied after a scrubbed copy of the parent environment (secret-
+    // shaped keys like *_API_KEY / ARBITER_* are stripped).  Extra keys
+    // override inherited parent keys of the same name so getenv sees the
+    // registry value (glibc first-match).  Useful for PATH, NODE_ENV,
+    // PLAYWRIGHT_BROWSERS_PATH, etc.  Throws std::runtime_error on
     // fork/exec failure (the child's exec failure is detected by an immediate
     // EOF on stdout, which manifests as recv_line returning std::nullopt
     // before any data — caller treats that as a startup failure).

src/mcp/subprocess.cpp

@@ -60,8 +60,11 @@ void purge_stdio_buffers() {
 }
 
 // True when an inherited env KEY should be stripped from MCP children.
-// Registry `env_extra` is appended after the scrub and is allowed through
+// Registry `env_extra` is applied after the scrub and is allowed through
 // even when the key would otherwise match — operators opt into those.
+// Extra keys replace inherited parent keys of the same name so getenv /
+// execvp PATH search / Node process.env see the registry value rather
+// than a duplicate later in environ (glibc getenv is first-match).
 bool is_secret_env_key(std::string_view key) {
     if (key.empty()) return true;
     auto has_suffix = [&](std::string_view suf) {
@@ -140,20 +143,37 @@ Subprocess::Subprocess(const std::vector<std::string>& argv,
 
         purge_stdio_buffers();
 
-        // Inherit parent environ minus secret-shaped keys, then append
+        // Inherit parent environ minus secret-shaped keys, then apply
         // registry env_extra.  MCP children (often untrusted npx packages)
         // must not see provider/admin credentials from the arbiter process.
+        // Extra keys override inherited ones: glibc/macOS getenv and
+        // execvp's PATH search use the first KEY= in environ, so appending
+        // a duplicate left PATH / NODE_ENV / HTTP_PROXY / etc. stuck on
+        // the parent value.  Skip parent keys that env_extra sets, then
+        // append extra (secrets in extra still pass — operator opt-in).
         // Done this way (rather than execvpe) because macOS doesn't expose
         // execvpe and we need the PATH-search behaviour for `npx`.
+        auto extra_key = [](const std::string& s) -> std::string_view {
+            const auto eq = s.find('=');
+            return eq == std::string::npos ? std::string_view(s)
+                                           : std::string_view(s.data(), eq);
+        };
+        auto extra_has_key = [&](std::string_view key) {
+            for (const auto& s : env_extra) {
+                if (extra_key(s) == key) return true;
+            }
+            return false;
+        };
         std::vector<std::string> env_storage;
         for (char** e = environ; e && *e; ++e) {
             const char* eq = std::strchr(*e, '=');
             std::string_view key = eq ? std::string_view(*e, static_cast<size_t>(eq - *e))
                                       : std::string_view(*e);
             if (is_secret_env_key(key)) continue;
+            if (extra_has_key(key)) continue;
             env_storage.emplace_back(*e);
         }
-        for (auto& s : env_extra) env_storage.push_back(s);
+        for (const auto& s : env_extra) env_storage.push_back(s);
         std::vector<char*> envp;
         envp.reserve(env_storage.size() + 1);
         for (auto& s : env_storage) envp.push_back(s.data());

tests/test_mcp.cpp

@@ -472,6 +472,21 @@ TEST_CASE("Subprocess: strips secret-shaped parent env; keeps env_extra") {
     ::unsetenv("ARBITER_ADMIN_TOKEN");
 }
 
+TEST_CASE("Subprocess: env_extra overrides inherited parent keys") {
+    // Non-secret parent keys used to stay first in environ; getenv and
+    // `printenv KEY` then returned the parent value despite env_extra.
+    ::setenv("MCP_ENV_OVERRIDE_PROBE", "from-parent", 1);
+    Subprocess proc({"/usr/bin/printenv", "MCP_ENV_OVERRIDE_PROBE"},
+                    {"MCP_ENV_OVERRIDE_PROBE=from-registry"});
+    auto line = proc.recv_line(500ms);
+    REQUIRE(line.has_value());
+    CHECK(*line == "from-registry");
+    // No leftover parent copy for programs that iterate environ.
+    auto extra = proc.recv_line(200ms);
+    CHECK_FALSE(extra.has_value());
+    ::unsetenv("MCP_ENV_OVERRIDE_PROBE");
+}
+
 // ── 3. /mcp slash dispatch ─────────────────────────────────────────
 
 TEST_CASE("parse_agent_commands recognises /mcp") {