From b90ce839467086a3140c31809641379c83f7235c Mon Sep 17 00:00:00 2001 From: Renn F Date: Fri, 15 May 2026 02:54:39 +0200 Subject: [PATCH] =?UTF-8?q?fix(mcp):=20expose=20pass/fail=20to=20QA=20via?= =?UTF-8?q?=20IntentSpec=E2=86=92public=20name=20mapping?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Smoke-7 surfaced this: QA spawned, claim_review succeeded, but every attempt to call `pass()` fell through to dm/say workarounds. The MCP tool 'pass' never existed. Root cause: foundation.policy.lifecycle declares the intent verbs as `pass_review`/`fail_review` (Python-friendly names — `pass`/`fail` are keywords). intents_for_role(Role.QA) returns those names, the spawn manifest carries them, and flow_server reads them. But flow_server's _TOOLS dict has keys 'pass'/'fail' — the manifest's pass_review keys didn't match and got silently dropped from the registration. Fix: add _INTENT_TO_PUBLIC = {'pass_review': 'pass', 'fail_review': 'fail'} in flow_server. _register_tools transforms manifest names through it before _TOOLS lookup. Manifest entries map to the public MCP tool names the prompts advertise. Also fixed _VERB_RETRY_LIMITS keys in foundation.agent_loop — they used the IntentSpec names too, but the SDK receives the public name from /verb/attempted (derived from the flow URL path), so the limit entries never matched real rejections. Renamed to 'pass'/'fail'. 3 regression tests pin: pass/fail register under public names; IntentSpec names don't leak through; the registered tool POSTs to the correct orchestrator path. --- roboco/foundation/policy/agent_loop.py | 10 +- roboco/mcp/flow_server.py | 22 ++- .../test_flow_server_intent_public_mapping.py | 132 ++++++++++++++++++ 3 files changed, 158 insertions(+), 6 deletions(-) create mode 100644 tests/unit/mcp_servers/test_flow_server_intent_public_mapping.py diff --git a/roboco/foundation/policy/agent_loop.py b/roboco/foundation/policy/agent_loop.py index 2497ec49..b8390d88 100644 --- a/roboco/foundation/policy/agent_loop.py +++ b/roboco/foundation/policy/agent_loop.py @@ -50,9 +50,13 @@ VERB_RETRY_LIMITS: dict[str, int] = { "submit_up": 3, "complete": 3, "delegate": 3, - # QA / Doc handoffs: - "pass_review": 3, - "fail_review": 3, + # QA / Doc handoffs. Keys are the public MCP verb names (what the SDK + # receives via /verb/attempted, derived from the flow URL path). + # IntentSpec uses `pass_review`/`fail_review` internally; the MCP layer + # exposes them as `pass`/`fail`. Smoke-7 surfaced the mismatch — the + # old keys here never matched any actual rejection. + "pass": 3, + "fail": 3, "i_documented": 3, # PR open is more network-flake-tolerant: "open_pr": 5, diff --git a/roboco/mcp/flow_server.py b/roboco/mcp/flow_server.py index af11f09b..48d5b839 100644 --- a/roboco/mcp/flow_server.py +++ b/roboco/mcp/flow_server.py @@ -451,7 +451,11 @@ _TOOLS: dict[str, Any] = { "unclaim": unclaim, "resume": resume, "i_am_idle": i_am_idle, - # qa + # qa — keys are the public MCP tool names (what agents see and prompts + # advertise). `pass`/`fail` are Python keywords so the IntentSpec uses + # `pass_review`/`fail_review` internally; the public-name mapping below + # bridges the two so the manifest's IntentSpec entries register as + # `mcp__roboco-flow__pass` / `mcp__roboco-flow__fail`. "claim_review": claim_review, "pass": pass_review, "fail": fail_review, @@ -472,6 +476,17 @@ _TOOLS: dict[str, Any] = { } +# IntentSpec verb names → MCP public tool names. The IntentSpec layer uses +# Python-friendly identifiers (no reserved keywords); the MCP layer exposes +# the user-facing verb name. Smoke-7 surfaced this gap: the manifest carried +# `pass_review`/`fail_review` (IntentSpec names) but flow_server only had +# `pass`/`fail` keys, so QA's tools were silently dropped at registration. +_INTENT_TO_PUBLIC: dict[str, str] = { + "pass_review": "pass", + "fail_review": "fail", +} + + def _load_manifest_flow_tools() -> list[str] | None: """Read the spawn manifest and return its ``flow_tools`` list. @@ -527,14 +542,15 @@ def _register_tools() -> list[str]: ) log.error("flow_server: manifest missing", role=AGENT_ROLE, path=manifest_path) raise RuntimeError(msg) - unknown = [verb for verb in allowed if verb not in _TOOLS] + public = [_INTENT_TO_PUBLIC.get(verb, verb) for verb in allowed] + unknown = [verb for verb in public if verb not in _TOOLS] if unknown: log.warning( "flow_server: manifest references unimplemented verbs", role=AGENT_ROLE, missing=sorted(unknown), ) - names = [verb for verb in allowed if verb in _TOOLS] + names = [verb for verb in public if verb in _TOOLS] for verb in names: mcp.tool(name=verb)(_TOOLS[verb]) diff --git a/tests/unit/mcp_servers/test_flow_server_intent_public_mapping.py b/tests/unit/mcp_servers/test_flow_server_intent_public_mapping.py new file mode 100644 index 00000000..380bbff0 --- /dev/null +++ b/tests/unit/mcp_servers/test_flow_server_intent_public_mapping.py @@ -0,0 +1,132 @@ +"""Smoke-7: flow_server maps IntentSpec verb names to public MCP tool names. + +`pass`/`fail` are Python keywords so the IntentSpec layer (foundation) names +them `pass_review`/`fail_review`. The MCP layer must expose them under the +public names — agents/prompts say `pass(task_id, notes)`, not `pass_review`. + +Original bug: the manifest carried `pass_review` (from `intents_for_role`) +but `_TOOLS` dict had key `pass`. The mismatch silently dropped both verbs +from registration. QA's smoke-7 run loop-spammed `dm`/`say` because the +`pass` tool didn't exist. +""" + +from __future__ import annotations + +import importlib +import json +from typing import TYPE_CHECKING +from unittest.mock import MagicMock, patch + +import pytest + +if TYPE_CHECKING: + import types + from pathlib import Path + + +def _qa_manifest() -> dict[str, object]: + """A minimal QA manifest using IntentSpec names (mirrors prod).""" + return { + "agent_id": "00000000-0000-0000-0000-000000000099", + "role": "qa", + "team": "backend", + "workspace_path": "/tmp/test", + # These are exactly what intents_for_role(Role.QA) produces in prod. + "flow_tools": [ + "claim_review", + "pass_review", + "fail_review", + "give_me_work", + "i_am_blocked", + "i_am_idle", + "unclaim", + "resume", + ], + "do_tools": [], + "read_tools": [], + "write_tools": [], + "bash_allowed": True, + "subagent_allowed": False, + "subagent_model": None, + "env": {}, + } + + +@pytest.fixture() +def flow_module_qa(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> types.ModuleType: + """Import flow_server with a QA manifest mounted at the expected path.""" + manifest_path = tmp_path / "tool-manifest.json" + manifest_path.write_text(json.dumps(_qa_manifest())) + + monkeypatch.setenv("ROBOCO_AGENT_ID", "00000000-0000-0000-0000-000000000099") + monkeypatch.setenv("ROBOCO_AGENT_ROLE", "qa") + monkeypatch.setenv("ROBOCO_ORCHESTRATOR_URL", "http://test-orchestrator:8000") + monkeypatch.setenv("ROBOCO_SDK_URL", "http://test-sdk:9000") + monkeypatch.setenv("ROBOCO_TOOL_MANIFEST_PATH", str(manifest_path)) + + import roboco.mcp.flow_server as srv + + importlib.reload(srv) + return srv + + +def test_pass_and_fail_register_under_public_names( + flow_module_qa: types.ModuleType, +) -> None: + """The manifest's pass_review/fail_review register as MCP tools 'pass'/'fail'.""" + registered = flow_module_qa._register_tools() + assert "pass" in registered, ( + f"public name 'pass' not registered. Registered: {sorted(registered)}. " + "The intent-to-public mapping is broken — QA cannot transition tasks." + ) + assert "fail" in registered + # IntentSpec names must NOT be exposed directly as MCP tool names. + assert "pass_review" not in registered + assert "fail_review" not in registered + + +def test_intent_public_mapping_used_on_unknowns( + flow_module_qa: types.ModuleType, +) -> None: + """Unmapped verbs not in _TOOLS surface as unknown; mapped ones don't.""" + # pass_review and fail_review must NOT appear in the 'unknown' warning log + # because they map to pass/fail which ARE in _TOOLS. + public_map = flow_module_qa._INTENT_TO_PUBLIC + assert public_map["pass_review"] == "pass" + assert public_map["fail_review"] == "fail" + + +def test_post_to_correct_orchestrator_path( + flow_module_qa: types.ModuleType, +) -> None: + """Calling the registered 'pass' tool POSTs to /api/v2/flow/qa/pass.""" + captured: list[tuple[str, dict]] = [] + + def _client_factory(*_a: object, **_kw: object) -> MagicMock: + client = MagicMock() + client.__enter__ = MagicMock(return_value=client) + client.__exit__ = MagicMock(return_value=False) + + def _post(url: str, **kwargs: object) -> MagicMock: + captured.append((url, kwargs.get("json", {}))) + resp = MagicMock() + resp.json.return_value = { + "status": "awaiting_documentation", + "error": None, + } + return resp + + client.post.side_effect = _post + return client + + with patch("httpx.Client", side_effect=_client_factory): + result = flow_module_qa.pass_review("task-id-123", notes="LGTM") + + assert result["status"] == "awaiting_documentation" + orchestrator_calls = [(u, b) for u, b in captured if "test-orchestrator" in u] + assert len(orchestrator_calls) == 1 + url, body = orchestrator_calls[0] + assert url.endswith("/api/v2/flow/qa/pass"), ( + f"pass_review must POST to /qa/pass, got {url}" + ) + assert body == {"task_id": "task-id-123", "notes": "LGTM"}