Skip to content

ams(mcp): route loopover_miner_ping through the miner dispatch chokepoint #9658

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

#9525 made withMinerToolErrorHandling
(packages/loopover-miner/bin/loopover-miner-mcp.ts:114-132) the miner server's dispatch-telemetry
chokepoint. Its own doc comment states the design contract:

#9525: the tool name, so this same wrapper doubles as the miner's dispatch-telemetry chokepoint.
REQUIRED -- every registration passes it, and leaving it optional would have made
"instrumented" a property of each call site rather than of the wrapper.

Ten of the eleven tools honour that. loopover_miner_ping does not — its registration passes a bare
async () => minerToolResult(MINER_PING_STATUS) handler that never calls the wrapper, and therefore
never calls recordMinerDispatchTelemetry
(packages/loopover-miner/lib/mcp-dispatch-telemetry.ts:34). No usage_event, no $mcp_tool_call.

The tool this omits is the health check — the one an operator's monitoring hits on a loop and the one
whose call volume is the cheapest signal that the miner MCP server is alive and being used at all. A
usage_event breakdown by tool on the miner surface silently reports zero pings forever.

Nothing catches it: the miner server has no structural test asserting every registration is wrapped
(contrast test/unit/mcp-dispatch-telemetry-sink.test.ts:146, which relies on the remote server's
register wrapper being the single chokepoint), and test/unit/miner-mcp-scaffold.test.ts:104-117
only asserts ping's payload.

Requirements

  1. loopover_miner_ping's handler routes through withMinerToolErrorHandling, exactly like the
    other ten, so it emits the same two events. The tool's returned content text and
    structuredContent must be byte-identical to today's (MINER_PING_STATUS), asserted by the
    existing invariant tests.
  2. A structural test that cannot be satisfied by hand-listing tool names: it introspects
    packages/loopover-miner/bin/loopover-miner-mcp.ts's source and fails if any
    server.registerTool( call's handler body does not reach withMinerToolErrorHandling. Model it
    on the repo's existing structural rules (test/unit/no-direct-octokit.test.ts).
  3. A behavioural test that connects to the miner server with a stubbed dispatch-telemetry recorder,
    calls every tool name from listToolDefinitions({ locality: ["miner"] }), and asserts each one
    produced exactly one telemetry record with the matching tool name. Deriving the name list from
    the registry (not a literal array) is required — test/unit/miner-mcp-scaffold.test.ts:91-102 is
    the precedent.

⚠️ Required pattern: withMinerToolErrorHandling(run, toolName) at
packages/loopover-miner/bin/loopover-miner-mcp.ts:114-132, used by the other ten registrations.
Adding a bespoke recordMinerDispatchTelemetry(...) call inline in the ping handler, or adding a
second wrapper just for tools that cannot fail, does NOT satisfy this issue — the point is that
"instrumented" is a property of the wrapper, not of the call site.

Deliverables

  • loopover_miner_ping's registration wraps its handler in withMinerToolErrorHandling
  • A structural test failing any miner registerTool handler that does not reach the wrapper,
    with a fixture case proving the test itself detects an unwrapped registration
  • A telemetry test driving every registry-derived miner tool name and asserting one dispatch
    record each, including loopover_miner_ping
  • The existing ping payload invariants in test/unit/miner-mcp-scaffold.test.ts:104-117 still
    pass unchanged

All Deliverables above are required in a single PR. A PR that satisfies only some of them — for
example wrapping the ping handler without adding the structural test, so the next tool added can
repeat the omission — does not resolve this issue.

Test Coverage Requirements

99%+ Codecov patch coverage, branch-counted, applies: packages/loopover-miner/bin/**/*.ts is
explicitly listed in coverage.include (vitest.config.ts:69-77, which calls
bin/loopover-miner-mcp.ts out by name as "genuinely unit-coverable"). Both the success and the
throw arm of the wrapper as reached from the ping registration need a test, and a named regression
test must assert loopover_miner_ping emits a dispatch telemetry record.

Expected Outcome

All eleven miner tools emit usage_event and $mcp_tool_call; the miner surface's telemetry is
complete rather than 10 of 11; and a future twelfth tool registered without the wrapper fails CI
instead of shipping uninstrumented.

Links & Resources

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