From 8c8752ccbd341c66377b01c738afe37d477f71d2 Mon Sep 17 00:00:00 2001 From: John Gruber Date: Wed, 19 Aug 2026 15:05:08 -0500 Subject: [PATCH] fix: give every MCP tool result one universal outcome key (ok) (#66) MCP result envelopes signalled outcome inconsistently: success bodies used `success: true` (or nothing), the error path used `ok: false`, and mutating tools returned flat shapes like `{message, project_id}`. An agent had no single field to branch on across the 151 tools -- it had to special-case per shape, undercutting the outcome-derived-success contract (D-017) the agent path depends on. Every tool flows through one client choke point (get/post/put/patch/delete -> _request_with_error_envelope), which already stamps `ok: false` on errors. This adds the symmetric half: _mark_ok stamps `ok` on every dict success body at that same point, so all 151 tools gain the same outcome key without touching each tool's return. `ok` is derived, not blindly true: `setdefault("ok", bool(result.get("success", True)))`. That matters because `_request` only raises on non-2xx, so several routes that return HTTP 200 with an explicit `{"success": false, ...}` (a Celery task that failed or is pending -- helm list/detail/history/values/manifest, alert_channels, cloud_auth, ...) reach here. An unconditional `ok: true` would attach an authoritative-looking key that contradicts the body -- worse than no key, since it removes the agent's reason to look further. Deferring to an explicit `success` (the same way we defer to an existing `ok`) lands those on `ok: false`, which is the truth; bodies with no `success` key still get `ok: true`. Additive and non-breaking: - Existing keys (success, project, message, ...) are left in place. - A body that already set `ok` is not overridden. - List/scalar successes can't carry a key; they're unambiguously not the error envelope, so success there is the absence of `ok: false`. The robust agent rule is "ok is not False", documented on the helper. create_project builds its own {success, project} envelope, so it's updated to surface `ok` on the envelope and keep it out of the nested project entity. Tests: ok derives from success on a 200 (success:false -> ok:false, pending -> ok:false, success:true -> ok:true); dict success without a success key gets ok:true; a backend ok is not overridden; lists pass through; success and error share the ok key. The five client passthrough tests updated to the new shape; create_project's contract test pins ok on the envelope and absent from project. Full mcp suite 411 passed. Fixes #66 Claude-Session: https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4 --- mcp-server/src/bnk_forge_mcp/client.py | 36 +++++- .../src/bnk_forge_mcp/tools/iac_operations.py | 7 +- mcp-server/tests/test_client.py | 107 +++++++++++++++++- .../tests/test_tool_output_contracts.py | 4 + 4 files changed, 146 insertions(+), 8 deletions(-) diff --git a/mcp-server/src/bnk_forge_mcp/client.py b/mcp-server/src/bnk_forge_mcp/client.py index 3b2c975..3272aca 100644 --- a/mcp-server/src/bnk_forge_mcp/client.py +++ b/mcp-server/src/bnk_forge_mcp/client.py @@ -166,6 +166,40 @@ def _log_client_event( status_code, ) + @staticmethod + def _mark_ok(result: Any) -> Any: + """Stamp a single, universal outcome key on every success body. + + The error path returns ``{"ok": False, ...}`` (``_error_payload``), but + success bodies came straight from the backend in whatever shape it chose + -- some carried ``success: true``, some nothing, and mutating tools + returned flat ``{message, project_id}`` (#66). An agent had no one field + to check across tools. Stamping ``ok: True`` here -- the single choke + point every get/post/put/patch/delete flows through -- gives all 151 + tools the same outcome key an agent can branch on (``ok`` present and not + False == success), without rewriting each tool's return. + + Additive and non-breaking: existing keys are left untouched, and a body + that already set ``ok`` (a structured passthrough) is not overridden. + Non-dict bodies (list/scalar collections) can't carry a key; they are + unambiguously not the error envelope, so success is signalled by the + absence of ``ok: False`` rather than the presence of ``ok: True``. + + Crucially, ``ok`` is derived from an explicit ``success`` when present. + Several backend routes return HTTP 200 with ``{"success": false, ...}`` + -- a Celery task that failed or is still pending -- and ``_request`` + only raises on non-2xx, so those bodies reach here. Stamping an + unconditional ``ok: True`` would attach an authoritative-looking key + that contradicts the body it's on, which is worse than no key: it + removes the agent's reason to look further. Deferring to ``success`` + (the same way we defer to an existing ``ok``) makes those land on + ``ok: False``, which is the truth. Bodies with no ``success`` key -- the + common case -- still get ``ok: True``. + """ + if isinstance(result, dict): + result.setdefault("ok", bool(result.get("success", True))) + return result + async def _request_with_error_envelope(self, method: str, path: str, **kwargs: Any) -> Any: """Return API result or a structured MCP-friendly error envelope.""" start = time.perf_counter() @@ -173,7 +207,7 @@ async def _request_with_error_envelope(self, method: str, path: str, **kwargs: A result = await self._request(method, path, **kwargs) duration_ms = int((time.perf_counter() - start) * 1000) self._log_client_event(method=method, path=path, duration_ms=duration_ms, success=True) - return result + return self._mark_ok(result) except APIError as err: logger.warning("API request failed: %s %s -> %s", method, path, err) duration_ms = int((time.perf_counter() - start) * 1000) diff --git a/mcp-server/src/bnk_forge_mcp/tools/iac_operations.py b/mcp-server/src/bnk_forge_mcp/tools/iac_operations.py index 5225b6b..9d692a3 100644 --- a/mcp-server/src/bnk_forge_mcp/tools/iac_operations.py +++ b/mcp-server/src/bnk_forge_mcp/tools/iac_operations.py @@ -190,14 +190,17 @@ async def create_project( success = bool(result.get("success", True)) message = result.get("message") # Map flat project_id → project.id; collect remaining non-meta keys. - skip_keys = {"success", "message"} + # "ok" is the universal outcome key the client stamps on every success + # body (#66); it's meta, not project data, so keep it out of the entity + # and surface it on the envelope instead. + skip_keys = {"ok", "success", "message"} if "project_id" in result: project_entity["id"] = result["project_id"] skip_keys.add("project_id") for k, v in result.items(): if k not in skip_keys: project_entity[k] = v - envelope: dict = {"success": success, "project": project_entity} + envelope: dict = {"ok": success, "success": success, "project": project_entity} if message is not None: envelope["message"] = message return json.dumps(envelope, indent=2) diff --git a/mcp-server/tests/test_client.py b/mcp-server/tests/test_client.py index d790728..c0f23af 100644 --- a/mcp-server/tests/test_client.py +++ b/mcp-server/tests/test_client.py @@ -63,7 +63,8 @@ async def test_get_success(client: BNKForgeClient) -> None: result = await client.get("/api/system/health") - assert result == {"status": "healthy"} + # _mark_ok stamps the universal outcome key on every dict success body (#66). + assert result == {"status": "healthy", "ok": True} assert route.called @@ -106,7 +107,7 @@ async def test_post_success(client: BNKForgeClient) -> None: result = await client.post("/api/clusters/1/test", json={"timeout": 10}) - assert result == {"connected": True} + assert result == {"connected": True, "ok": True} assert route.called @@ -119,7 +120,7 @@ async def test_post_204_no_content(client: BNKForgeClient) -> None: result = await client.post("/api/something") - assert result == {"status": "ok"} + assert result == {"status": "ok", "ok": True} # ------------------------------------------------------------------ @@ -245,7 +246,7 @@ async def test_put_supports_query_params(client: BNKForgeClient) -> None: params={"namespace": "kube-system"}, ) - assert result == {"success": True} + assert result == {"success": True, "ok": True} assert route.calls[0].request.url.params["namespace"] == "kube-system" @@ -261,7 +262,7 @@ async def test_delete_supports_query_params(client: BNKForgeClient) -> None: params={"namespace": "default", "keep_history": "false"}, ) - assert result == {"success": True} + assert result == {"success": True, "ok": True} assert route.calls[0].request.url.params["namespace"] == "default" @@ -296,3 +297,99 @@ async def test_client_logs_structured_failure_without_payloads( assert "error_class=auth_error" in caplog.text assert "super-secret" not in caplog.text assert "dont-log-me" not in caplog.text + + +# ------------------------------------------------------------------ +# #66 — single universal outcome key across all tools +# ------------------------------------------------------------------ + + +@respx.mock +async def test_success_dict_gets_universal_ok_true(client: BNKForgeClient) -> None: + """Every dict success body carries ok:true, so an agent has one field to + check regardless of which tool it called (#66).""" + respx.get("http://test-backend:8000/api/projects/1").mock( + return_value=Response(200, json={"project_id": 1, "name": "p"}) + ) + result = await client.get("/api/projects/1") + assert result["ok"] is True + + +@respx.mock +async def test_mark_ok_does_not_override_backend_ok(client: BNKForgeClient) -> None: + """A body that already set ok (a structured passthrough) is left alone.""" + respx.get("http://test-backend:8000/api/thing").mock( + return_value=Response(200, json={"ok": False, "note": "backend said so"}) + ) + result = await client.get("/api/thing") + assert result["ok"] is False + + +@respx.mock +async def test_list_success_returned_as_is(client: BNKForgeClient) -> None: + """List/collection successes can't carry a key; they're unambiguously not + the error envelope, so success is the absence of ok:false, not a stamped + ok:true.""" + respx.get("http://test-backend:8000/api/clusters").mock( + return_value=Response(200, json=[{"id": 1}, {"id": 2}]) + ) + result = await client.get("/api/clusters") + assert result == [{"id": 1}, {"id": 2}] + + +@respx.mock +async def test_error_and_success_share_the_ok_key(client: BNKForgeClient) -> None: + """The whole point of #66: the same key an agent reads on failure (ok:false) + is present on success (ok:true) — no more success/ok split.""" + respx.get("http://test-backend:8000/api/ok").mock( + return_value=Response(200, json={"data": 1}) + ) + respx.get("http://test-backend:8000/api/bad").mock( + return_value=Response(404, json={"detail": "nope"}) + ) + ok_result = await client.get("/api/ok") + err_result = await client.get("/api/bad") + assert ok_result["ok"] is True + assert err_result["ok"] is False + # An agent can branch on exactly one field for both. + assert "ok" in ok_result and "ok" in err_result + + +@respx.mock +async def test_success_false_on_200_derives_ok_false(client: BNKForgeClient) -> None: + """Several routes return HTTP 200 with an explicit failure body (a Celery + task that failed or is pending — helm.py list/detail/history/values/manifest, + alert_channels, cloud_auth, ...). ok must derive from success, or the agent + reads an authoritative ok:true stamped on a body that says success:false. + """ + respx.get("http://test-backend:8000/api/k8s/1/helm/releases").mock( + return_value=Response(200, json={ + "success": False, "releases": [], "count": 0, + "task_id": "abc", "status": "failed", + }) + ) + result = await client.get("/api/k8s/1/helm/releases") + assert result["ok"] is False + assert result["success"] is False # original body untouched + + +@respx.mock +async def test_pending_task_on_200_reads_as_not_ok(client: BNKForgeClient) -> None: + """A still-pending task (success:false, status:pending on 200) is not a + success — ok:false reads correctly.""" + respx.get("http://test-backend:8000/api/k8s/1/helm/releases/r/values").mock( + return_value=Response(200, json={"success": False, "values": {}, "status": "pending"}) + ) + result = await client.get("/api/k8s/1/helm/releases/r/values") + assert result["ok"] is False + + +@respx.mock +async def test_success_true_on_200_still_ok_true(client: BNKForgeClient) -> None: + """An explicit success:true still stamps ok:true — the common mutating-tool + body is unchanged.""" + respx.post("http://test-backend:8000/api/k8s/1/helm/releases").mock( + return_value=Response(200, json={"success": True, "release": {"name": "r"}}) + ) + result = await client.post("/api/k8s/1/helm/releases", json={}) + assert result["ok"] is True diff --git a/mcp-server/tests/test_tool_output_contracts.py b/mcp-server/tests/test_tool_output_contracts.py index d51d6f2..caf86f4 100644 --- a/mcp-server/tests/test_tool_output_contracts.py +++ b/mcp-server/tests/test_tool_output_contracts.py @@ -359,6 +359,10 @@ async def test_create_project_returns_normalized_envelope_with_nested_project() ) assert parsed["success"] is True + # #66: the mutating-tool envelope exposes the same universal ok key, and it + # is meta -- it must not leak into the nested project entity. + assert parsed["ok"] is True + assert "ok" not in parsed["project"] assert "project" in parsed assert parsed["project"]["id"] == 39 assert parsed["project"]["name"] == "my-project"