Give every MCP tool result one universal outcome key (ok)

mcp-server/src/bnk_forge_mcp/client.py

@@ -166,14 +166,48 @@ 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()
         try:
             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)

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)

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

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"