Skip to content

mcp(telemetry): derive the remote request-level ok from the tool result, not the HTTP status #10035

Description

@JSONbored

⚠️ Definition of Done: this issue must be completed in full, in a single PR. Do not split this
work across multiple PRs, and do not defer any Deliverable below to a follow-up issue. A PR that
satisfies only some of the Deliverables, stubs a required test, or leaves a checkbox
partially-done does NOT resolve this issue and will be closed.

Context

src/mcp/dispatch-telemetry.ts:70 states the invariant the whole chokepoint is built on:

 * `ok` follows the CALLER-VISIBLE outcome: a handler that reports failure by returning an error
 * envelope did not succeed, even though it never threw. That matches what the HTTP-level telemetry
 * has always recorded (`response.status < 400`) so the two views of the same call agree.

The two views do not agree, because a failed MCP tool call is an HTTP 200. src/mcp/server.ts:680
builds the handler with enableJsonResponse: true, and a tool failure is carried inside the JSON-RPC
result, not in the status line. The repo's own integration test proves it —
test/integration/api.test.ts:5049:

expect(forbiddenContributorMcp.status).toBe(200);
await expect(mcpJson(forbiddenContributorMcp)).resolves.toMatchObject({
  result: { isError: true, content: [expect.objectContaining({ text: expect.stringContaining("authenticated GitHub login") })] },
});

That call is a tools/call for loopover_get_decision_pack with someone else's login. The dispatch
chokepoint records it as a failure — src/mcp/dispatch-telemetry.ts:120-137 catches the thrown
authorization error and emits ok: false with a resolved errorCode. But two lines later in the request
handler, src/mcp/server.ts:682:

executionCtx.waitUntil(recordMcpToolTelemetry(c.env, usageMetadata.toolName, response.status < 400, Date.now() - startedAt));

records ok: true for the very same call, and src/mcp/server.ts:691:

outcome: response.status >= 400 ? "error" : "success",

records the product-usage event as a success.

So one refused tool call produces: usage_event {ok: false, error_code: "forbidden"}, legacy
mcp_tool_call {ok: true}, and product usage {outcome: "success"}. Every operator dashboard reading
the legacy event — which packages/loopover-mcp/lib/telemetry.ts:32 says is kept precisely because
"an operator's existing dashboards read it" — reports a 100% success rate for the remote server no matter
how many tool calls are refused. The self-host adoption/health views built on recordProductUsageEvent
have the same blind spot.

Requirements

  • handleMcpRequest must derive the per-tool ok/outcome from the JSON-RPC response body for a
    tools/call, not from response.status. A tools/call whose result carries isError: true, or whose
    envelope carries a top-level error, is ok: false / outcome: "error". A non-tools/call request
    (tools/list, initialize, …) keeps the current status-derived outcome.
  • The body inspection must not consume the response the caller receives. Clone or buffer, and on any
    parse failure fall back to the current response.status < 400 behaviour rather than throwing —
    telemetry must never turn a working call into a failed one, the guarantee
    src/mcp/dispatch-telemetry.ts:18 already states.
  • The comment at src/mcp/dispatch-telemetry.ts:70-75 must be updated to describe what the code now
    does; it must not keep asserting an agreement that only holds after this change.
  • What must NOT change: the HTTP status the caller sees (a refused tool call stays 200), the shape of the
    legacy mcp_tool_call event (LEGACY_MCP_TELEMETRY_PROPERTY_KEYS, four keys, built through
    buildLegacyToolCallProperties), and the dispatch chokepoint's own ok derivation, which is already
    correct.
  • What must NOT change: the latencyMs and metadata fields on the product-usage event.

⚠️ Required pattern: reuse describeMcpUsageRequest (src/mcp/server.ts:733), which already clones the
REQUEST and reads params.name out of it — the response side must be read the same way, with the same
.catch(() => null) posture. What does NOT satisfy this issue: (a) making a failed tool call return a
4xx/5xx HTTP status, which breaks every MCP client and contradicts
test/integration/api.test.ts:5049; (b) deleting the legacy mcp_tool_call event instead of fixing its
ok; (c) editing only the comment at src/mcp/dispatch-telemetry.ts:70 to admit the disagreement while
leaving both events wrong.

Deliverables

  • handleMcpRequest in src/mcp/server.ts derives a per-request ok from the response body for
    tools/call requests, and passes it to both recordMcpToolTelemetry (line 682) and the
    recordProductUsageEvent outcome (line 691).
  • The comment block at src/mcp/dispatch-telemetry.ts:70-75 states the actual derivation.
  • A regression test at test/integration/api.test.ts named for this bug that posts the same
    wrong-login loopover_get_decision_pack tools/call as line 5045, asserts the response is still
    HTTP 200 with result.isError === true, and asserts the recorded tool telemetry / product-usage
    outcome for that request is a failure — not a success.
  • A unit test in test/unit/mcp-dispatch-telemetry.test.ts covering the fallback arm: a tools/call
    response whose body does not parse as JSON still records an outcome (the status-derived one) rather
    than throwing.

All Deliverables above are required in a single PR. A PR that satisfies only some of them — for
example fixing recordMcpToolTelemetry but leaving recordProductUsageEvent's outcome on the
status check, or fixing both without the non-JSON fallback test — does not resolve this issue.

Test Coverage Requirements

This repo enforces 99%+ Codecov patch coverage, branch-counted. vitest.config.ts's
coverage.include covers src/**/*.ts (line 78), so both touched files are measured and gated.
Name every branch the change introduces and test both arms: the tools/call vs non-tools/call
discriminator, the isError === true vs normal-result arms, and the parse-failure fallback. The
existing outcome: response.status >= 400 ? "error" : "success" ternary is being replaced — its
replacement's arms both need a test, and the catch path at src/mcp/server.ts:698 must keep its
existing coverage.

Expected Outcome

A refused or failing MCP tool call is recorded as a failure by all three of the remote server's telemetry
views instead of one, so the legacy mcp_tool_call success rate and the product-usage outcome stop
reporting a clean sheet for a surface that is refusing calls.

Links & Resources

  • src/mcp/server.ts:680enableJsonResponse: true, why a tool failure is HTTP 200
  • src/mcp/server.ts:682response.status < 400 as the legacy event's ok
  • src/mcp/server.ts:691 — the same derivation for the product-usage outcome
  • src/mcp/dispatch-telemetry.ts:70 — the comment asserting the two views agree
  • test/integration/api.test.ts:5045 — the existing test proving a refused tool call is HTTP 200

Metadata

Metadata

Assignees

No one assigned

    Labels

    gittensor:bugGittensor-scored bug fix — scores a 0.05x multiplier.help wantedExtra attention is needed

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions