Skip to content

Conversation

nirinchev
Copy link
Collaborator

Proposed changes

fixes #485
fixes #454

@Copilot Copilot AI review requested due to automatic review settings August 28, 2025 11:56
@nirinchev nirinchev requested a review from a team as a code owner August 28, 2025 11:56
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements handling for MCP setLogLevel requests to dynamically control logging output. The changes address issues where the server could not respond to log level changes from MCP clients.

Key changes:

  • Added log level filtering in McpLogger based on server's configured minimum level
  • Implemented setLogLevel request handler in the Server class
  • Restructured logger initialization to create McpLogger after Server instantiation

Reviewed Changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/server.ts Added setLogLevel request handler and mcpLogLevel property management
src/common/logger.ts Implemented log level filtering in McpLogger and updated constructor
src/transports/base.ts Moved McpLogger creation after Server instantiation to fix dependency
tests/unit/logger.test.ts Added test coverage for log level filtering functionality
package.json Updated @modelcontextprotocol/inspector dependency version
.github/workflows/check.yml Updated inspector version in CI workflow

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +276 to +278
const minimumLevel = McpLogger.LOG_LEVELS.indexOf(this.server.mcpLogLevel);
const currentLevel = McpLogger.LOG_LEVELS.indexOf(level);
if (minimumLevel > currentLevel) {
Copy link

Copilot AI Aug 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comparison logic is inverted. If minimumLevel > currentLevel, it means the current level is actually lower priority than the minimum. The condition should be minimumLevel > currentLevel to filter out lower priority levels, but the comment says 'Don't log if the requested level is lower than the minimum level' which suggests the opposite logic. Please verify the intended behavior matches the LOG_LEVELS array ordering.

Copilot uses AI. Check for mistakes.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks wrong!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong as the copilot suggestion is wrong or the code itself?

Comment on lines +276 to +277
const minimumLevel = McpLogger.LOG_LEVELS.indexOf(this.server.mcpLogLevel);
const currentLevel = McpLogger.LOG_LEVELS.indexOf(level);
Copy link

Copilot AI Aug 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No validation for invalid log levels. If this.server.mcpLogLevel or level contains an invalid value not in LOG_LEVELS, indexOf() will return -1, leading to incorrect filtering behavior. Add validation to handle invalid log levels gracefully.

Suggested change
const minimumLevel = McpLogger.LOG_LEVELS.indexOf(this.server.mcpLogLevel);
const currentLevel = McpLogger.LOG_LEVELS.indexOf(level);
const currentLevel = McpLogger.LOG_LEVELS.indexOf(level);
if (minimumLevel === -1) {
// Invalid minimum log level configured; skip logging and optionally warn
console.warn(`[McpLogger] Invalid minimum log level: ${this.server.mcpLogLevel}`);
return;
}
if (currentLevel === -1) {
// Invalid log level requested; skip logging and optionally warn
console.warn(`[McpLogger] Invalid log level: ${level}`);
return;
}

Copilot uses AI. Check for mistakes.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a valid one.

Copy link
Collaborator

@himanshusinghs himanshusinghs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The core logic looks good! The copilot suggestion, might be something to look into.

Comment on lines +276 to +277
const minimumLevel = McpLogger.LOG_LEVELS.indexOf(this.server.mcpLogLevel);
const currentLevel = McpLogger.LOG_LEVELS.indexOf(level);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a valid one.

Comment on lines +276 to +278
const minimumLevel = McpLogger.LOG_LEVELS.indexOf(this.server.mcpLogLevel);
const currentLevel = McpLogger.LOG_LEVELS.indexOf(level);
if (minimumLevel > currentLevel) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks wrong!

@kmruiz kmruiz enabled auto-merge (squash) August 28, 2025 13:20
@kmruiz kmruiz merged commit cd5cf37 into main Aug 28, 2025
17 of 22 checks passed
@kmruiz kmruiz deleted the ni/logging branch August 28, 2025 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

4 participants