External/roboco
1
0
Fork 0
mirror of https://github.com/rennf93/roboco.git synced 2026-08-03 07:23:24 +02:00
Code Issues Packages Projects Releases Wiki Activity

[F068][F069] mcp servers: classify all rejection shapes + envelope 404s

Browse Source 
F068: the do/flow-server circuit breaker only counted rejections whose
`error` field was a STRING in _CIRCUIT_REJECTION_KINDS. A 422 validation
failure (no `error` field, a `detail` list) and a 500/HTTPException
(dict-shaped `error` from the exception handlers) both bypassed the breaker
→ unbounded retries on a storm of either. Added _classify_rejection(payload)
(shared, applied to both servers) mapping all three shapes to a counted kind:
string error (existing), dict error → substring-mapped code
(*DENIED*/*AUTHORIZED*/*FORBIDDEN*/*PERMISSION*→not_authorized,
INVALID_INPUT/*VALIDATION*→incomplete_input, *NOT_FOUND*→None parity, else
→invalid_state), 422 detail→incomplete_input. The dict TypeError defence lives
in the classifier (isinstance, never dict-in-frozenset).

F069: a manifest-registered verb whose HTTP route is missing got FastAPI's raw
`{"detail":"Not Found"}` 404 body — a non-envelope payload the breaker
couldn't classify, so a storm bypassed it. _post now synthesizes an
invalid_state Envelope rejection (with a remediate hint → i_am_blocked/i_am_idle)
for a 404 status, routed through _record_and_check_circuit so the breaker counts
it. A 404 that carries a real Envelope (error field present) is surfaced as-is,
preserving test_flow_post_returns_envelope_on_404. TDD: 422/dict/404 tests in
both server test files; updated test_dict_shaped_error_does_not_crash to assert
the SDK is now called with not_authorized (replacing the pass-through assertion
that encoded the bug).
This commit is contained in:
Renn F
2026-06-28 17:16:37 +02:00
parent 2e0792d00d
commit 648f45f2fc
4 changed files with 579 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files
tests/unit/mcp_servers/test_do_server_circuit_breaker.py
+177 -5
View File
@@ -263,12 +263,19 @@ def test_verb_extracted_from_path(do_module: types.ModuleType) -> None:
def test_dict_shaped_error_does_not_crash(do_module: types.ModuleType) -> None:
"""A RobocoError.to_dict()-shaped response must pass through without TypeError.
"""A RobocoError.to_dict()-shaped response must not TypeError the breaker.
Smoke-7: A2AAccessDeniedError escaped to middleware and was rendered as
{'error': {'code': ..., 'message': ..., 'details': ...}}. The circuit
breaker's `error in frozenset` check then crashed with
`TypeError: unhashable type: 'dict'`.
F068: a dict-shaped `error` is a retry-storm-worthy rejection (the
orchestrator's exception handlers all surface this shape on 4xx/5xx),
so the breaker must COUNT it — mapped to a counted kind by the
classifier — rather than passing it through silently. The original
dict payload still reaches the agent (the breaker only substitutes
when open). No TypeError may be raised either way.
"""
factory, captured = _make_client(
orchestrator_response={
@@ -278,12 +285,177 @@ def test_dict_shaped_error_does_not_crash(do_module: types.ModuleType) -> None:
"details": {},
}
},
sdk_response=None, # SDK must not be touched
sdk_response={
"verb": "dm",
"task_id": None,
"attempts": 1,
"limit": 3,
"window_seconds": 60,
"open": False,
"circuit_envelope": None,
},
)
# No TypeError; payload passes through untouched.
# No TypeError; the dict-shaped rejection is forwarded to the SDK.
with patch("httpx.Client", side_effect=factory):
result = do_module.dm(recipient="qa-all", text="x")
# Original dict payload still reaches the agent (breaker not open).
assert isinstance(result["error"], dict)
assert result["error"]["code"] == "A2A_ACCESS_DENIED"
# SDK breaker MUST NOT have been called for a non-string error.
assert all("test-sdk" not in url for url, _ in captured)
# SDK breaker MUST now be called so a storm of these counts.
sdk_calls = [(url, body) for url, body in captured if "test-sdk" in url]
assert len(sdk_calls) == 1
# ACCESS_DENIED maps to the not_authorized counted kind.
assert sdk_calls[0][1]["rejection_kind"] == "not_authorized"
def test_422_validation_failure_counts_as_incomplete_input(
do_module: types.ModuleType,
) -> None:
"""F068: a 422 validation-failure body (`{"detail": [...], "body": ...}`,
no `error` field) must count toward the breaker — a storm of 422s is
retry-storm-worthy (the agent keeps re-submitting malformed input).
Mapped to `incomplete_input` (the agent's input was incomplete/invalid).
"""
factory, captured = _make_client(
orchestrator_response={
"detail": [
{
"loc": ["body", "text"],
"msg": "field required",
"type": "value_error.missing",
}
],
"body": None,
},
sdk_response={
"verb": "note",
"task_id": None,
"attempts": 1,
"limit": 3,
"window_seconds": 60,
"open": False,
"circuit_envelope": None,
},
)
with patch("httpx.Client", side_effect=factory):
do_module.note(text="")
sdk_calls = [(url, body) for url, body in captured if "test-sdk" in url]
assert len(sdk_calls) == 1
assert sdk_calls[0][1]["rejection_kind"] == "incomplete_input"
def test_dict_shaped_internal_error_counts_as_invalid_state(
do_module: types.ModuleType,
) -> None:
"""F068: a 500 INTERNAL_ERROR dict-shaped response (generic_exception_handler)
must count toward the breaker as `invalid_state` — a storm of 500s is
retry-storm-worthy and previously bypassed the breaker entirely.
"""
factory, captured = _make_client(
orchestrator_response={
"error": {
"code": "INTERNAL_ERROR",
"message": "An internal error occurred",
"details": {"correlation_id": "abc"},
}
},
sdk_response={
"verb": "commit",
"task_id": None,
"attempts": 1,
"limit": 3,
"window_seconds": 60,
"open": False,
"circuit_envelope": None,
},
)
with patch("httpx.Client", side_effect=factory):
do_module.commit(message="[abc12345] a valid commit message here")
sdk_calls = [(url, body) for url, body in captured if "test-sdk" in url]
assert len(sdk_calls) == 1
assert sdk_calls[0][1]["rejection_kind"] == "invalid_state"
def test_dict_shaped_invalid_input_counts_as_incomplete_input(
do_module: types.ModuleType,
) -> None:
"""F068: a dict-shaped INVALID_INPUT (mapped from 422 by http_exception_handler)
counts as `incomplete_input` — semantically the agent's input was invalid.
"""
factory, captured = _make_client(
orchestrator_response={
"error": {"code": "INVALID_INPUT", "message": "bad payload"}
},
sdk_response={
"verb": "say",
"task_id": None,
"attempts": 1,
"limit": 3,
"window_seconds": 60,
"open": False,
"circuit_envelope": None,
},
)
with patch("httpx.Client", side_effect=factory):
do_module.say(channel="backend-cell", text="x")
sdk_calls = [(url, body) for url, body in captured if "test-sdk" in url]
assert len(sdk_calls) == 1
assert sdk_calls[0][1]["rejection_kind"] == "incomplete_input"
# ---------------------------------------------------------------------------
# F069 — a manifest-registered content tool whose route is missing must
# return an envelope rejection (not a raw 404 body) so the breaker counts it.
# ---------------------------------------------------------------------------
def test_missing_route_404_returns_envelope_and_counts(
do_module: types.ModuleType,
) -> None:
"""F069: a 404 from the orchestrator (manifest-registered tool with no
route) must surface as a proper `invalid_state` Envelope rejection — not
FastAPI's raw ``{"detail": "Not Found"}`` body — and the breaker must
count it. Without this, the agent retries the missing tool forever and
the breaker never trips. Mirrors flow_server's 404 handling.
"""
captured: list[tuple[str, dict[str, Any] | None]] = []
def _client_factory(*_args: Any, **_kwargs: Any) -> MagicMock:
client = MagicMock()
client.__enter__ = MagicMock(return_value=client)
client.__exit__ = MagicMock(return_value=False)
def _post(url: str, **kwargs: Any) -> MagicMock:
captured.append((url, kwargs.get("json")))
resp = MagicMock()
if "test-sdk" in url:
resp.json.return_value = {
"verb": "evidence",
"task_id": None,
"attempts": 1,
"limit": 3,
"window_seconds": 60,
"open": False,
"circuit_envelope": None,
}
else:
# FastAPI's default 404 for a missing route.
resp.status_code = 404
resp.json.return_value = {"detail": "Not Found"}
return resp
client.post.side_effect = _post
return client
with patch("httpx.Client", side_effect=_client_factory):
result = do_module.evidence(task_id="some-task")
# Envelope rejection, not the raw 404 body.
assert result["error"] == "invalid_state"
assert "remediate" in result
assert "detail" not in result # the raw 404 body was not passed through
# Breaker was notified so a storm of these trips it.
sdk_calls = [(url, body) for url, body in captured if "test-sdk" in url]
assert len(sdk_calls) == 1
_, sdk_body = sdk_calls[0]
assert sdk_body is not None
assert sdk_body["rejection_kind"] == "invalid_state"
tests/unit/mcp_servers/test_flow_server_circuit_breaker.py
+160
View File
@@ -429,3 +429,163 @@ def test_task_id_none_is_forwarded_as_null(flow_module: types.ModuleType) -> Non
_, sdk_body = sdk_calls[0]
assert sdk_body["task_id"] is None
assert sdk_body["verb"] == "give_me_work"
# ---------------------------------------------------------------------------
# F068 — non-string-error / no-error-field rejection shapes are counted
# ---------------------------------------------------------------------------
def test_422_validation_failure_counts_as_incomplete_input(
flow_module: types.ModuleType,
) -> None:
"""F068: a 422 validation-failure body (`{"detail": [...]}`, no `error`)
must count toward the breaker as `incomplete_input` — a storm of 422s is
retry-storm-worthy. Mirrors do_server's classifier (the two servers share
the same breaker logic and must stay in parity).
"""
factory, captured = _make_client(
orchestrator_response={
"detail": [
{
"loc": ["body", "task_id"],
"msg": "field required",
"type": "value_error.missing",
}
],
"body": None,
},
sdk_response={
"verb": "i_am_done",
"task_id": "task-A",
"attempts": 1,
"limit": 3,
"window_seconds": 60,
"open": False,
"circuit_envelope": None,
},
)
with patch("httpx.Client", side_effect=factory):
flow_module.i_am_done("task-A")
sdk_calls = [(url, body) for url, body in captured if "test-sdk" in url]
assert len(sdk_calls) == 1
assert sdk_calls[0][1]["rejection_kind"] == "incomplete_input"
def test_dict_shaped_internal_error_counts_as_invalid_state(
flow_module: types.ModuleType,
) -> None:
"""F068: a 500 INTERNAL_ERROR dict-shaped response (generic_exception_handler)
counts as `invalid_state` — a storm of 500s previously bypassed the breaker.
"""
factory, captured = _make_client(
orchestrator_response={
"error": {
"code": "INTERNAL_ERROR",
"message": "An internal error occurred",
"details": {"correlation_id": "abc"},
}
},
sdk_response={
"verb": "i_am_done",
"task_id": "task-A",
"attempts": 1,
"limit": 3,
"window_seconds": 60,
"open": False,
"circuit_envelope": None,
},
)
with patch("httpx.Client", side_effect=factory):
flow_module.i_am_done("task-A")
sdk_calls = [(url, body) for url, body in captured if "test-sdk" in url]
assert len(sdk_calls) == 1
assert sdk_calls[0][1]["rejection_kind"] == "invalid_state"
def test_dict_shaped_not_found_does_not_count(
flow_module: types.ModuleType,
) -> None:
"""F068: a dict-shaped NOT_FOUND (404 family) does NOT count — parity with
the string-error contract that a `not_found` rejection isn't counted
(retrying a missing resource won't help until state changes).
"""
factory, captured = _make_client(
orchestrator_response={
"error": {"code": "TASK_NOT_FOUND", "message": "no such task"}
},
sdk_response=None, # SDK must not be touched
)
with patch("httpx.Client", side_effect=factory):
result = flow_module.i_am_done("task-A")
assert isinstance(result["error"], dict)
assert result["error"]["code"] == "TASK_NOT_FOUND"
assert all("test-sdk" not in url for url, _ in captured)
# ---------------------------------------------------------------------------
# F069 — a manifest-registered verb whose route is missing must return an
# envelope rejection (not a raw 404 body) so the breaker counts it.
# ---------------------------------------------------------------------------
def _make_404_client() -> tuple[Any, list[tuple[str, dict[str, Any] | None]]]:
"""Build an httpx.Client mock whose orchestrator call returns FastAPI's
default 404 body (``{"detail": "Not Found"}``, status 404) — the shape a
manifest-registered verb sees when its route is missing. The SDK call
returns a not-yet-open breaker so the original envelope is preserved.
"""
captured: list[tuple[str, dict[str, Any] | None]] = []
def _client_factory(*_args: Any, **_kwargs: Any) -> MagicMock:
client = MagicMock()
client.__enter__ = MagicMock(return_value=client)
client.__exit__ = MagicMock(return_value=False)
def _post(url: str, **kwargs: Any) -> MagicMock:
captured.append((url, kwargs.get("json")))
resp = MagicMock()
if "test-sdk" in url:
resp.json.return_value = {
"verb": "triage",
"task_id": None,
"attempts": 1,
"limit": 3,
"window_seconds": 60,
"open": False,
"circuit_envelope": None,
}
else:
# FastAPI's default 404 for a missing route.
resp.status_code = 404
resp.json.return_value = {"detail": "Not Found"}
return resp
client.post.side_effect = _post
return client
return _client_factory, captured
def test_missing_route_404_returns_envelope_and_counts(
flow_module: types.ModuleType,
) -> None:
"""F069: a 404 from the orchestrator (manifest-registered verb with no
route) must surface as a proper `invalid_state` Envelope rejection — not
FastAPI's raw ``{"detail": "Not Found"}`` body — and the breaker must
count it. Without this, the agent retries the missing route forever and
the breaker never trips.
"""
factory, captured = _make_404_client()
with patch("httpx.Client", side_effect=factory):
result = flow_module.triage()
# Envelope rejection, not the raw 404 body.
assert result["error"] == "invalid_state"
assert "remediate" in result
assert "detail" not in result # the raw 404 body was not passed through
# Breaker was notified so a storm of these trips it.
sdk_calls = [(url, body) for url, body in captured if "test-sdk" in url]
assert len(sdk_calls) == 1
_, sdk_body = sdk_calls[0]
assert sdk_body is not None
assert sdk_body["rejection_kind"] == "invalid_state"