open/portfolio-optimiser
1
0
Fork 0
Code Issues Releases 3 Activity

feat(mcp): concrete MCP servers become tools the agents can call during a run

Browse source 
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
This commit is contained in:
Kjell Tore Guttormsen 2026-08-05 16:53:07 +02:00
commit 9668e17f2f
8 changed files with 706 additions and 4 deletions
Download patch file Download diff file Expand all files Collapse all files

87
tests/test_mandate_cli.py
View file

@ -184,3 +184,90 @@ def test_no_mandate_prints_neither_block(bundle, replies_file, capsys) -> None:
assert rc == 0, out
assert "Run mandate for" not in out
assert "Mandate outcome" not in out
# --- the egress declaration (Trekk B3): what a run will contact, before it contacts it ----------
_MCP = {
"servers": [
{
"name": "prisregister",
"transport": "http",
"url": "https://intern.example/mcp",
"allowed_tools": ["lookup_unit_price"],
"timeout_seconds": 15,
}
]
}
@pytest.fixture()
def mcp_file(tmp_path: Path) -> Path:
path = tmp_path / "mcp.json"
path.write_text(json.dumps(_MCP), encoding="utf-8")
return path
def test_configured_servers_are_named_before_the_run(
bundle, mandate_file, mcp_file, capsys
) -> None:
"""Every server and every permitted tool is named in the announcement — a run never reaches a
service it did not declare. ``--live-dry-run`` keeps the assertion offline."""
rc = run.main(
[
"BYGG-KONTOR-NORD",
"--docs-dir",
str(bundle),
"--bundle-dir",
str(bundle),
"--mandate",
str(mandate_file),
"--mcp-config",
str(mcp_file),
"--live-dry-run",
]
)
out = capsys.readouterr().out
assert rc == 0, out
assert "prisregister (lookup_unit_price)" in out
assert "no external services" not in out
def test_egress_is_declared_even_without_a_mandate(bundle, mcp_file, capsys) -> None:
"""The declaration cannot depend on a mandate being present. Without this, configuring servers
and omitting ``--mandate`` would contact third parties with nothing printed at all — silent
egress, which is the one thing this repo's data rules forbid outright."""
rc = run.main(
[
"BYGG-KONTOR-NORD",
"--docs-dir",
str(bundle),
"--bundle-dir",
str(bundle),
"--mcp-config",
str(mcp_file),
"--live-dry-run",
]
)
out = capsys.readouterr().out
assert rc == 0, out
assert "prisregister (lookup_unit_price)" in out
def test_malformed_mcp_config_refuses_the_run(bundle, replies_file, tmp_path, capsys) -> None:
"""An egress config that cannot be read refuses — degrading it to 'no external services' would
make the announcement describe a run nobody configured."""
bad = tmp_path / "mcp.json"
bad.write_text('{"servers": [{"name": "x", "transport": "http"}]}', encoding="utf-8")
rc = run.main(_argv(bundle, replies_file, "--mcp-config", str(bad)))
assert rc == 1
assert "refused" in capsys.readouterr().err.lower()
def test_no_mcp_config_contacts_nothing(bundle, replies_file, mandate_file, capsys) -> None:
"""CONTROL: without ``--mcp-config`` the announcement says so explicitly. An omitted line would
read the same as an unchecked one."""
rc = run.main(_argv(bundle, replies_file, "--mandate", str(mandate_file)))
out = capsys.readouterr().out
assert rc == 0, out
assert "no external services" in out

162
tests/test_mcp_run_loadbearing.py Normal file
View file

@ -0,0 +1,162 @@
"""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)

180
tests/test_mcp_tools.py Normal file
View file

@ -0,0 +1,180 @@
"""Concrete MCP servers as run-path tools (Trekk B1) — config + tool construction.
Krav 3: a run must be able to reach an external service (price register, meter data, contract
registry) while it is working. MAF already ships the client (``MCPStdioTool`` /
``MCPStreamableHTTPTool``), so what this module owns is the part MAF cannot decide for us: which
servers a run may contact, which of their tools it may call, what it waits, and where the
credential comes from.
Every refusal here exists because its absence is a real hazard, not for tidiness:
* an EMPTY ``allowed_tools`` would let a server expose any tool it likes to the agents — authority
by whatever the far end happens to serve. You must name what you allow;
* a missing/blank timeout would be an unbounded wait against a third party (the repo's fail-fast
invariant: stop criteria are never optional);
* an UNKNOWN field is refused rather than ignored, which is what keeps a literal secret from being
parked in the config file and silently carried along;
* a named credential env var that is not set refuses the run instead of quietly calling the
service unauthenticated.
"""
from __future__ import annotations
import json
import pytest
from agent_framework import MCPStdioTool, MCPStreamableHTTPTool
from pydantic import ValidationError
from portfolio_optimiser.mcp_tools import (
McpServerConfig,
build_mcp_tools,
load_mcp_config,
service_labels,
)
_STDIO = {
"name": "maalerdata",
"transport": "stdio",
"command": "uvx",
"args": ["maalerdata-mcp"],
"allowed_tools": ["read_meter"],
"timeout_seconds": 30,
}
_HTTP = {
"name": "prisregister",
"transport": "http",
"url": "https://intern.example/mcp",
"allowed_tools": ["lookup_unit_price", "list_categories"],
"timeout_seconds": 15,
}
def _write(tmp_path, *servers) -> str:
path = tmp_path / "mcp.json"
path.write_text(json.dumps({"servers": list(servers)}), encoding="utf-8")
return str(path)
# --- config shape ------------------------------------------------------------------------------
def test_stdio_and_http_configs_load(tmp_path) -> None:
configs = load_mcp_config(_write(tmp_path, _STDIO, _HTTP))
assert [c.name for c in configs] == ["maalerdata", "prisregister"]
assert configs[0].transport == "stdio"
assert configs[1].allowed_tools == ("lookup_unit_price", "list_categories")
def test_stdio_without_command_refused(tmp_path) -> None:
"""A stdio server with nothing to launch is not a server."""
with pytest.raises(ValidationError):
load_mcp_config(_write(tmp_path, {**_STDIO, "command": None}))
def test_http_without_url_refused(tmp_path) -> None:
with pytest.raises(ValidationError):
load_mcp_config(_write(tmp_path, {**_HTTP, "url": None}))
def test_transport_and_coordinates_must_agree(tmp_path) -> None:
"""A stdio server carrying a URL (or an http server carrying a command) is ambiguous about
what would actually be contacted — refused rather than resolved by precedence."""
with pytest.raises(ValidationError):
load_mcp_config(_write(tmp_path, {**_STDIO, "url": "https://intern.example/mcp"}))
with pytest.raises(ValidationError):
load_mcp_config(_write(tmp_path, {**_HTTP, "command": "uvx"}))
def test_unknown_transport_refused(tmp_path) -> None:
"""The transport set is CLOSED — a config can never name arbitrary machinery to load
(the same rule ``--embedder-config`` follows)."""
with pytest.raises(ValidationError):
load_mcp_config(_write(tmp_path, {**_STDIO, "transport": "carrier-pigeon"}))
def test_empty_allowed_tools_refused(tmp_path) -> None:
"""Closed by default: you must name the tools a run may call. An empty list would hand the
agents whatever the far end chooses to expose."""
with pytest.raises(ValidationError):
load_mcp_config(_write(tmp_path, {**_STDIO, "allowed_tools": []}))
def test_non_positive_timeout_refused(tmp_path) -> None:
"""Fail-fast: an unbounded wait against a third party is exactly what stop criteria exist
to prevent."""
with pytest.raises(ValidationError):
load_mcp_config(_write(tmp_path, {**_STDIO, "timeout_seconds": 0}))
def test_unknown_field_refused(tmp_path) -> None:
"""Refused, never ignored — and this is the rule that keeps a literal secret out of the file:
there is no field for one, and an unrecognised key does not slip through unnoticed."""
with pytest.raises(ValidationError):
load_mcp_config(_write(tmp_path, {**_STDIO, "credential": "sk-live-not-here-please"}))
def test_duplicate_server_names_refused(tmp_path) -> None:
"""``name`` is how a server is identified in the egress declaration; two servers on one name
would make the announcement ambiguous about what is contacted."""
with pytest.raises(ValidationError):
load_mcp_config(_write(tmp_path, _STDIO, {**_STDIO, "command": "other"}))
def test_missing_config_file_refused(tmp_path) -> None:
with pytest.raises(FileNotFoundError):
load_mcp_config(str(tmp_path / "nope.json"))
def test_malformed_config_refused(tmp_path) -> None:
bad = tmp_path / "mcp.json"
bad.write_text("not json", encoding="utf-8")
with pytest.raises(ValidationError):
load_mcp_config(str(bad))
# --- tool construction -------------------------------------------------------------------------
def test_builds_the_right_client_per_transport() -> None:
tools = build_mcp_tools((McpServerConfig(**_STDIO), McpServerConfig(**_HTTP)))
assert isinstance(tools[0], MCPStdioTool)
assert isinstance(tools[1], MCPStreamableHTTPTool)
def test_allowed_tools_and_timeout_reach_the_client() -> None:
"""The allowlist and the timeout are the two limits that matter, so they are asserted on the
built client — not merely on the config that was meant to produce it."""
tool = build_mcp_tools((McpServerConfig(**_HTTP),))[0]
assert set(tool.allowed_tools or ()) == {"lookup_unit_price", "list_categories"}
assert tool.request_timeout == 15
def test_credential_is_read_from_the_environment_at_build_time(monkeypatch) -> None:
"""The config names an env var; the value never lives in the file."""
monkeypatch.setenv("PRISREGISTER_TOKEN", "s3cret")
cfg = McpServerConfig(**{**_HTTP, "credential_env": "PRISREGISTER_TOKEN"})
tool = build_mcp_tools((cfg,))[0]
assert isinstance(tool, MCPStreamableHTTPTool) # built without raising
def test_missing_credential_env_refuses(monkeypatch) -> None:
"""A named-but-unset credential refuses rather than calling the service unauthenticated —
an anonymous call can succeed with the wrong scope, which is worse than not calling."""
monkeypatch.delenv("PRISREGISTER_TOKEN", raising=False)
cfg = McpServerConfig(**{**_HTTP, "credential_env": "PRISREGISTER_TOKEN"})
with pytest.raises(ValueError, match="PRISREGISTER_TOKEN"):
build_mcp_tools((cfg,))
# --- the egress declaration --------------------------------------------------------------------
def test_service_labels_name_every_server_and_its_allowed_tools() -> None:
"""These strings go straight into the run announcement, so an operator can see WHAT will be
contacted and WHICH tools are permitted before anything is spent."""
labels = service_labels((McpServerConfig(**_STDIO), McpServerConfig(**_HTTP)))
assert labels == (
"maalerdata (read_meter)",
"prisregister (lookup_unit_price, list_categories)",
)