From 8ffe36162332a66f65401b28619a53d7099e8d66 Mon Sep 17 00:00:00 2001 From: saravanakumardb1 Date: Sat, 23 May 2026 19:09:32 -0700 Subject: [PATCH] feat(mcp-client): TODO-3 \u2014 expose pluggable logger via McpLogger interface Previously the @bytelyst/mcp-client package logged directly to the global `console`, which made its output invisible to consumers running under Fastify/pino or any structured logger. The scanner exempted the whole package for console-log findings with a TODO-3 marker; this commit resolves the marker. packages/mcp-client/src/index.ts: + Added `McpLogger` interface (debug/info/warn/error, variadic) which is structurally compatible with the global console, pino, and Fastify's `request.log`. + Added optional `logger?: McpLogger` field on MCPConfig with a JSDoc explaining when consumers should supply their own. + MCPClient now stores a `private readonly log: McpLogger` field initialised from `config.logger ?? console` in the constructor. + All 17 internal logging sites switched from `console.X(...)` to `this.log.X(...)`. Mapping: console.log \u2192 this.log.info (pino does not have a 'log' method). scripts/check-rule-violations.sh: - Removed the blanket /packages/mcp-client/ exemption from the console-log scanner (TODO-3 marker comment retained for history). - The ts-any-type exemption stays \u2014 mcp-client still uses `any` at the JSON-RPC payload boundary (different concern). Verification: packages/mcp-client \u2192 `pnpm build` clean (tsc). `bash scripts/check-rule-violations.sh` \u2192 total still 88, no new console-log findings (mcp-client is now genuinely clean instead of blanket-exempted). --- packages/mcp-client/src/index.ts | 62 +++++++++++++++++++++++--------- scripts/check-rule-violations.sh | 13 ++++--- 2 files changed, 51 insertions(+), 24 deletions(-) diff --git a/packages/mcp-client/src/index.ts b/packages/mcp-client/src/index.ts index 1eef53ab..6d295bfe 100644 --- a/packages/mcp-client/src/index.ts +++ b/packages/mcp-client/src/index.ts @@ -8,6 +8,29 @@ export interface MCPConfig { timeoutMs: number; cacheTtlSec: number; maxRetries: number; + /** + * Optional logger override. When omitted, the client logs to the global + * `console` (matching the previous behaviour). Consumers in a Fastify / + * pino / structlog context should pass their own logger so MCP output is + * captured alongside the rest of the service's structured logs. + * + * Each method is invoked with a message string followed by zero or more + * structured arguments, mirroring the standard Console API. + */ + logger?: McpLogger; +} + +/** + * Minimal logger interface consumed by `MCPClient`. Compatible with `console` + * (which is the default), pino, Fastify's `request.log`, and any other + * logger that exposes `debug` / `info` / `warn` / `error` methods accepting + * variadic arguments. + */ +export interface McpLogger { + debug(message: string, ...args: unknown[]): void; + info(message: string, ...args: unknown[]): void; + warn(message: string, ...args: unknown[]): void; + error(message: string, ...args: unknown[]): void; } export interface Tool { @@ -34,21 +57,26 @@ export class MCPClient { private cache: Map = new Map(); private rateLimitMap: Map = new Map(); private config: MCPConfig; + private readonly log: McpLogger; private readonly RATE_LIMIT_WINDOW = 60000; // 1 minute window private readonly RATE_LIMIT_MAX_REQUESTS = 200; // Max 200 requests per minute constructor(config: MCPConfig) { this.config = config; + // Default to console when no logger is provided. The `console` global + // already implements the McpLogger interface (debug/info/warn/error + // are all variadic), so no shim is required. + this.log = config.logger ?? console; } async connect(): Promise { if (this.isConnected) { - console.warn('[MCP] Already connected'); + this.log.warn('[MCP] Already connected'); return; } try { - console.log( + this.log.info( `[MCP] Connecting to server via ${this.config.serverCommand} ${this.config.serverArgs}` ); @@ -73,9 +101,9 @@ export class MCPClient { await this.client.connect(this.transport); this.isConnected = true; - console.log('[MCP] Successfully connected to MCP server'); + this.log.info('[MCP] Successfully connected to MCP server'); } catch (error: any) { - console.error(`[MCP] Failed to connect: ${error.message}`); + this.log.error(`[MCP] Failed to connect: ${error.message}`); this.isConnected = false; throw error; } @@ -99,9 +127,9 @@ export class MCPClient { this.isConnected = false; this.client = null; this.transport = null; - console.log('[MCP] Disconnected from MCP server'); + this.log.info('[MCP] Disconnected from MCP server'); } catch (error: any) { - console.error(`[MCP] Error during disconnect: ${error.message}`); + this.log.error(`[MCP] Error during disconnect: ${error.message}`); this.isConnected = false; } } @@ -121,14 +149,14 @@ export class MCPClient { const cacheKey = this.getCacheKey(toolName, args); const cached = this.getFromCache(cacheKey); if (cached !== null) { - console.debug(`[MCP] Cache hit for ${toolName}`); + this.log.debug(`[MCP] Cache hit for ${toolName}`); return cached; } let lastError: Error | null = null; for (let attempt = 0; attempt < this.config.maxRetries; attempt++) { try { - console.debug( + this.log.debug( `[MCP] Calling tool ${toolName} (attempt ${attempt + 1}/${this.config.maxRetries})` ); @@ -152,14 +180,14 @@ export class MCPClient { // Audit log successful tool execution this.auditLogToolSuccess(toolName); - console.log(`[MCP] Tool ${toolName} executed successfully`); + this.log.info(`[MCP] Tool ${toolName} executed successfully`); return data; } throw new Error(`Empty response from tool ${toolName}`); } catch (error: any) { lastError = error; - console.warn(`[MCP] Tool ${toolName} attempt ${attempt + 1} failed: ${error.message}`); + this.log.warn(`[MCP] Tool ${toolName} attempt ${attempt + 1} failed: ${error.message}`); // Audit log failed tool execution this.auditLogToolFailure(toolName, error.message); @@ -176,7 +204,7 @@ export class MCPClient { } } - console.error(`[MCP] Tool ${toolName} failed after ${this.config.maxRetries} attempts`); + this.log.error(`[MCP] Tool ${toolName} failed after ${this.config.maxRetries} attempts`); throw lastError || new Error(`Tool ${toolName} failed`); } @@ -193,7 +221,7 @@ export class MCPClient { inputSchema: tool.inputSchema, })); } catch (error: any) { - console.error(`[MCP] Failed to list tools: ${error.message}`); + this.log.error(`[MCP] Failed to list tools: ${error.message}`); throw error; } } @@ -251,7 +279,7 @@ export class MCPClient { private clearCache(): void { this.cache.clear(); - console.debug('[MCP] Cache cleared'); + this.log.debug('[MCP] Cache cleared'); } private isReadOperation(toolName: string): boolean { @@ -299,7 +327,7 @@ export class MCPClient { await this.listTools(); return true; } catch (error) { - console.warn(`[MCP] Health check failed: ${error}`); + this.log.warn(`[MCP] Health check failed: ${error}`); return false; } } @@ -318,15 +346,15 @@ export class MCPClient { private auditLogToolCall(toolName: string, args: any): void { // Security: Sanitize sensitive parameters before logging const sanitizedArgs = this.sanitizeArgs(args); - console.log(`[MCP-AUDIT] Tool called: ${toolName}, Args: ${JSON.stringify(sanitizedArgs)}`); + this.log.info(`[MCP-AUDIT] Tool called: ${toolName}, Args: ${JSON.stringify(sanitizedArgs)}`); } private auditLogToolSuccess(toolName: string): void { - console.log(`[MCP-AUDIT] Tool succeeded: ${toolName}`); + this.log.info(`[MCP-AUDIT] Tool succeeded: ${toolName}`); } private auditLogToolFailure(toolName: string, error: string): void { - console.error(`[MCP-AUDIT] Tool failed: ${toolName}, Error: ${error}`); + this.log.error(`[MCP-AUDIT] Tool failed: ${toolName}, Error: ${error}`); } private sanitizeArgs(args: any): any { diff --git a/scripts/check-rule-violations.sh b/scripts/check-rule-violations.sh index 815751dd..d28680cc 100644 --- a/scripts/check-rule-violations.sh +++ b/scripts/check-rule-violations.sh @@ -174,11 +174,9 @@ scan_b4_console_log() { # Skip plugins/ \u2014 Tauri / Expo / Cowork plugins are CLI-like build tools # that emit progress to stdout when invoked by their host runtime. [[ "$file" =~ (^|/)plugins/ ]] && continue - # Skip MCP client library \u2014 console-based debug logging is the standard - # pattern for client SDKs without an injected logger interface. - # TODO-3: expose a `logger` callback option on McpClient so consumers - # can plumb their own logger (pino, structlog-via-wasm, etc.). - [[ "$file" =~ /packages/mcp-client/ ]] && continue + # (TODO-3 resolved as of mcp-client commit: package now exposes a + # `logger` callback on McpClient and uses `this.log.*` everywhere, + # so no special-case exemption is needed for the console-log rule.) # Skip the logger package itself (packages/logger) — console.log IS the # implementation when the user-provided logger is not configured. [[ "$file" =~ /packages/logger/ ]] && continue @@ -256,8 +254,9 @@ scan_ts_any_type() { # Skip mac_tooling — standalone forensics toolkit; not ByteLyst-style TS # (matches hex / python-print exemptions per its own AGENTS.md). [[ "$repo" == "learning_ai_mac_tooling" ]] && continue - # Skip the MCP client SDK — it parses arbitrary JSON-RPC payloads from - # untrusted servers and intentionally uses `any` at the boundary. + # MCP client SDK still uses `any` for JSON-RPC payloads at the + # untrusted-server boundary. Keep exempt for ts-any-type (this is a + # separate concern from the logger refactor in TODO-3). [[ "$file" =~ /packages/mcp-client/ ]] && continue # Skip lines preceded by `// eslint-disable-next-line @typescript-eslint/no-explicit-any` # or the broader `// @ts-ignore` / `// @ts-expect-error` opt-outs.