test(metrics): #403 review — lock the registerTool monkeypatch + reword overhead note (#402)

DO-1: add an integration test (test/mock/tool-timing-server.test.mjs) that
constructs a real createDocmostMcpServer with a spy onMetric, links a Client
over InMemoryTransport, invokes get_workspace (no input schema, so the wrapped
handler always runs) and asserts onMetric fired with
("mcp_tool_duration_seconds", <number>, { tool: "get_workspace" }). This locks
that the monkeypatch wraps every tool AND labels with the registration name —
which the isolated timeToolHandler unit test does not. Mutation-verified
(mislabel -> test fails). The tool's expected backend failure (ECONNREFUSED)
is tolerated: the wrapper times in a finally on throw too, so the metric fires.

DO-2: reword the mcp.service.ts "zero overhead when disabled" comment to
"negligible overhead" — the registerTool wrapper still runs performance.now()
+ an async try/finally per tool call when onMetric is undefined (the
`onMetric?.()` short-circuits the label build; cost is immaterial at
tool-call rate), so "zero" was literally inaccurate.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

apps/server/src/integrations/mcp/mcp.service.ts

@@ -338,9 +338,12 @@ export class McpService implements OnModuleDestroy {
             // mapping) is owned by SandboxStore.asSink().
             // Route the package's dependency-neutral metric samples onto the
             // prom-client registry. When metrics are disabled, onMetric is
-            // undefined → the package's tool-timer/timeout hooks no-op with
-            // zero overhead. labels?.tool is guarded defensively (the tool
-            // wrapper always sets it).
+            // undefined → the package's tool-timer/timeout hooks are a
+            // negligible-overhead no-op: the registerTool wrapper still runs a
+            // performance.now() + async try/finally per tool call, but the
+            // `onMetric?.()` short-circuits so no label/object is built. (Cost
+            // is immaterial at LLM tool-call rate.) labels?.tool is guarded
+            // defensively (the tool wrapper always sets it).
             return {
               ...resolved.config,
               sandbox: this.sandboxStore.asSink(),

packages/mcp/test/mock/tool-timing-server.test.mjs

@@ -0,0 +1,87 @@
+// #402 — INTEGRATION test that locks the registerTool monkeypatch installed by
+// createDocmostMcpServer (src/index.ts). The sibling unit test
+// (test/unit/tool-timing.test.mjs) only exercises the timeToolHandler helper in
+// ISOLATION; it never constructs the server, so nothing there proves the factory
+// actually (a) wraps every registered tool through that helper and (b) labels
+// each sample with the tool's REGISTRATION name.
+//
+// Here we stand up a real McpServer via the factory, connect a real MCP Client
+// over the SDK's in-memory transport, invoke one registered tool, and assert the
+// host's onMetric sink received `("mcp_tool_duration_seconds", <number>, { tool
+// })` with tool === the exact registration name. This locks both the "monkeypatch
+// wraps tools" and "label = registration name" halves of the contract, so a
+// mutation to args.slice(0, -1) / the handler-arg detection / the capture order
+// is caught.
+import { test } from "node:test";
+import assert from "node:assert/strict";
+
+import { Client } from "@modelcontextprotocol/sdk/client/index.js";
+import { InMemoryTransport } from "@modelcontextprotocol/sdk/inMemory.js";
+
+import { createDocmostMcpServer } from "../../build/index.js";
+
+// The tool we drive. get_workspace has NO input schema, so protocol-level input
+// validation cannot short-circuit before the handler runs — the wrapped handler
+// is guaranteed to execute (and then fail on the unreachable backend, which is
+// exactly what we want: the wrapper times in a finally on throw too).
+const TOOL_NAME = "get_workspace";
+
+test("the factory's registerTool monkeypatch times a live tool call and labels it with the registration name", async () => {
+  const calls = [];
+  const onMetric = (name, value, labels) => calls.push({ name, value, labels });
+
+  // Minimal valid credentials config. apiUrl points at a port that refuses
+  // connections immediately so the tool's backend call fails FAST (ECONNREFUSED)
+  // rather than hanging — the wrapper still emits the metric from its finally.
+  const server = createDocmostMcpServer({
+    apiUrl: "http://127.0.0.1:1",
+    email: "x@example.com",
+    password: "pw",
+    onMetric,
+  });
+
+  const [clientTransport, serverTransport] =
+    InMemoryTransport.createLinkedPair();
+
+  const client = new Client(
+    { name: "test-client", version: "0.0.0" },
+    { capabilities: {} },
+  );
+
+  await Promise.all([
+    server.connect(serverTransport),
+    client.connect(clientTransport),
+  ]);
+
+  try {
+    // Invoke the tool. The backend is unreachable, so this either resolves with
+    // an error result (isError) or rejects — both are fine. What matters is that
+    // the handler ran through the timing wrapper, which fires onMetric either way.
+    try {
+      await client.callTool({ name: TOOL_NAME, arguments: {} });
+    } catch {
+      // Tolerate the expected backend failure surfacing as a thrown protocol error.
+    }
+
+    // The wrapper must have fed exactly the timing sample for THIS tool.
+    const timing = calls.filter(
+      (c) => c.name === "mcp_tool_duration_seconds",
+    );
+    assert.ok(
+      timing.length >= 1,
+      "onMetric must receive a mcp_tool_duration_seconds sample from the wrapped handler",
+    );
+
+    const sample = timing.find((c) => c.labels && c.labels.tool === TOOL_NAME);
+    assert.ok(
+      sample,
+      `a timing sample must be labelled with the registration name "${TOOL_NAME}"; ` +
+        `got labels: ${JSON.stringify(timing.map((c) => c.labels))}`,
+    );
+    assert.equal(typeof sample.value, "number");
+    assert.ok(sample.value >= 0, "duration must be non-negative seconds");
+  } finally {
+    await client.close();
+    await server.close();
+  }
+});