chore(mcp): trim metrics comments — keep the why, drop the narration

Comments restated the privacy invariant in four places and narrated what
the next line of code did. Collapse to one canonical site on observe()'s
signature; drop AI-pattern phrasing, #1351 references, and dated bucket
prose. No behavior change; 11 tests still pass.

gitnexus/src/cli/mcp.ts

@@ -55,11 +55,6 @@ export const mcpCommand = async () => {
     import('../mcp/metrics.js'),
   ]);
 
-  // OpenTelemetry metrics + Prometheus exporter. Opt-in via GITNEXUS_OTEL_METRICS.
-  // In stdio mode (this entrypoint), each agent host spawns its own process, so
-  // metrics are off by default. Operators who want server-side observability
-  // should run `gitnexus serve` and scrape that instance instead. The opt-in
-  // path still works here for local development / single-process setups.
   try {
     const result = await initMetrics();
     if (result.enabled) {
@@ -69,8 +64,6 @@ export const mcpCommand = async () => {
       );
     }
   } catch (err: any) {
-    // Metrics must never crash the server. EADDRINUSE here is the common case;
-    // log and continue without metrics.
     logger.warn(
       { err: err?.message ?? String(err) },
       'GitNexus: failed to start metrics endpoint — continuing without metrics',

gitnexus/src/cli/serve.ts

@@ -32,9 +32,6 @@ export const serveCommand = async (options?: { port?: string; host?: string }) =
   const host = options?.host ?? 'localhost';
 
   try {
-    // OpenTelemetry metrics + Prometheus exporter. Opt-in via
-    // GITNEXUS_OTEL_METRICS. `gitnexus serve` is the supported scrape target
-    // (see src/mcp/metrics.ts for the rationale).
     try {
       const { initMetrics } = await import('../mcp/metrics.js');
       const result = await initMetrics();
@@ -45,7 +42,6 @@ export const serveCommand = async (options?: { port?: string; host?: string }) =
         );
       }
     } catch (metricsErr: any) {
-      // Never crash the server because of metrics. Log and continue.
       logger.warn(
         { err: metricsErr?.message ?? String(metricsErr) },
         '[gitnexus serve] failed to start metrics endpoint — continuing without metrics',

gitnexus/src/mcp/metrics.ts

@@ -1,27 +1,8 @@
 /**
- * MCP server-side observability — OpenTelemetry metrics with Prometheus exposition.
- *
- * Answers issue #1351: provides ground-truth "is this MCP server being used?"
- * for operators who scrape `/metrics`.
- *
- * Design constraints from the maintainer's framing:
- * - OpenTelemetry format (not custom JSON, not pino).
- * - Anonymous: no per-call identifiers, no input data, no error message text.
- *   Cypher parse errors echo user input, so error is recorded as a boolean only.
- * - Prometheus: pull model via `/metrics`. No collector required.
- * - Quantized: histograms with explicit buckets. No raw per-call values escape.
- *
- * Privacy boundary is type-enforced: observe() takes `(result) => boolean`,
- * never a message-returning detector. The error label is `error="true|false"`,
- * never a message.
- *
- * Initialization is opt-in via GITNEXUS_OTEL_METRICS=on. Bound to 127.0.0.1 by
- * default. The exporter is configured to suppress `target_info` and scope info
- * so the only labels emitted are `tool` (closed set) and `error` (boolean).
- *
- * Stdio mode emits no metrics by default — each agent host spawns its own
- * process and binding ephemeral ports is operationally useless. Supported
- * scrape target is `gitnexus serve`.
+ * Error messages from MCP tool handlers (notably cypher parse errors) echo
+ * user input, so `observe()` accepts a boolean error detector only — never
+ * the error string. Exporter is configured to suppress target_info /
+ * otel_scope_info so the label set stays bounded to {tool, error, le}.
  */
 
 import type { Counter, Histogram, UpDownCounter } from '@opentelemetry/api';
@@ -30,18 +11,9 @@ import { PrometheusExporter } from '@opentelemetry/exporter-prometheus';
 
 const METER_NAME = 'gitnexus.mcp';
 
-// Quantized buckets. Once published, these become a dashboard contract;
-// boundaries are intentionally coarse so we don't have to recut them.
-//
-// Duration: tools that hit the in-memory graph (`context`, `query` on a small
-// repo) commonly return in 1–30ms; `impact` / `detect_changes` on a large repo
-// can run multiple seconds. 1ms..30s covers cold-cache outliers.
+// Bucket boundaries are a published dashboard contract; do not recut without
+// coordinating with scrape consumers.
 export const DURATION_BUCKETS_SECONDS = [0.001, 0.005, 0.025, 0.1, 0.5, 2.5, 10, 30];
-
-// Result size: tool responses range from a few-hundred-byte error envelopes to
-// multi-megabyte `context`/`cypher` payloads. The modal `context`/`route_map`
-// answer lands in 2–8 KB; boundaries are chosen so that range straddles a
-// boundary rather than collapsing into one bucket.
 export const RESULT_BYTES_BUCKETS = [512, 2048, 8192, 32768, 131072, 524288, 2097152];
 
 let provider: MeterProvider | null = null;
@@ -58,10 +30,6 @@ function envFlag(value: string | undefined): boolean {
 }
 
 export interface InitMetricsOptions {
-  /**
-   * Override the env-driven master switch. Use in tests to force initialization
-   * without setting process env.
-   */
   forceEnabled?: boolean;
 }
 
@@ -72,13 +40,7 @@ export interface InitMetricsResult {
   endpoint?: string;
 }
 
-/**
- * Initialize the OTel meter provider and start the Prometheus exporter HTTP
- * server. No-op when GITNEXUS_OTEL_METRICS is unset, empty, off, false, or 0.
- *
- * Safe to call multiple times — second and later calls return the existing
- * state without re-binding the port.
- */
+/** Idempotent: subsequent calls return the bound state without re-binding. */
 export async function initMetrics(
   opts: InitMetricsOptions = {},
 ): Promise<InitMetricsResult> {
@@ -103,9 +65,8 @@ export async function initMetrics(
     host,
     port,
     endpoint,
-    // Privacy: suppress target_info (would ship every resource attribute as a
-    // separate metric series) and otel_scope_info. Only labels we want to
-    // expose are `tool` and `error`.
+    // target_info would publish every resource attribute as its own series —
+    // keep the label set bounded.
     withoutTargetInfo: true,
     withoutScopeInfo: true,
     appendTimestamp: false,
@@ -131,10 +92,8 @@ export async function initMetrics(
     ],
   });
 
-  // Use the provider directly rather than the global registration. Each
-  // process gets exactly one provider (init is idempotent above); skipping the
-  // global avoids state-leak across vitest fork boundaries when tests cycle
-  // init/shutdown repeatedly.
+  // Provider stays local — global registration would leak state across
+  // repeat init/shutdown cycles.
   const meter = provider.getMeter(METER_NAME);
 
   requestsCounter = meter.createCounter('gitnexus_mcp_tool_requests_total', {
@@ -155,17 +114,11 @@ export async function initMetrics(
     description: 'Currently executing MCP tool handlers, labelled by tool.',
   });
 
-  // Wait until the exporter HTTP server is actually accepting connections so
-  // callers can integration-test the endpoint immediately after init.
   await exporter.startServer();
 
   return { enabled: true, port, host, endpoint };
 }
 
-/**
- * Shut down the meter provider and exporter HTTP server. Safe to call when
- * init was a no-op (resets nothing). Use in graceful-shutdown paths.
- */
 export async function shutdownMetrics(): Promise<void> {
   const p = provider;
   const e = exporter;
@@ -179,21 +132,7 @@ export async function shutdownMetrics(): Promise<void> {
   if (p) await p.shutdown().catch(() => {});
 }
 
-/**
- * Wrap an MCP tool handler with metric instrumentation. Records:
- *   - requests counter (incremented once per call, with `error` label)
- *   - duration histogram (in seconds)
- *   - result bytes histogram (only on success — error-path size is meaningless)
- *   - in-flight up/down counter
- *
- * The `hasError` detector returns a boolean only. The result's error message
- * is intentionally not accepted at this seam — error messages echo user input
- * (cypher parse errors include the offending clause) and are an anonymity
- * leak.
- *
- * No-op when init was disabled: the metric handles are null, calls are
- * skipped, work runs untouched.
- */
+/** hasError must be a boolean; never accept the error string — cypher errors echo user input. */
 export async function observe<T>(
   tool: string,
   fn: () => Promise<T>,
@@ -223,9 +162,6 @@ export async function observe<T>(
     }
     return result;
   } catch (err) {
-    // Thrown errors are converted to error envelopes by the MCP handler upstream,
-    // so this branch is reached only on truly exceptional failures (transport,
-    // sentinel, programming errors). Record as error=true with zero bytes.
     const durationSeconds = Number(process.hrtime.bigint() - start) / 1e9;
     const labels = { tool, error: 'true' };
     requestsCounter?.add(1, labels);
@@ -236,9 +172,6 @@ export async function observe<T>(
   }
 }
 
-/**
- * Returns true if metrics are currently initialized. Test helper.
- */
 export function isMetricsEnabled(): boolean {
   return provider !== null;
 }

gitnexus/src/mcp/server.ts

@@ -163,10 +163,6 @@ export function createMCPServer(backend: LocalBackend): Server {
     })),
   }));
 
-  // Handle tool calls — append next-step hints to guide agent workflow.
-  // The body is wrapped in `observe()` (see `./metrics.ts`) so each call is
-  // recorded against the OTel meter when metrics are enabled. The wrapper is
-  // a no-op when metrics are disabled.
   server.setRequestHandler(CallToolRequestSchema, async (request) => {
     const { name, arguments: args } = request.params;
 
@@ -201,16 +197,12 @@ export function createMCPServer(backend: LocalBackend): Server {
         }
       },
       (result) => {
-        // Size = bytes of the text payload only. The envelope structure
-        // itself is not user data.
         const block = result.content?.[0];
         if (block && 'text' in block && typeof block.text === 'string') {
           return Buffer.byteLength(block.text, 'utf8');
         }
         return 0;
       },
-      // hasError must return a boolean only. Never accept the error message
-      // string at this seam — messages echo user input.
       (result) => result.isError === true,
     );
   });

gitnexus/test/unit/mcp/metrics.test.ts

@@ -1,16 +1,3 @@
-/**
- * Tests for src/mcp/metrics.ts — the OTel + Prometheus seam for issue #1351.
- *
- * Two privacy-critical invariants land here:
- *   1. `observe()` accepts a boolean error detector only, never a message string.
- *      (Type-enforced; tested by passing a result with an explicit message and
- *      asserting the message does not appear in the exposed metrics text.)
- *   2. The Prometheus exporter is configured to suppress `target_info` and
- *      scope info — only `tool` and `error` labels should appear.
- *
- * Lifecycle: each test that turns metrics on must `shutdownMetrics()` in afterEach,
- * otherwise the exporter HTTP server leaks port + handles across tests.
- */
 import { describe, it, expect, beforeEach, afterEach } from 'vitest';
 import {
   initMetrics,
@@ -31,8 +18,7 @@ function clearMetricsEnv(): void {
 }
 
 async function withRandomPort<T>(fn: () => Promise<T>): Promise<T> {
-  // Use port 0 by hand-rolling a free port — Node http will pick one but the
-  // exporter wants a concrete number. Bind/listen/close pattern.
+  // Exporter needs a concrete port number, not 0 — pre-bind to discover one.
   const net = await import('node:net');
   const srv = net.createServer();
   const port = await new Promise<number>((resolve, reject) => {
@@ -113,7 +99,6 @@ describe('observe() — success path', () => {
       expect(text).toContain(
         'gitnexus_mcp_tool_requests_total{tool="list_repos",error="false"} 1',
       );
-      // Histogram is exported by the OTel Prom format with _bucket / _sum / _count.
       expect(text).toContain('gitnexus_mcp_tool_request_duration_seconds_count');
       expect(text).toContain('gitnexus_mcp_tool_result_bytes_count');
     });
@@ -138,7 +123,6 @@ describe('observe() — error path', () => {
 
       const text = await fetch(`http://127.0.0.1:${init.port}/metrics`).then((r) => r.text());
       expect(text).toContain('gitnexus_mcp_tool_requests_total{tool="cypher",error="true"} 1');
-      // PRIVACY INVARIANT: error messages echo user input. They MUST NOT leak.
       expect(text).not.toContain(SECRET_ECHO);
       expect(text).not.toContain('parse error');
     });
@@ -186,8 +170,6 @@ describe('observe() — in-flight balance', () => {
       }
 
       const text = await fetch(`http://127.0.0.1:${init.port}/metrics`).then((r) => r.text());
-      // After all calls complete (regardless of success/failure), inflight returns to 0.
-      // Prom emits the latest value; assert it's a "0" reading for impact.
       expect(text).toMatch(/gitnexus_mcp_tool_inflight\{tool="impact"\} 0\b/);
     });
   });
@@ -208,7 +190,6 @@ describe('privacy: target_info / scope_info suppression', () => {
       const text = await fetch(`http://127.0.0.1:${init.port}/metrics`).then((r) => r.text());
       expect(text).not.toMatch(/^target_info/m);
       expect(text).not.toMatch(/otel_scope_info/);
-      // The exposed label set is bounded: only `tool` and `error` ever appear.
       const labelLines = text.split('\n').filter((l) => l.includes('gitnexus_mcp_'));
       for (const line of labelLines) {
         const match = line.match(/\{([^}]*)\}/);