mcp(telemetry): one envelope, one code, and no registration outside the chokepoint (#9659) - #9815
Merged
Merged
Conversation
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 |
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. 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
Flags with carried forward coverage won't be shown. Click here to find out more.
|
JSONbored
force-pushed
the
mcp/error-code-telemetry-9659
branch
2 times, most recently
from
July 29, 2026 12:01
062c929 to
ff54490
Compare
JSONbored
force-pushed
the
mcp/error-code-telemetry-9659
branch
from
July 29, 2026 12:18
ff54490 to
02d66fd
Compare
JSONbored
force-pushed
the
mcp/error-code-telemetry-9659
branch
2 times, most recently
from
July 29, 2026 12:38
56e2b04 to
05eddb9
Compare
…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
force-pushed
the
mcp/error-code-telemetry-9659
branch
from
July 29, 2026 12:54
05eddb9 to
84c2565
Compare
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #9659. Also closes #9658 — the miner's chokepoint is where both land.
The defect
@loopover/contractdefined both halves of an error-envelope contract and joined neither:toolErrorFields— "codeis 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 asz.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 readsenvelope.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:
errorCode: "unknown_error" as const— a literal, never the classifiererrorpassed at all, soresolveErrorCode(undefined)→unknown_errorThe miner's is the worst because it is actively contradictory: an
ENOENT: no such filemessage matches/not found\|no such/, so a store that would not open told the callerstore_unavailablewhile telemetry recordednot_found. Two classifications of one failure, produced by adjacent lines. There is a test for exactly that now.What changed
store_unavailablejoinsMCP_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.codeis that closed set (z.enum), as its own doc always claimed.toolErrorFields, so the field a caller must read to know why a call failed finally appears in an artifact.toolErrorEnvelope→resolveErrorCode). A code outside the closed set still falls back tounknown_error, so a tool cannot widen the dimension by inventing one.Two things this uncovered, fixed rather than left
ChatActionDispatchResult.statuswasstring. That is what made the refusal-to-code mapping unverifiable in the first place. It is now a discriminated union of the five outcomesdispatchChatActioncan actually return, and the mapping is aRecordover 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;blockedandreasonstill 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_pingdid 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
structuredContentvalidates 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 anisErrorresult from output validation (validateToolOutputreturns early onresult.isError). The envelope therefore rides alongside the success fields, and the test asserts what is actually load-bearing: every miner output advertiseserrorwith 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 --noEmitclean.docs:drift-check,manifest:drift-check,ui:openapi:check,contract:api-schemas:checkclean.import type { } from "@loopover/contract"indispatch-telemetry-sink.tsis gone.