diff --git a/CHANGELOG.md b/CHANGELOG.md index 2a7c2161..c4b24764 100644 --- a/CHANGELOG.md +++ b/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 ` 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 diff --git a/docs/concepts/mcp.md b/docs/concepts/mcp.md index ca10ea6b..9e970179 100644 --- a/docs/concepts/mcp.md +++ b/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 | 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. | diff --git a/include/mcp/subprocess.h b/include/mcp/subprocess.h index 80a6b9bc..2bdcce3f 100644 --- a/include/mcp/subprocess.h +++ b/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). diff --git a/src/mcp/subprocess.cpp b/src/mcp/subprocess.cpp index 9ed58792..51d82c23 100644 --- a/src/mcp/subprocess.cpp +++ b/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& 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 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(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 envp; envp.reserve(env_storage.size() + 1); for (auto& s : env_storage) envp.push_back(s.data()); diff --git a/tests/test_mcp.cpp b/tests/test_mcp.cpp index dba5b8c4..e15ebad1 100644 --- a/tests/test_mcp.cpp +++ b/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") {