diff --git a/scripts/build_runtime_package.py b/scripts/build_runtime_package.py index 54d1647a..1a5f02ec 100755 --- a/scripts/build_runtime_package.py +++ b/scripts/build_runtime_package.py @@ -58,6 +58,7 @@ TOP_LEVEL_MODULES = { "adapter_base", "agent", "agents_md", + "boot_routes", "card_helpers", "config", "configs_dir", diff --git a/workspace/boot_routes.py b/workspace/boot_routes.py new file mode 100644 index 00000000..a2c849d6 --- /dev/null +++ b/workspace/boot_routes.py @@ -0,0 +1,84 @@ +"""Build the Starlette routes for a workspace from its (card, adapter +state) pair. + +Pairs with PR #2756, which decoupled ``/.well-known/agent-card.json`` from +``adapter.setup()`` failure. main.py was the only consumer and was +``# pragma: no cover`` — so the wiring (card-route mounted unconditionally, +JSON-RPC route swapped between DefaultRequestHandler and the +not-configured handler based on ``adapter_ready``) had no pytest coverage. + +A future refactor that re-couples the two would silently bypass PR #2756 +and shipped the original "stuck booting forever" UX again. That gap is +what closes here: extract the route-assembly into a pure function whose +behaviour is unit-testable with Starlette's TestClient, and have main.py +call it. Issue molecule-core#2761. +""" +from __future__ import annotations + +from typing import Any + +from starlette.routing import Route + +from not_configured_handler import make_not_configured_handler + +# Heavy a2a-sdk imports are lazy: deferred to inside build_routes so +# tests that exercise only the not-configured branch (no executor) don't +# need a2a.server.request_handlers / routes stubbed in their conftest. +# Production boot pays the import cost once, on workspace startup. + + +def build_routes( + agent_card: Any, + executor: Any | None, + adapter_error: str | None, +) -> list: + """Return the list of Starlette routes for this workspace. + + Always mounts ``/.well-known/agent-card.json`` from ``agent_card``. + + JSON-RPC route at ``/`` swaps based on adapter state: + + * ``executor`` is non-None → ``DefaultRequestHandler`` with the + executor (production happy-path). + * ``executor`` is None → ``not_configured_handler`` returning JSON-RPC + ``-32603`` with ``adapter_error`` in ``error.data``. The + workspace stays REACHABLE (operator can introspect, deprovision, + redeploy with corrected env) instead of crash-looping invisibly. + + The two branches are mutually exclusive — caller passes one or the + other, never both. Test coverage at ``tests/test_boot_routes.py`` + pins the contract. + """ + from a2a.server.routes import create_agent_card_routes + + routes: list = [] + routes.extend(create_agent_card_routes(agent_card)) + + if executor is not None: + from a2a.server.request_handlers import DefaultRequestHandler + from a2a.server.routes import create_jsonrpc_routes + from a2a.server.tasks import InMemoryTaskStore + + handler = DefaultRequestHandler( + agent_executor=executor, + task_store=InMemoryTaskStore(), + agent_card=agent_card, + ) + # enable_v0_3_compat=True is the JSON-RPC wire-compat path: clients + # using v0.3-shaped payloads (`"role": "user"` lowercase + camelCase + # Pydantic field names) can talk to us without re-deploying. + # Outbound payloads must also use v0.3 shape — see main.py's + # original comment block for the full a2a-sdk 1.x migration note. + routes.extend( + create_jsonrpc_routes( + request_handler=handler, + rpc_url="/", + enable_v0_3_compat=True, + ) + ) + else: + routes.append( + Route("/", make_not_configured_handler(adapter_error), methods=["POST"]) + ) + + return routes diff --git a/workspace/main.py b/workspace/main.py index ca81ea01..5ae5ebef 100644 --- a/workspace/main.py +++ b/workspace/main.py @@ -277,54 +277,16 @@ async def main(): # pragma: no cover # 7. Wrap in A2A. # - # Regression fix (#204): PR #198 tried to wire push_config_store + - # push_sender to satisfy #175 (push notification capability), but - # PushNotificationSender is an abstract base class in the a2a-sdk and - # can't be instantiated directly. Passing it crashed main.py on startup - # with `TypeError: Can't instantiate abstract class`. Dropped back to - # DefaultRequestHandler's own defaults — pushNotifications capability - # in the AgentCard below is still advertised via AgentCapabilities so - # clients know we COULD do pushes; actually implementing them requires - # a concrete sender subclass, tracked as a Phase-H follow-up to #175. - routes = [] - routes.extend(create_agent_card_routes(agent_card)) - - if adapter_ready: - handler = DefaultRequestHandler( - agent_executor=executor, - task_store=InMemoryTaskStore(), - # a2a-sdk 1.x added agent_card as a required positional/keyword - # argument — it's used internally for capability dispatch (e.g. - # routing tasks/get historyLength based on the card's protocol - # version). Pass the same agent_card we registered with the - # platform so the handler's capability surface matches what the - # AgentCard advertises. - agent_card=agent_card, - ) - # v1: replace A2AStarletteApplication with Starlette route factory. - # rpc_url is required in a2a-sdk 1.x (was implicit at root in 0.x). - # Use '/' to match a2a.utils.constants.DEFAULT_RPC_URL — that's also - # what the platform's a2a_proxy.go POSTs to (it forwards to the - # workspace's URL without appending a path). Card endpoint stays at - # the well-known path /.well-known/agent-card.json (handled by - # create_agent_card_routes default). - # enable_v0_3_compat=True is the JSON-RPC wire-compat path: clients - # using v0.3-shaped payloads (`"role": "user"` lowercase + camelCase - # Pydantic field names) can talk to us without re-deploying. - routes.extend(create_jsonrpc_routes(request_handler=handler, rpc_url="/", enable_v0_3_compat=True)) - else: - # Misconfigured: serve the card but reject JSON-RPC with -32603 so - # canvas surfaces a useful "agent not configured: " instead - # of letting requests time out. Handler factory is in its own module - # so the behavior is unit-testable (workspace/tests/test_not_configured_handler.py). - from starlette.routing import Route - from not_configured_handler import make_not_configured_handler - - routes.append( - Route("/", make_not_configured_handler(adapter_error), methods=["POST"]) - ) - - app = Starlette(routes=routes) + # Route assembly is in workspace/boot_routes.py so the contract — + # card always mounted, JSON-RPC route swaps based on adapter state + # (DefaultRequestHandler when executor is non-None, not_configured + # handler returning -32603 otherwise) — is unit-testable with + # Starlette's TestClient. main.py is `# pragma: no cover` so without + # this extraction a future refactor that re-coupled card + setup() + # would silently bypass PR #2756. tests/test_boot_routes.py pins + # the four-branch contract. + from boot_routes import build_routes + app = Starlette(routes=build_routes(agent_card, executor, adapter_error)) # 8. Register with platform # When adapter.setup() failed, advertise via configuration_status so diff --git a/workspace/tests/conftest.py b/workspace/tests/conftest.py index dfb81472..3bbb0eee 100644 --- a/workspace/tests/conftest.py +++ b/workspace/tests/conftest.py @@ -181,6 +181,77 @@ def _make_a2a_mocks(): types_mod.AgentInterface = AgentInterface types_mod.AgentCard = AgentCard + # a2a.server.routes — used by boot_routes.build_routes (PR #2756 chain + # / #2761) to mount /.well-known/agent-card.json. The real SDK builds + # a Starlette route that serializes the card on each request; the stub + # mirrors that behaviour with json.dumps over the card's __dict__ so + # TestClient.get("/.well-known/agent-card.json") returns the same + # shape canvas would see in production. + routes_mod = ModuleType("a2a.server.routes") + + def _create_agent_card_routes(card): + from starlette.responses import JSONResponse + from starlette.routing import Route + + async def _card_handler(_request): + # Convert the stub AgentCard into a JSON-serialisable dict. + # Real a2a.types.AgentCard is a Pydantic model with proper + # serialisation; the stub stores attrs raw, so we walk + # __dict__ and serialise nested AgentSkill objects too. + def _to_dict(obj): + if hasattr(obj, "__dict__"): + return {k: _to_dict(v) for k, v in vars(obj).items()} + if isinstance(obj, list): + return [_to_dict(x) for x in obj] + if isinstance(obj, dict): + return {k: _to_dict(v) for k, v in obj.items()} + return obj + + return JSONResponse(_to_dict(card)) + + return [Route("/.well-known/agent-card.json", _card_handler, methods=["GET"])] + + def _create_jsonrpc_routes(request_handler=None, rpc_url="/", **_kwargs): + from starlette.responses import JSONResponse + from starlette.routing import Route + + async def _jsonrpc_handler(_request): + # Stub: real DefaultRequestHandler dispatches to the executor; + # tests that need real behaviour will use a test-side mock. + # This stub just returns a JSON-RPC envelope so the not-configured + # branch's discriminator (`error.data` containing "setup() failed") + # has something to differ from. + return JSONResponse({"jsonrpc": "2.0", "result": "stub-jsonrpc-handler"}) + + return [Route(rpc_url, _jsonrpc_handler, methods=["POST"])] + + routes_mod.create_agent_card_routes = _create_agent_card_routes + routes_mod.create_jsonrpc_routes = _create_jsonrpc_routes + sys.modules["a2a.server.routes"] = routes_mod + + # a2a.server.request_handlers — used by boot_routes' executor branch. + # DefaultRequestHandler stub takes the same kwargs as the real one; + # tests that exercise the executor path don't poke at the handler's + # internals, only that it gets mounted at "/". + rh_mod = ModuleType("a2a.server.request_handlers") + + class DefaultRequestHandler: + def __init__(self, agent_executor=None, task_store=None, agent_card=None, **_kwargs): + self.agent_executor = agent_executor + self.task_store = task_store + self.agent_card = agent_card + + rh_mod.DefaultRequestHandler = DefaultRequestHandler + sys.modules["a2a.server.request_handlers"] = rh_mod + + # InMemoryTaskStore is exposed via a2a.server.tasks (already stubbed + # above with TaskUpdater). Add it as a no-op class. + class _InMemoryTaskStore: + def __init__(self): + pass + + tasks_mod.InMemoryTaskStore = _InMemoryTaskStore + # a2a.helpers (v1: moved from a2a.utils, renamed new_agent_text_message # → new_text_message). Mock both names — production code only calls # new_text_message, but if any test still references the old name it diff --git a/workspace/tests/test_boot_routes.py b/workspace/tests/test_boot_routes.py new file mode 100644 index 00000000..d38b4ca8 --- /dev/null +++ b/workspace/tests/test_boot_routes.py @@ -0,0 +1,213 @@ +"""Integration tests for boot_routes.build_routes — pin the contract that +PR #2756's card-vs-setup decoupling depends on. + +Why these matter (issue #2761): main.py is ``# pragma: no cover``. The +inline if/else that mounted ``DefaultRequestHandler`` vs the +not-configured handler had no pytest coverage; a future refactor that +re-coupled card and setup() would have shipped the original "stuck +booting forever" UX again. Extracting to ``boot_routes.build_routes`` ++ these tests make the contract regression-proof. + +Each test exercises a real Starlette TestClient against the routes — +no uvicorn, no socket, but every assertion is the same one canvas's +TranscriptHandler / a2a_proxy would make in production. +""" +from __future__ import annotations + +import sys +from pathlib import Path +from unittest.mock import MagicMock + +import pytest + +# Make workspace/ importable in test isolation — same pattern as the +# adjacent tests (test_not_configured_handler.py, test_card_helpers.py). +WORKSPACE_DIR = Path(__file__).resolve().parents[1] +if str(WORKSPACE_DIR) not in sys.path: + sys.path.insert(0, str(WORKSPACE_DIR)) + + +@pytest.fixture +def agent_card(): + """Build a minimal AgentCard the way main.py does at boot.""" + from a2a.types import ( + AgentCard, + AgentCapabilities, + AgentInterface, + AgentSkill, + ) + + return AgentCard( + name="test-agent", + description="test-agent", + version="0.0.0", + supported_interfaces=[ + AgentInterface(protocol_binding="https://a2a.g/v1", url="http://test:8000") + ], + capabilities=AgentCapabilities(streaming=True, push_notifications=False), + skills=[ + AgentSkill(id="echo", name="echo", description="echo", tags=[], examples=[]) + ], + default_input_modes=["text/plain"], + default_output_modes=["text/plain"], + ) + + +# ---- card route always mounted, regardless of adapter state ------------- + + +def test_card_route_serves_200_when_adapter_ready(agent_card): + """Adapter setup OK → card serves 200, the canonical happy path.""" + from starlette.applications import Starlette + from starlette.testclient import TestClient + + from boot_routes import build_routes + + fake_executor = MagicMock() + app = Starlette(routes=build_routes(agent_card, fake_executor, None)) + client = TestClient(app) + resp = client.get("/.well-known/agent-card.json") + assert resp.status_code == 200 + body = resp.json() + assert body["name"] == "test-agent" + + +def test_card_route_serves_200_when_adapter_failed(agent_card): + """Adapter setup raised → card route is STILL mounted with the same + static skills. This is the entire point of PR #2756: a misconfigured + workspace stays REACHABLE so canvas can show the user a clear error + instead of silently looking dead.""" + from starlette.applications import Starlette + from starlette.testclient import TestClient + + from boot_routes import build_routes + + app = Starlette( + routes=build_routes( + agent_card, executor=None, adapter_error="MISSING_API_KEY" + ) + ) + client = TestClient(app) + resp = client.get("/.well-known/agent-card.json") + assert resp.status_code == 200 + body = resp.json() + assert body["name"] == "test-agent" + # Skill stubs survive even though setup() didn't run. + assert any(s.get("id") == "echo" for s in body.get("skills", [])) + + +# ---- JSON-RPC route swaps based on executor presence ------------------- + + +def test_jsonrpc_returns_503_when_no_executor(agent_card): + """The not-configured branch: POST / returns 503 with JSON-RPC -32603 + and the adapter_error in error.data. This is what canvas sees when a + user tries to message a workspace whose setup() failed — turns a + "stuck silent" workspace into "agent not configured: ".""" + from starlette.applications import Starlette + from starlette.testclient import TestClient + + from boot_routes import build_routes + + app = Starlette( + routes=build_routes( + agent_card, + executor=None, + adapter_error="RuntimeError: Neither OPENAI_API_KEY nor MINIMAX_API_KEY is set", + ) + ) + client = TestClient(app) + resp = client.post( + "/", + json={"jsonrpc": "2.0", "id": 42, "method": "message/send"}, + ) + assert resp.status_code == 503 + body = resp.json() + assert body["jsonrpc"] == "2.0" + assert body["id"] == 42 # echoed + assert body["error"]["code"] == -32603 + assert "MINIMAX_API_KEY" in body["error"]["data"] + + +def test_jsonrpc_returns_503_with_generic_when_no_error_string(agent_card): + """Defensive: if main.py reached this branch without a captured + error string (shouldn't happen in practice but the helper is + defensive), the handler still returns -32603 with a generic + fallback so the operator gets a useful response shape.""" + from starlette.applications import Starlette + from starlette.testclient import TestClient + + from boot_routes import build_routes + + app = Starlette( + routes=build_routes(agent_card, executor=None, adapter_error=None) + ) + client = TestClient(app) + resp = client.post( + "/", json={"jsonrpc": "2.0", "id": 1, "method": "message/send"} + ) + assert resp.status_code == 503 + assert resp.json()["error"]["code"] == -32603 + # Falls back to generic "adapter.setup() failed". + assert "setup() failed" in resp.json()["error"]["data"] + + +# ---- Specific regression: re-coupling card to setup would break this --- + + +def test_card_route_does_not_depend_on_executor(agent_card): + """Direct regression test for PR #2756. If a future refactor moved + create_agent_card_routes into the executor-only branch, this test + would catch it: the card MUST be served from a code path that runs + even when executor is None.""" + from boot_routes import build_routes + + routes_with_executor = build_routes(agent_card, MagicMock(), None) + routes_without_executor = build_routes(agent_card, None, "err") + + # Both branches mount /.well-known/agent-card.json. Find by path. + def has_card_route(routes): + for r in routes: + for attr in ("path", "path_format"): + p = getattr(r, attr, None) + if p and "agent-card.json" in p: + return True + return False + + assert has_card_route(routes_with_executor), ( + "card route MUST be mounted on the executor-present path" + ) + assert has_card_route(routes_without_executor), ( + "card route MUST be mounted on the executor-missing path " + "(this is the PR #2756 contract — re-coupling here breaks tenant readiness)" + ) + + +def test_executor_present_does_not_mount_not_configured_handler(agent_card): + """Sanity: when executor is present, the not-configured handler + must NOT be mounted at /. Otherwise a healthy workspace would + return -32603 to every JSON-RPC call. + + We call POST / with a malformed JSON-RPC body and assert the + response is NOT the -32603 not-configured envelope. (The real + DefaultRequestHandler may return its own error for the malformed + payload, but it won't have ``data: "adapter.setup() failed"``.)""" + from starlette.applications import Starlette + from starlette.testclient import TestClient + + from boot_routes import build_routes + + fake_executor = MagicMock() + app = Starlette(routes=build_routes(agent_card, fake_executor, None)) + client = TestClient(app) + resp = client.post( + "/", json={"jsonrpc": "2.0", "id": 1, "method": "message/send"} + ) + body = resp.json() if resp.headers.get("content-type", "").startswith("application/json") else {} + # Whatever DefaultRequestHandler does, it isn't the not-configured + # envelope. The cheap discriminator: error.data won't say "setup() failed". + err = body.get("error") or {} + data = err.get("data") if isinstance(err, dict) else "" + assert "setup() failed" not in (data or ""), ( + "executor-present branch must not mount the not-configured handler" + )