Skip to content

mcp(telemetry): one envelope, one code, and no registration outside the chokepoint (#9659) - #9815

Merged
JSONbored merged 1 commit into
mainfrom
mcp/error-code-telemetry-9659
Jul 29, 2026
Merged

mcp(telemetry): one envelope, one code, and no registration outside the chokepoint (#9659)#9815
JSONbored merged 1 commit into
mainfrom
mcp/error-code-telemetry-9659

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Closes #9659. Also closes #9658 — the miner's chokepoint is where both land.

Stacked on #9812 (mcp/advertise-contract-metadata-9655), which introduces registerMinerTool. Review the second commit; this rebases onto main once #9812 merges.

The defect

@loopover/contract defined both halves of an error-envelope contract and joined neither:

  • toolErrorFields"code is drawn from a closed, developer-defined set rather than free text so telemetry can break failures down by cause". Zero consumers. No output schema spread it. And it typed that code as z.string().min(1) — free text, which is what the doc says it is not.
  • resolveErrorCode"Map a thrown error or an error envelope onto the closed code set". Its first branch reads envelope.code. No caller ever passed it an envelope.

So the code dimension was dead on the one failure path that does not throw — which is the path every one of these servers actually uses:

Server What it recorded
Remote errorCode: "unknown_error" as const — a literal, never the classifier
Stdio no error passed at all, so resolveErrorCode(undefined)unknown_error
Miner built a typed envelope for the caller on one line, threw it away on the next and passed the raw error

The miner's is the worst because it is actively contradictory: an ENOENT: no such file message matches /not found\|no such/, so a store that would not open told the caller store_unavailable while telemetry recorded not_found. Two classifications of one failure, produced by adjacent lines. There is a test for exactly that now.

What changed

  • store_unavailable joins MCP_TELEMETRY_ERROR_CODES — the miner already returned it to callers; adding it is what lets one code serve both the envelope and the event.
  • toolErrorFields.error.code is that closed set (z.enum), as its own doc always claimed.
  • All 16 miner output schemas spread toolErrorFields, so the field a caller must read to know why a call failed finally appears in an artifact.
  • All three chokepoints classify from the result's own envelope through the single classifier (toolErrorEnveloperesolveErrorCode). A code outside the closed set still falls back to unknown_error, so a tool cannot widen the dimension by inventing one.
  • The miner keeps passing the raw thrown error too — the envelope has no stack, and the PostHog exception capture needs one.

Two things this uncovered, fixed rather than left

ChatActionDispatchResult.status was string. That is what made the refusal-to-code mapping unverifiable in the first place. It is now a discriminated union of the five outcomes dispatchChatAction can actually return, and the mapping is a Record over it — so adding a dispatch status without deciding what it means to a caller fails the build. A test fixture was inventing a sixth status (blocked_by_governor) the dispatcher cannot return, and therefore proved the shaping worked for a status that does not exist; it now uses a real one.

Three governor-gated outputs declared their own error: z.string(), colliding with the shared envelope — one field name, two meanings. The refusal detail moves inside the envelope under the code its status maps to; blocked and reason still carry the refusal's own vocabulary, so nothing is lost.

#9658 — the registration outside the chokepoint

withMinerToolErrorHandling's doc calls the tool name REQUIRED, "leaving it optional would have made 'instrumented' a property of each call site rather than of the wrapper". Twenty registrations honoured it. loopover_miner_ping did not — so the health check an operator's monitoring hits on a loop, the cheapest signal that this server is alive and being used at all, reported zero calls forever.

Two tests, because either alone can go quiet: a structural rule that fails on any registration whose handler does not reach the wrapper (with its own canary proving the pattern still matches), and a behavioural one that calls every tool listToolDefinitions({ locality: ["miner"] }) projects — derived from the registry, never a literal array — asserting each produced exactly one record.

One deliberate deviation from the issue text

Deliverable 2 asks for a test that "an error-envelope structuredContent validates against each tool's advertised output schema". Taken literally that requires every success field to become optional, which weakens exactly the contract this epic exists to strengthen — and it buys nothing, because the MCP SDK exempts an isError result from output validation (validateToolOutput returns early on result.isError). The envelope therefore rides alongside the success fields, and the test asserts what is actually load-bearing: every miner output advertises error with the closed code set, the schema accepts a payload carrying it, and an invented code is refused.

Validation

  • npx vitest run --changed=origin/main: 2586 passed, 175 files. test/contract/: 108 passed. tsc --noEmit clean.
  • The store-failure case asserted end to end: what the caller is told, what telemetry records, and that the message alone would have been classified differently.
  • docs:drift-check, manifest:drift-check, ui:openapi:check, contract:api-schemas:check clean.
  • The empty import type { } from "@loopover/contract" in dispatch-telemetry-sink.ts is gone.

@loopover-orb

loopover-orb Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Important

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏳ LoopOver is waiting…

LoopOver has seen this pull request and is waiting on CI checks to finish before reviewing it. This comment will update once the review runs.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed · 🟨 Waiting

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jul 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.64%. Comparing base (503eb0c) to head (84c2565).
⚠️ Report is 7 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #9815   +/-   ##
=======================================
  Coverage   91.64%   91.64%           
=======================================
  Files         915      915           
  Lines      112706   112713    +7     
  Branches    27071    27074    +3     
=======================================
+ Hits       103285   103293    +8     
  Misses       8130     8130           
+ Partials     1291     1290    -1     
Flag Coverage Δ
backend 95.67% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
packages/loopover-contract/src/shared.ts 100.00% <ø> (ø)
packages/loopover-contract/src/telemetry.ts 100.00% <100.00%> (ø)
packages/loopover-contract/src/tools/miner-ops.ts 100.00% <ø> (ø)
packages/loopover-contract/src/tools/miner.ts 100.00% <ø> (ø)
packages/loopover-mcp/lib/telemetry.ts 100.00% <100.00%> (ø)
packages/loopover-miner/bin/loopover-miner-mcp.ts 98.00% <100.00%> (+0.70%) ⬆️
...ackages/loopover-miner/lib/chat-action-dispatch.ts 100.00% <100.00%> (ø)
...kages/loopover-miner/lib/mcp-dispatch-telemetry.ts 100.00% <100.00%> (ø)
src/mcp/dispatch-telemetry-sink.ts 100.00% <ø> (ø)
src/mcp/dispatch-telemetry.ts 100.00% <100.00%> (ø)

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 29, 2026
@loopover-orb loopover-orb Bot added the manual-review Gittensor contributor context label Jul 29, 2026
@JSONbored
JSONbored force-pushed the mcp/error-code-telemetry-9659 branch 2 times, most recently from 062c929 to ff54490 Compare July 29, 2026 12:01
@JSONbored
JSONbored force-pushed the mcp/error-code-telemetry-9659 branch from ff54490 to 02d66fd Compare July 29, 2026 12:18
Base automatically changed from mcp/advertise-contract-metadata-9655 to main July 29, 2026 12:36
@JSONbored
JSONbored force-pushed the mcp/error-code-telemetry-9659 branch 2 times, most recently from 56e2b04 to 05eddb9 Compare July 29, 2026 12:38
…he chokepoint (#9659)

`@loopover/contract` defined both halves of an error-envelope contract and joined neither.
`toolErrorFields` -- "code is drawn from a closed, developer-defined set so telemetry can break
failures down by cause" -- had zero consumers and typed that code as `z.string()`.
`resolveErrorCode`'s first branch reads an envelope's declared code; nothing ever passed it one.

So the code dimension was dead on the failure path that does not throw:

- the REMOTE emitted a hardcoded `"unknown_error"` for every returned failure;
- the STDIO wrapper passed no error at all, so `resolveErrorCode(undefined)` said the same;
- the MINER built a typed envelope for the caller and then threw it away, passing the raw error --
  whose ENOENT message matches `/not found|no such/`, so a store that would not open told the caller
  `store_unavailable` while telemetry recorded `not_found`. Two classifications of one failure, from
  adjacent lines.

Now: `store_unavailable` joins the closed set (the miner already returned it to callers),
`toolErrorFields.error.code` IS that set rather than free text, all 16 miner output schemas spread it
so the envelope appears in the artifacts a consumer reads, and all three chokepoints classify from
the result's own envelope via the one classifier. A code outside the set still falls back to
`unknown_error`, so a tool cannot widen the dimension by inventing one.

Two things this uncovered, fixed here rather than left:

- `ChatActionDispatchResult.status` was `string`, which is what made the miner's refusal mapping
  unverifiable. It is a discriminated union of the five outcomes `dispatchChatAction` can return, and
  the refusal-to-code map is a `Record` over it -- so adding a status without deciding what it means
  to a caller fails the build. A test fixture was inventing a sixth status the dispatcher cannot
  return; it now uses a real one.
- Three governor-gated outputs declared their own `error: z.string()`, colliding with the shared
  envelope. The refusal detail moves inside the envelope under the code its status maps to; `blocked`
  and `reason` still carry the refusal's own vocabulary.

Also closes #9658: `loopover_miner_ping` was the one registration that never reached
`withMinerToolErrorHandling`, whose doc calls the tool name REQUIRED precisely so "instrumented" is a
property of the wrapper rather than of each call site. The health check an operator's monitoring hits
on a loop reported zero calls forever. A structural rule now fails on any registration that bypasses
the wrapper, and a behavioural test calls every tool the registry projects for this server and asserts
each produced exactly one record.

The empty `import type { } from "@loopover/contract"` in dispatch-telemetry-sink.ts is gone.
@JSONbored
JSONbored force-pushed the mcp/error-code-telemetry-9659 branch from 05eddb9 to 84c2565 Compare July 29, 2026 12:54
@JSONbored
JSONbored merged commit f881645 into main Jul 29, 2026
4 checks passed
@JSONbored
JSONbored deleted the mcp/error-code-telemetry-9659 branch July 29, 2026 13:04
JSONbored added a commit that referenced this pull request Jul 29, 2026
…ed (#9846)

main is red on ui:typecheck, which blocks every open PR.

#9659/#9815 correctly tightened ChatActionDispatchResult from a loose
`{ ok: boolean; status: string; ... }` into a union discriminated on `ok`, so the
miner MCP server's refusal -> error-code mapping could finally be checked. But
this miner-UI test mocks the dispatcher with a bare object literal, whose
`ok`/`status` widen to boolean/string on inference -- and a widened literal no
longer satisfies the closed union.

Annotate the mock's return type (and import the real type) so it is validated
against the shipped contract instead of inference. That also stops the mock
drifting from the union the next time it changes -- the widened version would
have kept compiling against any shape at all.
JSONbored added a commit that referenced this pull request Jul 29, 2026
…ed (#9846)

main is red on ui:typecheck, which blocks every open PR.

#9659/#9815 correctly tightened ChatActionDispatchResult from a loose
`{ ok: boolean; status: string; ... }` into a union discriminated on `ok`, so the
miner MCP server's refusal -> error-code mapping could finally be checked. But
this miner-UI test mocks the dispatcher with a bare object literal, whose
`ok`/`status` widen to boolean/string on inference -- and a widened literal no
longer satisfies the closed union.

Annotate the mock's return type (and import the real type) so it is validated
against the shipped contract instead of inference. That also stops the mock
drifting from the union the next time it changes -- the widened version would
have kept compiling against any shape at all.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. manual-review Gittensor contributor context

Projects

None yet

1 participant