portfolio-optimiser/tests/test_mcp_run_loadbearing.py
Kjell Tore Guttormsen 9668e17f2f feat(mcp): concrete MCP servers become tools the agents can call during a run
Krav 3, and the operator chose the run path explicitly: the external service must
be reachable WHILE the run works, not only when documents are ingested. Until now
the run path had one in-process tool against a local folder — and on the bundle
path the agents had no tools at all.

MAF already ships the client (MCPStdioTool / MCPStreamableHTTPTool, verified in
the pinned 1.9.0 with allowed_tools and request_timeout), so `mcp_tools.py` owns
only what MAF cannot decide for us: which servers a run may contact, which of
their tools it may call, how long it waits, and where the credential comes from.
This is a DIFFERENT seam from ingest_mcp.py on purpose — that one pulls source
documents before a run and speaks to null-argument tools. Same protocol, different
job.

Every refusal is a live hazard, not tidiness. An empty allowlist would let the far
end decide what the agents may call, so naming the tools is mandatory. A
non-positive timeout is an unbounded wait against a third party. An unknown field
is refused rather than ignored, which is also what keeps a literal secret from
being parked in the config — there is no field for one, only the NAME of an env
var. A named-but-unset credential refuses instead of calling anonymously, because
an anonymous call can succeed with the wrong scope.

Egress is declared, always. Every server and permitted tool is named in the run
announcement before the first call — including when no --mandate is given, which
was a real hole: the announcement only printed with a commission, so configuring
servers without one would have contacted third parties with nothing printed at
all. --live-dry-run still opens nothing, because the tools are entered after the
dry-run cut: the promise to stop before the first call now covers egress too.

Threaded through BOTH modes. A flag accepted in one mode and silently dropped in
the other is the defect class this CLI refuses by name.

Load-bearing MEASURED against the whole 744-test suite, four mutations all red:
build the tools but never hand them to the agents (2) · never enter the
AsyncExitStack, so they are constructed and useless (1) · never declare the egress
(2) · drop the allowlist on the built client (1).

Two live docs claimed MCP was unwired in the run path; both corrected rather than
left to rot.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ULCqjLF61rehj5cZmdUoR3
2026-08-05 16:53:07 +02:00

162 lines
6.3 KiB
Python

"""Load-bearing: configured MCP servers become tools the AGENTS can call during a run, they are
opened and closed around the debate, and nothing is contacted that was not announced (Trekk B2/B3).
Krav 3 is only met if the external service is reachable *while the run works*. Three detach points,
each RED on its own:
* drop the MCP tools from ``debate_tools`` -> the agents never get the tool (and on the bundle path
they get NO tools at all, which is what that path had before);
* skip the ``AsyncExitStack`` entry -> the tools are constructed but never connected, so they are
present and useless — the failure mode that looks like success;
* let a dry run enter them -> ``--live-dry-run`` would contact a third party while claiming to stop
before the first call.
The control asserts an un-configured run's tool list is unchanged, so no assertion above can pass
on something the run does anyway.
"""
from __future__ import annotations
from pathlib import Path
from typing import Any
import pytest
from portfolio_optimiser import run as run_module
from portfolio_optimiser.mcp_tools import McpServerConfig
from portfolio_optimiser.run import run_project
from portfolio_optimiser.simulation import ScriptedChatClient
from portfolio_optimiser.verdicts import VerdictStore
BUNDLE_DIR = Path(__file__).resolve().parents[1] / "shared" / "examples" / "bygg-energi-mikro"
_VERDICT_INPUT = {"decision": "approved", "rationale": "expert reviewed (sim)"}
_REPLY = (
'{"measure":"LED-retrofit av kontorbelysning","affected_items":'
'[{"code":"ENERGI-TOTAL-EL","quantity":300000,"unit_cost":1.0}],"claimed_saving_nok":30000}'
)
_SERVER = McpServerConfig(
name="prisregister",
transport="http",
url="https://intern.example/mcp",
allowed_tools=("lookup_unit_price",),
timeout_seconds=15,
)
class _FakeMcpTool:
"""Stands in for a MAF ``MCPTool``: an async context manager that RECORDS its lifecycle.
A real one would open a connection, which is exactly what a test must not do — and the thing
worth asserting is the lifecycle itself, not the protocol MAF already owns.
"""
def __init__(self, name: str) -> None:
self.name = name
self.entered = 0
self.exited = 0
self.entered_before_debate: bool | None = None
async def __aenter__(self) -> _FakeMcpTool:
self.entered += 1
return self
async def __aexit__(self, *exc: object) -> None:
self.exited += 1
@pytest.fixture()
def fake_tools(monkeypatch: pytest.MonkeyPatch) -> list[_FakeMcpTool]:
"""Replace the MAF client construction with recording doubles — the config still travels the
real path, only the socket-opening object is swapped."""
built: list[_FakeMcpTool] = []
def _build(configs: tuple[McpServerConfig, ...]) -> list[Any]:
# Return ONLY this call's tools. Accumulating across calls handed the second project both
# projects' tools under one name, and MAF refused it ("Duplicate tool name") — a defect in
# the double, but a useful reminder that each run builds its own clients.
fresh = [_FakeMcpTool(c.name) for c in configs]
built.extend(fresh)
return list(fresh)
monkeypatch.setattr(run_module, "build_mcp_tools", _build)
return built
@pytest.fixture()
def captured_tools(monkeypatch: pytest.MonkeyPatch) -> list[list[Any]]:
"""Capture what actually reaches the debate's ``tools=`` — the agents' real surface."""
seen: list[list[Any]] = []
original = run_module.fresh_workflow
def _spy(*args: Any, **kwargs: Any) -> Any:
seen.append(list(kwargs.get("tools") or []))
return original(*args, **kwargs)
monkeypatch.setattr(run_module, "fresh_workflow", _spy)
return seen
def _factory(_role: str) -> ScriptedChatClient:
return ScriptedChatClient(reply=_REPLY)
async def _run(**kwargs: Any):
return await run_project(
"BYGG-KONTOR-NORD",
"local",
docs_dir=str(BUNDLE_DIR),
bundle_dir=str(BUNDLE_DIR),
verdict_input=_VERDICT_INPUT,
store=VerdictStore(verdicts=[]),
client_factory=_factory,
**kwargs,
)
async def test_configured_server_becomes_a_tool_the_agents_have(fake_tools, captured_tools) -> None:
"""On the bundle path the agents had NO tools at all; a configured server is the first one."""
await _run(mcp_servers=(_SERVER,))
assert captured_tools, "the debate was never built"
assert any(isinstance(t, _FakeMcpTool) for t in captured_tools[0])
async def test_tools_are_opened_and_closed_around_the_debate(fake_tools) -> None:
"""Constructed is not connected. An ``MCPTool`` must be entered to expose anything, and exited
or the process hangs — so both halves are asserted, not just the first."""
await _run(mcp_servers=(_SERVER,))
assert fake_tools, "no MCP tool was built"
assert fake_tools[0].entered == 1
assert fake_tools[0].exited == 1
async def test_dry_run_never_contacts_a_configured_server(fake_tools) -> None:
"""``--live-dry-run`` stops before the first model call, and that promise has to cover egress
too: a dry run that opened a connection to a third party would be lying by omission."""
await _run(mcp_servers=(_SERVER,), live_dry_run=True)
assert all(t.entered == 0 for t in fake_tools)
async def test_run_without_mcp_servers_keeps_the_tool_list_unchanged(captured_tools) -> None:
"""CONTROL: with nothing configured the bundle path still hands the agents no tools — the
pre-Trekk-B behaviour, so every assertion above rests on the configuration and not on the run."""
await _run()
assert captured_tools[0] == []
async def test_portfolio_mode_gives_every_project_the_configured_tools(
fake_tools, captured_tools
) -> None:
"""A configured server reaches EVERY project in a portfolio pass. Without the threading the
flag would be accepted and silently dropped in one of the two modes — the defect class this
repo refuses by name ('refused, never ignored')."""
result = await run_module.run_portfolio(
["FV42-GSV-E1", "RV13-RAS-TP"],
"local",
store=VerdictStore(verdicts=[]),
client_factory=_factory,
mcp_servers=(_SERVER,),
)
assert len(result.runs) == 2
assert len(captured_tools) == 2
assert all(any(isinstance(t, _FakeMcpTool) for t in tools) for tools in captured_tools)