From 358911358f932008b215e2bd469775508e611685 Mon Sep 17 00:00:00 2001 From: Jimisola Laursen Date: Sat, 29 Aug 2026 20:55:34 +0200 Subject: [PATCH 1/2] fix(mcp): keep anticipated tool failures readable under mcp 2.1 The mcp 2.1 SDK narrowed which exceptions reach the client: only a ToolError keeps its message, and everything else is treated as a crash and reported as a bare "Error executing tool ". The server raised ValueError/RuntimeError for failures it fully anticipated, so all of them were masked -- an agent asking for a mistyped REQ_TYPO got "Error executing tool get_requirement" and no way to see the typo. Raise ToolError at the eight anticipated-failure sites instead, which also drops them from ERROR-with-traceback to INFO in the server log. Reverting only the source change fails eight tests, but main caught this with one: the others asserted is_error without asserting that the reason survives. They now assert the message too, so a future SDK bump cannot re-mask them silently. Refs reqstool/.github#111 Signed-off-by: Jimisola Laursen --- src/reqstool/mcp/server.py | 27 ++++++++++++------- .../reqstool/mcp/test_mcp_integration.py | 7 +++++ .../reqstool/mcp/test_server_freshness.py | 8 +++--- 3 files changed, 30 insertions(+), 12 deletions(-) diff --git a/src/reqstool/mcp/server.py b/src/reqstool/mcp/server.py index 42dd9f4..1c18c10 100644 --- a/src/reqstool/mcp/server.py +++ b/src/reqstool/mcp/server.py @@ -6,6 +6,7 @@ from reqstool_python_decorators.decorators.decorators import Requirements +from reqstool.common.exceptions import SnapshotReloadError from reqstool.common.project_session import ProjectSession from reqstool.common.enrichment.enricher import BUILT_IN_PRESETS, enrich_text from reqstool.common.queries.details import ( @@ -32,6 +33,11 @@ def start_server( # noqa: C901 ) -> None: try: from mcp.server.mcpserver import MCPServer + + # SDK 2.1: only a ToolError's message reaches the model. Any other exception is + # treated as a crash and reported as a bare "Error executing tool ", so + # every anticipated failure below raises ToolError to keep its text. + from mcp.server.mcpserver.exceptions import ToolError except ImportError as exc: raise ImportError("MCP server requires extra dependencies: pip install 'mcp>=2.0'") from exc @@ -51,10 +57,13 @@ def _repo() -> RequirementsRepository: Every tool must resolve the repository through this. Binding it once at startup is what let long-lived servers serve a snapshot from before the last build (#437). """ - session.ensure_fresh() + try: + session.ensure_fresh() + except SnapshotReloadError as exc: + raise ToolError(str(exc)) from exc repo = session.repo if repo is None: - raise RuntimeError(f"reqstool project is not loaded: {session.error}") + raise ToolError(f"reqstool project is not loaded: {session.error}") return repo @Requirements("MCP_0008") @@ -87,7 +96,7 @@ async def get_requirement(id: str) -> dict: """Get full details for a requirement by ID (e.g. REQ_010).""" result = get_requirement_details(id, _repo(), session.urn_source_paths) if result is None: - raise ValueError(f"Requirement {id!r} not found") + raise ToolError(f"Requirement {id!r} not found") return result @mcp.tool() @@ -109,7 +118,7 @@ async def get_svc(id: str) -> dict: """Get full details for an SVC by ID (e.g. SVC_010).""" result = get_svc_details(id, _repo(), session.urn_source_paths) if result is None: - raise ValueError(f"SVC {id!r} not found") + raise ToolError(f"SVC {id!r} not found") return result @mcp.tool() @@ -122,7 +131,7 @@ async def get_mvr(id: str) -> dict: """Get full details for an MVR by ID.""" result = get_mvr_details(id, _repo(), session.urn_source_paths) if result is None: - raise ValueError(f"MVR {id!r} not found") + raise ToolError(f"MVR {id!r} not found") return result @mcp.tool() @@ -144,7 +153,7 @@ async def refresh() -> dict: unconditionally — after a build, for instance — or to confirm what is being served.""" session.build() if not session.ready: - raise RuntimeError(f"Failed to reload reqstool project: {session.error}") + raise ToolError(f"Failed to reload reqstool project: {session.error}") return _snapshot_info() @mcp.tool() @@ -154,7 +163,7 @@ async def get_requirement_status(id: str, include_post_build: bool = False) -> d `status --with-post-tests` (scopes to post-build-phase SVCs too).""" result = _get_requirement_status(id, _repo(), include_post_build=include_post_build) if result is None: - raise ValueError(f"Requirement {id!r} not found") + raise ToolError(f"Requirement {id!r} not found") return result @mcp.tool() @@ -184,7 +193,7 @@ async def get_urn_details(urn: str) -> dict: """Get details for a URN: variant, title, location, file paths, and entity counts.""" result = _get_urn_details(urn, _repo(), session.urn_source_paths) if result is None: - raise ValueError(f"URN {urn!r} not found") + raise ToolError(f"URN {urn!r} not found") return result @mcp.tool() @@ -198,7 +207,7 @@ async def enrich_document(content: str, preset: str) -> str: openspec:proposal, openspec:tasks """ if preset not in BUILT_IN_PRESETS: - raise ValueError(f"Unknown preset {preset!r}. Valid: {sorted(BUILT_IN_PRESETS)}") + raise ToolError(f"Unknown preset {preset!r}. Valid: {sorted(BUILT_IN_PRESETS)}") config = BUILT_IN_PRESETS[preset] repo = _repo() return enrich_text(content, repo.get_all_requirements(), repo.get_all_svcs(), repo.get_all_mvrs(), config) diff --git a/tests/integration/reqstool/mcp/test_mcp_integration.py b/tests/integration/reqstool/mcp/test_mcp_integration.py index 4403529..5fccc3b 100644 --- a/tests/integration/reqstool/mcp/test_mcp_integration.py +++ b/tests/integration/reqstool/mcp/test_mcp_integration.py @@ -77,6 +77,9 @@ async def test_get_requirement_known(mcp_session): async def test_get_requirement_not_found(mcp_session): result = await mcp_session.call_tool("get_requirement", {"id": "REQ_NONEXISTENT"}) assert result.is_error + # The reason has to reach the model, not just the failure: an SDK that masks it + # leaves a bare "Error executing tool get_requirement" and no way to spot a typo. + assert "REQ_NONEXISTENT" in str(result.content) # --------------------------------------------------------------------------- @@ -114,6 +117,7 @@ async def test_get_svc_known(mcp_session): async def test_get_svc_not_found(mcp_session): result = await mcp_session.call_tool("get_svc", {"id": "SVC_NONEXISTENT"}) assert result.is_error + assert "SVC_NONEXISTENT" in str(result.content) # --------------------------------------------------------------------------- @@ -133,6 +137,7 @@ async def test_list_mvrs(mcp_session): async def test_get_mvr_not_found(mcp_session): result = await mcp_session.call_tool("get_mvr", {"id": "MVR_NONEXISTENT"}) assert result.is_error + assert "MVR_NONEXISTENT" in str(result.content) # --------------------------------------------------------------------------- @@ -167,6 +172,7 @@ async def test_get_requirement_status(mcp_session): async def test_get_requirement_status_not_found(mcp_session): result = await mcp_session.call_tool("get_requirement_status", {"id": "REQ_NONEXISTENT"}) assert result.is_error + assert "REQ_NONEXISTENT" in str(result.content) @pytest.mark.parametrize("include_post_build", [False, True]) @@ -176,6 +182,7 @@ async def test_get_requirement_status_not_found_with_include_post_build(mcp_sess "get_requirement_status", {"id": "REQ_NONEXISTENT", "include_post_build": include_post_build} ) assert result.is_error + assert "REQ_NONEXISTENT" in str(result.content) async def test_get_requirement_status_missing_automated_test_not_met(mcp_session): diff --git a/tests/unit/reqstool/mcp/test_server_freshness.py b/tests/unit/reqstool/mcp/test_server_freshness.py index 833d421..b811f29 100644 --- a/tests/unit/reqstool/mcp/test_server_freshness.py +++ b/tests/unit/reqstool/mcp/test_server_freshness.py @@ -12,9 +12,9 @@ import mcp.server.mcpserver import pytest +from mcp.server.mcpserver.exceptions import ToolError from reqstool_python_decorators.decorators.decorators import SVCs -from reqstool.common.exceptions import SnapshotReloadError from reqstool.locations.local_location import LocalLocation from reqstool.mcp import server as mcp_server @@ -111,9 +111,11 @@ def test_a_tool_errors_when_the_changed_project_cannot_be_reloaded(project_copy) async def scenario(tools): (project_copy / "requirements.yml").write_text(": this is not: [ valid yaml") - with pytest.raises(SnapshotReloadError, match="sources changed but reloading them failed"): + # ToolError, not the underlying SnapshotReloadError: only a ToolError's message + # survives the SDK, so asserting the type is what proves the reason reaches the model. + with pytest.raises(ToolError, match="sources changed but reloading them failed"): await tools["get_status"]() - with pytest.raises(SnapshotReloadError): + with pytest.raises(ToolError): await tools["list_requirements"]() return True From bd0322e62ad1eafdec259466f53d8173c7421af0 Mon Sep 17 00:00:00 2001 From: Jimisola Laursen Date: Sat, 29 Aug 2026 22:27:34 +0200 Subject: [PATCH 2/2] test(mcp): cover the two untested anticipated-failure paths get_urn_details and enrich_document raise ToolError like the rest, but neither had any error-path test, so the mcp 2.1 masking would have gone unnoticed there even with the rest of the suite strengthened. Both fail against the pre-fix server, so they bind the behaviour rather than describe it. refresh's reload-failure path is left uncovered -- provoking it needs a project that builds once and then cannot. Refs reqstool/.github#111 Signed-off-by: Jimisola Laursen --- .../reqstool/mcp/test_mcp_integration.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/tests/integration/reqstool/mcp/test_mcp_integration.py b/tests/integration/reqstool/mcp/test_mcp_integration.py index 5fccc3b..3e959ee 100644 --- a/tests/integration/reqstool/mcp/test_mcp_integration.py +++ b/tests/integration/reqstool/mcp/test_mcp_integration.py @@ -214,3 +214,22 @@ async def test_list_annotations(mcp_session): assert "req_urn" in ann assert "element_kind" in ann assert "fqn" in ann + + +# --------------------------------------------------------------------------- +# get_urn_details / enrich_document +# --------------------------------------------------------------------------- + + +async def test_get_urn_details_not_found(mcp_session): + result = await mcp_session.call_tool("get_urn_details", {"urn": "no-such-urn"}) + assert result.is_error + assert "no-such-urn" in str(result.content) + + +async def test_enrich_document_unknown_preset(mcp_session): + """The message has to name the valid presets, which is the only way to recover from this.""" + result = await mcp_session.call_tool("enrich_document", {"content": "REQ_PASS", "preset": "no:such:preset"}) + assert result.is_error + assert "no:such:preset" in str(result.content) + assert "openspec:spec" in str(result.content)