mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix(mcp): expose pass/fail to QA via IntentSpec→public name mapping
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.
This commit is contained in:
@@ -50,9 +50,13 @@ VERB_RETRY_LIMITS: dict[str, int] = {
|
|||||||
"submit_up": 3,
|
"submit_up": 3,
|
||||||
"complete": 3,
|
"complete": 3,
|
||||||
"delegate": 3,
|
"delegate": 3,
|
||||||
# QA / Doc handoffs:
|
# QA / Doc handoffs. Keys are the public MCP verb names (what the SDK
|
||||||
"pass_review": 3,
|
# receives via /verb/attempted, derived from the flow URL path).
|
||||||
"fail_review": 3,
|
# 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,
|
"i_documented": 3,
|
||||||
# PR open is more network-flake-tolerant:
|
# PR open is more network-flake-tolerant:
|
||||||
"open_pr": 5,
|
"open_pr": 5,
|
||||||
|
|||||||
@@ -451,7 +451,11 @@ _TOOLS: dict[str, Any] = {
|
|||||||
"unclaim": unclaim,
|
"unclaim": unclaim,
|
||||||
"resume": resume,
|
"resume": resume,
|
||||||
"i_am_idle": i_am_idle,
|
"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,
|
"claim_review": claim_review,
|
||||||
"pass": pass_review,
|
"pass": pass_review,
|
||||||
"fail": fail_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:
|
def _load_manifest_flow_tools() -> list[str] | None:
|
||||||
"""Read the spawn manifest and return its ``flow_tools`` list.
|
"""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)
|
log.error("flow_server: manifest missing", role=AGENT_ROLE, path=manifest_path)
|
||||||
raise RuntimeError(msg)
|
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:
|
if unknown:
|
||||||
log.warning(
|
log.warning(
|
||||||
"flow_server: manifest references unimplemented verbs",
|
"flow_server: manifest references unimplemented verbs",
|
||||||
role=AGENT_ROLE,
|
role=AGENT_ROLE,
|
||||||
missing=sorted(unknown),
|
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:
|
for verb in names:
|
||||||
mcp.tool(name=verb)(_TOOLS[verb])
|
mcp.tool(name=verb)(_TOOLS[verb])
|
||||||
|
|||||||
@@ -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"}
|
||||||
Reference in New Issue
Block a user