⚠️ 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
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:680 — enableJsonResponse: true, why a tool failure is HTTP 200
src/mcp/server.ts:682 — response.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
Context
src/mcp/dispatch-telemetry.ts:70states the invariant the whole chokepoint is built on:The two views do not agree, because a failed MCP tool call is an HTTP 200.
src/mcp/server.ts:680builds the handler with
enableJsonResponse: true, and a tool failure is carried inside the JSON-RPCresult, not in the status line. The repo's own integration test proves it —
test/integration/api.test.ts:5049:That call is a
tools/callforloopover_get_decision_packwith someone else's login. The dispatchchokepoint records it as a failure —
src/mcp/dispatch-telemetry.ts:120-137catches the thrownauthorization error and emits
ok: falsewith a resolvederrorCode. But two lines later in the requesthandler,
src/mcp/server.ts:682:records
ok: truefor the very same call, andsrc/mcp/server.ts:691:records the product-usage event as a success.
So one refused tool call produces:
usage_event {ok: false, error_code: "forbidden"}, legacymcp_tool_call {ok: true}, andproduct usage {outcome: "success"}. Every operator dashboard readingthe legacy event — which
packages/loopover-mcp/lib/telemetry.ts:32says 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
recordProductUsageEventhave the same blind spot.
Requirements
handleMcpRequestmust derive the per-toolok/outcomefrom the JSON-RPC response body for atools/call, not fromresponse.status. Atools/callwhose result carriesisError: true, or whoseenvelope carries a top-level
error, isok: false/outcome: "error". A non-tools/callrequest(
tools/list,initialize, …) keeps the current status-derived outcome.parse failure fall back to the current
response.status < 400behaviour rather than throwing —telemetry must never turn a working call into a failed one, the guarantee
src/mcp/dispatch-telemetry.ts:18already states.src/mcp/dispatch-telemetry.ts:70-75must be updated to describe what the code nowdoes; it must not keep asserting an agreement that only holds after this change.
legacy
mcp_tool_callevent (LEGACY_MCP_TELEMETRY_PROPERTY_KEYS, four keys, built throughbuildLegacyToolCallProperties), and the dispatch chokepoint's ownokderivation, which is alreadycorrect.
latencyMsandmetadatafields on the product-usage event.Deliverables
handleMcpRequestinsrc/mcp/server.tsderives a per-requestokfrom the response body fortools/callrequests, and passes it to bothrecordMcpToolTelemetry(line 682) and therecordProductUsageEventoutcome(line 691).src/mcp/dispatch-telemetry.ts:70-75states the actual derivation.test/integration/api.test.tsnamed for this bug that posts the samewrong-login
loopover_get_decision_packtools/callas line 5045, asserts the response is stillHTTP 200 with
result.isError === true, and asserts the recorded tool telemetry / product-usageoutcome for that request is a failure — not a success.
test/unit/mcp-dispatch-telemetry.test.tscovering the fallback arm: atools/callresponse 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
recordMcpToolTelemetrybut leavingrecordProductUsageEvent'soutcomeon thestatus 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'scoverage.includecoverssrc/**/*.ts(line 78), so both touched files are measured and gated.Name every branch the change introduces and test both arms: the
tools/callvs non-tools/calldiscriminator, the
isError === truevs normal-result arms, and the parse-failure fallback. Theexisting
outcome: response.status >= 400 ? "error" : "success"ternary is being replaced — itsreplacement's arms both need a test, and the
catchpath atsrc/mcp/server.ts:698must keep itsexisting 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_callsuccess rate and the product-usage outcome stopreporting a clean sheet for a surface that is refusing calls.
Links & Resources
src/mcp/server.ts:680—enableJsonResponse: true, why a tool failure is HTTP 200src/mcp/server.ts:682—response.status < 400as the legacy event'soksrc/mcp/server.ts:691— the same derivation for the product-usageoutcomesrc/mcp/dispatch-telemetry.ts:70— the comment asserting the two views agreetest/integration/api.test.ts:5045— the existing test proving a refused tool call is HTTP 200