Skip to content

Commit cade4d9

Browse files
committed
mcp(telemetry): one envelope, one code, and no registration outside the 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.
1 parent 5577551 commit cade4d9

16 files changed

Lines changed: 416 additions & 21 deletions

packages/loopover-contract/src/shared.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
// second. Two tools that happen to take the same fields today are usually a coincidence, and
66
// prematurely coupling them means a later divergence has to be un-shared under pressure.
77
import { z } from "zod";
8+
import { MCP_TELEMETRY_ERROR_CODES } from "./telemetry.js";
89

910
/** The owner/repo pair virtually every repo-scoped tool takes. */
1011
export const ownerRepoInput = z.object({
@@ -38,7 +39,10 @@ export const freshnessFields = {
3839
export const toolErrorFields = {
3940
error: z
4041
.object({
41-
code: z.string().min(1),
42+
// #9659: the closed set, not free text. The doc above always said "drawn from a closed,
43+
// developer-defined set so telemetry can break failures down by cause" -- while the type said
44+
// `z.string()`, which is what let a server return a code telemetry then re-guessed differently.
45+
code: z.enum(MCP_TELEMETRY_ERROR_CODES),
4246
message: z.string().min(1),
4347
})
4448
.optional(),

packages/loopover-contract/src/telemetry.ts

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,10 @@ export const MCP_TELEMETRY_ERROR_CODES = [
3535
"upstream_error",
3636
"timeout",
3737
"elicitation_declined",
38+
// #9659: the miner's own envelope has always returned this to callers -- a local SQLite store that
39+
// will not open. It belongs in the closed set so ONE code can serve both the envelope and the
40+
// telemetry event, rather than the event re-deriving a different one from the message.
41+
"store_unavailable",
3842
"unknown_error",
3943
] as const;
4044
export type McpTelemetryErrorCode = (typeof MCP_TELEMETRY_ERROR_CODES)[number];
@@ -264,6 +268,22 @@ export function mcpToolSpanName(tool: string): string {
264268
* Matches on shape and on the small set of messages the servers actually produce; everything else
265269
* is `unknown_error` rather than a guess. Never reads a caller-supplied string into the code.
266270
*/
271+
/**
272+
* The error envelope a tool's `structuredContent` carries, if it carries one (#9659).
273+
*
274+
* Every server reports failure the same way -- `isError: true` plus `{ error: { code, message } }` -- but
275+
* each was reading that result differently, or not at all: the remote emitted a hardcoded
276+
* `"unknown_error"`, the stdio one passed nothing to the classifier, and the miner passed the raw thrown
277+
* error so the code was re-derived from message regexes and disagreed with the code the caller was given.
278+
* Feed the result of this straight to `resolveErrorCode`, which validates the declared code against the
279+
* closed set and falls back for anything else.
280+
*/
281+
export function toolErrorEnvelope(structuredContent: unknown): { code?: unknown; message?: unknown } | undefined {
282+
if (typeof structuredContent !== "object" || structuredContent === null) return undefined;
283+
const envelope = (structuredContent as { error?: unknown }).error;
284+
return typeof envelope === "object" && envelope !== null ? (envelope as { code?: unknown; message?: unknown }) : undefined;
285+
}
286+
267287
export function resolveErrorCode(error: unknown): McpTelemetryErrorCode {
268288
const envelope = error as { code?: unknown } | null | undefined;
269289
if (envelope && typeof envelope.code === "string") {

packages/loopover-contract/src/tools/miner-ops.ts

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
// agent-safe control and is here instead.
2121
import { z } from "zod";
2222
import { defineTool } from "../tool-definition.js";
23+
import { toolErrorFields } from "../shared.js";
2324
import { INSTANCE_CHECK_STATUSES } from "../enums.js";
2425

2526
const RepoFullName = z.string().min(3).max(200).describe("owner/repo.");
@@ -32,6 +33,11 @@ export const MinerDoctorInput = z.object({});
3233
export const MinerDoctorOutput = z.looseObject({
3334
ok: z.boolean().describe("True when no check reported fail. Warnings do not clear it to false."),
3435
checks: z.array(z.looseObject({ name: z.string(), status: z.enum(INSTANCE_CHECK_STATUSES), detail: z.string().optional() })),
36+
// #9659: every miner tool answers a store failure with the shared error envelope
37+
// (`withMinerToolErrorHandling`), so the advertised schema declares it rather than describing only
38+
// the success shape. `error.code` is the closed telemetry set, which is what lets the code the caller
39+
// is told and the code telemetry records be the same one.
40+
...toolErrorFields,
3541
});
3642

3743
export const minerDoctorTool = defineTool({
@@ -59,6 +65,11 @@ export const MinerMetricsSnapshotOutput = z.looseObject({
5965
samples: z.array(z.looseObject({ value: z.number(), labels: z.record(z.string(), z.string()).optional() })),
6066
}),
6167
),
68+
// #9659: every miner tool answers a store failure with the shared error envelope
69+
// (`withMinerToolErrorHandling`), so the advertised schema declares it rather than describing only
70+
// the success shape. `error.code` is the closed telemetry set, which is what lets the code the caller
71+
// is told and the code telemetry records be the same one.
72+
...toolErrorFields,
6273
});
6374

6475
export const minerMetricsSnapshotTool = defineTool({
@@ -87,6 +98,11 @@ export const MinerGovernorActionOutput = z.looseObject({
8798
reason: z.string().optional(),
8899
result: z.unknown().optional(),
89100
error: z.string().optional(),
101+
// #9659: every miner tool answers a store failure with the shared error envelope
102+
// (`withMinerToolErrorHandling`), so the advertised schema declares it rather than describing only
103+
// the success shape. `error.code` is the closed telemetry set, which is what lets the code the caller
104+
// is told and the code telemetry records be the same one.
105+
...toolErrorFields,
90106
});
91107

92108
export const MinerGovernorPauseInput = z.object({
@@ -217,6 +233,11 @@ export const MinerRunMigrationsOutput = z.looseObject({
217233
blocked: z.boolean().optional(),
218234
reason: z.string().optional(),
219235
error: z.string().optional(),
236+
// #9659: every miner tool answers a store failure with the shared error envelope
237+
// (`withMinerToolErrorHandling`), so the advertised schema declares it rather than describing only
238+
// the success shape. `error.code` is the closed telemetry set, which is what lets the code the caller
239+
// is told and the code telemetry records be the same one.
240+
...toolErrorFields,
220241
});
221242

222243
export const minerRunMigrationsTool = defineTool({
@@ -249,6 +270,11 @@ export const MinerPurgeRepoOutput = z.looseObject({
249270
blocked: z.boolean().optional(),
250271
reason: z.string().optional(),
251272
error: z.string().optional(),
273+
// #9659: every miner tool answers a store failure with the shared error envelope
274+
// (`withMinerToolErrorHandling`), so the advertised schema declares it rather than describing only
275+
// the success shape. `error.code` is the closed telemetry set, which is what lets the code the caller
276+
// is told and the code telemetry records be the same one.
277+
...toolErrorFields,
252278
});
253279

254280
export const minerPurgeRepoTool = defineTool({

packages/loopover-contract/src/tools/miner.ts

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
// gains is a description of what it already returns, not a new shape.
1212
import { z } from "zod";
1313
import { defineTool } from "../tool-definition.js";
14+
import { toolErrorFields } from "../shared.js";
1415
import { PLAN_STEP_STATUSES } from "../enums.js";
1516

1617
/** Statuses a portfolio-queue entry can hold. */
@@ -35,6 +36,11 @@ export const MinerPingInput = z.object({});
3536
export const MinerPingOutput = z.looseObject({
3637
status: z.literal("ok"),
3738
tool: z.literal("loopover_miner_ping"),
39+
// #9659: every miner tool answers a store failure with the shared error envelope
40+
// (`withMinerToolErrorHandling`), so the advertised schema declares it rather than describing only
41+
// the success shape. `error.code` is the closed telemetry set, which is what lets the code the caller
42+
// is told and the code telemetry records be the same one.
43+
...toolErrorFields,
3844
});
3945

4046
export const minerPingTool = defineTool({
@@ -69,6 +75,11 @@ export const MinerPortfolioDashboardOutput = z.looseObject({
6975
}),
7076
),
7177
oldestQueuedAgeMs: z.number().nullable(),
78+
// #9659: every miner tool answers a store failure with the shared error envelope
79+
// (`withMinerToolErrorHandling`), so the advertised schema declares it rather than describing only
80+
// the success shape. `error.code` is the closed telemetry set, which is what lets the code the caller
81+
// is told and the code telemetry records be the same one.
82+
...toolErrorFields,
7283
});
7384

7485
export const minerPortfolioDashboardTool = defineTool({
@@ -114,6 +125,11 @@ export const MinerManageStatusOutput = z.looseObject({
114125
prs: z.array(manageStatusRowSchema),
115126
}),
116127
),
128+
// #9659: every miner tool answers a store failure with the shared error envelope
129+
// (`withMinerToolErrorHandling`), so the advertised schema declares it rather than describing only
130+
// the success shape. `error.code` is the closed telemetry set, which is what lets the code the caller
131+
// is told and the code telemetry records be the same one.
132+
...toolErrorFields,
117133
});
118134

119135
export const minerManageStatusTool = defineTool({
@@ -149,6 +165,11 @@ export const MinerListClaimsOutput = z.looseObject({
149165
note: z.string().nullish(),
150166
}),
151167
),
168+
// #9659: every miner tool answers a store failure with the shared error envelope
169+
// (`withMinerToolErrorHandling`), so the advertised schema declares it rather than describing only
170+
// the success shape. `error.code` is the closed telemetry set, which is what lets the code the caller
171+
// is told and the code telemetry records be the same one.
172+
...toolErrorFields,
152173
});
153174

154175
export const minerListClaimsTool = defineTool({
@@ -187,6 +208,11 @@ export const MinerAuditFeedOutput = z.looseObject({
187208
createdAt: z.string(),
188209
}),
189210
),
211+
// #9659: every miner tool answers a store failure with the shared error envelope
212+
// (`withMinerToolErrorHandling`), so the advertised schema declares it rather than describing only
213+
// the success shape. `error.code` is the closed telemetry set, which is what lets the code the caller
214+
// is told and the code telemetry records be the same one.
215+
...toolErrorFields,
190216
});
191217

192218
export const minerAuditFeedTool = defineTool({
@@ -219,6 +245,11 @@ export const MinerGetRunStateOutput = z.looseObject({
219245
repoFullName: z.string().optional(),
220246
state: z.enum(MINER_RUN_STATES).nullable().optional(),
221247
states: z.array(z.looseObject({ repoFullName: z.string(), state: z.enum(MINER_RUN_STATES).nullable() })).optional(),
248+
// #9659: every miner tool answers a store failure with the shared error envelope
249+
// (`withMinerToolErrorHandling`), so the advertised schema declares it rather than describing only
250+
// the success shape. `error.code` is the closed telemetry set, which is what lets the code the caller
251+
// is told and the code telemetry records be the same one.
252+
...toolErrorFields,
222253
});
223254

224255
export const minerGetRunStateTool = defineTool({
@@ -275,6 +306,11 @@ export const MinerListPlansInput = z.object({
275306

276307
export const MinerListPlansOutput = z.looseObject({
277308
plans: z.array(minerPlanRecordSchema),
309+
// #9659: every miner tool answers a store failure with the shared error envelope
310+
// (`withMinerToolErrorHandling`), so the advertised schema declares it rather than describing only
311+
// the success shape. `error.code` is the closed telemetry set, which is what lets the code the caller
312+
// is told and the code telemetry records be the same one.
313+
...toolErrorFields,
278314
});
279315

280316
export const minerListPlansTool = defineTool({
@@ -300,6 +336,11 @@ export const MinerGetPlanOutput = z.looseObject({
300336
planId: z.string().optional(),
301337
found: z.boolean(),
302338
plan: minerPlanRecordSchema.optional(),
339+
// #9659: every miner tool answers a store failure with the shared error envelope
340+
// (`withMinerToolErrorHandling`), so the advertised schema declares it rather than describing only
341+
// the success shape. `error.code` is the closed telemetry set, which is what lets the code the caller
342+
// is told and the code telemetry records be the same one.
343+
...toolErrorFields,
303344
});
304345

305346
export const minerGetPlanTool = defineTool({
@@ -335,6 +376,11 @@ export const MinerGovernorDecisionsOutput = z.looseObject({
335376
reason: z.string(),
336377
}),
337378
),
379+
// #9659: every miner tool answers a store failure with the shared error envelope
380+
// (`withMinerToolErrorHandling`), so the advertised schema declares it rather than describing only
381+
// the success shape. `error.code` is the closed telemetry set, which is what lets the code the caller
382+
// is told and the code telemetry records be the same one.
383+
...toolErrorFields,
338384
});
339385

340386
export const minerGovernorDecisionsTool = defineTool({
@@ -371,6 +417,11 @@ export const MinerStatusOutput = z.looseObject({
371417
}),
372418
}),
373419
doctor: z.array(z.looseObject({ name: z.string(), ok: z.boolean(), detail: z.string() })),
420+
// #9659: every miner tool answers a store failure with the shared error envelope
421+
// (`withMinerToolErrorHandling`), so the advertised schema declares it rather than describing only
422+
// the success shape. `error.code` is the closed telemetry set, which is what lets the code the caller
423+
// is told and the code telemetry records be the same one.
424+
...toolErrorFields,
374425
});
375426

376427
export const minerStatusTool = defineTool({
@@ -410,6 +461,11 @@ export const MinerCalibrationReportOutput = z.looseObject({
410461
}),
411462
),
412463
hasSignal: z.boolean(),
464+
// #9659: every miner tool answers a store failure with the shared error envelope
465+
// (`withMinerToolErrorHandling`), so the advertised schema declares it rather than describing only
466+
// the success shape. `error.code` is the closed telemetry set, which is what lets the code the caller
467+
// is told and the code telemetry records be the same one.
468+
...toolErrorFields,
413469
});
414470

415471
export const minerCalibrationReportTool = defineTool({

packages/loopover-mcp/lib/telemetry.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import {
77
MCP_TOOL_CALL_EVENT,
88
MCP_USAGE_EVENT,
99
resolveErrorCode,
10+
toolErrorEnvelope,
1011
toolExcludesPayloads,
1112
UNKNOWN_TOOL_CATEGORY,
1213
type McpTelemetryTransport,
@@ -131,6 +132,9 @@ export function wrapStdioToolHandler(
131132
transport,
132133
args: args[0],
133134
result: result?.structuredContent,
135+
// #9659: on the failure path this used to pass no error at all, so `resolveErrorCode(undefined)`
136+
// classified every returned failure as `unknown_error` regardless of what the tool told the caller.
137+
...(ok ? {} : { error: toolErrorEnvelope(result?.structuredContent) }),
134138
});
135139
return result;
136140
} catch (error) {

packages/loopover-miner/bin/loopover-miner-mcp.ts

Lines changed: 36 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
import { readFileSync, realpathSync } from "node:fs";
33
import { fileURLToPath } from "node:url";
44
import { McpServer, type ToolCallback } from "@modelcontextprotocol/sdk/server/mcp.js";
5+
import type { McpTelemetryErrorCode } from "@loopover/contract";
56
import { StdioServerTransport } from "@modelcontextprotocol/sdk/server/stdio.js";
67
// #9536: every tool's schemas come from the shared contract instead of being declared here. The
78
// remote and stdio servers already register from the same package (#9517/#9518) -- this closes the
@@ -53,7 +54,7 @@ import { initGovernorLedger } from "../lib/governor-ledger.js";
5354
import { collectStatus, runDoctorChecks } from "../lib/status.js";
5455
import { collectMinerPredictionMetrics } from "@loopover/engine";
5556
import { collectPredictionMetricRows } from "../lib/metrics-cli.js";
56-
import { dispatchChatAction } from "../lib/chat-action-dispatch.js";
57+
import { dispatchChatAction, type ChatActionRefusalStatus } from "../lib/chat-action-dispatch.js";
5758
import {
5859
MINER_CLAIM_RELEASE_ACTION,
5960
MINER_DENY_HOOKS_DECIDE_ACTION,
@@ -86,9 +87,21 @@ import { captureMinerPostHogErrorAndFlush, initMinerPostHog } from "../lib/posth
8687
* thrown values to this set rather than passing a caller/store-derived string through, matching the
8788
* `code`/`message` shape @loopover/contract's shared `toolErrorFields` describes.
8889
*/
89-
type MinerToolErrorCode = "store_unavailable" | "unknown_error";
90-
91-
function toolErrorCode(error: unknown): MinerToolErrorCode {
90+
/**
91+
* Each way a dispatch can refuse, as the closed telemetry code for it (#9659).
92+
*
93+
* A `Record` over the status union rather than a lookup with a fallback: adding a dispatch status without
94+
* deciding what it means to a caller then fails the build, which is the only reason a mapping between two
95+
* closed vocabularies is safe to write down at all.
96+
*/
97+
const DISPATCH_REFUSAL_CODES: Record<ChatActionRefusalStatus, McpTelemetryErrorCode> = {
98+
disabled: "not_configured",
99+
unknown_action: "not_found",
100+
invalid_params: "invalid_input",
101+
handler_error: "upstream_error",
102+
};
103+
104+
function toolErrorCode(error: unknown): McpTelemetryErrorCode {
92105
// A local SQLite store failing to open (missing file, corrupted file, permissions) is the one
93106
// failure mode every store-backed tool below can actually hit; anything else is unclassified.
94107
return error instanceof Error && /not found|not a database|permission|ENOENT/i.test(error.message) ? "store_unavailable" : "unknown_error";
@@ -173,7 +186,10 @@ async function withMinerToolErrorHandling<T extends object>(
173186
return payload;
174187
} catch (error) {
175188
const data = { error: { code: toolErrorCode(error), message: error instanceof Error ? error.message : String(error) } };
176-
recordMinerDispatchTelemetry({ tool: toolName, ok: false, durationMs: Date.now() - startedAt, error });
189+
// #9659: the ENVELOPE, not the raw error. Passing the raw one made telemetry re-derive a code from
190+
// the message regexes, so a store that would not open told the caller `store_unavailable` while the
191+
// event recorded `not_found` -- two classifications of one failure, from adjacent lines.
192+
recordMinerDispatchTelemetry({ tool: toolName, ok: false, durationMs: Date.now() - startedAt, error, errorEnvelope: data.error });
177193
return { ...minerToolResult(data), isError: true };
178194
}
179195
}
@@ -274,7 +290,11 @@ export interface MinerMcpServerOptions {
274290
export function createMinerMcpServer(options: MinerMcpServerOptions = {}) {
275291
const server = new McpServer({ name: "loopover-miner", version: ownPackageJson.version });
276292

277-
registerMinerTool(server, minerPingTool, async () => minerToolResult(MINER_PING_STATUS),
293+
// #9658: through the wrapper, like the other twenty. Ping is the health check an operator's monitoring
294+
// hits on a loop -- the cheapest signal that this server is alive and being used -- and it was the one
295+
// registration that never reached the dispatch-telemetry chokepoint, so a `usage_event` breakdown by
296+
// tool reported zero pings forever.
297+
registerMinerTool(server, minerPingTool, () => withMinerToolErrorHandling(() => MINER_PING_STATUS, minerPingTool.name),
278298
);
279299

280300
registerMinerTool(server, minerPortfolioDashboardTool, () =>
@@ -436,7 +456,16 @@ export function createMinerMcpServer(options: MinerMcpServerOptions = {}) {
436456
if (result.ok) return { ok: true, action, result: (result as { result?: unknown }).result };
437457
// A refusal is an ANSWER, not a transport failure: the caller needs to know the governor said no and
438458
// why, which a thrown error would flatten into a generic tool error.
439-
return { ok: false, action, blocked: true, reason: result.status, ...(result.error ? { error: result.error } : {}) };
459+
return {
460+
ok: false,
461+
action,
462+
blocked: true,
463+
reason: result.status,
464+
// #9659: the shared envelope, so one `error` field means one thing everywhere. `blocked` and
465+
// `reason` still carry the refusal's own vocabulary; what used to be a bare string detail is now
466+
// the envelope's `message`, under a code drawn from the same closed set telemetry records.
467+
error: { code: DISPATCH_REFUSAL_CODES[result.status] ?? "unknown_error", message: typeof result.error === "string" ? result.error : result.status },
468+
};
440469
};
441470

442471
registerMinerTool(server, minerGovernorPauseTool, (input) => withMinerToolErrorHandling(() => dispatchResult(GOVERNOR_PAUSE_CHAT_ACTION, input.reason ? { reason: input.reason } : {}), minerGovernorPauseTool.name),

0 commit comments

Comments
 (0)