feat(mcp): record local stdio tool calls at the dispatch chokepoint - #6432
Conversation
The local telemetry wrapper landed with the opt-in flag and the enable/disable/status commands, but nothing ever called it: a user could opt in and still have zero usage recorded. Wire it into the stdio tool-dispatch path. registerStdioTool is already the single point every one of the registered tools passes through, so no new chokepoint was needed -- it now wraps each handler to time the call and record exactly once, on both the return and the throw path. Fields match the remote side: tool name, callerType "local", ok, and coarse duration. Nothing else, and no behaviour change to any tool's response. `ok` mirrors the remote's caller-visible outcome (`response.status < 400`) rather than just "did it throw": a handler returning an error result is not a success. No handler does that today -- they signal failure by throwing, and the SDK maps it -- so this only matters if one ever starts. The opt-in read lives at module scope, not inside registerStdioTool: that function's second parameter is the TOOL's config and shadows the CLI's own `config`, so reading the flag inside it would have silently seen the wrong object and never fired. Telemetry failures stay invisible to callers: the chokepoint keeps a defensive try/catch over recordMcpToolCall's own never-throw guarantee, mirroring recordMcpToolTelemetry on the remote. Tests run the real stdio server as a subprocess against a local stand-in PostHog endpoint rather than mocking the SDK, because the opt-in guarantee is worth verifying on the wire and an in-process mock cannot reach a subprocess anyway. They cover: nothing sent by default even with an API key present, exactly one allowlisted event per call once opted in, one event per invocation rather than per session, disable returning to silence, and a failing tool recorded as ok=false while still failing identically for the caller. Closes JSONbored#6238
|
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 #6432 +/- ##
=======================================
Coverage 95.61% 95.61%
=======================================
Files 600 600
Lines 47346 47354 +8
Branches 15067 15068 +1
=======================================
+ Hits 45268 45276 +8
Misses 1291 1291
Partials 787 787
Flags with carried forward coverage won't be shown. Click here to find out more. |
loopover-orb
Bot
added
the
gittensor:feature
|
Tip ✅ LoopOver review result - approve/merge recommendedReview updated: 2026-07-16 07:52:30 UTC
Review summary Nits — 4 non-blocking
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionAddressed Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
![loopover-orb[bot]](https://avatars.githubusercontent.com/in/4139483?s=60&v=4){kind=small}
1 participant
Summary
The local telemetry wrapper landed with the opt-in flag and the
enable/disable/statuscommands, but nothing ever called it — a user could opt in and still have zero usage recorded. This wires it into the stdio tool-dispatch path.registerStdioToolis already the single point every registered tool passes through, so no new chokepoint was needed. It now wraps each handler to time the call and record exactly once, on both the return and the throw path. Fields match the remote side — tool name,callerType: "local",ok, coarse duration — and nothing else. No tool's response changes.Design notes
okmirrors the remote's caller-visible outcome (response.status < 400) rather than merely "did it throw": a handler that reports failure by returning an error result is not a success. No handler does that today — they signal failure by throwing and the SDK maps it — so this only matters if one ever starts.registerStdioTool(name, config, handler)'s second parameter is the tool's config and shadows the CLI's own module-levelconfig. Reading the flag inside that function would have silently resolved the wrong object and never fired — a bug that would have looked like working code and produced exactly zero events.try/catchoverrecordMcpToolCall's own never-throw guarantee, mirroringrecordMcpToolTelemetryon the remote.Tests: verified on the wire, not mocked
The issue asks that default-off be verified, not just documented, so these tests don't mock the PostHog SDK. The stdio server runs as a real subprocess (where an in-process
vi.mockcouldn't reach it anyway), pointed at a local stand-in ingestion endpoint viaLOOPOVER_MCP_POSTHOG_HOST. They assert what actually leaves the process:mcp_tool_callevent, with the allowlisted fieldstelemetry disableok=falseand still fails identically for the callerOpt-in is performed by running the real
telemetry enablecommand, not by hand-writing the config file.Two findings worth recording, both caught by the tests:
/batch/— so parsing silently yielded nothing and every "sent nothing" assertion would have passed no matter what the CLI did. The recorder now gunzips. To prove the gate tests are not vacuous, I mutation-tested them: forcingtelemetryEnabled: truemakes both fail loudly (expected [ { event: 'mcp_tool_call' … } ] to deeply equal []). The three positive tests fail against the unwired CLI.tool,caller_type,ok,duration_ms); the other four are the SDK's own$-prefixed vendor metadata —$lib,$lib_version,$is_server,$geoip_disable— which carry nothing about the user or the call. The test asserts the two sets separately, so a future field of ours can never hide among the vendor's. It also pinsdistinct_id === "loopover-mcp"(anonymous by construction),$geoip_disable === true, and that neither the commit message nor the PR body appears anywhere in the event.Validation
--body-filenon-regular inputs) are pre-existing on cleanmainwith my commit absent (Windows symlink restriction; they pass on CI's Linux).npm run typecheck— 0 errors.npm run build:mcp— passes.git diff --check— clean. Rebased on latestmain, no base conflict.Coverage
No patch surface: coverage is collected over
src/**,packages/loopover-engine/src/**, andpackages/loopover-miner/lib/**. This PR touches onlypackages/loopover-mcp/**(not collected) andtest/**(ignored). The behaviour is still covered end-to-end by the suite above.Scope
packages/,test/). The remote server and the wrapper module are untouched — this is only the wiring the wrapper was written for.lib/telemetry.js; the tests use a dummy key against a local endpoint.site/,CNAME, orlovablechanges.Safety
loopover-mcp telemetry enableand an API key is configured — verified on the wire, not assumed.Closes #6238