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

[jgruberf5]

What & why

Fixes #66 — MCP result envelopes signalled outcome three different ways, so an agent had no single field to check across tools:

create_project  → {"success": true,  "project": {...}, "message": "..."}
delete_project  → {"success": true,  "message": "...", "project_id": 88}
get_project 404 → {"ok": false, "request": {...}, "error": {...}}

Success used success: true (or nothing); failure used ok: false; shapes differed per tool. That forces brittle multi-shape parsing in every agent and undercuts the outcome-derived-success contract (D-017) the agent path relies on. It's the success-side complement to #67 (which fixed the error detail shape).

The fix — one choke point, non-breaking

All 151 tools flow through client.get/post/put/patch/delete → _request_with_error_envelope, which already stamps ok: false on errors via _error_payload. This adds the symmetric half: _mark_ok stamps ok: true on every dict success body at that same point. One ~5-line helper gives all 151 tools the same outcome key without rewriting a single tool's return.

Deliberately additive:

• Existing keys (success, project, message, …) are untouched — nothing that parses them today breaks.
• A body that already set ok (a structured passthrough) 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 — "ok is not False" — is 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 (it's meta, not project data).

Testing

New client tests: dict success gets ok: true; a backend-set ok isn't overridden; lists pass through as-is; success and error now share the ok key. The five passthrough client tests are updated to the new shape, and create_project's contract test pins ok present on the envelope and absent from project. Full MCP suite: 408 passed.

Non-vacuous: test_success_dict_gets_universal_ok_true KeyErrors against the unpatched client (raw body has no ok).

Fixes #66

https://claude.ai/code/session_01UpRYiFserdBE5ESHn759N4

[jgruberf5]

Self-review

Non-breaking is the whole design, so I pressure-tested it

The temptation with #66 is to replace every envelope with {ok, data}. I deliberately didn't — live agents (the Atlas persona that filed this) already parse success/project/project_id, and a wrapping change breaks all of them at once. Stamping ok alongside the existing keys satisfies "one field an agent can check" without a flag day. The five updated client tests are the proof that the only observable change is an added ok key.

The choke point interacting with a re-shaping tool — caught it

create_project doesn't passthrough; it rebuilds {success, project} and copies leftover keys into project. So once the client stamps ok: true on the raw body, that ok would have been swept into project_entity — leaking a meta field into the entity. I added ok to its skip_keys and surfaced it on the envelope instead, and pinned both halves in the contract test (ok present on envelope, absent from project). This is the one place the general mechanism needed a tool-specific adjustment; I grepped for other custom-envelope tools and create_project is the only reshaper — every other tool is a raw return json.dumps(result) passthrough that the choke point covers for free.

The honest limit — lists

A list-returning tool (e.g. list_clusters) still returns a bare JSON array with no ok, because you can't stamp a key on a list. I chose not to wrap lists as {ok, data: [...]} because that would break every list consumer — the exact thing this PR avoids. So the contract an agent should follow is "success == ok is not False", not "success == ok is True". I documented that on the helper and pinned it in test_list_success_returned_as_is. It's a real seam; wrapping lists would be a deliberate breaking follow-up if the team wants strict {ok, data} everywhere.

Verified, not assumed

Full suite 408 passed with the backend tree mounted (the test_url_audit suite cross-references backend routes and fails spuriously without it — not related to this change). The new positive test KeyErrors against the unpatched client, so it bites.

Scope I did not take

Normalizing the success payload shape (data vs project vs releases …) across tools — the issue's "consistent data/error payload" half. That's a much larger, genuinely breaking change touching every tool's return and every agent that reads them. This PR delivers the single-outcome-key half, which is what makes success/failure branchable; the payload-shape unification is worth its own tracked decision rather than smuggling it in here.

[mwiget]

One choke point instead of 151 edits is the right shape for this, and the additive rules (don't override an existing ok, leave success/message alone, keep ok out of the nested project entity) are all sound. But the stamp is unconditional, and that turns a class of existing responses into confident lies.

Blocking — ok: true gets stamped on bodies that say success: false.

_request only raises APIError on a non-2xx status, so _mark_ok runs for every HTTP 200. Several backend routes return 200 with an explicit failure body. backend/routes/helm.py:134, on GET /api/k8s/{cluster_id}/helm/releases — a path the MCP helm tools call:

if cel.failed():
    return {"success": False, "releases": [], "count": 0, "task_id": cel.id, "status": "failed"}

After this change the agent receives:

{"success": false, "releases": [], "count": 0, "status": "failed", "ok": true}

The docstring's rule — "ok present and not False == success" — reads that as a success. It is a Celery task that failed.

This is not a one-off. "success": False on a 200 appears 12 times in helm.py alone (list, detail, history, values, manifest — both the failed and pending branches), plus alert_channels.py:241, cloud_auth.py:306, module_library.py, and operator_polling.py.

Worth being precise about why this is worse than the status quo rather than merely incomplete. Today an agent facing {"success": false, ...} has three inconsistent conventions and has to look at the body. After this change there is one universal, documented, authoritative-looking key — and on these routes it contradicts the body it is attached to. #66 is about giving agents a field they can trust; a field that is wrong on a real path is more dangerous than no field, because it removes the reason to look further.

The fix fits your existing design instead of fighting it — defer to an explicit success the same way you already defer to an explicit ok:

result.setdefault("ok", bool(result.get("success", True)))

Bodies with no success key still get ok: true (the common case, unchanged). Bodies that already set ok are still not overridden. And the 200-with-success: false responses get ok: false, which is what an agent needs. The status: "pending" branches land on ok: false too, which reads correctly — the operation has not succeeded.

Worth a test pinning it, since nothing currently covers a 200 that carries success: false.

---

Second, procedural but consequential: this PR contains #172.

origin/staging.. on this branch is three commits, not one:

718bf98 fix: give every MCP tool result one universal outcome key (ok) (#66)
5b36209 fix: display the analyzer's computed weight in both BNK backend views (#8)
bf79783 fix: surface the analyzer's computed L4Route weights, not the declared spec weight

The base is staging, so merging this merges #172's two commits as well — including the analyzer-weight semantics I asked about an hour ago, which are still open. The changed-files list gives it away: topology.py, helpers.py, BackendsCollection.tsx, F5BNKTopologyViewer.tsx and their tests are #172's, not this PR's.

Either retarget this PR's base to #172's branch so the diff shows only the MCP change and the merge order is enforced, or rebase onto staging once #172 lands. As it stands a merge here would quietly bypass the review on #172.

---

The rest reads well. _mark_ok is correctly placed at the one point every verb flows through; setdefault gives you the non-override property for free; the list/scalar reasoning is right and worth having written down on the helper; and updating create_project's skip_keys so ok stays on the envelope rather than leaking into the project entity is exactly the sort of detail that gets missed. The non-vacuity check on test_success_dict_gets_universal_ok_true is the right kind of evidence.

[mwiget]

Unconditional here is the problem. _request only raises on non-2xx, so this also runs for 200 responses that carry an explicit failure body — helm.py:134 returns {"success": False, "status": "failed", ...} with a 200, and the MCP helm tools call that path. The agent then sees success: false and ok: true side by side, and the documented rule tells it to believe ok.

Deferring to an explicit success mirrors the deference you already give an explicit ok:

result.setdefault("ok", bool(result.get("success", True)))

Bodies without success are unaffected. There are 12 such returns in helm.py plus ones in alert_channels.py, cloud_auth.py, module_library.py and operator_polling.py.

[mwiget]

This rule is what makes the bug above dangerous rather than merely incomplete. Once it is documented that ok not being False means success, an agent has no reason to also read success — so on the 200-with-success: false routes it will confidently act on a failure. Worth restating here once the stamp defers to success.

[jgruberf5]

Both fixed. Thank you — the success: false on 200 is a genuine correctness bug I'd have shipped, and you're right that a wrong universal key is worse than none.

1. ok now derives from success. Exactly your one-liner:

result.setdefault("ok", bool(result.get("success", True)))

So a 200 carrying {"success": false, ...} — the failed/pending Celery branches in helm.py (all 12), alert_channels, cloud_auth, etc. — lands on ok: false, matching the body. Bodies with no success key still get ok: true (the common case), and a body that already set ok is still not overridden. I verified the count independently: 12 "success": False returns in helm.py alone, all on 200. Added three tests pinning it: success:false → ok:false, status:pending → ok:false, success:true → ok:true — the last so the common mutating-tool body is provably unchanged. Full suite now 411.

You framed why this matters better than my original design did — "it removes the agent's reason to look further" — so I put that reasoning on the helper docstring, where the next person deciding whether to touch this will read it.

2. The branch contained #172 — fixed by rebase. You were exactly right, and it was worse than a stray diff: fix/66 was cut from the fix/8 branch, so it carried the original guessed-effectiveWeight commits — the very code you'd just asked me to back out. Merging here would have resurrected them behind #172's review. I rebased --onto staging so the branch is now a single commit touching only mcp-server/. The PR's file list and commit range confirm it: one commit, four mcp files. #172 stays its own PR (now draft, pending the cluster sample).

[mwiget]

Approving. Both items are handled, and the derivation is the right shape.

result.setdefault("ok", bool(result.get("success", True))) keeps every property the original had — additive, non-overriding, one choke point — while making ok agree with the body it's attached to. The three new tests cover the cases that matter and cover them at the boundary that broke: success: false on a 200 derives ok: false and leaves the original success untouched; a pending task reads as not-ok; an explicit success: true still stamps ok: true, so the common mutating-tool body is unchanged. Pinning result["success"] is False alongside result["ok"] is False in the first test is the detail I'd have asked for — it proves the derivation reads the key rather than rewriting it.

The docstring now carries the reasoning rather than just the rule, including why an unconditional stamp would have been worse than no key at all. That's the part worth having in the file: the next person to touch _mark_ok will see why the success lookup isn't incidental.

The rebase is clean. origin/staging.. is a single commit now, and the changed-file list is four MCP files — #172's topology and frontend work is gone from this branch entirely. Doing it now rather than waiting for #172 to land is the better call: the two PRs are independent again and can merge in either order.

I checked the MCP suite actually ran rather than trusting the rollup — P1 · MCP Server Tests (advisory) is green with 411 passed, up from the 408 you reported, which accounts for exactly the three tests added here. Worth noting that job is advisory, so a regression in it wouldn't have blocked the gate; since this PR is entirely MCP, that's the number that mattered and I wanted to see it directly.

create_project's envelope stays consistent under the new derivation: it computes success the same way _mark_ok now does, so ok and success on the envelope can't disagree, and ok still stays out of the nested project entity.

CI fully green including CI Gate.