From 8e3e7715c6c1143a3560ed2ddbca5c5e69f9fdeb Mon Sep 17 00:00:00 2001 From: Tamaghna Sarkar Date: Thu, 3 Sep 2026 19:29:24 +0000 Subject: [PATCH 1/5] feat(mcp): expose Extra as an MCP server over stdio --- examples/mcp-server/README.md | 98 ++++ examples/mcp-server/agents.yaml | 33 ++ examples/mcp-server/client.py | 78 +++ examples/mcp-server/plugins/__init__.py | 3 + examples/mcp-server/plugins/plugins.toml | 39 ++ examples/mcp-server/plugins/tools/__init__.py | 3 + examples/mcp-server/plugins/tools/echo.py | 3 + .../mcp-server/prompts/echo_agent/system.md | 3 + src/agentctl/main.py | 23 + src/agentctl/mcp_serve.py | 128 +++++ tests/cli/test_mcp_serve_command.py | 510 ++++++++++++++++++ 11 files changed, 921 insertions(+) create mode 100644 examples/mcp-server/README.md create mode 100644 examples/mcp-server/agents.yaml create mode 100644 examples/mcp-server/client.py create mode 100644 examples/mcp-server/plugins/__init__.py create mode 100644 examples/mcp-server/plugins/plugins.toml create mode 100644 examples/mcp-server/plugins/tools/__init__.py create mode 100644 examples/mcp-server/plugins/tools/echo.py create mode 100644 examples/mcp-server/prompts/echo_agent/system.md create mode 100644 src/agentctl/mcp_serve.py create mode 100644 tests/cli/test_mcp_serve_command.py diff --git a/examples/mcp-server/README.md b/examples/mcp-server/README.md new file mode 100644 index 00000000..67c631a1 --- /dev/null +++ b/examples/mcp-server/README.md @@ -0,0 +1,98 @@ +# MCP server example + +The smallest possible Extra system, exposed as an MCP server over stdio. It +exists to demonstrate that ``agentctl mcp serve`` works end-to-end: + +1. an MCP client can discover the ``extra_chat`` tool, +2. the client can send a message and get an answer back, +3. the same ``session_id`` continues a previous conversation, +4. different ``session_id`` values keep their own history, +5. the Extra engine is built once and reused for every request. + +## Files + +``` +examples/mcp-server/ +├── agents.yaml # one-agent system +├── prompts/echo_agent/system.md +├── plugins/ # generated stub + implementation +│ ├── plugins.toml +│ └── tools/echo.py +└── client.py # tiny MCP client that drives the server +``` + +## Run it + +The example uses Ollama through the OpenAI-compatible API by default, just +like `examples/starter`. Make sure Ollama is running and the model is pulled: + +```bash +ollama pull qwen2.5:14b +``` + +Then start the server in one terminal: + +```bash +agentctl mcp serve --config examples/mcp-server/agents.yaml +``` + +And run the bundled client in another: + +```bash +python examples/mcp-server/client.py +``` + +If you want to use a different model, change `defaults.model.name` in +`agents.yaml` and set the corresponding provider key in your environment +(`OPENAI_API_KEY`, `ANTHROPIC_API_KEY`, etc.). + +The server speaks MCP over stdio, so any MCP-aware client can point at it. +For example, to wire it into Claude Desktop, add an entry like: + +```json +{ + "mcpServers": { + "extra": { + "command": "agentctl", + "args": ["mcp", "serve", "--config", "/abs/path/to/agents.yaml"] + } + } +} +``` + +## What the tool looks like + +The server exposes a single tool, ``extra_chat``: + +```json +{ + "message": "Search the internal documentation", + "session_id": "optional-session-id", + "user_id": "optional-user-id" +} +``` + +Response: + +```json +{ + "session_id": "abc123", + "answer": "The relevant documentation is...", + "visited": ["root", "knowledge_agent"], + "used_tools": [{"name": "search_internal_documents", "provider": "local"}] +} +``` + +## How it works + +The MCP layer in ``agentctl/mcp_serve.py`` does only five things: + +1. validate the YAML spec, +2. open the existing application repositories (one process-lifetime DB), +3. build the existing ``LangGraphEngine`` once, +4. on each tool call, create or load a session and call the existing + ``ConversationService.send``, +5. return the ``RunResult`` as an MCP ``TextContent`` block. + +Routing, tool execution, approvals, hooks, and access control are not +re-implemented — they continue to live in the Extra runtime. \ No newline at end of file diff --git a/examples/mcp-server/agents.yaml b/examples/mcp-server/agents.yaml new file mode 100644 index 00000000..a54d3d13 --- /dev/null +++ b/examples/mcp-server/agents.yaml @@ -0,0 +1,33 @@ +system: + name: Echo Agent MCP Server + +defaults: + model: + provider: openai + name: qwen2.5:14b + temperature: 0.0 + +execution: + max_iterations: 4 + max_tool_calls: 2 + max_tool_calls_per_agent: 2 + max_child_agent_calls: 1 + allow_duplicate_tool_calls: false + +tools: + echo: + description: Echoes the input back. Useful for verifying the MCP wiring. + +plugins: + import_roots: ["."] + +agents: + echo_agent: + name: Echo Agent + description: A minimal agent that answers questions directly. + prompts: + system: prompts/echo_agent/system.md + tools: [echo] + +graph: + echo_agent: \ No newline at end of file diff --git a/examples/mcp-server/client.py b/examples/mcp-server/client.py new file mode 100644 index 00000000..cf86d374 --- /dev/null +++ b/examples/mcp-server/client.py @@ -0,0 +1,78 @@ +"""Tiny MCP client that talks to ``agentctl mcp serve``. + +Start the server in one terminal:: + + agentctl mcp serve --config examples/mcp-server/agents.yaml + +Then run this in another:: + + python examples/mcp-server/client.py + +It discovers the ``extra_chat`` tool, sends a message, follows up in the +same session, and verifies that two different sessions keep independent +history. +""" + +from __future__ import annotations + +import asyncio +import json +import sys + +from mcp import ClientSession, StdioServerParameters +from mcp.client.stdio import stdio_client + + +async def call_chat( + session: ClientSession, message: str, session_id: str | None = None +) -> dict: + args: dict[str, str] = {"message": message} + if session_id: + args["session_id"] = session_id + result = await session.call_tool("extra_chat", args) + if result.isError: + raise RuntimeError(f"tool call failed: {result.content}") + for block in result.content: + if getattr(block, "text", None): + return json.loads(block.text) + raise RuntimeError("no text in tool response") + + +async def main() -> None: + server_cmd = [ + sys.executable, + "-m", + "agentctl", + "mcp", + "serve", + "--config", + "examples/mcp-server/agents.yaml", + ] + params = StdioServerParameters(command=server_cmd[0], args=server_cmd[1:]) + + async with stdio_client(params) as (read, write): + async with ClientSession(read, write) as session: + await session.initialize() + + tools = await session.list_tools() + names = [t.name for t in tools.tools] + print(f"discovered tools: {names}") + assert "extra_chat" in names + + first = await call_chat(session, "hello there") + sid = first["session_id"] + print(f"first sid={sid} answer={first['answer']!r}") + + second = await call_chat(session, "what did I just say?", session_id=sid) + print( + f"second sid={second['session_id']} answer={second['answer']!r}" + ) + assert second["session_id"] == sid + + fresh = await call_chat(session, "fresh session, no history") + print(f"third sid={fresh['session_id']} answer={fresh['answer']!r}") + assert fresh["session_id"] != sid + + +if __name__ == "__main__": + asyncio.run(main()) \ No newline at end of file diff --git a/examples/mcp-server/plugins/__init__.py b/examples/mcp-server/plugins/__init__.py new file mode 100644 index 00000000..50daa0d8 --- /dev/null +++ b/examples/mcp-server/plugins/__init__.py @@ -0,0 +1,3 @@ +"""Extra plugin stubs for the MCP-server example.""" + +from __future__ import annotations \ No newline at end of file diff --git a/examples/mcp-server/plugins/plugins.toml b/examples/mcp-server/plugins/plugins.toml new file mode 100644 index 00000000..699ea83a --- /dev/null +++ b/examples/mcp-server/plugins/plugins.toml @@ -0,0 +1,39 @@ +# plugins.toml — unified manifest for this client extension package. +# +# ONE manifest for ALL client extension code: hooks, resolvers, and tools. +# It is a catalog/generation companion. The runtime reads only [hooks.plugins] +# to resolve managed hook ids; resolvers and tools load by file path. +# `agentctl generate` creates this file if missing and merges new entries in +# without overwriting manual edits. +# +# SECURITY: never put secrets here. Only import refs and metadata — no tokens, +# client secrets, HMAC keys, or Authorization values. + +[package] +name = "plugins" +description = "Client extension package for hooks, resolvers, and tools." + +[paths] +hooks = "plugins.hooks" +resolvers = "plugins.resolvers" +tools = "plugins.tools" + +[hooks] +on_engine_start = [] +on_engine_stop = [] +on_run_start = [] +on_run_end = [] +on_run_error = [] +before_tool_call = [] +after_tool_call = [] +transform_tool_result = [] +on_tool_error = [] +before_mcp_request = [] +after_mcp_response = [] + +[hooks.plugins] + +[resolvers] + +[tools] +echo = "plugins.tools.echo:echo" diff --git a/examples/mcp-server/plugins/tools/__init__.py b/examples/mcp-server/plugins/tools/__init__.py new file mode 100644 index 00000000..7e0adfd6 --- /dev/null +++ b/examples/mcp-server/plugins/tools/__init__.py @@ -0,0 +1,3 @@ +"""Tool stubs for the MCP-server example.""" + +from __future__ import annotations \ No newline at end of file diff --git a/examples/mcp-server/plugins/tools/echo.py b/examples/mcp-server/plugins/tools/echo.py new file mode 100644 index 00000000..f120cc3b --- /dev/null +++ b/examples/mcp-server/plugins/tools/echo.py @@ -0,0 +1,3 @@ +def echo(input: dict) -> str: + """Echoes the input back. Useful for verifying the MCP wiring.""" + return str(input.get("text", input)) \ No newline at end of file diff --git a/examples/mcp-server/prompts/echo_agent/system.md b/examples/mcp-server/prompts/echo_agent/system.md new file mode 100644 index 00000000..89025e0b --- /dev/null +++ b/examples/mcp-server/prompts/echo_agent/system.md @@ -0,0 +1,3 @@ +You are Echo Agent. Answer the user's question directly and concisely. +You have an `echo` tool that simply echoes its input back — use it when asked +to "echo" something verbatim. Otherwise just answer in one or two sentences. \ No newline at end of file diff --git a/src/agentctl/main.py b/src/agentctl/main.py index 1f6d8b57..f0b5bab1 100644 --- a/src/agentctl/main.py +++ b/src/agentctl/main.py @@ -10,6 +10,7 @@ from agent_engine.engine.langgraph.engine import LangGraphEngine from agent_engine.generate.generator import Generator from agent_engine.parsers.yaml.parser import YAMLParser +from agentctl.mcp_serve import create_server from agentctl.session import SpecError, load_and_validate, load_env LOCAL_USER_ID = "local-user" @@ -281,5 +282,27 @@ def chat( asyncio.run(run_remote_chat(url, stream, session_id=session_id)) +@cli.group() +def mcp() -> None: + """MCP server commands.""" + + +@mcp.command(name="serve") +@click.option("--config", required=True, help="Path to agents.yml") +@click.option("--env", default=None, help="Path to .env file") +def mcp_serve(config: str, env: str | None) -> None: + """Run the Extra agent system as an MCP server (stdio transport).""" + load_env(config, env) + from agentctl.diagnostics import format_validation_report, validate_spec + + validation = validate_spec(config) + if not validation.ok: + click.echo(format_validation_report(validation), err=True) + sys.exit(1) + + server = create_server(config, env) + asyncio.run(server.run()) + + if __name__ == "__main__": cli() diff --git a/src/agentctl/mcp_serve.py b/src/agentctl/mcp_serve.py new file mode 100644 index 00000000..92e2bd83 --- /dev/null +++ b/src/agentctl/mcp_serve.py @@ -0,0 +1,128 @@ +"""Extra MCP server — expose the full Extra system as an MCP tool. + +The engine is built once when the server starts and reused for all requests. +Tool calls flow through the existing :class:`ConversationService` so history, +token-budget, persistence, and hooks behave exactly as they do in the CLI. +""" + +from __future__ import annotations + +from dataclasses import asdict +from pathlib import Path +from typing import Any + +from mcp.server.fastmcp import FastMCP + +from agent_engine.engine.langgraph.engine import LangGraphEngine +from agent_manager.application import ConversationService +from agent_manager.composition import ApplicationRepositories, application_repositories +from agent_manager.config import Settings +from agent_manager.domain.identity import Principal +from agentctl.session import load_and_validate, load_env + +DEFAULT_USER_ID = "local-user" + + +def _principal_for(user_id: str) -> Principal: + """Build a host-verified :class:`Principal` for the caller-supplied id.""" + return Principal.external(user_id) + + +class ExtraMCPServer: + """Build and run an MCP server wrapping the Extra graph.""" + + def __init__(self, config: str, env: str | None) -> None: + load_env(config, env) + self._spec, self._base_dir = load_and_validate(config) + self._config_path = Path(config).resolve() + self._settings = Settings() + self._server = FastMCP(name=self._spec.meta.name) + + self._repositories_cm = application_repositories(self._settings) + self._repositories: ApplicationRepositories | None = None + self._engine: LangGraphEngine | None = None + self._service: ConversationService | None = None + + self._register_tool() + + def _register_tool(self) -> None: + """Register the single ``extra_chat`` tool, capturing ``self``.""" + + @self._server.tool() + async def extra_chat( + message: str, + session_id: str = "", + user_id: str = "", + ) -> dict[str, Any]: + """Send a message to the Extra agent system and return the answer. + + When ``session_id`` is omitted or empty, a new conversation is + created automatically and its id is returned in the response. The + same ``session_id`` can be sent again to continue the previous + conversation; different ``session_id`` values keep their own + history. + """ + return await self._handle_chat(message, session_id, user_id) + + async def _setup(self) -> None: + assert self._repositories is not None + engine = LangGraphEngine( + self._base_dir, + session_approval_repository=self._repositories.session_approvals, + ) + # Assign the engine before ``build`` so a build failure still + # triggers ``close`` in the outer ``run`` cleanup block. + self._engine = engine + await engine.build(self._spec) + self._service = ConversationService( + engine, + self._repositories.conversations, + window=self._settings.context_window, + max_chars=self._settings.context_max_chars, + max_tokens=self._settings.context_max_tokens, + snapshot_ttl_seconds=self._settings.snapshot_ttl_seconds, + system_name=self._spec.meta.name, + config_path=str(self._config_path), + ) + await self._server.run_stdio_async() + + async def _handle_chat(self, message: str, session_id: str, user_id: str) -> dict[str, Any]: + service = self._service + if service is None: + raise RuntimeError("MCP server has not finished initializing") + effective_session_id = session_id or "" + effective_user_id = user_id or DEFAULT_USER_ID + principal = _principal_for(effective_user_id) + if not effective_session_id: + effective_session_id = await service.create(principal) + result = await service.send(effective_session_id, message, principal) + return { + "session_id": effective_session_id, + "answer": result.answer, + "visited": list(result.visited), + "used_tools": [ + {k: v for k, v in asdict(tool).items() if v is not None} + for tool in result.used_tools + ], + } + + async def run(self) -> None: + try: + self._repositories = await self._repositories_cm.__aenter__() + await self._setup() + finally: + if self._engine is not None: + await self._engine.close() + self._service = None + self._engine = None + if self._repositories is not None: + await self._repositories_cm.__aexit__(None, None, None) + self._repositories = None + + +def create_server(config: str, env: str | None) -> ExtraMCPServer: + """Factory used by the CLI and tests.""" + return ExtraMCPServer(config, env) + + +__all__ = ["ExtraMCPServer", "create_server"] diff --git a/tests/cli/test_mcp_serve_command.py b/tests/cli/test_mcp_serve_command.py new file mode 100644 index 00000000..d4c755c9 --- /dev/null +++ b/tests/cli/test_mcp_serve_command.py @@ -0,0 +1,510 @@ +"""Tests for ``agentctl mcp serve``. + +These tests cover three layers: + +* the CLI subcommand validates the spec and starts stdio mode +* the ``extra_chat`` tool reuses :class:`ConversationService` instead of + reimplementing history or session logic +* a real MCP client (over stdio) can discover ``extra_chat``, send a + message, receive the answer, and isolate sessions from each other +""" + +from __future__ import annotations + +import asyncio +import json +import os +import sys +from pathlib import Path +from typing import Any, ClassVar + +import pytest +from click.testing import CliRunner + +from agent_engine.engine.types import ChatMessage, RunResult +from agent_engine.runtime.hooks import RunContext +from agentctl.diagnostics import ValidationResult +from agentctl.main import cli + + +class FakeRuntimeEngine: + """Stand-in for ``LangGraphEngine`` that records every run. + + It echoes the prompt as the answer so tests can assert on it. The + ``answer`` keeps track of which call number this is, which is useful for + verifying that history reaches the engine on the second turn. + + The ``build_count`` counter lets the integration test assert the engine is + built exactly once even though the integration test runs the server in a + subprocess where class variables are not shared with the parent. + """ + + prompts: ClassVar[list[str]] = [] + histories: ClassVar[list[tuple[ChatMessage, ...]]] = [] + contexts: ClassVar[list[RunContext | None]] = [] + build_count: ClassVar[int] = 0 + + def __init__(self, _base_dir: Path, **_kwargs: object) -> None: + self._closed = False + + async def __aenter__(self) -> FakeRuntimeEngine: + return self + + async def __aexit__(self, *args: object) -> None: + await self.close() + + async def build(self, _spec: object) -> None: + type(self).build_count += 1 + + async def run( + self, + message: str, + *, + history: tuple[ChatMessage, ...] = (), + context: RunContext | None = None, + ) -> RunResult: + self.prompts.append(message) + self.histories.append(history) + self.contexts.append(context) + return RunResult( + system_name="Fake System", + visited=["fake_agent"], + answer=f"echo:{message}", + ) + + async def close(self) -> None: + if not self._closed: + self._closed = True + + +def _write_spec(tmp_path: Path) -> Path: + spec = tmp_path / "agents.yml" + spec.write_text( + "system: {name: Fake System}\n" + "agents: {fake_agent: {description: fake}}\n" + "graph: {fake_agent: null}\n", + encoding="utf-8", + ) + return spec + + +def test_mcp_serve_validates_config_before_starting( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The command should reject an invalid spec without launching the server.""" + spec = _write_spec(tmp_path) + + def fake_validate(config: str) -> ValidationResult: + return ValidationResult( + ok=False, + errors=["[agents.fake_agent] agent is not implemented"], + ) + + monkeypatch.setattr("agentctl.diagnostics.validate_spec", fake_validate) + + res = CliRunner().invoke(cli, ["mcp", "serve", "--config", str(spec)]) + assert res.exit_code == 1 + assert "Validation failed:" in res.output + + +def test_mcp_serve_starts_with_valid_config( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The command should build the engine and enter stdio mode.""" + spec = _write_spec(tmp_path) + + captured: dict[str, object] = {} + + async def fake_run_stdio(self: object) -> None: + captured["ran"] = True + + def fake_validate(config: str) -> ValidationResult: + return ValidationResult(ok=True) + + monkeypatch.setattr("agentctl.diagnostics.validate_spec", fake_validate) + + monkeypatch.setattr("agentctl.mcp_serve.LangGraphEngine", FakeRuntimeEngine) + monkeypatch.setattr("agentctl.mcp_serve.FastMCP.run_stdio_async", fake_run_stdio) + + res = CliRunner().invoke(cli, ["mcp", "serve", "--config", str(spec)]) + assert res.exit_code == 0, res.output + assert captured.get("ran") is True + + +def test_extra_chat_tool_creates_session_when_missing( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """When no session_id is provided, the tool should create one and return it.""" + spec = _write_spec(tmp_path) + + created_sessions: list[str] = [] + sent_messages: list[tuple[str, str]] = [] + + from agent_manager.domain.identity import Principal + + async def fake_create( + self_inner: object, + principal: Principal, + *, + session_id: str | None = None, + ) -> str: + sid = session_id or "new-session-123" + created_sessions.append(sid) + return sid + + async def fake_send( + self_inner: object, + conversation_id: str, + text: str, + principal: Principal, + ) -> RunResult: + sent_messages.append((conversation_id, text)) + return RunResult( + system_name="Fake System", + visited=["fake_agent"], + answer=f"answer-{len(sent_messages)}", + ) + + from agent_manager.application import ConversationService + + monkeypatch.setattr(ConversationService, "create", fake_create) + monkeypatch.setattr(ConversationService, "send", fake_send) + + from agentctl.mcp_serve import create_server + + server = create_server(str(spec), None) + fake_engine = FakeRuntimeEngine(Path(".")) + server._repositories = _FakeRepositories() # type: ignore[assignment] + server._engine = fake_engine # type: ignore[assignment] + server._service = ConversationService(fake_engine, _FakeRepository()) # type: ignore[arg-type] + + result = asyncio.run(server._handle_chat("hello", "", "u1")) + + assert result["session_id"] == "new-session-123" + assert result["answer"] == "answer-1" + assert result["visited"] == ["fake_agent"] + assert sent_messages == [("new-session-123", "hello")] + assert created_sessions == ["new-session-123"] + + +def test_extra_chat_tool_reuses_session(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """When a session_id is provided, the tool should reuse that session.""" + spec = _write_spec(tmp_path) + + sent_messages: list[tuple[str, str]] = [] + + from agent_manager.domain.identity import Principal + + async def fake_send( + self_inner: object, + conversation_id: str, + text: str, + principal: Principal, + ) -> RunResult: + sent_messages.append((conversation_id, text)) + return RunResult( + system_name="Fake System", + visited=["fake_agent"], + answer=f"answer-{len(sent_messages)}", + ) + + from agent_manager.application import ConversationService + + monkeypatch.setattr(ConversationService, "send", fake_send) + + from agentctl.mcp_serve import create_server + + server = create_server(str(spec), None) + fake_engine = FakeRuntimeEngine(Path(".")) + server._repositories = _FakeRepositories() # type: ignore[assignment] + server._engine = fake_engine # type: ignore[assignment] + server._service = ConversationService(fake_engine, _FakeRepository()) # type: ignore[arg-type] + + result = asyncio.run(server._handle_chat("follow-up", "sess-42", "u1")) + + assert result["session_id"] == "sess-42" + assert result["answer"] == "answer-1" + assert sent_messages == [("sess-42", "follow-up")] + + +def test_extra_chat_tool_default_user_id(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """When user_id is omitted, the tool should fall back to a stable default.""" + spec = _write_spec(tmp_path) + + sent: list[tuple[str, str]] = [] + + from agent_manager.domain.identity import Principal + + async def fake_send( + self_inner: object, + conversation_id: str, + text: str, + principal: Principal, + ) -> RunResult: + sent.append((text, principal.user_id)) + return RunResult( + system_name="Fake System", + visited=["fake_agent"], + answer="ok", + ) + + from agent_manager.application import ConversationService + + monkeypatch.setattr(ConversationService, "send", fake_send) + + from agentctl.mcp_serve import create_server + + server = create_server(str(spec), None) + server._repositories = _FakeRepositories() # type: ignore[assignment] + server._engine = FakeRuntimeEngine(Path(".")) # type: ignore[assignment] + server._service = ConversationService(server._engine, _FakeRepository()) # type: ignore[arg-type] + + asyncio.run(server._handle_chat("hello", "sess-1", "")) + + assert sent == [("hello", "ext:3477cdab157537bd83c95a3c0b0e4883")] + + +def test_extra_chat_tool_returns_used_tools( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The response should expose the ``used_tools`` from the run result.""" + spec = _write_spec(tmp_path) + + from agent_engine.runtime.tool_models import ToolUsageRecord + + record = ToolUsageRecord( + name="search_internal_documents", + provider="local", + status="succeeded", + agent_id="fake_agent", + ) + + from agent_manager.domain.identity import Principal + + async def fake_send( + self_inner: object, + conversation_id: str, + text: str, + principal: Principal, + ) -> RunResult: + return RunResult( + system_name="Fake System", + visited=["root", "knowledge_agent"], + answer="the answer", + used_tools=(record,), + ) + + from agent_manager.application import ConversationService + + monkeypatch.setattr(ConversationService, "send", fake_send) + + from agentctl.mcp_serve import create_server + + server = create_server(str(spec), None) + server._repositories = _FakeRepositories() # type: ignore[assignment] + server._engine = FakeRuntimeEngine(Path(".")) # type: ignore[assignment] + server._service = ConversationService(server._engine, _FakeRepository()) # type: ignore[arg-type] + + result = asyncio.run(server._handle_chat("search docs", "sess-x", "u1")) + + assert result["visited"] == ["root", "knowledge_agent"] + assert result["used_tools"] == [ + { + "name": "search_internal_documents", + "provider": "local", + "status": "succeeded", + "agent_id": "fake_agent", + } + ] + + +def test_extra_chat_tool_response_is_json_serialisable(tmp_path: Path) -> None: + """The dict shape returned by ``_handle_chat`` must round-trip through JSON.""" + + from agentctl.mcp_serve import create_server + + spec = _write_spec(tmp_path) + create_server(str(spec), None) # construction must succeed + payload = { + "session_id": "abc", + "answer": "hi", + "visited": ["root"], + "used_tools": [{"name": "echo", "provider": "local"}], + } + json.dumps(payload) # must not raise + + +def test_run_closes_engine_and_db_on_failure( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """If ``setup`` fails, the server should still close the engine and DB.""" + spec = _write_spec(tmp_path) + + db_disposed = False + + class _RepoCM: + async def __aenter__(self) -> _FakeRepositories: + return _FakeRepositories() + + async def __aexit__(self, *args: object) -> None: + nonlocal db_disposed + db_disposed = True + + engine = FakeRuntimeEngine(Path(".")) + + async def boom(_self: object, *_args: object, **_kw: object) -> None: + raise RuntimeError("boom") + + monkeypatch.setattr("agentctl.mcp_serve.application_repositories", lambda _s: _RepoCM()) + monkeypatch.setattr("agentctl.mcp_serve.LangGraphEngine", lambda *a, **kw: engine) + monkeypatch.setattr(engine, "build", boom) + + from agentctl.mcp_serve import create_server + + server = create_server(str(spec), None) + + with pytest.raises(RuntimeError, match="boom"): + asyncio.run(server.run()) + + assert engine._closed is True + assert db_disposed is True + + +class _FakeRepository: + """A no-op stand-in for the conversation repository.""" + + async def whatever(self, *_args: object, **_kwargs: object) -> None: + return None + + +class _FakeRepositories: + """Stand-in for :class:`ApplicationRepositories`.""" + + def __init__(self) -> None: + self.conversations = _FakeRepository() + self.session_approvals = object() + + async def __aenter__(self) -> _FakeRepositories: + return self + + async def __aexit__(self, *args: object) -> None: + return None + + +# --------------------------------------------------------------------------- +# End-to-end MCP integration test (real stdio transport). +# --------------------------------------------------------------------------- + + +def test_mcp_client_can_call_extra_chat_over_stdio( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Spin up the MCP server as a subprocess and talk to it from an MCP client. + + The engine is replaced in the subprocess by a tiny bootstrap script that + monkeypatches ``agentctl.mcp_serve.LangGraphEngine`` before importing the + CLI. This proves the full subprocess → stdio → MCP → ConversationService + → fake engine path works end to end. + + The fake engine records ``prompts``/``histories``/``contexts`` and a + ``build_count`` in its class state inside the subprocess. We don't need + those values inside the parent process; the assertions on the wire-level + MCP responses are sufficient to prove that the engine ran (answers are + ``echo:``), that the second turn's ``session_id`` matched the + first (so ConversationService reused the conversation), and that a third + call without a ``session_id`` got a different id (fresh session). + """ + pytest.importorskip("mcp") + + spec = _write_spec(tmp_path) + db_path = tmp_path / "chat.db" + monkeypatch.setenv("AGENT_DB_BACKEND", "sqlite") + monkeypatch.setenv("AGENT_DB_URL", f"sqlite+aiosqlite:///{db_path}") + + repo_root = Path(__file__).resolve().parents[2] + src_path = repo_root / "src" + + bootstrap = tmp_path / "bootstrap.py" + bootstrap.write_text( + "import sys\n" + f"sys.path.insert(0, {str(src_path)!r})\n" + f"sys.path.insert(0, {str(repo_root)!r})\n" + "from agent_manager.infrastructure.persistence.database import upgrade_database\n" + "upgrade_database()\n" + "from tests.cli.test_mcp_serve_command import FakeRuntimeEngine\n" + "import agentctl.mcp_serve as _mod\n" + "_mod.LangGraphEngine = FakeRuntimeEngine\n" + "FakeRuntimeEngine.prompts.clear()\n" + "FakeRuntimeEngine.histories.clear()\n" + "FakeRuntimeEngine.contexts.clear()\n" + "FakeRuntimeEngine.build_count = 0\n" + "from agentctl.main import cli\n" + "cli()\n", + encoding="utf-8", + ) + + env = os.environ.copy() + env["PYTHONPATH"] = ( + str(repo_root) + os.pathsep + str(src_path) + os.pathsep + env.get("PYTHONPATH", "") + ) + cmd = [sys.executable, str(bootstrap), "mcp", "serve", "--config", str(spec)] + + async def scenario() -> dict[str, Any]: + from mcp import ClientSession, StdioServerParameters + from mcp.client.stdio import stdio_client + + params = StdioServerParameters(command=cmd[0], args=cmd[1:], env=env) + async with ( + stdio_client(params) as streams, + ClientSession(*streams) as session, + ): + await session.initialize() + tools = await session.list_tools() + names = {t.name for t in tools.tools} + assert "extra_chat" in names, names + + first = await session.call_tool( + "extra_chat", + {"message": "hello world", "user_id": "alice"}, + ) + assert first.isError is False, first.content + first_payload = _extract_payload(first.content) + assert first_payload["answer"] == "echo:hello world" + session_id = first_payload["session_id"] + assert session_id + + second = await session.call_tool( + "extra_chat", + {"message": "again", "session_id": session_id, "user_id": "alice"}, + ) + assert second.isError is False, second.content + second_payload = _extract_payload(second.content) + assert second_payload["session_id"] == session_id + assert second_payload["answer"] == "echo:again" + + third = await session.call_tool( + "extra_chat", + {"message": "separate", "user_id": "alice"}, + ) + third_payload = _extract_payload(third.content) + assert third_payload["session_id"] != session_id + assert third_payload["answer"] == "echo:separate" + + return { + "first": first_payload, + "second": second_payload, + "third": third_payload, + } + + result = asyncio.run(scenario()) + assert result["first"]["session_id"] == result["second"]["session_id"] + assert result["third"]["session_id"] != result["first"]["session_id"] + + +def _extract_payload(content: list[Any]) -> dict[str, Any]: + """Pull the JSON dict out of an MCP tool-call response.""" + for block in content: + text = getattr(block, "text", None) + if text: + return json.loads(text) + raise AssertionError(f"no text block in {content!r}") From 97911af3d1686e1cda99969a1249ce66ee471559 Mon Sep 17 00:00:00 2001 From: Tamaghna Sarkar Date: Fri, 4 Sep 2026 19:58:00 +0000 Subject: [PATCH 2/5] fix(mcp): align runtime composition, migrations, HITL, and identity boundary --- src/agentctl/main.py | 3 + src/agentctl/mcp_serve.py | 74 +++++++++++++-- tests/cli/test_mcp_serve_command.py | 142 +++++++++++++++++++++++++++- 3 files changed, 210 insertions(+), 9 deletions(-) diff --git a/src/agentctl/main.py b/src/agentctl/main.py index f0b5bab1..1889b18c 100644 --- a/src/agentctl/main.py +++ b/src/agentctl/main.py @@ -293,6 +293,9 @@ def mcp() -> None: def mcp_serve(config: str, env: str | None) -> None: """Run the Extra agent system as an MCP server (stdio transport).""" load_env(config, env) + from agent_manager.infrastructure.persistence.database import upgrade_database + + upgrade_database() from agentctl.diagnostics import format_validation_report, validate_spec validation = validate_spec(config) diff --git a/src/agentctl/mcp_serve.py b/src/agentctl/mcp_serve.py index 92e2bd83..d8e23268 100644 --- a/src/agentctl/mcp_serve.py +++ b/src/agentctl/mcp_serve.py @@ -13,6 +13,7 @@ from mcp.server.fastmcp import FastMCP +from agent_engine.approvals.decision import ApprovalDecision, parse_decision from agent_engine.engine.langgraph.engine import LangGraphEngine from agent_manager.application import ConversationService from agent_manager.composition import ApplicationRepositories, application_repositories @@ -24,8 +25,13 @@ def _principal_for(user_id: str) -> Principal: - """Build a host-verified :class:`Principal` for the caller-supplied id.""" - return Principal.external(user_id) + """Build an anonymous :class:`Principal` for the caller-supplied id. + + The MCP caller is an external process connected over stdio, not the host + product that vouches for real identities. ``user_id`` is therefore treated + as an opaque conversation label rather than a host-verified external id. + """ + return Principal.anonymous(user_id) class ExtraMCPServer: @@ -43,10 +49,10 @@ def __init__(self, config: str, env: str | None) -> None: self._engine: LangGraphEngine | None = None self._service: ConversationService | None = None - self._register_tool() + self._register_tools() - def _register_tool(self) -> None: - """Register the single ``extra_chat`` tool, capturing ``self``.""" + def _register_tools(self) -> None: + """Register ``extra_chat`` and ``decide_approval`` tools, capturing ``self``.""" @self._server.tool() async def extra_chat( @@ -64,11 +70,31 @@ async def extra_chat( """ return await self._handle_chat(message, session_id, user_id) + @self._server.tool() + async def decide_approval( + session_id: str, + run_id: str, + approval_id: str, + decision: str = "approve", + ) -> dict[str, Any]: + """Approve or reject a pending tool-call approval. + + ``session_id`` identifies the conversation. ``run_id`` and + ``approval_id`` come from the ``pending_approval`` block returned + by ``extra_chat`` when the run is suspended. ``decision`` accepts + ``approve`` (allow this invocation once), ``allow_for_session`` + (allow and stop asking for this tool in this conversation), and + ``reject`` (do not run; store nothing). + """ + return await self._handle_decide_approval(session_id, run_id, approval_id, decision) + async def _setup(self) -> None: assert self._repositories is not None engine = LangGraphEngine( self._base_dir, session_approval_repository=self._repositories.session_approvals, + tool_usage_repository=self._repositories.tool_usage, + run_repository=self._repositories.runs, ) # Assign the engine before ``build`` so a build failure still # triggers ``close`` in the outer ``run`` cleanup block. @@ -83,6 +109,7 @@ async def _setup(self) -> None: snapshot_ttl_seconds=self._settings.snapshot_ttl_seconds, system_name=self._spec.meta.name, config_path=str(self._config_path), + run_repository=self._repositories.runs, ) await self._server.run_stdio_async() @@ -96,8 +123,43 @@ async def _handle_chat(self, message: str, session_id: str, user_id: str) -> dic if not effective_session_id: effective_session_id = await service.create(principal) result = await service.send(effective_session_id, message, principal) - return { + payload: dict[str, Any] = { "session_id": effective_session_id, + "status": result.status.value, + "answer": result.answer, + "visited": list(result.visited), + "used_tools": [ + {k: v for k, v in asdict(tool).items() if v is not None} + for tool in result.used_tools + ], + } + if result.pending_approval is not None: + payload["pending_approval"] = asdict(result.pending_approval) + return payload + + async def _handle_decide_approval( + self, + session_id: str, + run_id: str, + approval_id: str, + decision: str, + ) -> dict[str, Any]: + service = self._service + if service is None: + raise RuntimeError("MCP server has not finished initializing") + effective_user_id = DEFAULT_USER_ID + principal = _principal_for(effective_user_id) + parsed_decision = parse_decision(decision, default=ApprovalDecision.DENY) + result = await service.decide_approval( + session_id, + run_id, + approval_id, + parsed_decision, + principal, + ) + return { + "session_id": session_id, + "status": result.status.value, "answer": result.answer, "visited": list(result.visited), "used_tools": [ diff --git a/tests/cli/test_mcp_serve_command.py b/tests/cli/test_mcp_serve_command.py index d4c755c9..fb192aba 100644 --- a/tests/cli/test_mcp_serve_command.py +++ b/tests/cli/test_mcp_serve_command.py @@ -21,6 +21,7 @@ import pytest from click.testing import CliRunner +from agent_engine.approvals.models import RunStatus from agent_engine.engine.types import ChatMessage, RunResult from agent_engine.runtime.hooks import RunContext from agentctl.diagnostics import ValidationResult @@ -261,7 +262,7 @@ async def fake_send( asyncio.run(server._handle_chat("hello", "sess-1", "")) - assert sent == [("hello", "ext:3477cdab157537bd83c95a3c0b0e4883")] + assert sent == [("hello", "anon:local-user")] def test_extra_chat_tool_returns_used_tools( @@ -318,6 +319,140 @@ async def fake_send( ] +def test_extra_chat_tool_exposes_completed_status( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A normal response should include status=completed and no pending_approval.""" + spec = _write_spec(tmp_path) + + from agent_manager.domain.identity import Principal + + async def fake_send( + self_inner: object, + conversation_id: str, + text: str, + principal: Principal, + ) -> RunResult: + return RunResult( + system_name="Fake System", + visited=["fake_agent"], + answer="done", + ) + + from agent_manager.application import ConversationService + + monkeypatch.setattr(ConversationService, "send", fake_send) + + from agentctl.mcp_serve import create_server + + server = create_server(str(spec), None) + server._repositories = _FakeRepositories() # type: ignore[assignment] + server._engine = FakeRuntimeEngine(Path(".")) # type: ignore[assignment] + server._service = ConversationService(server._engine, _FakeRepository()) # type: ignore[arg-type] + + result = asyncio.run(server._handle_chat("hi", "sess-1", "u1")) + + assert result["status"] == "completed" + assert "pending_approval" not in result + assert result["answer"] == "done" + + +def test_extra_chat_tool_exposes_pending_approval( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """When the run suspends at an approval, the response must expose it.""" + spec = _write_spec(tmp_path) + + from agent_engine.engine.types import PendingApproval + from agent_manager.domain.identity import Principal + + pending = PendingApproval( + run_id="run-1", + approval_id="approval-1", + agent_id="fake_agent", + tool_name="dangerous_tool", + description="do something risky", + provider="local", + arguments={"x": 1}, + ) + + async def fake_send( + self_inner: object, + conversation_id: str, + text: str, + principal: Principal, + ) -> RunResult: + return RunResult( + system_name="Fake System", + visited=["fake_agent"], + answer="", + status=RunStatus.PENDING_APPROVAL, + pending_approval=pending, + ) + + from agent_manager.application import ConversationService + + monkeypatch.setattr(ConversationService, "send", fake_send) + + from agentctl.mcp_serve import create_server + + server = create_server(str(spec), None) + server._repositories = _FakeRepositories() # type: ignore[assignment] + server._engine = FakeRuntimeEngine(Path(".")) # type: ignore[assignment] + server._service = ConversationService(server._engine, _FakeRepository()) # type: ignore[arg-type] + + result = asyncio.run(server._handle_chat("do it", "sess-1", "u1")) + + assert result["status"] == "pending_approval" + assert result["answer"] == "" + assert result["pending_approval"]["run_id"] == "run-1" + assert result["pending_approval"]["approval_id"] == "approval-1" + assert result["pending_approval"]["tool_name"] == "dangerous_tool" + + +def test_decide_approval_tool(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """The decide_approval tool should delegate to ConversationService.""" + spec = _write_spec(tmp_path) + + from agent_engine.engine.types import RunResult as _RunResult + from agent_manager.domain.identity import Principal + + decide_calls: list[tuple[str, str, str, str]] = [] + + async def fake_decide( + self_inner: object, + conversation_id: str, + run_id: str, + approval_id: str, + decision: str, + principal: Principal, + ) -> _RunResult: + decide_calls.append((conversation_id, run_id, approval_id, decision)) + return _RunResult( + system_name="Fake System", + visited=["fake_agent"], + answer="approved-result", + status=RunStatus.COMPLETED, + ) + + from agent_manager.application import ConversationService + + monkeypatch.setattr(ConversationService, "decide_approval", fake_decide) + + from agentctl.mcp_serve import create_server + + server = create_server(str(spec), None) + server._repositories = _FakeRepositories() # type: ignore[assignment] + server._engine = FakeRuntimeEngine(Path(".")) # type: ignore[assignment] + server._service = ConversationService(server._engine, _FakeRepository()) # type: ignore[arg-type] + + result = asyncio.run(server._handle_decide_approval("sess-1", "run-1", "approval-1", "approve")) + + assert result["status"] == "completed" + assert result["answer"] == "approved-result" + assert decide_calls == [("sess-1", "run-1", "approval-1", "allow_once")] + + def test_extra_chat_tool_response_is_json_serialisable(tmp_path: Path) -> None: """The dict shape returned by ``_handle_chat`` must round-trip through JSON.""" @@ -327,6 +462,7 @@ def test_extra_chat_tool_response_is_json_serialisable(tmp_path: Path) -> None: create_server(str(spec), None) # construction must succeed payload = { "session_id": "abc", + "status": "completed", "answer": "hi", "visited": ["root"], "used_tools": [{"name": "echo", "provider": "local"}], @@ -383,6 +519,8 @@ class _FakeRepositories: def __init__(self) -> None: self.conversations = _FakeRepository() self.session_approvals = object() + self.tool_usage = object() + self.runs = object() async def __aenter__(self) -> _FakeRepositories: return self @@ -429,8 +567,6 @@ def test_mcp_client_can_call_extra_chat_over_stdio( "import sys\n" f"sys.path.insert(0, {str(src_path)!r})\n" f"sys.path.insert(0, {str(repo_root)!r})\n" - "from agent_manager.infrastructure.persistence.database import upgrade_database\n" - "upgrade_database()\n" "from tests.cli.test_mcp_serve_command import FakeRuntimeEngine\n" "import agentctl.mcp_serve as _mod\n" "_mod.LangGraphEngine = FakeRuntimeEngine\n" From 35d3ecbc2493b55c878276caffe031e5a3230b6c Mon Sep 17 00:00:00 2001 From: Tamaghna Sarkar Date: Sun, 6 Sep 2026 20:13:02 +0000 Subject: [PATCH 3/5] refactor(mcp): align serialization, strict approval parsing, and identity handling --- src/agentctl/mcp_serve.py | 49 +++++++++++++++++++++++++-------------- 1 file changed, 31 insertions(+), 18 deletions(-) diff --git a/src/agentctl/mcp_serve.py b/src/agentctl/mcp_serve.py index d8e23268..43c5478b 100644 --- a/src/agentctl/mcp_serve.py +++ b/src/agentctl/mcp_serve.py @@ -13,8 +13,9 @@ from mcp.server.fastmcp import FastMCP -from agent_engine.approvals.decision import ApprovalDecision, parse_decision +from agent_engine.approvals.decision import ApprovalDecision from agent_engine.engine.langgraph.engine import LangGraphEngine +from agent_engine.engine.types import RunResult from agent_manager.application import ConversationService from agent_manager.composition import ApplicationRepositories, application_repositories from agent_manager.config import Settings @@ -76,6 +77,7 @@ async def decide_approval( run_id: str, approval_id: str, decision: str = "approve", + user_id: str = "", ) -> dict[str, Any]: """Approve or reject a pending tool-call approval. @@ -86,7 +88,9 @@ async def decide_approval( (allow and stop asking for this tool in this conversation), and ``reject`` (do not run; store nothing). """ - return await self._handle_decide_approval(session_id, run_id, approval_id, decision) + return await self._handle_decide_approval( + session_id, run_id, approval_id, decision, user_id + ) async def _setup(self) -> None: assert self._repositories is not None @@ -123,19 +127,7 @@ async def _handle_chat(self, message: str, session_id: str, user_id: str) -> dic if not effective_session_id: effective_session_id = await service.create(principal) result = await service.send(effective_session_id, message, principal) - payload: dict[str, Any] = { - "session_id": effective_session_id, - "status": result.status.value, - "answer": result.answer, - "visited": list(result.visited), - "used_tools": [ - {k: v for k, v in asdict(tool).items() if v is not None} - for tool in result.used_tools - ], - } - if result.pending_approval is not None: - payload["pending_approval"] = asdict(result.pending_approval) - return payload + return self._serialize_run_result(result, effective_session_id) async def _handle_decide_approval( self, @@ -143,13 +135,24 @@ async def _handle_decide_approval( run_id: str, approval_id: str, decision: str, + user_id: str = "", ) -> dict[str, Any]: service = self._service if service is None: raise RuntimeError("MCP server has not finished initializing") - effective_user_id = DEFAULT_USER_ID + effective_user_id = user_id or DEFAULT_USER_ID principal = _principal_for(effective_user_id) - parsed_decision = parse_decision(decision, default=ApprovalDecision.DENY) + normalized = decision.strip().lower() + mapping = { + "approve": ApprovalDecision.ALLOW_ONCE, + "reject": ApprovalDecision.DENY, + "allow_for_session": ApprovalDecision.ALLOW_FOR_SESSION, + } + if normalized not in mapping: + raise ValueError( + f"Invalid decision {decision!r}. Use 'approve', 'reject', or 'allow_for_session'." + ) + parsed_decision = mapping[normalized] result = await service.decide_approval( session_id, run_id, @@ -157,7 +160,14 @@ async def _handle_decide_approval( parsed_decision, principal, ) - return { + return self._serialize_run_result(result, session_id) + + def _serialize_run_result( + self, + result: RunResult, + session_id: str, + ) -> dict[str, Any]: + payload: dict[str, Any] = { "session_id": session_id, "status": result.status.value, "answer": result.answer, @@ -167,6 +177,9 @@ async def _handle_decide_approval( for tool in result.used_tools ], } + if result.pending_approval is not None: + payload["pending_approval"] = asdict(result.pending_approval) + return payload async def run(self) -> None: try: From a21c18895c5ba99a71922c4851a7b00ea11b6df1 Mon Sep 17 00:00:00 2001 From: Tamaghna Sarkar Date: Mon, 7 Sep 2026 15:22:06 +0000 Subject: [PATCH 4/5] refactor(mcp): reuse parse_decision, add approval regression tests --- src/agentctl/mcp_serve.py | 13 +-- tests/cli/test_mcp_serve_command.py | 130 ++++++++++++++++++++++++++++ 2 files changed, 134 insertions(+), 9 deletions(-) diff --git a/src/agentctl/mcp_serve.py b/src/agentctl/mcp_serve.py index 43c5478b..d44af9fb 100644 --- a/src/agentctl/mcp_serve.py +++ b/src/agentctl/mcp_serve.py @@ -13,7 +13,7 @@ from mcp.server.fastmcp import FastMCP -from agent_engine.approvals.decision import ApprovalDecision +from agent_engine.approvals.decision import InvalidDecision, parse_decision from agent_engine.engine.langgraph.engine import LangGraphEngine from agent_engine.engine.types import RunResult from agent_manager.application import ConversationService @@ -142,17 +142,12 @@ async def _handle_decide_approval( raise RuntimeError("MCP server has not finished initializing") effective_user_id = user_id or DEFAULT_USER_ID principal = _principal_for(effective_user_id) - normalized = decision.strip().lower() - mapping = { - "approve": ApprovalDecision.ALLOW_ONCE, - "reject": ApprovalDecision.DENY, - "allow_for_session": ApprovalDecision.ALLOW_FOR_SESSION, - } - if normalized not in mapping: + try: + parsed_decision = parse_decision(decision) + except InvalidDecision: raise ValueError( f"Invalid decision {decision!r}. Use 'approve', 'reject', or 'allow_for_session'." ) - parsed_decision = mapping[normalized] result = await service.decide_approval( session_id, run_id, diff --git a/tests/cli/test_mcp_serve_command.py b/tests/cli/test_mcp_serve_command.py index fb192aba..be052561 100644 --- a/tests/cli/test_mcp_serve_command.py +++ b/tests/cli/test_mcp_serve_command.py @@ -453,6 +453,136 @@ async def fake_decide( assert decide_calls == [("sess-1", "run-1", "approval-1", "allow_once")] +def test_decide_approval_preserves_user_id_identity( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The same user_id supplied to extra_chat must reach decide_approval.""" + spec = _write_spec(tmp_path) + + from agent_manager.domain.identity import Principal + + chat_principals: list[Principal] = [] + decide_principals: list[Principal] = [] + + async def fake_send( + self_inner: object, + conversation_id: str, + text: str, + principal: Principal, + ) -> RunResult: + chat_principals.append(principal) + return RunResult( + system_name="Fake System", + visited=["fake_agent"], + answer="hi", + ) + + async def fake_decide( + self_inner: object, + conversation_id: str, + run_id: str, + approval_id: str, + decision: str, + principal: Principal, + ) -> RunResult: + decide_principals.append(principal) + return RunResult( + system_name="Fake System", + visited=["fake_agent"], + answer="approved-result", + status=RunStatus.COMPLETED, + ) + + from agent_manager.application import ConversationService + + monkeypatch.setattr(ConversationService, "send", fake_send) + monkeypatch.setattr(ConversationService, "decide_approval", fake_decide) + + from agentctl.mcp_serve import create_server + + server = create_server(str(spec), None) + server._repositories = _FakeRepositories() # type: ignore[assignment] + server._engine = FakeRuntimeEngine(Path(".")) # type: ignore[assignment] + server._service = ConversationService(server._engine, _FakeRepository()) # type: ignore[arg-type] + + asyncio.run(server._handle_chat("hello", "sess-1", "alice")) + asyncio.run(server._handle_decide_approval("sess-1", "run-1", "approval-1", "approve", "alice")) + + assert len(chat_principals) == 1 + assert len(decide_principals) == 1 + assert chat_principals[0].external_id == decide_principals[0].external_id == "alice" + + +def test_decide_approval_preserves_chained_pending_approval( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A resumed run that hits another approval must still expose it.""" + spec = _write_spec(tmp_path) + + from agent_engine.engine.types import PendingApproval + from agent_manager.domain.identity import Principal + + pending_b = PendingApproval( + run_id="run-2", + approval_id="approval-2", + agent_id="fake_agent", + tool_name="another_tool", + description="more risk", + provider="local", + arguments={}, + ) + + async def fake_decide( + self_inner: object, + conversation_id: str, + run_id: str, + approval_id: str, + decision: str, + principal: Principal, + ) -> RunResult: + return RunResult( + system_name="Fake System", + visited=["fake_agent"], + answer="", + status=RunStatus.PENDING_APPROVAL, + pending_approval=pending_b, + ) + + from agent_manager.application import ConversationService + + monkeypatch.setattr(ConversationService, "decide_approval", fake_decide) + + from agentctl.mcp_serve import create_server + + server = create_server(str(spec), None) + server._repositories = _FakeRepositories() # type: ignore[assignment] + server._engine = FakeRuntimeEngine(Path(".")) # type: ignore[assignment] + server._service = ConversationService(server._engine, _FakeRepository()) # type: ignore[arg-type] + + result = asyncio.run(server._handle_decide_approval("sess-1", "run-1", "approval-1", "approve")) + + assert result["status"] == "pending_approval" + assert result["pending_approval"]["run_id"] == "run-2" + assert result["pending_approval"]["approval_id"] == "approval-2" + assert result["pending_approval"]["tool_name"] == "another_tool" + + +def test_decide_approval_rejects_invalid_decision(tmp_path: Path) -> None: + """Invalid decision strings must raise an explicit error.""" + spec = _write_spec(tmp_path) + + from agent_manager.application import ConversationService + from agentctl.mcp_serve import create_server + + server = create_server(str(spec), None) + server._repositories = _FakeRepositories() # type: ignore[assignment] + server._engine = FakeRuntimeEngine(Path(".")) # type: ignore[assignment] + server._service = ConversationService(server._engine, _FakeRepository()) # type: ignore[arg-type] + + with pytest.raises(ValueError, match="Invalid decision 'aproove'"): + asyncio.run(server._handle_decide_approval("sess-1", "run-1", "approval-1", "aproove")) + + def test_extra_chat_tool_response_is_json_serialisable(tmp_path: Path) -> None: """The dict shape returned by ``_handle_chat`` must round-trip through JSON.""" From 423ef7c247810420636d312efcc0736ba4c6d270 Mon Sep 17 00:00:00 2001 From: Tamaghna Sarkar Date: Mon, 7 Sep 2026 15:33:31 +0000 Subject: [PATCH 5/5] fix(mcp): chain InvalidDecision explicitly in parse_decision wrapper --- src/agentctl/mcp_serve.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/agentctl/mcp_serve.py b/src/agentctl/mcp_serve.py index d44af9fb..da08ecc9 100644 --- a/src/agentctl/mcp_serve.py +++ b/src/agentctl/mcp_serve.py @@ -144,10 +144,10 @@ async def _handle_decide_approval( principal = _principal_for(effective_user_id) try: parsed_decision = parse_decision(decision) - except InvalidDecision: + except InvalidDecision as err: raise ValueError( f"Invalid decision {decision!r}. Use 'approve', 'reject', or 'allow_for_session'." - ) + ) from err result = await service.decide_approval( session_id, run_id,