diff --git a/CHANGELOG.md b/CHANGELOG.md index 5005c6e8..d0a3f56e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,47 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [0.3.0 engine / 0.4.0 wrapper] — 2026-05-27 + +### Fixed + +- **Engine** `_runtime.py` — three latent runtime-crashing bugs in MCP server config handling, all silenced by `# pyright: ignore` suppressions: + - `AttributeError: 'PreparedBundle' object has no attribute 'config'` — author wrote prose comments asserting `PreparedBundle.config` was the merged bundle yaml; it does not exist. The merged yaml lives on `mount_plan`. + - `AttributeError: 'list' object has no attribute 'get'` — `mount_plan["tools"]` is a list of `{module, source, config}` dicts, not a dict keyed by module name. The author treated it as a dict. + - `TypeError: PreparedBundle.create_session() got an unexpected keyword argument 'tool_overrides'` — the kwarg does not exist on the foundation API. + Each suppression masked a real attribute or call error pyright had flagged. The whole `--mcp-servers` flow was non-functional at 0.2.0; the file-based discovery paths documented in `amplifier-module-tool-mcp` continued to work. + +### Changed + +- **Wire (BREAKING)** `PROTOCOL_VERSION` bumped `0.1.0` → `0.2.0`. MCP server delivery refactored from inline `mcpServers: dict` to path-based `mcpConfigPath: str`. The engine forwards the path to `tool-mcp` via `AMPLIFIER_MCP_CONFIG` (one of four documented config priorities in the module). Old wrappers fail with a clean `protocol_version_mismatch` rather than a confusing runtime crash. +- **Engine CLI** `--mcp-servers` flag renamed to `--mcp-config-path`. The engine no longer parses MCP config contents — it validates the path exists and forwards it to the module. +- **Wrapper** `mcp-spill.ts` now always spills to a `0600` tmpfile (dropping the inline-JSON-on-argv branch — also eliminates server-config visibility in `ps aux`) and writes content in the format the module expects (`{"mcpServers": }`). +- **Wrapper** `resolveMcpServersFlag` → `resolveMcpConfigPath`; `McpSpillResult.flag` → `configPath`; `mcpServersFlag` in `AssembleArgvInput` → `mcpConfigPath`. +- **Engine** audit field renamed `mcpServersDigest` → `mcpConfigPathDigest` (hashes the path string for stable identifier without dragging file IO into the audit path). + +### Architecture + +Each layer now owns exactly one responsibility: + +| Layer | Responsibility | +|---|---| +| Wrapper | Write the file in the format the module expects; manage tmpfile lifecycle | +| Engine CLI | Validate that the path exists; forward to runtime | +| Engine runtime | One line: `os.environ["AMPLIFIER_MCP_CONFIG"] = mcp_config_path` | +| Module (`amplifier-module-tool-mcp`, unchanged) | Read the file via its existing 4-source config priority | + +The previous design tried to merge MCP config inside the engine via a non-existent `tool_overrides` kwarg, building parallel rails to a config-discovery mechanism the module already implemented. + +### Released + +- `amplifier-agent` (engine) 0.3.0 — consumers pin via `git+https://github.com/microsoft/amplifier-agent@engine-v0.3.0`. PyPI publishing not yet wired. +- `amplifier-agent-ts` (wrapper) 0.4.0 — published to npm with provenance via the existing OIDC trusted-publishing workflow (`publish-wrapper.yml` on `wrapper-v*` tag push). + +### Migration + +- **Wrapper callers**: no API change. `SpawnAgentParams.mcpServers: Record` is preserved. The wire-level rename is internal to the wrapper. +- **Container/Dockerfile consumers (e.g. nanoclaw)**: bump `AMPLIFIER_AGENT_REF` to `engine-v0.3.0` and bump the wrapper dependency to `0.4.0`. The pair must move together — protocol mismatch errors are explicit and ship with remediation hints. + ## [0.2.0] — 2026-05-22 ### Added diff --git a/pyproject.toml b/pyproject.toml index 8ab545bb..49a5b43d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = 'amplifier-agent' -version = '0.2.0' +version = '0.3.0' requires-python = '>=3.12' license = 'MIT' dependencies = [ diff --git a/src/amplifier_agent_cli/modes/single_turn.py b/src/amplifier_agent_cli/modes/single_turn.py index 78c92e02..a5b20dac 100644 --- a/src/amplifier_agent_cli/modes/single_turn.py +++ b/src/amplifier_agent_cli/modes/single_turn.py @@ -173,7 +173,7 @@ def _write_audit( started_at: str, ended_at: str, argv: list[str], - mcp_servers: dict[str, Any] | None, + mcp_config_path: str | None, env_allowlist: list[str] | None, env_extra: dict[str, Any] | None, host_capabilities: dict[str, Any] | None, @@ -188,7 +188,9 @@ def _write_audit( audits_dir.mkdir(parents=True, exist_ok=True) audit = { "argvDigest": _sha256(" ".join(argv)), - "mcpServersDigest": (_sha256(json.dumps(mcp_servers, sort_keys=True)) if mcp_servers else None), + # Path is non-secret; hashing gives a stable identifier for audit + # correlation without dragging file I/O into the audit path. + "mcpConfigPathDigest": (_sha256(mcp_config_path) if mcp_config_path else None), "envDigest": _sha256(json.dumps({"allow": env_allowlist or [], "extra": env_extra or {}}, sort_keys=True)), "hostCapabilities": host_capabilities, "protocolVersion": protocol_version, @@ -323,7 +325,7 @@ class _TurnSpec: display: CliDisplaySystem provider: str # detected provider short-name (e.g. 'anthropic') allow_protocol_skew: bool = False - mcp_servers: dict[str, Any] | None = None + mcp_config_path: str | None = None # --------------------------------------------------------------------------- @@ -355,7 +357,7 @@ async def _execute_turn(spec: _TurnSpec) -> dict[str, Any]: prepared, cwd=spec.cwd, is_resumed=spec.resume and not spec.fresh, - mcp_servers=spec.mcp_servers, + mcp_config_path=spec.mcp_config_path, ) engine = Engine( turn_handler=handler, @@ -417,10 +419,10 @@ async def _execute_turn(spec: _TurnSpec) -> dict[str, Any]: help="Output mode: 'json' (default, envelope) or 'text' (reply only).", ) @click.option( - "--mcp-servers", - "mcp_servers_raw", + "--mcp-config-path", + "mcp_config_path", default=None, - help="MCP servers config as inline JSON or '@' to JSON file.", + help="Path to MCP config JSON (see amplifier-module-tool-mcp for the schema; written by the host/wrapper).", ) @click.option( "--allow-protocol-skew", @@ -468,7 +470,7 @@ def run( no_flag: bool, quiet: bool, output_mode: str, - mcp_servers_raw: str | None, + mcp_config_path: str | None, allow_protocol_skew: bool, host_capabilities_raw: str | None, env_allowlist_raw: str | None, @@ -524,9 +526,15 @@ def run( stream=sys.stderr, ) - # (5b) Parse --mcp-servers (inline JSON or @path). Emits a §4.1 error - # envelope and exits 2 on parse / IO / type errors. - mcp_servers = _parse_json_or_atpath(mcp_servers_raw, flag_name="--mcp-servers") + # (5b) Validate --mcp-config-path is a real file if provided. The file's + # contents are not parsed here — the engine forwards the path to + # tool-mcp via AMPLIFIER_MCP_CONFIG and the module reads/validates. + if mcp_config_path is not None: + if not Path(mcp_config_path).is_file(): + _emit_argv_envelope( + "mcp_config_path_invalid", + f"--mcp-config-path: file not found: {mcp_config_path}", + ) # (5c) Parse host capabilities, env extras, and env allowlist (A1'/D12'). # env_extra and env_allowlist are parsed here but threaded into the engine @@ -560,7 +568,7 @@ def run( display=display, provider=provider_name, allow_protocol_skew=allow_protocol_skew or bool(os.environ.get("AMPLIFIER_AGENT_ALLOW_PROTOCOL_SKEW")), - mcp_servers=mcp_servers, + mcp_config_path=mcp_config_path, ) # (7) Run with error handling. @@ -605,7 +613,7 @@ def run( started_at=started_iso, ended_at=datetime.now(UTC).isoformat(), argv=sys.argv, - mcp_servers=mcp_servers, + mcp_config_path=mcp_config_path, env_allowlist=env_allowlist, env_extra=env_extra, host_capabilities=host_capabilities, @@ -633,7 +641,7 @@ def run( started_at=started_iso, ended_at=datetime.now(UTC).isoformat(), argv=sys.argv, - mcp_servers=mcp_servers, + mcp_config_path=mcp_config_path, env_allowlist=env_allowlist, env_extra=env_extra, host_capabilities=host_capabilities, @@ -663,7 +671,7 @@ def run( started_at=started_iso, ended_at=datetime.now(UTC).isoformat(), argv=sys.argv, - mcp_servers=mcp_servers, + mcp_config_path=mcp_config_path, env_allowlist=env_allowlist, env_extra=env_extra, host_capabilities=host_capabilities, diff --git a/src/amplifier_agent_lib/_runtime.py b/src/amplifier_agent_lib/_runtime.py index d7e43d21..d984b088 100644 --- a/src/amplifier_agent_lib/_runtime.py +++ b/src/amplifier_agent_lib/_runtime.py @@ -5,13 +5,15 @@ replay; OpenClaw pattern). ``handle_initialize`` is the wire-side entry point that loads the prepared -bundle, threads wire-supplied ``mcpServers`` into ``tool-mcp.mount()`` via -``tool_overrides``, and stores ``host.capabilities`` on ``session.metadata``. +bundle, forwards a wire-supplied ``mcpConfigPath`` to ``tool-mcp`` via +``AMPLIFIER_MCP_CONFIG``, and stores ``host.capabilities`` on +``session.metadata``. """ from __future__ import annotations import logging +import os from pathlib import Path from typing import TYPE_CHECKING, Any @@ -34,7 +36,7 @@ def make_turn_handler( *, cwd: str | None, is_resumed: bool, - mcp_servers: dict[str, Any] | None = None, + mcp_config_path: str | None = None, ) -> TurnHandler: """Return a TurnHandler closed over the loaded PreparedBundle. @@ -54,13 +56,13 @@ def make_turn_handler( if provided; None otherwise. is_resumed: Whether the session should be treated as a resumed session. - mcp_servers: - Dynamic MCP server configurations supplied by the CLI invoker (e.g. via - the ``--mcp-servers`` flag). Merged with the bundle's static ``tool-mcp`` - config and passed to ``prepared.create_session(tool_overrides=...)`` so - ``tool-mcp.mount()`` sees the dynamic ``servers`` dict. Mirrors the wire - path's behavior in ``handle_initialize`` (this function's CLI-side twin). - Defaults to ``None`` (treated as ``{}``) for backward compatibility. + mcp_config_path: + Path to a JSON file containing MCP server configuration in the format + documented by amplifier-module-tool-mcp. When provided, the engine sets + ``AMPLIFIER_MCP_CONFIG`` so the module loads it via its standard config + discovery (config.py priority chain). The wrapper owns the file format; + the engine just routes the path. + Defaults to ``None`` (no MCP config override). Returns ------- @@ -72,15 +74,12 @@ def make_turn_handler( resolved_cwd: Path | None = Path(cwd).resolve() if cwd else None - # Merge dynamic mcp_servers (CLI-supplied) with the bundle's static tool-mcp - # config and close over the resulting dict. Mirrors the wire path in - # handle_initialize so that --mcp-servers reaches tool-mcp.mount() the same - # way params["mcpServers"] does. Per amplifier_module_tool_mcp/config.py, - # the tool_overrides config dict has highest priority at mount-time. - _tool_mcp_static = ( - prepared.config.get("tools", {}).get("tool-mcp", {}).get("config", {}) # pyright: ignore[reportAttributeAccessIssue] - ) - _tool_mcp_config = {**_tool_mcp_static, "servers": mcp_servers or {}} + # Forward CLI-supplied MCP config to the tool-mcp module via its + # documented AMPLIFIER_MCP_CONFIG entry in the config priority chain + # (see amplifier_module_tool_mcp/config.py). The wrapper owns the file + # format; the engine just routes the path. + if mcp_config_path: + os.environ["AMPLIFIER_MCP_CONFIG"] = mcp_config_path # Pre-hydrate agent overlays from the vendored agent markdown files. # This is done once at handler-creation time (cold path) so each turn @@ -112,7 +111,6 @@ async def handler(ctx: TurnContext) -> str: session_id=session_id, session_cwd=resolved_cwd, is_resumed=is_resumed, - tool_overrides={"tool-mcp": {"config": _tool_mcp_config}}, # pyright: ignore[reportCallIssue] ) # Wire display and approval into the coordinator so hook events can @@ -217,8 +215,8 @@ async def _spawn_fn(**kw: Any) -> dict[str, Any]: async def handle_initialize(params: dict[str, Any]) -> Any: """Wire-side initialize entry point. - Loads the prepared bundle from cache, threads wire-supplied - ``params["mcpServers"]`` into ``tool-mcp.mount()`` via ``tool_overrides``, + Loads the prepared bundle from cache, forwards a wire-supplied + ``params["mcpConfigPath"]`` to ``tool-mcp`` via ``AMPLIFIER_MCP_CONFIG``, and stores ``params.host.capabilities`` on ``session.metadata`` for future capability-flag logic without wire-protocol changes. @@ -226,7 +224,7 @@ async def handle_initialize(params: dict[str, Any]) -> Any: ---------- params: An ``InitializeParams``-shaped dict. Reads ``sessionId``, ``resume``, - ``mcpServers``, and ``host.capabilities``. + ``mcpConfigPath``, and ``host.capabilities``. Returns ------- @@ -234,29 +232,28 @@ async def handle_initialize(params: dict[str, Any]) -> Any: Notes ----- - The static ``tool-mcp`` config (e.g. ``verbose_servers``, ``max_content_size``) - declared in the bundle is merged with the dynamic ``servers`` dict supplied - over the wire. The combined dict is passed to ``mount()`` with highest - priority per ``amplifier_module_tool_mcp/config.py``. + The wrapper writes the MCP config file in the format the module expects + (see amplifier-module-tool-mcp config.py). The engine just sets + ``AMPLIFIER_MCP_CONFIG`` so the module's ``_load_from_env`` picks it up + during mount alongside the bundle's static tool-mcp config. + See methods.py for the wire-protocol field. """ prepared = await load_and_prepare_cached(aaa_version=__version__) session_id: str | None = params.get("sessionId") or None is_resumed: bool = bool(params.get("resume", False)) - # ── A5: Q9 — thread MCP servers into tool-mcp.mount() ── - # PreparedBundle stubs are incomplete; .config is the merged bundle yaml. - _tool_mcp_static = ( - prepared.config.get("tools", {}).get("tool-mcp", {}).get("config", {}) # pyright: ignore[reportAttributeAccessIssue] - ) - tool_mcp_config = {**_tool_mcp_static, "servers": params.get("mcpServers") or {}} + # ── A5: Q9 — forward wire-supplied MCP config path to tool-mcp ── + # The wrapper writes the file in the format the module expects; the + # engine just sets the env var so the module's _load_from_env picks + # it up during mount. See methods.py for the wire-protocol field. + _wire_mcp_config_path = params.get("mcpConfigPath") or None + if _wire_mcp_config_path: + os.environ["AMPLIFIER_MCP_CONFIG"] = _wire_mcp_config_path - # ``tool_overrides`` is accepted by create_session per amplifier_module_tool_mcp/config.py:35-53,56-61 - # — the config dict passed to mount() has highest priority. session = await prepared.create_session( session_id=session_id, is_resumed=is_resumed, - tool_overrides={"tool-mcp": {"config": tool_mcp_config}}, # pyright: ignore[reportCallIssue] ) # ── A5: host capabilities storage ── diff --git a/src/amplifier_agent_lib/protocol/methods.py b/src/amplifier_agent_lib/protocol/methods.py index 96d48f15..3756b5a4 100644 --- a/src/amplifier_agent_lib/protocol/methods.py +++ b/src/amplifier_agent_lib/protocol/methods.py @@ -8,8 +8,17 @@ from typing import Any, NotRequired, TypedDict -PROTOCOL_VERSION = "0.1.0" -"""Wire protocol version. Bump on breaking changes; semver applies.""" +PROTOCOL_VERSION = "0.2.0" +"""Wire protocol version. Bump on breaking changes; semver applies. + +0.2.0 — MCP config delivery changed from inline ``mcpServers`` dict to a + path string (``mcpConfigPath``) pointing at a JSON file in the format + documented by amplifier-module-tool-mcp (top-level ``mcpServers`` key). + The engine sets ``AMPLIFIER_MCP_CONFIG`` from this path; the module + reads it via its standard config discovery (config.py priority chain). + See _runtime.py for the host-side semantics. +0.1.0 — Initial Mode A v2 protocol. +""" class ClientInfo(TypedDict): @@ -82,7 +91,12 @@ class InitializeParams(TypedDict): resume: NotRequired[bool] providerOverride: NotRequired[str] cwd: NotRequired[str] - mcpServers: NotRequired[dict[str, McpServerConfig]] + # MCP config: pass a path to a JSON file in the format documented by + # amplifier-module-tool-mcp (top-level "mcpServers" key). The engine + # sets AMPLIFIER_MCP_CONFIG from this path; the module reads it via its + # standard config discovery. The wrapper handles dict-to-file + # translation for hosts that prefer the inline-dict API. + mcpConfigPath: NotRequired[str] host: NotRequired[InitializeHostParams] diff --git a/tests/test_runtime_mcp_threading.py b/tests/test_runtime_mcp_threading.py index 26ab5aa6..ef5210e1 100644 --- a/tests/test_runtime_mcp_threading.py +++ b/tests/test_runtime_mcp_threading.py @@ -1,146 +1,124 @@ -"""Tests for A5 — MCP threading and host-capabilities storage in _runtime.py. +"""Tests for A5 — MCP config path threading and host-capabilities storage in _runtime.py. -Phase 2 A5 adds an ``handle_initialize(params)`` entry point to ``_runtime.py`` -that loads the prepared bundle, threads wire-supplied ``mcpServers`` into -``tool-mcp.mount()`` via ``tool_overrides``, and stores ``host.capabilities`` -on ``session.metadata`` for future capability-flag logic. +Protocol 0.2.0: ``handle_initialize`` and ``make_turn_handler`` no longer +accept an inline ``mcpServers`` dict. Instead they receive a pre-written +file path (``mcpConfigPath`` / ``mcp_config_path``) and forward it to +``tool-mcp`` by setting ``os.environ["AMPLIFIER_MCP_CONFIG"]``. The module +reads the file via its standard config discovery (config.py priority chain). """ from __future__ import annotations +import os +import tempfile +import json from typing import Any from unittest.mock import AsyncMock, MagicMock, patch import pytest +def _make_mcp_config_file(servers: dict[str, Any]) -> str: + """Write a tmp file in the format tool-mcp expects and return the path.""" + fd, path = tempfile.mkstemp(prefix="test-mcp-", suffix=".json") + with os.fdopen(fd, "w") as f: + json.dump({"mcpServers": servers}, f) + return path + + def _make_params( *, session_id: str = "sess-test-1", - mcp_servers: dict[str, Any] | None = None, + mcp_config_path: str | None = None, host_capabilities: dict[str, Any] | None = None, resume: bool = False, ) -> dict[str, Any]: """Build a minimal InitializeParams dict for testing.""" - return { + params: dict[str, Any] = { "sessionId": session_id, "resume": resume, - "protocolVersion": "0.1.0", + "protocolVersion": "0.2.0", "clientInfo": {"name": "test-harness", "version": "0.0.0"}, "capabilities": {"display": {"events": ["result/final"]}}, - "mcpServers": mcp_servers or {}, "host": {"capabilities": host_capabilities or {}}, } + if mcp_config_path is not None: + params["mcpConfigPath"] = mcp_config_path + return params -def _make_mock_bundle( - *, - tool_mcp_static_config: dict[str, Any] | None = None, -) -> tuple[MagicMock, dict[str, Any], MagicMock]: - """Return (mock_bundle, captured_create_session_kwargs, mock_session).""" - captured: dict[str, Any] = {} - +def _make_mock_bundle() -> tuple[MagicMock, MagicMock]: + """Return (mock_bundle, mock_session).""" mock_session = MagicMock() mock_session.metadata = {} - async def _create_session(**kwargs: Any) -> MagicMock: - captured.update(kwargs) + async def _create_session(**_kwargs: Any) -> MagicMock: return mock_session mock_bundle = MagicMock() - mock_bundle.config = { - "tools": { - "tool-mcp": {"config": tool_mcp_static_config or {"verbose_servers": False, "max_content_size": 65536}} - } - } mock_bundle.create_session = _create_session - return mock_bundle, captured, mock_session + return mock_bundle, mock_session @pytest.mark.asyncio -async def test_mcp_servers_threaded_to_tool_overrides() -> None: - """mcpServers from params must appear in tool_overrides['tool-mcp']['config']['servers'].""" +async def test_mcp_config_path_forwarded_to_env() -> None: + """mcpConfigPath from params must be set in AMPLIFIER_MCP_CONFIG env var.""" from amplifier_agent_lib._runtime import handle_initialize mcp_servers = {"test-mcp": {"transport": "stdio", "command": "/usr/bin/echo", "args": ["hello"]}} - params = _make_params(mcp_servers=mcp_servers) - mock_bundle, captured, _mock_session = _make_mock_bundle() - - mock_store = MagicMock() - mock_store.load.return_value = None # no prior transcript - - with ( - patch( - "amplifier_agent_lib._runtime.load_and_prepare_cached", - AsyncMock(return_value=mock_bundle), - ), - patch("amplifier_agent_lib._runtime.SessionStore", return_value=mock_store), - ): - await handle_initialize(params) - - assert "tool_overrides" in captured, ( - "handle_initialize must pass tool_overrides to create_session. " - "Add: tool_overrides={'tool-mcp': {'config': tool_mcp_config}} to the create_session call." - ) - tool_mcp_cfg = captured["tool_overrides"]["tool-mcp"]["config"] - assert tool_mcp_cfg["servers"] == mcp_servers, ( - f"tool_overrides['tool-mcp']['config']['servers'] should be {mcp_servers!r}, " - f"got {tool_mcp_cfg.get('servers')!r}" - ) - - -@pytest.mark.asyncio -async def test_static_tool_mcp_config_merged_with_servers() -> None: - """Static bundle config keys must be preserved alongside dynamic servers.""" - from amplifier_agent_lib._runtime import handle_initialize - - mcp_servers = {"nano-mcp": {"transport": "sse", "url": "http://localhost:9999"}} - params = _make_params(mcp_servers=mcp_servers) - static = {"verbose_servers": False, "max_content_size": 65536} - mock_bundle, captured, _mock_session = _make_mock_bundle(tool_mcp_static_config=static) - - mock_store = MagicMock() - mock_store.load.return_value = None - - with ( - patch( - "amplifier_agent_lib._runtime.load_and_prepare_cached", - AsyncMock(return_value=mock_bundle), - ), - patch("amplifier_agent_lib._runtime.SessionStore", return_value=mock_store), - ): - await handle_initialize(params) - - tool_mcp_cfg = captured["tool_overrides"]["tool-mcp"]["config"] - assert tool_mcp_cfg.get("verbose_servers") is False, "static verbose_servers must be preserved" - assert tool_mcp_cfg.get("max_content_size") == 65536, "static max_content_size must be preserved" - assert tool_mcp_cfg.get("servers") == mcp_servers, "servers must be merged in" + config_path = _make_mcp_config_file(mcp_servers) + + try: + params = _make_params(mcp_config_path=config_path) + mock_bundle, _ = _make_mock_bundle() + + with ( + patch( + "amplifier_agent_lib._runtime.load_and_prepare_cached", + AsyncMock(return_value=mock_bundle), + ), + ): + # Remove any pre-existing env var to ensure clean state. + old_val = os.environ.pop("AMPLIFIER_MCP_CONFIG", None) + try: + await handle_initialize(params) + assert os.environ.get("AMPLIFIER_MCP_CONFIG") == config_path, ( + f"AMPLIFIER_MCP_CONFIG should be {config_path!r}, " + f"got {os.environ.get('AMPLIFIER_MCP_CONFIG')!r}" + ) + finally: + if old_val is None: + os.environ.pop("AMPLIFIER_MCP_CONFIG", None) + else: + os.environ["AMPLIFIER_MCP_CONFIG"] = old_val + finally: + os.unlink(config_path) @pytest.mark.asyncio -async def test_empty_mcp_servers_still_passes_tool_overrides() -> None: - """Empty mcpServers must still produce tool_overrides with servers={}.""" +async def test_missing_mcp_config_path_leaves_env_unchanged() -> None: + """When mcpConfigPath is absent, AMPLIFIER_MCP_CONFIG is not set.""" from amplifier_agent_lib._runtime import handle_initialize - params = _make_params(mcp_servers={}) # empty - mock_bundle, captured, _mock_session = _make_mock_bundle() - - mock_store = MagicMock() - mock_store.load.return_value = None + params = _make_params() # no mcpConfigPath + mock_bundle, _ = _make_mock_bundle() with ( patch( "amplifier_agent_lib._runtime.load_and_prepare_cached", AsyncMock(return_value=mock_bundle), ), - patch("amplifier_agent_lib._runtime.SessionStore", return_value=mock_store), ): - await handle_initialize(params) - - assert "tool_overrides" in captured - tool_mcp_cfg = captured["tool_overrides"]["tool-mcp"]["config"] - assert tool_mcp_cfg.get("servers") == {}, "empty mcpServers should produce servers={}" + old_val = os.environ.pop("AMPLIFIER_MCP_CONFIG", None) + try: + await handle_initialize(params) + assert "AMPLIFIER_MCP_CONFIG" not in os.environ, ( + "AMPLIFIER_MCP_CONFIG must not be set when mcpConfigPath is absent" + ) + finally: + if old_val is not None: + os.environ["AMPLIFIER_MCP_CONFIG"] = old_val @pytest.mark.asyncio @@ -150,17 +128,13 @@ async def test_host_capabilities_stored_in_session_metadata() -> None: host_caps = {"supports_structured_errors": True, "supports_steering": False} params = _make_params(host_capabilities=host_caps) - mock_bundle, _captured, mock_session = _make_mock_bundle() - - mock_store = MagicMock() - mock_store.load.return_value = None + mock_bundle, mock_session = _make_mock_bundle() with ( patch( "amplifier_agent_lib._runtime.load_and_prepare_cached", AsyncMock(return_value=mock_bundle), ), - patch("amplifier_agent_lib._runtime.SessionStore", return_value=mock_store), ): await handle_initialize(params) @@ -172,19 +146,11 @@ async def test_host_capabilities_stored_in_session_metadata() -> None: # --------------------------------------------------------------------------- # make_turn_handler — CLI path mirror of handle_initialize's wire path. -# Regression coverage for the phase-A-task-7 gap (TODO removed in this fix): -# `amplifier-agent run --mcp-servers ''` must reach tool-mcp.mount() -# the same way the wire's `params["mcpServers"]` does. # --------------------------------------------------------------------------- -def _make_mock_bundle_for_turn( - *, - tool_mcp_static_config: dict[str, Any] | None = None, -) -> tuple[MagicMock, dict[str, Any]]: - """Bundle mock for make_turn_handler — captures create_session kwargs.""" - captured: dict[str, Any] = {} - +def _make_mock_bundle_for_turn() -> tuple[MagicMock, MagicMock]: + """Bundle mock for make_turn_handler tests.""" mock_session = MagicMock() mock_session.metadata = {} mock_session.coordinator = MagicMock() @@ -194,111 +160,84 @@ def _make_mock_bundle_for_turn( mock_session.__aenter__ = AsyncMock(return_value=mock_session) mock_session.__aexit__ = AsyncMock(return_value=None) - async def _create_session(**kwargs: Any) -> MagicMock: - captured.update(kwargs) + async def _create_session(**_kwargs: Any) -> MagicMock: return mock_session mock_bundle = MagicMock() - mock_bundle.config = { - "tools": { - "tool-mcp": {"config": tool_mcp_static_config or {"verbose_servers": False, "max_content_size": 65536}} - } - } mock_bundle.mount_plan = {"agents": {}} mock_bundle.create_session = _create_session - return mock_bundle, captured + return mock_bundle, mock_session @pytest.mark.asyncio -async def test_make_turn_handler_threads_mcp_servers_to_tool_overrides() -> None: - """mcp_servers kwarg must reach create_session(tool_overrides=...) — CLI path.""" +async def test_make_turn_handler_mcp_config_path_sets_env() -> None: + """mcp_config_path kwarg must set AMPLIFIER_MCP_CONFIG — CLI path.""" from amplifier_agent_lib._runtime import make_turn_handler mcp_servers = {"test-mcp": {"transport": "stdio", "command": "/usr/bin/echo", "args": ["hi"]}} - mock_bundle, captured = _make_mock_bundle_for_turn() - - # Patch the streaming-hook mount so we don't drag the real coordinator in. - with patch("amplifier_agent_lib._runtime.SessionStore"), patch( - "amplifier_agent_lib.bundle.hook_streaming.mount", AsyncMock(return_value=None) - ): - handler = make_turn_handler( - mock_bundle, - cwd=None, - is_resumed=False, - mcp_servers=mcp_servers, - ) - ctx = MagicMock() - ctx.session_id = "sess-test-1" - ctx.turn_id = "turn-1" - ctx.prompt = "ping" - ctx.display = MagicMock() - ctx.display.emit = AsyncMock() - ctx.approval = MagicMock() - ctx.approval.request = AsyncMock() - await handler(ctx) - - assert "tool_overrides" in captured, ( - "make_turn_handler must pass tool_overrides to create_session " - "to mirror handle_initialize's wire-path behavior." - ) - tool_mcp_cfg = captured["tool_overrides"]["tool-mcp"]["config"] - assert tool_mcp_cfg["servers"] == mcp_servers, ( - f"tool_overrides['tool-mcp']['config']['servers'] should be {mcp_servers!r}, " - f"got {tool_mcp_cfg.get('servers')!r}" - ) - - -@pytest.mark.asyncio -async def test_make_turn_handler_merges_static_config_with_mcp_servers() -> None: - """Static bundle config (verbose_servers, max_content_size) must survive the merge.""" - from amplifier_agent_lib._runtime import make_turn_handler - - mcp_servers = {"nano-mcp": {"transport": "sse", "url": "http://localhost:9999"}} - static = {"verbose_servers": False, "max_content_size": 65536} - mock_bundle, captured = _make_mock_bundle_for_turn(tool_mcp_static_config=static) - - with patch("amplifier_agent_lib._runtime.SessionStore"), patch( - "amplifier_agent_lib.bundle.hook_streaming.mount", AsyncMock(return_value=None) - ): - handler = make_turn_handler( - mock_bundle, cwd=None, is_resumed=False, mcp_servers=mcp_servers - ) - ctx = MagicMock() - ctx.session_id = "sess-test-2" - ctx.turn_id = "turn-1" - ctx.prompt = "ping" - ctx.display = MagicMock() - ctx.display.emit = AsyncMock() - ctx.approval = MagicMock() - ctx.approval.request = AsyncMock() - await handler(ctx) - - tool_mcp_cfg = captured["tool_overrides"]["tool-mcp"]["config"] - assert tool_mcp_cfg.get("verbose_servers") is False - assert tool_mcp_cfg.get("max_content_size") == 65536 - assert tool_mcp_cfg.get("servers") == mcp_servers + config_path = _make_mcp_config_file(mcp_servers) + + try: + mock_bundle, _ = _make_mock_bundle_for_turn() + + with patch("amplifier_agent_lib._runtime.SessionStore"), patch( + "amplifier_agent_lib.bundle.hook_streaming.mount", AsyncMock(return_value=None) + ): + old_val = os.environ.pop("AMPLIFIER_MCP_CONFIG", None) + try: + handler = make_turn_handler( + mock_bundle, + cwd=None, + is_resumed=False, + mcp_config_path=config_path, + ) + # Env var should be set immediately (at handler-creation time, + # before the handler coroutine is awaited). + assert os.environ.get("AMPLIFIER_MCP_CONFIG") == config_path, ( + f"AMPLIFIER_MCP_CONFIG should be {config_path!r}, " + f"got {os.environ.get('AMPLIFIER_MCP_CONFIG')!r}" + ) + ctx = MagicMock() + ctx.session_id = "sess-test-1" + ctx.turn_id = "turn-1" + ctx.prompt = "ping" + ctx.display = MagicMock() + ctx.display.emit = AsyncMock() + ctx.approval = MagicMock() + ctx.approval.request = AsyncMock() + await handler(ctx) + finally: + if old_val is None: + os.environ.pop("AMPLIFIER_MCP_CONFIG", None) + else: + os.environ["AMPLIFIER_MCP_CONFIG"] = old_val + finally: + os.unlink(config_path) @pytest.mark.asyncio -async def test_make_turn_handler_default_mcp_servers_none_is_empty_dict() -> None: - """When mcp_servers is None (CLI flag omitted), tool_overrides still passes servers={}.""" +async def test_make_turn_handler_no_mcp_config_path_leaves_env_unchanged() -> None: + """When mcp_config_path is None, AMPLIFIER_MCP_CONFIG is not set.""" from amplifier_agent_lib._runtime import make_turn_handler - mock_bundle, captured = _make_mock_bundle_for_turn() + mock_bundle, _ = _make_mock_bundle_for_turn() with patch("amplifier_agent_lib._runtime.SessionStore"), patch( "amplifier_agent_lib.bundle.hook_streaming.mount", AsyncMock(return_value=None) ): - handler = make_turn_handler(mock_bundle, cwd=None, is_resumed=False) - ctx = MagicMock() - ctx.session_id = "sess-test-3" - ctx.turn_id = "turn-1" - ctx.prompt = "ping" - ctx.display = MagicMock() - ctx.display.emit = AsyncMock() - ctx.approval = MagicMock() - ctx.approval.request = AsyncMock() - await handler(ctx) - - assert "tool_overrides" in captured - assert captured["tool_overrides"]["tool-mcp"]["config"]["servers"] == {} + old_val = os.environ.pop("AMPLIFIER_MCP_CONFIG", None) + try: + handler = make_turn_handler(mock_bundle, cwd=None, is_resumed=False) + assert "AMPLIFIER_MCP_CONFIG" not in os.environ + ctx = MagicMock() + ctx.session_id = "sess-test-3" + ctx.turn_id = "turn-1" + ctx.prompt = "ping" + ctx.display = MagicMock() + ctx.display.emit = AsyncMock() + ctx.approval = MagicMock() + ctx.approval.request = AsyncMock() + await handler(ctx) + finally: + if old_val is not None: + os.environ["AMPLIFIER_MCP_CONFIG"] = old_val diff --git a/wrappers/typescript/dist/argv-builder.d.ts b/wrappers/typescript/dist/argv-builder.d.ts index bb266ca0..e707e0f3 100644 --- a/wrappers/typescript/dist/argv-builder.d.ts +++ b/wrappers/typescript/dist/argv-builder.d.ts @@ -23,11 +23,11 @@ export interface AssembleArgvInput { /** Provider override; emits `--provider `. */ providerOverride?: string; /** - * Pre-resolved value for `--mcp-servers`. Caller decides whether this is - * inline JSON or an `@/path/to/file.json` spill reference; argv-builder - * threads it through unchanged. + * Path to the MCP config JSON file, pre-spilled by `resolveMcpConfigPath`. + * Passed to the engine as `--mcp-config-path `; the engine sets + * `AMPLIFIER_MCP_CONFIG` so the tool-mcp module loads it during mount. */ - mcpServersFlag?: string; + mcpConfigPath?: string; /** Host capabilities object — emitted as `--host-capabilities `. */ hostCapabilities?: unknown; /** Allowlisted env variable names — emits `--env-allowlist `. */ diff --git a/wrappers/typescript/dist/argv-builder.js b/wrappers/typescript/dist/argv-builder.js index 506edabe..8ae6c546 100644 --- a/wrappers/typescript/dist/argv-builder.js +++ b/wrappers/typescript/dist/argv-builder.js @@ -26,8 +26,8 @@ export function assembleArgv(input) { if (input.providerOverride !== undefined) { argv.push("--provider", input.providerOverride); } - if (input.mcpServersFlag !== undefined) { - argv.push("--mcp-servers", input.mcpServersFlag); + if (input.mcpConfigPath !== undefined) { + argv.push("--mcp-config-path", input.mcpConfigPath); } if (input.hostCapabilities !== undefined) { argv.push("--host-capabilities", JSON.stringify(input.hostCapabilities)); diff --git a/wrappers/typescript/dist/index.d.ts b/wrappers/typescript/dist/index.d.ts index aa676bba..0793c37e 100644 --- a/wrappers/typescript/dist/index.d.ts +++ b/wrappers/typescript/dist/index.d.ts @@ -20,7 +20,7 @@ export type { McpServerConfig, HostCapabilities } from "./types.js"; * The protocol version that this TypeScript wrapper requires. * Forwarded to the engine via `--protocol-version` on every `submit()`. */ -export declare const PROTOCOL_VERSION_REQUIRED_BY_WRAPPER = "0.1.0"; +export declare const PROTOCOL_VERSION_REQUIRED_BY_WRAPPER = "0.2.0"; /** Parameters for spawnAgent(). Signature is locked verbatim by design §8.2. */ export interface SpawnAgentParams { /** 'burst' reserved; throws AaaError(lifecycle_unsupported) at runtime. */ diff --git a/wrappers/typescript/dist/index.js b/wrappers/typescript/dist/index.js index edb43dd6..0615e5d7 100644 --- a/wrappers/typescript/dist/index.js +++ b/wrappers/typescript/dist/index.js @@ -16,7 +16,7 @@ import { resolveBinaryPath, buildEnv, DEFAULT_ALLOWLIST } from "./spawn.js"; * The protocol version that this TypeScript wrapper requires. * Forwarded to the engine via `--protocol-version` on every `submit()`. */ -export const PROTOCOL_VERSION_REQUIRED_BY_WRAPPER = "0.1.0"; +export const PROTOCOL_VERSION_REQUIRED_BY_WRAPPER = "0.2.0"; // --------------------------------------------------------------------------- // spawnAgent() — locked public entry point (Mode A v2) // --------------------------------------------------------------------------- diff --git a/wrappers/typescript/dist/mcp-spill.d.ts b/wrappers/typescript/dist/mcp-spill.d.ts index 4194a851..ba68f076 100644 --- a/wrappers/typescript/dist/mcp-spill.d.ts +++ b/wrappers/typescript/dist/mcp-spill.d.ts @@ -1,40 +1,40 @@ /** - * Result of resolving the `--mcp-servers` flag value. + * Result of resolving the `--mcp-config-path` flag value. * - * - When `mcpServers` is null/undefined/empty: both fields are `null`. - * - When no server has a non-empty env block: `flag` is inline JSON, - * `spillPath` is `null` (no cleanup needed). - * - When any server has a non-empty env block: `flag` is `@`, - * `spillPath` points at the 0600 tmpfile (caller must cleanup). + * - When `mcpServers` is null/undefined/empty: `configPath` is `null`. + * - When servers are present: `configPath` points at the 0600 spill file. + * The file contains `{"mcpServers": }` in the format that + * amplifier-module-tool-mcp expects when reading `AMPLIFIER_MCP_CONFIG`. */ export interface McpSpillResult { - flag: string | null; - spillPath: string | null; + configPath: string | null; } /** - * Loose shape for an MCP server entry. We only inspect `env` here; the rest - * is threaded through untouched into the spilled JSON or inline payload. + * Loose shape for an MCP server entry. The full map is written verbatim + * into the spill file; no field is inspected beyond presence of the key. */ interface McpServerLike { - env?: Record | undefined; [k: string]: unknown; } type McpServersMap = Record; /** - * Resolve the value to pass for `--mcp-servers`. + * Resolve the MCP config file path to pass as `--mcp-config-path`. + * + * Always spills to a 0600 tmpfile. The file content wraps the server map + * in the top-level `mcpServers` key that amplifier-module-tool-mcp expects. * * @param mcpServers Map of server-id -> config, or null/undefined. * @param sessionId Session identifier; used as the per-session subdirectory * under the spill base so concurrent sessions never clash. * - * @returns A `McpSpillResult` with the flag value and (if spilled) the - * on-disk path for later cleanup. + * @returns A `McpSpillResult` with the on-disk config path (or null if there + * are no servers to spill). */ -export declare function resolveMcpServersFlag(mcpServers: McpServersMap | null | undefined, sessionId: string): Promise; +export declare function resolveMcpConfigPath(mcpServers: McpServersMap | null | undefined, sessionId: string): Promise; /** * Idempotently remove a spill file. Safe to call with `null` (no-op) and * safe to call when the file is already gone (ENOENT swallowed). Other * I/O errors propagate. */ -export declare function cleanupSpillFile(spillPath: string | null | undefined): Promise; +export declare function cleanupSpillFile(configPath: string | null | undefined): Promise; export {}; diff --git a/wrappers/typescript/dist/mcp-spill.js b/wrappers/typescript/dist/mcp-spill.js index 045e5c94..d760fca3 100644 --- a/wrappers/typescript/dist/mcp-spill.js +++ b/wrappers/typescript/dist/mcp-spill.js @@ -1,12 +1,12 @@ /** - * mcp-spill.ts — secret-aware MCP servers config resolution (CR-A). + * mcp-spill.ts — MCP servers config path resolution (CR-A, protocol 0.2.0). * - * A3'/CR-A: When forwarding `--mcp-servers` to the engine binary, the wrapper - * must avoid placing secret-bearing env blocks on the command line. If any - * server in the config has a non-empty `env` block, the full JSON is spilled - * to a 0600 tmpfile under `${XDG_RUNTIME_DIR || os.tmpdir()}/amplifier-agent//` - * and the flag value is `@`. When no server has env, the JSON is inlined - * directly (no spill, no cleanup needed). + * The wrapper always spills the MCP server map to a 0600 tmpfile under + * `${XDG_RUNTIME_DIR || os.tmpdir()}/amplifier-agent//mcp.json`. + * The file is written in the format documented by amplifier-module-tool-mcp: + * a top-level `{"mcpServers": }` object. The engine receives the plain + * file path via `--mcp-config-path` and sets `AMPLIFIER_MCP_CONFIG`; the + * module reads it via its standard config discovery (config.py priority chain). * * `cleanupSpillFile` is the matching teardown — idempotent unlink that * swallows ENOENT so callers can call it unconditionally on every exit path. @@ -14,23 +14,6 @@ import { mkdir, writeFile, unlink } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; -/** - * Return true when at least one server has a non-empty `env` block. - * An empty object (`{}`) does NOT trigger spilling — only env blocks with - * at least one key are considered secret-bearing. - */ -function anyServerHasEnv(mcpServers) { - for (const key of Object.keys(mcpServers)) { - const server = mcpServers[key]; - if (!server) - continue; - const env = server.env; - if (env && typeof env === "object" && Object.keys(env).length > 0) { - return true; - } - } - return false; -} /** * Compute the base directory for spill files. Prefers * `$XDG_RUNTIME_DIR/amplifier-agent` (typically a tmpfs on Linux) and falls @@ -44,40 +27,43 @@ function spillBaseDir() { return join(tmpdir(), "amplifier-agent"); } /** - * Resolve the value to pass for `--mcp-servers`. + * Resolve the MCP config file path to pass as `--mcp-config-path`. + * + * Always spills to a 0600 tmpfile. The file content wraps the server map + * in the top-level `mcpServers` key that amplifier-module-tool-mcp expects. * * @param mcpServers Map of server-id -> config, or null/undefined. * @param sessionId Session identifier; used as the per-session subdirectory * under the spill base so concurrent sessions never clash. * - * @returns A `McpSpillResult` with the flag value and (if spilled) the - * on-disk path for later cleanup. + * @returns A `McpSpillResult` with the on-disk config path (or null if there + * are no servers to spill). */ -export async function resolveMcpServersFlag(mcpServers, sessionId) { +export async function resolveMcpConfigPath(mcpServers, sessionId) { if (!mcpServers || Object.keys(mcpServers).length === 0) { - return { flag: null, spillPath: null }; - } - if (!anyServerHasEnv(mcpServers)) { - // No secrets — safe to inline as a JSON string. - return { flag: JSON.stringify(mcpServers), spillPath: null }; + return { configPath: null }; } - // Secret-bearing: spill to a 0600 tmpfile under a 0700 per-session dir. + // Always spill to a 0600 tmpfile under a 0700 per-session dir. + // Wrap the server map in the top-level "mcpServers" key that the module + // expects when reading AMPLIFIER_MCP_CONFIG (see tool-mcp/config.py). const dir = join(spillBaseDir(), sessionId); await mkdir(dir, { recursive: true, mode: 0o700 }); const filePath = join(dir, "mcp.json"); - await writeFile(filePath, JSON.stringify(mcpServers), { mode: 0o600 }); - return { flag: `@${filePath}`, spillPath: filePath }; + await writeFile(filePath, JSON.stringify({ mcpServers: mcpServers }), { + mode: 0o600, + }); + return { configPath: filePath }; } /** * Idempotently remove a spill file. Safe to call with `null` (no-op) and * safe to call when the file is already gone (ENOENT swallowed). Other * I/O errors propagate. */ -export async function cleanupSpillFile(spillPath) { - if (!spillPath) +export async function cleanupSpillFile(configPath) { + if (!configPath) return; try { - await unlink(spillPath); + await unlink(configPath); } catch (err) { const code = err.code; diff --git a/wrappers/typescript/dist/session.d.ts b/wrappers/typescript/dist/session.d.ts index 7ed23130..09317266 100644 --- a/wrappers/typescript/dist/session.d.ts +++ b/wrappers/typescript/dist/session.d.ts @@ -89,7 +89,7 @@ export interface SessionHandleParams { resume?: boolean; /** Working directory for the subprocess. */ cwd?: string; - /** MCP servers to forward via `--mcp-servers` (CR-A spill applies). */ + /** MCP servers to forward via `--mcp-config-path` (CR-A spill applies). */ mcpServers?: Record; /** Host capabilities forwarded via `--host-capabilities`. */ hostCapabilities?: HostCapabilities; @@ -133,7 +133,7 @@ export declare class SessionHandle { /** * Async generator implementing the §5.2 iterable behavior: * (i) yield `{type:'init', sessionId}` synchronously (SC-1); - * (ii) CR-A: resolve `--mcp-servers` flag (spill if env-bearing); + * (ii) CR-A: resolve `--mcp-config-path` (always spill to tmpfile); * (iii) build argv via `assembleArgv`; * (iv) SC-B: spawn with `detached:true` so PID == PGID for group signals; * (v) accumulate stdout/stderr from chunks; diff --git a/wrappers/typescript/dist/session.js b/wrappers/typescript/dist/session.js index 23b541b0..e6454db7 100644 --- a/wrappers/typescript/dist/session.js +++ b/wrappers/typescript/dist/session.js @@ -24,7 +24,7 @@ */ import { spawn as childSpawn } from "node:child_process"; import { assembleArgv } from "./argv-builder.js"; -import { resolveMcpServersFlag, cleanupSpillFile } from "./mcp-spill.js"; +import { resolveMcpConfigPath, cleanupSpillFile } from "./mcp-spill.js"; import { parseRunOutput, STDERR_TAIL_BYTES } from "./run-output-parser.js"; /** Typed error for AaA wrapper lifecycle and protocol violations. */ export class AaaError extends Error { @@ -121,7 +121,7 @@ export class SessionHandle { /** * Async generator implementing the §5.2 iterable behavior: * (i) yield `{type:'init', sessionId}` synchronously (SC-1); - * (ii) CR-A: resolve `--mcp-servers` flag (spill if env-bearing); + * (ii) CR-A: resolve `--mcp-config-path` (always spill to tmpfile); * (iii) build argv via `assembleArgv`; * (iv) SC-B: spawn with `detached:true` so PID == PGID for group signals; * (v) accumulate stdout/stderr from chunks; @@ -135,13 +135,13 @@ export class SessionHandle { async *makeIterable(prompt) { // (i) SC-1: yield init synchronously, BEFORE any async work. yield { type: "init", sessionId: this.params.sessionId }; - // (ii) CR-A: resolve --mcp-servers (inline JSON OR spill to 0600 tmpfile). + // (ii) CR-A: resolve --mcp-config-path (always spill to 0600 tmpfile). // Cast through `unknown`: McpServerConfig is the schema-validated wire type - // (no index signature), while resolveMcpServersFlag's McpServerLike has an - // open index signature. The two are runtime-compatible — only `env` is - // inspected — but TS rightly rejects assignability without the cast. - const spill = await resolveMcpServersFlag((this.params.mcpServers ?? null), this.params.sessionId); - this.mcpSpillPath = spill.spillPath; + // (no index signature), while resolveMcpConfigPath's McpServerLike has an + // open index signature. The two are runtime-compatible but TS rightly + // rejects assignability without the cast. + const spill = await resolveMcpConfigPath((this.params.mcpServers ?? null), this.params.sessionId); + this.mcpSpillPath = spill.configPath; // (iii) build argv (pure function — no I/O). const argv = assembleArgv({ sessionId: this.params.sessionId, @@ -150,7 +150,7 @@ export class SessionHandle { resume: this.params.resume, cwd: this.params.cwd, providerOverride: this.params.providerOverride, - mcpServersFlag: spill.flag ?? undefined, + mcpConfigPath: spill.configPath ?? undefined, hostCapabilities: this.params.hostCapabilities, envAllowlist: this.params.envAllowlist, envExtra: this.params.envExtra, diff --git a/wrappers/typescript/package.json b/wrappers/typescript/package.json index 9588dcb4..ed0d741a 100644 --- a/wrappers/typescript/package.json +++ b/wrappers/typescript/package.json @@ -1,6 +1,6 @@ { "name": "amplifier-agent-ts", - "version": "0.3.1", + "version": "0.4.0", "description": "TypeScript SDK for the Amplifier agent. Spawns and drives the amplifier-agent Python CLI over a stdio protocol.", "type": "module", "exports": { diff --git a/wrappers/typescript/src/argv-builder.ts b/wrappers/typescript/src/argv-builder.ts index dc85031c..9371315e 100644 --- a/wrappers/typescript/src/argv-builder.ts +++ b/wrappers/typescript/src/argv-builder.ts @@ -24,11 +24,11 @@ export interface AssembleArgvInput { /** Provider override; emits `--provider `. */ providerOverride?: string; /** - * Pre-resolved value for `--mcp-servers`. Caller decides whether this is - * inline JSON or an `@/path/to/file.json` spill reference; argv-builder - * threads it through unchanged. + * Path to the MCP config JSON file, pre-spilled by `resolveMcpConfigPath`. + * Passed to the engine as `--mcp-config-path `; the engine sets + * `AMPLIFIER_MCP_CONFIG` so the tool-mcp module loads it during mount. */ - mcpServersFlag?: string; + mcpConfigPath?: string; /** Host capabilities object — emitted as `--host-capabilities `. */ hostCapabilities?: unknown; /** Allowlisted env variable names — emits `--env-allowlist `. */ @@ -58,8 +58,8 @@ export function assembleArgv(input: AssembleArgvInput): string[] { if (input.providerOverride !== undefined) { argv.push("--provider", input.providerOverride); } - if (input.mcpServersFlag !== undefined) { - argv.push("--mcp-servers", input.mcpServersFlag); + if (input.mcpConfigPath !== undefined) { + argv.push("--mcp-config-path", input.mcpConfigPath); } if (input.hostCapabilities !== undefined) { argv.push("--host-capabilities", JSON.stringify(input.hostCapabilities)); diff --git a/wrappers/typescript/src/index.ts b/wrappers/typescript/src/index.ts index c628f4f9..a7a127ca 100644 --- a/wrappers/typescript/src/index.ts +++ b/wrappers/typescript/src/index.ts @@ -32,7 +32,7 @@ export type { McpServerConfig, HostCapabilities } from "./types.js"; * The protocol version that this TypeScript wrapper requires. * Forwarded to the engine via `--protocol-version` on every `submit()`. */ -export const PROTOCOL_VERSION_REQUIRED_BY_WRAPPER = "0.1.0"; +export const PROTOCOL_VERSION_REQUIRED_BY_WRAPPER = "0.2.0"; // --------------------------------------------------------------------------- // SpawnAgentParams — locked public API (design §8.2, amended for Mode A v2) diff --git a/wrappers/typescript/src/mcp-spill.ts b/wrappers/typescript/src/mcp-spill.ts index 553bd243..125e0b29 100644 --- a/wrappers/typescript/src/mcp-spill.ts +++ b/wrappers/typescript/src/mcp-spill.ts @@ -1,12 +1,12 @@ /** - * mcp-spill.ts — secret-aware MCP servers config resolution (CR-A). + * mcp-spill.ts — MCP servers config path resolution (CR-A, protocol 0.2.0). * - * A3'/CR-A: When forwarding `--mcp-servers` to the engine binary, the wrapper - * must avoid placing secret-bearing env blocks on the command line. If any - * server in the config has a non-empty `env` block, the full JSON is spilled - * to a 0600 tmpfile under `${XDG_RUNTIME_DIR || os.tmpdir()}/amplifier-agent//` - * and the flag value is `@`. When no server has env, the JSON is inlined - * directly (no spill, no cleanup needed). + * The wrapper always spills the MCP server map to a 0600 tmpfile under + * `${XDG_RUNTIME_DIR || os.tmpdir()}/amplifier-agent//mcp.json`. + * The file is written in the format documented by amplifier-module-tool-mcp: + * a top-level `{"mcpServers": }` object. The engine receives the plain + * file path via `--mcp-config-path` and sets `AMPLIFIER_MCP_CONFIG`; the + * module reads it via its standard config discovery (config.py priority chain). * * `cleanupSpillFile` is the matching teardown — idempotent unlink that * swallows ENOENT so callers can call it unconditionally on every exit path. @@ -16,47 +16,27 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; /** - * Result of resolving the `--mcp-servers` flag value. + * Result of resolving the `--mcp-config-path` flag value. * - * - When `mcpServers` is null/undefined/empty: both fields are `null`. - * - When no server has a non-empty env block: `flag` is inline JSON, - * `spillPath` is `null` (no cleanup needed). - * - When any server has a non-empty env block: `flag` is `@`, - * `spillPath` points at the 0600 tmpfile (caller must cleanup). + * - When `mcpServers` is null/undefined/empty: `configPath` is `null`. + * - When servers are present: `configPath` points at the 0600 spill file. + * The file contains `{"mcpServers": }` in the format that + * amplifier-module-tool-mcp expects when reading `AMPLIFIER_MCP_CONFIG`. */ export interface McpSpillResult { - flag: string | null; - spillPath: string | null; + configPath: string | null; } /** - * Loose shape for an MCP server entry. We only inspect `env` here; the rest - * is threaded through untouched into the spilled JSON or inline payload. + * Loose shape for an MCP server entry. The full map is written verbatim + * into the spill file; no field is inspected beyond presence of the key. */ interface McpServerLike { - env?: Record | undefined; [k: string]: unknown; } type McpServersMap = Record; -/** - * Return true when at least one server has a non-empty `env` block. - * An empty object (`{}`) does NOT trigger spilling — only env blocks with - * at least one key are considered secret-bearing. - */ -function anyServerHasEnv(mcpServers: McpServersMap): boolean { - for (const key of Object.keys(mcpServers)) { - const server = mcpServers[key]; - if (!server) continue; - const env = server.env; - if (env && typeof env === "object" && Object.keys(env).length > 0) { - return true; - } - } - return false; -} - /** * Compute the base directory for spill files. Prefers * `$XDG_RUNTIME_DIR/amplifier-agent` (typically a tmpfs on Linux) and falls @@ -71,35 +51,37 @@ function spillBaseDir(): string { } /** - * Resolve the value to pass for `--mcp-servers`. + * Resolve the MCP config file path to pass as `--mcp-config-path`. + * + * Always spills to a 0600 tmpfile. The file content wraps the server map + * in the top-level `mcpServers` key that amplifier-module-tool-mcp expects. * * @param mcpServers Map of server-id -> config, or null/undefined. * @param sessionId Session identifier; used as the per-session subdirectory * under the spill base so concurrent sessions never clash. * - * @returns A `McpSpillResult` with the flag value and (if spilled) the - * on-disk path for later cleanup. + * @returns A `McpSpillResult` with the on-disk config path (or null if there + * are no servers to spill). */ -export async function resolveMcpServersFlag( +export async function resolveMcpConfigPath( mcpServers: McpServersMap | null | undefined, sessionId: string, ): Promise { if (!mcpServers || Object.keys(mcpServers).length === 0) { - return { flag: null, spillPath: null }; - } - - if (!anyServerHasEnv(mcpServers)) { - // No secrets — safe to inline as a JSON string. - return { flag: JSON.stringify(mcpServers), spillPath: null }; + return { configPath: null }; } - // Secret-bearing: spill to a 0600 tmpfile under a 0700 per-session dir. + // Always spill to a 0600 tmpfile under a 0700 per-session dir. + // Wrap the server map in the top-level "mcpServers" key that the module + // expects when reading AMPLIFIER_MCP_CONFIG (see tool-mcp/config.py). const dir = join(spillBaseDir(), sessionId); await mkdir(dir, { recursive: true, mode: 0o700 }); const filePath = join(dir, "mcp.json"); - await writeFile(filePath, JSON.stringify(mcpServers), { mode: 0o600 }); + await writeFile(filePath, JSON.stringify({ mcpServers: mcpServers }), { + mode: 0o600, + }); - return { flag: `@${filePath}`, spillPath: filePath }; + return { configPath: filePath }; } /** @@ -108,11 +90,11 @@ export async function resolveMcpServersFlag( * I/O errors propagate. */ export async function cleanupSpillFile( - spillPath: string | null | undefined, + configPath: string | null | undefined, ): Promise { - if (!spillPath) return; + if (!configPath) return; try { - await unlink(spillPath); + await unlink(configPath); } catch (err) { const code = (err as NodeJS.ErrnoException).code; if (code === "ENOENT") return; diff --git a/wrappers/typescript/src/session.ts b/wrappers/typescript/src/session.ts index f63bbba5..926d50a7 100644 --- a/wrappers/typescript/src/session.ts +++ b/wrappers/typescript/src/session.ts @@ -27,7 +27,7 @@ import { spawn as childSpawn } from "node:child_process"; import type { ChildProcess } from "node:child_process"; import { assembleArgv } from "./argv-builder.js"; -import { resolveMcpServersFlag, cleanupSpillFile } from "./mcp-spill.js"; +import { resolveMcpConfigPath, cleanupSpillFile } from "./mcp-spill.js"; import { parseRunOutput, STDERR_TAIL_BYTES } from "./run-output-parser.js"; import type { McpServerConfig, HostCapabilities } from "./types.js"; @@ -111,7 +111,7 @@ export interface SessionHandleParams { resume?: boolean; /** Working directory for the subprocess. */ cwd?: string; - /** MCP servers to forward via `--mcp-servers` (CR-A spill applies). */ + /** MCP servers to forward via `--mcp-config-path` (CR-A spill applies). */ mcpServers?: Record; /** Host capabilities forwarded via `--host-capabilities`. */ hostCapabilities?: HostCapabilities; @@ -211,7 +211,7 @@ export class SessionHandle { /** * Async generator implementing the §5.2 iterable behavior: * (i) yield `{type:'init', sessionId}` synchronously (SC-1); - * (ii) CR-A: resolve `--mcp-servers` flag (spill if env-bearing); + * (ii) CR-A: resolve `--mcp-config-path` (always spill to tmpfile); * (iii) build argv via `assembleArgv`; * (iv) SC-B: spawn with `detached:true` so PID == PGID for group signals; * (v) accumulate stdout/stderr from chunks; @@ -226,18 +226,18 @@ export class SessionHandle { // (i) SC-1: yield init synchronously, BEFORE any async work. yield { type: "init", sessionId: this.params.sessionId }; - // (ii) CR-A: resolve --mcp-servers (inline JSON OR spill to 0600 tmpfile). + // (ii) CR-A: resolve --mcp-config-path (always spill to 0600 tmpfile). // Cast through `unknown`: McpServerConfig is the schema-validated wire type - // (no index signature), while resolveMcpServersFlag's McpServerLike has an - // open index signature. The two are runtime-compatible — only `env` is - // inspected — but TS rightly rejects assignability without the cast. - const spill = await resolveMcpServersFlag( + // (no index signature), while resolveMcpConfigPath's McpServerLike has an + // open index signature. The two are runtime-compatible but TS rightly + // rejects assignability without the cast. + const spill = await resolveMcpConfigPath( (this.params.mcpServers ?? null) as unknown as Parameters< - typeof resolveMcpServersFlag + typeof resolveMcpConfigPath >[0], this.params.sessionId, ); - this.mcpSpillPath = spill.spillPath; + this.mcpSpillPath = spill.configPath; // (iii) build argv (pure function — no I/O). const argv = assembleArgv({ @@ -247,7 +247,7 @@ export class SessionHandle { resume: this.params.resume, cwd: this.params.cwd, providerOverride: this.params.providerOverride, - mcpServersFlag: spill.flag ?? undefined, + mcpConfigPath: spill.configPath ?? undefined, hostCapabilities: this.params.hostCapabilities, envAllowlist: this.params.envAllowlist, envExtra: this.params.envExtra, diff --git a/wrappers/typescript/test/argv-builder.test.ts b/wrappers/typescript/test/argv-builder.test.ts index c7315ed0..79cc0a04 100644 --- a/wrappers/typescript/test/argv-builder.test.ts +++ b/wrappers/typescript/test/argv-builder.test.ts @@ -1,12 +1,11 @@ /** * Tests for argv-builder.ts: assembleArgv() * - * TDD cases (task-5): + * TDD cases (task-5 / protocol 0.2.0): * (i) happy path minimal session — exact argv array * (ii) resume mode replaces --fresh with --resume * (iii) --host-capabilities threaded as JSON string and parseable - * (iv) --mcp-servers threaded as inline JSON when no env spill - * (v) --mcp-servers @path threaded when caller pre-spilled + * (iv) --mcp-config-path threaded as plain path */ import { describe, it, expect } from "vitest"; import { assembleArgv } from "../src/argv-builder.js"; @@ -62,31 +61,17 @@ describe("assembleArgv", () => { expect(JSON.parse(jsonArg as string)).toEqual(caps); }); - it("(iv) --mcp-servers threaded as inline JSON when no env spill", () => { - const inlineJson = '{"servers":[{"id":"a","command":"foo"}]}'; + it("(iv) --mcp-config-path threaded as plain path", () => { + const configPath = "/tmp/amplifier-agent/sess-abc/mcp.json"; const input: AssembleArgvInput = { sessionId: "sid", prompt: "hello", protocolVersion: "0.1.0", - mcpServersFlag: inlineJson, + mcpConfigPath: configPath, }; const argv = assembleArgv(input); - const idx = argv.indexOf("--mcp-servers"); + const idx = argv.indexOf("--mcp-config-path"); expect(idx).toBeGreaterThanOrEqual(0); - expect(argv[idx + 1]).toBe(inlineJson); - }); - - it("(v) --mcp-servers @path threaded when caller pre-spilled", () => { - const spilled = "@/tmp/aaa-mcp-servers-abc.json"; - const input: AssembleArgvInput = { - sessionId: "sid", - prompt: "hello", - protocolVersion: "0.1.0", - mcpServersFlag: spilled, - }; - const argv = assembleArgv(input); - const idx = argv.indexOf("--mcp-servers"); - expect(idx).toBeGreaterThanOrEqual(0); - expect(argv[idx + 1]).toBe(spilled); + expect(argv[idx + 1]).toBe(configPath); }); }); diff --git a/wrappers/typescript/test/mcp-spill.test.ts b/wrappers/typescript/test/mcp-spill.test.ts index b61d00fb..180ce0a3 100644 --- a/wrappers/typescript/test/mcp-spill.test.ts +++ b/wrappers/typescript/test/mcp-spill.test.ts @@ -1,12 +1,12 @@ /** - * Tests for mcp-spill.ts: resolveMcpServersFlag() and cleanupSpillFile() + * Tests for mcp-spill.ts: resolveMcpConfigPath() and cleanupSpillFile() * - * TDD cases (task-6 / A3'/CR-A): - * (i) null/undefined mcpServers returns { flag: null, spillPath: null } - * (ii) no server has non-empty env block -> inline JSON, no spill file - * (flag does not start with '@') - * (iii) any server has non-empty env block -> spill to tmpfile, flag is - * `@`, file contains full config, mode is 0600 + * TDD cases (protocol 0.2.0 — always-spill, wrapped format): + * (i) null/undefined/empty mcpServers returns { configPath: null } + * (ii) non-empty mcpServers always spills to tmpfile; file contains + * { mcpServers: } (top-level wrapper required by tool-mcp); + * file mode is 0600; no inline JSON branch. + * (iii) configPath is a plain path (no '@' prefix) * (iv) cleanupSpillFile is idempotent (ENOENT is fine) */ import { describe, it, expect, afterEach } from "vitest"; @@ -15,7 +15,7 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { - resolveMcpServersFlag, + resolveMcpConfigPath, cleanupSpillFile, } from "../src/mcp-spill.js"; import type { McpSpillResult } from "../src/mcp-spill.js"; @@ -38,37 +38,40 @@ afterEach(async () => { } }); -describe("resolveMcpServersFlag", () => { - it("(i) returns {null, null} for null mcpServers", async () => { - const result: McpSpillResult = await resolveMcpServersFlag(null, SID); - expect(result).toEqual({ flag: null, spillPath: null }); +describe("resolveMcpConfigPath", () => { + it("(i) returns {configPath: null} for null mcpServers", async () => { + const result: McpSpillResult = await resolveMcpConfigPath(null, SID); + expect(result).toEqual({ configPath: null }); }); - it("(i) returns {null, null} for undefined mcpServers", async () => { - const result: McpSpillResult = await resolveMcpServersFlag(undefined, SID); - expect(result).toEqual({ flag: null, spillPath: null }); + it("(i) returns {configPath: null} for undefined mcpServers", async () => { + const result: McpSpillResult = await resolveMcpConfigPath(undefined, SID); + expect(result).toEqual({ configPath: null }); }); - it("(i) returns {null, null} for empty mcpServers object", async () => { - const result: McpSpillResult = await resolveMcpServersFlag({}, SID); - expect(result).toEqual({ flag: null, spillPath: null }); + it("(i) returns {configPath: null} for empty mcpServers object", async () => { + const result: McpSpillResult = await resolveMcpConfigPath({}, SID); + expect(result).toEqual({ configPath: null }); }); - it("(ii) inlines JSON when no server has a non-empty env block", async () => { + it("(ii) always spills to tmpfile, even when no server has an env block", async () => { const mcpServers = { alpha: { command: "echo", args: ["hi"] }, - // env present but empty -> still considered "no env" for spill purposes - beta: { command: "true", env: {} }, + beta: { command: "true" }, }; - const result = await resolveMcpServersFlag(mcpServers, SID); - expect(result.spillPath).toBeNull(); - expect(result.flag).not.toBeNull(); - // Inline JSON: must NOT start with '@' - expect(result.flag!.startsWith("@")).toBe(false); - expect(JSON.parse(result.flag!)).toEqual(mcpServers); + const result = await resolveMcpConfigPath(mcpServers, SID); + expect(result.configPath).not.toBeNull(); + created.push(result.configPath!); + + // configPath is a plain path — no '@' prefix + expect(result.configPath!.startsWith("@")).toBe(false); + + // File content wraps the server map in top-level "mcpServers" key + const contents = await readFile(result.configPath!, "utf8"); + expect(JSON.parse(contents)).toEqual({ mcpServers }); }); - it("(iii) spills to tmpfile with 0600 mode when any server has a non-empty env block", async () => { + it("(ii) spills with 0600 mode when servers have env blocks", async () => { const mcpServers = { alpha: { command: "echo" }, secret: { @@ -76,31 +79,36 @@ describe("resolveMcpServersFlag", () => { env: { API_KEY: "super-secret-value" }, }, }; - const result = await resolveMcpServersFlag(mcpServers, SID); - expect(result.spillPath).not.toBeNull(); - expect(result.flag).not.toBeNull(); - created.push(result.spillPath!); - - // Flag should be '@' - expect(result.flag).toBe(`@${result.spillPath}`); - expect(result.flag!.startsWith("@")).toBe(true); + const result = await resolveMcpConfigPath(mcpServers, SID); + expect(result.configPath).not.toBeNull(); + created.push(result.configPath!); - // File contents should be the full mcpServers config - const contents = await readFile(result.spillPath!, "utf8"); - expect(JSON.parse(contents)).toEqual(mcpServers); + // File contents should be wrapped in top-level "mcpServers" + const contents = await readFile(result.configPath!, "utf8"); + expect(JSON.parse(contents)).toEqual({ mcpServers }); // File mode should be 0600 (owner read/write only) - const st = await stat(result.spillPath!); - // Mask out file-type bits, keep permission bits only + const st = await stat(result.configPath!); const mode = st.mode & 0o777; expect(mode).toBe(0o600); }); + it("(iii) configPath is a plain path under the per-session spill dir", async () => { + const mcpServers = { srv: { command: "node" } }; + const result = await resolveMcpConfigPath(mcpServers, SID); + expect(result.configPath).not.toBeNull(); + created.push(result.configPath!); + + // Must not start with '@' (plain path, not an @-prefixed spill reference) + expect(result.configPath!.startsWith("@")).toBe(false); + // Must end with mcp.json under the session dir + expect(result.configPath!).toMatch(/amplifier-agent[/\\]test-session-abc[/\\]mcp\.json$/); + }); + it("(iv) cleanupSpillFile is idempotent — second call on missing file does not throw", async () => { // Create a file we know exists, then cleanup twice. const dir = await mkdtemp(join(tmpdir(), "mcp-spill-cleanup-")); const path = join(dir, "mcp.json"); - // write a dummy file const { writeFile } = await import("node:fs/promises"); await writeFile(path, "{}", { mode: 0o600 }); @@ -110,7 +118,6 @@ describe("resolveMcpServersFlag", () => { // Second cleanup on missing path must not throw (ENOENT swallowed) await expect(cleanupSpillFile(path)).resolves.toBeUndefined(); - // remove tmp dir await rm(dir, { recursive: true, force: true }); });