Logging updates (including reported SonicBoom error)

[BobDickinson]

Summary

Made logging consistent between all clients (using MCP_LOG_FILE), updated client readmes. Made logging initialization async so we have logging ready before app startup processing or early exit (this addresses the SonicBoom errors seen on early app termination). Reorganized shared logging code (which is node code) into core/logging/node. Fixed CLI error reporting through launcher (just error message now, no stack trace). Made logger required throughout using silentLogger (null logger) as default.

Testing

• Tested in UI mode
• Tested in CLI mode
• Manual testing performed

Checklist

• Code follows the style guidelines (ran npm run prettier-fix)
• Self-review completed
• Code is commented where necessary
• Documentation updated (README, comments, etc.)

[BobDickinson]

@claude review

[claude]

Claude finished @BobDickinson's task in 1m 35s —— View job

---

Code Review

• Gather context and understand the PR
• Read changed files
• Analyze code for issues
• Post review feedback

---

Overall this is a well-structured improvement. The core fix (awaiting the SonicBoom ready event before using the logger) is correct and addresses the reported issue cleanly. The refactoring to centralize file logger creation and make loggers required (non-optional) is a good step forward. A few things worth flagging:

Issues

1. Browser components importing from logging/node path

AuthDebugger.tsx and OAuthCallback.tsx are React components that run in the browser. They now import silentLogger from @modelcontextprotocol/inspector-core/logging/node:

// clients/web/src/components/AuthDebugger.tsx
import { silentLogger } from "@modelcontextprotocol/inspector-core/logging/node";

The logging/node barrel re-exports both silentLogger (browser-safe) and createFileLogger (Node.js-specific — uses pino.destination() → SonicBoom → fs). Vite's tree-shaking should prevent fileLogger.ts from being bundled, but semantically importing Node.js-specific paths in browser code is fragile: any future addition to logging/node/index.ts that has side effects or isn't tree-shakeable could silently break the browser bundle.

The old core/logging/index.ts provided a clean browser-safe re-export point for silentLogger. Deleting it and pointing browser components at /node loses that separation. A better approach would be restoring a /logging or /logging/browser export that only contains browser-safe utilities, and using that from the React components.

Fix this →

2. Missing logger name in web.ts

cli.ts passes name: "mcp-inspector-cli" to createFileLogger, but web.ts (inside runWeb()) does not:

// clients/web/src/web.ts:260
const logger = process.env.MCP_LOG_FILE
  ? await createFileLogger({
      dest: process.env.MCP_LOG_FILE,
      append: true,
      mkdir: true,
      level: "info",
      // missing: name: "mcp-inspector-web"
    })
  : silentLogger;

Log lines from the web runner won't have a name field, making it harder to distinguish sources when multiple clients log to the same file.

Fix this →

Minor Observations

honoMiddlewarePlugin accepting Promise<WebServerConfig> — The WebServerConfig | Promise<WebServerConfig> union type is a pragmatic workaround for the async config issue, and it works. An alternative would be an async factory function createHonoPlugin(config: Promise<WebServerConfig>): Promise<Plugin>, but the current approach avoids changing callers so it's reasonable.

tuiLogger mutable export — export let tuiLogger: Logger = silentLogger with reassignment inside initTuiLogger() relies on ES module live bindings for correctness (consumers re-reading tuiLogger after init). This works, but any module that destructures the import early (e.g. const { tuiLogger } = await import(...)) would miss the update. Worth a note in the file that it must be referenced by name, not cached.

createFileLogger error listener cleanup — Both once("ready", resolve) and once("error", reject) are properly using once so they auto-clean up after firing. No leak.

What Works Well

• The ready-event await in createFileLogger is the correct fix for the SonicBoom flush race on early exit.
• Making logger: Logger required (non-optional) with silentLogger as the default is cleaner than checking logger !== undefined everywhere.
• Unifying all three clients to use MCP_LOG_FILE is a good consistency improvement.
• The async logger initialization pattern (init before any work that could process.exit()) is exactly right.

---

[BobDickinson]

Claude review feedback implemented.

[cliffhall]

@BobDickinson is this one ready for review?

[BobDickinson]

@BobDickinson is this one ready for review?

Yes, ready for review.

[Copilot]

Pull request overview

This PR standardizes logging across the Inspector clients by introducing explicit Node vs browser logging entrypoints in @modelcontextprotocol/inspector-core, adding an async file logger initializer to avoid early-exit SonicBoom issues, and updating clients/docs to use MCP_LOG_FILE consistently.

Changes:

• Split core logging into logging/node and logging/browser exports (and updated imports accordingly).
• Added createFileLogger() (async, awaits destination readiness) and adopted it in CLI/Web/TUI when MCP_LOG_FILE is set.
• Updated web dev/standalone bootstrapping to support async config creation and adjusted CLI error handling / readmes.

Reviewed changes

Copilot reviewed 18 out of 20 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
core/package.json	Replaces generic ./logging export with explicit ./logging/node and ./logging/browser subpath exports.
core/mcp/inspectorClient.ts	Switches default logger import to the Node logging module.
core/logging/node/logger.ts	Introduces Node silentLogger implementation.
core/logging/node/index.ts	Exports Node logging utilities (silentLogger, createFileLogger).
core/logging/node/fileLogger.ts	Adds async createFileLogger() that waits for destination readiness.
core/logging/browser/logger.ts	Introduces browser silentLogger implementation.
core/logging/browser/index.ts	Exports browser silentLogger.
core/auth/index.ts	Re-exports silentLogger from Node logging.
clients/web/src/web.ts	Creates a file logger from MCP_LOG_FILE and injects it into the web server config.
clients/web/src/web-server-config.ts	Makes logger required and makes env-based config builder async to await file logger readiness.
clients/web/src/vite-hono-plugin.ts	Accepts `WebServerConfig
clients/web/src/server.ts	Awaits async env config builder; logs startup.
clients/web/src/components/OAuthCallback.tsx	Switches to browser logging import to avoid pulling Node logging into the UI bundle.
clients/web/src/components/AuthDebugger.tsx	Switches to browser logging import to avoid pulling Node logging into the UI bundle.
clients/web/README.md	Documents that MCP_LOG_FILE captures both server and InspectorClient logs.
clients/tui/tui.tsx	Initializes TUI logging asynchronously before startup work.
clients/tui/src/logger.ts	Replaces hardcoded ~/.mcp-inspector/auth.log logger with MCP_LOG_FILE + async init.
clients/tui/README.md	Documents MCP_LOG_FILE for TUI logging.
clients/cli/src/cli.ts	Uses MCP_LOG_FILE + async file logger; wraps runCli in error handler.
clients/cli/README.md	Documents MCP_LOG_FILE for CLI logging.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

core/auth/index.ts is part of the package’s default (isomorphic) auth entrypoint (exports["./auth"]). Re-exporting silentLogger from ../logging/node makes every import of @modelcontextprotocol/inspector-core/auth / auth/* pull in the Node logging module (and its pino Node entry), which can break browser bundling even when silentLogger is not used. Consider removing this re-export from the shared auth index and instead exporting it from auth/node (and/or auth/browser) or requiring callers to import from logging/node vs logging/browser directly.

Suggested change

	// Logging (re-exported from core/logging/node)
	export { silentLogger } from "../logging/node/index.js";

[Copilot]

In createFileLogger, the dest.once("error", reject) listener remains attached after the Promise resolves on ready. If the destination emits an error later (disk full, permission change, etc.), that error will be effectively swallowed (the Promise is already resolved), and the presence of the listener may prevent the process from surfacing the failure. Consider removing the error listener once ready fires, or attaching a persistent error handler that reports/propagates errors in a way callers can observe.

Suggested change

	dest.once("ready", resolve);
	dest.once("error", reject);
	const handleReady = () => {
	dest.off("error", handleError);
	resolve();
	};
	const handleError = (error: Error) => {
	dest.off("ready", handleReady);
	reject(error);
	};
	dest.once("ready", handleReady);
	dest.once("error", handleError);

[Copilot]

createFileLogger introduces new async behavior (waiting for the destination ready event) and error handling around the destination stream, but there are no tests validating that it actually waits for readiness and produces a writable logger (or that errors are handled as expected). Adding a small vitest that creates a temp log file, awaits createFileLogger, writes a log line, flushes/closes, and asserts the file contains output would help prevent regressions (and would specifically cover the SonicBoom early-exit scenario this PR is addressing).